SR-IOV support for explicitly defining the VF count (#130)

This is a possible feature request to the just-landed SR-IOV support in netplan. This PR introduces an optional 'virtual-function-count:' parameter that can be defined for physical functions to force the allocation of a given number of VFs, regardless of how many are actually used in the netplan config. There are of course safety checks to ensure that we can't request less VFs than actually needed in netplan.

This feature request came from the OpenStack team. We did consider such a thing originally, but then decided that it's better if we let netplan handle it. This PR adds this as an option (not recommended for everyday usage tho).

Co-authored-by: Łukasz 'sil2100' Zemczak <[email protected]>

Author: sil2100

doc/netplan.md

@@ -576,6 +576,15 @@ Example:
       enp1s16f1:
         link: enp1
 
+``virtual-function-count`` (scalar)
+
+:    (SR-IOV devices only) In certain special cases VFs might need to be
+     configured outside of netplan. For such configurations ``virtual-function-count``
+     can be optionally used to set an explicit number of Virtual Functions for
+     the given Physical Function. If unset, the default is to create only as many
+     VFs as are defined in the netplan configuration. This should be used for special
+     cases only.
+
 ## Properties for device type ``modems:``
 GSM/CDMA modem configuration is only supported for the ``NetworkManager`` backend. ``systemd-networkd`` does
 not support modems.

netplan/cli/sriov.py

@@ -27,44 +27,60 @@
 import netifaces
 
 
+def _get_target_interface(interfaces, config_manager, pf_link, pfs):
+    if pf_link not in pfs:
+        # handle the match: syntax, get the actual device name
+        pf_match = config_manager.ethernets[pf_link].get('match')
+        if pf_match:
+            by_name = pf_match.get('name')
+            by_mac = pf_match.get('macaddress')
+            by_driver = pf_match.get('driver')
+
+            for interface in interfaces:
+                if ((by_name and not utils.is_interface_matching_name(interface, by_name)) or
+                        (by_mac and not utils.is_interface_matching_macaddress(interface, by_mac)) or
+                        (by_driver and not utils.is_interface_matching_driver_name(interface, by_driver))):
+                    continue
+                # we have a matching PF
+                # store the matching interface in the dictionary of
+                # active PFs, but error out if we matched more than one
+                if pf_link in pfs:
+                    raise ConfigurationError('matched more than one interface for a PF device: %s' % pf_link)
+                pfs[pf_link] = interface
+        else:
+            # no match field, assume entry name is interface name
+            if pf_link in interfaces:
+                pfs[pf_link] = pf_link
+
+    return pfs.get(pf_link, None)
+
+
 def get_vf_count_and_functions(interfaces, config_manager,
                                vf_counts, vfs, pfs):
     """
     Go through the list of netplan ethernet devices and identify which are
     PFs and VFs, matching the former with actual networking interfaces.
     Count how many VFs each PF will need.
     """
+    explicit_counts = {}
     for ethernet, settings in config_manager.ethernets.items():
         if not settings:
             continue
         if ethernet == 'renderer':
             continue
 
+        # we now also support explicitly stating how many VFs should be
+        # allocated for a PF
+        explicit_num = settings.get('virtual-function-count')
+        if explicit_num:
+            pf = _get_target_interface(interfaces, config_manager, ethernet, pfs)
+            if pf:
+                explicit_counts[pf] = explicit_num
+            continue
+
         pf_link = settings.get('link')
         if pf_link and pf_link in config_manager.ethernets:
-            if pf_link not in pfs:
-                # handle the match: syntax, get the actual device name
-                pf_match = config_manager.ethernets[pf_link].get('match')
-                if pf_match:
-                    by_name = pf_match.get('name')
-                    by_mac = pf_match.get('macaddress')
-                    by_driver = pf_match.get('driver')
-
-                    for interface in interfaces:
-                        if ((by_name and not utils.is_interface_matching_name(interface, by_name)) or
-                                (by_mac and not utils.is_interface_matching_macaddress(interface, by_mac)) or
-                                (by_driver and not utils.is_interface_matching_driver_name(interface, by_driver))):
-                            continue
-                        # we have a matching PF
-                        # store the matching interface in the dictionary of
-                        # active PFs, but error out if we matched more than one
-                        if pf_link in pfs:
-                            raise ConfigurationError('matched more than one interface for a PF device: %s' % pf_link)
-                        pfs[pf_link] = interface
-                else:
-                    # no match field, assume entry name is interface name
-                    if pf_link in interfaces:
-                        pfs[pf_link] = pf_link
+            _get_target_interface(interfaces, config_manager, pf_link, pfs)
 
             if pf_link in pfs:
                 vf_counts[pfs[pf_link]] += 1
@@ -78,6 +94,15 @@ def get_vf_count_and_functions(interfaces, config_manager,
             # VFs that we encounter so far
             vfs[ethernet] = None
 
+    # sanity check: since we can explicitly state the VF count, make sure
+    # that this number isn't smaller than the actual number of VFs declared
+    # the explicit number also overrides the number of actual VFs
+    for pf, count in explicit_counts.items():
+        if pf in vf_counts and vf_counts[pf] > count:
+            raise ConfigurationError(
+                'more VFs allocated than the explicit size declared: %s > %s' % (vf_counts[pf], count))
+        vf_counts[pf] = count
+
 
 def set_numvfs_for_pf(pf, vf_count):
     """
@@ -91,27 +116,17 @@ def set_numvfs_for_pf(pf, vf_count):
     numvfs_path = os.path.join(devdir, 'sriov_numvfs')
     totalvfs_path = os.path.join(devdir, 'sriov_totalvfs')
     try:
-        with open(numvfs_path) as f:
-            vf_current = int(f.read().strip())
         with open(totalvfs_path) as f:
             vf_max = int(f.read().strip())
     except IOError as e:
-        raise RuntimeError('failed parsing sriov_numvfs/sriov_totalvfs for %s: %s' % (pf, str(e)))
+        raise RuntimeError('failed parsing sriov_totalvfs for %s: %s' % (pf, str(e)))
     except ValueError:
-        raise RuntimeError('invalid sriov_numvfs/sriov_totalvfs value for %s' % pf)
+        raise RuntimeError('invalid sriov_totalvfs value for %s' % pf)
 
     if vf_count > vf_max:
         raise ConfigurationError(
             'cannot allocate more VFs for PF %s than supported: %s > %s (sriov_totalvfs)' % (pf, vf_count, vf_max))
 
-    if vf_count <= vf_current:
-        # XXX: this might be a wrong assumption, but I assume that
-        #  the operation of adding/removing VFs is very invasive,
-        #  so it makes no sense to decrease the number of VFs if
-        #  less are needed - leaving the unused ones unconfigured?
-        logging.debug('the %s PF already defines more VFs than required (%s > %s), skipping' % (pf, vf_current, vf_count))
-        return False
-
     try:
         with open(numvfs_path, 'w') as f:
             f.write(str(vf_count))

src/parse.c

@@ -1726,6 +1726,7 @@ static const mapping_entry_handler ethernet_def_handlers[] = {
     PHYSICAL_LINK_HANDLERS,
     {"auth", YAML_MAPPING_NODE, handle_auth},
     {"link", YAML_SCALAR_NODE, handle_netdef_id_ref, NULL, netdef_offset(sriov_link)},
+    {"virtual-function-count", YAML_SCALAR_NODE, handle_netdef_guint, NULL, netdef_offset(sriov_explicit_vf_count)},
     {NULL}
 };
 

src/parse.h

@@ -336,6 +336,7 @@ struct net_definition {
     /* these properties are only valid for SR-IOV NICs */
     struct net_definition* sriov_link;
     gboolean sriov_vlan_filter;
+    guint sriov_explicit_vf_count;
 
     union {
         struct NetplanNMSettings {

tests/generator/test_ethernets.py

@@ -82,7 +82,7 @@ def test_eth_mtu(self):
 '''})
         self.assert_networkd_udev(None)
 
-    def test_eth_sriov_link(self):
+    def test_eth_sriov_vlan_filterv_link(self):
         self.generate('''network:
   version: 2
   ethernets:
@@ -101,6 +101,21 @@ def test_eth_sriov_link(self):
                               'enp1s16f1.network': '''[Match]
 Name=enp1s16f1
 
+[Network]
+LinkLocalAddressing=ipv6
+'''})
+        self.assert_networkd_udev(None)
+
+    def test_eth_sriov_virtual_functions(self):
+        self.generate('''network:
+  version: 2
+  ethernets:
+    enp1:
+      virtual-function-count: 8''')
+
+        self.assert_networkd({'enp1.network': '''[Match]
+Name=enp1
+
 [Network]
 LinkLocalAddressing=ipv6
 '''})
@@ -387,6 +402,31 @@ def test_eth_sriov_link(self):
 [ipv4]
 method=link-local
 
+[ipv6]
+method=ignore
+'''})
+
+    def test_eth_sriov_virtual_functions(self):
+        self.generate('''network:
+  version: 2
+  renderer: NetworkManager
+  ethernets:
+    enp1:
+      dhcp4: n
+      virtual-function-count: 8''')
+
+        self.assert_networkd({})
+        self.assert_nm({'enp1': '''[connection]
+id=netplan-enp1
+type=ethernet
+interface-name=enp1
+
+[ethernet]
+wake-on-lan=0
+
+[ipv4]
+method=link-local
+
 [ipv6]
 method=ignore
 '''})

tests/test_sriov.py

@@ -130,6 +130,8 @@ def test_get_vf_count_and_functions(self, gim, gidn):
         name: enp[4-5]
     enp0:
       mtu: 9000
+    enp8:
+      virtual-function-count: 7
     enp9: {}
     wlp6s0: {}
     enp1s16f1:
@@ -151,7 +153,7 @@ def test_get_vf_count_and_functions(self, gim, gidn):
       link: enp9
 ''', file=fd)
         self.configmanager.parse()
-        interfaces = ['enp1', 'enp2', 'enp3', 'enp5', 'enp0']
+        interfaces = ['enp1', 'enp2', 'enp3', 'enp5', 'enp0', 'enp8']
         vf_counts = defaultdict(int)
         vfs = {}
         pfs = {}
@@ -162,7 +164,7 @@ def test_get_vf_count_and_functions(self, gim, gidn):
         # check if the right vf counts have been recorded in vf_counts
         self.assertDictEqual(
             vf_counts,
-            {'enp1': 2, 'enp2': 2, 'enp3': 1, 'enp5': 1})
+            {'enp1': 2, 'enp2': 2, 'enp3': 1, 'enp5': 1, 'enp8': 7})
         # also check if the vfs and pfs dictionaries got properly set
         self.assertDictEqual(
             vfs,
@@ -171,7 +173,7 @@ def test_get_vf_count_and_functions(self, gim, gidn):
         self.assertDictEqual(
             pfs,
             {'enp1': 'enp1', 'enp2': 'enp2', 'enp3': 'enp3',
-             'enpx': 'enp5'})
+             'enpx': 'enp5', 'enp8': 'enp8'})
 
     @patch('netplan.cli.utils.get_interface_driver_name')
     @patch('netplan.cli.utils.get_interface_macaddress')
@@ -207,24 +209,60 @@ def test_get_vf_count_and_functions_many_match(self, gim, gidn):
         self.assertIn('matched more than one interface for a PF device: enpx',
                       str(e.exception))
 
+    @patch('netplan.cli.utils.get_interface_driver_name')
+    @patch('netplan.cli.utils.get_interface_macaddress')
+    def test_get_vf_count_and_functions_not_enough_explicit(self, gim, gidn):
+        # we mock-out get_interface_driver_name and get_interface_macaddress
+        # to return useful values for the test
+        gim.side_effect = lambda x: '00:01:02:03:04:05' if x == 'enp3' else '00:00:00:00:00:00'
+        gidn.side_effect = lambda x: 'foo' if x == 'enp2' else 'bar'
+        with open(os.path.join(self.workdir.name, "etc/netplan/test.yaml"), 'w') as fd:
+            print('''network:
+  version: 2
+  renderer: networkd
+  ethernets:
+    renderer: networkd
+    enp1:
+      virtual-function-count: 2
+      mtu: 9000
+    enp1s16f1:
+      link: enp1
+    enp1s16f2:
+      link: enp1
+    enp1s16f3:
+      link: enp1
+''', file=fd)
+        self.configmanager.parse()
+        interfaces = ['enp1', 'wlp6s0']
+        vf_counts = defaultdict(int)
+        vfs = {}
+        pfs = {}
+
+        # call the function under test
+        with self.assertRaises(ConfigurationError) as e:
+            sriov.get_vf_count_and_functions(interfaces, self.configmanager,
+                                             vf_counts, vfs, pfs)
+
+        self.assertIn('more VFs allocated than the explicit size declared: 3 > 2',
+                      str(e.exception))
+
     def test_set_numvfs_for_pf(self):
         sriov_open = MockSRIOVOpen()
-        sriov_open.read_queue = ['1\n', '8\n']
+        sriov_open.read_queue = ['8\n']
 
         with patch('builtins.open', sriov_open.open):
             ret = sriov.set_numvfs_for_pf('enp1', 2)
 
         self.assertTrue(ret)
         self.assertListEqual(sriov_open.open.call_args_list,
-                             [call('/sys/class/net/enp1/device/sriov_numvfs'),
-                              call('/sys/class/net/enp1/device/sriov_totalvfs'),
+                             [call('/sys/class/net/enp1/device/sriov_totalvfs'),
                               call('/sys/class/net/enp1/device/sriov_numvfs', 'w')])
         handle = sriov_open.open()
         handle.write.assert_called_once_with('2')
 
     def test_set_numvfs_for_pf_failsafe(self):
         sriov_open = MockSRIOVOpen()
-        sriov_open.read_queue = ['1\n', '8\n']
+        sriov_open.read_queue = ['8\n']
         sriov_open.write_queue = [IOError(16, 'Error'), None, None]
 
         with patch('builtins.open', sriov_open.open):
@@ -236,7 +274,7 @@ def test_set_numvfs_for_pf_failsafe(self):
 
     def test_set_numvfs_for_pf_over_max(self):
         sriov_open = MockSRIOVOpen()
-        sriov_open.read_queue = ['1\n', '8\n']
+        sriov_open.read_queue = ['8\n']
 
         with patch('builtins.open', sriov_open.open):
             with self.assertRaises(ConfigurationError) as e:
@@ -247,7 +285,7 @@ def test_set_numvfs_for_pf_over_max(self):
 
     def test_set_numvfs_for_pf_over_theoretical_max(self):
         sriov_open = MockSRIOVOpen()
-        sriov_open.read_queue = ['1\n', '1337\n']
+        sriov_open.read_queue = ['1337\n']
 
         with patch('builtins.open', sriov_open.open):
             with self.assertRaises(ConfigurationError) as e:
@@ -256,24 +294,11 @@ def test_set_numvfs_for_pf_over_theoretical_max(self):
             self.assertIn('cannot allocate more VFs for PF enp1 than the SR-IOV maximum',
                           str(e.exception))
 
-    def test_set_numvfs_for_pf_smaller(self):
-        sriov_open = MockSRIOVOpen()
-        sriov_open.read_queue = ['4\n', '8\n']
-
-        with patch('builtins.open', sriov_open.open):
-            ret = sriov.set_numvfs_for_pf('enp1', 3)
-
-        self.assertFalse(ret)
-        handle = sriov_open.open()
-        self.assertEqual(handle.write.call_count, 0)
-
     def test_set_numvfs_for_pf_read_failed(self):
         sriov_open = MockSRIOVOpen()
         cases = (
             [IOError],
             ['not a number\n'],
-            ['1\n', IOError],
-            ['1\n', 'not a number\n'],
             )
 
         with patch('builtins.open', sriov_open.open):
@@ -284,7 +309,7 @@ def test_set_numvfs_for_pf_read_failed(self):
 
     def test_set_numvfs_for_pf_write_failed(self):
         sriov_open = MockSRIOVOpen()
-        sriov_open.read_queue = ['1\n', '8\n']
+        sriov_open.read_queue = ['8\n']
         sriov_open.write_queue = [IOError(16, 'Error'), IOError(16, 'Error')]
 
         with patch('builtins.open', sriov_open.open):