openstack
/
nova
Code Issues Proposed changes

Translate the return value of attachment_create and _update 

The return value from attachment create and update contains the
connection_info without the 'data' sub-dictionary in it, which
is a non-backwards-compatible change.

This patch adds a translation helper method to volume/cinder.py
to build the connection_info dict back to be a nested dict with
the 'data' key in it.

We will switch to the version without the 'data' key at a later
time with the aim of removing complexity.

The test_swap_volume_with_new_attachment_id_driver_swap_fails
test is updated a bit to validate the attachment_update return
value is used properly.

Change-Id: I8924da50867c1447cf6e441c85393429d21ef74a
This commit is contained in:
Ildiko Vancsa 2017-07-21 13:26:35 +02:00 committed by Matt Riedemann
parent 7234e6e474
commit 74a72ecef0
4 changed files with 86 additions and 47 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
nova/compute/manager.py
View File

@ -4912,7 +4912,7 @@ class ComputeManager(manager.Manager):
# the host connector, which will give us back the new attachment
# connection_info.
new_cinfo = self.volume_api.attachment_update(
context, new_attachment_id, connector).connection_info
context, new_attachment_id, connector)['connection_info']
old_cinfo = jsonutils.loads(bdm['connection_info'])
if old_cinfo and 'serial' not in old_cinfo:

18
nova/tests/unit/compute/test_compute_mgr.py
View File

@ -2007,8 +2007,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase):
@mock.patch.object(objects.BlockDeviceMapping,
'get_by_volume_and_instance')
@mock.patch('nova.volume.cinder.API.get')
@mock.patch('nova.volume.cinder.API.attachment_update',
return_value=mock.Mock(connection_info={}))
@mock.patch('nova.volume.cinder.API.attachment_update')
@mock.patch('nova.volume.cinder.API.attachment_delete')
@mock.patch('nova.volume.cinder.API.migrate_volume_completion',
return_value={'save_volume_id': uuids.old_volume_id})
@ -2030,6 +2029,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase):
new_volume = {
'id': uuids.new_volume_id, 'size': 1, 'status': 'reserved'
}
attachment_update.return_value = {"connection_info": {"data": {}}}
get_bdm.return_value = bdm
get_volume.side_effect = (old_volume, new_volume)
instance = fake_instance.fake_instance_obj(self.context)
@ -2073,8 +2073,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase):
@mock.patch.object(objects.BlockDeviceMapping,
'get_by_volume_and_instance')
@mock.patch('nova.volume.cinder.API.get')
@mock.patch('nova.volume.cinder.API.attachment_update',
return_value=mock.Mock(connection_info={}))
@mock.patch('nova.volume.cinder.API.attachment_update')
@mock.patch('nova.volume.cinder.API.attachment_delete')
@mock.patch('nova.volume.cinder.API.migrate_volume_completion')
def test_swap_volume_with_new_attachment_id_cinder_migrate_false(
@ -2095,6 +2094,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase):
new_volume = {
'id': uuids.new_volume_id, 'size': 2, 'status': 'reserved'
}
attachment_update.return_value = {"connection_info": {"data": {}}}
get_bdm.return_value = bdm
get_volume.side_effect = (old_volume, new_volume)
instance = fake_instance.fake_instance_obj(self.context)
@ -2200,8 +2200,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase):
@mock.patch.object(objects.BlockDeviceMapping,
'get_by_volume_and_instance')
@mock.patch('nova.volume.cinder.API.get')
@mock.patch('nova.volume.cinder.API.attachment_update',
return_value=mock.Mock(connection_info={}))
@mock.patch('nova.volume.cinder.API.attachment_update')
@mock.patch('nova.volume.cinder.API.roll_detaching')
@mock.patch('nova.volume.cinder.API.attachment_delete')
@mock.patch('nova.volume.cinder.API.migrate_volume_completion')
@ -2224,6 +2223,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase):
new_volume = {
'id': uuids.new_volume_id, 'size': 2, 'status': 'reserved'
}
attachment_update.return_value = {"connection_info": {"data": {}}}
get_bdm.return_value = bdm
get_volume.side_effect = (old_volume, new_volume)
instance = fake_instance.fake_instance_obj(self.context)
@ -2235,13 +2235,17 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase):
mock.patch.object(self.compute.driver, 'swap_volume',
side_effect=test.TestingException('yikes'))
) as (
mock_elevated, mock_get_volume_connector, mock_save
mock_elevated, mock_get_volume_connector, mock_driver_swap
):
self.assertRaises(
test.TestingException, self.compute.swap_volume,
self.context, uuids.old_volume_id, uuids.new_volume_id,
instance, uuids.new_attachment_id)
# Assert the expected calls.
# The new connection_info has the new_volume_id as the serial.
new_cinfo = mock_driver_swap.call_args[0][1]
self.assertIn('serial', new_cinfo)
self.assertEqual(uuids.new_volume_id, new_cinfo['serial'])
get_bdm.assert_called_once_with(
self.context, uuids.old_volume_id, instance.uuid)
# We updated the new attachment with the host connector.

