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

During creating or moving of an instance with qos SRIOV port the PCI
device claim on the destination compute needs to be restricted to select
PCI VFs from the same PF where the bandwidth for the qos port is
allocated from. This is achieved by updating the spec part of the
InstancePCIRequest with the device name of the PF by calling
update_pci_request_spec_with_allocated_interface_name(). Until now
such update of the instance object was directly persisted by the call.

During code review it was came up that the instance.save() in the util
is not appropriate as the caller has a lot more context to decide when
to persist the changes.

The original eager instance.save was introduced when support added to
the server create flow. Now I realized that the need for such save was
due to a mistake in the original ResourceTracker.instance_claim() call
that loads the InstancePCIRequest from the DB instead of using the
requests through the passed in instance object. By removing the extra DB
call the need for eagerly persisting the PCI spec update is also
removed. It turned out that both the server create code path and every
server move code paths eventually persist the instance object either
during at the end of the claim process or in case of live migration in
the post_live_migration_at_destination compute manager call. This means
that the code now can be simplified. Especially the live migration cases.

In the live migrate abort case we don't need to roll back the eagerly
persisted PCI change as now such change is only persisted at the end
of the migration but still we need to refresh pci_requests field of
the instance object during the rollback as that field might be stale,
containing dest host related PCI information.

Also in case of rescheduling during live migrate if the rescheduling
failed the PCI change needed to be rolled back to the source host by a
specific code. But now those change are never persisted until the
migration finishes so this rollback code can be removed too.

Change-Id: Ied8f96b4e67f79498519931cb6b35dad5288bbb8
blueprint: support-move-ops-with-qos-ports-ussuri

Author: Balazs Gibizer

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

@@ -9545,6 +9545,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')
@@ -9556,11 +9557,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,
@@ -9574,6 +9576,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()
 
@@ -9590,13 +9594,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
@@ -9605,13 +9610,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()