From 368d0d1e894472bd088ae3729fbc46f223045094 Mon Sep 17 00:00:00 2001 From: Marius Leustean Date: Fri, 14 Aug 2026 18:33:34 +0300 Subject: [PATCH] block_device: Use the cinder attachment connection_info on detach Nova detaches a volume with the connection_info it stashed in the BDM at attach time. Cinder is the authoritative source for that data, so the BDM copy can be stale by the time we detach. The case we hit in production is a volume that was migrated to the FCD driver behind nova's back: the BDM still says driver_volume_type 'vmdk' while cinder says 'fcd'. VMwareVolumeOps.detach_volume dispatches on driver_volume_type, so nova only notices the migration once the detach fails. Refresh the connection_info from the cinder attachment record before handing it to the virt driver. The multipath_id that os-brick found on this host and the multiattach flag are known to nova only, so they are preserved across the refresh. This is best-effort: if the attachment cannot be read or carries no connection_info, we keep using the BDM data as before, so an unreachable cinder does not turn into a failed detach. Only the local detach path is changed. driver_detach() as called from _remove_volume_connection() is left alone, because during live migration the BDM attachment is swapped between source and destination and refreshing there could hand a host the other host's connection_info. Change-Id: I662960d9a72057a6e976300314038001da2ff54e --- nova/tests/unit/virt/test_block_device.py | 101 ++++++++++++++++++++++ nova/virt/block_device.py | 101 +++++++++++++++++++--- 2 files changed, 189 insertions(+), 13 deletions(-) diff --git a/nova/tests/unit/virt/test_block_device.py b/nova/tests/unit/virt/test_block_device.py index 3910e46f14c..33783733791 100644 --- a/nova/tests/unit/virt/test_block_device.py +++ b/nova/tests/unit/virt/test_block_device.py @@ -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', diff --git a/nova/virt/block_device.py b/nova/virt/block_device.py index f67e7bede00..f4080a58659 100644 --- a/nova/virt/block_device.py +++ b/nova/virt/block_device.py @@ -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'] @@ -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.", @@ -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