Skip to content

Conversation

@rsafrono
Copy link
Collaborator

@rsafrono rsafrono commented Nov 9, 2025

Since commits 4d5fa1c and 4853d4d grout is running in it's own namespace.
When tests are using hardware ports we should ensure that the ports are accessible from that namespace.

Summary by CodeRabbit

  • New Features

    • Optional hardware-port mode that can be enabled to use physical network ports (disabled by default).
  • Improvements

    • Network interfaces are now moved into isolated network namespaces when hardware-port mode is enabled.
    • Centralized orchestration for hardware-port handling for more predictable behavior.
    • Cleanup now reliably restores interfaces into the correct namespace.
    • Test initialization refined for consistent FRR-related test behavior.

@coderabbitai
Copy link

coderabbitai bot commented Nov 9, 2025

📝 Walkthrough

Walkthrough

Modifies smoke/_init.sh to add a use_hardware_ports flag (default false) and initialize it when NET_INTERFACES and VFIO_PCI_PORTS are set and have equal lengths. If running inside the grout network namespace and the flag is true, interfaces listed in NET_INTERFACES are moved into the grout namespace before re-execution. Hardware-port hookup is centralized earlier; cleanup now restores interfaces via nsenter. test_frr is initialized to false.

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: enabling hardware ports to work within the grout namespace, which is the core objective of the PR.

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 90dd337 and 086bce2.

📒 Files selected for processing (1)
  • smoke/_init.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.sh
🧠 Learnings (2)
📓 Common learnings
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.169Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.
📚 Learning: 2025-11-05T13:55:26.169Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.169Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.

Applied to files:

  • smoke/_init.sh
🪛 Shellcheck (0.11.0)
smoke/_init.sh

[warning] 11-11: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.

(SC2206)


[warning] 12-12: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.

(SC2206)


[error] 23-23: Double quote array expansions to avoid re-splitting elements.

(SC2068)

⏰ 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)
  • GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
  • GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
  • GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
  • GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
  • GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
  • GitHub Check: deb
  • GitHub Check: rpm
🔇 Additional comments (5)
smoke/_init.sh (5)

6-18: Initialization logic is sound.

The flag setup correctly validates that NET_INTERFACES and VFIO_PCI_PORTS have matching lengths before enabling hardware port usage. Error messaging is clear.


22-26: Interface migration logic is correct.

Interfaces are properly moved to the grout namespace before script re-execution. The conditional ensures this only happens when hardware ports are in use.


128-149: Hardware vs. virtual port handling is well-designed.

The port_add function correctly branches on hardware port usage. The altname cleanup (line 137) shows thoughtful handling of namespace deletion edge cases. Counter management is consistent across both paths.


44-106: Namespace restoration via nsenter is properly sequenced.

Using nsenter -t 1 -n to restore interfaces in the root namespace is the correct approach. The file existence check with -s guards against attempts to restore when hardware ports weren't used.


30-30: Default initializations and conditional flags are consistent.

The test_frr=false default and the conditional -t TAP flag for grout both align with the hardware port orchestration. Logic is sound.

Also applies to: 194-196


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@rsafrono rsafrono force-pushed the use_hw_ports_in_grout_ns branch from 90dd337 to 086bce2 Compare November 9, 2025 20:53
@rsafrono rsafrono marked this pull request as draft November 9, 2025 21:18
Since commits 4d5fa1c and 4853d4d grout is running in it's own
namespace. When tests are using hardware ports we should ensure
that the ports are accessible from that namespace.

Signed-off-by: Roman Safronov <[email protected]>
@rsafrono rsafrono force-pushed the use_hw_ports_in_grout_ns branch from 086bce2 to 472982d Compare November 10, 2025 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant