Petr Horáček has uploaded a new change for review. Change subject: net: configurators: persist custom bond option ......................................................................
net: configurators: persist custom bond option Until now, custom bond property was exposed only to before_network_setup hook and then dropped (so it was not passed to configurators and persisted). But this is not how custom *network* property behaves. We want these two custom properties to be symmetric - they have to be persisted (if unified persistence is used) and must not be shown in netinfo. This patch moves removal of custom property down to configurators. Thanks to it, custom property is persisted, but not set and bond option. Custom property is not dropped only in configurators, but in models.py:Bond:areOptionsApplied as well - we don't want configurator to reconfigure bond when there is a custom property defined while its value is not used by configurator. Change-Id: Ic4f1564e19904cc1d53bc6d1cf732ca35375332e Signed-off-by: Petr Horáček <phora...@redhat.com> --- M tests/functional/networkTests.py M vdsm/network/configurators/__init__.py M vdsm/network/configurators/ifcfg.py M vdsm/network/configurators/iproute2.py M vdsm/network/configurators/pyroute_two.py M vdsm/network/models.py 6 files changed, 38 insertions(+), 23 deletions(-) git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/91/43891/1 diff --git a/tests/functional/networkTests.py b/tests/functional/networkTests.py index e5bfe25..4d9f480 100644 --- a/tests/functional/networkTests.py +++ b/tests/functional/networkTests.py @@ -2648,8 +2648,12 @@ NOCHK, test_kernel_config=False) self.assertEqual(status, SUCCESS, msg) self.assertBondExists(BONDING_NAME, nics) - opts = self.vdsm_net.config.bonds.get(BONDING_NAME).get('options') - self.assertEquals(opts, 'mode=balance-rr') + # custom property have to be persisted (if unified persistence is + # used) and must not be shown in netinfo. + self._assert_exact_bond_opts(BONDING_NAME, ('mode=balance-rr',)) + if vdsm.config.config.get('vars', 'net_persistence') == 'unified': + bond = self.vdsm_net.config.bonds.get(BONDING_NAME) + self.assertEquals(bond.get('options'), 'mode=balance-rr') status, msg = self.setupNetworks( {}, {BONDING_NAME: {'remove': True}}, NOCHK) diff --git a/vdsm/network/configurators/__init__.py b/vdsm/network/configurators/__init__.py index e51d3ed..bc41119 100644 --- a/vdsm/network/configurators/__init__.py +++ b/vdsm/network/configurators/__init__.py @@ -178,3 +178,16 @@ if iface.blockingdhcp and rc: raise ConfigNetworkError(ERR_FAILED_IFUP, 'dhclient%s failed', family) + + +def remove_custom_bond_option(options): + """ Removes 'custom' option from bond options string. + + >>> remove_custom_bond_option('custom=foo=bar mode=1') + 'mode=1' + """ + d_opts = dict(opt.split('=', 1) for opt in options.split()) + if d_opts.pop('custom', None) is not None: + options = ' '.join(['%s=%s' % (key, value) + for key, value in d_opts.items()]) + return options diff --git a/vdsm/network/configurators/ifcfg.py b/vdsm/network/configurators/ifcfg.py index f2dbd5e..c5e2c0d 100644 --- a/vdsm/network/configurators/ifcfg.py +++ b/vdsm/network/configurators/ifcfg.py @@ -44,7 +44,8 @@ if utils.isOvirtNode(): from ovirt.node.utils import fs as node_fs -from . import Configurator, dhclient, getEthtoolOpts, libvirt +from . import (Configurator, dhclient, getEthtoolOpts, libvirt, + remove_custom_bond_option) from ..errors import ConfigNetworkError, ERR_FAILED_IFUP from ..models import Nic, Bridge, IPv4, IPv6 from ..sourceroute import StaticSourceRoute, DynamicSourceRoute @@ -567,7 +568,9 @@ def addBonding(self, bond, **opts): """ Create ifcfg-* file with proper fields for bond """ - conf = 'BONDING_OPTS=%s\n' % pipes.quote(bond.options or '') + options = (remove_custom_bond_option(bond.options) + if bond.options else '') + conf = 'BONDING_OPTS=%s\n' % pipes.quote(options) opts['hotplug'] = 'no' # So that udev doesn't trigger an ifup if bond.bridge: conf += 'BRIDGE=%s\n' % pipes.quote(bond.bridge.name) diff --git a/vdsm/network/configurators/iproute2.py b/vdsm/network/configurators/iproute2.py index 0b5b73e..0850212 100644 --- a/vdsm/network/configurators/iproute2.py +++ b/vdsm/network/configurators/iproute2.py @@ -29,7 +29,8 @@ from vdsm.utils import CommandPath from vdsm.utils import execCmd -from . import Configurator, runDhclient, getEthtoolOpts, libvirt +from . import (Configurator, runDhclient, getEthtoolOpts, libvirt, + remove_custom_bond_option) from .dhclient import DhcpClient from ..errors import ConfigNetworkError, ERR_FAILED_IFUP, ERR_FAILED_IFDOWN from ..models import Nic @@ -340,7 +341,9 @@ def addBondOptions(self, bond): logging.debug('Add bond options %s', bond.options) - for option in bond.options.split(): + options = (remove_custom_bond_option(bond.options) + if bond.options else '') + for option in options: key, value = option.split('=') with open(netinfo.BONDING_OPT % (bond.name, key), 'w') as f: f.write(value) diff --git a/vdsm/network/configurators/pyroute_two.py b/vdsm/network/configurators/pyroute_two.py index c7730d3..b194a03 100644 --- a/vdsm/network/configurators/pyroute_two.py +++ b/vdsm/network/configurators/pyroute_two.py @@ -24,7 +24,7 @@ from vdsm import ipwrapper from vdsm.netconfpersistence import RunningConfig -from . import libvirt, runDhclient +from . import libvirt, runDhclient, remove_custom_bond_option from .dhclient import DhcpClient from .iproute2 import Iproute2 @@ -153,7 +153,9 @@ def addBondOptions(self, bond): logging.debug('Add bond options %s', bond.options) - for option in bond.options.split(): + options = (remove_custom_bond_option(bond.options) + if bond.options else '') + for option in options: key, value = option.split('=') with open(netinfo.BONDING_OPT % (bond.name, key), 'w') as f: f.write(value) diff --git a/vdsm/network/models.py b/vdsm/network/models.py index 6466917..19d0179 100644 --- a/vdsm/network/models.py +++ b/vdsm/network/models.py @@ -24,6 +24,7 @@ from vdsm import netinfo +from .configurators import remove_custom_bond_option from .errors import ConfigNetworkError from . import errors as ne @@ -241,9 +242,11 @@ # TODO: this method returns True iff self.options are a subset of the # TODO: current bonding options. VDSM should probably compute if the # TODO: non-default settings are equal to the non-default state. - if not self.options: + options = (remove_custom_bond_option(self.options) + if self.options else '') + if options == '': return True - confOpts = [option.split('=', 1) for option in self.options.split(' ')] + confOpts = [option.split('=', 1) for option in options.split(' ')] activeOpts = netinfo.bondOpts(self.name, (name for name, value in confOpts)) return all(value in activeOpts[name] for name, value in confOpts) @@ -267,22 +270,9 @@ _netinfo=_netinfo)) return slaves - @staticmethod - def remove_custom_option(options): - """ 'custom' property should not be exposed to configurator, it is - only for purpose of hooks """ - d_opts = dict(opt.split('=', 1) for opt in options.split()) - if d_opts.pop('custom', None) is not None: - options = ' '.join(['%s=%s' % (key, value) - for key, value in d_opts.items()]) - return options - @classmethod def objectivize(cls, name, configurator, options, nics, mtu, _netinfo, destroyOnMasterRemoval=None): - - if options: - options = cls.remove_custom_option(options) if nics: # New bonding or edit bonding. slaves = cls._objectivizeSlaves(name, configurator, _nicSort(nics), -- To view, visit https://gerrit.ovirt.org/43891 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ic4f1564e19904cc1d53bc6d1cf732ca35375332e Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Petr Horáček <phora...@redhat.com> _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches