openstack/nova
Code Issues Proposed changes

Pass mountpoint to volume attachment_update

Browse Source 
The old volume attach flow would pass the mountpoint,
which is the BlockDeviceMapping.device_name, to the
os-attach volume action API. With the new flow, apparently
the mountpoint is expected to be passed to the update
attachment API via the connector dict, which is not obvious
and if not provided, causes Cinder to default the mountpoint
to "/dev/na" which is wrong.

We work around this in Nova by providing the mountpoint in a
copy of the connector dict and pass that to attachment_update,
and update the two places in the code that are updating an
attachment (the normal driver block device attach code and
swap volume). Long-term this should be fixed in the Cinder
attachments update API, but that requires a microversion so
we need to handle it client-side for now.

Change-Id: I11ba269c3f7a2e7707b2b7e27d26eb7a2c948a82
Partial-Bug: #1737779
This commit is contained in:
Matt Riedemann
2017-12-12 12:09:50 -05:00
parent 32c8ac6b7d
commit 16d0ad344e
7 changed files with 82 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
nova/compute/manager.py
View File

@@ -5292,7 +5292,7 @@ class ComputeManager(manager.Manager):
def _init_volume_connection(self, context, new_volume_id,
old_volume_id, connector, bdm,
new_attachment_id):
new_attachment_id, mountpoint):
if new_attachment_id is None:
# We're dealing with an old-style attachment so initialize the
@@ -5307,7 +5307,8 @@ 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,
mountpoint)['connection_info']
old_cinfo = jsonutils.loads(bdm['connection_info'])
if old_cinfo and 'serial' not in old_cinfo:
@@ -5328,7 +5329,7 @@ class ComputeManager(manager.Manager):
try:
old_cinfo, new_cinfo = self._init_volume_connection(
context, new_volume_id, old_volume_id, connector,
bdm, new_attachment_id)
bdm, new_attachment_id, mountpoint)
# NOTE(lyarwood): The Libvirt driver, the only virt driver
# currently implementing swap_volume, will modify the contents of
# new_cinfo when connect_volume is called. This is then saved to

3
nova/tests/fixtures.py
View File

@@ -1592,7 +1592,8 @@ class CinderFixtureNewAttachFlow(fixtures.Fixture):
if attachment_id == CinderFixtureNewAttachFlow.SWAP_ERR_ATTACH_ID:
self.swap_error = True
def fake_attachment_update(_self, context, attachment_id, connector):
def fake_attachment_update(_self, context, attachment_id, connector,
mountpoint=None):
attachment_ref = {'driver_volume_type': 'fake_type',
'id': attachment_id,
'connection_info': {'data':

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

@@ -2092,7 +2092,8 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase):
self.context, uuids.old_volume_id, instance.uuid)
# We updated the new attachment with the host connector.
attachment_update.assert_called_once_with(
self.context, uuids.new_attachment_id, mock.sentinel.connector)
self.context, uuids.new_attachment_id, mock.sentinel.connector,
bdm.device_name)
# We tell Cinder that the new volume is connected
attachment_complete.assert_called_once_with(
self.context, uuids.new_attachment_id)
@@ -2162,7 +2163,8 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase):
self.context, uuids.old_volume_id, instance.uuid)
# We updated the new attachment with the host connector.
attachment_update.assert_called_once_with(
self.context, uuids.new_attachment_id, mock.sentinel.connector)
self.context, uuids.new_attachment_id, mock.sentinel.connector,
bdm.device_name)
# We tell Cinder that the new volume is connected
attachment_complete.assert_called_once_with(
self.context, uuids.new_attachment_id)
@@ -2231,7 +2233,8 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase):
self.context, uuids.old_volume_id, instance.uuid)
# We tried to update the new attachment with the host connector.
attachment_update.assert_called_once_with(
self.context, uuids.new_attachment_id, mock.sentinel.connector)
self.context, uuids.new_attachment_id, mock.sentinel.connector,
bdm.device_name)
# After a failure, we rollback the detaching status of the old
# volume.
roll_detaching.assert_called_once_with(
@@ -2300,7 +2303,8 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase):
self.context, uuids.old_volume_id, instance.uuid)
# We updated the new attachment with the host connector.
attachment_update.assert_called_once_with(
self.context, uuids.new_attachment_id, mock.sentinel.connector)
self.context, uuids.new_attachment_id, mock.sentinel.connector,
bdm.device_name)
# After a failure, we rollback the detaching status of the old
# volume.
roll_detaching.assert_called_once_with(

3
nova/tests/unit/virt/test_block_device.py
View File

@@ -462,7 +462,8 @@ class TestDriverBlockDevice(test.NoDBTestCase):
connector).AndReturn(connection_info)
else:
self.volume_api.attachment_update(
elevated_context, self.attachment_id, connector).AndReturn(
elevated_context, self.attachment_id, connector,
bdm_dict['device_name']).AndReturn(
{'connection_info': connection_info})
if driver_attach:

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

