From 7f8a14dbc88f8bb99d6a9da8eab8b16b067603ee Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Tue, 19 Dec 2017 18:40:31 -0500 Subject: [PATCH] Workaround missing RequestSpec.project_id when moving an instance The online data migration routine to create request specs for old instances used an admin context which has an empty project_id, so when scheduling (moving) one of these, if we try to PUT /allocations in placement using the FilterScheduler we'll fail because the project_id is None. This works around that by putting the instance.project_id on the request spec before calling the scheduler to pick a node and claim resources against it. A later change will need to add some sort of online data migration routine so that we properly update and persist the fix for these older records. Conflicts: nova/conductor/manager.py nova/conductor/tasks/live_migrate.py nova/conductor/tasks/migrate.py nova/tests/unit/conductor/test_conductor.py NOTE(mriedem): The conflicts are due to ca716ce4dd512e864886998a24e111e7d6e05848 and 685c16041c7e84fb0861b9b1833fc6c3cc372d05 not being in Pike. Change-Id: I34b1d99a9d0d2aca80f094a79ec1656abaf762dc Partial-Bug: #1739318 (cherry picked from commit f9a06c42535ef7ebba92c793afd947073352f850) --- nova/conductor/manager.py | 2 ++ nova/conductor/tasks/live_migrate.py | 1 + nova/conductor/tasks/migrate.py | 1 + nova/objects/request_spec.py | 6 ++++++ .../unit/conductor/tasks/test_live_migrate.py | 2 ++ nova/tests/unit/conductor/test_conductor.py | 17 ++++++++++++++--- 6 files changed, 26 insertions(+), 3 deletions(-) diff --git a/nova/conductor/manager.py b/nova/conductor/manager.py index 27cb5d22e70c..d82ac46bd94a 100644 --- a/nova/conductor/manager.py +++ b/nova/conductor/manager.py @@ -705,6 +705,7 @@ class ComputeTaskManager(base.Base): objects.Destination( cell=instance_mapping.cell_mapping)) + request_spec.ensure_project_id(instance) hosts = self._schedule_instances(context, request_spec, [instance.uuid]) host_state = hosts[0] @@ -867,6 +868,7 @@ class ComputeTaskManager(base.Base): # is not forced to be the original host request_spec.reset_forced_destinations() try: + request_spec.ensure_project_id(instance) hosts = self._schedule_instances(context, request_spec, [instance.uuid]) host_dict = hosts.pop(0) diff --git a/nova/conductor/tasks/live_migrate.py b/nova/conductor/tasks/live_migrate.py index c1aa607ef05e..a5b1ddfde95b 100644 --- a/nova/conductor/tasks/live_migrate.py +++ b/nova/conductor/tasks/live_migrate.py @@ -262,6 +262,7 @@ class LiveMigrationTask(base.TaskBase): request_spec.requested_destination = objects.Destination( cell=cell_mapping) + request_spec.ensure_project_id(self.instance) host = None while host is None: self._check_not_over_max_retries(attempted_hosts) diff --git a/nova/conductor/tasks/migrate.py b/nova/conductor/tasks/migrate.py index f64a08a2a254..f472fabe85fc 100644 --- a/nova/conductor/tasks/migrate.py +++ b/nova/conductor/tasks/migrate.py @@ -64,6 +64,7 @@ class MigrationTask(base.TaskBase): self.request_spec.requested_destination = objects.Destination( cell=instance_mapping.cell_mapping) + self.request_spec.ensure_project_id(self.instance) hosts = self.scheduler_client.select_destinations( self.context, self.request_spec, [self.instance.uuid]) host_state = hosts[0] diff --git a/nova/objects/request_spec.py b/nova/objects/request_spec.py index 0734559c1f68..f2282db4bf9e 100644 --- a/nova/objects/request_spec.py +++ b/nova/objects/request_spec.py @@ -50,6 +50,8 @@ class RequestSpec(base.NovaObject): nullable=True), 'pci_requests': fields.ObjectField('InstancePCIRequests', nullable=True), + # TODO(mriedem): The project_id shouldn't be nullable since the + # scheduler relies on it being set. 'project_id': fields.StringField(nullable=True), 'availability_zone': fields.StringField(nullable=True), 'flavor': fields.ObjectField('Flavor', nullable=False), @@ -432,6 +434,10 @@ class RequestSpec(base.NovaObject): spec_obj.obj_set_defaults() return spec_obj + def ensure_project_id(self, instance): + if 'project_id' not in self or self.project_id is None: + self.project_id = instance.project_id + @staticmethod def _from_db_object(context, spec, db_spec): spec_obj = spec.obj_from_primitive(jsonutils.loads(db_spec['spec'])) diff --git a/nova/tests/unit/conductor/tasks/test_live_migrate.py b/nova/tests/unit/conductor/tasks/test_live_migrate.py index 526f5680131b..a75709fa464d 100644 --- a/nova/tests/unit/conductor/tasks/test_live_migrate.py +++ b/nova/tests/unit/conductor/tasks/test_live_migrate.py @@ -304,6 +304,8 @@ class LiveMigrationTaskTestCase(test.NoDBTestCase): # Make sure the request_spec was updated to include the cell # mapping. self.assertIsNotNone(self.fake_spec.requested_destination.cell) + # Make sure the spec was updated to include the project_id. + self.assertEqual(self.fake_spec.project_id, self.instance.project_id) def test_find_destination_works_with_no_request_spec(self): task = live_migrate.LiveMigrationTask( diff --git a/nova/tests/unit/conductor/test_conductor.py b/nova/tests/unit/conductor/test_conductor.py index c011d9a3cd6d..80eea73eb93c 100644 --- a/nova/tests/unit/conductor/test_conductor.py +++ b/nova/tests/unit/conductor/test_conductor.py @@ -952,6 +952,10 @@ class _BaseTaskTestCase(object): from_primitives.return_value = fake_spec sched_instances.return_value = [host] self.conductor.unshelve_instance(self.context, instance, fake_spec) + # The fake_spec already has a project_id set which doesn't match + # the instance.project_id so the spec's project_id won't be + # overridden using the instance.project_id. + self.assertNotEqual(fake_spec.project_id, instance.project_id) reset_forced_destinations.assert_called_once_with() from_primitives.assert_called_once_with(self.context, request_spec, filter_properties) @@ -1226,6 +1230,7 @@ class _BaseTaskTestCase(object): rebuild_mock.assert_called_once_with(self.context, instance=inst_obj, **compute_args) + self.assertEqual(inst_obj.project_id, fake_spec.project_id) self.assertEqual('compute.instance.rebuild.scheduled', fake_notifier.NOTIFICATIONS[0].event_type) @@ -2120,7 +2125,8 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase): flavor=flavor, availability_zone=None, pci_requests=None, - numa_topology=None) + numa_topology=None, + project_id=self.context.project_id) resvs = 'fake-resvs' image = 'fake-image' fake_spec = objects.RequestSpec(image=objects.ImageMeta()) @@ -2143,6 +2149,7 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase): True, None) metadata_mock.assert_called_with({}) sig_mock.assert_called_once_with(self.context, fake_spec) + self.assertEqual(inst_obj.project_id, fake_spec.project_id) notify_mock.assert_called_once_with(self.context, inst_obj.uuid, 'migrate_server', updates, exc_info, legacy_request_spec) @@ -2170,7 +2177,8 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase): flavor=flavor, numa_topology=None, pci_requests=None, - availability_zone=None) + availability_zone=None, + project_id=self.context.project_id) image = 'fake-image' resvs = 'fake-resvs' @@ -2194,6 +2202,7 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase): True, None) metadata_mock.assert_called_with({}) sig_mock.assert_called_once_with(self.context, fake_spec) + self.assertEqual(inst_obj.project_id, fake_spec.project_id) notify_mock.assert_called_once_with(self.context, inst_obj.uuid, 'migrate_server', updates, exc_info, legacy_request_spec) @@ -2295,7 +2304,8 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase): flavor=flavor, availability_zone=None, pci_requests=None, - numa_topology=None) + numa_topology=None, + project_id=self.context.project_id) image = 'fake-image' resvs = 'fake-resvs' fake_spec = objects.RequestSpec(image=objects.ImageMeta()) @@ -2326,6 +2336,7 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase): metadata_mock.assert_called_with({}) sig_mock.assert_called_once_with(self.context, fake_spec) + self.assertEqual(inst_obj.project_id, fake_spec.project_id) select_dest_mock.assert_called_once_with( self.context, fake_spec, [inst_obj.uuid]) prep_resize_mock.assert_called_once_with(