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.

Change-Id: I2131558f0edfe603ee1e8d8bae66a3caf5182a58
Closes-Bug: #1815153
tags/19.0.0.0rc1
Takashi Natsume 7 months ago
parent
commit
67d5970445

+ 10
- 2
nova/objects/request_spec.py View File

@@ -525,13 +525,21 @@ class RequestSpec(base.NovaObject):
525 525
             # though they should match.
526 526
             if key in ['id', 'instance_uuid']:
527 527
                 setattr(spec, key, db_spec[key])
528
-            elif key == 'requested_resources':
528
+            elif key in ('requested_destination', 'requested_resources',
529
+                         'network_metadata'):
529 530
                 # Do not override what we already have in the object as this
530 531
                 # field is not persisted. If save() is called after
531
-                # requested_resources is populated, it will reset the field to
532
+                # requested_resources, requested_destination or
533
+                # network_metadata is populated, it will reset the field to
532 534
                 # None and we'll lose what is set (but not persisted) on the
533 535
                 # object.
534 536
                 continue
537
+            elif key == 'retry':
538
+                # NOTE(takashin): Do not override the 'retry' field
539
+                # which is not a persistent. It is not lazy-loadable field.
540
+                # If it is not set, set None.
541
+                if not spec.obj_attr_is_set(key):
542
+                    setattr(spec, key, None)
535 543
             elif key in spec_obj:
536 544
                 setattr(spec, key, getattr(spec_obj, key))
537 545
         spec._context = context

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

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

+ 78
- 11
nova/tests/unit/objects/test_request_spec.py View File

@@ -587,7 +587,8 @@ class _TestRequestSpecObject(object):
587 587
         self.assertIsInstance(req_obj.pci_requests,
588 588
                 objects.InstancePCIRequests)
589 589
         self.assertIsInstance(req_obj.flavor, objects.Flavor)
590
-        self.assertIsInstance(req_obj.retry, objects.SchedulerRetries)
590
+        # The 'retry' field is not persistent.
591
+        self.assertIsNone(req_obj.retry)
591 592
         self.assertIsInstance(req_obj.limits, objects.SchedulerLimits)
592 593
         self.assertIsInstance(req_obj.instance_group, objects.InstanceGroup)
593 594
         self.assertEqual('fresh', req_obj.instance_group.name)
@@ -635,10 +636,24 @@ class _TestRequestSpecObject(object):
635 636
 
636 637
         self.assertRaises(exception.ObjectActionError, req_obj.create)
637 638
 
638
-    def test_create_does_not_persist_requested_resources(self):
639
+    def test_create_does_not_persist_requested_fields(self):
639 640
         req_obj = fake_request_spec.fake_spec_obj(remove_id=True)
641
+
642
+        expected_network_metadata = objects.NetworkMetadata(
643
+            physnets=set(['foo', 'bar']), tunneled=True)
644
+        req_obj.network_metadata = expected_network_metadata
645
+        expected_destination = request_spec.Destination(host='sample-host')
646
+        req_obj.requested_destination = expected_destination
640 647
         rg = request_spec.RequestGroup(resources={'fake-rc': 13})
641 648
         req_obj.requested_resources = [rg]
649
+        expected_retry = objects.SchedulerRetries(
650
+            num_attempts=2,
651
+            hosts=objects.ComputeNodeList(objects=[
652
+                objects.ComputeNode(host='host1', hypervisor_hostname='node1'),
653
+                objects.ComputeNode(host='host2', hypervisor_hostname='node2'),
654
+            ]))
655
+        req_obj.retry = expected_retry
656
+
642 657
         orig_create_in_db = request_spec.RequestSpec._create_in_db
643 658
         with mock.patch.object(request_spec.RequestSpec, '_create_in_db') \
644 659
                 as mock_create_in_db:
@@ -646,22 +661,54 @@ class _TestRequestSpecObject(object):
646 661
             req_obj.create()
647 662
             mock_create_in_db.assert_called_once()
648 663
             updates = mock_create_in_db.mock_calls[0][1][1]
649
-            # assert that the requested_resources field is not stored in the db
664
+            # assert that the following fields are not stored in the db
665
+            # 1. network_metadata
666
+            # 2. requested_destination
667
+            # 3. requested_resources
668
+            # 4. retry
650 669
             data = jsonutils.loads(updates['spec'])['nova_object.data']
670
+            self.assertNotIn('network_metadata', data)
671
+            self.assertIsNone(data['requested_destination'])
651 672
             self.assertIsNone(data['requested_resources'])
673
+            self.assertIsNone(data['retry'])
652 674
             self.assertIsNotNone(data['instance_uuid'])
