Browse Source

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.

Conflicts:
    nova/objects/request_spec.py
    nova/tests/unit/objects/test_request_spec.py

    Conflicts are due to not including the following changes in rocky.

    I53e5debcffd6de2b3a2ff838e7f5da33fa1a82b8
    Idaed39629095f86d24a54334c699a26c218c6593

Change-Id: I2131558f0edfe603ee1e8d8bae66a3caf5182a58
Closes-Bug: #1815153
(cherry picked from commit 67d5970445)
tags/18.2.0
Takashi Natsume 7 months ago
parent
commit
02d6146e4d

+ 13
- 0
nova/objects/request_spec.py View File

@@ -497,6 +497,19 @@ class RequestSpec(base.NovaObject):
497 497
             # though they should match.
498 498
             if key in ['id', 'instance_uuid']:
499 499
                 setattr(spec, key, db_spec[key])
500
+            elif key in ('requested_destination', 'network_metadata'):
501
+                # Do not override what we already have in the object as this
502
+                # field is not persisted. If save() is called after
503
+                # requested_destination or network_metadata is populated,
504
+                # it will reset the field to None and we'll lose what is set
505
+                # (but not persisted) on the object.
506
+                continue
507
+            elif key == 'retry':
508
+                # NOTE(takashin): Do not override the 'retry' field
509
+                # which is not a persistent. It is not lazy-loadable field.
510
+                # If it is not set, set None.
511
+                if not spec.obj_attr_is_set(key):
512
+                    setattr(spec, key, None)
500 513
             elif key in spec_obj:
501 514
                 setattr(spec, key, getattr(spec_obj, key))
502 515
         spec._context = context

+ 173
- 0
nova/tests/functional/regressions/test_bug_1815153.py View File

