[FUP] Follow-up patch for SR-IOV live migration

This commit addresses some of the non-blocking comments/nits
in[1].

Main changes:
- Add 'source' property to InstancePCIRequest object
  which contains the source of the request i.e. flavor alias
  or neutron port. this avoids duplication of logic
  and comments in the code.

- Move class method create_skeleton_migrate_vifs from
  LiveMigrateData object to VIFMigrateData object as
  frankly its more sensible to have it there.

- Separate unit test, test__claim_pci_for_instance_vifs,
  to two separate unit tests for clarity.

- Some trivial nits and typos.

[1] https://review.opendev.org/#/c/620115/

Change-Id: I292a0e2d840bbf657ba6d0932f9a3decbcb2778f

Author: adrianchiris

nova/compute/manager.py

@@ -6298,7 +6298,7 @@ def check_can_live_migrate_destination(self, ctxt, instance,
                                                               dest_check_data)
             # Create migrate_data vifs
             migrate_data.vifs = \
-                migrate_data_obj.LiveMigrateData.create_skeleton_migrate_vifs(
+                migrate_data_obj.VIFMigrateData.create_skeleton_migrate_vifs(
                     instance.get_network_info())
             # Claim PCI devices for VIFs on destination (if needed)
             port_id_to_pci = self._claim_pci_for_instance_vifs(ctxt, instance)

nova/compute/resource_tracker.py

@@ -309,13 +309,11 @@ def _move_claim(self, context, instance, new_instance_type, nodename,
         new_pci_requests = pci_request.get_pci_requests_from_flavor(
             new_instance_type)
         new_pci_requests.instance_uuid = instance.uuid
-        # PCI requests come from two sources: instance flavor and
-        # SR-IOV ports. SR-IOV ports pci_request don't have an alias_name.
-        # On resize merge the SR-IOV ports pci_requests with the new
-        # instance flavor pci_requests.
+        # On resize merge the SR-IOV ports pci_requests
+        # with the new instance flavor pci_requests.
         if instance.pci_requests:
             for request in instance.pci_requests.requests:
-                if request.alias_name is None:
+                if request.source == objects.InstancePCIRequest.NEUTRON_PORT:
                     new_pci_requests.requests.append(request)
         claim = claims.MoveClaim(context, instance, nodename,
                                  new_instance_type, image_meta, self, cn,

nova/conductor/tasks/live_migrate.py

@@ -214,14 +214,8 @@ def _check_can_migrate_pci(self, src_host, dest_host):
             return
 
         for pci_request in self.instance.pci_requests.requests:
-            if pci_request.alias_name is not None:
+            if pci_request.source != objects.InstancePCIRequest.NEUTRON_PORT:
                 # allow only VIF related PCI requests in live migration.
-                # PCI requests come from two sources: instance flavor and
-                # SR-IOV ports.
-                # SR-IOV ports pci_request don't have an alias_name.
-                # TODO(adrianc): add an is_sriov_port property to PCIRequest
-                # to make this cryptic check clearer (also in resource_tracker)
-
                 raise exception.MigrationPreCheckError(
                     reason= "non-VIF related PCI requests for instance "
                             "are not allowed for live migration.")
@@ -232,6 +226,7 @@ def _check_can_migrate_pci(self, src_host, dest_host):
             raise exception.MigrationPreCheckError(
                 reason="Cannot live migrate VIF with related PCI, Neutron "
                        "does not support required port binding extension.")
+
         if not (supports_vif_related_pci_allocations(self.context,
                                                      src_host) and
                 supports_vif_related_pci_allocations(self.context,
@@ -340,8 +335,8 @@ def _call_livem_checks_on_host(self, destination):
                 # migrate data vifs were not constructed in dest compute
                 # during check_can_live_migrate_destination, construct a
                 # skeleton to be updated after port binding.
-                # TODO(adrianc): This can be removed once we move to T release
-                self.migrate_data.vifs = migrate_data_obj.LiveMigrateData.\
+                # TODO(adrianc): This can be removed once we move to U release
+                self.migrate_data.vifs = migrate_data_obj.VIFMigrateData.\
                     create_skeleton_migrate_vifs(
                     self.instance.get_network_info())
             bindings = self._bind_ports_on_destination(destination)
@@ -356,7 +351,7 @@ def _bind_ports_on_destination(self, destination):
         # that was bound. This information is then stuffed into the
         # migrate_data.
         try:
-            # Note(adrianc): migrate_data.vifs was partially filled
+            # NOTE(adrianc): migrate_data.vifs was partially filled
             # by destination compute if compute is new enough.
             # if that is the case, it may have updated the required port
             # profile for the destination node (e.g new PCI address if SR-IOV)
@@ -372,7 +367,8 @@ def _bind_ports_on_destination(self, destination):
                                  for mig_vif in migrate_vifs_with_profile}
 
             bindings = self.network_api.bind_ports_to_host(
-                self.context, self.instance, destination, None, ports_profile)
+                context=self.context, instance=self.instance, host=destination,
+                vnic_types=None, port_profiles=ports_profile)
         except exception.PortBindingFailed as e:
             # Port binding failed for that host, try another one.
             raise exception.MigrationPreCheckError(

nova/objects/instance_pci_requests.py

@@ -28,6 +28,12 @@ class InstancePCIRequest(base.NovaObject,
     # Version 1.3: Add requester_id
     VERSION = '1.3'
 
+    # Possible sources for a PCI request:
+    # FLAVOR_ALIAS : Request originated from a flavor alias.
+    # NEUTRON_PORT : Request originated from a neutron port.
+    FLAVOR_ALIAS = 0
+    NEUTRON_PORT = 1
+
     fields = {
         'count': fields.IntegerField(),
         'spec': fields.ListOfDictOfNullableStringsField(),
@@ -40,6 +46,14 @@ class InstancePCIRequest(base.NovaObject,
         'numa_policy': fields.PCINUMAAffinityPolicyField(nullable=True),
     }
 
+    @property
+    def source(self):
+        # PCI requests originate from two sources: instance flavor alias and
+        # neutron SR-IOV ports.
+        # SR-IOV ports pci_request don't have an alias_name.
+        return (InstancePCIRequest.NEUTRON_PORT if self.alias_name is None
+                else InstancePCIRequest.FLAVOR_ALIAS)
+
     def obj_load_attr(self, attr):
         setattr(self, attr, None)
 

nova/objects/migrate_data.py

@@ -94,6 +94,21 @@ def get_dest_vif(self):
         vif['details'] = self.vif_details
         return vif
 
+    @classmethod
+    def create_skeleton_migrate_vifs(cls, vifs):
+        """Create migrate vifs for live migration.
+
+        :param vifs: a list of VIFs.
+        :return: list of VIFMigrateData object corresponding to the provided
+                 VIFs.
+        """
+        vif_mig_data = []
+
+        for vif in vifs:
+            mig_vif = cls(port_id=vif['id'], source_vif=vif)
+            vif_mig_data.append(mig_vif)
+        return vif_mig_data
+
 
 @obj_base.NovaObjectRegistry.register_if(False)
 class LiveMigrateData(obj_base.NovaObject):
@@ -120,23 +135,6 @@ class LiveMigrateData(obj_base.NovaObject):
         'vifs': fields.ListOfObjectsField('VIFMigrateData'),
     }
 
-    @staticmethod
-    def create_skeleton_migrate_vifs(vifs):
-        """Create migrate vifs for live migration.
-
-        :param vifs: a list of VIFs.
-        :return: list of VIFMigrateData object corresponding to the provided
-                 VIFs.
-        """
-        vif_mig_data = []
-
-        for vif in vifs:
-            mig_vif = VIFMigrateData(
-                port_id=vif['id'],
-                source_vif=vif)
-            vif_mig_data.append(mig_vif)
-        return vif_mig_data
-
 
 @obj_base.NovaObjectRegistry.register
 class LibvirtLiveMigrateBDMInfo(obj_base.NovaObject):

nova/tests/unit/compute/test_compute_mgr.py

@@ -2716,7 +2716,7 @@ def _test_check_can_live_migrate_destination(self, do_raise=False):
                               'cleanup_live_migration_destination_check'),
             mock.patch.object(db, 'instance_fault_create'),
             mock.patch.object(compute_utils, 'EventReporter'),
-            mock.patch.object(migrate_data_obj.LiveMigrateData,
+            mock.patch.object(migrate_data_obj.VIFMigrateData,
                               'create_skeleton_migrate_vifs'),
             mock.patch.object(instance, 'get_network_info'),
             mock.patch.object(self.compute, '_claim_pci_for_instance_vifs'),
@@ -8911,7 +8911,7 @@ def fake_reschedule_resize_or_reraise(*args, **kwargs):
         # (_error_out_instance_on_exception will set to ACTIVE by default).
         self.assertEqual(vm_states.STOPPED, instance.vm_state)
 
-    def test__claim_pci_for_instance_vifs(self):
+    def test__claim_pci_for_instance_no_vifs(self):
         @mock.patch.object(self.compute, 'rt')
         @mock.patch.object(pci_request, 'get_instance_pci_request_from_vif')
         @mock.patch.object(self.instance, 'get_network_info')
@@ -8925,6 +8925,16 @@ def _test(mock_get_network_info,
                 self.instance)
             rt_mock.claim_pci_devices.assert_not_called()
             self.assertEqual(0, len(port_id_to_pci))
+
+        _test()
+
+    def test__claim_pci_for_instance_vifs(self):
+        @mock.patch.object(self.compute, 'rt')
+        @mock.patch.object(pci_request, 'get_instance_pci_request_from_vif')
+        @mock.patch.object(self.instance, 'get_network_info')
+        def _test(mock_get_network_info,
+                  mock_get_instance_pci_request_from_vif,
+                  rt_mock):
             # when there are VIFs, expect only ones with related PCI to be
             # claimed and their migrate vif profile to be updated.
 
@@ -9001,7 +9011,7 @@ def test__update_migrate_vifs_profile_with_pci(self):
                                     vendor_id='15b3',
                                     product_id='1018')
         port_id_to_pci_dev = {uuids.port0: pci_dev}
-        mig_vifs = migrate_data_obj.LiveMigrateData.\
+        mig_vifs = migrate_data_obj.VIFMigrateData.\
             create_skeleton_migrate_vifs(nw_vifs)
         self.compute._update_migrate_vifs_profile_with_pci(mig_vifs,
                                                            port_id_to_pci_dev)

nova/tests/unit/objects/test_instance_pci_requests.py

@@ -181,6 +181,24 @@ def test_obj_make_compatible_pre_1_3(self):
         self.assertNotIn('requester_id', primitive['nova_object.data'])
         self.assertIn('numa_policy', primitive['nova_object.data'])
 
+    def test_source_property(self):
+        neutron_port_pci_req = objects.InstancePCIRequest(
+            count=1,
+            spec=[{'vendor_id': '15b3', 'device_id': '1018'}],
+            request_id=uuids.pci_request_id1,
+            requester_id=uuids.requester_id1,
+            alias_name = None)
+        flavor_alias_pci_req = objects.InstancePCIRequest(
+            count=1,
+            spec=[{'vendor_id': '15b3', 'device_id': '1810'}],
+            request_id=uuids.pci_request_id2,
+            requester_id=uuids.requester_id2,
+            alias_name = 'alias_1')
+        self.assertEqual(neutron_port_pci_req.source,
+                         objects.InstancePCIRequest.NEUTRON_PORT)
+        self.assertEqual(flavor_alias_pci_req.source,
+                         objects.InstancePCIRequest.FLAVOR_ALIAS)
+
 
 class TestInstancePCIRequests(test_objects._LocalTest,
                               _TestInstancePCIRequests):

nova/tests/unit/objects/test_migrate_data.py

@@ -44,15 +44,7 @@ def test_obj_make_compatible(self):
 
 class TestLiveMigrateData(test_objects._LocalTest,
                           _TestLiveMigrateData):
-    def test_create_skeleton_migrate_vifs(self):
-        vifs = [
-            network_model.VIF(id=uuids.port1),
-            network_model.VIF(id=uuids.port2)]
-        mig_vifs = migrate_data.LiveMigrateData.create_skeleton_migrate_vifs(
-            vifs)
-        self.assertEqual(len(vifs), len(mig_vifs))
-        self.assertEqual([vif['id'] for vif in vifs],
-                         [mig_vif.port_id for mig_vif in mig_vifs])
+    pass
 
 
 class TestRemoteLiveMigrateData(test_objects._RemoteTest,
@@ -311,3 +303,13 @@ def test_get_dest_vif(self):
         self.assertEqual(migrate_vif.vif_details, dest_vif['details'])
         self.assertEqual(migrate_vif.profile, dest_vif['profile'])
         self.assertEqual(uuids.ovs_interfaceid, dest_vif['ovs_interfaceid'])
+
+    def test_create_skeleton_migrate_vifs(self):
+        vifs = [
+            network_model.VIF(id=uuids.port1),
+            network_model.VIF(id=uuids.port2)]
+        mig_vifs = migrate_data.VIFMigrateData.create_skeleton_migrate_vifs(
+            vifs)
+        self.assertEqual(len(vifs), len(mig_vifs))
+        self.assertEqual([vif['id'] for vif in vifs],
+                         [mig_vif.port_id for mig_vif in mig_vifs])