openstack
/
nova
Code Issues Proposed changes

Modernize set_vm_state_and_notify 

This cleans up several TODOs in the conductor manager
code which calls scheduler_utils.set_vm_state_and_notify
and does mangling of the RequestSpec to make sure it's
a primitive by the time it gets to set_vm_state_and_notify.
That is abstracted in set_vm_state_and_notify itself now.

While doing this, we also remove some translation markers,
update the docs and fix some alignment issues.

Part of blueprint request-spec-use-by-compute

Change-Id: Ibc44e3b2261b314bb92062a88ca9ee6b81298dc3
This commit is contained in:
Matt Riedemann 2017-09-12 11:17:47 -04:00
parent 43e03b5d5b
commit f7de2d66a3
4 changed files with 81 additions and 67 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

35
nova/conductor/manager.py
View File

@ -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.
@ -1172,12 +1150,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.

42
nova/scheduler/utils.py
View File

@ -300,34 +300,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)

26
nova/tests/unit/conductor/test_conductor.py
View File

@ -2100,7 +2100,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
@ -2120,7 +2119,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')
@ -2151,7 +2150,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,
@ -2171,7 +2169,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):
@ -2232,7 +2230,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
@ -2244,7 +2241,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')
@ -2311,7 +2308,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')
@ -2341,12 +2338,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
@ -2560,7 +2554,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'))
@ -2587,8 +2581,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'))
@ -2623,8 +2616,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):

45
nova/tests/unit/scheduler/test_scheduler_utils.py
View File

@ -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']}