Fix resetting non-persistent fields when saving obj

The 'requested_destination', 'network_metadata', 'retry' fields
in the RequestSpec object are reset when saving the object currently.

When cold migrating a server, the API sets the requested_destination
so conductor will pass that information to the scheduler
to restrict the cold migration to that host.
But the 'heal_reqspec_is_bfv' method called from the conductor
makes an update to the RequestSpec which resets
the requested_destination so the server could end up being cold migrated
to some other host than the one that was requested by the API user.

So make them not be reset when saving the object.

Change-Id: I2131558f0edfe603ee1e8d8bae66a3caf5182a58
Closes-Bug: #1815153

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()