diff options
| -rw-r--r-- | nova/objects/request_spec.py | 12 | ||||
| -rw-r--r-- | nova/tests/functional/regressions/test_bug_1815153.py | 174 | ||||
| -rw-r--r-- | nova/tests/unit/fake_request_spec.py | 2 | ||||
| -rw-r--r-- | nova/tests/unit/objects/test_request_spec.py | 89 |
4 files changed, 264 insertions, 13 deletions
diff --git a/nova/objects/request_spec.py b/nova/objects/request_spec.py index 9654e5d6b5..e9c97e9598 100644 --- a/nova/objects/request_spec.py +++ b/nova/objects/request_spec.py @@ -525,13 +525,21 @@ class RequestSpec(base.NovaObject): # though they should match. if key in ['id', 'instance_uuid']: setattr(spec, key, db_spec[key]) - elif key == 'requested_resources': + elif key in ('requested_destination', 'requested_resources', + 'network_metadata'): # Do not override what we already have in the object as this # field is not persisted. If save() is called after - # requested_resources is populated, it will reset the field to + # requested_resources, 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 0000000000..15012b0971 --- /dev/null +++ b/nova/tests/functional/regressions/test_bug_1815153.py @@ -0,0 +1,174 @@ +# 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 fixtures as func_fixtures +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(func_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 6d7e50fcb1..e7958eab90 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 142cd7dad4..988e4d1b8d 100644 --- a/nova/tests/unit/objects/test_request_spec.py +++ b/nova/tests/unit/objects/test_request_spec.py @@ -587,7 +587,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) @@ -635,10 +636,24 @@ class _TestRequestSpecObject(object): self.assertRaises(exception.ObjectActionError, req_obj.create) - def test_create_does_not_persist_requested_resources(self): + 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 rg = request_spec.RequestGroup(resources={'fake-rc': 13}) req_obj.requested_resources = [rg] + 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: @@ -646,22 +661,54 @@ class _TestRequestSpecObject(object): req_obj.create() mock_create_in_db.assert_called_once() updates = mock_create_in_db.mock_calls[0][1][1] - # assert that the requested_resources field is not stored in the db + # assert that the following fields are not stored in the db + # 1. network_metadata + # 2. requested_destination + # 3. requested_resources + # 4. retry data = jsonutils.loads(updates['spec'])['nova_object.data'] + self.assertNotIn('network_metadata', data) + self.assertIsNone(data['requested_destination']) self.assertIsNone(data['requested_resources']) + self.assertIsNone(data['retry']) self.assertIsNotNone(data['instance_uuid']) - # also we expect that requested_resources field does not reset after - # create + # also we expect that the following fields are not reset after create + # 1. network_metadata + # 2. requested_destination + # 3. requested_resources + # 4. 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.requested_resources) self.assertEqual( 13, req_obj.requested_resources[0].resources['fake-rc']) + 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_resources(self): + def test_save_does_not_persist_requested_fields(self): req_obj = fake_request_spec.fake_spec_obj(remove_id=True) - rg = request_spec.RequestGroup(resources={'fake-rc': 13}) - req_obj.requested_resources = [rg] 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 + rg = request_spec.RequestGroup(resources={'fake-rc': 13}) + req_obj.requested_resources = [rg] + 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 req_obj.num_instances = 2 orig_save_in_db = request_spec.RequestSpec._save_in_db @@ -671,15 +718,35 @@ class _TestRequestSpecObject(object): req_obj.save() mock_save_in_db.assert_called_once() updates = mock_save_in_db.mock_calls[0][1][2] - # assert that the requested_resources field is not stored in the db + # assert that the following fields are not stored in the db + # 1. network_metadata + # 2. requested_destination + # 3. requested_resources + # 4. retry data = jsonutils.loads(updates['spec'])['nova_object.data'] + self.assertNotIn('network_metadata', data) + self.assertIsNone(data['requested_destination']) self.assertIsNone(data['requested_resources']) + self.assertIsNone(data['retry']) self.assertIsNotNone(data['instance_uuid']) - # also we expect that requested_resources field does not reset after - # save + # also we expect that the following fields are not reset after save + # 1. network_metadata + # 2. requested_destination + # 3. requested_resources + # 4. 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.requested_resources) self.assertEqual(13, req_obj.requested_resources[0].resources ['fake-rc']) + 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() |