Change in vdsm[master]: netinfo: Report current system values for bonding options

osvoboda at redhat.com osvoboda at redhat.com
Thu Jun 12 10:03:04 UTC 2014


Ondřej Svoboda has posted comments on this change.

Change subject: netinfo: Report current system values for bonding options
......................................................................


Patch Set 52:

(4 comments)

Some final (?) comments before we push this. They contain a warning about the bond not being listed as the first option.

http://gerrit.ovirt.org/#/c/24456/52/lib/vdsm/netinfo.py
File lib/vdsm/netinfo.py:

Line 644:     mode = opts['mode'][-1] if 'mode' in opts else None
Line 645:     defaults = getDefaultBondingOptions(mode)
Line 646: 
Line 647:     return dict(((opt, val) for (opt, val) in opts.iteritems()
Line 648:                  if val and val != defaults.get(opt)))
For simplicity and coherence, the dictionary returned is in the same format as given by bondOpts (arrays of 1 or 2 strings, even for numerical values).

A GUI for editing bonding options could differentiate between the two types of values easily:

* numerical bonding option is returned as an array containing 1 string only

* symbolical bonding option is returned as an array containing 2 strings: first, the name of the value, second, the numerical code of the value

In this patch, it is premature to do any converting of values. As hinted above, it is not necessary anyway.
Line 649: 
Line 650: 
Line 651: def _bondOptsForIfcfg(opts):
Line 652:     """


Line 654:     the order symbolic name, numeric value, e.g. 'balance-rr 0'.
Line 655:     Choose the numeric value from a list given by bondOpts().
Line 656:     """
Line 657:     return ' '.join((opt + '=' + val[-1] for (opt, val)
Line 658:                      in sorted(opts.iteritems())))
No special handling for the mode in this patch.
It must be applied/set first, so it should also be reported as the first.
Line 659: 
Line 660: 
Line 661: def _bondinfo(link, ipaddrs):
Line 662:     info = _devinfo(link, ipaddrs)


Line 668:     # Replace or empty legacy ifcfg options
Line 669:     if opts:
Line 670:         info['cfg']['BONDING_OPTS'] = _bondOptsForIfcfg(opts)
Line 671:     elif 'BONDING_OPTS' in info['cfg']:
Line 672:         info['cfg']['BONDING_OPTS'] = ''
If 'BONDING_OPTS' was not present in an ifcfg file and the options are default, no compatibility string is added (to avoid clutter).
Line 673: 
Line 674:     return info
Line 675: 
Line 676: 


http://gerrit.ovirt.org/#/c/24456/52/tests/functional/networkTests.py
File tests/functional/networkTests.py:

Line 229:                              set(config.bonds[bondName].get('nics')))
Line 230:         if options is None:
Line 231:             options = 'mode=4 miimon=150'
Line 232:         active = netinfo.bondings[bondName]['cfg']['BONDING_OPTS']
Line 233:         self.assertTrue(set(options.split()) <= set(active.split()))
The condition mirrors a check 'areOptionsApplied' in models (Bond).

Previously, I was too strict. With the patch to reset pre-existing options to defaults, I wanted VDSM to behave in such a way that options it did not modify were cleared. That after editing options, it would end up with nothing else but requested.
Line 234: 
Line 235:     def assertBondDoesntExist(self, bondName, nics=None):
Line 236:         netinfo = self.vdsm_net.netinfo
Line 237:         config = self.vdsm_net.config


-- 
To view, visit http://gerrit.ovirt.org/24456
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ief6d366b1b761627c7203cf236b75ef538af3e26
Gerrit-PatchSet: 52
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda <osvoboda at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Ondřej Svoboda <osvoboda at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list