libvirt: Delegate OVS plug to os-vif

os-vif 1.15.0 added the ability to create an OVS port during plugging
by specifying the 'create_port' attribute in the 'port_profile' field.
By delegating port creation to os-vif, we can rely on it's 'isolate_vif'
config option [1] that will temporarily configure the VLAN to 4095
(0xfff), which is reserved for implementation use [2] and is used by
neutron to as a dead VLAN [3]. By doing this, we ensure VIFs are plugged
securely, preventing guests from accessing other tenants' networks
before the neutron OVS agent can wire up the port.

This change requires a little dance as part of the live migration flow.
Since we can't be certain the destination host has a version of os-vif
that supports this feature, we need to use a sentinel to indicate when
it does. Typically we would do so with a field in
'LibvirtLiveMigrateData', such as the 'src_supports_numa_live_migration'
and 'dst_supports_numa_live_migration' fields used to indicate support
for NUMA-aware live migration. However, doing this prevents us
backporting this important fix since o.vo changes are not backportable.
Instead, we (somewhat evilly) rely on the free-form nature of the
'VIFMigrateData.profile_json' string field, which stores JSON blobs and
is included in 'LibvirtLiveMigrateData' via the 'vifs' attribute, to
transport this sentinel. This is a hack but is necessary to work around
the lack of a free-form "capabilities" style dict that would allow us do
backportable fixes to live migration features.

Note that this change has the knock on effect of modifying the XML
generated for OVS ports: when hybrid plug is false will now be of type
'ethernet' rather than 'bridge' as before. This explains the larger than
expected test damage but should not affect users.

Changes:
  lower-constraints.txt
  requirements.txt
  nova/network/os_vif_util.py
  nova/tests/unit/virt/libvirt/test_vif.py
  nova/tests/unit/virt/libvirt/test_driver.py
  nova/virt/libvirt/driver.py

NOTE(stephenfin): Change I362deb1088c88cdcd8219922da9dc9a01b10a940
("objects: Fix VIFMigrateData.supports_os_vif_delegation setter") which
contains an important fix for the original change, is squashed into this
change. In addition, the os-vif version bump we introduced in the
original version of this patch is not backportable and as a result, we
must introduce two additional checks. Both checks ensure we have a
suitable version of os-vif and skip the new code paths if not. The first
check is in the libvirt driver's 'check_can_live_migrate_destination'
function, which as the name suggests runs on the destination host early
in the live migration process. If os-vif is not new enough on the
destination, we will report that we cannot support os-vif delegation.
The other check is in the '_nova_to_osvif_vif_ovs' helper in
'nova.network.os_vif_util'. This simply ensures we don't try to set an
invalid attribute on the os-vif object if the version isn't new enough.
Two tests are modified to accommodate these checks with similar version
check logic. The lower-constraints job can be relied on to validate
behavior on the older version of os-vif.

[1] https://opendev.org/openstack/os-vif/src/tag/2.4.0/vif_plug_ovs/ovs.py#L90-L93
[2] https://en.wikipedia.org/wiki/IEEE_802.1Q#Frame_format
[3] https://answers.launchpad.net/neutron/+question/231806

NOTE(auniyal):
  * libvirt: Always delegate OVS plug to os-vif is squashed
openstack@fa0fb2f
  * Updated fakelibvirt.Domain.XMLDesc to add pci address
for ovs interface, which is a patial backport of
openstack@1ad287b

Change-Id: I11fb5d3ada7f27b39c183157ea73c8b72b4e672e
Depends-On: Id12486b3127ab4ac8ad9ef2b3641da1b79a25a50
Closes-Bug: #1734320
Closes-Bug: #1815989
(cherry picked from commit a62dd42)

Author: stephenfin

nova/compute/manager.py

@@ -7997,11 +7997,12 @@ def check_can_live_migrate_destination(self, ctxt, instance,
                          'but source is either too old, or is set to an '
                          'older upgrade level.', instance=instance)
             if self.network_api.supports_port_binding_extension(ctxt):
-                # Create migrate_data vifs
-                migrate_data.vifs = \
-                    migrate_data_obj.\
-                    VIFMigrateData.create_skeleton_migrate_vifs(
-                        instance.get_network_info())
+                # Create migrate_data vifs if not provided by driver.
+                if 'vifs' not in migrate_data:
+                    migrate_data.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/network/model.py

@@ -384,7 +384,8 @@ def __init__(self, id=None, address=None, network=None, type=None,
                  details=None, devname=None, ovs_interfaceid=None,
                  qbh_params=None, qbg_params=None, active=False,
                  vnic_type=VNIC_TYPE_NORMAL, profile=None,
-                 preserve_on_delete=False, **kwargs):
+                 preserve_on_delete=False, delegate_create=False,
+                 **kwargs):
         super(VIF, self).__init__()
 
         self['id'] = id
