openstack
/
nova
Code Issues Proposed changes

Fix accumulated non-docs nits for cross-cell-resize series 

[1] https://review.opendev.org/#/c/638046/56/nova/conductor/tasks/cross_cell_migrate.py@899
[2] https://review.opendev.org/#/c/638046/56/nova/conductor/tasks/cross_cell_migrate.py@1160
[3] https://review.opendev.org/#/c/638046/56/nova/conductor/tasks/cross_cell_migrate.py@1187
[4] https://review.opendev.org/#/c/695334/5/nova/conductor/tasks/cross_cell_migrate.py@1464
[5] https://review.opendev.org/#/c/638048/56/nova/tests/functional/test_cross_cell_migrate.py@570
[6] https://review.opendev.org/#/c/638269/63/nova/compute/api.py@3735
[7] https://review.opendev.org/#/c/691991/15/nova/objects/migration_context.py@131
[8] https://review.opendev.org/#/c/638268/56/nova/tests/functional/test_cross_cell_migrate.py@762

Part of blueprint cross-cell-resize

Change-Id: I92a7c0c35415661db16edb89dc860f4512b98316
This commit is contained in:
Matt Riedemann 2019-12-13 10:07:09 -05:00
parent 74e76b1a6b
commit 3a66b8fdc0
7 changed files with 65 additions and 41 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
nova/compute/api.py
View File

@ -3737,7 +3737,7 @@ class API(base.Base):
# compute services.
if allowed:
# TODO(mriedem): We can remove this minimum compute version check
# in the 21.0.0 "U" release.
# in the 22.0.0 "V" release.
min_compute_version = (
objects.service.get_minimum_version_all_cells(
context, ['nova-compute']))

23
nova/conductor/tasks/cross_cell_migrate.py
View File

