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