Skip to content

block_device: Use the cinder attachment connection_info on detach #655

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
leust wants to merge 1 commit into
base: stable/2023.2-m3
Choose a base branch
from
Draft

block_device: Use the cinder attachment connection_info on detach #655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 101 additions & 0 deletions nova/tests/unit/virt/test_block_device.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -571,6 +571,107 @@ def test_volume_delete_attachment_raises_attachment_not_found(self):
delete_attachment_raises=exception.VolumeAttachmentNotFound(
attachment_id=uuids.attachment_id))

def _test_detach_connection_info(
self,
expected_connection_info,
attachment_connection_info=None,
attachment_get_side_effect=None,
bdm_connection_info=None,
attachment_id=ATTACHMENT_ID,
):
"""Detach a volume from the local host and assert that the virt
driver was called with the expected connection_info.
"""
self.flags(host='fake-host')
instance = fake_instance.fake_instance_obj(
self.context, host='fake-host', uuid=uuids.uuid)
driver_bdm = self.driver_classes['volume'](self.volume_bdm)
driver_bdm['attachment_id'] = attachment_id
if bdm_connection_info is not None:
driver_bdm['connection_info'] = bdm_connection_info
volume = {'id': driver_bdm.volume_id,
'attach_status': 'attached',
'status': 'in-use'}
self.virt_driver.get_volume_connector.return_value = {
'ip': 'fake_ip', 'host': 'fake-host'}
self.volume_api.attachment_get.side_effect = attachment_get_side_effect
self.volume_api.attachment_get.return_value = {
'connection_info': attachment_connection_info}

with test.nested(
mock.patch.object(driver_bdm, '_get_volume', return_value=volume),
mock.patch('os_brick.initiator.utils.guard_connection'),
mock.patch.object(encryptors, 'get_encryption_metadata',
return_value={}),
):
driver_bdm.detach(self.context, instance, self.volume_api,
self.virt_driver, attachment_id=attachment_id)

self.assertEqual(expected_connection_info,
driver_bdm['connection_info'])
self.virt_driver.detach_volume.assert_called_once_with(
self.context, expected_connection_info, instance,
driver_bdm['mount_device'], encryption={})

def test_detach_uses_cinder_connection_info(self):
"""The connection_info of the attachment wins over the possibly stale
one of the BDM, e.g. when the volume got migrated to another backend.
"""
self._test_detach_connection_info(
bdm_connection_info={'driver_volume_type': 'vmdk',
'data': {'volume': 'vm-1'}},
attachment_connection_info={'driver_volume_type': 'fcd',
'data': {'id': 'fcd-1'}},
expected_connection_info={
'driver_volume_type': 'fcd',
'data': {'id': 'fcd-1'},
'serial': 'fake-volume-id-1'})

def test_detach_preserves_local_connection_info(self):
"""The multipath_id is only known to this host and the multiattach flag
is only stashed in the BDM, so both survive the refresh.
"""
self._test_detach_connection_info(
bdm_connection_info={'driver_volume_type': 'iscsi',
'multiattach': True,
'data': {'multipath_id': 'fake-multipath-id',
'target_lun': 0}},
attachment_connection_info={'driver_volume_type': 'iscsi',
'data': {'target_lun': 1}},
expected_connection_info={
'driver_volume_type': 'iscsi',
'multiattach': True,
'data': {'target_lun': 1,
'multipath_id': 'fake-multipath-id'},
'serial': 'fake-volume-id-1'})

def test_detach_falls_back_to_bdm_when_attachment_get_fails(self):
self._test_detach_connection_info(
bdm_connection_info={'driver_volume_type': 'vmdk',
'data': {'volume': 'vm-1'}},
attachment_get_side_effect=exception.VolumeAttachmentNotFound(
attachment_id=ATTACHMENT_ID),
expected_connection_info={'driver_volume_type': 'vmdk',
'data': {'volume': 'vm-1'}})

def test_detach_falls_back_to_bdm_without_attachment_connection_info(self):
self._test_detach_connection_info(
bdm_connection_info={'driver_volume_type': 'vmdk',
'data': {'volume': 'vm-1'}},
attachment_connection_info=None,
expected_connection_info={'driver_volume_type': 'vmdk',
'data': {'volume': 'vm-1'}})

def test_detach_without_attachment_id_does_not_query_cinder(self):
"""The legacy attach flow has no attachment record to ask."""
self._test_detach_connection_info(
attachment_id=None,
bdm_connection_info={'driver_volume_type': 'vmdk',
'data': {'volume': 'vm-1'}},
expected_connection_info={'driver_volume_type': 'vmdk',
'data': {'volume': 'vm-1'}})
self.volume_api.attachment_get.assert_not_called()

