pseudo-ethernet: T8540: Add anycast-gateway support for EVPN#5156
pseudo-ethernet: T8540: Add anycast-gateway support for EVPN#5156alexandr-san4ez wants to merge 1 commit into
Conversation
alexandr-san4ez
requested review from
dmbaturin,
hedrok,
jestabro,
natali-rs1985,
sarthurdev,
sever-sever and
zdc
and removed request for
a team
alexandr-san4ez
added
current
bp/circinus
|
👍 |
|
❌ Typos detected in PR (1 found)
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (6)
📜 Recent review details⏰ 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). (4)
🧰 Additional context used🔍 Remote MCP Context7Additional facts found
Sources:
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds EVPN anycast-gateway for pseudo-ethernet: schema entry, BridgeIf FDB add/del API, MACVLAN source lookup, interface VLAN parser, verification requiring MAC and bridge source, apply-time FDB lifecycle (add/remove), and a smoke test validating commit and FDB behavior. ChangesAnycast Gateway Feature
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Comment |
alexandr-san4ez
force-pushed
the
T8540-current
branch
from
79f0fdb to
8742576
Compare
|
One issue I foresee is if someone disables the bridge interface, then the FDB entry will be purged. When they reenable the bridge interface, there isn't a mechanism to add it back. It'd only be added back if/when the |
It is a good point. I will try to fix that by some approach. |
It’d probably make more sense to make this a normal leafNode within a bridge interface. That would not only solve the issue I stated, but it would eliminate some of the checks that you created, like the hardcoded MAC and source-interface checks. So instead of the |
It is interesting idea. @c-po, @aapostoliuk, @sever-sever what do you think about it? |
alexandr-san4ez
force-pushed
the
T8540-current
branch
from
8742576 to
c7ed21e
Compare
Not sure why we need the word Or Without additional magic words. |
alexandr-san4ez
force-pushed
the
T8540-current
branch
from
c7ed21e to
d82c534
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@smoketest/scripts/cli/test_interfaces_pseudo-ethernet.py`:
- Around line 51-57: The test hardcodes interface names when configuring the
bridge; update the setup to reuse discovered or created interfaces instead of
"eth1" and "vxlan0". Replace the literal strings in the cli_set calls that use
base_bridge_path + ['member', 'interface', 'eth1', ...] and base_bridge_path +
['member', 'interface', 'vxlan0'] with variables populated during test setup
(e.g., use the discovered interface name stored on the test instance like
self.discovered_eth or create and assign a vxlan name to self.vxlan_if), and
ensure cli_set uses those variables (refer to base_bridge_path and self.cli_set
in the diff). Make sure the discovery/creation happens before these cli_set
calls so the test no longer assumes specific interface names.
In `@src/conf_mode/interfaces_pseudo-ethernet.py`:
- Around line 136-190: The FDB entry reconciliation in _apply_anycast_gateway is
only invoked on pseudo-ethernet config changes, so FDBs get lost when a bridge
is re-enabled; extend the bridge lifecycle handling to re-apply anycast FDBs by
invoking the same logic for relevant peths when a Bridge is created/enabled.
Concretely, in the bridge apply/bring-up handler (where BridgeIf(...) instances
are created or the bridge transition to enabled is processed) iterate stored
pseudo-ethernet configs that reference that bridge (inspect
peth['source_interface'] / effective config) and call the existing logic to add
entries (reuse _apply_anycast_gateway semantics or directly call
BridgeIf.add_local_fdb_entry with peth.get('mac') when peth has
'anycast_gateway'), and ensure on bridge delete/disable you still remove old
entries via BridgeIf.del_local_fdb_entry as currently done; this ensures
_apply_anycast_gateway behavior runs on bridge lifecycle events as well as peth
changes.
- Around line 184-190: The code unconditionally calls bridge.add_local_fdb_entry
when anycast_gateway is active, which can re-add an unchanged MAC/source and
cause a failing duplicate FDB entry; modify the block inside the 'if "deleted"
not in peth' to first read the existing values from peth (e.g.
old_mac/old_source = peth.get(...)) and only call
_get_bridge_by_source(new_source) and bridge.add_local_fdb_entry(new_mac) if
either new_mac or new_source differ from the stored old_mac/old_source (and
ensure new_mac/new_source are non-empty), keeping the early check for is_anycast
and the existing helpers (_get_bridge_by_source, bridge.add_local_fdb_entry).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 82c86398-bc21-468b-947f-d4fb256ad2d8
📒 Files selected for processing (5)
interface-definitions/interfaces_pseudo-ethernet.xml.inpython/vyos/ifconfig/bridge.pypython/vyos/utils/network.pysmoketest/scripts/cli/test_interfaces_pseudo-ethernet.pysrc/conf_mode/interfaces_pseudo-ethernet.py
📜 Review details
⏰ 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). (5)
- GitHub Check: build_iso
- GitHub Check: codeql-analysis-call / Analyze (python)
- GitHub Check: add-rebase-label / add-rebase-label
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python code must use Python 3.11 or higher
Files:
smoketest/scripts/cli/test_interfaces_pseudo-ethernet.pypython/vyos/utils/network.pypython/vyos/ifconfig/bridge.pysrc/conf_mode/interfaces_pseudo-ethernet.py
smoketest/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Smoketest scripts must be nose2 compatible and located in smoketest/
Files:
smoketest/scripts/cli/test_interfaces_pseudo-ethernet.py
{**/*.py,data/templates/**/*}
📄 CodeRabbit inference engine (AGENTS.md)
Linting must use ruff 0.6.4, darker, pylint W0611, and Jinja2 lint as enforced by vyos/.github reusables
Files:
smoketest/scripts/cli/test_interfaces_pseudo-ethernet.pypython/vyos/utils/network.pypython/vyos/ifconfig/bridge.pysrc/conf_mode/interfaces_pseudo-ethernet.py
src/conf_mode/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Conf-mode entry-point scripts must be named after CLI components and located in src/conf_mode/
Files:
src/conf_mode/interfaces_pseudo-ethernet.py
🔍 Remote MCP Context7
Now let me search for FDB-related documentation to understand the lifecycle concern better.Based on my research, I now have sufficient information to provide a comprehensive summary of relevant context for this PR review.
Relevant Technical Context for PR #5156 Review
EVPN Anycast Gateway Architecture and FDB Management
In EVPN deployments with anycast gateways, each VTEP must have a local FDB entry for the anycast MAC address to prevent it from going over the overlay. This is achieved via bridge fdb add aa:bb:cc:dd:ee:ff dev br0 self local, where the key differences from non-anycast configurations are that each L2VNI requires a macvlan device using the same MAC address across all VTEPs, and each VTEP must have a local FDB entry.
The distributed anycast gateway feature for EVPN VXLAN is a default gateway addressing mechanism that enables the use of the same gateway IP addresses across all leaf switches in a VXLAN network, ensuring that every leaf switch can function as the default gateway for directly connected workloads while facilitating flexible workload placement, host mobility, and optimal traffic forwarding.
Pseudo-Ethernet Interface Capabilities in VyOS
MACVLAN (pseudo-Ethernet) interfaces operate as logical subinterfaces of standard Ethernet interfaces where each subinterface has a unique MAC address but shares a single physical Ethernet port, allowing users to send packets from different source IPv4 or IPv6 addresses using different MAC addresses. By using pseudo-Ethernet interfaces there is less system overhead compared to traditional bridging, and they can be used to workaround the general limit of 4096 VLANs per physical Ethernet port since that limit applies to a single MAC address.
Bridge FDB Management
VyOS bridge interfaces maintain their FDB with configurable MAC address aging time - the duration in seconds that a MAC address remains in the bridge's FDB before removal if no traffic is received from that address, with a default value of 300 seconds.
Architectural Design Concern: Lifecycle Issue
The PR comments identify a legitimate lifecycle concern: the critical step of adding a local FDB entry (bridge fdb add aa:bb:cc:dd:ee:ff dev br0 self local) is essential to prevent the anycast MAC from going over the overlay. However, when a bridge interface is disabled and then re-enabled, kernel-managed FDB entries may be purged. The current implementation only installs these entries when the pseudo-ethernet interface is modified, meaning re-enabling the bridge without changing the pseudo-ethernet configuration would not restore the FDB entry. This creates an operational gap where manual intervention or pseudo-ethernet modification would be required to restore the anycast gateway functionality after a bridge state change.
Design Alternatives Under Discussion
Commenters suggest two alternative architectural approaches:
- Bridge-level configuration: Moving the anycast-gateway configuration to the bridge interface hierarchy (e.g.,
set interfaces bridge br0 vif 100 anycast-gateway <mac>) to align lifecycle management with the bridge state rather than pseudo-ethernet - Generic FDB naming: Using more generic FDB-related configuration naming (e.g.,
set fdb address <mac> interface br0) that decouples the feature from EVPN-specific terminology, [::web_search::]
🔇 Additional comments (3)
python/vyos/utils/network.py (1)
162-194: LGTM!interface-definitions/interfaces_pseudo-ethernet.xml.in (1)
65-70: LGTM!python/vyos/ifconfig/bridge.py (1)
107-112: LGTM!Also applies to: 281-299
alexandr-san4ez
force-pushed
the
T8540-current
branch
from
d82c534 to
b76b411
Compare
Could be done using proper dependencies, like if peth is a bridge member, add this peth instance as bridge dependency. |
That can definitely be done, but I think the suggestion @sever-sever made with making this a generic FDB entry mechanism makes the most sense. That's all that is really being done here. The ability to statically add FDB entries is missing from VyOS anyways, so you'd be adding utility beyond just By attaching the entry directly to the bridge, Basically,
|
alexandr-san4ez
force-pushed
the
T8540-current
branch
from
b76b411 to
fd2e803
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
smoketest/scripts/cli/test_interfaces_pseudo-ethernet.py (1)
64-67:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCreate or discover the VXLAN bridge member instead of assuming
vxlan{i}exists.Line 66 still binds the test to pre-existing
vxlan{i}interfaces, but this test never creates them. That makes the smoketest topology-dependent and prone to failing on valid environments that only expose ethernet members.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@smoketest/scripts/cli/test_interfaces_pseudo-ethernet.py` around lines 64 - 67, The test currently assumes a member named f'vxlan{i}' exists when calling self.cli_set(base_br_member_path + [f'vxlan{i}']), which makes it topology-dependent; change the code to first discover an existing VXLAN member (e.g., list/inspect current bridge members via the same CLI interface) and use that name if found, otherwise create a new VXLAN member before binding it: use self.cli_set to create the VXLAN interface and then call self.cli_set(base_br_member_path + [vxlan_name]) (referencing the existing symbols self.cli_set, base_br_member_path, base_bridge_path, eth, vlan, and f'vxlan{i}') so the test will work whether or not vxlan{i} pre-exists.src/conf_mode/interfaces_pseudo-ethernet.py (1)
146-190:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReconcile against the current bridge FDB before treating state as converged.
Lines 184-187 only re-add on config deltas. If the local
selfentry disappears whileanycast_gateway,mac, andsource_interfacestay unchanged, apply becomes a no-op and the bridge stays out of sync with config. Keep the effective snapshot for old values, but consult the live FDB before skipping the add path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/conf_mode/interfaces_pseudo-ethernet.py` around lines 146 - 190, The current logic only re-adds the FDB entry on config deltas and misses the case where the local anycast MAC was removed from the live bridge FDB despite no config change; update the "if 'deleted' not in peth" branch to also consult the live bridge FDB and add the entry if it is missing. Concretely: after computing should_add and resolving new_source/new_mac, call _get_bridge_by_source(new_source) and query the bridge for the presence of new_mac (use the bridge's existing FDB-query API, e.g., has/get_local_fdb_entry or equivalent) and treat a missing live entry the same as should_add so bridge.add_local_fdb_entry(new_mac) runs; keep the existing use of effective_config/old_* values and the existing delete cleanup with bridge.del_local_fdb_entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@smoketest/scripts/cli/test_interfaces_pseudo-ethernet.py`:
- Around line 81-90: The test is asserting on a generic "master {br} permanent"
FDB entry but the anycast-gateway feature adds/removes a feature-specific entry
using BridgeIf.add_local_fdb_entry()/del_local_fdb_entry() which creates "self
local" entries; update the two assertions in
smoketest/scripts/cli/test_interfaces_pseudo-ethernet.py (the cmd(...) checks
around the self.cli_delete(self._base_path + [peth, 'anycast-gateway']) /
self.cli_commit() calls) to assert the presence and absence of the MAC entry
with the "self local" flags instead of "master {br} permanent" — either call
bridge -j fdb show dev {br} and validate the JSON entry contains flags
["self","local"] for mac_address, or string-match "self local" in the plain
bridge fdb show output when checking both the before (assertIn) and after
(assertNotIn) conditions.
---
Duplicate comments:
In `@smoketest/scripts/cli/test_interfaces_pseudo-ethernet.py`:
- Around line 64-67: The test currently assumes a member named f'vxlan{i}'
exists when calling self.cli_set(base_br_member_path + [f'vxlan{i}']), which
makes it topology-dependent; change the code to first discover an existing VXLAN
member (e.g., list/inspect current bridge members via the same CLI interface)
and use that name if found, otherwise create a new VXLAN member before binding
it: use self.cli_set to create the VXLAN interface and then call
self.cli_set(base_br_member_path + [vxlan_name]) (referencing the existing
symbols self.cli_set, base_br_member_path, base_bridge_path, eth, vlan, and
f'vxlan{i}') so the test will work whether or not vxlan{i} pre-exists.
In `@src/conf_mode/interfaces_pseudo-ethernet.py`:
- Around line 146-190: The current logic only re-adds the FDB entry on config
deltas and misses the case where the local anycast MAC was removed from the live
bridge FDB despite no config change; update the "if 'deleted' not in peth"
branch to also consult the live bridge FDB and add the entry if it is missing.
Concretely: after computing should_add and resolving new_source/new_mac, call
_get_bridge_by_source(new_source) and query the bridge for the presence of
new_mac (use the bridge's existing FDB-query API, e.g., has/get_local_fdb_entry
or equivalent) and treat a missing live entry the same as should_add so
bridge.add_local_fdb_entry(new_mac) runs; keep the existing use of
effective_config/old_* values and the existing delete cleanup with
bridge.del_local_fdb_entry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: ff99234d-2f63-4e4b-b3d4-dcff477c9cac
📒 Files selected for processing (5)
interface-definitions/interfaces_pseudo-ethernet.xml.inpython/vyos/ifconfig/bridge.pypython/vyos/utils/network.pysmoketest/scripts/cli/test_interfaces_pseudo-ethernet.pysrc/conf_mode/interfaces_pseudo-ethernet.py
📜 Review details
⏰ 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). (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python code must use Python 3.11 or higher
Files:
python/vyos/ifconfig/bridge.pypython/vyos/utils/network.pysmoketest/scripts/cli/test_interfaces_pseudo-ethernet.pysrc/conf_mode/interfaces_pseudo-ethernet.py
{**/*.py,data/templates/**/*}
📄 CodeRabbit inference engine (AGENTS.md)
Linting must use ruff 0.6.4, darker, pylint W0611, and Jinja2 lint as enforced by vyos/.github reusables
Files:
python/vyos/ifconfig/bridge.pypython/vyos/utils/network.pysmoketest/scripts/cli/test_interfaces_pseudo-ethernet.pysrc/conf_mode/interfaces_pseudo-ethernet.py
smoketest/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Smoketest scripts must be nose2 compatible and located in smoketest/
Files:
smoketest/scripts/cli/test_interfaces_pseudo-ethernet.py
src/conf_mode/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Conf-mode entry-point scripts must be named after CLI components and located in src/conf_mode/
Files:
src/conf_mode/interfaces_pseudo-ethernet.py
🧠 Learnings (2)
📚 Learning: 2026-05-26T06:03:59.703Z
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:206-207
Timestamp: 2026-05-26T06:03:59.703Z
Learning: In VyOS smoketests that verify processes running inside a VRF using iproute2, remember that `ip vrf pids <vrf>` outputs one entry per line as `<pid> <process_name>` (e.g., `300431 nginx`), not PIDs alone. Therefore, assertions should check for the presence of the expected process name in the command output (e.g., `assertIn(PROCESS_NAME, cmd(f'ip vrf pids {vrf}'))`) rather than trying to match PID-only output.
Applied to files:
smoketest/scripts/cli/test_interfaces_pseudo-ethernet.py
📚 Learning: 2026-05-26T06:04:29.163Z
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:118-120
Timestamp: 2026-05-26T06:04:29.163Z
Learning: In VyOS smoketest scripts under `smoketest/scripts/cli/`, it is intentional to call `self.cli_delete(['vrf'])` in both `setUpClass` and `tearDown` to wipe the entire VRF subtree and ensure a clean slate. During code review, do not recommend narrowing the delete to specific VRF identifiers or name subsets (e.g., `['vrf', 'name', 'mgmt']`)—the broad teardown behavior is the established project-wide pattern for these tests.
Applied to files:
smoketest/scripts/cli/test_interfaces_pseudo-ethernet.py
🔍 Remote MCP Context7
Additional facts relevant to this PR
-
The bridge FDB is managed via the iproute2 "bridge" utility; commands like
bridge fdb showandbridge fdb add/delare the standard userland mechanism for viewing and modifying the bridge forwarding database (FDB) on Linux systems. -
iproute2
bridge fdbusage and bridge lifecycle operations (create bridge, add port, show FDB) are documented in common iproute2 documentation (examples:bridge fdb show [dev <intf>],ip link add name <bridge> type bridge,ip link set dev <if> master <bridge>). These docs support that the PR’s approach (callingbridge fdb add/bridge fdb delto install/remove local FDB entries on a bridge) aligns with standard tooling and kernel interaction via netlink.
Sources used: Context7 iproute2 documentation and iproute2 examples,
🔇 Additional comments (4)
interface-definitions/interfaces_pseudo-ethernet.xml.in (1)
65-70: LGTM!python/vyos/ifconfig/bridge.py (1)
107-112: LGTM!Also applies to: 281-299
python/vyos/utils/network.py (1)
162-193: LGTM!src/conf_mode/interfaces_pseudo-ethernet.py (1)
84-127: LGTM!
alexandr-san4ez
force-pushed
the
T8540-current
branch
from
fd2e803 to
e08c693
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
smoketest/scripts/cli/test_interfaces_pseudo-ethernet.py (1)
81-92:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAssert only the anycast-managed FDB entry lifecycle.
These checks still key on
master {br} permanent, which is not exclusively controlled byanycast-gateway; this can produce false pass/fail behavior. Assert presence/absence on the feature-ownedselfentry formac_address(preferably viabridge -j fdb show dev {br}and field checks).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@smoketest/scripts/cli/test_interfaces_pseudo-ethernet.py` around lines 81 - 92, The test is asserting on the FDB entry string "master {br} permanent" which is not exclusively controlled by the anycast-gateway flag and can produce false results; update the assertions to verify only the anycast-managed "self" entry for mac_address (e.g. assert that fdb contains '{mac_address} self permanent' before removal and does not contain it after removal) or preferably call the JSON output via cmd(f'bridge -j fdb show dev {br}') and check the entry objects' flags for 'self' rather than relying on "master" text; adjust the checks around cmd(), mac_address, cli_delete(self._base_path + [peth, 'anycast-gateway']) and cli_commit() accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@smoketest/scripts/cli/test_interfaces_pseudo-ethernet.py`:
- Around line 81-92: The test is asserting on the FDB entry string "master {br}
permanent" which is not exclusively controlled by the anycast-gateway flag and
can produce false results; update the assertions to verify only the
anycast-managed "self" entry for mac_address (e.g. assert that fdb contains
'{mac_address} self permanent' before removal and does not contain it after
removal) or preferably call the JSON output via cmd(f'bridge -j fdb show dev
{br}') and check the entry objects' flags for 'self' rather than relying on
"master" text; adjust the checks around cmd(), mac_address,
cli_delete(self._base_path + [peth, 'anycast-gateway']) and cli_commit()
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: ee0327cc-ba62-45b8-90c3-71161a814787
📒 Files selected for processing (5)
interface-definitions/interfaces_pseudo-ethernet.xml.inpython/vyos/ifconfig/bridge.pypython/vyos/utils/network.pysmoketest/scripts/cli/test_interfaces_pseudo-ethernet.pysrc/conf_mode/interfaces_pseudo-ethernet.py
📜 Review details
⏰ 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). (3)
- GitHub Check: build_iso
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python code must use Python 3.11 or higher
Files:
smoketest/scripts/cli/test_interfaces_pseudo-ethernet.pypython/vyos/utils/network.pypython/vyos/ifconfig/bridge.pysrc/conf_mode/interfaces_pseudo-ethernet.py
smoketest/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Smoketest scripts must be nose2 compatible and located in smoketest/
Files:
smoketest/scripts/cli/test_interfaces_pseudo-ethernet.py
{**/*.py,data/templates/**/*}
📄 CodeRabbit inference engine (AGENTS.md)
Linting must use ruff 0.6.4, darker, pylint W0611, and Jinja2 lint as enforced by vyos/.github reusables
Files:
smoketest/scripts/cli/test_interfaces_pseudo-ethernet.pypython/vyos/utils/network.pypython/vyos/ifconfig/bridge.pysrc/conf_mode/interfaces_pseudo-ethernet.py
src/conf_mode/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Conf-mode entry-point scripts must be named after CLI components and located in src/conf_mode/
Files:
src/conf_mode/interfaces_pseudo-ethernet.py
🧠 Learnings (2)
📚 Learning: 2026-05-26T06:03:59.703Z
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:206-207
Timestamp: 2026-05-26T06:03:59.703Z
Learning: In VyOS smoketests that verify processes running inside a VRF using iproute2, remember that `ip vrf pids <vrf>` outputs one entry per line as `<pid> <process_name>` (e.g., `300431 nginx`), not PIDs alone. Therefore, assertions should check for the presence of the expected process name in the command output (e.g., `assertIn(PROCESS_NAME, cmd(f'ip vrf pids {vrf}'))`) rather than trying to match PID-only output.
Applied to files:
smoketest/scripts/cli/test_interfaces_pseudo-ethernet.py
📚 Learning: 2026-05-26T06:04:29.163Z
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:118-120
Timestamp: 2026-05-26T06:04:29.163Z
Learning: In VyOS smoketest scripts under `smoketest/scripts/cli/`, it is intentional to call `self.cli_delete(['vrf'])` in both `setUpClass` and `tearDown` to wipe the entire VRF subtree and ensure a clean slate. During code review, do not recommend narrowing the delete to specific VRF identifiers or name subsets (e.g., `['vrf', 'name', 'mgmt']`)—the broad teardown behavior is the established project-wide pattern for these tests.
Applied to files:
smoketest/scripts/cli/test_interfaces_pseudo-ethernet.py
🔍 Remote MCP Context7
Additional facts relevant to review (concise)
-
The iproute2 bridge utility supports adding/removing FDB entries with the exact flags the PR uses — commands include "bridge fdb add/del ADDR dev DEV [ self ] [ master ] ... [ local | static | dynamic ]" (i.e., "self" and "local/static" options are valid) — see iproute2 bridge fdb usage/source.
-
Documentation/examples show "bridge fdb show [dev ]" is the standard way to inspect the bridge FDB (used by the PR's smoketest and manual steps).
-
The iproute2 source/usage confirms the bridge fdb command supports many lifecycle/option flags (self, master, vlan, etc.), which aligns with the PR adding/removing a "self local" FDB entry on the bridge via userland bridge commands.
Sources/tools used:
- Context7 resolve + docs queries for iproute2 / bridge fdb usage and examples (,).
🔇 Additional comments (4)
interface-definitions/interfaces_pseudo-ethernet.xml.in (1)
65-70: LGTM!python/vyos/ifconfig/bridge.py (1)
107-112: LGTM!Also applies to: 281-299
python/vyos/utils/network.py (1)
162-194: LGTM!src/conf_mode/interfaces_pseudo-ethernet.py (1)
34-37: LGTM!Also applies to: 54-63, 84-112, 127-127, 135-191, 206-206
alexandr-san4ez
force-pushed
the
T8540-current
branch
from
e08c693 to
378c8e4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
python/vyos/ifconfig/macvlan.py (1)
48-50: 💤 Low valueUse
.get('link')for defensive dict access.If
get_interface_configreturns a dict without a'link'key (edge case for non-MACVLAN interfaces), this raisesKeyError. Using.get()maintains consistentNonereturn behavior.def get_source_interface(self): interface_config = get_interface_config(self.ifname) - return interface_config['link'] if interface_config is not None else None + return interface_config.get('link') if interface_config is not None else None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/vyos/ifconfig/macvlan.py` around lines 48 - 50, In get_source_interface, avoid direct dict indexing of interface_config['link'] which can raise KeyError for configs missing 'link'; instead call get_interface_config(self.ifname), check for None, and return interface_config.get('link') so the method defensively returns None when the key is absent (refer to get_source_interface and get_interface_config).src/conf_mode/interfaces_pseudo-ethernet.py (1)
176-179: 💤 Low valueRedundant
if piface:check.
MACVLANIf(peth_ifname, create=False)always returns an instance (neverNone). Theinterface_exists()check on line 175 already guards this path.if interface_exists(peth_ifname): piface = MACVLANIf(peth_ifname, create=False) - if piface: - current_mac = piface.get_mac() - current_source = piface.get_source_interface() + current_mac = piface.get_mac() + current_source = piface.get_source_interface()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/conf_mode/interfaces_pseudo-ethernet.py` around lines 176 - 179, The conditional "if piface:" is redundant because MACVLANIf(peth_ifname, create=False) always returns an instance and interface_exists() already guards the path; remove the if piface: check and unindent the calls to piface.get_mac() and piface.get_source_interface() so they execute directly after piface is assigned (referencing MACVLANIf, piface, get_mac, get_source_interface and the prior interface_exists() check to locate the code).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/conf_mode/interfaces_pseudo-ethernet.py`:
- Line 73: Rename the misspelled function _verify_anycast_getway to
_verify_anycast_gateway and update every call/reference to use the corrected
name (including the call site that currently invokes _verify_anycast_getway).
Search for symbol "_verify_anycast_getway" and replace it with
"_verify_anycast_gateway" in the module and any imports/exports, update any
docstrings/comments/tests that mention the old name, and run the test/lint to
ensure no remaining references.
- Around line 160-166: The current condition always re-adds the FDB when anycast
is enabled; change the logic so BridgeIf.add_local_fdb_entry(new_mac) is only
called when the MAC or source actually changed or when the interface transitions
from non-anycast to anycast. Replace should_add = is_anycast or mac_changed or
source_changed with a condition like should_add = mac_changed or source_changed
or (is_anycast and not previously_anycast), where previously_anycast is derived
from the existing peth/old configuration for the interface; update the code
around the block in src/conf_mode/interfaces_pseudo-ethernet.py that calls
BridgeIf.add_local_fdb_entry to use this new condition to avoid re-adding an
existing FDB entry.
---
Nitpick comments:
In `@python/vyos/ifconfig/macvlan.py`:
- Around line 48-50: In get_source_interface, avoid direct dict indexing of
interface_config['link'] which can raise KeyError for configs missing 'link';
instead call get_interface_config(self.ifname), check for None, and return
interface_config.get('link') so the method defensively returns None when the key
is absent (refer to get_source_interface and get_interface_config).
In `@src/conf_mode/interfaces_pseudo-ethernet.py`:
- Around line 176-179: The conditional "if piface:" is redundant because
MACVLANIf(peth_ifname, create=False) always returns an instance and
interface_exists() already guards the path; remove the if piface: check and
unindent the calls to piface.get_mac() and piface.get_source_interface() so they
execute directly after piface is assigned (referencing MACVLANIf, piface,
get_mac, get_source_interface and the prior interface_exists() check to locate
the code).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e41b2ff5-cf6f-4929-9900-b38e850e4a1a
📒 Files selected for processing (6)
interface-definitions/interfaces_pseudo-ethernet.xml.inpython/vyos/ifconfig/bridge.pypython/vyos/ifconfig/macvlan.pypython/vyos/utils/network.pysmoketest/scripts/cli/test_interfaces_pseudo-ethernet.pysrc/conf_mode/interfaces_pseudo-ethernet.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ansible/ansible(manual)
🚧 Files skipped from review as they are similar to previous changes (3)
- interface-definitions/interfaces_pseudo-ethernet.xml.in
- python/vyos/utils/network.py
- smoketest/scripts/cli/test_interfaces_pseudo-ethernet.py
📜 Review details
⏰ 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). (4)
- GitHub Check: build_iso
- GitHub Check: codeql-analysis-call / Analyze (python)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code must target Python 3.11 or higher
Prefer storing Jinja2 templates as discrete files underdata/templates/rather than inline Python strings
Use ruff (version 0.6.4), darker, pylint W0611, and Jinja2 lint for linting; configuration viaruff.tomlandnose2.cfgat repository root
Files:
python/vyos/ifconfig/macvlan.pypython/vyos/ifconfig/bridge.pysrc/conf_mode/interfaces_pseudo-ethernet.py
src/conf_mode/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Conf-mode entry-point scripts must be named after CLI components and placed in
src/conf_mode/
Files:
src/conf_mode/interfaces_pseudo-ethernet.py
🔍 Remote MCP Context7
Relevant facts for reviewing PR #5156
-
iproute2's bridge fdb command supports add/del with the exact options used by the PR, including the keywords self, local, master, static, dynamic, vlan, etc. The official bridge fdb usage lists: "bridge fdb { add | ... | del } ADDR dev DEV [ self ] [ master ] ... [ local | static | dynamic ]" (from iproute2 source) — confirms the PR's approach of installing/removing a "self local" FDB entry on the parent bridge is supported by the toolchain.
-
The iproute2 documentation also documents bridge fdb show/get/flush variants (e.g., "bridge fdb show [dev ]") which aligns with the smoketest and manual verification steps that inspect bridge FDB state.
Tools used:
- Context7_resolve-library-id to find the iproute2 library entry.
- Context7_query-docs to retrieve iproute2 bridge fdb usage and examples (source: iproute2 bridge/fdb.c).
🔇 Additional comments (7)
python/vyos/ifconfig/bridge.py (2)
107-112: LGTM!
281-299: LGTM!python/vyos/ifconfig/macvlan.py (1)
17-17: LGTM!src/conf_mode/interfaces_pseudo-ethernet.py (4)
74-100: LGTM!
34-36: LGTM!
127-133: LGTM!
146-158: LGTM!
Introduce 'anycast-gateway' leafNode for pseudo-ethernet interfaces. When set, a local FDB entry is installed on the parent bridge to prevent the shared anycast MAC from leaking over the VXLAN overlay.
alexandr-san4ez
force-pushed
the
T8540-current
branch
from
378c8e4 to
e3b5524
Compare
|
CI integration 👍 passed! Details
|
6 participants
Change summary
Introduce
anycast-gatewayoption for pseudo-ethernet interfaces. When set, a local FDB entry is installed on the parent bridge to prevent the shared anycast MAC from leaking over the VXLAN overlay.Types of changes
Related Task(s)
Related PR(s)
How to test / Smoketest result
Manual test
Smoketest
Checklist: