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

@@ -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(

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

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}

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)

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.