diff --git a/nova/conductor/manager.py b/nova/conductor/manager.py index 529da9ea92da..619b2c6de996 100644 --- a/nova/conductor/manager.py +++ b/nova/conductor/manager.py @@ -973,8 +973,7 @@ class ComputeTaskManager(base.Base): elif recreate: # NOTE(sbauza): Augment the RequestSpec object by excluding # the source host for avoiding the scheduler to pick it - request_spec.ignore_hosts = request_spec.ignore_hosts or [] - request_spec.ignore_hosts.append(instance.host) + request_spec.ignore_hosts = [instance.host] # NOTE(sbauza): Force_hosts/nodes needs to be reset # if we want to make sure that the next destination # is not forced to be the original host diff --git a/nova/objects/request_spec.py b/nova/objects/request_spec.py index 640307d385e4..e515f0d61810 100644 --- a/nova/objects/request_spec.py +++ b/nova/objects/request_spec.py @@ -504,10 +504,10 @@ class RequestSpec(base.NovaObject): # it will reset the field to None and we'll lose what is set # (but not persisted) on the object. continue - elif key == 'retry': - # NOTE(takashin): Do not override the 'retry' field - # which is not a persistent. It is not lazy-loadable field. - # If it is not set, set None. + elif key in ('retry', 'ignore_hosts'): + # NOTE(takashin): Do not override the 'retry' or 'ignore_hosts' + # fields which are not persisted. They are not lazy-loadable + # fields. If they are not set, set None. if not spec.obj_attr_is_set(key): setattr(spec, key, None) elif key in spec_obj: @@ -571,9 +571,9 @@ class RequestSpec(base.NovaObject): if 'instance_group' in spec and spec.instance_group: spec.instance_group.members = None spec.instance_group.hosts = None - # NOTE(mriedem): Don't persist retries or requested_destination - # since those are per-request - for excluded in ('retry', 'requested_destination'): + # NOTE(mriedem): Don't persist retries, requested_destination + # or ignored hosts since those are per-request + for excluded in ('retry', 'requested_destination', 'ignore_hosts'): if excluded in spec and getattr(spec, excluded): setattr(spec, excluded, None) # NOTE(stephenfin): Don't persist network metadata since we have diff --git a/nova/tests/functional/regressions/test_bug_1669054.py b/nova/tests/functional/regressions/test_bug_1669054.py index 47440d6c1680..73b8fcc18503 100644 --- a/nova/tests/functional/regressions/test_bug_1669054.py +++ b/nova/tests/functional/regressions/test_bug_1669054.py @@ -63,21 +63,17 @@ class ResizeEvacuateTestCase(integrated_helpers._IntegratedTestBase, # Disable the host on which the server is now running (host2). host2.stop() self.api.force_down_service('host2', 'nova-compute', forced_down=True) - # Now try to evacuate the server back to the original source compute. - # FIXME(mriedem): This is bug 1669054 where the evacuate fails with - # NoValidHost because the RequestSpec.ignore_hosts field has the - # original source host in it which is the only other available host to - # which we can evacuate the server. req = {'evacuate': {'onSharedStorage': False}} - self.api.post_server_action(server['id'], req, - check_response_status=[500]) - # There should be fault recorded with the server. - server = self._wait_for_state_change(self.api, server, 'ERROR') - self.assertIn('fault', server) - self.assertIn('No valid host was found', server['fault']['message']) - # Assert the RequestSpec.ignore_hosts is still populated. + self.api.post_server_action(server['id'], req) + server = self._wait_for_state_change(self.api, server, 'ACTIVE') + # The evacuate flow in the compute manager is annoying in that it + # sets the instance status to ACTIVE before updating the host, so we + # have to wait for the migration record to be 'done' to avoid a race. + self._wait_for_migration_status(server, ['done']) + self.assertEqual(self.compute.host, server['OS-EXT-SRV-ATTR:host']) + + # Assert the RequestSpec.ignore_hosts field is not populated. reqspec = objects.RequestSpec.get_by_instance_uuid( context.get_admin_context(), server['id']) - self.assertIsNotNone(reqspec.ignore_hosts) - self.assertIn(self.compute.host, reqspec.ignore_hosts) + self.assertIsNone(reqspec.ignore_hosts) diff --git a/nova/tests/unit/conductor/test_conductor.py b/nova/tests/unit/conductor/test_conductor.py index 48e8fb2f5650..86e02a18c1f5 100644 --- a/nova/tests/unit/conductor/test_conductor.py +++ b/nova/tests/unit/conductor/test_conductor.py @@ -1524,7 +1524,7 @@ class _BaseTaskTestCase(object): **compute_args) @mock.patch('nova.compute.utils.notify_about_instance_rebuild') - def test_rebuild_instance_with_request_spec(self, mock_notify): + def test_evacuate_instance_with_request_spec(self, mock_notify): inst_obj = self._create_fake_instance_obj() inst_obj.host = 'noselect' expected_host = 'thebesthost' @@ -1532,10 +1532,10 @@ class _BaseTaskTestCase(object): expected_limits = None fake_selection = objects.Selection(service_host=expected_host, nodename=expected_node, limits=None) - fake_spec = objects.RequestSpec(ignore_hosts=[]) + fake_spec = objects.RequestSpec(ignore_hosts=[uuids.ignored_host]) rebuild_args, compute_args = self._prepare_rebuild_args( {'host': None, 'node': expected_node, 'limits': expected_limits, - 'request_spec': fake_spec}) + 'request_spec': fake_spec, 'recreate': True}) with test.nested( mock.patch.object(self.conductor_manager.compute_rpcapi, 'rebuild_instance'), @@ -1549,10 +1549,9 @@ class _BaseTaskTestCase(object): self.conductor_manager.rebuild_instance(context=self.context, instance=inst_obj, **rebuild_args) - if rebuild_args['recreate']: - reset_fd.assert_called_once_with() - else: - reset_fd.assert_not_called() + reset_fd.assert_called_once_with() + # The RequestSpec.ignore_hosts field should be overwritten. + self.assertEqual([inst_obj.host], fake_spec.ignore_hosts) select_dest_mock.assert_called_once_with(self.context, fake_spec, [inst_obj.uuid], return_objects=True, return_alternates=False) diff --git a/nova/tests/unit/objects/test_request_spec.py b/nova/tests/unit/objects/test_request_spec.py index e77912683cea..526ddbd600e3 100644 --- a/nova/tests/unit/objects/test_request_spec.py +++ b/nova/tests/unit/objects/test_request_spec.py @@ -555,7 +555,8 @@ class _TestRequestSpecObject(object): fake_spec['instance_uuid']) self.assertEqual(1, req_obj.num_instances) - self.assertEqual(['host2', 'host4'], req_obj.ignore_hosts) + # ignore_hosts is not persisted + self.assertIsNone(req_obj.ignore_hosts) self.assertEqual('fake', req_obj.project_id) self.assertEqual({'hint': ['over-there']}, req_obj.scheduler_hints) self.assertEqual(['host1', 'host3'], req_obj.force_hosts) @@ -579,7 +580,7 @@ class _TestRequestSpecObject(object): jsonutils.loads(changes['spec'])) # primitive fields - for field in ['instance_uuid', 'num_instances', 'ignore_hosts', + for field in ['instance_uuid', 'num_instances', 'project_id', 'scheduler_hints', 'force_hosts', 'availability_zone', 'force_nodes']: self.assertEqual(getattr(req_obj, field), @@ -596,6 +597,7 @@ class _TestRequestSpecObject(object): self.assertIsNone(serialized_obj.instance_group.hosts) self.assertIsNone(serialized_obj.retry) self.assertIsNone(serialized_obj.requested_destination) + self.assertIsNone(serialized_obj.ignore_hosts) def test_create(self): req_obj = fake_request_spec.fake_spec_obj(remove_id=True) @@ -679,6 +681,7 @@ class _TestRequestSpecObject(object): objects.ComputeNode(host='host2', hypervisor_hostname='node2'), ])) req_obj.retry = expected_retry + req_obj.ignore_hosts = [uuids.ignored_host] orig_save_in_db = request_spec.RequestSpec._save_in_db with mock.patch.object(request_spec.RequestSpec, '_save_in_db') \ @@ -691,16 +694,19 @@ class _TestRequestSpecObject(object): # 1. network_metadata # 2. requested_destination # 3. retry + # 4. ignore_hosts data = jsonutils.loads(updates['spec'])['nova_object.data'] self.assertNotIn('network_metadata', data) self.assertIsNone(data['requested_destination']) self.assertIsNone(data['retry']) + self.assertIsNone(data['ignore_hosts']) self.assertIsNotNone(data['instance_uuid']) # also we expect that the following fields are not reset after save # 1. network_metadata # 2. requested_destination # 3. retry + # 4. ignore_hosts self.assertIsNotNone(req_obj.network_metadata) self.assertJsonEqual(expected_network_metadata.obj_to_primitive(), req_obj.network_metadata.obj_to_primitive()) @@ -710,6 +716,8 @@ class _TestRequestSpecObject(object): self.assertIsNotNone(req_obj.retry) self.assertJsonEqual(expected_retry.obj_to_primitive(), req_obj.retry.obj_to_primitive()) + self.assertIsNotNone(req_obj.ignore_hosts) + self.assertEqual([uuids.ignored_host], req_obj.ignore_hosts) def test_save(self): req_obj = fake_request_spec.fake_spec_obj() diff --git a/nova/virt/fake.py b/nova/virt/fake.py index 8d5aa6e93b51..300c2510e97f 100644 --- a/nova/virt/fake.py +++ b/nova/virt/fake.py @@ -584,7 +584,11 @@ class FakeDriver(driver.ComputeDriver): block_device_info=block_device_info) def confirm_migration(self, context, migration, instance, network_info): - return + # Confirm migration cleans up the guest from the source host so just + # destroy the guest to remove it from the list of tracked instances + # unless it is a same-host resize. + if migration.source_compute != migration.dest_compute: + self.destroy(context, instance, network_info) def pre_live_migration(self, context, instance, block_device_info, network_info, disk_info, migrate_data):