Merge "Remove extra instance.save() calls related to qos SRIOV ports"

Author: Zuul

nova/compute/manager.py

@@ -8552,6 +8552,18 @@ def _rollback_live_migration(self, context, instance,
                             None
 
         """
+        # NOTE(gibi): We need to refresh pci_requests of the instance as it
+        # might be changed by the conductor during scheduling based on the
+        # selected destination host. If the instance has SRIOV ports with
+        # resource request then the LiveMigrationTask._find_destination call
+        # updated the instance.pci_requests.requests[].spec with the SRIOV PF
+        # device name to be used on the destination host. As the migration is
+        # rolling back to the source host now we don't want to persist the
+        # destination host related changes in the DB.
+        instance.pci_requests = \
+            objects.InstancePCIRequests.get_by_instance_uuid(
+                context, instance.uuid)
+
         if (isinstance(migrate_data, migrate_data_obj.LiveMigrateData) and
               migrate_data.obj_attr_is_set('migration')):
             migration = migrate_data.migration

nova/compute/resource_tracker.py

@@ -156,8 +156,7 @@ def instance_claim(self, context, instance, nodename, allocations,
                         instance=instance)
 
         cn = self.compute_nodes[nodename]
-        pci_requests = objects.InstancePCIRequests.get_by_instance_uuid(
-            context, instance.uuid)
+        pci_requests = instance.pci_requests
         claim = claims.Claim(context, instance, nodename, self, cn,
                              pci_requests, limits=limits)
 

nova/compute/utils.py

@@ -1509,7 +1509,6 @@ def needs_update(pci_request, mapping):
         return (pci_request.requester_id and
                 pci_request.requester_id in mapping)
 
-    modified = False
     for pci_request in instance.pci_requests.requests:
         if needs_update(pci_request, provider_mapping):
 
@@ -1535,6 +1534,3 @@ def needs_update(pci_request, mapping):
 
             for spec in pci_request.spec:
                 spec['parent_ifname'] = rp_name_pieces[2]
-                modified = True
-    if modified:
-        instance.save()

nova/conductor/tasks/live_migrate.py

@@ -9,7 +9,6 @@
 #    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 copy
 
 from oslo_log import log as logging
 import oslo_messaging as messaging
@@ -68,7 +67,6 @@ def __init__(self, context, instance, destination,
         self._source_cn = None
         self._held_allocations = None
         self.network_api = neutron.API()
-        self.instance_pci_request = None
 
     def _execute(self):
         self._check_instance_is_active()
@@ -84,11 +82,6 @@ def _execute(self):
                                                       self.instance,
                                                       self.migration))
 
-        # NOTE(gibi): migrating with qos SRIOV ports will update the PCI
-        # request so we saving the request here so that we can roll that change
-        # back if the migration fails.
-        self.instance_pci_request = copy.deepcopy(self.instance.pci_requests)
-
         if not self.destination:
             # Either no host was specified in the API request and the user
             # wants the scheduler to pick a destination host, or a host was
@@ -158,9 +151,6 @@ def rollback(self, ex):
                                                     self._source_cn,
                                                     self.instance,
                                                     self.migration)
-        if self.instance_pci_request:
-            self.instance.pci_requests = self.instance_pci_request
-            self.instance.save()
 
     def _check_instance_is_active(self):
         if self.instance.power_state not in (power_state.RUNNING,
@@ -521,6 +511,10 @@ def _find_destination(self):
             provider_mapping = request_spec.get_request_group_mapping()
 
             if provider_mapping:
+                # NOTE(gibi): this call might update the pci_requests of the
+                # instance based on the destination host if so then such change
+                # will be persisted when post_live_migration_at_destination
+                # runs.
                 compute_utils.\
                     update_pci_request_spec_with_allocated_interface_name(
                         self.context, self.report_client, self.instance,

nova/tests/functional/test_servers.py

@@ -6653,6 +6653,8 @@ def _test_resize_or_migrate_server_with_qos_ports(self, new_flavor=None):
             migration_uuid, source_compute_rp_uuid=self.compute1_rp_uuid,
             new_flavor=new_flavor)
 
+        self._assert_pci_request_pf_device_name(server, 'host2-ens2')
+
         self._confirm_resize(server)
 
         # check that allocation is still OK
@@ -6789,6 +6791,8 @@ def fake_prep_resize(_self, *args, **kwargs):
             source_compute_rp_uuid=self.compute1_rp_uuid,
             new_flavor=new_flavor)
 
+        self._assert_pci_request_pf_device_name(server, 'host3-ens2')
+
         self._confirm_resize(server)
 
         # check that allocation is still OK
@@ -7141,6 +7145,8 @@ def test_evacuate_with_qos_port(self, host=None):
             self.compute2_rp_uuid, non_qos_normal_port, qos_normal_port,
             qos_sriov_port)
 
+        self._assert_pci_request_pf_device_name(server, 'host2-ens2')
+
         # recover source compute
         self.admin_api.put_service(
             self.compute1_service_id, {'forced_down': 'false'})
@@ -7300,6 +7306,8 @@ def test_live_migrate_with_qos_port(self, host=None):
             server, self.compute2_rp_uuid, non_qos_normal_port,
             qos_normal_port, qos_sriov_port, self.flavor_with_group_policy)
 
+        self._assert_pci_request_pf_device_name(server, 'host2-ens2')
+
         self._delete_server_and_check_allocations(
             server, qos_normal_port, qos_sriov_port)
 
@@ -7364,6 +7372,8 @@ def fake_check_can_live_migrate_destination(
             server, compute3_rp_uuid, non_qos_normal_port,
             qos_normal_port, qos_sriov_port, self.flavor_with_group_policy)
 
+        self._assert_pci_request_pf_device_name(server, 'host3-ens2')
+
         self._delete_server_and_check_allocations(
             server, qos_normal_port, qos_sriov_port)
 
@@ -7529,10 +7539,7 @@ def test_live_migrate_with_qos_port_abort_migration(self):
             qos_normal_port, qos_sriov_port, self.flavor_with_group_policy)
 
         # Assert that the InstancePCIRequests rolled back to point to host1
-        # This assert is fails now as the abort does not change the PCI device
-        # back
-        # TODO(gibi): come up with an idea to fix this
-        # self._assert_pci_request_pf_device_name(server, 'host1-ens2')
+        self._assert_pci_request_pf_device_name(server, 'host1-ens2')
 
         self._delete_server_and_check_allocations(
             server, qos_normal_port, qos_sriov_port)

nova/tests/unit/compute/test_compute.py

@@ -6578,6 +6578,7 @@ def test_rollback_live_migration(self, mock_bdms, mock_get_node,
         bdms = objects.BlockDeviceMappingList()
         mock_bdms.return_value = bdms
 
+        @mock.patch('nova.objects.InstancePCIRequests.get_by_instance_uuid')
         @mock.patch('nova.compute.utils.notify_about_instance_action')
         @mock.patch.object(migration, 'save')
         @mock.patch.object(self.compute, '_revert_allocation')
@@ -6586,7 +6587,8 @@ def test_rollback_live_migration(self, mock_bdms, mock_get_node,
         @mock.patch.object(compute_rpcapi.ComputeAPI,
                            'drop_move_claim_at_destination')
         def _test(mock_drop_claim, mock_nw_api, mock_lmcf, mock_ra,
-                  mock_mig_save, mock_notify):
+                  mock_mig_save, mock_notify, mock_get_pci):
+            mock_get_pci.return_value = objects.InstancePCIRequests()
             mock_lmcf.return_value = False, False
             if migration_status:
                 self.compute._rollback_live_migration(
@@ -6611,6 +6613,8 @@ def _test(mock_drop_claim, mock_nw_api, mock_lmcf, mock_ra,
             ])
             mock_ra.assert_called_once_with(mock.ANY, instance, migration)
             mock_mig_save.assert_called_once_with()
+            mock_get_pci.assert_called_once_with(c, instance.uuid)
+            self.assertEqual(mock_get_pci.return_value, instance.pci_requests)
 
         _test()
 
@@ -6637,6 +6641,7 @@ def test_rollback_live_migration_network_teardown_fails(
         migrate_data = objects.LibvirtLiveMigrateData(migration=migration)
         source_bdms = objects.BlockDeviceMappingList()
 
+        @mock.patch('nova.objects.InstancePCIRequests.get_by_instance_uuid')
         @mock.patch.object(self.compute, '_notify_about_instance_usage')
         @mock.patch('nova.compute.utils.notify_about_instance_action')
         @mock.patch.object(instance, 'save')
@@ -6647,7 +6652,10 @@ def test_rollback_live_migration_network_teardown_fails(
         @mock.patch.object(self.compute.network_api, 'setup_networks_on_host',
                            side_effect=(None, test.TestingException))
         def _test(mock_nw_setup, _mock_lmcf, mock_ra, mock_mig_save,
-                  mock_inst_save, _mock_notify_action, mock_notify_usage):
+                  mock_inst_save, _mock_notify_action, mock_notify_usage,
+                  mock_get_pci):
+            mock_get_pci.return_value = objects.InstancePCIRequests()
+
             self.assertRaises(test.TestingException,
                               self.compute._rollback_live_migration,
                               ctxt, instance, 'dest-host', migrate_data,
@@ -6672,6 +6680,8 @@ def _test(mock_nw_setup, _mock_lmcf, mock_ra, mock_mig_save,
             # Since we failed during rollback, the migration status gets set
             # to 'error' instead of 'goofballs'.
             self.assertEqual('error', migration.status)
+            mock_get_pci.assert_called_once_with(ctxt, instance.uuid)
+            self.assertEqual(mock_get_pci.return_value, instance.pci_requests)
 
         _test()
 

nova/tests/unit/compute/test_compute_mgr.py

@@ -9546,6 +9546,7 @@ def fake_bdm():
         bdm.connection_info = new_attachment_id
         bdms = objects.BlockDeviceMappingList(objects=[bdm])
 
+        @mock.patch('nova.objects.InstancePCIRequests.get_by_instance_uuid')
         @mock.patch.object(compute.volume_api, 'attachment_delete')
         @mock.patch.object(compute_utils, 'notify_about_instance_action')
         @mock.patch.object(instance, 'save')
@@ -9557,11 +9558,12 @@ def fake_bdm():
         @mock.patch.object(objects.Instance, 'drop_migration_context')
         def _test(mock_drop_mig_ctxt, mock_get_bdms, mock_net_api,
                   mock_remove_conn, mock_usage, mock_instance_save,
-                  mock_action, mock_attach_delete):
+                  mock_action, mock_attach_delete, mock_get_pci):
             # this tests that _rollback_live_migration replaces the bdm's
             # attachment_id with the original attachment id that is in
             # migrate_data.
             mock_get_bdms.return_value = bdms
+            mock_get_pci.return_value = objects.InstancePCIRequests()
 
             compute._rollback_live_migration(self.context, instance, None,
                                              migrate_data=migrate_data,
@@ -9575,6 +9577,8 @@ def _test(mock_drop_mig_ctxt, mock_get_bdms, mock_net_api,
             self.assertEqual(orig_attachment_id, bdm.connection_info)
             bdm.save.assert_called_once_with()
             mock_drop_mig_ctxt.assert_called_once_with()
+            mock_get_pci.assert_called_once_with(self.context, instance.uuid)
+            self.assertEqual(mock_get_pci.return_value, instance.pci_requests)
 
         _test()
 
@@ -9591,13 +9595,14 @@ def test_rollback_live_migration_port_binding_delete_fails(
             migration=self.migration, is_shared_instance_path=True,
             is_shared_block_storage=True)
 
+        @mock.patch('nova.objects.InstancePCIRequests.get_by_instance_uuid')
         @mock.patch.object(self.compute, '_revert_allocation')
         @mock.patch.object(self.instance, 'save')
         @mock.patch.object(self.compute, 'network_api')
         @mock.patch.object(self.compute, '_notify_about_instance_usage')
         @mock.patch.object(objects.Instance, 'drop_migration_context')
         def _do_test(drop_mig_ctxt, legacy_notify, network_api, instance_save,
-                     _revert_allocation):
+                     _revert_allocation, mock_get_pci):
             # setup_networks_on_host is called two times:
             # 1. set the migrating_to attribute in the port binding profile,
             #    which is a no-op in this case for neutron
@@ -9606,13 +9611,18 @@ def _do_test(drop_mig_ctxt, legacy_notify, network_api, instance_save,
                 None,
                 exception.PortBindingDeletionFailed(
                     port_id=uuids.port_id, host='fake-dest-host')]
+            mock_get_pci.return_value = objects.InstancePCIRequests()
             self.compute._rollback_live_migration(
                 self.context, self.instance, 'fake-dest-host', migrate_data,
                 source_bdms=objects.BlockDeviceMappingList())
             self.assertEqual(1, mock_log_error.call_count)
             self.assertIn('Network cleanup failed for destination host',
                           mock_log_error.call_args[0][0])
             drop_mig_ctxt.assert_called_once_with()
+            mock_get_pci.assert_called_once_with(
+                self.context, self.instance.uuid)
+            self.assertEqual(
+                mock_get_pci.return_value, self.instance.pci_requests)
 
         _do_test()