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