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

asegurap at redhat.com asegurap at redhat.com
Fri Aug 16 00:02:51 UTC 2013


Antoni Segura Puimedon has posted comments on this change.

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


Patch Set 10:

(7 comments)

Thanks for the review.

....................................................
File tests/functional/networkTests.py
Line 51: def nonChangingOperstate(device):
Line 52:     def changed(dev, changes):
Line 53:         status = operstate(dev)
Line 54:         while not done:
Line 55:             newState = operstate(dev)
I'll add a try except that adds 'exception' to changes. So that the tests will fail.
Line 56:             if status != newState:
Line 57:                 changes.append(newState)
Line 58:                 status = newState
Line 59: 


Line 52:     def changed(dev, changes):
Line 53:         status = operstate(dev)
Line 54:         while not done:
Line 55:             newState = operstate(dev)
Line 56:             if status != newState:
Inotify can't work with sysfs because the content files is created only on open for read, that's why I'm opening and closing all the time. I'm aware that it could be raceful, but the only alternative is to register to netlink, which we still don't have the capability of doing. In the future I'll replace it with netlink reading.
Line 57:                 changes.append(newState)
Line 58:                 status = newState
Line 59: 
Line 60:     try:


Line 64:                               args=(device, changes))
Line 65:         monitoring_t.start()
Line 66:         yield
Line 67:     finally:
Line 68:         time.sleep(3)  # So that the last action in yield gets to kernel
The purpose of this sleep is:

ifup returns when the kernel accepts the netlink message, not when it is reflected on sysfs. Without this sleep we'd fail to detect a late change in the operstate (I verified that 1-2s were at times not long enough).
Line 69:         done = True
Line 70:         if changes:
Line 71:             raise ValueError('%s operstate changed: %r' % (device, changes))
Line 72: 


Line 65:         monitoring_t.start()
Line 66:         yield
Line 67:     finally:
Line 68:         time.sleep(3)  # So that the last action in yield gets to kernel
Line 69:         done = True
I modeled this method after the utils.pinger. In practice the thread will exit when seeing done and will not remain alive. I don't think we need to explicitly wait for it to exit.
Line 70:         if changes:
Line 71:             raise ValueError('%s operstate changed: %r' % (device, changes))
Line 72: 
Line 73: 


....................................................
File vdsm/netconf/ifcfg.py
Line 105:         # to bond.
Line 106:         isIfcfgControlled = os.path.isfile(netinfo.NET_CONF_PREF + bond.name)
Line 107:         areOptionsApplied = bond.areOptionsApplied()
Line 108:         if not isIfcfgControlled or not areOptionsApplied:
Line 109:             bridgeName = _netinfo.getBridgedNetworkForIface(bond.name)
What I meant with the code is:
If the bond exists but there is no ifcfg file for it, we need to make our own configuration for it (thus the configApplier.addBonding). It is arguable if we should get the bridge in the case where there was no ifcfg. I originally thought that it wouldn't hurt us to have it, and the logic is simpler, but I guess it could work if we were to only add the bond.master if it is ifcfgControlled.
Line 110:             if bridgeName:
Line 111:                 bond.master = Bridge(bridgeName, self, port=bond)
Line 112:             self.configApplier.addBonding(bond)
Line 113: 


Line 116:                 ifdown(slave.name)  # nics must be down to join a bond
Line 117:                 self.configApplier.addNic(slave)
Line 118:                 ifup(slave.name)
Line 119: 
Line 120:         if not isIfcfgControlled or not areOptionsApplied:
this configurator doesn't have a way to change the bond configuration on the live bond, that's why I set it to ifdown and ifup after the bond gets the ifcfg file for the first time (created in line 112).
Line 121:             ifdown(bond.name)
Line 122:             ifup(bond.name)
Line 123: 
Line 124:     def configureNic(self, nic, **opts):


....................................................
File vdsm/netmodels.py
Line 193: 
Line 194:     def areOptionsApplied(self):
Line 195:         activeOpts = netinfo.bondOpts(self.name)
Line 196:         confOpts = (option.split('=', 1) for option in self.options.split(' '))
Line 197:         return all(value in activeOpts[name] for name, value in confOpts)
Do you mean that bondOpts would be like this:


def bondOpts(bonding, keys=None):
    """ Returns a dictionary of bond option name and a values iterable. E.g.,
    {'mode': ('balance-rr', '0'), 'xmit_hash_policy': ('layer2', '0')}

    :param bonding: The name of the bond to get the options of.
    :param keys: An iterable that if passed will restrict which options are
                 returned.
    """
    opts = {}
    for path in iglob(BONDING_OPTS % bonding):
        if keys and os.path.basename(path) not in keys:
            continue
        with open(path) as optFile:
            opts[os.path.basename(path)] = \
                optFile.read().rstrip().split(' ')
    return opts
Line 198: 
Line 199:     def remove(self):
Line 200:         logging.debug('Removing bond %r', self)
Line 201:         self.configurator.removeBond(self)


-- 
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: 10
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