From 3a66b8fdc04f65f7d10968d5359dfa1b9bad1807 Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Fri, 13 Dec 2019 10:07:09 -0500 Subject: [PATCH] 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 --- nova/compute/api.py | 2 +- nova/conductor/tasks/cross_cell_migrate.py | 23 +++++++----- nova/objects/migration_context.py | 10 ++---- nova/tests/functional/integrated_helpers.py | 3 ++ .../functional/test_cross_cell_migrate.py | 36 +++++++++++-------- .../tasks/test_cross_cell_migrate.py | 29 ++++++++++----- .../unit/objects/test_migration_context.py | 3 -- 7 files changed, 65 insertions(+), 41 deletions(-) diff --git a/nova/compute/api.py b/nova/compute/api.py index c0bf2da6f668..629eb4dd4f29 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -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'])) diff --git a/nova/conductor/tasks/cross_cell_migrate.py b/nova/conductor/tasks/cross_cell_migrate.py index 133303a068a6..4091f8032062 100644 --- a/nova/conductor/tasks/cross_cell_migrate.py +++ b/nova/conductor/tasks/cross_cell_migrate.py @@ -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. diff --git a/nova/objects/migration_context.py b/nova/objects/migration_context.py index 540905ed9544..b9504c8ae63a 100644 --- a/nova/objects/migration_context.py +++ b/nova/objects/migration_context.py @@ -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 diff --git a/nova/tests/functional/integrated_helpers.py b/nova/tests/functional/integrated_helpers.py index bf48ef00113d..9592da5f252f 100644 --- a/nova/tests/functional/integrated_helpers.py +++ b/nova/tests/functional/integrated_helpers.py @@ -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 diff --git a/nova/tests/functional/test_cross_cell_migrate.py b/nova/tests/functional/test_cross_cell_migrate.py index 94a4927236b3..cd72e212c08b 100644 --- a/nova/tests/functional/test_cross_cell_migrate.py +++ b/nova/tests/functional/test_cross_cell_migrate.py @@ -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 diff --git a/nova/tests/unit/conductor/tasks/test_cross_cell_migrate.py b/nova/tests/unit/conductor/tasks/test_cross_cell_migrate.py index ec09d718a5b4..545ce37a862d 100644 --- a/nova/tests/unit/conductor/tasks/test_cross_cell_migrate.py +++ b/nova/tests/unit/conductor/tasks/test_cross_cell_migrate.py @@ -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 diff --git a/nova/tests/unit/objects/test_migration_context.py b/nova/tests/unit/objects/test_migration_context.py index 3ed32a851baa..69e3f0b9581f 100644 --- a/nova/tests/unit/objects/test_migration_context.py +++ b/nova/tests/unit/objects/test_migration_context.py @@ -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):