Merge pull request #1517 from initramfs/current-fix-bond-members

bonding: T4668: Fix bond members not adding/interface state incorrect

2 files changed, 45 insertions, 23 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 <maintainers@vyos.io>
+# Copyright 2019-2022 VyOS maintainers and contributors <maintainers@vyos.io>
 #
 # 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 7e146f446..21cf204fc 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,83 @@ 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'])
+
+    # Reset config level to interfaces
+    old_level = conf.get_level()
+    conf.set_level(['interfaces'])
+
     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': ''}})
+            if conf.exists([section, 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
+
+    # 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: 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