Merge "Fix VolumeAttachment is not bound to a Session"

cinder/objects/volume_attachment.py

@@ -95,8 +95,12 @@ class VolumeAttachment(base.CinderPersistentObject, base.CinderObject,
                         jsonutils.loads(value) if value else None)
             else:
                 attachment[name] = value
-        if 'volume' in expected_attrs:
+        # NOTE: hasattr check is necessary to avoid doing a lazy loading when
-            db_volume = db_attachment.get('volume')
+        # 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'):
+            db_volume = db_attachment.volume
             if db_volume:
                 attachment.volume = objects.Volume._from_db_object(
                     context, objects.Volume(), db_volume)

cinder/tests/unit/fake_volume.py

@@ -15,6 +15,7 @@
 from oslo_utils.uuidutils import is_uuid_like
 from oslo_versionedobjects import fields
 
+from cinder.db.sqlalchemy import models
 from cinder import objects
 from cinder.objects import fields as c_fields
 from cinder.tests.unit import fake_constants as fake
@@ -123,3 +124,24 @@ def fake_volume_attachment_obj(context, **updates):
     return objects.VolumeAttachment._from_db_object(
         context, objects.VolumeAttachment(),
         fake_db_volume_attachment(**updates))
+
+
+def volume_db_obj(**updates):
+    """Return a volume ORM object."""
+    updates.setdefault('id', fake.VOLUME_ID)
+    updates.setdefault('size', 1)
+    return models.Volume(**updates)
+
+
+def volume_attachment_db_obj(**updates):
+    updates.setdefault('id', fake.ATTACHMENT_ID)
+    updates.setdefault('volume_id', fake.VOLUME_ID)
+    updates.setdefault('volume', volume_db_obj())
+    return models.VolumeAttachment(**updates)
+
+
+def volume_attachment_ovo(context, **updates):
+    orm = volume_attachment_db_obj(**updates)
+    return objects.VolumeAttachment._from_db_object(context,
+                                                    objects.VolumeAttachment(),
+                                                    orm)

cinder/tests/unit/objects/test_volume_attachment.py

@@ -29,8 +29,8 @@ class TestVolumeAttachment(test_objects.BaseObjectsTestCase):
 
     @mock.patch('cinder.db.sqlalchemy.api.volume_attachment_get')
     def test_get_by_id(self, volume_attachment_get):
-        db_attachment = fake_volume.fake_db_volume_attachment()
+        db_attachment = fake_volume.volume_attachment_db_obj()
-        attachment_obj = fake_volume.fake_volume_attachment_obj(self.context)
+        attachment_obj = fake_volume.volume_attachment_ovo(self.context)
         volume_attachment_get.return_value = db_attachment
         attachment = objects.VolumeAttachment.get_by_id(self.context,
                                                         fake.ATTACHMENT_ID)
@@ -58,11 +58,11 @@ class TestVolumeAttachment(test_objects.BaseObjectsTestCase):
 
     @mock.patch('cinder.db.sqlalchemy.api.volume_attachment_get')
     def test_refresh(self, attachment_get):
-        db_attachment1 = fake_volume.fake_db_volume_attachment()
+        db_attachment1 = fake_volume.volume_attachment_db_obj()
-        attachment_obj1 = fake_volume.fake_volume_attachment_obj(self.context)
+        attachment_obj1 = fake_volume.volume_attachment_ovo(self.context)
-        db_attachment2 = db_attachment1.copy()
+        db_attachment2 = fake_volume.volume_attachment_db_obj()
-        db_attachment2['mountpoint'] = '/dev/sdc'
+        db_attachment2.mountpoint = '/dev/sdc'
-        attachment_obj2 = fake_volume.fake_volume_attachment_obj(
+        attachment_obj2 = fake_volume.volume_attachment_ovo(
             self.context, mountpoint='/dev/sdc')
 
         # On the second volume_attachment_get, return the volume attachment
@@ -167,33 +167,34 @@ class TestVolumeAttachment(test_objects.BaseObjectsTestCase):
 class TestVolumeAttachmentList(test_objects.BaseObjectsTestCase):
     @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()
+        db_attachment = fake_volume.volume_attachment_db_obj()
         get_used_by_volume_id.return_value = [db_attachment]
-        attachment_obj = fake_volume.fake_volume_attachment_obj(self.context)
+        attachment_obj = fake_volume.volume_attachment_ovo(self.context)
 
         attachments = objects.VolumeAttachmentList.get_all_by_volume_id(
             self.context, mock.sentinel.volume_id)
+
         self.assertEqual(1, len(attachments))
-        TestVolumeAttachment._compare(self, attachment_obj, attachments[0])
+        self._compare(self, attachment_obj, attachments[0])
 
     @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()
+        db_attachment = fake_volume.volume_attachment_db_obj()
-        attachment_obj = fake_volume.fake_volume_attachment_obj(self.context)
+        attachment_obj = fake_volume.volume_attachment_ovo(self.context)
         get_by_host.return_value = [db_attachment]
 
         attachments = objects.VolumeAttachmentList.get_all_by_host(
             self.context, mock.sentinel.host)
         self.assertEqual(1, len(attachments))
-        TestVolumeAttachment._compare(self, attachment_obj, attachments[0])
+        self._compare(self, attachment_obj, attachments[0])
 
     @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()
+        db_attachment = fake_volume.volume_attachment_db_obj()
         get_by_instance_uuid.return_value = [db_attachment]
-        attachment_obj = fake_volume.fake_volume_attachment_obj(self.context)
+        attachment_obj = fake_volume.volume_attachment_ovo(self.context)
 
         attachments = objects.VolumeAttachmentList.get_all_by_instance_uuid(
             self.context, mock.sentinel.uuid)
         self.assertEqual(1, len(attachments))
-        TestVolumeAttachment._compare(self, attachment_obj, attachments[0])
+        self._compare(self, attachment_obj, attachments[0])

cinder/tests/unit/volume/drivers/test_lvm_driver.py

@@ -1000,15 +1000,15 @@ class LVMISCSITestCase(test_driver.BaseDriverTestCase):
                            'host': 'fakehost2',
                            'initiator': 'iqn.2012-07.org.fake:02'}
 
-        host1_attachment1 = fake_volume.fake_volume_attachment_obj(
+        host1_attachment1 = fake_volume.volume_attachment_ovo(
             self.context)
         host1_attachment1.connector = host1_connector
 
-        host1_attachment2 = fake_volume.fake_volume_attachment_obj(
+        host1_attachment2 = fake_volume.volume_attachment_ovo(
             self.context)
         host1_attachment2.connector = host1_connector
 
-        host2_attachment = fake_volume.fake_volume_attachment_obj(self.context)
+        host2_attachment = fake_volume.volume_attachment_ovo(self.context)
         host2_attachment.connector = host2_connector
 
         # Create a multiattach volume object with two active attachments on