Add get_all capability to volume_attachments

One of the useful things that was missing from the
volume_attachments code was get_all methods.

This patch adds a get_all and a get_all_by_project, it
also goes ahead and adds some filtering capability to
the existing get_by_xxxx calls since we added the framework
for it in the get_all additions.

I also looked at refactoring our db methods for attach to just:
  * attach_create
  * attach_update
  * attach_destroy
  * attach_get
  * attach_get_all

This would probably be good as an independent effort to
clean things up and bring these calls more in line with
others, but there's a lot of work to update the objects
and existing code, might be better to wait until after
implementing the new attach API.

Co-Authored-By: Michał Dulko <michal.dulko@intel.com>

Change-Id: I40614fe702f726c74ff05f93faaf6ee79253447f
This commit is contained in:
John Griffith 2016-10-11 14:34:54 -06:00 committed by john.griffith8@gmail.com
parent f568d09543
commit 3f930bb10d
8 changed files with 243 additions and 43 deletions

View File

@ -336,24 +336,44 @@ def volume_attachment_update(context, attachment_id, values):
return IMPL.volume_attachment_update(context, attachment_id, values)
def volume_attachment_get(context, attachment_id, session=None):
return IMPL.volume_attachment_get(context, attachment_id, session)
def volume_attachment_get(context, attachment_id):
return IMPL.volume_attachment_get(context, attachment_id)
def volume_attachment_get_all_by_volume_id(context, volume_id):
return IMPL.volume_attachment_get_all_by_volume_id(context, volume_id)
def volume_attachment_get_all_by_volume_id(context, volume_id,
session=None):
return IMPL.volume_attachment_get_all_by_volume_id(context,
volume_id,
session)
def volume_attachment_get_all_by_host(context, host):
def volume_attachment_get_all_by_host(context, host, filters=None):
# FIXME(jdg): Not using filters
return IMPL.volume_attachment_get_all_by_host(context, host)
def volume_attachment_get_all_by_instance_uuid(context,
instance_uuid):
instance_uuid, filters=None):
# FIXME(jdg): Not using filters
return IMPL.volume_attachment_get_all_by_instance_uuid(context,
instance_uuid)
def volume_attachment_get_all(context, filters=None, marker=None, limit=None,
offset=None, sort_keys=None, sort_dirs=None):
return IMPL.volume_attachment_get_all(context, filters, marker, limit,
offset, sort_keys, sort_dirs)
def volume_attachment_get_all_by_project(context, project_id, filters=None,
marker=None, limit=None, offset=None,
sort_keys=None, sort_dirs=None):
return IMPL.volume_attachment_get_all_by_project(context, project_id,
filters, marker, limit,
offset, sort_keys,
sort_dirs)
def volume_update_status_based_on_attachment(context, volume_id):
"""Update volume status according to attached instance id"""
return IMPL.volume_update_status_based_on_attachment(context, volume_id)

View File

