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/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.