@@ -0,0 +1,173 @@
1
+# Copyright 2019 NTT Corporation
2
+#
3
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
4
+# not use this file except in compliance with the License. You may obtain
5
+# a copy of the License at
6
+#
7
+#      http://www.apache.org/licenses/LICENSE-2.0
8
+#
9
+# Unless required by applicable law or agreed to in writing, software
10
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
11
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
12
+# License for the specific language governing permissions and limitations
13
+# under the License.
14
+
15
+import six
16
+
17
+from nova import context
18
+from nova import objects
19
+from nova import test
20
+from nova.tests import fixtures as nova_fixtures
21
+from nova.tests.functional.api import client
22
+from nova.tests.functional import integrated_helpers
23
+from nova.tests.unit.image import fake as image_fake
24
+from nova.tests.unit import policy_fixture
25
+from nova.virt import fake
26
+
27
+
28
+class NonPersistentFieldNotResetTest(
29
+        test.TestCase, integrated_helpers.InstanceHelperMixin):
30
+    """Test for regression bug 1815153
31
+
32
+    The bug is that the 'requested_destination' field in the RequestSpec
33
+    object is reset when saving the object in the 'heal_reqspec_is_bfv'
34
+    method in the case that a server created before Rocky which does not
35
+    have is_bfv field.
36
+
37
+    Tests the following two cases here.
38
+
39
+    * Cold migration with a target host without a force flag
40
+    * Evacuate with a target host without a force flag
41
+
42
+    The following two cases are not tested here because
43
+    'requested_destination' is not set when the 'heal_reqspec_is_bfv' method
44
+    is called.
45
+
46
+    * Live migration without a destination host.
47
+    * Unshelve a server
48
+    """
49
+
50
+    def setUp(self):
51
+        super(NonPersistentFieldNotResetTest, self).setUp()
52
+        self.useFixture(policy_fixture.RealPolicyFixture())
53
+        self.useFixture(nova_fixtures.NeutronFixture(self))
54
+        self.useFixture(nova_fixtures.PlacementFixture())
55
+
56
+        api_fixture = self.useFixture(nova_fixtures.OSAPIFixture(
57
+            api_version='v2.1'))
58
+        self.api = api_fixture.admin_api
59
+        # Use the latest microversion available to make sure something does
60
+        # not regress in new microversions; cap as necessary.
61
+        self.api.microversion = 'latest'
62
+
63
+        image_fake.stub_out_image_service(self)
64
+        self.addCleanup(image_fake.FakeImageService_reset)
65
+
66
+        self.start_service('conductor')
67
+        self.start_service('scheduler')
68
+
69
+        self.compute = {}
70
+
71
+        self.addCleanup(fake.restore_nodes)
72
+        for host in ('host1', 'host2', 'host3'):
73
+            fake.set_nodes([host])
74
+            compute_service = self.start_service('compute', host=host)
75
+            self.compute.update({host: compute_service})
76
+
77
+        self.ctxt = context.get_admin_context()
78
+
79
+    @staticmethod
80
+    def _get_target_host(host):
81
+        target_host = {'host1': 'host2',
82
+                       'host2': 'host3',
83
+                       'host3': 'host1'}
84
+        return target_host[host]
85
+
86
+    def _create_server(self):
87
+        # Create a server, it doesn't matter on which host it builds.
88
+        server = self._build_minimal_create_server_request(
89
+            self.api, 'sample-server',
90
+            image_uuid='155d900f-4e14-4e4c-a73d-069cbf4541e6',
91
+            networks='none')
92
+        server = self.api.post_server({'server': server})
93
+        server = self._wait_for_state_change(self.api, server, 'ACTIVE')
94
+
95
+        return server
96
+
97
+    def _remove_is_bfv_in_request_spec(self, server_id):
98
+        # Now let's hack the RequestSpec.is_bfv field to mimic migrating an
99
+        # old instance created before RequestSpec.is_bfv was set in the API,
100
+        reqspec = objects.RequestSpec.get_by_instance_uuid(self.ctxt,
101
+                                                           server_id)
102
+        del reqspec.is_bfv
103
+        reqspec.save()
104
+        reqspec = objects.RequestSpec.get_by_instance_uuid(self.ctxt,
105
+                                                           server_id)
106
+        # Make sure 'is_bfv' is not set.
107
+        self.assertNotIn('is_bfv', reqspec)
108
+
109
+    def test_cold_migrate(self):
110
+        server = self._create_server()
111
+        original_host = server['OS-EXT-SRV-ATTR:host']
112
+        target_host = self._get_target_host(original_host)
113
+        self._remove_is_bfv_in_request_spec(server['id'])
114
+
115
+        # Force a target host down
116
+        source_compute_id = self.api.get_services(
117
+            host=target_host, binary='nova-compute')[0]['id']
118
+        self.compute[target_host].stop()
119
+        self.api.put_service(
120
+            source_compute_id, {'forced_down': 'true'})
121
+
122
+        # Cold migrate a server with a target host.
123
+        # If requested_destination is reset, the server is moved to a host
124
+        # that is not a requested target host.
125
+        # In that case, the response code is 202.
126
+        # If requested_destination is not reset, no valid host error (400) is
127
+        # returned because the target host is down.
128
+        ex = self.assertRaises(client.OpenStackApiException,
129
+                               self.api.post_server_action,
130
+                               server['id'],
131
+                               {'migrate': {'host': target_host}})
132
+        self.assertEqual(400, ex.response.status_code)
133
+        self.assertIn('No valid host was found. No valid host found '
134
+                      'for cold migrate', six.text_type(ex))
135
+
136
+        # Make sure 'is_bfv' is set.
137
+        reqspec = objects.RequestSpec.get_by_instance_uuid(self.ctxt,
138
+                                                           server['id'])
139
+        self.assertIn('is_bfv', reqspec)
140
+        self.assertIs(reqspec.is_bfv, False)
141
+
142
+    def test_evacuate(self):
143
+        server = self._create_server()
144
+        original_host = server['OS-EXT-SRV-ATTR:host']
145
+        target_host = self._get_target_host(original_host)
146
+        self._remove_is_bfv_in_request_spec(server['id'])
147
+
148
+        # Force source and target hosts down
149
+        for host in (original_host, target_host):
150
+            source_compute_id = self.api.get_services(
151
+                host=host, binary='nova-compute')[0]['id']
152
+            self.compute[host].stop()
153
+            self.api.put_service(
154
+                source_compute_id, {'forced_down': 'true'})
155
+
156
+        # Evacuate a server with a target host.
157
+        # If requested_destination is reset, the server is moved to a host
158
+        # that is not the target host.
159
+        # Its status becomes 'ACTIVE'.
160
+        # If requested_destination is not reset, a status of the server
161
+        # becomes 'ERROR' because the target host is down.
162
+        self.api.post_server_action(
163
+            server['id'], {'evacuate': {'host': target_host}})
164
+        expected_params = {'OS-EXT-SRV-ATTR:host': original_host,
165
+                           'status': 'ERROR'}
166
+        server = self._wait_for_server_parameter(self.api, server,
167
+                                                 expected_params)
168
+
169
+        # Make sure 'is_bfv' is set.
170
+        reqspec = objects.RequestSpec.get_by_instance_uuid(self.ctxt,
171
+                                                           server['id'])
172
+        self.assertIn('is_bfv', reqspec)
173
+        self.assertIs(reqspec.is_bfv, False)