@ -257,7 +257,7 @@ def handle_db_data_error(f):
return wrapper
def model_query(context, *args, **kwargs):
def model_query(context, model, *args, **kwargs):
"""Query helper that accounts for context's `read_deleted` field.
:param context: context to query under
@ -270,7 +270,7 @@ def model_query(context, *args, **kwargs):
read_deleted = kwargs.get('read_deleted') or context.read_deleted
project_only = kwargs.get('project_only')
query = session.query(*args)
query = session.query(model, *args)
if read_deleted == 'no':
query = query.filter_by(deleted=False)
@ -285,7 +285,13 @@ def model_query(context, *args, **kwargs):
_("Unrecognized read_deleted value '%s'") % read_deleted)
if project_only and is_user_context(context):
query = query.filter_by(project_id=context.project_id)
if model == models.VolumeAttachment:
# NOTE(dulek): In case of VolumeAttachment, we need to join
# `project_id` through `volume` relationship.
query = query.filter(models.Volume.project_id ==
context.project_id)
else:
query = query.filter_by(project_id=context.project_id)
return query
@ -1379,8 +1385,8 @@ def volume_attach(context, values):
session = get_session()
with session.begin():
volume_attachment_ref.save(session=session)
return volume_attachment_get(context, values['id'],
session=session)
return _attachment_get(context, values['id'],
session=session)
@require_admin_context
@ -1398,8 +1404,8 @@ def volume_attached(context, attachment_id, instance_uuid, host_name,
session = get_session()
with session.begin():
volume_attachment_ref = volume_attachment_get(context, attachment_id,
session=session)
volume_attachment_ref = _attachment_get(context, attachment_id,
session=session)
updated_values = {'mountpoint': mountpoint,
'attach_status': fields.VolumeAttachStatus.ATTACHED,
@ -1606,11 +1612,17 @@ def volume_detached(context, volume_id, attachment_id):
if this was the last detachment made.
"""
# NOTE(jdg): This is a funky band-aid for the earlier attempts at
# multiattach, it's a bummer because these things aren't really being used
# but at the same time we don't want to break them until we work out the
# new proposal for multi-attach
remain_attachment = True
session = get_session()
with session.begin():
try:
attachment = volume_attachment_get(context, attachment_id,
session=session)
attachment = _attachment_get(context, attachment_id,
session=session)
except exception.VolumeAttachmentNotFound:
attachment_updates = None
attachment = None
@ -1629,10 +1641,20 @@ def volume_detached(context, volume_id, attachment_id):
attachment.save(session=session)
del attachment_updates['updated_at']
attachment_list = None
volume_ref = _volume_get(context, volume_id,
session=session)
volume_updates = {'updated_at': literal_column('updated_at')}
if not volume_ref.volume_attachment:
# NOTE(jdg): We kept the old arg style allowing session exclusively
# for this one call
attachment_list = volume_attachment_get_all_by_volume_id(
context, volume_id, session=session)
remain_attachment = False
if attachment_list and len(attachment_list) > 0:
remain_attachment = True
if not remain_attachment:
# Hide status update from user if we're performing volume migration
# or uploading it to image
if ((not volume_ref.migration_status and
@ -1706,25 +1728,73 @@ def _volume_get(context, volume_id, session=None, joined_load=True):
return result
@require_context
def volume_attachment_get(context, attachment_id, session=None):
result = model_query(context, models.VolumeAttachment,
session=session).\
filter_by(id=attachment_id).\
first()
def _attachment_get_all(context, filters=None, marker=None, limit=None,
offset=None, sort_keys=None, sort_dirs=None):
project_id = filters.pop('project_id', None)
if filters and not is_valid_model_filters(models.VolumeAttachment,
filters):
return []
session = get_session()
with session.begin():
# Generate the paginate query
query = _generate_paginate_query(context, session, marker,
limit, sort_keys, sort_dirs, filters,
offset, models.VolumeAttachment)
if query is None:
return []
query = query.options(joinedload('volume'))
if project_id:
query = query.filter(models.Volume.project_id == project_id)
return query.all()
def _attachment_get(context, attachment_id, session=None, read_deleted=False,
project_only=True):
result = (model_query(context, models.VolumeAttachment, session=session,
read_deleted=read_deleted)
.filter_by(id=attachment_id)
.options(joinedload('volume'))
.first())
if not result:
raise exception.VolumeAttachmentNotFound(filter='attachment_id = %s' %
attachment_id)
msg = _("Unable to find attachment with id: %s"), attachment_id
raise exception.VolumeAttachmentNotFound(msg)
return result
def _attachment_get_query(context, session=None, project_only=False):
return model_query(context, models.VolumeAttachment, session=session,
project_only=project_only).options(joinedload('volume'))
def _process_attachment_filters(query, filters):
if filters:
# Ensure that filters' keys exist on the model
if not is_valid_model_filters(models.VolumeAttachment, filters):
return
query = query.filter_by(**filters)
return query
@require_admin_context
def volume_attachment_get_all(context, filters=None, marker=None, limit=None,
offset=None, sort_keys=None, sort_dirs=None):
"""Retrieve all Attachment records with filter and pagination options."""
return _attachment_get_all(context, filters, marker, limit, offset,
sort_keys, sort_dirs)
@require_context
def volume_attachment_get_all_by_volume_id(context, volume_id, session=None):
result = model_query(context, models.VolumeAttachment,
session=session).\
filter_by(volume_id=volume_id).\
filter(models.VolumeAttachment.attach_status !=
fields.VolumeAttachStatus.DETACHED).\
fields.VolumeAttachStatus.DETACHED). \
options(joinedload('volume')).\
all()
return result
@ -1737,11 +1807,17 @@ def volume_attachment_get_all_by_host(context, host):
session=session).\
filter_by(attached_host=host).\
filter(models.VolumeAttachment.attach_status !=
fields.VolumeAttachStatus.DETACHED).\
fields.VolumeAttachStatus.DETACHED). \
options(joinedload('volume')).\
all()
return result
@require_context
def volume_attachment_get(context, attachment_id):
return _attachment_get(context, attachment_id)
@require_context
def volume_attachment_get_all_by_instance_uuid(context,
instance_uuid):
@ -1752,10 +1828,29 @@ def volume_attachment_get_all_by_instance_uuid(context,
filter_by(instance_uuid=instance_uuid).\
filter(models.VolumeAttachment.attach_status !=
fields.VolumeAttachStatus.DETACHED).\
options(joinedload('volume')).\
all()
return result
@require_context
def volume_attachment_get_all_by_project(context, project_id, filters=None,
marker=None, limit=None, offset=None,
sort_keys=None, sort_dirs=None):
"""Retrieve all Attachment records for specific project."""
authorize_project_context(context, project_id)
if not filters:
filters = {}
else:
filters = filters.copy()
filters['project_id'] = project_id
return _attachment_get_all(context, filters, marker,
limit, offset, sort_keys,
sort_dirs)
@require_context
def volume_get(context, volume_id):
return _volume_get(context, volume_id)
@ -2226,8 +2321,8 @@ def volumes_update(context, values_list):
def volume_attachment_update(context, attachment_id, values):
session = get_session()
with session.begin():
volume_attachment_ref = volume_attachment_get(context, attachment_id,
session=session)
volume_attachment_ref = _attachment_get(context, attachment_id,
session=session)
volume_attachment_ref.update(values)
volume_attachment_ref.save(session=session)
return volume_attachment_ref
@ -6023,6 +6118,9 @@ PAGINATION_HELPERS = {
models.Group: (_groups_get_query,
_process_groups_filters,
_group_get),
models.VolumeAttachment: (_attachment_get_query,
_process_attachment_filters,
_attachment_get),
}

View File

@ -121,6 +121,7 @@ OBJ_VERSIONS.add('1.13', {'CleanupRequest': '1.0'})
OBJ_VERSIONS.add('1.14', {'VolumeAttachmentList': '1.1'})
OBJ_VERSIONS.add('1.15', {'Volume': '1.6', 'Snapshot': '1.2'})
OBJ_VERSIONS.add('1.16', {'BackupDeviceInfo': '1.0'})
OBJ_VERSIONS.add('1.17', {'VolumeAttachment': '1.1'})
class CinderObjectRegistry(base.VersionedObjectRegistry):

View File

@ -27,7 +27,11 @@ class VolumeAttachment(base.CinderPersistentObject, base.CinderObject,
base.CinderObjectDictCompat,
base.CinderComparableObject):
# Version 1.0: Initial version
VERSION = '1.0'
# Version 1.1: Added volume relationship
VERSION = '1.1'
OPTIONAL_FIELDS = ['volume']
obj_extra_fields = ['project_id', 'volume_host']
fields = {
'id': fields.UUIDField(),
@ -41,23 +45,68 @@ class VolumeAttachment(base.CinderPersistentObject, base.CinderObject,
'attach_status': c_fields.VolumeAttachStatusField(nullable=True),
'attach_mode': fields.StringField(nullable=True),
'volume': fields.ObjectField('Volume', nullable=False),
}
@staticmethod
def _from_db_object(context, attachment, db_attachment):
@property
def project_id(self):
return self.volume.project_id
@property
def volume_host(self):
return self.volume.host
@classmethod
def _get_expected_attrs(cls, context, *args, **kwargs):
return ['volume']
@classmethod
def _from_db_object(cls, context, attachment, db_attachment,
expected_attrs=None):
if expected_attrs is None:
expected_attrs = cls._get_expected_attrs(context)
for name, field in attachment.fields.items():
if name in cls.OPTIONAL_FIELDS:
continue
value = db_attachment.get(name)
if isinstance(field, fields.IntegerField):
value = value or 0
attachment[name] = value
if 'volume' in expected_attrs:
db_volume = db_attachment.get('volume')
if db_volume:
attachment.volume = objects.Volume._from_db_object(
context, objects.Volume(), db_volume)
attachment._context = context
attachment.obj_reset_changes()
return attachment
def obj_load_attr(self, attrname):
if attrname not in self.OPTIONAL_FIELDS:
raise exception.ObjectActionError(
action='obj_load_attr',
reason=_('attribute %s not lazy-loadable') % attrname)
if not self._context:
raise exception.OrphanedObjectError(method='obj_load_attr',
objtype=self.obj_name())
if attrname == 'volume':
volume = objects.Volume.get_by_id(self._context, self.id)
self.volume = volume
self.obj_reset_changes(fields=[attrname])
def save(self):
updates = self.cinder_obj_get_changes()
if updates:
if 'volume' in updates:
raise exception.ObjectActionError(action='save',
reason=_('volume changed'))
db.volume_attachment_update(self._context, self.id, updates)
self.obj_reset_changes()
@ -104,15 +153,37 @@ class VolumeAttachmentList(base.ObjectListBase, base.CinderObject):
attachments)
@classmethod
def get_all_by_host(cls, context, host):
def get_all_by_host(cls, context, host, search_opts=None):
attachments = db.volume_attachment_get_all_by_host(context,
host)
host,
search_opts)
return base.obj_make_list(context, cls(context),
objects.VolumeAttachment, attachments)
@classmethod
def get_all_by_instance_uuid(cls, context, instance_uuid):
def get_all_by_instance_uuid(cls, context,
instance_uuid, search_opts=None):
attachments = db.volume_attachment_get_all_by_instance_uuid(
context, instance_uuid)
context, instance_uuid, search_opts)
return base.obj_make_list(context, cls(context),
objects.VolumeAttachment, attachments)
@classmethod
def get_all(cls, context, search_opts=None,
marker=None, limit=None, offset=None,
sort_keys=None, sort_direction=None):
attachments = db.volume_attachment_get_all(
context, search_opts, marker, limit, offset, sort_keys,
sort_direction)
return base.obj_make_list(context, cls(context),
objects.VolumeAttachment, attachments)
@classmethod
def get_all_by_project(cls, context, project_id, search_opts=None,
marker=None, limit=None, offset=None,
sort_keys=None, sort_direction=None):
attachments = db.volume_attachment_get_all_by_project(
context, project_id, search_opts, marker, limit, offset, sort_keys,
sort_direction)
return base.obj_make_list(context, cls(context),
objects.VolumeAttachment, attachments)

