From f38f0bfa06f999d538bfd36823ef5133e498ad56 Mon Sep 17 00:00:00 2001 From: Eitan Raviv Date: Tue, 26 Jul 2022 11:36:59 +0300 Subject: [PATCH] Revert "network: use nmstate for bridge options" This reverts commit 35e919fd23023f64bb4c59de9631363b675f165b. The revert is temporary until a fix for [1] is merged. [1] https://bugzilla.redhat.com/2108974 Change-Id: I45d2a7be386d8e192f12597403be870fa8a92ce3 Signed-off-by: Eitan Raviv --- lib/vdsm/network/link/setup.py | 77 ++++++--------------- lib/vdsm/network/netswitch/configurator.py | 2 + lib/vdsm/network/nmstate/bridge_util.py | 1 - lib/vdsm/network/nmstate/linux_bridge.py | 7 +- tests/network/functional/bridge_test.py | 4 +- tests/network/functional/netrestore_test.py | 8 ++- tests/network/functional/rollback_test.py | 3 +- tests/network/unit/link_setup_test.py | 21 ++++-- 8 files changed, 47 insertions(+), 76 deletions(-) diff --git a/lib/vdsm/network/link/setup.py b/lib/vdsm/network/link/setup.py index 87f33355a2..91eba93809 100644 --- a/lib/vdsm/network/link/setup.py +++ b/lib/vdsm/network/link/setup.py @@ -20,50 +20,21 @@ from __future__ import absolute_import from __future__ import division - -class NmstateBridgeOpts: - def __init__(self): - self._types_dict = { - 'mac-ageing-time': int, - 'group-forward-mask': int, - 'hash-max': int, - 'multicast-snooping': self._str_to_bool, - 'multicast-router': int, - 'multicast-last-member-count': int, - 'multicast-last-member-interval': int, - 'multicast-membership-interval': int, - 'multicast-querier': self._str_to_bool, - 'multicast-querier-interval': int, - 'multicast-query-use-ifaddr': self._str_to_bool, - 'multicast-query-interval': int, - 'multicast-query-response-interval': int, - 'multicast-startup-query-count': int, - 'multicast-startup-query-interval': int, - } - - @property - def types_dict(self): - return self._types_dict - - def convert_value(self, key, value): - conversion_func = self._types_dict[key] - return conversion_func(value) - - @staticmethod - def _str_to_bool(str_value): - str_value = str_value.lower() - false_values = ['false', '0'] - return str_value not in false_values - - -class NmstateBridgeOptionNotSupported(Exception): - pass +from vdsm.network.link.bridge import Bridge def parse_bond_options(options): """ Parse bonding options into a dictionary. """ + + def _string_to_dict(str, div, eq): + if options == '': + return {} + return dict( + option.split(eq, 1) for option in str.strip(div).split(div) + ) + if options: d_options = _string_to_dict(options, ' ', '=') return d_options @@ -71,24 +42,16 @@ def parse_bond_options(options): return {} -def parse_nets_bridge_opts(opts): - - if not opts: - return {} - opts_str = opts.replace('_', '-') - bridge_opts = _string_to_dict(opts_str, ' ', '=') - try: - nmstate_bridge_opts = NmstateBridgeOpts() - for key, value in bridge_opts.items(): - bridge_opts[key] = nmstate_bridge_opts.convert_value(key, value) - except KeyError: - raise NmstateBridgeOptionNotSupported( - f'{key} is not a valid nmstate bridge option' - ) - return bridge_opts +def setup_custom_bridge_opts(nets): + for name, opts in parse_nets_bridge_opts(nets): + Bridge(name, opts) -def _string_to_dict(str, div, eq): - if str == '': - return {} - return dict(option.split(eq, 1) for option in str.strip(div).split(div)) +def parse_nets_bridge_opts(nets): + for name, opts in nets.items(): + opts_str = opts.get('custom', {}).get('bridge_opts') + if opts_str: + bridge_opts = dict( + opt.split('=', 1) for opt in opts_str.split(' ') + ) + yield name, bridge_opts diff --git a/lib/vdsm/network/netswitch/configurator.py b/lib/vdsm/network/netswitch/configurator.py index 8b7e0ef481..10b05191cc 100644 --- a/lib/vdsm/network/netswitch/configurator.py +++ b/lib/vdsm/network/netswitch/configurator.py @@ -32,6 +32,7 @@ from vdsm.network.configurators import qos from vdsm.network.dhcp_monitor import MonitoredItemPool from vdsm.network.link import nic +from vdsm.network.link import setup as link_setup from vdsm.network.link.iface import iface as iface_obj from vdsm.network.netconfpersistence import RunningConfig, Transaction from vdsm.network.netlink import waitfor @@ -106,6 +107,7 @@ def _setup_nmstate(networks, bondings, options, in_rollback): if not bond_attrs.get('remove'): config.setBonding(bond_name, bond_attrs) config.save() + link_setup.setup_custom_bridge_opts(networks) connectivity.check(options) diff --git a/lib/vdsm/network/nmstate/bridge_util.py b/lib/vdsm/network/nmstate/bridge_util.py index 1d2b33a8cd..8ce5431dda 100644 --- a/lib/vdsm/network/nmstate/bridge_util.py +++ b/lib/vdsm/network/nmstate/bridge_util.py @@ -40,7 +40,6 @@ def __init__(self, name, attrs): self.bridged = attrs.get('bridged') self.stp = attrs.get('stp') self.mtu = attrs.get('mtu', DEFAULT_MTU) - self.bridge_opts = attrs.get('custom', {}).get('bridge_opts') self.ipv4addr = attrs.get('ipaddr') self.ipv4netmask = attrs.get('netmask') diff --git a/lib/vdsm/network/nmstate/linux_bridge.py b/lib/vdsm/network/nmstate/linux_bridge.py index 4f28849891..d7e6311c02 100644 --- a/lib/vdsm/network/nmstate/linux_bridge.py +++ b/lib/vdsm/network/nmstate/linux_bridge.py @@ -19,8 +19,6 @@ from collections import defaultdict -from vdsm.network.link.setup import parse_nets_bridge_opts - from .bridge_util import DEFAULT_MTU from .bridge_util import is_iface_up from .bridge_util import is_default_mtu @@ -213,14 +211,11 @@ def _create_bridge_iface(self, port, options=None): return bridge_state def _create_bridge_options(self): - opts = parse_nets_bridge_opts(self._netconf.bridge_opts) - birdge_opts_dict = { + return { LinuxBridge.STP_SUBTREE: { LinuxBridge.STP.ENABLED: self._netconf.stp } } - birdge_opts_dict.update(opts) - return birdge_opts_dict def _add_ip(self, sb_iface, vlan_iface, bridge_iface): ip_addr = IpAddress(self._netconf, self._auto_dns) diff --git a/tests/network/functional/bridge_test.py b/tests/network/functional/bridge_test.py index 150b9e6c6f..608b45cbbd 100644 --- a/tests/network/functional/bridge_test.py +++ b/tests/network/functional/bridge_test.py @@ -70,7 +70,9 @@ def test_add_bridge_with_custom_opts(self, adapter, switch, nic0): NET_ATTRS = { 'nic': nic0, 'switch': switch, - 'custom': {'bridge_opts': 'multicast_snooping=0'}, + 'custom': { + 'bridge_opts': 'multicast_snooping=0 multicast_router=0' + }, } NETCREATE = {NETWORK_NAME: NET_ATTRS} with adapter.setupNetworks(NETCREATE, {}, nftestlib.NOCHK): diff --git a/tests/network/functional/netrestore_test.py b/tests/network/functional/netrestore_test.py index 40fde68f13..9615191b9f 100644 --- a/tests/network/functional/netrestore_test.py +++ b/tests/network/functional/netrestore_test.py @@ -45,8 +45,12 @@ def nic1(): @pytest.mark.legacy_switch class TestRestoreLegacyBridge(object): def test_restore_bridge_with_custom_opts(self, adapter, nic0): - CUSTOM_OPTS1 = {'bridge_opts': 'multicast_snooping=0'} - CUSTOM_OPTS2 = {'bridge_opts': 'multicast_snooping=1'} + CUSTOM_OPTS1 = { + 'bridge_opts': 'multicast_router=0 multicast_snooping=0' + } + CUSTOM_OPTS2 = { + 'bridge_opts': 'multicast_router=0 multicast_snooping=1' + } NETCREATE = { NETWORK_NAME: { 'nic': nic0, diff --git a/tests/network/functional/rollback_test.py b/tests/network/functional/rollback_test.py index 3fc09be6b1..90db4eb81f 100644 --- a/tests/network/functional/rollback_test.py +++ b/tests/network/functional/rollback_test.py @@ -21,7 +21,6 @@ import pytest from vdsm.network import errors as ne -from vdsm.network.link.setup import NmstateBridgeOptionNotSupported from . import netfunctestlib as nftestlib from .netfunctestlib import SetupNetworksError @@ -239,7 +238,7 @@ def test_setup_invalid_bridge_opts_fails(adapter, nic0): 'custom': {'bridge_opts': 'foo=0'}, } - with pytest.raises(NmstateBridgeOptionNotSupported): + with pytest.raises(SetupNetworksError): adapter.setupNetworks({NETWORK_NAME: net_attrs}, {}, NOCHK) adapter.update_netinfo() diff --git a/tests/network/unit/link_setup_test.py b/tests/network/unit/link_setup_test.py index f22a925b78..3197c3cd6f 100644 --- a/tests/network/unit/link_setup_test.py +++ b/tests/network/unit/link_setup_test.py @@ -25,14 +25,21 @@ def test_parse_nets_bridge_opts(): nets = { - 'br1': 'multicast_router=0 multicast_snooping=0', - 'br2': 'multicast_router=1 multicast_snooping=1', + 'br1': { + 'custom': { + 'bridge_opts': 'multicast_router=0 multicast_snooping=0' + } + }, + 'br2': { + 'custom': { + 'bridge_opts': 'multicast_router=1 multicast_snooping=1' + } + }, } expected = { - 'br1': {'multicast-router': 0, 'multicast-snooping': False}, - 'br2': {'multicast-router': 1, 'multicast-snooping': True}, + 'br1': {'multicast_router': '0', 'multicast_snooping': '0'}, + 'br2': {'multicast_router': '1', 'multicast_snooping': '1'}, } - for name, opts in nets.items(): - parsed_opts = linksetup.parse_nets_bridge_opts(opts) - assert expected[name] == parsed_opts + for name, opts in linksetup.parse_nets_bridge_opts(nets): + assert expected[name] == opts