Skip to content

Commit

Permalink
apply: fix is_composite_member
Browse files Browse the repository at this point in the history
This function was always returning False. It was happening because is
expects a dict of dicts as input but (for a long time now I suppose) it
is actually a dict of NetDefinitions.

The function was changed to check if the netdef has a link to a parent
NetDefinition, in which case it will be a member.

Due to this bug, Netplan would not skip renaming interfaces that are
members of bonds and bridges. Although, networkd will still detect and
rename interfaces that had their names changed.
  • Loading branch information
daniloegea committed Aug 14, 2024
1 parent 0f4626d commit 79d1b6a
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 21 deletions.
23 changes: 11 additions & 12 deletions netplan_cli/cli/commands/apply.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
import filecmp
import time

from netplan.netdef import NetDefinition

from .. import utils
from ...configmanager import ConfigManager, ConfigurationError
from ..sriov import apply_sriov_config
Expand Down Expand Up @@ -331,21 +333,19 @@ def command_apply(self, run_generate=True, sync=False, exit_on_error=True, state
utils.nm_bring_interface_up(loopback_connection)

@staticmethod
def is_composite_member(composites, phy):
def is_composite_member(netdef: NetDefinition) -> bool:
"""
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
# Should this be extended to other types of parent interfaces (vlans, vrfs)
bond_link = netdef.links.get('bond')
bridge_link = netdef.links.get('bridge')

is_composite = bool(bond_link) or bool(bridge_link)

return is_composite

@staticmethod
def clear_virtual_links(prev_links, curr_links, devices=[]):
Expand Down Expand Up @@ -381,7 +381,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,7 +390,7 @@ 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):
if NetplanApply.is_composite_member(netdef):
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.
Expand Down
54 changes: 45 additions & 9 deletions tests/cli/test_units.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@
import tempfile

from unittest.mock import patch

from netplan.parser import Parser
from netplan.state import State
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 @@ -43,19 +46,52 @@ def tearDown(self):
shutil.rmtree(self.tmproot)

def test_is_composite_member(self):
res = NetplanApply.is_composite_member([{'br0': {'interfaces': ['eth0']}}], 'eth0')
with open(os.path.join(self.tmproot, 'etc/netplan/test.yaml'), 'w') as f:
f.write('''network:
ethernets:
eth0: {}
eth1: {}
eth2: {}
bonds:
bn0:
interfaces: [eth0]
bridges:
br0:
interfaces: [eth2]
''')

parser = Parser()
state = State()

parser.load_yaml(os.path.join(self.tmproot, 'etc/netplan/test.yaml'))
state.import_parser_results(parser)
netdef = state.ethernets.get('eth0')
res = NetplanApply.is_composite_member(netdef)
self.assertTrue(res)

netdef = state.ethernets.get('eth2')
res = NetplanApply.is_composite_member(netdef)
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)
with open(os.path.join(self.tmproot, 'etc/netplan/test.yaml'), 'w') as f:
f.write('''network:
ethernets:
eth0: {}
eth1: {}
bonds:
bn0:
interfaces: [eth0]
''')

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

parser.load_yaml(os.path.join(self.tmproot, 'etc/netplan/test.yaml'))
state.import_parser_results(parser)
netdef = state.ethernets.get('eth1')
res = NetplanApply.is_composite_member(netdef)
self.assertFalse(res)

@patch('subprocess.check_call')
def test_clear_virtual_links(self, mock):
Expand Down

0 comments on commit 79d1b6a

Please sign in to comment.