Change in vdsm[ovirt-3.5]: net: bonding: validate options based on pre-dumped ones

Hello Ondřej Svoboda, Dan Kenigsberg,

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

https://gerrit.ovirt.org/43148

to review the following change.

Change subject: net: bonding: validate options based on pre-dumped ones
......................................................................

net: bonding: validate options based on pre-dumped ones

The removed code created a bond device only to check if the provided
options are recongnized by the kernel. This was buggy, as the options
depends on the bonding mode. It was also redundant, as we already depend
on pre-dumped maps of bond options per bond mode, so we can (and should)
use it here, too.

Change-Id: I03fce903280d5eda817e713a51c8079dab9d7649
Signed-off-by: Dan Kenigsberg 
Reviewed-on: https://gerrit.ovirt.org/41616
Continuous-Integration: Jenkins CI
Reviewed-by: Ondřej Svoboda 
Reviewed-by: Petr Horáček 
Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1234867
---
M tests/netmodelsTests.py
M vdsm/network/api.py
M vdsm/network/models.py
3 files changed, 58 insertions(+), 41 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/48/43148/1

diff --git a/tests/netmodelsTests.py b/tests/netmodelsTests.py
index a2a3ee3..548f01b 100644
--- a/tests/netmodelsTests.py
+++ b/tests/netmodelsTests.py
@@ -28,8 +28,6 @@
 from network.models import _nicSort

from testrunner import VdsmTestCase as TestCaseBase
-from testValidation import ValidateRunningAsRoot
-from nose.plugins.skip import SkipTest

from monkeypatch import MonkeyPatch

@@ -80,18 +78,17 @@
         self.assertEqual(Bond.validateName('bond11'), None)
         self.assertEqual(Bond.validateName('bond11128421982'), None)

-    @ValidateRunningAsRoot
+    @MonkeyPatch(netinfo, 'BONDING_DEFAULTS', netinfo.BONDING_DEFAULTS
+                 if os.path.exists(netinfo.BONDING_DEFAULTS)
+                 else '../vdsm/bonding-defaults.json')
     def testValidateBondingOptions(self):
-        if not os.path.exists(netinfo.BONDING_MASTERS):
-            raise SkipTest("bonding kernel module could not be found.")
-
         opts = 'mode=802.3ad miimon=150'
         badOpts = 'foo=bar badopt=one'

with self.assertRaises(errors.ConfigNetworkError) as cne:
-            Bond.validateOptions('bond0', badOpts)
+            Bond.validateOptions(badOpts)
         self.assertEqual(cne.exception.errCode, errors.ERR_BAD_BONDING)
-        self.assertEqual(Bond.validateOptions('bond0', opts), None)
+        self.assertEqual(Bond.validateOptions(opts), None)

