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

phoracek at redhat.com phoracek at redhat.com
Thu Jul 2 11:53:18 UTC 2015


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 <danken at redhat.com>
Reviewed-on: https://gerrit.ovirt.org/41616
Continuous-Integration: Jenkins CI
Reviewed-by: Ondřej Svoboda <osvoboda at redhat.com>
Reviewed-by: Petr Horáček <phoracek at redhat.com>
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):


-- 
To view, visit https://gerrit.ovirt.org/43148
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I03fce903280d5eda817e713a51c8079dab9d7649
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.5
Gerrit-Owner: Petr Horáček <phoracek at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Ondřej Svoboda <osvoboda at redhat.com>


More information about the vdsm-patches mailing list