diff --git a/nova/compute/manager.py b/nova/compute/manager.py index 63c12a9bc523..930f396293d3 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -4543,8 +4543,11 @@ class ComputeManager(manager.Manager): # or if we did claim but the spawn failed, because aborting the # instance claim will not remove the allocations. rt.reportclient.delete_allocation_for_instance(instance.uuid) - # FIXME: Umm, shouldn't we be rolling back volume connections - # and port bindings? + # FIXME: Umm, shouldn't we be rolling back port bindings too? + self._terminate_volume_connections(context, instance, bdms) + # The reverts_task_state decorator on unshelve_instance will + # eventually save these updates. + self._nil_out_instance_obj_host_and_node(instance) if image: instance.image_ref = shelved_image_ref diff --git a/nova/tests/unit/compute/test_shelve.py b/nova/tests/unit/compute/test_shelve.py index af5319ef9f09..2a2b0d1219bf 100644 --- a/nova/tests/unit/compute/test_shelve.py +++ b/nova/tests/unit/compute/test_shelve.py @@ -430,6 +430,91 @@ class ShelveComputeManagerTestCase(test_compute.BaseTestCase): block_device_info='fake_bdm') mock_get_power_state.assert_called_once_with(self.context, instance) + @mock.patch('nova.objects.BlockDeviceMappingList.get_by_instance_uuid') + @mock.patch('nova.compute.utils.notify_about_instance_action') + @mock.patch.object(nova.compute.resource_tracker.ResourceTracker, + 'instance_claim') + @mock.patch.object(neutron_api.API, 'setup_instance_network_on_host') + @mock.patch.object(nova.virt.fake.SmallFakeDriver, 'spawn', + side_effect=test.TestingException('oops!')) + @mock.patch.object(nova.compute.manager.ComputeManager, + '_prep_block_device', return_value='fake_bdm') + @mock.patch.object(nova.compute.manager.ComputeManager, + '_notify_about_instance_usage') + @mock.patch('nova.utils.get_image_from_system_metadata') + @mock.patch.object(nova.compute.manager.ComputeManager, + '_terminate_volume_connections') + def test_unshelve_spawn_fails_cleanup_volume_connections( + self, mock_terminate_volume_connections, mock_image_meta, + mock_notify_instance_usage, mock_prep_block_device, mock_spawn, + mock_setup_network, mock_instance_claim, + mock_notify_instance_action, mock_get_bdms): + """Tests error handling when a instance fails to unshelve and makes + sure that volume connections are cleaned up from the host + and that the host/node values are unset on the instance. + """ + mock_bdms = mock.Mock() + mock_get_bdms.return_value = mock_bdms + instance = self._create_fake_instance_obj() + node = test_compute.NODENAME + limits = {} + filter_properties = {'limits': limits} + instance.task_state = task_states.UNSHELVING + instance.save() + image_meta = {'properties': {'base_image_ref': uuids.image_id}} + mock_image_meta.return_value = image_meta + + tracking = {'last_state': instance.task_state} + + def fake_claim(context, instance, node, limits): + instance.host = self.compute.host + instance.node = node + requests = objects.InstancePCIRequests(requests=[]) + return claims.Claim(context, instance, node, + self.rt, _fake_resources(), + requests, limits=limits) + mock_instance_claim.side_effect = fake_claim + + def check_save(expected_task_state=None): + if tracking['last_state'] == task_states.UNSHELVING: + # This is before we've failed. + self.assertEqual(task_states.SPAWNING, instance.task_state) + tracking['last_state'] = instance.task_state + elif tracking['last_state'] == task_states.SPAWNING: + # This is after we've failed. + self.assertIsNone(instance.host) + self.assertIsNone(instance.node) + self.assertIsNone(instance.task_state) + tracking['last_state'] = instance.task_state + else: + self.fail('Unexpected save!') + + with mock.patch.object(instance, 'save') as mock_save: + mock_save.side_effect = check_save + self.assertRaises(test.TestingException, + self.compute.unshelve_instance, + self.context, instance, image=None, + filter_properties=filter_properties, node=node) + + mock_notify_instance_action.assert_called_once_with( + self.context, instance, 'fake-mini', action='unshelve', + phase='start') + mock_notify_instance_usage.assert_called_once_with( + self.context, instance, 'unshelve.start') + mock_prep_block_device.assert_called_once_with( + self.context, instance, mock_bdms) + mock_setup_network.assert_called_once_with(self.context, instance, + self.compute.host) + mock_instance_claim.assert_called_once_with(self.context, instance, + test_compute.NODENAME, + limits) + mock_spawn.assert_called_once_with( + self.context, instance, test.MatchType(objects.ImageMeta), + injected_files=[], admin_password=None, + network_info=[], block_device_info='fake_bdm') + mock_terminate_volume_connections.assert_called_once_with( + self.context, instance, mock_bdms) + @mock.patch.object(objects.InstanceList, 'get_by_filters') def test_shelved_poll_none_offloaded(self, mock_get_by_filters): # Test instances are not offloaded when shelved_offload_time is -1