Browse Source

Fix VolumeAttachment is not bound to a Session

In some cases, when loading a list of volumes using
cinder.objects.VolumeList.get_all we may end up with a SQL alchemy error
like this one:

 sqlalchemy.orm.exc.DetachedInstanceError: Parent instance
 <VolumeAttachment at 0x7f048ff22290> is not bound to a Session; lazy
 load operation of attribute 'volume' cannot proceed (Background on this
 error at: http://sqlalche.me/e/bhk3)

This happens because when loading a Cinder Volume we also load the
volume attachments, so the Volume OVO tries to create the
VolumeAttachmentList OVO, which in turn creates the individual
VolumeAttachment OVOs, but these have the volume field set in
expected_attrs, but in this case the volume data is not loaded.

In most cases we don't see this issue because the session is still
available and SQLAlchemy will return this information.

Instead of using the dictionary "get" method that will do the lazy
loading we use the hasattr method that will return False if the lazy
loading exception happens when trying to get the volume.

Depends-On: I9f0fec25444ed865d56d0d250fb6d840ab5b4095
Closes-Bug: #1812913
Change-Id: I253123d5451b32f0e3143916e41aaa1af75561c2
(cherry picked from commit 42ce739a08)
changes/82/679682/1
Gorka Eguileor 7 months ago
parent
commit
b0f6384751

+ 6
- 2
cinder/objects/volume_attachment.py View File

@@ -95,8 +95,12 @@ class VolumeAttachment(base.CinderPersistentObject, base.CinderObject,
95 95
                         jsonutils.loads(value) if value else None)
96 96
             else:
97 97
                 attachment[name] = value
98
-        if 'volume' in expected_attrs:
99
-            db_volume = db_attachment.get('volume')
98
+        # NOTE: hasattr check is necessary to avoid doing a lazy loading when
99
+        # loading VolumeList.get_all: Getting a Volume loads its
100
+        # VolumeAttachmentList, which think they have the volume loaded, but
101
+        # they don't.  More detail on https://review.openstack.org/632549
102
+        if 'volume' in expected_attrs and hasattr(db_attachment, 'volume'):
103
+            db_volume = db_attachment.volume
100 104
             if db_volume:
101 105
                 attachment.volume = objects.Volume._from_db_object(
102 106
                     context, objects.Volume(), db_volume)

+ 22
- 0
cinder/tests/unit/fake_volume.py View File

@@ -15,6 +15,7 @@
15 15
 from oslo_utils.uuidutils import is_uuid_like
16 16
 from oslo_versionedobjects import fields
17 17
 
18
+from cinder.db.sqlalchemy import models
18 19
 from cinder import objects
19 20
 from cinder.objects import fields as c_fields
20 21
 from cinder.tests.unit import fake_constants as fake
@@ -123,3 +124,24 @@ def fake_volume_attachment_obj(context, **updates):
123 124
     return objects.VolumeAttachment._from_db_object(
124 125
         context, objects.VolumeAttachment(),
125 126
         fake_db_volume_attachment(**updates))
127
+
128
+
129
+def volume_db_obj(**updates):
130
+    """Return a volume ORM object."""
131
+    updates.setdefault('id', fake.VOLUME_ID)
132
+    updates.setdefault('size', 1)
133
+    return models.Volume(**updates)
134
+
135
+
136
+def volume_attachment_db_obj(**updates):
137
+    updates.setdefault('id', fake.ATTACHMENT_ID)
138
+    updates.setdefault('volume_id', fake.VOLUME_ID)
139
+    updates.setdefault('volume', volume_db_obj())
140
+    return models.VolumeAttachment(**updates)
141
+
142
+
143
+def volume_attachment_ovo(context, **updates):
144
+    orm = volume_attachment_db_obj(**updates)
145
+    return objects.VolumeAttachment._from_db_object(context,
146
+                                                    objects.VolumeAttachment(),
147
+                                                    orm)

+ 17
- 16
cinder/tests/unit/objects/test_volume_attachment.py View File

@@ -29,8 +29,8 @@ class TestVolumeAttachment(test_objects.BaseObjectsTestCase):
29 29
 
30 30
     @mock.patch('cinder.db.sqlalchemy.api.volume_attachment_get')
31 31
     def test_get_by_id(self, volume_attachment_get):
32
-        db_attachment = fake_volume.fake_db_volume_attachment()
33
-        attachment_obj = fake_volume.fake_volume_attachment_obj(self.context)
32
+        db_attachment = fake_volume.volume_attachment_db_obj()
33
+        attachment_obj = fake_volume.volume_attachment_ovo(self.context)
34 34
         volume_attachment_get.return_value = db_attachment
35 35
         attachment = objects.VolumeAttachment.get_by_id(self.context,
36 36
                                                         fake.ATTACHMENT_ID)
@@ -58,11 +58,11 @@ class TestVolumeAttachment(test_objects.BaseObjectsTestCase):
58 58
 
59 59
     @mock.patch('cinder.db.sqlalchemy.api.volume_attachment_get')
60 60
     def test_refresh(self, attachment_get):
61
-        db_attachment1 = fake_volume.fake_db_volume_attachment()
62
-        attachment_obj1 = fake_volume.fake_volume_attachment_obj(self.context)
63
-        db_attachment2 = db_attachment1.copy()
64
-        db_attachment2['mountpoint'] = '/dev/sdc'
65
-        attachment_obj2 = fake_volume.fake_volume_attachment_obj(
61
+        db_attachment1 = fake_volume.volume_attachment_db_obj()
62
+        attachment_obj1 = fake_volume.volume_attachment_ovo(self.context)
63
+        db_attachment2 = fake_volume.volume_attachment_db_obj()
64
+        db_attachment2.mountpoint = '/dev/sdc'
65
+        attachment_obj2 = fake_volume.volume_attachment_ovo(
66 66
             self.context, mountpoint='/dev/sdc')
67 67
 
68 68
         # On the second volume_attachment_get, return the volume attachment
@@ -167,33 +167,34 @@ class TestVolumeAttachment(test_objects.BaseObjectsTestCase):
167 167
 class TestVolumeAttachmentList(test_objects.BaseObjectsTestCase):
168 168
     @mock.patch('cinder.db.volume_attachment_get_all_by_volume_id')
169 169
     def test_get_all_by_volume_id(self, get_used_by_volume_id):
170
-        db_attachment = fake_volume.fake_db_volume_attachment()
170
+        db_attachment = fake_volume.volume_attachment_db_obj()
171 171
         get_used_by_volume_id.return_value = [db_attachment]
172
-        attachment_obj = fake_volume.fake_volume_attachment_obj(self.context)
172
+        attachment_obj = fake_volume.volume_attachment_ovo(self.context)
173 173
 
174 174
         attachments = objects.VolumeAttachmentList.get_all_by_volume_id(
175 175
             self.context, mock.sentinel.volume_id)
176
+
176 177
         self.assertEqual(1, len(attachments))
177
-        TestVolumeAttachment._compare(self, attachment_obj, attachments[0])
178
+        self._compare(self, attachment_obj, attachments[0])
178 179
 
179 180
     @mock.patch('cinder.db.volume_attachment_get_all_by_host')
180 181
     def test_get_all_by_host(self, get_by_host):
181
-        db_attachment = fake_volume.fake_db_volume_attachment()
182
-        attachment_obj = fake_volume.fake_volume_attachment_obj(self.context)
182
+        db_attachment = fake_volume.volume_attachment_db_obj()
183
+        attachment_obj = fake_volume.volume_attachment_ovo(self.context)
183 184
         get_by_host.return_value = [db_attachment]
184 185
 
185 186
         attachments = objects.VolumeAttachmentList.get_all_by_host(
186 187
             self.context, mock.sentinel.host)
187 188
         self.assertEqual(1, len(attachments))
188
-        TestVolumeAttachment._compare(self, attachment_obj, attachments[0])
189
+        self._compare(self, attachment_obj, attachments[0])
189 190
 
190 191
     @mock.patch('cinder.db.volume_attachment_get_all_by_instance_uuid')
191 192
     def test_get_all_by_instance_uuid(self, get_by_instance_uuid):
192
-        db_attachment = fake_volume.fake_db_volume_attachment()
193
+        db_attachment = fake_volume.volume_attachment_db_obj()
193 194
         get_by_instance_uuid.return_value = [db_attachment]
194
-        attachment_obj = fake_volume.fake_volume_attachment_obj(self.context)
195
+        attachment_obj = fake_volume.volume_attachment_ovo(self.context)
195 196
 
196 197
         attachments = objects.VolumeAttachmentList.get_all_by_instance_uuid(
197 198
             self.context, mock.sentinel.uuid)
198 199
         self.assertEqual(1, len(attachments))
199
-        TestVolumeAttachment._compare(self, attachment_obj, attachments[0])
200
+        self._compare(self, attachment_obj, attachments[0])

+ 3
- 3
cinder/tests/unit/volume/drivers/test_lvm_driver.py View File

@@ -1020,15 +1020,15 @@ class LVMISCSITestCase(test_driver.BaseDriverTestCase):
1020 1020
                            'host': 'fakehost2',
1021 1021
                            'initiator': 'iqn.2012-07.org.fake:02'}
1022 1022
 
1023
-        host1_attachment1 = fake_volume.fake_volume_attachment_obj(
1023
+        host1_attachment1 = fake_volume.volume_attachment_ovo(
1024 1024
             self.context)
1025 1025
         host1_attachment1.connector = host1_connector
1026 1026
 
1027
-        host1_attachment2 = fake_volume.fake_volume_attachment_obj(
1027
+        host1_attachment2 = fake_volume.volume_attachment_ovo(
1028 1028
             self.context)
1029 1029
         host1_attachment2.connector = host1_connector
1030 1030
 
1031
-        host2_attachment = fake_volume.fake_volume_attachment_obj(self.context)
1031
+        host2_attachment = fake_volume.volume_attachment_ovo(self.context)
1032 1032
         host2_attachment.connector = host2_connector
1033 1033
 
1034 1034
         # Create a multiattach volume object with two active attachments on

Loading…
Cancel
Save