openstack
/
cinder
Code Issues Proposed changes

Fix attachment_get_by_* to return entire list 

Commit 10d5421687 added the
ability to query volume_attachments by host and instance_uuid.

Minor bug however is that the original implementation also used
a "first()" filter, so regardless of how many attachments might
exist, only one was being returned.

The volume_attachments object code assumes we receive a list, as
do a number of other places.  So, this patch changes the names of
those get methods to "get_all_by_xxx" to eliminate any confusion
and updates things to use the list appropriately.

Change-Id: I63e4faf8a7ee3528a4dfbd7bf4e61b607ad86841
Closes-Bug: #1585690
This commit is contained in:
John Griffith 2016-05-25 10:04:50 -06:00
parent adf6529673
commit 61a63dc77d
10 changed files with 49 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
cinder/api/contrib/admin_actions.py
View File

@ -84,7 +84,7 @@ class AdminController(wsgi.Controller):
def _clean_volume_attachment(context, id):
attachments = (
db.volume_attachment_get_used_by_volume_id(context, id))
db.volume_attachment_get_all_by_volume_id(context, id))
for attachment in attachments:
db.volume_detached(context, id, attachment.id)
db.volume_admin_metadata_delete(context, id,

16
cinder/db/api.py
View File

@ -236,17 +236,19 @@ def volume_attachment_get(context, attachment_id, session=None):
return IMPL.volume_attachment_get(context, attachment_id, session)
def volume_attachment_get_used_by_volume_id(context, volume_id):
return IMPL.volume_attachment_get_used_by_volume_id(context, volume_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_by_host(context, volume_id, host):
return IMPL.volume_attachment_get_by_host(context, volume_id, host)
def volume_attachment_get_all_by_host(context, volume_id, host):
return IMPL.volume_attachment_get_all_by_host(context, volume_id, host)
def volume_attachment_get_by_instance_uuid(context, volume_id, instance_uuid):
return IMPL.volume_attachment_get_by_instance_uuid(context, volume_id,
instance_uuid)
def volume_attachment_get_all_by_instance_uuid(context,
volume_id,
instance_uuid):
return IMPL.volume_attachment_get_all_by_instance_uuid(context, volume_id,
instance_uuid)
def volume_update_status_based_on_attachment(context, volume_id):

14
cinder/db/sqlalchemy/api.py
View File

@ -1354,7 +1354,7 @@ def volume_detached(context, volume_id, attachment_id):
attachment['deleted_at'] = now
attachment.save(session=session)
attachment_list = volume_attachment_get_used_by_volume_id(
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:
@ -1441,7 +1441,7 @@ def volume_attachment_get(context, attachment_id, session=None):
@require_context
def volume_attachment_get_used_by_volume_id(context, volume_id, session=None):
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).\
@ -1451,7 +1451,7 @@ def volume_attachment_get_used_by_volume_id(context, volume_id, session=None):
@require_context
def volume_attachment_get_by_host(context, volume_id, host):
def volume_attachment_get_all_by_host(context, volume_id, host):
session = get_session()
with session.begin():
result = model_query(context, models.VolumeAttachment,
@ -1459,12 +1459,14 @@ def volume_attachment_get_by_host(context, volume_id, host):
filter_by(volume_id=volume_id).\
filter_by(attached_host=host).\
filter(models.VolumeAttachment.attach_status != 'detached').\
first()
all()
return result
@require_context
def volume_attachment_get_by_instance_uuid(context, volume_id, instance_uuid):
def volume_attachment_get_all_by_instance_uuid(context,
volume_id,
instance_uuid):
session = get_session()
with session.begin():
result = model_query(context, models.VolumeAttachment,
@ -1472,7 +1474,7 @@ def volume_attachment_get_by_instance_uuid(context, volume_id, instance_uuid):
filter_by(volume_id=volume_id).\
filter_by(instance_uuid=instance_uuid).\
filter(models.VolumeAttachment.attach_status != 'detached').\
first()
all()
return result

17
cinder/objects/volume_attachment.py
View File

@ -74,21 +74,24 @@ class VolumeAttachmentList(base.ObjectListBase, base.CinderObject):
@base.remotable_classmethod
def get_all_by_volume_id(cls, context, volume_id):
attachments = db.volume_attachment_get_used_by_volume_id(context,
volume_id)
return base.obj_make_list(context, cls(context),
objects.VolumeAttachment, attachments)
attachments = db.volume_attachment_get_all_by_volume_id(context,
volume_id)
return base.obj_make_list(context,
cls(context),
objects.VolumeAttachment,
attachments)
@base.remotable_classmethod
def get_all_by_host(cls, context, volume_id, host):
attachments = db.volume_attachment_get_by_host(context, volume_id,
host)
attachments = db.volume_attachment_get_all_by_host(context,
volume_id,
host)
return base.obj_make_list(context, cls(context),
objects.VolumeAttachment, attachments)
@base.remotable_classmethod
def get_all_by_instance_uuid(cls, context, volume_id, instance_uuid):
attachments = db.volume_attachment_get_by_instance_uuid(
attachments = db.volume_attachment_get_all_by_instance_uuid(
context, volume_id, instance_uuid)
return base.obj_make_list(context, cls(context),
objects.VolumeAttachment, attachments)

6
cinder/tests/unit/objects/test_volume_attachment.py
View File

@ -68,7 +68,7 @@ class TestVolumeAttachment(test_objects.BaseObjectsTestCase):
class TestVolumeAttachmentList(test_objects.BaseObjectsTestCase):
@mock.patch('cinder.db.volume_attachment_get_used_by_volume_id')
@mock.patch('cinder.db.volume_attachment_get_all_by_volume_id')
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]
@ -78,7 +78,7 @@ class TestVolumeAttachmentList(test_objects.BaseObjectsTestCase):
self.assertEqual(1, len(attachments))
TestVolumeAttachment._compare(self, db_attachment, attachments[0])
@mock.patch('cinder.db.volume_attachment_get_by_host')
@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()
get_by_host.return_value = [db_attachment]
@ -88,7 +88,7 @@ class TestVolumeAttachmentList(test_objects.BaseObjectsTestCase):
self.assertEqual(1, len(attachments))
TestVolumeAttachment._compare(self, db_attachment, attachments[0])
@mock.patch('cinder.db.volume_attachment_get_by_instance_uuid')
@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]

2
cinder/tests/unit/scheduler/test_scheduler.py
View File

@ -255,7 +255,7 @@ class SchedulerManagerTestCase(test.TestCase):
request_spec, {})
@mock.patch('cinder.db.volume_update')
@mock.patch('cinder.db.volume_attachment_get_used_by_volume_id')
@mock.patch('cinder.db.volume_attachment_get_all_by_volume_id')
def test_retype_volume_exception_returns_volume_state(
self, _mock_vol_attachment_get, _mock_vol_update):
# Test NoValidHost exception behavior for retype.

10
cinder/tests/unit/test_volume.py
View File

@ -5030,11 +5030,13 @@ class VolumeMigrationTestCase(BaseVolumeTestCase):
mock_detach_volume.assert_called_with(self.context,
old_volume.id,
attachment_id)
attachment = db.volume_attachment_get_by_instance_uuid(
attachments = db.volume_attachment_get_all_by_instance_uuid(
self.context, old_volume.id, instance_uuid)
self.assertIsNotNone(attachment)
self.assertEqual(attached_host, attachment['attached_host'])
self.assertEqual(instance_uuid, attachment['instance_uuid'])
self.assertIsNotNone(attachments)
self.assertEqual(attached_host,
attachments[0]['attached_host'])
self.assertEqual(instance_uuid,
attachments[0]['instance_uuid'])
else:
self.assertFalse(mock_detach_volume.called)
self.assertTrue(mock_delete_volume.called)

2
cinder/tests/unit/test_volume_utils.py
View File

@ -161,7 +161,7 @@ class NotifyUsageTestCase(test.TestCase):
self.assertDictMatch(expected_snapshot, usage_info)
@mock.patch('cinder.db.volume_glance_metadata_get')
@mock.patch('cinder.db.volume_attachment_get_used_by_volume_id')
@mock.patch('cinder.db.volume_attachment_get_all_by_volume_id')
def test_usage_from_volume(self, mock_attachment, mock_image_metadata):
mock_image_metadata.return_value = {'image_id': 'fake_image_id'}
mock_attachment.return_value = [{'instance_uuid': 'fake_instance_id'}]

17
cinder/volume/manager.py
View File

@ -965,18 +965,19 @@ class VolumeManager(manager.SchedulerDependentManager):
raise exception.InvalidVolume(
reason=_("volume is already attached"))
attachment = None
host_name_sanitized = utils.sanitize_hostname(
host_name) if host_name else None
if instance_uuid:
attachment = \
self.db.volume_attachment_get_by_instance_uuid(
attachments = \
self.db.volume_attachment_get_all_by_instance_uuid(
context, volume_id, instance_uuid)
else:
attachment = \
self.db.volume_attachment_get_by_host(context, volume_id,
host_name_sanitized)
if attachment is not None:
attachments = (
self.db.volume_attachment_get_all_by_host(
context,
volume_id,
host_name_sanitized))
if attachments:
self.db.volume_update(context, volume_id,
{'status': 'in-use'})
return
@ -1067,7 +1068,7 @@ class VolumeManager(manager.SchedulerDependentManager):
# We can try and degrade gracefully here by trying to detach
# a volume without the attachment_id here if the volume only has
# one attachment. This is for backwards compatibility.
attachments = self.db.volume_attachment_get_used_by_volume_id(
attachments = self.db.volume_attachment_get_all_by_volume_id(
context, volume_id)
if len(attachments) > 1:
# There are more than 1 attachments for this volume

2
cinder/volume/utils.py
View File

@ -76,7 +76,7 @@ def _usage_from_volume(context, volume_ref, **kw):
usage_info.update(kw)
try:
attachments = db.volume_attachment_get_used_by_volume_id(
attachments = db.volume_attachment_get_all_by_volume_id(
context, volume_ref['id'])
usage_info['volume_attachment'] = attachments