@mock.patch.object(encryptors, 'get_encryption_metadata')
@mock.patch.object(driver_block_device,
'_get_volume_create_scheduler_hints',
Expand Down
101 changes: 88 additions & 13 deletions nova/virt/block_device.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,88 @@ def _preserve_multipath_id(self, connection_info):
LOG.info('preserve multipath_id %s',
connection_info['data']['multipath_id'])

def _get_attachment_connection_info(self, context, volume_api):
"""Return the connection_info cinder has for our attachment.

:param context: nova auth RequestContext
:param volume_api: nova.volume.cinder.API instance
:returns: the connection_info dict of the attachment record
"""
attachment_ref = volume_api.attachment_get(context,
self['attachment_id'])
connection_info = attachment_ref['connection_info']
# The _volume_attach method stashes a 'multiattach' flag in the
# BlockDeviceMapping.connection_info which is not persisted back in
# cinder, so before we overwrite the BDM.connection_info we need to
# make sure and preserve the multiattach flag if it's set. Note that
# this is safe to do across refreshes because the multiattach
# capability of a volume cannot be changed while the volume is in-use.
if connection_info and self['connection_info'].get('multiattach',
False):
connection_info['multiattach'] = True
return connection_info

def _refresh_connection_info_for_detach(self, context, instance,
volume_api):
"""Update our connection_info with what cinder currently has.

Cinder is the authoritative source for the connection_info, while the
copy stashed in the BDM can be stale, e.g. after the volume was
migrated to another backend, in which case even the driver_volume_type
may have changed. Nova only learns about such a change when it talks to
cinder, so we refresh the data before handing it over to the virt
driver for the detach.

This is best-effort: if we cannot get usable data from cinder, we keep
what we have in the BDM and let the detach continue.
"""
if not self['attachment_id'] or not self['connection_info']:
# Either the legacy attach flow, where there is no attachment
# record to ask, or nothing was ever connected.
return

try:
connection_info = self._get_attachment_connection_info(
context, volume_api)
except Exception:
LOG.warning('Failed to get the connection_info of attachment '
'%(attachment_id)s for volume %(volume_id)s. Using '
'the connection_info of the block device mapping.',
{'attachment_id': self['attachment_id'],
'volume_id': self.volume_id},
exc_info=True, instance=instance)
return

if not connection_info:
LOG.debug('Attachment %(attachment_id)s of volume %(volume_id)s '
'has no connection_info. Using the connection_info of '
'the block device mapping.',
{'attachment_id': self['attachment_id'],
'volume_id': self.volume_id}, instance=instance)
return

if 'serial' not in connection_info:
connection_info['serial'] = self.volume_id
# The multipath_id is found by os-brick on this host during the attach
# and is unknown to cinder, so it needs to survive the refresh.
self._preserve_multipath_id(connection_info)

# Only the driver_volume_type and the data are relevant for the
# detach, the rest of the attachment record (e.g. its status) is
# expected to have changed since the attach.
stale = self['connection_info']
if (connection_info.get('driver_volume_type') !=
stale.get('driver_volume_type') or
connection_info.get('data') != stale.get('data')):
LOG.info('The connection_info of the block device mapping for '
'volume %(volume_id)s differs from the one of attachment '
'%(attachment_id)s. Using the latter for the detach.',
{'volume_id': self.volume_id,
'attachment_id': self['attachment_id']},
instance=instance)

self['connection_info'] = connection_info

def driver_detach(self, context, instance, volume_api, virt_driver):
connection_info = self['connection_info']
mp = self['mount_device']
Expand Down Expand Up @@ -470,6 +552,10 @@ def _do_detach(self, context, instance, volume_api, virt_driver,
# Only attempt to detach and disconnect from the volume if the instance
# is currently associated with the local compute host.
if CONF.host == instance.host:
# The connection_info in the BDM may have gone stale since the
# attach, so use the data cinder has for the attachment.
self._refresh_connection_info_for_detach(context, instance,
volume_api)
self.driver_detach(context, instance, volume_api, virt_driver)
elif not destroy_bdm:
LOG.debug("Skipping driver_detach during remote rebuild.",
Expand Down Expand Up @@ -761,19 +847,8 @@ def refresh_connection_info(self, context, instance,
self.volume_id,
connector)
else:
attachment_ref = volume_api.attachment_get(context,
self['attachment_id'])
# The _volume_attach method stashes a 'multiattach' flag in the
# BlockDeviceMapping.connection_info which is not persisted back
# in cinder so before we overwrite the BDM.connection_info (via
# the update_db decorator on this method), we need to make sure
# and preserve the multiattach flag if it's set. Note that this
# is safe to do across refreshes because the multiattach capability
# of a volume cannot be changed while the volume is in-use.
multiattach = self['connection_info'].get('multiattach', False)
connection_info = attachment_ref['connection_info']
if multiattach:
connection_info['multiattach'] = True
connection_info = self._get_attachment_connection_info(
context, volume_api)

if 'serial' not in connection_info:
connection_info['serial'] = self.volume_id
Expand Down