From 6287cc214be8f506f0656b7ad75143265213d140 Mon Sep 17 00:00:00 2001 From: initramfs Date: Thu, 1 Sep 2022 20:31:00 +0800 Subject: bonding: T4668: refactor configuration mode interface bonding script Refactor interfaces-bonding.py to simplify existing code and to remove potentially bugprone sections in preparation for member add/remove fixes for T4668. --- src/conf_mode/interfaces-bonding.py | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/src/conf_mode/interfaces-bonding.py b/src/conf_mode/interfaces-bonding.py index 7e146f446..8ef3d9990 100755 --- a/src/conf_mode/interfaces-bonding.py +++ b/src/conf_mode/interfaces-bonding.py @@ -1,6 +1,6 @@ #!/usr/bin/env python3 # -# Copyright (C) 2019-2020 VyOS maintainers and contributors +# Copyright (C) 2019-2022 VyOS maintainers and contributors # # This program is free software; you can redistribute it and/or modify # it under the terms of the GNU General Public License version 2 or later as @@ -73,63 +73,62 @@ def get_config(config=None): # To make our own life easier transfor the list of member interfaces # into a dictionary - we will use this to add additional information - # later on for wach member + # later on for each member if 'member' in bond and 'interface' in bond['member']: - # convert list if member interfaces to a dictionary - bond['member']['interface'] = dict.fromkeys( - bond['member']['interface'], {}) + # convert list of member interfaces to a dictionary + bond['member']['interface'] = {k: {} for k in bond['member']['interface']} if 'mode' in bond: bond['mode'] = get_bond_mode(bond['mode']) tmp = leaf_node_changed(conf, base + [ifname, 'mode']) - if tmp: bond.update({'shutdown_required': {}}) + if tmp: bond['shutdown_required'] = {} tmp = leaf_node_changed(conf, base + [ifname, 'lacp-rate']) - if tmp: bond.update({'shutdown_required': {}}) + if tmp: bond['shutdown_required'] = {} # determine which members have been removed interfaces_removed = leaf_node_changed(conf, base + [ifname, 'member', 'interface']) if interfaces_removed: - bond.update({'shutdown_required': {}}) + bond['shutdown_required'] = {} if 'member' not in bond: - bond.update({'member': {}}) + bond['member'] = {} tmp = {} for interface in interfaces_removed: section = Section.section(interface) # this will be 'ethernet' for 'eth0' if conf.exists(['insterfaces', section, interface, 'disable']): - tmp.update({interface : {'disable': ''}}) + tmp[interface] = {'disable': ''} else: - tmp.update({interface : {}}) + tmp[interface] = {} # also present the interfaces to be removed from the bond as dictionary - bond['member'].update({'interface_remove': tmp}) + bond['member']['interface_remove'] = tmp if dict_search('member.interface', bond): for interface, interface_config in bond['member']['interface'].items(): # Check if member interface is already member of another bridge tmp = is_member(conf, interface, 'bridge') - if tmp: bond['member']['interface'][interface].update({'is_bridge_member' : tmp}) + if tmp: interface_config['is_bridge_member'] = tmp # Check if member interface is already member of a bond tmp = is_member(conf, interface, 'bonding') for tmp in is_member(conf, interface, 'bonding'): if bond['ifname'] == tmp: continue - bond['member']['interface'][interface].update({'is_bond_member' : tmp}) + interface_config['is_bond_member'] = tmp # Check if member interface is used as source-interface on another interface tmp = is_source_interface(conf, interface) - if tmp: bond['member']['interface'][interface].update({'is_source_interface' : tmp}) + if tmp: interface_config['is_source_interface'] = tmp # bond members must not have an assigned address tmp = has_address_configured(conf, interface) - if tmp: bond['member']['interface'][interface].update({'has_address' : {}}) + if tmp: interface_config['has_address'] = {} # bond members must not have a VRF attached tmp = has_vrf_configured(conf, interface) - if tmp: bond['member']['interface'][interface].update({'has_vrf' : {}}) + if tmp: interface_config['has_vrf'] = {} return bond -- cgit v1.2.3 From 95e7676aa8ae5b3476b14a334b3815c2ae59f8d6 Mon Sep 17 00:00:00 2001 From: initramfs Date: Thu, 1 Sep 2022 20:51:21 +0800 Subject: bonding: T4668: fix live bonding member add or remove Fixes several bugs around bonding member interface states not matching the committed configuration, including: - Disabled removed interfaces coming back up - Newly added disabled interfaces not staying down - Newly added interfaces not showing up in the bond --- python/vyos/ifconfig/bond.py | 12 +++++++----- src/conf_mode/interfaces-bonding.py | 23 ++++++++++++++++++++++- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/python/vyos/ifconfig/bond.py b/python/vyos/ifconfig/bond.py index 98bf6162b..0edd17055 100644 --- a/python/vyos/ifconfig/bond.py +++ b/python/vyos/ifconfig/bond.py @@ -1,4 +1,4 @@ -# Copyright 2019-2021 VyOS maintainers and contributors +# Copyright 2019-2022 VyOS maintainers and contributors # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -405,10 +405,12 @@ class BondIf(Interface): # Remove ALL bond member interfaces for interface in self.get_slaves(): self.del_port(interface) - # Removing an interface from a bond will always place the underlaying - # physical interface in admin-down state! If physical interface is - # not disabled, re-enable it. - if not dict_search(f'member.interface_remove.{interface}.disable', config): + + # Restore correct interface status based on config + if dict_search(f'member.interface.{interface}.disable', config) is not None or \ + dict_search(f'member.interface_remove.{interface}.disable', config) is not None: + Interface(interface).set_admin_state('down') + else: Interface(interface).set_admin_state('up') # Bonding policy/mode - default value, always present diff --git a/src/conf_mode/interfaces-bonding.py b/src/conf_mode/interfaces-bonding.py index 8ef3d9990..21cf204fc 100755 --- a/src/conf_mode/interfaces-bonding.py +++ b/src/conf_mode/interfaces-bonding.py @@ -89,6 +89,11 @@ def get_config(config=None): # determine which members have been removed interfaces_removed = leaf_node_changed(conf, base + [ifname, 'member', 'interface']) + + # Reset config level to interfaces + old_level = conf.get_level() + conf.set_level(['interfaces']) + if interfaces_removed: bond['shutdown_required'] = {} if 'member' not in bond: @@ -97,7 +102,7 @@ def get_config(config=None): tmp = {} for interface in interfaces_removed: section = Section.section(interface) # this will be 'ethernet' for 'eth0' - if conf.exists(['insterfaces', section, interface, 'disable']): + if conf.exists([section, interface, 'disable']): tmp[interface] = {'disable': ''} else: tmp[interface] = {} @@ -105,8 +110,24 @@ def get_config(config=None): # also present the interfaces to be removed from the bond as dictionary bond['member']['interface_remove'] = tmp + # Restore existing config level + conf.set_level(old_level) + if dict_search('member.interface', bond): for interface, interface_config in bond['member']['interface'].items(): + # Check if member interface is a new member + if not conf.exists_effective(['member', 'interface', interface]): + bond['shutdown_required'] = {} + + # Check if member interface is disabled + conf.set_level(['interfaces']) + + section = Section.section(interface) # this will be 'ethernet' for 'eth0' + if conf.exists([section, interface, 'disable']): + interface_config['disable'] = '' + + conf.set_level(old_level) + # Check if member interface is already member of another bridge tmp = is_member(conf, interface, 'bridge') if tmp: interface_config['is_bridge_member'] = tmp -- cgit v1.2.3