Merge "Fix DetachedInstanceError for VolumeAttachment" into stable/queens

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

@@ -1080,10 +1080,10 @@ class LVMISCSITestCase(test_driver.BaseDriverTestCase):
                                   'mountpoint': '/dev/vdc',
                                   'initiator': 'iqn.2012-07.org.fake:01'}
 
-        attachment1 = fake_volume.fake_volume_attachment_obj(self.context)
+        attachment1 = fake_volume.volume_attachment_ovo(self.context)
         attachment1.connector = connector_mountpoint_1
 
-        attachment2 = fake_volume.fake_volume_attachment_obj(self.context)
+        attachment2 = fake_volume.volume_attachment_ovo(self.context)
         attachment2.connector = connector_mountpoint_2
 
         # Create a multiattach volume object with two active attachments on

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.