Change in vdsm[ovirt-3.5]: network: allow custom bondOption

Hello Dan Kenigsberg,

I'd like you to do a code review.  Please visit

https://gerrit.ovirt.org/43149

to review the following change.

Change subject: network: allow custom bondOption
......................................................................

network: allow custom bondOption

Allow 'custom' in bond options. This value will be accepted by
Bond.validateOptions method but it will not be passed to configurator.

Thanks to this patch, users will be allowed to pass custom properties
to setupNetworks hooks.

This path was reverted in past because of malfunction, now it should
work, there is a new test for sure.

Change-Id: I9ace532d959bc3195a8a92b4536bdc0062bc7d1d
Signed-off-by: Petr Horáček 
Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1234867
Reviewed-on: https://gerrit.ovirt.org/40882
Reviewed-by: Dan Kenigsberg 
Continuous-Integration: Dan Kenigsberg 
---
M lib/vdsm/netinfo.py
M tests/functional/networkTests.py
M vdsm/network/models.py
3 files changed, 37 insertions(+), 3 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/49/43149/1

diff --git a/lib/vdsm/netinfo.py b/lib/vdsm/netinfo.py
index 01f25c7..987a40c 100644
--- a/lib/vdsm/netinfo.py
+++ b/lib/vdsm/netinfo.py
@@ -73,8 +73,9 @@
 _BONDING_FAILOVER_MODES = frozenset(('1', '3'))
 _BONDING_LOADBALANCE_MODES = frozenset(('0', '2', '4', '5', '6'))
 _EXCLUDED_BONDING_ENTRIES = frozenset((
-    'slaves',  'active_slave', 'mii_status', 'queue_id', 'ad_aggregator',
-    'ad_num_ports', 'ad_actor_key', 'ad_partner_key', 'ad_partner_mac'
+    'slaves', 'active_slave', 'mii_status', 'queue_id', 'ad_aggregator',
+    'ad_num_ports', 'ad_actor_key', 'ad_partner_key', 'ad_partner_mac',
+    'custom'
 ))
 _IFCFG_ZERO_SUFFIXED = frozenset(
     ('IPADDR0', 'GATEWAY0', 'PREFIX0', 'NETMASK0'))
diff --git a/tests/functional/networkTests.py b/tests/functional/networkTests.py
index 37979ba..3a3d6f9 100644
--- a/tests/functional/networkTests.py
+++ b/tests/functional/networkTests.py
@@ -2337,3 +2337,22 @@
             self.assertNetworkDoesntExist(NETWORK_NAME)
             self.assertBondDoesntExist(BONDING_NAME, [nic])
             self.vdsm_net.save_config()
+
+    @cleanupNet
+    @ValidateRunningAsRoot
+    def test_setupNetworks_bond_with_custom_option(self):
+        with dummyIf(2) as nics:
+            status, msg = self.setupNetworks(
+                {},
+                {BONDING_NAME: {'nics': nics,
+                                'options': 'custom=foo mode=balance-rr'}},
+                NOCHK, test_kernel_config=False)
+            self.assertEqual(status, SUCCESS, msg)
+            self.assertBondExists(BONDING_NAME, nics)
+            opts = self.vdsm_net.config.bonds.get(BONDING_NAME).get('options')
+            self.assertEquals(opts, 'mode=balance-rr')
+
+            status, msg = self.setupNetworks(
+                {}, {BONDING_NAME: {'remove': True}}, NOCHK)
+            self.assertEqual(status, SUCCESS, msg)
+            self.assertBondDoesntExist(BONDING_NAME, nics)
diff --git a/vdsm/network/models.py b/vdsm/network/models.py
index 9608937..aacf462 100644
--- a/vdsm/network/models.py
+++ b/vdsm/network/models.py
@@ -235,9 +235,23 @@
                               _netinfo=_netinfo))
         return slaves

+    @staticmethod
+    def remove_custom_option(options):
+        """ 'custom' property should not be exposed to configurator, it is
+        only for purpose of hooks """
+        d_opts = dict(opt.split('=', 1) for opt in options.split())
+        if d_opts.pop('custom', None) is not None:
+            options = ' '.join(['%s=%s' % (key, value)
+                                for key, value in d_opts.items()])
+        return options
+
     @classmethod
     def objectivize(cls, name, configurator, options, nics, mtu, _netinfo,
                     destroyOnMasterRemoval=None):
