Change in vdsm[master]: Don't down bonds and nics when adding vlan or resizing

gvallare at redhat.com gvallare at redhat.com
Wed Aug 14 19:23:03 UTC 2013


Giuseppe Vallarelli has posted comments on this change.

Change subject: Don't down bonds and nics when adding vlan or resizing
......................................................................


Patch Set 9: Code-Review-1

(5 comments)

Very minor refinements.

....................................................
File lib/vdsm/netinfo.py
Line 218:     return open(BONDING_SLAVES % bonding).readline().split()
Line 219: 
Line 220: 
Line 221: def bondOpts(bonding):
Line 222:     """ Returns a dictionary of bond mode name and a values iterable."""
bond mode and which values ? Since you added the docstring better be complete :)
Thanks I appreciate.
Line 223:     opts = {}
Line 224:     for path in iglob(BONDING_OPTS % bonding):
Line 225:         with open(path) as optFile:
Line 226:             opts[os.path.basename(path)] = optFile.read().rstrip().split(' ')


Line 278:     try:
Line 279:         # nics() filters out OS devices (bonds, vlans, bridges)
Line 280:         # operstat() filters out down/disabled nics
Line 281:         # virtio is a valid device, but doesn't support speed
Line 282:         if dev in nics() and operstate(dev) == OPERSTATE_UP and \
Nitpick: instead of \ use parenthesis for a longer explanation have a look at the link below:
http://www.python.org/dev/peps/pep-0008/#maximum-line-length
Line 283:                 not isvirtio(dev):
Line 284:             # the device may have been disabled/downed after checking
Line 285:             # so we validate the return value as sysfs may return
Line 286:             # special values to indicate the device is down/disabled


....................................................
File tests/functional/networkTests.py
Line 47:     restoreNetConfig()
Line 48: 
Line 49: 
Line 50: @contextmanager
Line 51: def nonChangingOperstate(device):
why in networkTest module and not utils?

what about constantOperstate instead of nonChaningOperstate?
Line 52:     def changed(dev, changes):
Line 53:         status = operstate(dev)
Line 54:         while not done:
Line 55:             newState = operstate(dev)


....................................................
File vdsm/netmodels.py
Line 80:         super(Nic, self).__init__(name, configurator, ipconfig,
Line 81:                                   mtu=max(mtu, netinfo.getMtu(name)))
Line 82: 
Line 83:     def configure(self, **opts):
Line 84:         if not self.vlan or \
Parenthesis instead of \ :
http://www.python.org/dev/peps/pep-0008/#maximum-line-length
Line 85:                 netinfo.operstate(self.name) != netinfo.OPERSTATE_UP:
Line 86:             self.configurator.configureNic(self, **opts)
Line 87: 
Line 88:     def remove(self):


Line 180:     def __repr__(self):
Line 181:         return 'Bond(%s: %r)' % (self.name, self.slaves)
Line 182: 
Line 183:     def configure(self, **opts):
Line 184:         # When the bond is up and we are not changing the configuration that
At this point I think you can turn the comment into a docstring.
Line 185:         # is already applied in any way, we can skip the configuring.
Line 186:         if not(self.vlan and
Line 187:                self.name in netinfo.bondings() and
Line 188:                netinfo.operstate(self.name) == netinfo.OPERSTATE_UP and


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id3075a2e65f06416c840c6a98689b77555b22e5d
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap 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: Giuseppe Vallarelli <gvallare at redhat.com>
Gerrit-Reviewer: Livnat Peer <lpeer at redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: Petr Ĺ ebek <psebek at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list