hooks: fix initial sriov configurations

To configure SRIOV devices it was expected that the 'sriov-numvfs'
config option to be changed but during an initial setup this not
happens.

In this commit we remove the condition but add a logic in
PCINetDevice to avoid reconfiguring PF devices if not necessary.

Change-Id: Ib8232b29f76ca7e25e1cd835d5e31a276000f1d4
Closes-Bug: #1817079
Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@canonical.com>

hooks/neutron_ovs_utils.py

@@ -610,69 +610,68 @@ def configure_sriov():
     os.chmod(NEUTRON_SRIOV_INIT_SCRIPT, 0o755)
     service('enable', 'neutron-openvswitch-networking-sriov')
 
-    if charm_config.changed('sriov-numvfs'):
-        devices = PCINetDevices()
-        sriov_numvfs = charm_config.get('sriov-numvfs')
+    devices = PCINetDevices()
+    sriov_numvfs = charm_config.get('sriov-numvfs')
 
-        # automatic configuration of all SR-IOV devices
-        if sriov_numvfs == 'auto':
-            log('Configuring SR-IOV device VF functions in auto mode')
+    # automatic configuration of all SR-IOV devices
+    if sriov_numvfs == 'auto':
+        log('Configuring SR-IOV device VF functions in auto mode')
+        for device in devices.pci_devices:
+            if device and device.sriov:
+                log("Configuring SR-IOV device"
+                    " {} with {} VF's".format(device.interface_name,
+                                              device.sriov_totalvfs))
+                device.set_sriov_numvfs(device.sriov_totalvfs)
+    else:
+        # Single int blanket configuration
+        try:
+            log('Configuring SR-IOV device VF functions'
+                ' with blanket setting')
             for device in devices.pci_devices:
                 if device and device.sriov:
-                    log("Configuring SR-IOV device"
-                        " {} with {} VF's".format(device.interface_name,
-                                                  device.sriov_totalvfs))
-                    device.set_sriov_numvfs(device.sriov_totalvfs)
-        else:
-            # Single int blanket configuration
-            try:
-                log('Configuring SR-IOV device VF functions'
-                    ' with blanket setting')
-                for device in devices.pci_devices:
-                    if device and device.sriov:
-                        numvfs = min(int(sriov_numvfs), device.sriov_totalvfs)
-                        if int(sriov_numvfs) > device.sriov_totalvfs:
-                            log('Requested value for sriov-numvfs ({}) too '
-                                'high for interface {}. Falling back to '
-                                'interface totalvfs '
-                                'value: {}'.format(sriov_numvfs,
-                                                   device.interface_name,
-                                                   device.sriov_totalvfs))
-                        log("Configuring SR-IOV device {} with {} "
-                            "VFs".format(device.interface_name, numvfs))
-                        device.set_sriov_numvfs(numvfs)
-            except ValueError:
-                # <device>:<numvfs>[ <device>:numvfs] configuration
-                sriov_numvfs = sriov_numvfs.split()
-                for device_config in sriov_numvfs:
-                    log('Configuring SR-IOV device VF functions per interface')
-                    interface_name, numvfs = device_config.split(':')
-                    device = devices.get_device_from_interface_name(
-                        interface_name)
-                    if device and device.sriov:
-                        if int(numvfs) > device.sriov_totalvfs:
-                            log('Requested value for sriov-numfs ({}) too '
-                                'high for interface {}. Falling back to '
-                                'interface totalvfs '
-                                'value: {}'.format(numvfs,
-                                                   device.interface_name,
-                                                   device.sriov_totalvfs))
-                            numvfs = device.sriov_totalvfs
-                        log("Configuring SR-IOV device {} with {} "
-                            "VF's".format(device.interface_name, numvfs))
-                        device.set_sriov_numvfs(int(numvfs))
+                    numvfs = min(int(sriov_numvfs), device.sriov_totalvfs)
+                    if int(sriov_numvfs) > device.sriov_totalvfs:
+                        log('Requested value for sriov-numvfs ({}) too '
+                            'high for interface {}. Falling back to '
+                            'interface totalvfs '
+                            'value: {}'.format(sriov_numvfs,
+                                               device.interface_name,
+                                               device.sriov_totalvfs))
+                    log("Configuring SR-IOV device {} with {} "
+                        "VFs".format(device.interface_name, numvfs))
+                    device.set_sriov_numvfs(numvfs)
+        except ValueError:
+            # <device>:<numvfs>[ <device>:numvfs] configuration
+            sriov_numvfs = sriov_numvfs.split()
+            for device_config in sriov_numvfs:
+                log('Configuring SR-IOV device VF functions per interface')
+                interface_name, numvfs = device_config.split(':')
+                device = devices.get_device_from_interface_name(
+                    interface_name)
+                if device and device.sriov:
+                    if int(numvfs) > device.sriov_totalvfs:
+                        log('Requested value for sriov-numfs ({}) too '
+                            'high for interface {}. Falling back to '
+                            'interface totalvfs '
+                            'value: {}'.format(numvfs,
+                                               device.interface_name,
+                                               device.sriov_totalvfs))
+                        numvfs = device.sriov_totalvfs
+                    log("Configuring SR-IOV device {} with {} "
+                        "VF's".format(device.interface_name, numvfs))
+                    device.set_sriov_numvfs(int(numvfs))
 