73
nova/tests/unit/volume/test_cinder.py
View File

@ -66,6 +66,41 @@ class FakeSnapshot(object):
self.project_id = 'fake_project'
class FakeAttachment(object):
def __init__(self):
self.id = uuids.attachment_id
self.status = 'attached'
self.instance = uuids.instance_uuid
self.volume_id = uuids.volume_id
self.attached_at = timeutils.utcnow()
self.detached_at = None
self.attach_mode = 'rw'
self.connection_info = {'driver_volume_type': 'fake_type',
'target_lun': '1',
'foo': 'bar'}
self.att = {'id': self.id,
'status': self.status,
'instance': self.instance,
'volume_id': self.volume_id,
'attached_at': self.attached_at,
'detached_at': self.detached_at,
'attach_mode': self.attach_mode,
'connection_info': self.connection_info}
def get(self, key, default=None):
return self.att.get(key, default)
def __setitem__(self, key, value):
self.att[key] = value
def __getitem__(self, key):
return self.att[key]
def to_dict(self):
return self.att
class CinderApiTestCase(test.NoDBTestCase):
def setUp(self):
super(CinderApiTestCase, self).setUp()
@ -265,23 +300,15 @@ class CinderApiTestCase(test.NoDBTestCase):
@mock.patch('nova.volume.cinder.cinderclient')
def test_attachment_create(self, mock_cinderclient):
"""Tests the happy path for creating a volume attachment."""
values = {
'id': uuids.attachment_id,
'status': 'reserved',
'instance': uuids.instance_id,
'volume_id': uuids.volume_id,
'attached_at': timeutils.utcnow(),
'detached_at': None,
'attach_mode': 'rw',
'connection_info': None
}
fake_attachment = mock.Mock(
autospec='cinderclient.v3.attachments.VolumeAttachment', **values)
attachment_ref = {'id': uuids.attachment_id,
'connection_info': {}}
expected_attachment_ref = {'id': uuids.attachment_id,
'connection_info': {'data': {}}}
mock_cinderclient.return_value.attachments.create.return_value = (
fake_attachment)
attachment_ref)
result = self.api.attachment_create(
self.ctx, uuids.volume_id, uuids.instance_id)
self.assertEqual(fake_attachment, result)
self.assertEqual(expected_attachment_ref, result)
mock_cinderclient.return_value.attachments.create.\
assert_called_once_with(uuids.volume_id, None, uuids.instance_id)
@ -310,23 +337,23 @@ class CinderApiTestCase(test.NoDBTestCase):
@mock.patch('nova.volume.cinder.cinderclient')
def test_attachment_update(self, mock_cinderclient):
"""Tests the happy path for updating a volume attachment."""
values = {
fake_attachment = FakeAttachment()
expected_attachment_ref = {
'id': uuids.attachment_id,
'status': 'attached',
'instance': uuids.instance_id,
'volume_id': uuids.volume_id,
'attached_at': timeutils.utcnow(),
'instance': fake_attachment.instance,
'volume_id': fake_attachment.volume_id,
'attached_at': fake_attachment.attached_at,
'detached_at': None,
'attach_mode': 'rw',
'connection_info': {'data': {'foo': 'bar'}}
}
fake_attachment = mock.Mock(
autospec='cinderclient.v3.attachments.VolumeAttachment', **values)
'connection_info': {'driver_volume_type': 'fake_type',
'data': {'foo': 'bar',
'target_lun': '1'}}}
mock_cinderclient.return_value.attachments.update.return_value = (
fake_attachment)
result = self.api.attachment_update(
self.ctx, uuids.attachment_id, connector={'host': 'fake-host'})
self.assertEqual(fake_attachment, result)
self.assertEqual(expected_attachment_ref, result)
@mock.patch('nova.volume.cinder.cinderclient')
def test_attachment_update_attachment_not_found(self, mock_cinderclient):