+
+        if options:
+            options = cls.remove_custom_option(options)
+
         if nics:  # New bonding or edit bonding.
             slaves = cls._objectivizeSlaves(name, configurator, _nicSort(nics),
                                             mtu, _netinfo)
@@ -299,7 +313,7 @@

for option in bondingOptions.split():
             key, _ = option.split('=', 1)
-            if key not in defaults:
+            if key not in defaults and key != 'custom':
                 raise ConfigNetworkError(ne.ERR_BAD_BONDING, '%r is not a '
                                          'valid bonding option' % key)

[e-mail] has posted comments on this change.

Change subject: network: allow custom bondOption
......................................................................

Patch Set 1:

* Update tracker::#1234867::OK
* Check Bug-Url::OK
* Check Public Bug::#1234867::OK, public bug
* Check Product::#1234867::OK, Correct product oVirt
* Check TR::#1234867::ERROR, wrong target release for stable branch,  should match ^3.[54321].*
* Check merged to previous::OK, change not open on any previous branch

[e-mail] has posted comments on this change.

Change subject: network: allow custom bondOption
......................................................................

Patch Set 2:

* Update tracker::#1234867::OK
* Check Bug-Url::OK
* Check Public Bug::#1234867::OK, public bug
* Check Product::#1234867::OK, Correct product oVirt
* Check TR::#1234867::OK, correct target release 3.5.5
* Check merged to previous::OK, change not open on any previous branch

Petr Horáček has posted comments on this change.

Change subject: network: allow custom bondOption
......................................................................

Patch Set 2: Verified+1

Passes functional network tests and net* unit tests without a regression.

[e-mail] has posted comments on this change.

Change subject: network: allow custom bondOption
......................................................................

Patch Set 3:

* Update tracker::#1234867::OK
* Check Bug-Url::OK
* Check Public Bug::#1234867::OK, public bug
* Check Product::#1234867::OK, Correct product oVirt
* Check TR::#1234867::OK, correct target release 3.5.5
* Check merged to previous::OK, change not open on any previous branch

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: network: allow custom bondOption
......................................................................

Patch Set 3: Code-Review-1

this is test-only, but it sounds real:

13:37:38 ERROR: testGetBondingOptions (netinfoTests.TestNetinfo)
13:37:38 ----------------------------------------------------------------------
13:37:38 Traceback (most recent call last):
13:37:38   File "/tmp/run/vdsm/tests/monkeypatch.py", line 133, in wrapper
13:37:38     return f(*args, **kw)
13:37:38   File "/tmp/run/vdsm/tests/testValidation.py", line 100, in wrapper
13:37:38     return f(*args, **kwargs)
13:37:38   File "/tmp/run/vdsm/tests/netinfoTests.py", line 370, in testGetBondingOptions
13:37:38     with open(BONDING_MASTERS, 'w') as bonds:
13:37:38 IOError: [Errno 13] Permission denied: '/sys/class/net/bonding_masters'

[e-mail] has posted comments on this change.

Change subject: network: allow custom bondOption
......................................................................

Patch Set 4:

* Update tracker::#1234867::OK
* Check Bug-Url::OK
* Check Public Bug::#1234867::OK, public bug
* Check Product::#1234867::OK, Correct product oVirt
* Check TR::#1234867::OK, correct target release 3.5.5
* Check merged to previous::OK, change not open on any previous branch

Petr Horáček has posted comments on this change.

Change subject: network: allow custom bondOption
......................................................................

Patch Set 4: Verified+1

Passed unit net*.py tests and funtional network tests without a regression.

Petr Horáček has abandoned this change.

Change subject: network: allow custom bondOption
......................................................................

Abandoned

OVS will not be backported to 3.5

[e-mail] has posted comments on this change.

Change subject: network: allow custom bondOption
......................................................................

Patch Set 4:

* Update tracker::#1234867::OK