Skip to content

Commit

Permalink
apply: don't skip members of bonds on renaming
Browse files Browse the repository at this point in the history
Skipping members of bonds when searching for interfaces that need to
be renamed was done as a workaround for LP: #1802322.

The actual problem was that members of bonds will have their MAC
addresses changed to a common address when they are added as members.
The renaming process looked for interfaces based on their transient MAC
addresses not the permanent one. Because of that, it would confuse one
interface by another and try to rename it.

The process was changed to look for the permanent MAC address so this
problem shouldn't happen. It also searches only for physical interfaces,
which should have a permanent MAC address.

Skipping the renaming of members of bonds will cause the backend to
"forget" about it. Once the configuration is generated with a different
name, the backend will look for the new name. As the interface will
continue to have the previous name, it will not be managed anymore.

Lets fix this by allowing the renaming these interfaces. If the system
administrator changed the "set-name" setting, they probably want to get
the interface renamed. This might cause a quick loss of connectivity on
the interface being renamed.

Drop is_composite_member(). This method is not needed anymore.
  • Loading branch information
daniloegea committed Aug 22, 2024
1 parent 61af3e8 commit 2d32857
Show file tree
Hide file tree
Showing 2 changed files with 1 addition and 38 deletions.
23 changes: 0 additions & 23 deletions netplan_cli/cli/commands/apply.py
Original file line number Diff line number Diff line change
Expand Up @@ -330,23 +330,6 @@ def command_apply(self, run_generate=True, sync=False, exit_on_error=True, state
if 'lo' in nm_interfaces and loopback_connection:
utils.nm_bring_interface_up(loopback_connection)

@staticmethod
def is_composite_member(composites, phy):
"""
Is this physical interface a member of a 'composite' virtual
interface? (bond, bridge)
"""
for composite in composites:
for _, settings in composite.items():
if not type(settings) is dict:
continue
members = settings.get('interfaces', [])
for iface in members:
if iface == phy:
return True

return False

@staticmethod
def clear_virtual_links(prev_links, curr_links, devices=[]):
"""
Expand Down Expand Up @@ -381,7 +364,6 @@ def process_link_changes(interfaces, config_manager: ConfigManager): # pragma:
"""

changes = {}
composite_interfaces = [config_manager.bridges, config_manager.bonds]

# Find physical interfaces which need a rename
# But do not rename virtual interfaces
Expand All @@ -391,11 +373,6 @@ def process_link_changes(interfaces, config_manager: ConfigManager): # pragma:
continue # Skip if no new name needs to be set
if not netdef._has_match:
continue # Skip if no match for current name is given
if NetplanApply.is_composite_member(composite_interfaces, netdef.id):
logging.debug('Skipping composite member {}'.format(netdef.id))
# do not rename members of virtual devices. MAC addresses
# may be the same for all interface members.
continue
# Find current name of the interface, according to match conditions and globs (name, mac, driver)
current_iface_name = utils.find_matching_iface(interfaces, netdef)
if not current_iface_name:
Expand Down
16 changes: 1 addition & 15 deletions tests/cli/test_units.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import tempfile

from unittest.mock import patch

from netplan_cli.cli.commands.apply import NetplanApply
from netplan_cli.cli.commands.try_command import NetplanTry
from netplan_cli.cli.core import Netplan
Expand All @@ -42,21 +43,6 @@ def setUp(self):
def tearDown(self):
shutil.rmtree(self.tmproot)

def test_is_composite_member(self):
res = NetplanApply.is_composite_member([{'br0': {'interfaces': ['eth0']}}], 'eth0')
self.assertTrue(res)

def test_is_composite_member_false(self):
res = NetplanApply.is_composite_member([
{'br0': {'interfaces': ['eth42']}},
{'bond0': {'interfaces': ['eth1']}}
], 'eth0')
self.assertFalse(res)

def test_is_composite_member_with_renderer(self):
res = NetplanApply.is_composite_member([{'renderer': 'networkd', 'br0': {'interfaces': ['eth0']}}], 'eth0')
self.assertTrue(res)

@patch('subprocess.check_call')
def test_clear_virtual_links(self, mock):
# simulate as if 'tun3' would have already been delete another way,
Expand Down

0 comments on commit 2d32857

Please sign in to comment.