+ 2
- 0
nova/tests/unit/fake_request_spec.py View File

@@ -57,6 +57,8 @@ PCI_REQUESTS.obj_reset_changes(recursive=True)
57 57
 
58 58
 def fake_db_spec():
59 59
     req_obj = fake_spec_obj()
60
+    # NOTE(takashin): There is not 'retry' information in the DB table.
61
+    del req_obj.retry
60 62
     db_request_spec = {
61 63
             'id': 1,
62 64
             'instance_uuid': req_obj.instance_uuid,

+ 97
- 1
nova/tests/unit/objects/test_request_spec.py View File

@@ -567,7 +567,8 @@ class _TestRequestSpecObject(object):
567 567
         self.assertIsInstance(req_obj.pci_requests,
568 568
                 objects.InstancePCIRequests)
569 569
         self.assertIsInstance(req_obj.flavor, objects.Flavor)
570
-        self.assertIsInstance(req_obj.retry, objects.SchedulerRetries)
570
+        # The 'retry' field is not persistent.
571
+        self.assertIsNone(req_obj.retry)
571 572
         self.assertIsInstance(req_obj.limits, objects.SchedulerLimits)
572 573
         self.assertIsInstance(req_obj.instance_group, objects.InstanceGroup)
573 574
         self.assertEqual('fresh', req_obj.instance_group.name)
@@ -615,6 +616,101 @@ class _TestRequestSpecObject(object):
615 616
 
616 617
         self.assertRaises(exception.ObjectActionError, req_obj.create)
617 618
 
619
+    def test_create_does_not_persist_requested_fields(self):
620
+        req_obj = fake_request_spec.fake_spec_obj(remove_id=True)
621
+
622
+        expected_network_metadata = objects.NetworkMetadata(
623
+            physnets=set(['foo', 'bar']), tunneled=True)
624
+        req_obj.network_metadata = expected_network_metadata
625
+        expected_destination = request_spec.Destination(host='sample-host')
626
+        req_obj.requested_destination = expected_destination
627
+        expected_retry = objects.SchedulerRetries(
628
+            num_attempts=2,
629
+            hosts=objects.ComputeNodeList(objects=[
630
+                objects.ComputeNode(host='host1', hypervisor_hostname='node1'),
631
+                objects.ComputeNode(host='host2', hypervisor_hostname='node2'),
632
+            ]))
633
+        req_obj.retry = expected_retry
634
+
635
+        orig_create_in_db = request_spec.RequestSpec._create_in_db
636
+        with mock.patch.object(request_spec.RequestSpec, '_create_in_db') \
637
+                as mock_create_in_db:
638
+            mock_create_in_db.side_effect = orig_create_in_db
639
+            req_obj.create()
640
+            mock_create_in_db.assert_called_once()
641
+            updates = mock_create_in_db.mock_calls[0][1][1]
642
+            # assert that the following fields are not stored in the db
643
+            # 1. network_metadata
644
+            # 2. requested_destination
645
+            # 3. retry
646
+            data = jsonutils.loads(updates['spec'])['nova_object.data']
647
+            self.assertNotIn('network_metadata', data)
648
+            self.assertIsNone(data['requested_destination'])
649
+            self.assertIsNone(data['retry'])
650
+            self.assertIsNotNone(data['instance_uuid'])
651
+
652
+        # also we expect that the following fields are not reset after create
653
+        # 1. network_metadata
654
+        # 2. requested_destination
655
+        # 3. retry
656
+        self.assertIsNotNone(req_obj.network_metadata)
657
+        self.assertJsonEqual(expected_network_metadata.obj_to_primitive(),
658
+                             req_obj.network_metadata.obj_to_primitive())
659
+        self.assertIsNotNone(req_obj.requested_destination)
660
+        self.assertJsonEqual(expected_destination.obj_to_primitive(),
661
+                             req_obj.requested_destination.obj_to_primitive())
662
+        self.assertIsNotNone(req_obj.retry)
663
+        self.assertJsonEqual(expected_retry.obj_to_primitive(),
664
+                             req_obj.retry.obj_to_primitive())
665
+
666
+    def test_save_does_not_persist_requested_fields(self):
667
+        req_obj = fake_request_spec.fake_spec_obj(remove_id=True)
668
+        req_obj.create()
669
+        # change something to make sure _save_in_db is called
670
+        expected_network_metadata = objects.NetworkMetadata(
671
+            physnets=set(['foo', 'bar']), tunneled=True)
672
+        req_obj.network_metadata = expected_network_metadata
673
+        expected_destination = request_spec.Destination(host='sample-host')
674
+        req_obj.requested_destination = expected_destination
675
+        expected_retry = objects.SchedulerRetries(
676
+            num_attempts=2,
677
+            hosts=objects.ComputeNodeList(objects=[
678
+                objects.ComputeNode(host='host1', hypervisor_hostname='node1'),
679
+                objects.ComputeNode(host='host2', hypervisor_hostname='node2'),
680
+            ]))
681
+        req_obj.retry = expected_retry
682
+
683
+        orig_save_in_db = request_spec.RequestSpec._save_in_db
684
+        with mock.patch.object(request_spec.RequestSpec, '_save_in_db') \
685
+                as mock_save_in_db:
686
+            mock_save_in_db.side_effect = orig_save_in_db
687
+            req_obj.save()
688
+            mock_save_in_db.assert_called_once()
689
+            updates = mock_save_in_db.mock_calls[0][1][2]
690
+            # assert that the following fields are not stored in the db
691
+            # 1. network_metadata
692
+            # 2. requested_destination
693
+            # 3. retry
694
+            data = jsonutils.loads(updates['spec'])['nova_object.data']
695
+            self.assertNotIn('network_metadata', data)
696
+            self.assertIsNone(data['requested_destination'])
697
+            self.assertIsNone(data['retry'])
698
+            self.assertIsNotNone(data['instance_uuid'])
699
+
700
+        # also we expect that the following fields are not reset after save
701
+        # 1. network_metadata
702
+        # 2. requested_destination
703
+        # 3. retry
704
+        self.assertIsNotNone(req_obj.network_metadata)
705
+        self.assertJsonEqual(expected_network_metadata.obj_to_primitive(),
706
+                             req_obj.network_metadata.obj_to_primitive())
707
+        self.assertIsNotNone(req_obj.requested_destination)
708
+        self.assertJsonEqual(expected_destination.obj_to_primitive(),
709
+                             req_obj.requested_destination.obj_to_primitive())
710
+        self.assertIsNotNone(req_obj.retry)
711
+        self.assertJsonEqual(expected_retry.obj_to_primitive(),
712
+                             req_obj.retry.obj_to_primitive())
713
+
618 714
     def test_save(self):
619 715
         req_obj = fake_request_spec.fake_spec_obj()
620 716
         # Make sure the requested_destination is not persisted since it is

Loading…
Cancel
Save