From ce3af5e33ae6843411e611e81c6ca1c21e0f1e09 Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Fri, 12 Oct 2018 12:03:41 -0400 Subject: [PATCH] Don't persist RequestSpec.requested_destination The RequestSpec.requested_destination, similar to the retry field, is per-request/operation, and persisting it can caues issues when subsequent move requests. For example, if you cold migrate a server to a specific host and then live migrate that server without specifying a host, the requested target host from the cold migrate is sent to the scheduler for the live migration, but since that is where the instance is already running, it's rejected with NoValidHost. This is a similar issue to the need to call RequestSpec.reset_forced_destinations() in all move operations in conductor. However, rather than try to whack this mole in every place the request spec is sent to the scheduler, like reset_forced_destinations() is used, we simply don't need to persist the requested_destination field since it's just a vehicle to tell the scheduler which host we want. The related functional regression test is updated to show the bug is now fixed. Change-Id: I2a78f0754c63381c57e7e1c610d0938b6df0f537 Closes-Bug: #1797580 --- nova/objects/request_spec.py | 8 +++++--- nova/tests/functional/regressions/test_bug_1797580.py | 8 +++----- nova/tests/unit/objects/test_request_spec.py | 4 ++++ 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/nova/objects/request_spec.py b/nova/objects/request_spec.py index 964248a2538d..3143478c8524 100644 --- a/nova/objects/request_spec.py +++ b/nova/objects/request_spec.py @@ -558,9 +558,11 @@ 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 since those are per-request - if 'retry' in spec and spec.retry: - spec.retry = None + # NOTE(mriedem): Don't persist retries or requested_destination + # since those are per-request + for excluded in ('retry', 'requested_destination'): + if excluded in spec and getattr(spec, excluded): + setattr(spec, excluded, None) # NOTE(stephenfin): Don't persist network metadata since we have # no need for it after scheduling if 'network_metadata' in spec and spec.network_metadata: diff --git a/nova/tests/functional/regressions/test_bug_1797580.py b/nova/tests/functional/regressions/test_bug_1797580.py index b040ffa7f801..2af23f53d679 100644 --- a/nova/tests/functional/regressions/test_bug_1797580.py +++ b/nova/tests/functional/regressions/test_bug_1797580.py @@ -92,8 +92,6 @@ class ColdMigrateTargetHostThenLiveMigrateTest( 'os-migrateLive': {'host': None, 'block_migration': 'auto'}} self.admin_api.post_server_action(server['id'], live_migrate_req) server = self._wait_for_state_change(self.admin_api, server, 'ACTIVE') - - # FIXME(mriedem): Until bug 1797580 is resolved the migration will - # fail during scheduling. - migration = self._wait_for_migration_status(server, ['error']) - self.assertEqual('live-migration', migration['migration_type']) + # The live migration should have been successful and the server is now + # back on the original host. + self.assertEqual(original_host, server['OS-EXT-SRV-ATTR:host']) diff --git a/nova/tests/unit/objects/test_request_spec.py b/nova/tests/unit/objects/test_request_spec.py index b9675a410d3d..217f216a04e1 100644 --- a/nova/tests/unit/objects/test_request_spec.py +++ b/nova/tests/unit/objects/test_request_spec.py @@ -594,6 +594,7 @@ class _TestRequestSpecObject(object): self.assertIsNone(serialized_obj.instance_group.members) self.assertIsNone(serialized_obj.instance_group.hosts) self.assertIsNone(serialized_obj.retry) + self.assertIsNone(serialized_obj.requested_destination) def test_create(self): req_obj = fake_request_spec.fake_spec_obj(remove_id=True) @@ -616,6 +617,9 @@ class _TestRequestSpecObject(object): def test_save(self): req_obj = fake_request_spec.fake_spec_obj() + # Make sure the requested_destination is not persisted since it is + # only valid per request/operation. + req_obj.requested_destination = objects.Destination(host='fake') def _test_save_args(self2, context, instance_uuid, changes): self._check_update_primitive(req_obj, changes)