From 395338fe0692b4ecf8547ab5cf18dd147e07902d Mon Sep 17 00:00:00 2001 From: Christian Breunig Date: Wed, 9 Sep 2026 18:42:24 +0000 Subject: bond: T9269: keep the bond MAC when appending a member interface Commit 6ebb8cd26 ("bond: T7571: fix inconsistent MAC address behaviour") re-assigns the first member's MAC address to the bond, as newer Kernels no longer do so themselves. The flag driving it was initialized unconditionally and an already enslaved member hits "continue" without clearing it, so the step ran again on every append and handed the bond the newly added member's address: bond0 aa:c1:ab:c3:62:48 eth1 aa:c1:ab:98:48:0f eth2 aa:c1:ab:c3:62:48 Only adopt a member MAC while the bond has no member at all, which is what T7571 intended. The added smoketest covers this together with T9269 in leaf_node_changed: it commits one member, appends a second, and verifies both stay enslaved, the pre-existing member stays admin up, and the bond MAC does not move. --- python/vyos/ifconfig/bond.py | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) (limited to 'python') diff --git a/python/vyos/ifconfig/bond.py b/python/vyos/ifconfig/bond.py index 5171f12ab..3b81801c3 100644 --- a/python/vyos/ifconfig/bond.py +++ b/python/vyos/ifconfig/bond.py @@ -481,7 +481,9 @@ class BondIf(Interface): # Add new interfaces to the bond first before removing no longer # required members - this is to ensure that in theory there is always an # active member link - is_first = True + # T7571: Kernel 6.6 assigns a synthetic MAC instead of the first + # member's - adopt it ourselves, but only on an empty bond + adopt_member_mac = not bond_members for interface in dict_search('member.interface', config, default=[]): # Only add interface to bond if it is not already a member if interface in bond_members: @@ -493,15 +495,9 @@ class BondIf(Interface): # configured addresses, so we can safely flush any remaining ones. tmp_if.flush_addrs() - # T7571: This behavior changed from Linux Kernel 5.4 (used in VyOS 1.3) - # to Kernel 6.6 (starting with VyOS 1.4). Previously, the MAC address of - # the first member interface in a bond was adopted as the bond's default MAC. - # In newer versions, a synthetic MAC address is assigned instead. - # - # Re-assign first underlay MAC address to the bond - if is_first: + if adopt_member_mac: self.set_mac(tmp_if.get_mac()) - is_first = False + adopt_member_mac = False # Assign underlying interface to logical bond self.add_port(interface) -- cgit v1.2.3 From aa3f1c548198a1110d67385f19ccc910c5c48be8 Mon Sep 17 00:00:00 2001 From: Christian Breunig Date: Wed, 9 Sep 2026 22:31:18 +0200 Subject: configdict: T9269: no phantom removal when a multi node gains a value A multi node with a single value is a plain string in the JSON rendering of the config tree, and only becomes a list once it holds several values. leaf_node_changed() tested for a string before diffing the lists, so appending a value reported the already configured one as gone. Adding a second bond member thus produced an "interface_remove" entry for the first, and BondIf.update() released it right after enslaving the new one, leaving the bond with the new member only. Normalize to a list and reuse list_diff(). Only the string to list append changes, from [old] to []. interface.py was immune as it re-applies list_diff() to address_old, protocols_static_arp.py was not. --- python/vyos/configdict.py | 7 ++++- src/tests/test_configdict.py | 70 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 1 deletion(-) create mode 100644 src/tests/test_configdict.py (limited to 'python') diff --git a/python/vyos/configdict.py b/python/vyos/configdict.py index ff784251a..da2e9792c 100644 --- a/python/vyos/configdict.py +++ b/python/vyos/configdict.py @@ -133,8 +133,13 @@ def leaf_node_changed(conf, path): return True if old is None: return [] + # A multi node holding a single value is a plain string in the JSON + # rendering of the config tree, and only becomes a list once it holds + # several values. Normalize before diffing, else appending a value to a + # single-valued multi node reports the already configured value as gone + # just because its representation changed from string to list if isinstance(old, str): - return [old] + old = [old] if isinstance(old, list): if isinstance(new, str): new = [new] diff --git a/src/tests/test_configdict.py b/src/tests/test_configdict.py new file mode 100644 index 000000000..5e1aba7c0 --- /dev/null +++ b/src/tests/test_configdict.py @@ -0,0 +1,70 @@ +# Copyright (C) VyOS Inc. +# +# 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 +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . + +from unittest import TestCase +from unittest.mock import patch + +from vyos.configdict import leaf_node_changed + + +class TestLeafNodeChanged(TestCase): + """leaf_node_changed() reports the old value(s) of an altered leaf node. + + get_value_diff() reads the JSON rendering of the config tree, where a + multi node holding a single value is a plain string and only becomes a + list once it holds several values. A caller deleting whatever comes back + must therefore never see a value that is still configured. + """ + + def _changed(self, new, old): + # leaf_node_changed() imports get_config_diff() at call time + with patch('vyos.configdiff.get_config_diff') as get_diff: + get_diff.return_value.get_value_diff.return_value = (new, old) + return leaf_node_changed(None, ['some', 'path']) + + def test_unchanged(self): + self.assertIsNone(self._changed('eth1', 'eth1')) + self.assertIsNone(self._changed(['eth1', 'eth2'], ['eth1', 'eth2'])) + + def test_value_replaced(self): + self.assertEqual(self._changed('eth2', 'eth1'), ['eth1']) + + def test_value_deleted(self): + self.assertEqual(self._changed(None, 'eth1'), ['eth1']) + self.assertEqual(self._changed(None, ['eth1', 'eth2']), ['eth1', 'eth2']) + + def test_value_added(self): + self.assertEqual(self._changed('eth1', None), []) + + def test_valueless_node(self): + self.assertTrue(self._changed(None, {})) + self.assertTrue(self._changed({}, None)) + + def test_appended_to_single_valued_multi_node(self): + # T9269: the single configured value renders as a string while the two + # configured values render as a list. Nothing was removed here - adding + # a second bond member used to release the first one because this + # returned ['eth1'] + self.assertEqual(self._changed(['eth1', 'eth2'], 'eth1'), []) + + def test_appended_to_multi_valued_multi_node(self): + self.assertEqual(self._changed(['eth1', 'eth2', 'eth3'], ['eth1', 'eth2']), []) + + def test_replaced_in_multi_node(self): + self.assertEqual(self._changed(['eth2', 'eth3'], 'eth1'), ['eth1']) + self.assertEqual(self._changed(['eth1', 'eth3'], ['eth1', 'eth2']), ['eth2']) + + def test_shrunk_to_single_value(self): + # and the reverse representation change: list back to plain string + self.assertEqual(self._changed('eth1', ['eth1', 'eth2']), ['eth2']) -- cgit v1.2.3