Skip to content

Conversation

@rjarry
Copy link
Collaborator

@rjarry rjarry commented Dec 10, 2025

Allow configuring vlans with bonds as parent interfaces.

@coderabbitai
Copy link

coderabbitai bot commented Dec 10, 2025

📝 Walkthrough

Walkthrough

VLAN CLI parsing now accepts BOND as a valid parent type in addition to PORT; other types still return EMEDIUMTYPE. When creating a new VLAN (not updating), MAC assignment is chosen by parent-type: MAC is taken from port info for PORT parents or from bond info for BOND parents. The test suite script adds a bonded interface (bond0 in lacp mode with members p0 and p1), creates VLANs v42 and v43 on bond0, and includes corresponding cleanup steps for v42, v43, bond0, p0, and p1.


📜 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 ffaaad2 and 0d01745.

📒 Files selected for processing (2)
  • modules/infra/cli/vlan.c (2 hunks)
  • smoke/config_test.sh (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{c,h}

⚙️ CodeRabbit configuration file

**/*.{c,h}: - gr_vec_*() functions cannot fail. No need to check their return value.

  • gr_vec_free(x) always sets x = NULL. There is no risk of double free.
  • ec_node_*() functions consume their ec_node arguments. No leaks on error.
  • rte_node->ctx is an uint8_t array of size 16, not a pointer.
  • Never suggest to replace assert() with graceful error checking.
  • We compile with -std=gnu2x. Unnamed parameters in function definitions are valid.

Files:

  • modules/infra/cli/vlan.c
**/*.sh

⚙️ CodeRabbit configuration file

**/*.sh: - Don't bother about unquoted shell variables.

Files:

  • smoke/config_test.sh
🧠 Learnings (3)
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: The grout project uses a modular CLI build system where individual modules contribute their CLI sources to a global cli_src variable via `cli_src += files(...)`, and the top-level meson.build file builds the final grcli executable using all collected CLI sources. The grcli executable is defined in the top-level meson.build at lines 122-127.

Applied to files:

  • modules/infra/cli/vlan.c
📚 Learning: 2025-10-13T08:37:56.819Z
Learnt from: abhiramnarayana
Repo: DPDK/grout PR: 337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.

Applied to files:

  • modules/infra/cli/vlan.c
  • smoke/config_test.sh
📚 Learning: 2025-11-05T13:55:26.189Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
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/config_test.sh
⏰ 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-16, none, debugoptimized, ubuntu-24.04, 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-18, none, debugoptimized, ubuntu-24.04, false)
  • GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
  • GitHub Check: deb
  • GitHub Check: rpm
🔇 Additional comments (4)
modules/infra/cli/vlan.c (2)

66-74: LGTM!

The switch statement correctly validates that the parent interface is either a PORT or a BOND, returning EMEDIUMTYPE for other interface types.


88-106: LGTM!

The MAC extraction logic properly handles both PORT and BOND parent types via the switch statement, with appropriate casts to access the MAC from each interface type's info structure.

smoke/config_test.sh (2)

9-11: LGTM!

Good test coverage for the new VLAN-on-bond feature. Creates a bond with LACP mode and then creates VLANs on top of it.


53-55: LGTM!

Cleanup order is correct: VLANs are deleted before the bond, and the bond is deleted before its member ports.


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.

Copy link
Collaborator

@christophefontaine christophefontaine left a comment

Choose a reason for hiding this comment

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

It may be useful to add a smoke test ?

Allow configuring vlans with bonds as parent interfaces.

Update the config smoke test to ensure it is possible.

Signed-off-by: Robin Jarry <[email protected]>
Reviewed-by: Christophe Fontaine <[email protected]>
@rjarry
Copy link
Collaborator Author

rjarry commented Dec 10, 2025

It may be useful to add a smoke test ?

done, I preserved your reviewed-by tag

@christophefontaine christophefontaine merged commit 672cfdc into DPDK:main Dec 10, 2025
10 checks passed
@rjarry rjarry deleted the vlan-on-bond branch December 10, 2025 14:37
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.

2 participants