Change in vdsm[master]: netinfo: Retrieve bonding options differing from defaults

osvoboda at redhat.com osvoboda at redhat.com
Wed Feb 19 10:10:12 UTC 2014


Ondřej Svoboda has posted comments on this change.

Change subject: netinfo: Retrieve bonding options differing from defaults
......................................................................


Patch Set 7:

(5 comments)

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

Line 75: 
Line 76: LIBVIRT_NET_PREFIX = 'vdsm-'
Line 77: DUMMY_BRIDGE = ';vdsmdummy;'
Line 78: DEFAULT_MTU = '1500'
Line 79: MAX_BOND_NAME_LENGTH = 15
> being a class member is not a real problem. but being out of lib/vdsm direc
Yes, that. Moved back.
Line 80: 
Line 81: REQUIRED_BONDINGS = frozenset(('bond0', 'bond1', 'bond2', 'bond3', 'bond4'))
Line 82: 
Line 83: _Qos = namedtuple('Qos', 'inbound outbound')


Line 538:     return info
Line 539: 
Line 540: 
Line 541: def _randomIfaceName():
Line 542:     CHARS = string.ascii_lowercase + string.ascii_uppercase
> string.digits are valid, too, btw.
Yep, I was trying them too. Adding.
Line 543: 
Line 544:     return ''.join(random.choice(CHARS) for _ in range(MAX_BOND_NAME_LENGTH))
Line 545: 
Line 546: 


Line 565: def _getBondingOptions(bond):
Line 566:     defaults = _getDefaultBondingOptions()
Line 567:     opts = bondOpts(bond)
Line 568: 
Line 569:     for key, val in dict(opts).iteritems():
> creating a copy and deleting elements from it is a bit cumbersome. Starting
Dictionary comprehension should work well in this case, defaults.get(k) returns None which never comes out of bondOpts.
Line 570:         if key in defaults and val == defaults[key]:
Line 571:             del opts[key]
Line 572: 
Line 573:     return opts


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

Line 1189:                                                  'options': 'mode=4 miimon=9'})
Line 1190: 
Line 1191:                 # Check that only the 'miimon' option is different from default
Line 1192:                 opts = self.vdsm_net.netinfo.bondings[BONDING_NAME]['options']
Line 1193:                 self.assertEqual(opts, {'miimon': ['9']})
> please add an assertion for  legacy ['cfg'][BONDING_OPTS]
Assertion added.

The check is not general. The default of "802.3ad 4" is actually VDSM's (vdsm/netmodels.py:182), normally I get "balance-rr 0". To move the assertion inside the function I would have to pass the expected bonding options.
Line 1194: 
Line 1195:                 # cleanup
Line 1196:                 networks = dict((NETWORK_NAME + str(num), {'remove': True}) for
Line 1197:                                 num in range(5))


http://gerrit.ovirt.org/#/c/24456/7/tests/netinfoTests.py
File tests/netinfoTests.py:

Line 344: 
Line 345:         with open(BONDING_MASTERS, 'w') as bonds:
Line 346:             try:
Line 347:                 bonds.write('+' + bondName)
Line 348:                 bonds.flush()
> these should be taken out of the try block: -bondName  should be written on
Done
Line 349: 
Line 350:                 self.assertEqual(_getBondingOptions(bondName), {},
Line 351:                                  'No options should be returned.')
Line 352: 


-- 
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: 7
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: Assaf Muller <amuller at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Ondřej Svoboda <osvoboda at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list