View File

@ -81,6 +81,7 @@ def fake_db_volume_attachment(**updates):
db_volume_attachment = {
'id': fake.ATTACHMENT_ID,
'volume_id': fake.VOLUME_ID,
'volume': fake_db_volume(),
}
for name, field in objects.VolumeAttachment.fields.items():

View File

@ -43,7 +43,7 @@ object_data = {
'SnapshotList': '1.0-15ecf022a68ddbb8c2a6739cfc9f8f5e',
'Volume': '1.6-8a56256db74c0642dca1a30739d88074',
'VolumeList': '1.1-15ecf022a68ddbb8c2a6739cfc9f8f5e',
'VolumeAttachment': '1.0-6a2216211f579ffd7fd22708703c13a3',
'VolumeAttachment': '1.1-e98b04a372a303b01bedab1e47ee9f6d',
'VolumeAttachmentList': '1.1-15ecf022a68ddbb8c2a6739cfc9f8f5e',
'VolumeProperties': '1.1-cadac86b2bdc11eb79d1dcea988ff9e8',
'VolumeType': '1.3-a5d8c3473db9bc3bbcdbab9313acf4d1',

View File

@ -27,10 +27,11 @@ class TestVolumeAttachment(test_objects.BaseObjectsTestCase):
@mock.patch('cinder.db.sqlalchemy.api.volume_attachment_get')
def test_get_by_id(self, volume_attachment_get):
db_attachment = fake_volume.fake_db_volume_attachment()
attachment_obj = fake_volume.fake_volume_attachment_obj(self.context)
volume_attachment_get.return_value = db_attachment
attachment = objects.VolumeAttachment.get_by_id(self.context,
fake.ATTACHMENT_ID)
self._compare(self, db_attachment, attachment)
self._compare(self, attachment_obj, attachment)
@mock.patch('cinder.db.volume_attachment_update')
def test_save(self, volume_attachment_update):
@ -44,20 +45,23 @@ class TestVolumeAttachment(test_objects.BaseObjectsTestCase):
@mock.patch('cinder.db.sqlalchemy.api.volume_attachment_get')
def test_refresh(self, attachment_get):
db_attachment1 = fake_volume.fake_db_volume_attachment()
attachment_obj1 = fake_volume.fake_volume_attachment_obj(self.context)
db_attachment2 = db_attachment1.copy()
db_attachment2['mountpoint'] = '/dev/sdc'
attachment_obj2 = fake_volume.fake_volume_attachment_obj(
self.context, mountpoint='/dev/sdc')
# On the second volume_attachment_get, return the volume attachment
# with an updated mountpoint
attachment_get.side_effect = [db_attachment1, db_attachment2]
attachment = objects.VolumeAttachment.get_by_id(self.context,
fake.ATTACHMENT_ID)
self._compare(self, db_attachment1, attachment)
self._compare(self, attachment_obj1, attachment)
# mountpoint was updated, so a volume attachment refresh should have a
# new value for that field
attachment.refresh()
self._compare(self, db_attachment2, attachment)
self._compare(self, attachment_obj2, attachment)
if six.PY3:
call_bool = mock.call.__bool__()
else:
@ -98,28 +102,31 @@ class TestVolumeAttachmentList(test_objects.BaseObjectsTestCase):
def test_get_all_by_volume_id(self, get_used_by_volume_id):
db_attachment = fake_volume.fake_db_volume_attachment()
get_used_by_volume_id.return_value = [db_attachment]
attachment_obj = fake_volume.fake_volume_attachment_obj(self.context)
attachments = objects.VolumeAttachmentList.get_all_by_volume_id(
self.context, mock.sentinel.volume_id)
self.assertEqual(1, len(attachments))
TestVolumeAttachment._compare(self, db_attachment, attachments[0])
TestVolumeAttachment._compare(self, attachment_obj, attachments[0])
@mock.patch('cinder.db.volume_attachment_get_all_by_host')
def test_get_all_by_host(self, get_by_host):
db_attachment = fake_volume.fake_db_volume_attachment()
attachment_obj = fake_volume.fake_volume_attachment_obj(self.context)
get_by_host.return_value = [db_attachment]
attachments = objects.VolumeAttachmentList.get_all_by_host(
self.context, mock.sentinel.host)
self.assertEqual(1, len(attachments))
TestVolumeAttachment._compare(self, db_attachment, attachments[0])
TestVolumeAttachment._compare(self, attachment_obj, attachments[0])
@mock.patch('cinder.db.volume_attachment_get_all_by_instance_uuid')
def test_get_all_by_instance_uuid(self, get_by_instance_uuid):
db_attachment = fake_volume.fake_db_volume_attachment()
get_by_instance_uuid.return_value = [db_attachment]
attachment_obj = fake_volume.fake_volume_attachment_obj(self.context)
attachments = objects.VolumeAttachmentList.get_all_by_instance_uuid(
self.context, mock.sentinel.uuid)
self.assertEqual(1, len(attachments))
TestVolumeAttachment._compare(self, db_attachment, attachments[0])
TestVolumeAttachment._compare(self, attachment_obj, attachments[0])

View File

@ -308,13 +308,14 @@ class DBAPIVolumeTestCase(BaseTest):
self._assertEqualObjects(volume, volume_db,
ignored_keys='volume_attachment')
self._assertEqualListsOfObjects(volume.volume_attachment,
volume_db.volume_attachment)
volume_db.volume_attachment, 'volume')
self.assertEqual('in-use', volume['status'])
self.assertEqual('/tmp', attachment['mountpoint'])
self.assertEqual(fields.VolumeAttachStatus.ATTACHED,
attachment['attach_status'])
self.assertEqual(instance_uuid, attachment['instance_uuid'])
self.assertIsNone(attachment['attached_host'])
self.assertEqual(volume.project_id, attachment['volume']['project_id'])
def test_volume_attached_to_host(self):
volume = db.volume_create(self.ctxt, {'host': 'host1'})
@ -338,7 +339,7 @@ class DBAPIVolumeTestCase(BaseTest):
self._assertEqualObjects(volume, volume_db,
ignored_keys='volume_attachment')
self._assertEqualListsOfObjects(volume.volume_attachment,
volume_db.volume_attachment)
volume_db.volume_attachment, 'volume')
attachment = db.volume_attachment_get(self.ctxt, attachment['id'])
self.assertEqual('in-use', volume['status'])
self.assertEqual('/tmp', attachment['mountpoint'])
@ -346,6 +347,7 @@ class DBAPIVolumeTestCase(BaseTest):
attachment['attach_status'])
self.assertIsNone(attachment['instance_uuid'])
self.assertEqual(attachment['attached_host'], host_name)
self.assertEqual(volume.project_id, attachment['volume']['project_id'])
def test_volume_data_get_for_host(self):
for i in range(THREE):