@@ -401,14 +402,15 @@ def __init__(self, id=None, address=None, network=None, type=None,
         self['vnic_type'] = vnic_type
         self['profile'] = profile
         self['preserve_on_delete'] = preserve_on_delete
+        self['delegate_create'] = delegate_create
 
         self._set_meta(kwargs)
 
     def __eq__(self, other):
         keys = ['id', 'address', 'network', 'vnic_type',
                 'type', 'profile', 'details', 'devname',
                 'ovs_interfaceid', 'qbh_params', 'qbg_params',
-                'active', 'preserve_on_delete']
+                'active', 'preserve_on_delete', 'delegate_create']
         return all(self[k] == other[k] for k in keys)
 
     def __ne__(self, other):

nova/network/neutron.py

@@ -3066,7 +3066,8 @@ def _build_vif_model(self, context, client, current_neutron_port,
             ovs_interfaceid=ovs_interfaceid,
             devname=devname,
             active=vif_active,
-            preserve_on_delete=preserve_on_delete
+            preserve_on_delete=preserve_on_delete,
+            delegate_create=True,
         )
 
     def _build_network_info_model(self, context, instance, networks=None,

nova/network/os_vif_util.py

@@ -21,6 +21,7 @@
 from os_vif import objects
 from oslo_config import cfg
 from oslo_log import log as logging
+import pbr.version
 
 from nova import exception
 from nova.i18n import _
@@ -347,6 +348,27 @@ def _nova_to_osvif_vif_ovs(vif):
             vif_name=vif_name,
             bridge_name=_get_hybrid_bridge_name(vif))
     else:
