openstack
/
cinder
Code Issues Proposed changes

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)
This commit is contained in:
Gorka Eguileor 2019-01-22 21:24:37 +01:00 committed by Eric Harney
parent 98196cc765
commit cb928c4b3a
4 changed files with 48 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
cinder/objects/volume_attachment.py
View File

@ -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:
db_volume = db_attachment.get('volume')
# 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'):
db_volume = db_attachment.volume
if db_volume:
attachment.volume = objects.Volume._from_db_object(
context, objects.Volume(), db_volume)

22
cinder/tests/unit/fake_volume.py
View File

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

33
cinder/tests/unit/objects/test_volume_attachment.py
View File

@ -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()
attachment_obj = fake_volume.fake_volume_attachment_obj(self.context)
db_attachment = fake_volume.volume_attachment_db_obj()
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()
attachment_obj1 = fake_volume.fake_volume_attachment_obj(self.context)
db_attachment2 = db_attachment1.copy()
db_attachment2['mountpoint'] = '/dev/sdc'
attachment_obj2 = fake_volume.fake_volume_attachment_obj(
db_attachment1 = fake_volume.volume_attachment_db_obj()
attachment_obj1 = fake_volume.volume_attachment_ovo(self.context)
db_attachment2 = fake_volume.volume_attachment_db_obj()
db_attachment2.mountpoint = '/dev/sdc'
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()
attachment_obj = fake_volume.fake_volume_attachment_obj(self.context)
db_attachment = fake_volume.volume_attachment_db_obj()
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])

6
cinder/tests/unit/volume/drivers/test_lvm_driver.py
View File

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