diff --git a/nova/conductor/manager.py b/nova/conductor/manager.py index fa9ac3ac960d..716e67202d60 100644 --- a/nova/conductor/manager.py +++ b/nova/conductor/manager.py @@ -307,9 +307,6 @@ class ComputeTaskManager(base.Base): task = self._build_cold_migrate_task(context, instance, flavor, request_spec, reservations, clean_shutdown) - # TODO(sbauza): Provide directly the RequestSpec object once - # _set_vm_state_and_notify() accepts it - legacy_spec = request_spec.to_legacy_request_spec_dict() try: task.execute() except exception.NoValidHost as ex: @@ -319,7 +316,7 @@ class ComputeTaskManager(base.Base): updates = {'vm_state': vm_state, 'task_state': None} self._set_vm_state_and_notify(context, instance.uuid, 'migrate_server', - updates, ex, legacy_spec) + updates, ex, request_spec) # if the flavor IDs match, it's migrate; otherwise resize if flavor.id == instance.instance_type_id: @@ -335,14 +332,14 @@ class ComputeTaskManager(base.Base): updates = {'vm_state': vm_state, 'task_state': None} self._set_vm_state_and_notify(context, instance.uuid, 'migrate_server', - updates, ex, legacy_spec) + updates, ex, request_spec) except Exception as ex: with excutils.save_and_reraise_exception(): updates = {'vm_state': instance.vm_state, 'task_state': None} self._set_vm_state_and_notify(context, instance.uuid, 'migrate_server', - updates, ex, legacy_spec) + updates, ex, request_spec) # NOTE(sbauza): Make sure we persist the new flavor in case we had # a successful scheduler call if and only if nothing bad happened if request_spec.obj_what_changed(): @@ -540,8 +537,7 @@ class ComputeTaskManager(base.Base): # but if we've exceeded max retries... then we really only # have a single instance. # TODO(sbauza): Provide directly the RequestSpec object - # when _set_vm_state_and_notify() and populate_retry() - # accept it + # when populate_retry() accepts it request_spec = scheduler_utils.build_request_spec( context, image, instances) scheduler_utils.populate_retry( @@ -754,13 +750,6 @@ class ComputeTaskManager(base.Base): context, host, use_slave=True)) except exception.ComputeHostNotFound as ex: with excutils.save_and_reraise_exception(): - # TODO(mriedem): This ugly RequestSpec handling should be - # tucked away in _set_vm_state_and_notify. - if request_spec: - request_spec = \ - request_spec.to_legacy_request_spec_dict() - else: - request_spec = {} self._set_vm_state_and_notify( context, instance.uuid, 'rebuild_server', {'vm_state': instance.vm_state, @@ -784,13 +773,6 @@ class ComputeTaskManager(base.Base): source_node, dest_node) except exception.NoValidHost as ex: with excutils.save_and_reraise_exception(): - # TODO(mriedem): This ugly RequestSpec handling should be - # tucked away in _set_vm_state_and_notify. - if request_spec: - request_spec = \ - request_spec.to_legacy_request_spec_dict() - else: - request_spec = {} self._set_vm_state_and_notify( context, instance.uuid, 'rebuild_server', {'vm_state': instance.vm_state, @@ -843,8 +825,6 @@ class ComputeTaskManager(base.Base): # NOTE(sbauza): We were unable to find an original # RequestSpec object - probably because the instance is old # We need to mock that the old way - # TODO(sbauza): Provide directly the RequestSpec object - # when _set_vm_state_and_notify() accepts it filter_properties = {'ignore_hosts': [instance.host]} request_spec = scheduler_utils.build_request_spec( context, image_ref, [instance]) @@ -870,7 +850,6 @@ class ComputeTaskManager(base.Base): if migration: migration.status = 'error' migration.save() - request_spec = request_spec.to_legacy_request_spec_dict() with excutils.save_and_reraise_exception(): self._set_vm_state_and_notify(context, instance.uuid, 'rebuild_server', @@ -986,7 +965,6 @@ class ComputeTaskManager(base.Base): instances_by_uuid[instance.uuid] = instance updates = {'vm_state': vm_states.ERROR, 'task_state': None} - legacy_spec = request_spec.to_legacy_request_spec_dict() for instance in instances_by_uuid.values(): with obj_target_cell(instance, cell0) as cctxt: instance.create() @@ -994,7 +972,7 @@ class ComputeTaskManager(base.Base): # now in cell0. self._set_vm_state_and_notify( cctxt, instance.uuid, 'build_instances', updates, - exc, legacy_spec) + exc, request_spec) try: # We don't need the cell0-targeted context here because the # instance mapping is in the API DB. @@ -1174,12 +1152,11 @@ class ComputeTaskManager(base.Base): if instance is None: continue updates = {'vm_state': vm_states.ERROR, 'task_state': None} - legacy_spec = request_spec.to_legacy_request_spec_dict() cell = cell_mapping_cache[instance.uuid] with try_target_cell(context, cell) as cctxt: self._set_vm_state_and_notify(cctxt, instance.uuid, 'build_instances', updates, exc, - legacy_spec) + request_spec) # TODO(mnaser): The cell mapping should already be populated by # this point to avoid setting it below here. diff --git a/nova/scheduler/utils.py b/nova/scheduler/utils.py index d0724b281de0..7eeffbe99f6e 100644 --- a/nova/scheduler/utils.py +++ b/nova/scheduler/utils.py @@ -301,34 +301,52 @@ def claim_resources_on_destination( def set_vm_state_and_notify(context, instance_uuid, service, method, updates, ex, request_spec): - """changes VM state and notifies.""" - LOG.warning(_LW("Failed to %(service)s_%(method)s: %(ex)s"), + """Updates the instance, sets the fault and sends an error notification. + + :param context: The request context. + :param instance_uuid: The UUID of the instance to update. + :param service: The name of the originating service, e.g. 'compute_task'. + This becomes part of the publisher_id for the notification payload. + :param method: The method that failed, e.g. 'migrate_server'. + :param updates: dict of updates for the instance object, typically a + vm_state and/or task_state value. + :param ex: An exception which occurred during the given method. + :param request_spec: Optional request spec. + """ + # e.g. "Failed to compute_task_migrate_server: No valid host was found" + LOG.warning("Failed to %(service)s_%(method)s: %(ex)s", {'service': service, 'method': method, 'ex': ex}) + # Convert the request spec to a dict if needed. + if request_spec is not None: + if isinstance(request_spec, objects.RequestSpec): + request_spec = request_spec.to_legacy_request_spec_dict() + else: + request_spec = {} + vm_state = updates['vm_state'] properties = request_spec.get('instance_properties', {}) - # NOTE(vish): We shouldn't get here unless we have a catastrophic - # failure, so just set the instance to its internal state notifier = rpc.get_notifier(service) state = vm_state.upper() - LOG.warning(_LW('Setting instance to %s state.'), state, + LOG.warning('Setting instance to %s state.', state, instance_uuid=instance_uuid) instance = objects.Instance(context=context, uuid=instance_uuid, **updates) instance.obj_reset_changes(['uuid']) instance.save() - compute_utils.add_instance_fault_from_exc(context, - instance, ex, sys.exc_info()) + compute_utils.add_instance_fault_from_exc( + context, instance, ex, sys.exc_info()) payload = dict(request_spec=request_spec, - instance_properties=properties, - instance_id=instance_uuid, - state=vm_state, - method=method, - reason=ex) + instance_properties=properties, + instance_id=instance_uuid, + state=vm_state, + method=method, + reason=ex) event_type = '%s.%s' % (service, method) + # TODO(mriedem): Send a versioned notification. notifier.error(context, event_type, payload) diff --git a/nova/tests/unit/conductor/test_conductor.py b/nova/tests/unit/conductor/test_conductor.py index 0a8d0acf9d11..bfacf1470cef 100644 --- a/nova/tests/unit/conductor/test_conductor.py +++ b/nova/tests/unit/conductor/test_conductor.py @@ -2115,7 +2115,6 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase): image = 'fake-image' fake_spec = objects.RequestSpec(image=objects.ImageMeta()) spec_fc_mock.return_value = fake_spec - legacy_request_spec = fake_spec.to_legacy_request_spec_dict() metadata_mock.return_value = image exc_info = exc.NoValidHost(reason="") select_dest_mock.side_effect = exc_info @@ -2135,7 +2134,7 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase): sig_mock.assert_called_once_with(self.context, fake_spec) notify_mock.assert_called_once_with(self.context, inst_obj.uuid, 'migrate_server', updates, - exc_info, legacy_request_spec) + exc_info, fake_spec) rollback_mock.assert_called_once_with() @mock.patch.object(objects.InstanceMapping, 'get_by_instance_uuid') @@ -2166,7 +2165,6 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase): fake_spec = objects.RequestSpec(image=objects.ImageMeta()) spec_fc_mock.return_value = fake_spec - legacy_request_spec = fake_spec.to_legacy_request_spec_dict() im_mock.return_value = objects.InstanceMapping( cell_mapping=objects.CellMapping.get_by_uuid(self.context, @@ -2186,7 +2184,7 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase): sig_mock.assert_called_once_with(self.context, fake_spec) notify_mock.assert_called_once_with(self.context, inst_obj.uuid, 'migrate_server', updates, - exc_info, legacy_request_spec) + exc_info, fake_spec) rollback_mock.assert_called_once_with() def test_cold_migrate_no_valid_host_error_msg(self): @@ -2247,7 +2245,6 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase): exception = exc.UnsupportedPolicyException(reason='') fake_spec = fake_request_spec.fake_spec_obj() spec_fc_mock.return_value = fake_spec - legacy_request_spec = fake_spec.to_legacy_request_spec_dict() image_mock.return_value = image task_exec_mock.side_effect = exception @@ -2259,7 +2256,7 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase): updates = {'vm_state': vm_states.STOPPED, 'task_state': None} set_vm_mock.assert_called_once_with(self.context, inst_obj.uuid, 'migrate_server', updates, - exception, legacy_request_spec) + exception, fake_spec) @mock.patch.object(objects.InstanceMapping, 'get_by_instance_uuid') @mock.patch.object(scheduler_utils, 'setup_instance_group') @@ -2326,7 +2323,7 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase): node=hosts[0]['nodename'], clean_shutdown=True) notify_mock.assert_called_once_with(self.context, inst_obj.uuid, 'migrate_server', updates, - exc_info, legacy_request_spec) + exc_info, fake_spec) rollback_mock.assert_called_once_with() @mock.patch.object(objects.RequestSpec, 'save') @@ -2356,12 +2353,9 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase): # Just make sure we have an original flavor which is different from # the new one self.assertNotEqual(flavor, fake_spec.flavor) - with mock.patch.object( - fake_spec, 'to_legacy_request_spec_dict') as spec_to_dict_mock: - self.conductor._cold_migrate(self.context, inst_obj, flavor, {}, - [resvs], True, fake_spec) + self.conductor._cold_migrate(self.context, inst_obj, flavor, {}, + [resvs], True, fake_spec) - spec_to_dict_mock.assert_called_once_with() # Now the RequestSpec should be updated... self.assertEqual(flavor, fake_spec.flavor) # ...and persisted @@ -2575,7 +2569,7 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase): self.ctxt, instance.host, instance.node) notify.assert_called_once_with( self.ctxt, instance.uuid, 'rebuild_server', - {'vm_state': instance.vm_state, 'task_state': None}, ex, {}) + {'vm_state': instance.vm_state, 'task_state': None}, ex, None) @mock.patch.object(objects.ComputeNode, 'get_by_host_and_nodename', return_value=objects.ComputeNode(host='source-host')) @@ -2602,8 +2596,7 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase): self.ctxt, 'dest-host', use_slave=True) notify.assert_called_once_with( self.ctxt, instance.uuid, 'rebuild_server', - {'vm_state': instance.vm_state, 'task_state': None}, ex, - reqspec.to_legacy_request_spec_dict()) + {'vm_state': instance.vm_state, 'task_state': None}, ex, reqspec) @mock.patch.object(objects.ComputeNode, 'get_by_host_and_nodename', return_value=objects.ComputeNode(host='source-host')) @@ -2638,8 +2631,7 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase): get_source_node.return_value, get_dest_node.return_value) notify.assert_called_once_with( self.ctxt, instance.uuid, 'rebuild_server', - {'vm_state': instance.vm_state, 'task_state': None}, ex, - reqspec.to_legacy_request_spec_dict()) + {'vm_state': instance.vm_state, 'task_state': None}, ex, reqspec) @mock.patch('nova.conductor.tasks.live_migrate.LiveMigrationTask.execute') def test_live_migrate_instance(self, mock_execute): diff --git a/nova/tests/unit/scheduler/test_scheduler_utils.py b/nova/tests/unit/scheduler/test_scheduler_utils.py index 12dc39f6f948..9b5fb5219f8f 100644 --- a/nova/tests/unit/scheduler/test_scheduler_utils.py +++ b/nova/tests/unit/scheduler/test_scheduler_utils.py @@ -23,7 +23,6 @@ from nova.compute import flavors from nova.compute import utils as compute_utils from nova import exception from nova import objects -from nova import rpc from nova.scheduler import utils as scheduler_utils from nova import test from nova.tests.unit import fake_instance @@ -61,19 +60,19 @@ class SchedulerUtilsTestCase(test.NoDBTestCase): mock_get.assert_called_once_with() self.assertIsInstance(request_spec['instance_properties'], dict) - @mock.patch.object(rpc, 'get_notifier', return_value=mock.Mock()) + @mock.patch('nova.rpc.LegacyValidatingNotifier') @mock.patch.object(compute_utils, 'add_instance_fault_from_exc') @mock.patch.object(objects.Instance, 'save') - def test_set_vm_state_and_notify(self, mock_save, mock_add, mock_get): + def _test_set_vm_state_and_notify(self, mock_save, mock_add, mock_notifier, + request_spec, payload_request_spec): expected_uuid = uuids.instance - request_spec = dict(instance_properties=dict(uuid='other-uuid')) updates = dict(vm_state='fake-vm-state') service = 'fake-service' method = 'fake-method' exc_info = 'exc_info' - payload = dict(request_spec=request_spec, - instance_properties=request_spec.get( + payload = dict(request_spec=payload_request_spec, + instance_properties=payload_request_spec.get( 'instance_properties', {}), instance_id=expected_uuid, state='fake-vm-state', @@ -93,9 +92,37 @@ class SchedulerUtilsTestCase(test.NoDBTestCase): exc_info, mock.ANY) self.assertIsInstance(mock_add.call_args[0][1], objects.Instance) self.assertIsInstance(mock_add.call_args[0][3], tuple) - mock_get.return_value.error.assert_called_once_with(self.context, - event_type, - payload) + mock_notifier.return_value.error.assert_called_once_with(self.context, + event_type, + payload) + + def test_set_vm_state_and_notify_request_spec_dict(self): + """Tests passing a legacy dict format request spec to + set_vm_state_and_notify. + """ + request_spec = dict(instance_properties=dict(uuid='other-uuid')) + # The request_spec in the notification payload should be unchanged. + self._test_set_vm_state_and_notify( + request_spec=request_spec, payload_request_spec=request_spec) + + def test_set_vm_state_and_notify_request_spec_object(self): + """Tests passing a RequestSpec object to set_vm_state_and_notify.""" + request_spec = objects.RequestSpec.from_primitives( + self.context, dict(instance_properties=dict(uuid='other-uuid')), + filter_properties=dict()) + # The request_spec in the notification payload should be converted + # to the legacy format. + self._test_set_vm_state_and_notify( + request_spec=request_spec, + payload_request_spec=request_spec.to_legacy_request_spec_dict()) + + def test_set_vm_state_and_notify_request_spec_none(self): + """Tests passing None for the request_spec to set_vm_state_and_notify. + """ + # The request_spec in the notification payload should be changed to + # just an empty dict. + self._test_set_vm_state_and_notify( + request_spec=None, payload_request_spec={}) def test_build_filter_properties(self): sched_hints = {'hint': ['over-there']}