def testIsIpValid(self):
         addresses = ('10.18.1.254', '10.50.25.177', '250.0.0.1',
@@ -122,7 +119,6 @@
             self.assertEqual(IPv4.validateNetmask(mask), None)

@MonkeyPatch(netinfo, 'getMtu', lambda *x: 1500)
-    @MonkeyPatch(Bond, 'validateOptions', lambda *x: 0)
     def testTextualRepr(self):
         _netinfo = {'networks': {}, 'vlans': {},
                     'nics': ['testnic1', 'testnic2'],
diff --git a/vdsm/network/api.py b/vdsm/network/api.py
index f829732..52bbf45 100755
--- a/vdsm/network/api.py
+++ b/vdsm/network/api.py
@@ -470,7 +470,7 @@
     for bonding, bondingAttrs in bondings.iteritems():
         Bond.validateName(bonding)
         if 'options' in bondingAttrs:
-            Bond.validateOptions(bonding, bondingAttrs['options'])
+            Bond.validateOptions(bondingAttrs['options'])

if bondingAttrs.get('remove', False):
             continue
diff --git a/vdsm/network/models.py b/vdsm/network/models.py
index 8e33950..9608937 100644
--- a/vdsm/network/models.py
+++ b/vdsm/network/models.py
@@ -17,9 +17,7 @@
 # Refer to the README and COPYING files for full details of the license
 #
 from collections import namedtuple
-from contextlib import contextmanager
 import logging
-import os
 import re
 import socket
 import struct
@@ -179,8 +177,11 @@
         for slave in slaves:
             slave.master = self
         self.slaves = slaves
-        self.options = options or ''
-        self.validateOptions(name, self.options)
+        if options is None:
+            self.options = 'mode=802.3ad miimon=150'
+        else:
+            self.validateOptions(options)
+            self.options = self._reorderOptions(options)
         self.destroyOnMasterRemoval = destroyOnMasterRemoval
         super(Bond, self).__init__(name, configurator, ipconfig, mtu)

@@ -270,36 +271,56 @@
                                      name)

@classmethod
-    def validateOptions(cls, bonding, bondingOptions):
+    def validateOptions(cls, bondingOptions):
         'Example: BONDING_OPTS="mode=802.3ad miimon=150"'
-        with cls._validationBond(bonding) as bond:
-            try:
-                for option in bondingOptions.split():
-                    key, _ = option.split('=')
-                    if not os.path.exists('/sys/class/net/%s/bonding/%s' %
-                                          (bond, key)):
-                        raise ConfigNetworkError(ne.ERR_BAD_BONDING, '%r is '
-                                                 'not a valid bonding option' %
-                                                 key)
-            except ValueError:
-                raise ConfigNetworkError(ne.ERR_BAD_BONDING, 'Error parsing '
-                                         'bonding options: %r' %
-                                         bondingOptions)
+        mode = 'balance-rr'
+        try:
+            for option in bondingOptions.split():
+                key, value = option.split('=', 1)
+                if key == 'mode':
+                    mode = value
+        except ValueError:
+            raise ConfigNetworkError(ne.ERR_BAD_BONDING, 'Error parsing '
+                                     'bonding options: %r' % bondingOptions)
+
+        MODE_NAME_TO_NUMBER = {
+            'balance-rr': '0',
+            'active-backup': '1',
+            'balance-xor': '2',
+            'broadcast': '3',
+            '802.3ad': '4',
+            'balance-tlb': '5',
+            'balance-alb': '6',
+        }
+
+        if mode in MODE_NAME_TO_NUMBER:
+            mode = MODE_NAME_TO_NUMBER[mode]
+        defaults = netinfo.getDefaultBondingOptions(mode)
+
+        for option in bondingOptions.split():
+            key, _ = option.split('=', 1)
+            if key not in defaults:
+                raise ConfigNetworkError(ne.ERR_BAD_BONDING, '%r is not a '
+                                         'valid bonding option' % key)

@staticmethod
-    @contextmanager
-    def _validationBond(bonding):
-        bond_created = False
-        try:
-            bonding = open(netinfo.BONDING_MASTERS, 'r').read().split()[0]
-        except IndexError:
-            open(netinfo.BONDING_MASTERS, 'w').write('+%s\n' % bonding)
-            bond_created = True
-        try:
-            yield bonding
-        finally:
-            if bond_created:
-                open(netinfo.BONDING_MASTERS, 'w').write('-%s\n' % bonding)
+    def _reorderOptions(options):
+        """Order the mode first and the rest of options alphabetically."""
+        if not options.strip():
+            return ''
+
+        opts = dict((option.split('=', 1) for option in options.split()))
+
+        mode = opts.pop('mode', None)
+        opts = sorted(opts.iteritems())
+        if mode:
+            opts.insert(0, ('mode', mode))
+
+        return ' '.join((opt + '=' + val for (opt, val) in opts))
+
+    @property
+    def backing_device(self):
+        return True

class IPv4(object):

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

Change subject: net: bonding: validate options based on pre-dumped ones
......................................................................

Patch Set 1:

* update_tracker: 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

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

Change subject: net: bonding: validate options based on pre-dumped ones
......................................................................

Patch Set 2:

* Update tracker::#1234867::OK
* Check Bug-Url::OK
* Check Public Bug::#1234867::ERROR, private bug
* Check Public Bug::WARN, no public bug url found
* Check merged to previous::OK, change not open on any previous branch

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

Change subject: net: bonding: validate options based on pre-dumped ones
......................................................................

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: net: bonding: validate options based on pre-dumped ones
......................................................................

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

Petr Horáček has abandoned this change.

Change subject: net: bonding: validate options based on pre-dumped ones
......................................................................

Abandoned

OVS will not be backported to 3.5

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

Change subject: net: bonding: validate options based on pre-dumped ones
......................................................................

Patch Set 3:

* Update tracker::#1234867::OK