40
nova/volume/cinder.py
View File

@ -213,6 +213,20 @@ def _untranslate_snapshot_summary_view(context, snapshot):
return d
def _translate_attachment_ref(attachment_ref):
"""Building old style connection_info by adding the 'data' key back."""
connection_info = attachment_ref.pop('connection_info', {})
attachment_ref['connection_info'] = {}
if connection_info.get('driver_volume_type'):
attachment_ref['connection_info']['driver_volume_type'] = \
connection_info['driver_volume_type']
attachment_ref['connection_info']['data'] = {}
for k, v in connection_info.items():
if k != "driver_volume_type":
attachment_ref['connection_info']['data'][k] = v
return attachment_ref
def translate_cinder_exception(method):
"""Transforms a cinder exception but keeps its traceback intact."""
@functools.wraps(method)
@ -531,22 +545,14 @@ class API(object):
attached.
:param connector: host connector dict; if None, the attachment will
be 'reserved' but not yet attached.
:returns: cinderclient.v3.attachments.VolumeAttachment object
representing the new volume attachment with attributes::
'id': attachment.id, # this is a uuid
'status': attachment.attach_status,
'instance': attachment.instance_uuid,
'volume_id': attachment.volume_id,
'attached_at': cls._normalize(attachment.attach_time),
'detached_at': cls._normalize(attachment.detach_time),
'attach_mode': attachment.attach_mode,
'connection_info': \
getattr(attachment, 'connection_info', None)
:returns: a dict created from the
cinderclient.v3.attachments.VolumeAttachment object with a backward
compatible connection_info dict
"""
try:
return cinderclient(context, '3.27').attachments.create(
attachment_ref = cinderclient(context, '3.27').attachments.create(
volume_id, connector, instance_id)
return _translate_attachment_ref(attachment_ref)
except cinder_exception.ClientException as ex:
with excutils.save_and_reraise_exception():
LOG.error(('Create attachment failed for volume '
@ -566,13 +572,15 @@ class API(object):
:param connector: host connector dict. This is required when updating
a volume attachment. To terminate a connection, the volume
attachment for that connection must be deleted.
:returns: cinderclient.v3.attachments.VolumeAttachment object
representing the updated volume attachment.
:returns: a dict created from the
cinderclient.v3.attachments.VolumeAttachment object with a backward
compatible connection_info dict
"""
try:
return cinderclient(
attachment_ref = cinderclient(
context, '3.27', skip_version_check=True).attachments.update(
attachment_id, connector)
return _translate_attachment_ref(attachment_ref.to_dict())
except cinder_exception.ClientException as ex:
with excutils.save_and_reraise_exception():
LOG.error(('Update attachment failed for attachment '