smoke: add option to use real hardware ports #369
Conversation
📝 WalkthroughWalkthroughUpdates Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.sh⚙️ CodeRabbit configuration file
Files:
🪛 Shellcheck (0.11.0)smoke/_init.sh[warning] 25-25: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a. (SC2206) [warning] 26-26: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a. (SC2206) [warning] 138-138: Declare and assign separately to avoid masking return values. (SC2155) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
smoke/ip6_forward_test.sh (1)
36-36: Device naming inconsistency at line 36.Line 36 uses
dev $p(namespace name) while line 32 correctly usesdev $dev(actual interface name). Since$devis moved into namespace$pviaip link set $dev netns $p(line 28), addresses should target the interface name, not the namespace name.- ip -n $p addr add fd00:f00:$n::2/64 dev $p + ip -n $p addr add fd00:f00:$n::2/64 dev $devIn hw mode, $dev is a hardware interface name (e.g., "0000:8a:01.0"); in non-hw mode, $dev is the namespace-relative name. Either way, the moved interface retains its original name inside the namespace.
🧹 Nitpick comments (2)
smoke/ip6_rs_ra_test.sh (2)
18-18: Single-iteration loop is intentional but unconventional.The loop
for n in 1; doexecutes exactly once (SC2043). While this is intentional for this script's logic, using a simple variable assignment (n=1) would be more idiomatic and clearer about intent.
21-21: Consider using if/else instead of && || for better readability.The conditional assignment
[ "$1" == "hw" ] && dev=${net_interfaces[$n]} || dev=$pworks correctly but is less readable than an explicit if/else block. This also guards against potential edge cases if the first assignment fails.- [ "$1" == "hw" ] && dev=${net_interfaces[$n]} || dev=$p + if [ "$1" == "hw" ]; then + dev=${net_interfaces[$n]} + else + dev=$p + fi
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
smoke/_init.sh(2 hunks)smoke/cross_vrf_forward_test.sh(2 hunks)smoke/dnat44_test.sh(2 hunks)smoke/ip6_add_del_test.sh(1 hunks)smoke/ip6_builtin_icmp_test.sh(1 hunks)smoke/ip6_forward_test.sh(1 hunks)smoke/ip6_rs_ra_test.sh(1 hunks)smoke/ip_add_del_test.sh(1 hunks)smoke/ip_builtin_icmp_test.sh(1 hunks)smoke/ip_forward_test.sh(2 hunks)smoke/ipip_encap_test.sh(1 hunks)smoke/nexthop_ageing_test.sh(2 hunks)smoke/snat44_test.sh(2 hunks)smoke/srv6_test.sh(2 hunks)smoke/vlan_forward_test.sh(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/_init.shsmoke/vlan_forward_test.shsmoke/ip_add_del_test.shsmoke/srv6_test.shsmoke/ip6_forward_test.shsmoke/snat44_test.shsmoke/ip6_add_del_test.shsmoke/ip6_builtin_icmp_test.shsmoke/ipip_encap_test.shsmoke/ip_forward_test.shsmoke/cross_vrf_forward_test.shsmoke/nexthop_ageing_test.shsmoke/ip_builtin_icmp_test.shsmoke/ip6_rs_ra_test.shsmoke/dnat44_test.sh
🧬 Code graph analysis (7)
smoke/srv6_test.sh (1)
smoke/_init.sh (1)
netns_add(101-107)
smoke/ip6_forward_test.sh (1)
smoke/_init.sh (1)
netns_add(101-107)
smoke/ip6_builtin_icmp_test.sh (1)
smoke/_init.sh (1)
netns_add(101-107)
smoke/ipip_encap_test.sh (1)
smoke/_init.sh (1)
netns_add(101-107)
smoke/nexthop_ageing_test.sh (1)
smoke/_init.sh (1)
netns_add(101-107)
smoke/ip_builtin_icmp_test.sh (1)
smoke/_init.sh (1)
netns_add(101-107)
smoke/ip6_rs_ra_test.sh (1)
smoke/_init.sh (1)
netns_add(101-107)
🪛 Shellcheck (0.11.0)
smoke/_init.sh
[warning] 23-23: net_interfaces appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 24-24: vfio_pci_ports appears unused. Verify use (or export if used externally).
(SC2034)
smoke/vlan_forward_test.sh
[warning] 13-13: vfio_pci_ports is referenced but not assigned.
(SC2154)
[warning] 29-29: net_interfaces is referenced but not assigned.
(SC2154)
smoke/ip_add_del_test.sh
[warning] 10-10: vfio_pci_ports is referenced but not assigned.
(SC2154)
smoke/srv6_test.sh
[warning] 12-12: vfio_pci_ports is referenced but not assigned.
(SC2154)
[warning] 25-25: net_interfaces is referenced but not assigned.
(SC2154)
smoke/ip6_forward_test.sh
[warning] 7-7: run_id is referenced but not assigned.
(SC2154)
[warning] 11-11: vfio_pci_ports is referenced but not assigned.
(SC2154)
[warning] 27-27: net_interfaces is referenced but not assigned.
(SC2154)
smoke/snat44_test.sh
[warning] 11-11: vfio_pci_ports is referenced but not assigned.
(SC2154)
[warning] 28-28: net_interfaces is referenced but not assigned.
(SC2154)
smoke/ip6_add_del_test.sh
[warning] 10-10: vfio_pci_ports is referenced but not assigned.
(SC2154)
smoke/ip6_builtin_icmp_test.sh
[warning] 11-11: vfio_pci_ports is referenced but not assigned.
(SC2154)
[warning] 24-24: net_interfaces is referenced but not assigned.
(SC2154)
smoke/ipip_encap_test.sh
[warning] 12-12: vfio_pci_ports is referenced but not assigned.
(SC2154)
[warning] 24-24: net_interfaces is referenced but not assigned.
(SC2154)
smoke/ip_forward_test.sh
[warning] 11-11: vfio_pci_ports is referenced but not assigned.
(SC2154)
[warning] 27-27: net_interfaces is referenced but not assigned.
(SC2154)
smoke/cross_vrf_forward_test.sh
[warning] 11-11: vfio_pci_ports is referenced but not assigned.
(SC2154)
[warning] 34-34: net_interfaces is referenced but not assigned.
(SC2154)
smoke/nexthop_ageing_test.sh
[warning] 33-33: vfio_pci_ports is referenced but not assigned.
(SC2154)
[warning] 46-46: net_interfaces is referenced but not assigned.
(SC2154)
smoke/ip_builtin_icmp_test.sh
[warning] 11-11: vfio_pci_ports is referenced but not assigned.
(SC2154)
[warning] 25-25: net_interfaces is referenced but not assigned.
(SC2154)
smoke/ip6_rs_ra_test.sh
[warning] 12-12: vfio_pci_ports is referenced but not assigned.
(SC2154)
[warning] 18-18: This loop will only ever run once. Bad quoting or missing glob/expansion?
(SC2043)
[warning] 21-21: net_interfaces is referenced but not assigned.
(SC2154)
smoke/dnat44_test.sh
[warning] 11-11: vfio_pci_ports is referenced but not assigned.
(SC2154)
[warning] 27-27: net_interfaces is referenced but not assigned.
(SC2154)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-cross-aarch64
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: deb
- GitHub Check: rpm
🔇 Additional comments (15)
smoke/dnat44_test.sh (3)
10-16: Clean hardware/software interface branching.The conditional structure correctly toggles between vfio_pci_ports and net_tap modes. MAC addresses are only set in non-hw mode, which is appropriate.
27-30: Device selection and namespace operations properly handle both modes.The dynamic
devvariable ensures operations target the correct interface whether hardware or TAP-based.
34-43: Address assignment correctly targets the selected device.Logic properly branches to assign addresses to either hardware interfaces or TAP devices via dev0/dev1.
smoke/srv6_test.sh (3)
11-17: Hardware/software branching consistent with established pattern.Interface creation correctly selects vfio_pci_ports or net_tap based on mode, with MACs only in non-hw.
25-29: Device selection and namespace operations follow consistent convention.Dynamic device targeting ensures both hw and non-hw modes work correctly.
31-39: Address assignment properly scoped to selected device.dev0/dev1 variables ensure addresses attach to the correct interface in both modes.
smoke/cross_vrf_forward_test.sh (2)
10-16: VRF support correctly integrated with hardware/software branching.Interface creation properly passes vrf parameter in both modes.
34-38: Device selection and namespace operations consistent.Dynamic dev variable handling maintains pattern across all tests.
smoke/vlan_forward_test.sh (2)
12-18: Hardware/software interface branching clean and consistent.Properly toggles between vfio_pci_ports and net_tap with appropriate MACs.
29-37: VLAN operations correctly target dynamic device.Device selection and VLAN creation properly handle both modes.
smoke/ip6_forward_test.sh (1)
7-16: Hardware mode branching consistent, but verify loop indexing implications.Loop changed from
for n in 1 2tofor n in 0 1with corresponding variable renames. Interface branching follows established pattern.smoke/ip_forward_test.sh (2)
10-16: Hardware/software interface branching clean and consistent.Correctly branches on hw mode to use vfio_pci_ports or net_tap with appropriate MACs.
27-36: Device selection and conditional addressing properly implemented.Loopback vs physical device addressing correctly distinguishes based on n value and properly targets the moved interface via $dev.
smoke/ip6_rs_ra_test.sh (2)
21-21: No issues found. The net_interfaces array is properly initialized in _init.sh viaread -r -a net_interfaces <<< "$NET_INTERFACES". The code correctly accesses the second interface using index 1 (since bash arrays are 0-indexed). The loop only runs with n=1, so the indexing is correct.
11-15: vfio_pci_ports is properly initialized in _init.sh at line 24.Array is correctly assigned from
$VFIO_PCI_PORTSusingread -r -a, making index [1] accessible in the sourced test script. No action required.
rsafrono
force-pushed
the
adjust_smoke_tests_for_real_hw
branch
5 times, most recently
from
f5b28aa to
ba3fcb1
Compare
rsafrono
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
smoke/_init.sh (2)
139-144: Fix SC2155: split declare and assignment of macAvoid masking return values and silence ShellCheck warning.
- local mac=$(echo "$name" | md5sum | sed -E 's/(..)(..)(..)(..)(..).*/02:\1:\2:\3:\4:\5/') + local mac + mac=$(echo "$name" | md5sum | sed -E 's/(..)(..)(..)(..)(..).*/02:\1:\2:\3:\4:\5/')
49-49: Run restoration with -e for safer rollbackFail fast on restore errors; keeps system state consistent.
-[ -s $tmp/restore_interfaces ] && sh -x $tmp/restore_interfaces +[ -s $tmp/restore_interfaces ] && sh -ex $tmp/restore_interfaces
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
smoke/_init.sh(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/_init.sh
🪛 Shellcheck (0.11.0)
smoke/_init.sh
[warning] 142-142: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (1)
smoke/_init.sh (1)
190-192: CLI flag handling looks goodConditional -t only in non-hardware mode and unified -vvx verbosity are consistent with hardware path.
If CI has both modes, please run one hardware and one tap job to confirm grout start flags are as expected in logs.
Also applies to: 195-200
rsafrono
force-pushed
the
adjust_smoke_tests_for_real_hw
branch
from
ba3fcb1 to
4f140e5
Compare
rsafrono
force-pushed
the
adjust_smoke_tests_for_real_hw
branch
from
4f140e5 to
28c1101
Compare
grout-bot
force-pushed
the
adjust_smoke_tests_for_real_hw
branch
from
28c1101 to
22cca2c
Compare
rjarry
left a comment
rjarry
left a comment
There was a problem hiding this comment.
Oh, sorry, as always, Github does not make the commit messages visible. Can you remove the mention to any "PR" in git and wrap to 72 columns:
smoke: add option to use real hardware ports
It is assumed that the host has at least 2 ports bound to kernel driver
and 2 bound to vfio-pci driver.
Hardware ports should be specified as environment variables with
space-separated values, e.g.
export NET_INTERFACES="eno12399v1 eno12409v1"
export VFIO_PCI_PORTS="0000:8a:01.0 0000:8a:11.0"
Signed-off-by: Roman Safronov <[email protected]>
Reviewed-by: Robin Jarry <[email protected]>
It is assumed that the host has at least 2 ports bound to kernel driver
and 2 bound to vfio-pci driver.
Hardware ports should be specified as environment variables with
space-separated values, e.g.
export NET_INTERFACES="eno12399v1 eno12409v1"
export VFIO_PCI_PORTS="0000:8a:01.0 0000:8a:11.0"
Signed-off-by: Roman Safronov <[email protected]>
Reviewed-by: Robin Jarry <[email protected]>
rsafrono
force-pushed
the
adjust_smoke_tests_for_real_hw
branch
from
22cca2c to
b7dcb65
Compare
2 participants
smoke: add option to use real hardware ports
It is assumed that the host has at least 2 ports bound to kernel driver
and 2 bound to vfio-pci driver.
Hardware ports should be specified as environment variables with
space-separated values, e.g.
Signed-off-by: Roman Safronov [email protected]
Reviewed-by: Robin Jarry [email protected]
Summary by CodeRabbit
New Features
Improvements