653 675
 
654
-        # also we expect that requested_resources field does not reset after
655
-        # create
676
+        # also we expect that the following fields are not reset after create
677
+        # 1. network_metadata
678
+        # 2. requested_destination
679
+        # 3. requested_resources
680
+        # 4. retry
681
+        self.assertIsNotNone(req_obj.network_metadata)
682
+        self.assertJsonEqual(expected_network_metadata.obj_to_primitive(),
683
+                             req_obj.network_metadata.obj_to_primitive())
684
+        self.assertIsNotNone(req_obj.requested_destination)
685
+        self.assertJsonEqual(expected_destination.obj_to_primitive(),
686
+                             req_obj.requested_destination.obj_to_primitive())
687
+        self.assertIsNotNone(req_obj.requested_resources)
656 688
         self.assertEqual(
657 689
             13, req_obj.requested_resources[0].resources['fake-rc'])
690
+        self.assertIsNotNone(req_obj.retry)
691
+        self.assertJsonEqual(expected_retry.obj_to_primitive(),
692
+                             req_obj.retry.obj_to_primitive())
658 693
 
659
-    def test_save_does_not_persist_requested_resources(self):
694
+    def test_save_does_not_persist_requested_fields(self):
660 695
         req_obj = fake_request_spec.fake_spec_obj(remove_id=True)
661
-        rg = request_spec.RequestGroup(resources={'fake-rc': 13})
662
-        req_obj.requested_resources = [rg]
663 696
         req_obj.create()
664 697
         # change something to make sure _save_in_db is called
698
+        expected_network_metadata = objects.NetworkMetadata(
699
+            physnets=set(['foo', 'bar']), tunneled=True)
700
+        req_obj.network_metadata = expected_network_metadata
701
+        expected_destination = request_spec.Destination(host='sample-host')
702
+        req_obj.requested_destination = expected_destination
703
+        rg = request_spec.RequestGroup(resources={'fake-rc': 13})
704
+        req_obj.requested_resources = [rg]
705
+        expected_retry = objects.SchedulerRetries(
706
+            num_attempts=2,
707
+            hosts=objects.ComputeNodeList(objects=[
708
+                objects.ComputeNode(host='host1', hypervisor_hostname='node1'),
709
+                objects.ComputeNode(host='host2', hypervisor_hostname='node2'),
710
+            ]))
711
+        req_obj.retry = expected_retry
665 712
         req_obj.num_instances = 2
666 713
 
667 714
         orig_save_in_db = request_spec.RequestSpec._save_in_db
@@ -671,15 +718,35 @@ class _TestRequestSpecObject(object):
671 718
             req_obj.save()
672 719
             mock_save_in_db.assert_called_once()
673 720
             updates = mock_save_in_db.mock_calls[0][1][2]
674
-            # assert that the requested_resources field is not stored in the db
721
+            # assert that the following fields are not stored in the db
722
+            # 1. network_metadata
723
+            # 2. requested_destination
724
+            # 3. requested_resources
725
+            # 4. retry
675 726
             data = jsonutils.loads(updates['spec'])['nova_object.data']
727
+            self.assertNotIn('network_metadata', data)
728
+            self.assertIsNone(data['requested_destination'])
676 729
             self.assertIsNone(data['requested_resources'])
730
+            self.assertIsNone(data['retry'])
677 731
             self.assertIsNotNone(data['instance_uuid'])
678 732
 
679
-        # also we expect that requested_resources field does not reset after
680
-        # save
733
+        # also we expect that the following fields are not reset after save
734
+        # 1. network_metadata
735
+        # 2. requested_destination
736
+        # 3. requested_resources
737
+        # 4. retry
738
+        self.assertIsNotNone(req_obj.network_metadata)
739
+        self.assertJsonEqual(expected_network_metadata.obj_to_primitive(),
740
+                             req_obj.network_metadata.obj_to_primitive())
741
+        self.assertIsNotNone(req_obj.requested_destination)
742
+        self.assertJsonEqual(expected_destination.obj_to_primitive(),
743
+                             req_obj.requested_destination.obj_to_primitive())
744
+        self.assertIsNotNone(req_obj.requested_resources)
681 745
         self.assertEqual(13, req_obj.requested_resources[0].resources
682 746
                              ['fake-rc'])
747
+        self.assertIsNotNone(req_obj.retry)
748
+        self.assertJsonEqual(expected_retry.obj_to_primitive(),
749
+                             req_obj.retry.obj_to_primitive())
683 750
 
684 751
     def test_save(self):
685 752
         req_obj = fake_request_spec.fake_spec_obj()

Loading…
Cancel
Save