Skip to content

Reduce the number of ifconfig calls made in add_topo #19119

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Reduce the number of ifconfig calls made in add_topo #19119

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8ce39fb
sonic-mgmt: fix excessive ifconfig calls
ccroy-arista Dec 5, 2024
d938a62
Fix E201/E202 python formatting errors
ccroy-arista Jun 23, 2025
e646b47
Replace 'ifconfig -a -s' with 'ip -j addr' in check_bridge.
ccroy-arista Jun 24, 2025
e00fb20
Merge branch 'sonic-net:master' into fix-excess-ifconfigs
ccroy-arista Jun 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 26 additions & 8 deletions ansible/roles/vm_set/library/vm_topology.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ def __init__(self, vm_names, vm_properties, fp_mtu, max_fp_num, topo, worker,
def init(self, vm_set_name, vm_base, duts_fp_ports, duts_name, ptf_exists=True, check_bridge=True):
self.vm_set_name = vm_set_name
self.duts_name = duts_name
self._is_multi_duts = True if len(self.duts_name) > 1 else False

if ptf_exists:
self.pid = VMTopology.get_pid(PTF_NAME_TEMPLATE % vm_set_name)
Expand Down Expand Up @@ -286,13 +287,31 @@ def init(self, vm_set_name, vm_base, duts_fp_ports, duts_name, ptf_exists=True,
self.VMs[k] = v

if check_bridge:
for hostname, attrs in self.VMs.items():
vmname = self.vm_names[self.vm_base_index +
attrs['vm_offset']]
vm_bridges = self.get_vm_bridges(vmname)
if len(attrs['vlans']) > len(vm_bridges):
raise Exception("Wrong vlans parameter for hostname %s, vm %s. Too many vlans. Maximum is %d"
% (hostname, vmname, len(vm_bridges)))
if self._is_multi_duts:
for hostname, attrs in self.VMs.items():
vmname = self.vm_names[self.vm_base_index + attrs['vm_offset']]
vm_bridges = self.get_vm_bridges(vmname)
if len(attrs['vlans']) > len(vm_bridges):
raise Exception("Wrong vlans parameter for hostname %s, vm %s. Too many vlans. Maximum is %d"
% (hostname, vmname, len(vm_bridges)))
# The approach below offers a vast speed improvement for performing check_bridge (particularly for
# topologies with a large number of interfaces, but it does not work for multi-dut approaches such as
# dualtor, hence why the former approach is preserved above for that case.
else:
# Get the intf information in a tabulated format. Discard the header row.
intf_rows = VMTopology.cmd('ifconfig -a -s').split('\n')[1:]
Copy link
Contributor

@saiarcot895 saiarcot895 Jun 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can ip -j addr (JSON output of ip addr) be used? That way, manual string parsing isn't needed, and it also replaces one use of ifconfig with ip.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to use ip -j addr.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saiarcot895 I'm looking into using a sysfs approach with os.listdir, which should be an even more portable and general solution; I will provide an update once I have tested this approach, and if it works well then I'll update the PR as well.

# Keep first column only.
intf_names = [ row.split(' ')[0] for row in intf_rows ]
for hostname, attrs in self.VMs.items():
vmname = self.vm_names[self.vm_base_index + attrs['vm_offset']]
vm_bridge_regx = OVS_FP_BRIDGE_REGEX % vmname
num_intfs = 0
for intf in intf_names:
if re.search(vm_bridge_regx, intf):
num_intfs += 1
if len(attrs['vlans']) > num_intfs:
raise Exception("Wrong vlans parameter for hostname %s, vm %s. Too many vlans. Maximum is %d"
% (hostname, vmname, num_intfs))

self.VM_LINKs = {}
if 'VM_LINKs' in self.topo:
Expand All @@ -304,7 +323,6 @@ def init(self, vm_set_name, vm_base, duts_fp_ports, duts_name, ptf_exists=True,
for k, v in self.topo['OVS_LINKs'].items():
self.OVS_LINKs[k] = v

self._is_multi_duts = True if len(self.duts_name) > 1 else False
# For now distinguish a cable topology since it does not contain any vms and there are two ToR's
self._is_cable = True if len(
self.duts_name) > 1 and 'VMs' not in self.topo else False
Expand Down
Loading