@ -856,7 +856,7 @@ class CrossCellMigrationTask(base.TaskBase):
LOG.exception('Rollback for task %s failed.', task_name)
def get_instance_from_source_cell(
def get_inst_and_cell_map_from_source(
target_cell_context, source_compute, instance_uuid):
"""Queries the instance from the source cell database.
@ -1022,7 +1022,7 @@ class ConfirmResizeTask(base.TaskBase):
def _execute(self):
# First get the instance from the source cell so we can cleanup.
source_cell_instance = get_instance_from_source_cell(
source_cell_instance = get_inst_and_cell_map_from_source(
self.context, self.migration.source_compute, self.instance.uuid)[0]
# Send the resize.confirm.start notification(s) using the source
# cell instance since we start there.
@ -1119,7 +1119,7 @@ class RevertResizeTask(base.TaskBase):
self.legacy_notifier, ctxt, instance, 'resize.revert.%s' % phase)
# Send the versioned notification.
compute_utils.notify_about_instance_action(
ctxt, instance, instance.host, # TODO(mriedem): Use CONF.host?
ctxt, instance, CONF.host,
action=fields.NotificationAction.RESIZE_REVERT,
phase=phase)
@ -1127,6 +1127,10 @@ class RevertResizeTask(base.TaskBase):
def _update_source_obj_from_target_cell(source_obj, target_obj):
"""Updates the object from the source cell using the target cell object
WARNING: This method does not support objects with nested objects, i.e.
objects that have fields which are other objects. An error will be
raised in that case.
All fields on the source object are updated from the target object
except for the ``id`` and ``created_at`` fields since those value must
not change during an update. The ``updated_at`` field is also skipped
@ -1142,10 +1146,15 @@ class RevertResizeTask(base.TaskBase):
:param source_obj: Versioned object from the source cell database
:param target_obj: Versioned object from the target cell database
:raises: ObjectActionError if nested object fields are encountered
"""
ignore_fields = ['created_at', 'id', 'updated_at']
for field in source_obj.obj_fields:
if field in target_obj and field not in ignore_fields:
if isinstance(source_obj.fields[field], fields.ObjectField):
raise exception.ObjectActionError(
action='_update_source_obj_from_target_cell',
reason='nested objects are not supported')
setattr(source_obj, field, getattr(target_obj, field))
def _update_bdms_in_source_cell(self, source_cell_context):
@ -1351,7 +1360,7 @@ class RevertResizeTask(base.TaskBase):
self.instance, fields.NotificationPhase.START)
source_cell_instance, source_cell_mapping = (
get_instance_from_source_cell(
get_inst_and_cell_map_from_source(
self.context, self.migration.source_compute,
self.instance.uuid))
self._source_cell_instance = source_cell_instance
@ -1415,10 +1424,8 @@ class RevertResizeTask(base.TaskBase):
# cell we update the records in the source cell, otherwise we
# update the records in the target cell.
instance_at_source = self._source_cell_migration is not None
migration = self._source_cell_migration \
if self._source_cell_migration else self.migration
instance = self._source_cell_instance \
if self._source_cell_instance else self.instance
migration = self._source_cell_migration or self.migration
instance = self._source_cell_instance or self.instance
# NOTE(mriedem): This exception log is fairly generic. We could
# probably make this more targeted based on what we know of the
# state of the system if we want to make it more detailed, e.g.

10
nova/objects/migration_context.py
View File

@ -128,12 +128,8 @@ class MigrationContext(base.NovaPersistentObject, base.NovaObject):
Based on the ``migration_id`` in this context, gets the Migration
object and returns its ``cross_cell_move`` value.
The result is cached for subsequent lookups.
:return: True if this is a cross cell move migration, False otherwise.
"""
if not hasattr(self, '_cached_cross_cell_move'):
migration = objects.Migration.get_by_id(
self._context, self.migration_id)
setattr(self, '_cached_cross_cell_move', migration.cross_cell_move)
return self._cached_cross_cell_move
migration = objects.Migration.get_by_id(
self._context, self.migration_id)
return migration.cross_cell_move

3
nova/tests/functional/integrated_helpers.py
View File

@ -793,6 +793,8 @@ class ProviderUsageBaseTestCase(test.TestCase, InstanceHelperMixin):
migration-based allocations are also cleaned up.
:param server: The API representation of the instance to be deleted
:returns: The uuid of the migration record associated with the resize
or cold migrate operation
"""
# First check to see if there is a related migration record so we can
@ -833,6 +835,7 @@ class ProviderUsageBaseTestCase(test.TestCase, InstanceHelperMixin):
# and no allocations for the delete migration
allocations = self._get_allocations_by_server_uuid(migration_uuid)
self.assertEqual(0, len(allocations))
return migration_uuid
def _run_periodics(self):
"""Run the update_available_resource task on every compute manager

36
nova/tests/functional/test_cross_cell_migrate.py
View File

@ -287,9 +287,7 @@ class TestMultiCellMigrate(integrated_helpers.ProviderUsageBaseTestCase):
# resize. There will be a third action if the server was stopped. Use
# assertGreaterEqual in case a test performed some actions on a
# pre-created server before resizing it, like attaching a volume.
actions = self.api.api_get(
'/servers/%s/os-instance-actions' % server['id']
).body['instanceActions']
actions = self.api.get_instance_actions(server['id'])
expected_num_of_actions = 3 if stopped else 2
self.assertGreaterEqual(len(actions), expected_num_of_actions, actions)
# Each action should have events (make sure these were copied from
@ -470,7 +468,8 @@ class TestMultiCellMigrate(integrated_helpers.ProviderUsageBaseTestCase):
end = fake_notifier.VERSIONED_NOTIFICATIONS[1]['event_type']
self.assertEqual('instance.resize_confirm.end', end)
def delete_server_and_assert_cleanup(self, server):
def delete_server_and_assert_cleanup(self, server,
assert_confirmed_migration=False):
"""Deletes the server and makes various cleanup checks.
- makes sure allocations from placement are gone
@ -478,6 +477,10 @@ class TestMultiCellMigrate(integrated_helpers.ProviderUsageBaseTestCase):
- makes sure there are no leaked volume attachments
:param server: dict of the server resource to delete
:param assert_confirmed_migration: If True, asserts that the Migration
record for the server has status "confirmed". This is useful when
testing that deleting a resized server automatically confirms the
resize.
"""
# Determine which cell the instance was in when the server was deleted
# in the API so we can check hard vs soft delete in the DB.
@ -485,9 +488,16 @@ class TestMultiCellMigrate(integrated_helpers.ProviderUsageBaseTestCase):
server['OS-EXT-SRV-ATTR:host']]
# Delete the server and check that the allocations are gone from
# the placement service.
self._delete_and_check_allocations(server)
# Make sure the instance record is gone from both cell databases.
mig_uuid = self._delete_and_check_allocations(server)
ctxt = nova_context.get_admin_context()
if assert_confirmed_migration:
# Get the Migration object from the last cell the instance was in
# and assert its status is "confirmed".
cell = self.cell_mappings[current_cell]
with nova_context.target_cell(ctxt, cell) as cctxt:
migration = objects.Migration.get_by_uuid(cctxt, mig_uuid)
self.assertEqual('confirmed', migration.status)
# Make sure the instance record is gone from both cell databases.
for cell_name in self.host_to_cell_mappings.values():
cell = self.cell_mappings[cell_name]
with nova_context.target_cell(ctxt, cell) as cctxt:
@ -507,9 +517,7 @@ class TestMultiCellMigrate(integrated_helpers.ProviderUsageBaseTestCase):
self.cinder.volume_to_attachment)
def assert_resize_confirm_actions(self, server):
actions = self.api.api_get(
'/servers/%s/os-instance-actions' % server['id']
).body['instanceActions']
actions = self.api.get_instance_actions(server['id'])
actions_by_action = {action['action']: action for action in actions}
self.assertIn(instance_actions.CONFIRM_RESIZE, actions_by_action)
confirm_action = actions_by_action[instance_actions.CONFIRM_RESIZE]
@ -644,9 +652,7 @@ class TestMultiCellMigrate(integrated_helpers.ProviderUsageBaseTestCase):
self.assertEqual('instance.resize_revert.end', end)
def assert_resize_revert_actions(self, server, source_host, dest_host):
actions = self.api.api_get(
'/servers/%s/os-instance-actions' % server['id']
).body['instanceActions']
actions = self.api.get_instance_actions(server['id'])
# The revert instance action should have been copied from the target
# cell to the source cell and "completed" there, i.e. an event
# should show up under that revert action.
@ -802,10 +808,12 @@ class TestMultiCellMigrate(integrated_helpers.ProviderUsageBaseTestCase):
def test_delete_while_in_verify_resize_status(self):
"""Tests that when deleting a server in VERIFY_RESIZE status, the
data is cleaned from both the source and target cell.
data is cleaned from both the source and target cell and the resize
is automatically confirmed.
"""
server = self._resize_and_validate()[0]
self.delete_server_and_assert_cleanup(server)
self.delete_server_and_assert_cleanup(server,
assert_confirmed_migration=True)
def test_cold_migrate_target_host_in_other_cell(self):
"""Tests cold migrating to a target host in another cell. This is

29
nova/tests/unit/conductor/tasks/test_cross_cell_migrate.py
View File

@ -1063,8 +1063,8 @@ class UtilityTestCase(test.NoDBTestCase):
return_value=objects.HostMapping(
cell_mapping=objects.CellMapping(uuid=uuids.cell)))
@mock.patch('nova.objects.Instance.get_by_uuid')
def test_get_instance_from_source_cell(self, mock_get_inst,
mock_get_by_host):
def test_get_inst_and_cell_map_from_source(self, mock_get_inst,
mock_get_by_host):
target_cell_context = nova_context.get_admin_context()
# Stub out Instance.get_by_uuid to make sure a copy of the context is
# targeted at the source cell mapping.
@ -1074,8 +1074,9 @@ class UtilityTestCase(test.NoDBTestCase):
self.assertEqual(uuids.cell, ctxt.cell_uuid)
return mock.sentinel.instance
mock_get_inst.side_effect = stub_get_by_uuid
inst, cell_mapping = cross_cell_migrate.get_instance_from_source_cell(
target_cell_context, 'source-host', uuids.instance)
inst, cell_mapping = (
cross_cell_migrate.get_inst_and_cell_map_from_source(
target_cell_context, 'source-host', uuids.instance))
self.assertIs(inst, mock.sentinel.instance)
self.assertIs(cell_mapping, mock_get_by_host.return_value.cell_mapping)
mock_get_by_host.assert_called_once_with(
@ -1105,7 +1106,7 @@ class ConfirmResizeTaskTestCase(test.NoDBTestCase):
compute_rpcapi)
@mock.patch('nova.conductor.tasks.cross_cell_migrate.'
'get_instance_from_source_cell')
'get_inst_and_cell_map_from_source')
def test_execute(self, mock_get_instance):
source_cell_instance = objects.Instance(
mock.MagicMock(), uuid=uuids.instance)
@ -1134,7 +1135,7 @@ class ConfirmResizeTaskTestCase(test.NoDBTestCase):
_finish_confirm_in_target_cell.assert_called_once_with()
@mock.patch('nova.conductor.tasks.cross_cell_migrate.'
'get_instance_from_source_cell',
'get_inst_and_cell_map_from_source',
side_effect=exception.InstanceNotFound(
instance_id=uuids.instance))
@mock.patch('nova.objects.Migration.save')
@ -1259,7 +1260,7 @@ class RevertResizeTaskTestCase(test.NoDBTestCase, ObjectComparatorMixin):
return source_cell_instance
@mock.patch('nova.conductor.tasks.cross_cell_migrate.'
'get_instance_from_source_cell')
'get_inst_and_cell_map_from_source')
@mock.patch('nova.objects.InstanceActionEvent') # Stub EventReport calls.
def test_execute(self, mock_action_event, mock_get_instance):
"""Happy path test for the execute method."""
@ -1392,6 +1393,7 @@ class RevertResizeTaskTestCase(test.NoDBTestCase, ObjectComparatorMixin):
@mock.patch('nova.compute.utils.notify_about_instance_action')
def test_send_resize_revert_notification(self, mock_notify_action,
mock_notify_usage):
self.flags(host='fake-conductor-host')
instance = self.task.instance
self.task._send_resize_revert_notification(instance, 'foo')
# Assert the legacy notification was sent.
@ -1400,7 +1402,7 @@ class RevertResizeTaskTestCase(test.NoDBTestCase, ObjectComparatorMixin):
'resize.revert.foo')
# Assert the versioned notification was sent.
mock_notify_action.assert_called_once_with(
instance._context, instance, instance.host,
instance._context, instance, 'fake-conductor-host',
action=fields.NotificationAction.RESIZE_REVERT, phase='foo')
def test_update_instance_in_source_cell(self):
@ -1582,6 +1584,17 @@ class RevertResizeTaskTestCase(test.NoDBTestCase, ObjectComparatorMixin):
# Now make sure the rest of the fields are the same.
self._compare_objs(source_obj, target_obj, ignored_keys=ignored_keys)
def test_update_source_obj_from_target_cell_nested_object(self):
"""Tests that calling _update_source_obj_from_target_cell with an
object that has nested object fields will raise ObjectActionError.
"""
source = objects.Instance(flavor=objects.Flavor(flavorid='a'))
target = objects.Instance(flavor=objects.Flavor(flavorid='b'))
ex = self.assertRaises(exception.ObjectActionError,
self.task._update_source_obj_from_target_cell,
source, target)
self.assertIn('nested objects are not supported', six.text_type(ex))
@mock.patch('nova.objects.Migration.get_by_uuid')
def test_update_migration_in_source_cell(self, mock_get_migration):
"""Tests updating the migration record in the source cell from the

3
nova/tests/unit/objects/test_migration_context.py
View File

@ -133,9 +133,6 @@ class _TestMigrationContext(object):
mig_ctx = get_fake_migration_context_obj(ctxt)
self.assertTrue(mig_ctx.is_cross_cell_move())
mock_get_by_id.assert_called_once_with(ctxt, mig_ctx.migration_id)
# Call it again to make sure the result was cached.
self.assertTrue(mig_ctx.is_cross_cell_move())
mock_get_by_id.assert_called_once_with(ctxt, mig_ctx.migration_id)
class TestMigrationContext(test_objects._LocalTest, _TestMigrationContext):