-        # Trigger remote restart in parent application
-        remote_restart('neutron-plugin', 'nova-compute')
+    # Trigger remote restart in parent application
+    remote_restart('neutron-plugin', 'nova-compute')
 
-        # Restart of SRIOV agent is required after changes to system runtime
-        # VF configuration
-        cmp_release = CompareOpenStackReleases(
-            os_release('neutron-common', base='icehouse'))
-        if cmp_release >= 'mitaka':
-            service_restart('neutron-sriov-agent')
-        else:
-            service_restart('neutron-plugin-sriov-agent')
+    # Restart of SRIOV agent is required after changes to system runtime
+    # VF configuration
+    cmp_release = CompareOpenStackReleases(
+        os_release('neutron-common', base='icehouse'))
+    if cmp_release >= 'mitaka':
+        service_restart('neutron-sriov-agent')
+    else:
+        service_restart('neutron-plugin-sriov-agent')
 
 
 def get_shared_secret():

hooks/pci.py

@@ -190,7 +190,7 @@ class PCINetDevice(object):
 
         @param numvfs: integer to set the current number of VF's to
         """
-        if self.sriov:
+        if self.sriov and numvfs != self.sriov_numvfs:
             # NOTE(fnordahl): run-time change of numvfs is disallowed
             # without resetting to 0 first.
             self._set_sriov_numvfs(0)

unit_tests/test_neutron_ovs_utils.py

@@ -795,13 +795,11 @@ class TestNeutronOVSUtils(CharmTestCase):
             # ports=None whilst port checks are disabled.
             f.assert_called_once_with('assessor', services='s1', ports=None)
 
-    def _configure_sriov_base(self, config,
-                              changed=False):
+    def _configure_sriov_base(self, config):
         self.mock_config = MagicMock()
         self.config.side_effect = None
         self.config.return_value = self.mock_config
         self.mock_config.get.side_effect = lambda x: config.get(x)
-        self.mock_config.changed.return_value = changed
 
         self.mock_eth_device = MagicMock()
         self.mock_eth_device.sriov = False
@@ -835,19 +833,6 @@ class TestNeutronOVSUtils(CharmTestCase):
         mock_pci_devices.get_device_from_interface_name.side_effect = \
             lambda x: self.pci_devices.get(x)
 
-    @patch('os.chmod')
-    def test_configure_sriov_no_changes(self, _os_chmod):
-        self.os_release.return_value = 'kilo'
-        _config = {
-            'enable-sriov': True,
-            'sriov-numvfs': 'auto'
-        }
-        self._configure_sriov_base(_config, False)
-
-        nutils.configure_sriov()
-
-        self.assertFalse(self.remote_restart.called)
-
     @patch('os.chmod')
     def test_configure_sriov_auto(self, _os_chmod):
         self.os_release.return_value = 'mitaka'
@@ -855,7 +840,7 @@ class TestNeutronOVSUtils(CharmTestCase):
             'enable-sriov': True,
             'sriov-numvfs': 'auto'
         }
-        self._configure_sriov_base(_config, True)
+        self._configure_sriov_base(_config)
 
         nutils.configure_sriov()
 
@@ -874,7 +859,7 @@ class TestNeutronOVSUtils(CharmTestCase):
             'enable-sriov': True,
             'sriov-numvfs': '32',
         }
-        self._configure_sriov_base(_config, True)
+        self._configure_sriov_base(_config)
 
         nutils.configure_sriov()
 
@@ -890,7 +875,7 @@ class TestNeutronOVSUtils(CharmTestCase):
             'enable-sriov': True,
             'sriov-numvfs': 'ens0:32 sriov23:64'
         }
-        self._configure_sriov_base(_config, True)
+        self._configure_sriov_base(_config)
 
         nutils.configure_sriov()
 
@@ -899,6 +884,24 @@ class TestNeutronOVSUtils(CharmTestCase):
 
         self.assertTrue(self.remote_restart.called)
 
+    @patch('os.chmod')
+    def test_configure_sriov_auto_avoid_recall(self, _os_chmod):
+        self.os_release.return_value = 'mitaka'
+        _config = {
+            'enable-sriov': True,
+            'sriov-numvfs': 'auto'
+        }
+        self._configure_sriov_base(_config)
+
+        nutils.configure_sriov()
+
+        self.mock_sriov_device2.sriov_numvfs = 64
+        self.mock_sriov_device2.set_sriov_numvfs.assert_called_with(
+            self.mock_sriov_device2.sriov_totalvfs)
+        self.mock_sriov_device2._set_sriov_numvfs.assert_not_called()
+
+        self.assertTrue(self.remote_restart.called)
+
     @patch.object(nutils, 'subprocess')
     @patch.object(nutils, 'shutil')
     def test_install_tmpfilesd_lxd(self, mock_shutil, mock_subprocess):

unit_tests/test_pci.py

@@ -220,6 +220,14 @@ class PCINetDeviceTest(CharmTestCase):
         mock__set_sriov_numvfs.assert_has_calls([
             call(0), call(4)])
 
+    @patch('pci.PCINetDevice._set_sriov_numvfs')
+    def test_set_sriov_numvfs_avoid_call(self, mock__set_sriov_numvfs):
+        dev = pci.PCINetDevice('0000:10:00.0')
+        dev.sriov = True
+        dev.sriov_numvfs = 4
+        dev.set_sriov_numvfs(4)
+        self.assertFalse(mock__set_sriov_numvfs.called)
+
 
 class PCINetDevicesTest(CharmTestCase):