Marcin Mirecki has uploaded a new change for review. Change subject: supervdsm: moving nics between bonds ......................................................................
supervdsm: moving nics between bonds This patch fixes an problem where a slave removed from one bond could sometimes not be assigned to another bond in the same request. This happened because removing, editing and adding of slaves to a bond was done bond by bond, so if a nic was added to a bond which was handled before the bond from which the nic was removed the operation failed. Example: bond0: nic0, nic1 => nic2, nic3 bond1: nic2, nic3 => nic0, nic1 In this situation first bond0 is processed: nic0 and nic1 will be removed, but nic2 and nic3 can not yet be removed as they are part of bond1 (validation fails). Processing of bond1 will introduce errors, as nic2 and nic3 will be removed (already assigned to bond0), which will in effect reset their ifcfg files, and cause the previous attachement to bond0 to be undone. The patch changes the validation to check if a slave added to a bond is deleted from another bond. Also the removing of slaves from all bonds is done before any other bond operation. Bug-Url: https://bugzilla.redhat.com/1269175 Change-Id: Ia43fbfe90d1335ed4a740a1bc4f09610da2ecdce Signed-off-by: Marcin Mirecki <mmire...@redhat.com> --- M lib/vdsm/network/api.py M lib/vdsm/network/configurators/ifcfg.py M lib/vdsm/network/models.py 3 files changed, 43 insertions(+), 10 deletions(-) git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/96/50596/1 diff --git a/lib/vdsm/network/api.py b/lib/vdsm/network/api.py index c3d82d4..d70b39f 100755 --- a/lib/vdsm/network/api.py +++ b/lib/vdsm/network/api.py @@ -680,10 +680,14 @@ else: addition.append((name, attrs)) + _handle_removed_slaves(edition, _netinfo, configurator, bondings) + for name, attrs in edition: bond = Bond.objectivize(name, configurator, attrs.get('options'), - attrs.get('nics'), mtu=None, _netinfo=_netinfo, - destroyOnMasterRemoval='remove' in attrs) + attrs.get('nics'), mtu=None, + _netinfo=_netinfo, + destroyOnMasterRemoval='remove' in attrs, + modified_bondings=bondings) if len(bond.slaves) == 0: raise ConfigNetworkError(ne.ERR_BAD_PARAMS, 'Missing required nics' ' for bonding device.') @@ -700,6 +704,16 @@ configurator.configureBond(bond) +def _handle_removed_slaves(edited_bonds, _netinfo, configurator, + modified_bondings): + for name, attrs in edited_bonds: + removed_slaves = [] + for slave_name in _netinfo.bondings[name]['slaves']: + if slave_name not in attrs.get('nics'): + removed_slaves.append(slave_name) + configurator.remove_slaves(removed_slaves, _netinfo) + + def _buildBondOptions(bondName, bondings, _netinfo): logger = logging.getLogger("_buildBondOptions") diff --git a/lib/vdsm/network/configurators/ifcfg.py b/lib/vdsm/network/configurators/ifcfg.py index f169400..6d49c61 100644 --- a/lib/vdsm/network/configurators/ifcfg.py +++ b/lib/vdsm/network/configurators/ifcfg.py @@ -126,6 +126,11 @@ bond.name, {'options': bond.options, 'nics': [slave.name for slave in bond.slaves]}) + def remove_slaves(self, removed_slaves, _netinfo): + for nic in removed_slaves: + ifdown(nic) # So that no users will be detected for it. + Nic(nic, self, _netinfo=_netinfo).remove() + def editBonding(self, bond, _netinfo): """ Modifies the bond so that the bond in the system ends up with the @@ -148,10 +153,6 @@ bond.master = Bridge(bridgeName, self, port=bond) self.configApplier.addBonding(bond) bondIfcfgWritten = True - - for nic in currentNics - nicsToSet: - ifdown(nic) # So that no users will be detected for it. - Nic(nic, self, _netinfo=_netinfo).remove() for slave in bond.slaves: if slave.name in nicsToAdd: diff --git a/lib/vdsm/network/models.py b/lib/vdsm/network/models.py index a5609fd..9305a4d 100644 --- a/lib/vdsm/network/models.py +++ b/lib/vdsm/network/models.py @@ -257,13 +257,31 @@ self.configurator.removeBond(self) @classmethod - def _objectivizeSlaves(cls, name, configurator, nics, mtu, _netinfo): + def is_slave_used(cls, new_bond_name, existing_bond_name, slave_name, + modified_bondings): + if not existing_bond_name or new_bond_name == existing_bond_name: + return False + if not modified_bondings: + return False + if existing_bond_name in modified_bondings: + bond = modified_bondings[existing_bond_name] + if 'remove' in bond and bond['remove'] == 'true': + return False + if 'nics' in bond and slave_name not in bond['nics']: + return False + return True + + @classmethod + def _objectivizeSlaves(cls, name, configurator, nics, mtu, _netinfo, + modified_bondings): slaves = [] for nic in nics: nicVlans = tuple(_netinfo.getVlansForIface(nic)) nicNet = _netinfo.getNetworkForIface(nic) nicBond = _netinfo.getBondingForNic(nic) - if nicVlans or nicNet or nicBond and nicBond != name: + is_slave_in_bond = cls.is_slave_used(name, nicBond, nic, + modified_bondings) + if nicVlans or nicNet or is_slave_in_bond: raise ConfigNetworkError( ne.ERR_USED_NIC, 'nic %s already used by %s' % (nic, nicVlans or nicNet or nicBond)) @@ -273,11 +291,11 @@ @classmethod def objectivize(cls, name, configurator, options, nics, mtu, _netinfo, - destroyOnMasterRemoval=None): + destroyOnMasterRemoval=None, modified_bondings=None): if nics: # New bonding or edit bonding. slaves = cls._objectivizeSlaves(name, configurator, _nicSort(nics), - mtu, _netinfo) + mtu, _netinfo, modified_bondings) if name in _netinfo.bondings: if _netinfo.ifaceUsers(name): mtu = max(mtu, mtus.getMtu(name)) -- To view, visit https://gerrit.ovirt.org/50596 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ia43fbfe90d1335ed4a740a1bc4f09610da2ecdce Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Marcin Mirecki <mmire...@redhat.com> _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
