diff --git a/nova/objects/request_spec.py b/nova/objects/request_spec.py index 3143478c8524..640307d385e4 100644 --- a/nova/objects/request_spec.py +++ b/nova/objects/request_spec.py @@ -497,6 +497,19 @@ class RequestSpec(base.NovaObject): # though they should match. if key in ['id', 'instance_uuid']: setattr(spec, key, db_spec[key]) + elif key in ('requested_destination', 'network_metadata'): + # Do not override what we already have in the object as this + # field is not persisted. If save() is called after + # requested_destination or network_metadata is populated, + # 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. + if not spec.obj_attr_is_set(key): + setattr(spec, key, None) elif key in spec_obj: setattr(spec, key, getattr(spec_obj, key)) spec._context = context diff --git a/nova/tests/functional/regressions/test_bug_1815153.py b/nova/tests/functional/regressions/test_bug_1815153.py new file mode 100644 index 000000000000..2e5cda03561a --- /dev/null +++ b/nova/tests/functional/regressions/test_bug_1815153.py @@ -0,0 +1,173 @@ +# Copyright 2019 NTT Corporation +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import six + +from nova import context +from nova import objects +from nova import test +from nova.tests import fixtures as nova_fixtures +from nova.tests.functional.api import client +from nova.tests.functional import integrated_helpers +from nova.tests.unit.image import fake as image_fake +from nova.tests.unit import policy_fixture +from nova.virt import fake + + +class NonPersistentFieldNotResetTest( + test.TestCase, integrated_helpers.InstanceHelperMixin): + """Test for regression bug 1815153 + + The bug is that the 'requested_destination' field in the RequestSpec + object is reset when saving the object in the 'heal_reqspec_is_bfv' + method in the case that a server created before Rocky which does not + have is_bfv field. + + Tests the following two cases here. + + * Cold migration with a target host without a force flag + * Evacuate with a target host without a force flag + + The following two cases are not tested here because + 'requested_destination' is not set when the 'heal_reqspec_is_bfv' method + is called. + + * Live migration without a destination host. + * Unshelve a server + """ + + def setUp(self): + super(NonPersistentFieldNotResetTest, self).setUp() + self.useFixture(policy_fixture.RealPolicyFixture()) + self.useFixture(nova_fixtures.NeutronFixture(self)) + self.useFixture(nova_fixtures.PlacementFixture()) + + api_fixture = self.useFixture(nova_fixtures.OSAPIFixture( + api_version='v2.1')) + self.api = api_fixture.admin_api + # Use the latest microversion available to make sure something does + # not regress in new microversions; cap as necessary. + self.api.microversion = 'latest' + + image_fake.stub_out_image_service(self) + self.addCleanup(image_fake.FakeImageService_reset) + + self.start_service('conductor') + self.start_service('scheduler') + + self.compute = {} + + self.addCleanup(fake.restore_nodes) + for host in ('host1', 'host2', 'host3'): + fake.set_nodes([host]) + compute_service = self.start_service('compute', host=host) + self.compute.update({host: compute_service}) + + self.ctxt = context.get_admin_context() + + @staticmethod + def _get_target_host(host): + target_host = {'host1': 'host2', + 'host2': 'host3', + 'host3': 'host1'} + return target_host[host] + + def _create_server(self): + # Create a server, it doesn't matter on which host it builds. + server = self._build_minimal_create_server_request( + self.api, 'sample-server', + image_uuid='155d900f-4e14-4e4c-a73d-069cbf4541e6', + networks='none') + server = self.api.post_server({'server': server}) + server = self._wait_for_state_change(self.api, server, 'ACTIVE') + + return server + + def _remove_is_bfv_in_request_spec(self, server_id): + # Now let's hack the RequestSpec.is_bfv field to mimic migrating an + # old instance created before RequestSpec.is_bfv was set in the API, + reqspec = objects.RequestSpec.get_by_instance_uuid(self.ctxt, + server_id) + del reqspec.is_bfv + reqspec.save() + reqspec = objects.RequestSpec.get_by_instance_uuid(self.ctxt, + server_id) + # Make sure 'is_bfv' is not set. + self.assertNotIn('is_bfv', reqspec) + + def test_cold_migrate(self): + server = self._create_server() + original_host = server['OS-EXT-SRV-ATTR:host'] + target_host = self._get_target_host(original_host) + self._remove_is_bfv_in_request_spec(server['id']) + + # Force a target host down + source_compute_id = self.api.get_services( + host=target_host, binary='nova-compute')[0]['id'] + self.compute[target_host].stop() + self.api.put_service( + source_compute_id, {'forced_down': 'true'}) + + # Cold migrate a server with a target host. + # If requested_destination is reset, the server is moved to a host + # that is not a requested target host. + # In that case, the response code is 202. + # If requested_destination is not reset, no valid host error (400) is + # returned because the target host is down. + ex = self.assertRaises(client.OpenStackApiException, + self.api.post_server_action, + server['id'], + {'migrate': {'host': target_host}}) + self.assertEqual(400, ex.response.status_code) + self.assertIn('No valid host was found. No valid host found ' + 'for cold migrate', six.text_type(ex)) + + # Make sure 'is_bfv' is set. + reqspec = objects.RequestSpec.get_by_instance_uuid(self.ctxt, + server['id']) + self.assertIn('is_bfv', reqspec) + self.assertIs(reqspec.is_bfv, False) + + def test_evacuate(self): + server = self._create_server() + original_host = server['OS-EXT-SRV-ATTR:host'] + target_host = self._get_target_host(original_host) + self._remove_is_bfv_in_request_spec(server['id']) + + # Force source and target hosts down + for host in (original_host, target_host): + source_compute_id = self.api.get_services( + host=host, binary='nova-compute')[0]['id'] + self.compute[host].stop() + self.api.put_service( + source_compute_id, {'forced_down': 'true'}) + + # Evacuate a server with a target host. + # If requested_destination is reset, the server is moved to a host + # that is not the target host. + # Its status becomes 'ACTIVE'. + # If requested_destination is not reset, a status of the server + # becomes 'ERROR' because the target host is down. + self.api.post_server_action( + server['id'], {'evacuate': {'host': target_host}}) + expected_params = {'OS-EXT-SRV-ATTR:host': original_host, + 'status': 'ERROR'} + server = self._wait_for_server_parameter(self.api, server, + expected_params) + + # Make sure 'is_bfv' is set. + reqspec = objects.RequestSpec.get_by_instance_uuid(self.ctxt, + server['id']) + self.assertIn('is_bfv', reqspec) + self.assertIs(reqspec.is_bfv, False) diff --git a/nova/tests/unit/fake_request_spec.py b/nova/tests/unit/fake_request_spec.py index 3c7cf316d914..ddc26c882fa3 100644 --- a/nova/tests/unit/fake_request_spec.py +++ b/nova/tests/unit/fake_request_spec.py @@ -57,6 +57,8 @@ PCI_REQUESTS.obj_reset_changes(recursive=True) def fake_db_spec(): req_obj = fake_spec_obj() + # NOTE(takashin): There is not 'retry' information in the DB table. + del req_obj.retry db_request_spec = { 'id': 1, 'instance_uuid': req_obj.instance_uuid, diff --git a/nova/tests/unit/objects/test_request_spec.py b/nova/tests/unit/objects/test_request_spec.py index b51b243c09b2..e77912683cea 100644 --- a/nova/tests/unit/objects/test_request_spec.py +++ b/nova/tests/unit/objects/test_request_spec.py @@ -567,7 +567,8 @@ class _TestRequestSpecObject(object): self.assertIsInstance(req_obj.pci_requests, objects.InstancePCIRequests) self.assertIsInstance(req_obj.flavor, objects.Flavor) - self.assertIsInstance(req_obj.retry, objects.SchedulerRetries) + # The 'retry' field is not persistent. + self.assertIsNone(req_obj.retry) self.assertIsInstance(req_obj.limits, objects.SchedulerLimits) self.assertIsInstance(req_obj.instance_group, objects.InstanceGroup) self.assertEqual('fresh', req_obj.instance_group.name) @@ -615,6 +616,101 @@ class _TestRequestSpecObject(object): self.assertRaises(exception.ObjectActionError, req_obj.create) + def test_create_does_not_persist_requested_fields(self): + req_obj = fake_request_spec.fake_spec_obj(remove_id=True) + + expected_network_metadata = objects.NetworkMetadata( + physnets=set(['foo', 'bar']), tunneled=True) + req_obj.network_metadata = expected_network_metadata + expected_destination = request_spec.Destination(host='sample-host') + req_obj.requested_destination = expected_destination + expected_retry = objects.SchedulerRetries( + num_attempts=2, + hosts=objects.ComputeNodeList(objects=[ + objects.ComputeNode(host='host1', hypervisor_hostname='node1'), + objects.ComputeNode(host='host2', hypervisor_hostname='node2'), + ])) + req_obj.retry = expected_retry + + orig_create_in_db = request_spec.RequestSpec._create_in_db + with mock.patch.object(request_spec.RequestSpec, '_create_in_db') \ + as mock_create_in_db: + mock_create_in_db.side_effect = orig_create_in_db + req_obj.create() + mock_create_in_db.assert_called_once() + updates = mock_create_in_db.mock_calls[0][1][1] + # assert that the following fields are not stored in the db + # 1. network_metadata + # 2. requested_destination + # 3. retry + data = jsonutils.loads(updates['spec'])['nova_object.data'] + self.assertNotIn('network_metadata', data) + self.assertIsNone(data['requested_destination']) + self.assertIsNone(data['retry']) + self.assertIsNotNone(data['instance_uuid']) + + # also we expect that the following fields are not reset after create + # 1. network_metadata + # 2. requested_destination + # 3. retry + self.assertIsNotNone(req_obj.network_metadata) + self.assertJsonEqual(expected_network_metadata.obj_to_primitive(), + req_obj.network_metadata.obj_to_primitive()) + self.assertIsNotNone(req_obj.requested_destination) + self.assertJsonEqual(expected_destination.obj_to_primitive(), + req_obj.requested_destination.obj_to_primitive()) + self.assertIsNotNone(req_obj.retry) + self.assertJsonEqual(expected_retry.obj_to_primitive(), + req_obj.retry.obj_to_primitive()) + + def test_save_does_not_persist_requested_fields(self): + req_obj = fake_request_spec.fake_spec_obj(remove_id=True) + req_obj.create() + # change something to make sure _save_in_db is called + expected_network_metadata = objects.NetworkMetadata( + physnets=set(['foo', 'bar']), tunneled=True) + req_obj.network_metadata = expected_network_metadata + expected_destination = request_spec.Destination(host='sample-host') + req_obj.requested_destination = expected_destination + expected_retry = objects.SchedulerRetries( + num_attempts=2, + hosts=objects.ComputeNodeList(objects=[ + objects.ComputeNode(host='host1', hypervisor_hostname='node1'), + objects.ComputeNode(host='host2', hypervisor_hostname='node2'), + ])) + req_obj.retry = expected_retry + + orig_save_in_db = request_spec.RequestSpec._save_in_db + with mock.patch.object(request_spec.RequestSpec, '_save_in_db') \ + as mock_save_in_db: + mock_save_in_db.side_effect = orig_save_in_db + req_obj.save() + mock_save_in_db.assert_called_once() + updates = mock_save_in_db.mock_calls[0][1][2] + # assert that the following fields are not stored in the db + # 1. network_metadata + # 2. requested_destination + # 3. retry + data = jsonutils.loads(updates['spec'])['nova_object.data'] + self.assertNotIn('network_metadata', data) + self.assertIsNone(data['requested_destination']) + self.assertIsNone(data['retry']) + 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 + self.assertIsNotNone(req_obj.network_metadata) + self.assertJsonEqual(expected_network_metadata.obj_to_primitive(), + req_obj.network_metadata.obj_to_primitive()) + self.assertIsNotNone(req_obj.requested_destination) + self.assertJsonEqual(expected_destination.obj_to_primitive(), + req_obj.requested_destination.obj_to_primitive()) + self.assertIsNotNone(req_obj.retry) + self.assertJsonEqual(expected_retry.obj_to_primitive(), + req_obj.retry.obj_to_primitive()) + def test_save(self): req_obj = fake_request_spec.fake_spec_obj() # Make sure the requested_destination is not persisted since it is