From 2e73bede80cc2acdb3527f06bc5c5f9c1a8463a7 Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Tue, 2 Jul 2019 11:06:17 +0200 Subject: [PATCH] Fix DetachedInstanceError for VolumeAttachment Patch I253123d5451b32f0e3143916e41aaa1af75561c2 fixed the DetachedInstanceError for VolumeAttachment OVOs but only partially, as apparently it was dependent on the SQLAlchemy version due to the use os "hasattr". This patch replaces "hasattr" with a check on the object's dictionary, which will never trigger a Lazy Load. Closes-Bug: #1834845 Change-Id: Iac785eef9be4b9cdb5c739ee0a87949805282867 --- cinder/objects/volume_attachment.py | 13 +++++++----- cinder/tests/unit/objects/test_volume.py | 4 ++-- .../unit/objects/test_volume_attachment.py | 20 +++++++++++++++++-- .../unit/volume/drivers/hpe/test_hpe3par.py | 4 ++-- ...etachedinstanceerror-64be35894c624eae.yaml | 6 ++++++ 5 files changed, 36 insertions(+), 11 deletions(-) create mode 100644 releasenotes/notes/detachedinstanceerror-64be35894c624eae.yaml diff --git a/cinder/objects/volume_attachment.py b/cinder/objects/volume_attachment.py index 75effc06832..d4462e9dc68 100644 --- a/cinder/objects/volume_attachment.py +++ b/cinder/objects/volume_attachment.py @@ -95,11 +95,14 @@ class VolumeAttachment(base.CinderPersistentObject, base.CinderObject, jsonutils.loads(value) if value else None) else: attachment[name] = value - # NOTE: hasattr check is necessary to avoid doing a lazy loading when - # loading VolumeList.get_all: Getting a Volume loads its - # VolumeAttachmentList, which think they have the volume loaded, but - # they don't. More detail on https://review.openstack.org/632549 - if 'volume' in expected_attrs and hasattr(db_attachment, 'volume'): + # NOTE: Check against the ORM instance's dictionary instead of using + # hasattr or get to avoid the lazy loading of the Volume on + # VolumeList.get_all. + # Getting a Volume loads its VolumeAttachmentList, which think they + # have the volume loaded, but they don't. More detail on + # https://review.opendev.org/632549 + # and its related bug report. + if 'volume' in expected_attrs and 'volume' in vars(db_attachment): db_volume = db_attachment.volume if db_volume: attachment.volume = objects.Volume._from_db_object( diff --git a/cinder/tests/unit/objects/test_volume.py b/cinder/tests/unit/objects/test_volume.py index 53d186e5cf5..59f9bb02a52 100644 --- a/cinder/tests/unit/objects/test_volume.py +++ b/cinder/tests/unit/objects/test_volume.py @@ -307,7 +307,7 @@ class TestVolume(test_objects.BaseObjectsTestCase): db_admin_metadata = [{'key': 'admin_foo', 'value': 'admin_bar'}] db_glance_metadata = [{'key': 'glance_foo', 'value': 'glance_bar'}] db_volume_type = fake_volume.fake_db_volume_type() - db_volume_attachments = fake_volume.fake_db_volume_attachment() + db_volume_attachments = fake_volume.volume_attachment_db_obj() db_consistencygroup = fake_consistencygroup.fake_db_consistencygroup() db_snapshots = fake_snapshot.fake_db_snapshot() @@ -450,7 +450,7 @@ class TestVolume(test_objects.BaseObjectsTestCase): @mock.patch('cinder.db.sqlalchemy.api.volume_attach') def test_begin_attach(self, volume_attach, metadata_update): volume = fake_volume.fake_volume_obj(self.context) - db_attachment = fake_volume.fake_db_volume_attachment( + db_attachment = fake_volume.volume_attachment_db_obj( volume_id=volume.id, attach_status=fields.VolumeAttachStatus.ATTACHING) volume_attach.return_value = db_attachment diff --git a/cinder/tests/unit/objects/test_volume_attachment.py b/cinder/tests/unit/objects/test_volume_attachment.py index 101c394296f..1eba69af841 100644 --- a/cinder/tests/unit/objects/test_volume_attachment.py +++ b/cinder/tests/unit/objects/test_volume_attachment.py @@ -15,6 +15,7 @@ import ddt import mock import six +from sqlalchemy.orm import attributes from cinder import db from cinder import objects @@ -47,9 +48,24 @@ class TestVolumeAttachment(test_objects.BaseObjectsTestCase): self.assertEqual(volume, r) volume_get_mock.assert_called_once_with(self.context, volume.id) + def test_from_db_object_no_volume(self): + original_get = attributes.InstrumentedAttribute.__get__ + + def my_get(get_self, instance, owner): + self.assertNotEqual('volume', get_self.key) + return original_get(get_self, instance, owner) + + # Volume field is not loaded + attach = fake_volume.models.VolumeAttachment(id=fake.ATTACHMENT_ID, + volume_id=fake.VOLUME_ID) + patch_str = 'sqlalchemy.orm.attributes.InstrumentedAttribute.__get__' + with mock.patch(patch_str, side_effect=my_get): + objects.VolumeAttachment._from_db_object( + self.context, objects.VolumeAttachment(), attach) + @mock.patch('cinder.db.volume_attachment_update') def test_save(self, volume_attachment_update): - attachment = fake_volume.fake_volume_attachment_obj(self.context) + attachment = fake_volume.volume_attachment_ovo(self.context) attachment.attach_status = fields.VolumeAttachStatus.ATTACHING attachment.save() volume_attachment_update.assert_called_once_with( @@ -88,7 +104,7 @@ class TestVolumeAttachment(test_objects.BaseObjectsTestCase): @mock.patch('cinder.db.sqlalchemy.api.volume_attached') def test_volume_attached(self, volume_attached): - attachment = fake_volume.fake_volume_attachment_obj(self.context) + attachment = fake_volume.volume_attachment_ovo(self.context) updated_values = {'mountpoint': '/dev/sda', 'attach_status': fields.VolumeAttachStatus.ATTACHED, 'instance_uuid': fake.INSTANCE_ID} diff --git a/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py b/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py index 791b4e1a64b..7b568c97a52 100644 --- a/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py +++ b/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py @@ -10140,9 +10140,9 @@ class TestHPE3PARISCSIDriver(HPE3PARBaseDriver): def test_terminate_connection_multiattach(self): ctx = context.get_admin_context() mock_client = self.setup_driver() - att_1 = fake_volume.fake_volume_attachment_obj( + att_1 = fake_volume.volume_attachment_ovo( ctx, id=uuidutils.generate_uuid()) - att_2 = fake_volume.fake_volume_attachment_obj( + att_2 = fake_volume.volume_attachment_ovo( ctx, id=uuidutils.generate_uuid()) volume = fake_volume.fake_volume_obj( ctx, multiattach=True, host=self.FAKE_CINDER_HOST) diff --git a/releasenotes/notes/detachedinstanceerror-64be35894c624eae.yaml b/releasenotes/notes/detachedinstanceerror-64be35894c624eae.yaml new file mode 100644 index 00000000000..900a6d349a6 --- /dev/null +++ b/releasenotes/notes/detachedinstanceerror-64be35894c624eae.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + Fix DetachedInstanceError is not bound to a Session for VolumeAttachments. + This affected VolumeList.get_all, and could make a service fail on startup + and make it stay in down state.