Change in vdsm[master]: supervdsm: moving nics between bonds

edwardh at redhat.com edwardh at redhat.com
Thu Dec 17 08:37:26 UTC 2015


Edward Haas has posted comments on this change.

Change subject: supervdsm: moving nics between bonds
......................................................................


Patch Set 1: Code-Review-1

(7 comments)

https://gerrit.ovirt.org/#/c/50596/1/lib/vdsm/network/api.py
File lib/vdsm/network/api.py:

Line 690: modified_bondings=bondings
You probably meant to pass 'edition'.
Why not passing the removed_slaves instead?


Line 708: modified_bondings
Not in use.


Line 711:         for slave_name in _netinfo.bondings[name]['slaves']:
        :             if slave_name not in attrs.get('nics')
removed_slaves = _netinfo.bondings[name]['slaves'] - attrs.get('nics')


Line 714:  configurator.remove_slaves(removed_slaves, _netinfo)
Why this is saved here?
In general, saving data on another object directly seems wrong.


https://gerrit.ovirt.org/#/c/50596/1/lib/vdsm/network/configurators/ifcfg.py
File lib/vdsm/network/configurators/ifcfg.py:

Line 129: remove_slaves
Who calls remove_slaves?


Line 125:             self.runningConfig.setBonding(
Line 126:                 bond.name, {'options': bond.options,
Line 127:                             'nics': [slave.name for slave in bond.slaves]})
Line 128: 
Line 129:     def remove_slaves(self, removed_slaves, _netinfo):
If this is now a configurator API, the superclass and all other configurators should support it.
Line 130:         for nic in removed_slaves:
Line 131:             ifdown(nic)  # So that no users will be detected for it.
Line 132:             Nic(nic, self, _netinfo=_netinfo).remove()
Line 133: 


https://gerrit.ovirt.org/#/c/50596/1/lib/vdsm/network/models.py
File lib/vdsm/network/models.py:

Line 281:             nicBond = _netinfo.getBondingForNic(nic)
        :             is_slave_in_bond = cls.is_slave_used(name, nicBond, nic,
        :                                                  modified_bondings)
        :             if nicVlans or nicNet or is_slave_in_bond:
I think it can be simplified if the list of removed slaves is made available here:

def is_nic_used_by_other_bond(bond, nic, _net_info, removed_slaves):
    other_bond = _netinfo.getBondingForNic(nic) if nic not in removed_slaves else None
    used = True if other_bond else False
    return used

At the caller, if the nic is added as a slave, remove it from the removed_slaves list.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia43fbfe90d1335ed4a740a1bc4f09610da2ecdce
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki <mmirecki at redhat.com>
Gerrit-Reviewer: Alona Kaplan <alkaplan at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Ido Barkan <ibarkan at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list