@@ -337,15 +337,18 @@ 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."""
"""Tests the happy path for updating a volume attachment without
a mountpoint.
"""
fake_attachment = FakeAttachment()
connector = {'host': 'fake-host'}
expected_attachment_ref = {
'id': uuids.attachment_id,
'volume_id': fake_attachment.volume_id,
'connection_info': {
'attach_mode': 'rw',
'attached_at': fake_attachment.attached_at,
'connector': {'host': 'fake-host'},
'connector': connector,
'data': {'foo': 'bar', 'target_lun': '1'},
'detached_at': None,
'driver_volume_type': 'fake_type',
@@ -355,8 +358,45 @@ class CinderApiTestCase(test.NoDBTestCase):
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.ctx, uuids.attachment_id, connector=connector)
self.assertEqual(expected_attachment_ref, result)
# Make sure the connector wasn't modified.
self.assertNotIn('mountpoint', connector)
mock_cinderclient.return_value.attachments.update.\
assert_called_once_with(uuids.attachment_id, connector)
@mock.patch('nova.volume.cinder.cinderclient')
def test_attachment_update_with_mountpoint(self, mock_cinderclient):
"""Tests the happy path for updating a volume attachment with
a mountpoint.
"""
fake_attachment = FakeAttachment()
original_connector = {'host': 'fake-host'}
updated_connector = dict(original_connector, mountpoint='/dev/vdb')
expected_attachment_ref = {
'id': uuids.attachment_id,
'volume_id': fake_attachment.volume_id,
'connection_info': {
'attach_mode': 'rw',
'attached_at': fake_attachment.attached_at,
'connector': updated_connector,
'data': {'foo': 'bar', 'target_lun': '1'},
'detached_at': None,
'driver_volume_type': 'fake_type',
'instance': fake_attachment.instance,
'status': 'attaching',
'volume_id': fake_attachment.volume_id}}
mock_cinderclient.return_value.attachments.update.return_value = (
fake_attachment)
result = self.api.attachment_update(
self.ctx, uuids.attachment_id, connector=original_connector,
mountpoint='/dev/vdb')
self.assertEqual(expected_attachment_ref, result)
# Make sure the original connector wasn't modified.
self.assertNotIn('mountpoint', original_connector)
# Make sure the mountpoint was passed through via the connector.
mock_cinderclient.return_value.attachments.update.\
assert_called_once_with(uuids.attachment_id, updated_connector)
@mock.patch('nova.volume.cinder.cinderclient')
def test_attachment_update_attachment_not_found(self, mock_cinderclient):

3
nova/virt/block_device.py
View File

@@ -442,7 +442,8 @@ class DriverVolumeBlockDevice(DriverBlockDevice):
LOG.debug("Updating existing volume attachment record: %s",
attachment_id, instance=instance)
connection_info = volume_api.attachment_update(
context, attachment_id, connector)['connection_info']
context, attachment_id, connector,
self['mount_device'])['connection_info']
if 'serial' not in connection_info:
connection_info['serial'] = self.volume_id
self._preserve_multipath_id(connection_info)

24
nova/volume/cinder.py
View File

@@ -614,7 +614,8 @@ class API(object):
'code': getattr(ex, 'code', None)})
@translate_attachment_exception
def attachment_update(self, context, attachment_id, connector):
def attachment_update(self, context, attachment_id, connector,
mountpoint=None):
"""Updates the connector on the volume attachment. An attachment
without a connector is considered reserved but not fully attached.
@@ -623,17 +624,34 @@ 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.
:param mountpoint: Optional mount device name for the attachment,
e.g. "/dev/vdb". Theoretically this is optional per volume backend,
but in practice it's normally required so it's best to always
provide a value.
:returns: a dict created from the
cinderclient.v3.attachments.VolumeAttachment object with a backward
compatible connection_info dict
"""
# NOTE(mriedem): Due to a limitation in the PUT /attachments/{id}
# API in Cinder, we have to pass the mountpoint in via the
# host connector rather than pass it in as a top-level parameter
# like in the os-attach volume action API. Hopefully this will be
# fixed some day with a new Cinder microversion but until then we
# work around it client-side.
_connector = connector
if mountpoint and 'mountpoint' not in connector:
# Make a copy of the connector so we don't modify it by
# reference.
_connector = copy.deepcopy(connector)
_connector['mountpoint'] = mountpoint
try:
attachment_ref = cinderclient(
context, '3.44', skip_version_check=True).attachments.update(
attachment_id, connector)
attachment_id, _connector)
translated_attach_ref = _translate_attachment_ref(
attachment_ref.to_dict())
translated_attach_ref['connection_info']['connector'] = connector
translated_attach_ref['connection_info']['connector'] = _connector
return translated_attach_ref
except cinder_exception.ClientException as ex:
with excutils.save_and_reraise_exception():