+        # NOTE(stephenfin): The 'create_port' attribute was added in os-vif
+        # 1.15.0 and isn't available with the lowest version supported here
+        if (
+            pbr.version.VersionInfo('os-vif').semantic_version() >=
+            pbr.version.SemanticVersion.from_pip_string('1.15.0')
+        ):
+            profile.create_port = vif.get('delegate_create', False)
+        elif vif.get('delegate_create', False):
+            # NOTE(stephenfin): This should never happen, since if the
+            # 'delegate_create' attribute is true then it was set by the
+            # destination compute node as part of the live migration operation:
+            # the same destination compute node that we are currently running
+            # on. We include it for sanity-preservation
+            LOG.warning(
+                'os-vif 1.15.0 or later is required to support '
+                'delegated port creation but you have %s; consider '
+                'updating this package to resolve bugs #1734320 and '
+                '#1815989',
+                pbr.version.VersionInfo('os-vif').version_string(),
+            )
+
         obj = _get_vif_instance(
             vif,
             objects.vif.VIFOpenVSwitch,

nova/objects/migrate_data.py

@@ -22,8 +22,8 @@
 from nova.objects import base as obj_base
 from nova.objects import fields
 
-
 LOG = log.getLogger(__name__)
+OS_VIF_DELEGATION = 'os_vif_delegation'
 
 
 @obj_base.NovaObjectRegistry.register
@@ -60,6 +60,8 @@ class VIFMigrateData(obj_base.NovaObject):
 
     @property
     def vif_details(self):
+        if 'vif_details_json' not in self:
+            return {}
         return jsonutils.loads(self.vif_details_json)
 
     @vif_details.setter
@@ -68,12 +70,27 @@ def vif_details(self, vif_details_dict):
 
     @property
     def profile(self):
+        if 'profile_json' not in self:
+            return {}
         return jsonutils.loads(self.profile_json)
 
     @profile.setter
     def profile(self, profile_dict):
         self.profile_json = jsonutils.dumps(profile_dict)
 
+    @property
+    def supports_os_vif_delegation(self):
+        return self.profile.get(OS_VIF_DELEGATION, False)
+
+    # TODO(stephenfin): add a proper delegation field instead of storing this
+    # info in the profile catch-all blob
+    @supports_os_vif_delegation.setter
+    def supports_os_vif_delegation(self, supported):
+        # we can't simply set the attribute using dict notation since the
+        # getter returns a copy of the data, not the data itself
+        self.profile = dict(
+            self.profile or {}, **{OS_VIF_DELEGATION: supported})
+
     def get_dest_vif(self):
         """Get a destination VIF representation of this object.
 
@@ -91,6 +108,7 @@ def get_dest_vif(self):
         vif['vnic_type'] = self.vnic_type
         vif['profile'] = self.profile
         vif['details'] = self.vif_details
+        vif['delegate_create'] = self.supports_os_vif_delegation
         return vif
 
     @classmethod

nova/tests/functional/libvirt/test_pci_sriov_servers.py

@@ -577,16 +577,15 @@ def test_get_server_diagnostics_server_with_VF(self):
         self.start_compute(pci_info=pci_info)
 
         # create the SR-IOV port
-        self.neutron.create_port({'port': self.neutron.network_4_port_1})
+        port = self.neutron.create_port(
+            {'port': self.neutron.network_4_port_1})
 
-        # create a server using the VF and multiple networks
-        extra_spec = {'pci_passthrough:alias': f'{self.VFS_ALIAS_NAME}:1'}
-        flavor_id = self._create_flavor(extra_spec=extra_spec)
+        flavor_id = self._create_flavor()
         server = self._create_server(
             flavor_id=flavor_id,
             networks=[
                 {'uuid': base.LibvirtNeutronFixture.network_1['id']},
-                {'port': base.LibvirtNeutronFixture.network_4_port_1['id']},
+                {'port': port['port']['id']},
             ],
         )
 
@@ -600,13 +599,16 @@ def test_get_server_diagnostics_server_with_VF(self):
             base.LibvirtNeutronFixture.network_1_port_2['mac_address'],
             diagnostics['nic_details'][0]['mac_address'],
         )
-        self.assertIsNotNone(diagnostics['nic_details'][0]['tx_packets'])
+
+        for key in ('rx_packets', 'tx_packets'):
+            self.assertIn(key, diagnostics['nic_details'][0])
 
         self.assertEqual(
             base.LibvirtNeutronFixture.network_4_port_1['mac_address'],
             diagnostics['nic_details'][1]['mac_address'],
         )
-        self.assertIsNone(diagnostics['nic_details'][1]['tx_packets'])
+        for key in ('rx_packets', 'tx_packets'):
+            self.assertIn(key, diagnostics['nic_details'][1])
 
     def test_create_server_after_change_in_nonsriov_pf_to_sriov_pf(self):
         # Starts a compute with PF not configured with SRIOV capabilities

nova/tests/unit/fake_network.py

@@ -120,7 +120,7 @@ def update_cache_fake(*args, **kwargs):
             qbg_params=None,
             active=False,
             vnic_type='normal',
-            profile=None,
+            profile={},
             preserve_on_delete=False,
             meta={'rxtx_cap': 30},
         )

nova/tests/unit/network/test_neutron.py

@@ -3271,6 +3271,7 @@ def test_build_network_info_model(self, mock_get_client,
                 requested_ports[index].get(
                     constants.BINDING_PROFILE) or {},
                 nw_info.get('profile'))
+            self.assertTrue(nw_info.get('delegate_create'))
             index += 1
 
         self.assertFalse(nw_infos[0]['active'])

nova/tests/unit/network/test_os_vif_util.py

@@ -458,7 +458,8 @@ def test_nova_to_osvif_vif_agilio_ovs_fallthrough(self):
                 subnets=[]),
             details={
                 model.VIF_DETAILS_PORT_FILTER: True,
-            }
+            },
+            delegate_create=False,
         )
 
         actual = os_vif_util.nova_to_osvif_vif(vif)
@@ -471,7 +472,8 @@ def test_nova_to_osvif_vif_agilio_ovs_fallthrough(self):
             plugin="ovs",
             port_profile=osv_objects.vif.VIFPortProfileOpenVSwitch(
                 interface_id="dc065497-3c8d-4f44-8fb4-e1d33c16a536",
-                datapath_type=None),
+                datapath_type=None,
+                create_port=False),
             preserve_on_delete=False,
             vif_name="nicdc065497-3c",
             network=osv_objects.network.Network(
@@ -588,6 +590,7 @@ def test_nova_to_osvif_vif_ovs_plain(self):
                 model.VIF_DETAILS_OVS_DATAPATH_TYPE:
                     model.VIF_DETAILS_OVS_DATAPATH_SYSTEM
             },
+            delegate_create=True,
         )
 
         actual = os_vif_util.nova_to_osvif_vif(vif)
@@ -600,7 +603,8 @@ def test_nova_to_osvif_vif_ovs_plain(self):
             plugin="ovs",
             port_profile=osv_objects.vif.VIFPortProfileOpenVSwitch(
                 interface_id="dc065497-3c8d-4f44-8fb4-e1d33c16a536",
-                datapath_type=model.VIF_DETAILS_OVS_DATAPATH_SYSTEM),
+                datapath_type=model.VIF_DETAILS_OVS_DATAPATH_SYSTEM,
+                create_port=True),
             preserve_on_delete=False,
             vif_name="nicdc065497-3c",
             network=osv_objects.network.Network(

nova/tests/unit/objects/test_migrate_data.py

@@ -316,3 +316,24 @@ def test_create_skeleton_migrate_vifs(self):
         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])
+
+    def test_supports_os_vif_delegation(self):
+        # first try setting on a object without 'profile' defined
+        migrate_vif = objects.VIFMigrateData(
+            port_id=uuids.port_id, vnic_type=network_model.VNIC_TYPE_NORMAL,
+            vif_type=network_model.VIF_TYPE_OVS, vif_details={},
+            host='fake-dest-host')
+        migrate_vif.supports_os_vif_delegation = True
+        self.assertTrue(migrate_vif.supports_os_vif_delegation)
+        self.assertEqual(migrate_vif.profile, {'os_vif_delegation': True})
+
+        # now do the same but with profile defined
+        migrate_vif = objects.VIFMigrateData(
+            port_id=uuids.port_id, vnic_type=network_model.VNIC_TYPE_NORMAL,
+            vif_type=network_model.VIF_TYPE_OVS, vif_details={},
+            host='fake-dest-host', profile={'interface_name': 'eth0'})
+        migrate_vif.supports_os_vif_delegation = True
+        self.assertTrue(migrate_vif.supports_os_vif_delegation)
+        self.assertEqual(
+            migrate_vif.profile,
+            {'os_vif_delegation': True, 'interface_name': 'eth0'})