diff --git a/nova/compute/manager.py b/nova/compute/manager.py index 1c4c2114894..f0efbe02ec1 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -4753,6 +4753,8 @@ def _driver_detach_volume(self, context, instance, bdm): context=context, instance=instance) self.volume_api.roll_detaching(context, volume_id) + return connection_info + def _detach_volume(self, context, volume_id, instance, destroy_bdm=True, attachment_id=None): """Detach a volume from an instance. @@ -4797,8 +4799,46 @@ def _detach_volume(self, context, volume_id, instance, destroy_bdm=True, self.notifier.info(context, 'volume.usage', compute_utils.usage_volume_info(vol_usage)) - self._driver_detach_volume(context, instance, bdm) + connection_info = self._driver_detach_volume(context, instance, bdm) connector = self.driver.get_volume_connector(instance) + + if connection_info and not destroy_bdm and ( + connector.get('host') != instance.host): + # If the volume is attached to another host (evacuate) then + # this connector is for the wrong host. Use the connector that + # was stored in connection_info instead (if we have one, and it + # is for the expected host). + stashed_connector = connection_info.get('connector') + if not stashed_connector: + # Volume was attached before we began stashing connectors + LOG.warning(_LW("Host mismatch detected, but stashed " + "volume connector not found. Instance host is " + "%(ihost)s, but volume connector host is " + "%(chost)s."), + {'ihost': instance.host, + 'chost': connector.get('host')}) + elif stashed_connector.get('host') != instance.host: + # Unexpected error. The stashed connector is also not matching + # the needed instance host. + LOG.error(_LE("Host mismatch detected in stashed volume " + "connector. Will use local volume connector. " + "Instance host is %(ihost)s. Local volume " + "connector host is %(chost)s. Stashed volume " + "connector host is %(schost)s."), + {'ihost': instance.host, + 'chost': connector.get('host'), + 'schost': stashed_connector.get('host')}) + else: + # Fix found. Use stashed connector. + LOG.debug("Host mismatch detected. Found usable stashed " + "volume connector. Instance host is %(ihost)s. " + "Local volume connector host was %(chost)s. " + "Stashed volume connector host is %(schost)s.", + {'ihost': instance.host, + 'chost': connector.get('host'), + 'schost': stashed_connector.get('host')}) + connector = stashed_connector + self.volume_api.terminate_connection(context, volume_id, connector) if destroy_bdm: diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index 58616e81383..487da4156cf 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -23,6 +23,7 @@ import netaddr from oslo_config import cfg import oslo_messaging as messaging +from oslo_serialization import jsonutils from oslo_utils import importutils from oslo_utils import timeutils from oslo_utils import uuidutils @@ -2199,6 +2200,8 @@ def _test_detach_volume(self, notify_inst_usage, detach, bdm.device_name = 'vdb' bdm_get.return_value = bdm + detach.return_value = {} + with mock.patch.object(self.compute, 'volume_api') as volume_api: with mock.patch.object(self.compute, 'driver') as driver: connector_sentinel = mock.sentinel.connector @@ -2226,6 +2229,93 @@ def _test_detach_volume(self, notify_inst_usage, detach, else: self.assertFalse(bdm.destroy.called) + def test_detach_volume_evacuate(self): + """For evacuate, terminate_connection is called with original host.""" + expected_connector = {'host': 'evacuated-host'} + conn_info_str = '{"connector": {"host": "evacuated-host"}}' + self._test_detach_volume_evacuate(conn_info_str, + expected=expected_connector) + + def test_detach_volume_evacuate_legacy(self): + """Test coverage for evacuate with legacy attachments. + + In this case, legacy means the volume was attached to the instance + before nova stashed the connector in connection_info. The connector + sent to terminate_connection will still be for the local host in this + case because nova does not have the info to get the connector for the + original (evacuated) host. + """ + conn_info_str = '{"foo": "bar"}' # Has no 'connector'. + self._test_detach_volume_evacuate(conn_info_str) + + def test_detach_volume_evacuate_mismatch(self): + """Test coverage for evacuate with connector mismatch. + + For evacuate, if the stashed connector also has the wrong host, + then log it and stay with the local connector. + """ + conn_info_str = '{"connector": {"host": "other-host"}}' + self._test_detach_volume_evacuate(conn_info_str) + + @mock.patch('nova.objects.BlockDeviceMapping.get_by_volume_and_instance') + @mock.patch('nova.compute.manager.ComputeManager.' + '_notify_about_instance_usage') + def _test_detach_volume_evacuate(self, conn_info_str, notify_inst_usage, + bdm_get, expected=None): + """Re-usable code for detach volume evacuate test cases. + + :param conn_info_str: String form of the stashed connector. + :param expected: Dict of the connector that is expected in the + terminate call (optional). Default is to expect the + local connector to be used. + """ + volume_id = 'vol_id' + instance = fake_instance.fake_instance_obj(self.context, + host='evacuated-host') + bdm = mock.Mock() + bdm.connection_info = conn_info_str + bdm_get.return_value = bdm + + local_connector = {'host': 'local-connector-host'} + expected_connector = local_connector if not expected else expected + + with mock.patch.object(self.compute, 'volume_api') as volume_api: + with mock.patch.object(self.compute, 'driver') as driver: + driver.get_volume_connector.return_value = local_connector + + self.compute._detach_volume(self.context, + volume_id, + instance, + destroy_bdm=False) + + driver.get_volume_connector.assert_called_once_with(instance) + volume_api.terminate_connection.assert_called_once_with( + self.context, volume_id, expected_connector) + volume_api.detach.assert_called_once_with(mock.ANY, + volume_id, + instance.uuid, + None) + notify_inst_usage.assert_called_once_with( + self.context, instance, "volume.detach", + extra_usage_info={'volume_id': volume_id} + ) + + def test__driver_detach_volume_return(self): + """_driver_detach_volume returns the connection_info from loads().""" + with mock.patch.object(jsonutils, 'loads') as loads: + conn_info_str = 'test-expected-loads-param' + bdm = mock.Mock() + bdm.connection_info = conn_info_str + loads.return_value = {'test-loads-key': 'test loads return value'} + instance = fake_instance.fake_instance_obj(self.context) + + ret = self.compute._driver_detach_volume(self.context, + instance, + bdm) + + self.assertEqual(loads.return_value, ret) + loads.assert_called_once_with(conn_info_str) + def _test_rescue(self, clean_shutdown=True): instance = fake_instance.fake_instance_obj( self.context, vm_state=vm_states.ACTIVE) diff --git a/nova/tests/unit/volume/test_cinder.py b/nova/tests/unit/volume/test_cinder.py index 6c86cdcbceb..10e09f1e865 100644 --- a/nova/tests/unit/volume/test_cinder.py +++ b/nova/tests/unit/volume/test_cinder.py @@ -289,15 +289,22 @@ def test_detach(self): self.api.detach(self.ctx, 'id1', instance_uuid='fake_uuid') - def test_initialize_connection(self): - cinder.cinderclient(self.ctx).AndReturn(self.cinderclient) - self.mox.StubOutWithMock(self.cinderclient.volumes, - 'initialize_connection', - use_mock_anything=True) - self.cinderclient.volumes.initialize_connection('id1', 'connector') - self.mox.ReplayAll() + @mock.patch('nova.volume.cinder.cinderclient') + def test_initialize_connection(self, mock_cinderclient): + connection_info = {'foo': 'bar'} + mock_cinderclient.return_value.volumes. \ + initialize_connection.return_value = connection_info + + volume_id = 'fake_vid' + connector = {'host': 'fakehost1'} + actual = self.api.initialize_connection(self.ctx, volume_id, connector) + + expected = connection_info + expected['connector'] = connector + self.assertEqual(expected, actual) - self.api.initialize_connection(self.ctx, 'id1', 'connector') + mock_cinderclient.return_value.volumes. \ + initialize_connection.assert_called_once_with(volume_id, connector) @mock.patch('nova.volume.cinder.cinderclient') def test_initialize_connection_rollback(self, mock_cinderclient): diff --git a/nova/volume/cinder.py b/nova/volume/cinder.py index 8aa56f184bf..4c09e3e3a41 100644 --- a/nova/volume/cinder.py +++ b/nova/volume/cinder.py @@ -395,8 +395,10 @@ def detach(self, context, volume_id, instance_uuid=None, @translate_volume_exception def initialize_connection(self, context, volume_id, connector): try: - return cinderclient(context).volumes.initialize_connection( - volume_id, connector) + connection_info = cinderclient( + context).volumes.initialize_connection(volume_id, connector) + connection_info['connector'] = connector + return connection_info except cinder_exception.ClientException as ex: with excutils.save_and_reraise_exception(): LOG.error(_LE('Initialize connection failed for volume '