smoke: Add BGP test case #366
Conversation
|
You should look at using json outputs. |
smoke/images/frrbgp/Dockerfile
Outdated
| FROM quay.io/frrouting/frr:10.2.1 | ||
|
|
||
| COPY daemons /etc/frr/daemons | ||
| COPY vtysh.conf /etc/frr/vtysh.conf | ||
| COPY frr.conf /etc/frr/frr.conf |
There was a problem hiding this comment.
Do we really need to use a container ?
FRR provides the capability to run multiple instances in parallel with the -N option:
-N, --pathspace Insert prefix into config & socket paths
Also, I'd rather use the existing FRR version (which is based on 10.4.X by the way, so we'll be able to configure SRv6 in the future).
There was a problem hiding this comment.
I get that adding a technology for a single test might not be ideal.
I updated the test case to leverage the -N parameters and to run the FRR BGP peer in a separate network namespace.
zeeke
force-pushed
the
frr-bgp-smoke-test
branch
3 times, most recently
from
61f6bad to
5df1bcd
Compare
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds FRR/Grout smoke-test infrastructure: updates smoke/_init.sh to change tmp permissions, append a cleanup banner, and enable bgpd before starting FRR; introduces smoke/_init_frr.sh with a new public function start_frr_on_namespace() to create a namespace, prepare per-namespace FRR files, start FRR there, and poll for bgpd; adds smoke/bgp_frr_test.sh to build a multi-namespace BGP test topology and validate route exchange and connectivity; provides a daemons file enabling zebra/bgpd/staticd and an frr.conf with extensive debug settings. Pre-merge checks✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
smoke/_init.sh(1 hunks)smoke/_init_frr.sh(1 hunks)smoke/bgp_frr_test.sh(1 hunks)smoke/frr-bgp-peer/daemons(1 hunks)smoke/frr-bgp-peer/frr.conf(1 hunks)smoke/frr-bgp-peer/vtysh.conf(1 hunks)subprojects/packagefiles/frr/meson-add-dependency-definition.patch(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/bgp_frr_test.shsmoke/_init_frr.shsmoke/_init.sh
🧬 Code graph analysis (2)
smoke/bgp_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)start_frr_on_namespace(238-264)smoke/_init.sh (1)
netns_add(96-104)
smoke/_init_frr.sh (1)
smoke/_init.sh (1)
netns_add(96-104)
🪛 Shellcheck (0.11.0)
smoke/bgp_frr_test.sh
[warning] 34-34: Quote this to prevent word splitting.
(SC2046)
[warning] 63-63: tmp is referenced but not assigned.
(SC2154)
smoke/_init_frr.sh
[warning] 243-243: builddir is referenced but not assigned.
(SC2154)
[warning] 246-246: Quote this to prevent word splitting.
(SC2046)
[warning] 247-247: Quote this to prevent word splitting.
(SC2046)
[warning] 248-248: Quote this to prevent word splitting.
(SC2046)
[warning] 252-252: tmp 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). (6)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, 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 (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: rpm
🔇 Additional comments (9)
subprojects/packagefiles/frr/meson-add-dependency-definition.patch (1)
55-56: LGTM!The sed command correctly enables bgpd in the daemons configuration file.
smoke/frr-bgp-peer/vtysh.conf (1)
1-2: LGTM!Valid placeholder configuration file with appropriate license header.
smoke/_init.sh (1)
141-141: LGTM!Adding a cleanup message improves observability during test teardown.
smoke/frr-bgp-peer/frr.conf (1)
1-23: LGTM!Comprehensive debugging configuration is appropriate for a test environment to aid troubleshooting.
smoke/frr-bgp-peer/daemons (1)
1-39: LGTM!Daemon configuration correctly enables required services for BGP testing with appropriate localhost binding.
smoke/bgp_frr_test.sh (4)
59-66: Verify veth pair cleanup works correctly.The veth pair is created in ns-a namespace (line 59) and one end is moved to frr-bgp-peer namespace (line 78), but the cleanup tries to delete from the default namespace. Verify this cleanup approach works when veth ends are in different namespaces.
141-144: Verify loopback route configuration is correct.The routes added to gr-loop0 specify "via" addresses that are already configured on the same interface (172.16.0.1/32 and 16.1.0.1/32). Confirm this routing setup correctly supports the BGP neighbor relationship configured on line 131 where neighbor 172.16.0.2 uses "interface gr-loop0".
146-155: LGTM!The wait loop with timeout correctly verifies BGP route learning with appropriate error handling.
175-177: LGTM!Bidirectional connectivity verification between hosts validates the end-to-end BGP route exchange functionality.
rjarry
left a comment
rjarry
left a comment
There was a problem hiding this comment.
Typo in the first commit title:
smoke: echo a marker in clenaup pahse
^^ ^^
smoke/_init.sh
Outdated
| fi | ||
|
|
||
| cat >> $tmp/cleanup <<EOF | ||
| echo "Cleaning up resources" |
There was a problem hiding this comment.
You could even be more explicit:
echo ================== CLEANUP ==================
smoke/bgp_frr_test.sh
Outdated
| # This test leverages the following architecture to test the BGP route exchange between an FRR+Grout setup and a pure FRR BGP peer. | ||
| # At the end of the configuration phase, Host-A and Host-B must be able to ping each other, as the respective default gateway routers | ||
| # have learned the correct routes. |
There was a problem hiding this comment.
Can you limit line length to 100 characters?
smoke/frr-bgp-peer/vtysh.conf
Outdated
| # SPDX-License-Identifier: BSD-3-Clause | ||
| # Copyright (c) 2025 Andrea Panattoni No newline at end of file |
There was a problem hiding this comment.
no, I can use touch. fixing
|
There is a suspicious error in the logs: https://github.com/DPDK/grout/actions/runs/18849993173/job/53783949694?pr=366#step:10:3553 |
smoke/frr-bgp-peer/daemons
Outdated
|
|
||
| zebra=yes | ||
| bgpd=yes | ||
| ospfd=yes |
There was a problem hiding this comment.
maybe a typo, but ospfd isn't built in subprojects/packagefiles/frr/meson-add-dependency-definition.patch
smoke/frr-bgp-peer/daemons
Outdated
| ripngd=no | ||
| isisd=no | ||
| pimd=no | ||
| ldpd=yes |
There was a problem hiding this comment.
Same, ldpd isn't built.
smoke/frr-bgp-peer/daemons
Outdated
| sharpd=no | ||
| staticd=yes | ||
| pbrd=no | ||
| bfdd=yes |
There was a problem hiding this comment.
idem bfdd isn't built.
zeeke
force-pushed
the
frr-bgp-smoke-test
branch
2 times, most recently
from
d7764dd to
aeb25a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
smoke/_init.sh(2 hunks)smoke/_init_frr.sh(1 hunks)smoke/bgp_frr_test.sh(1 hunks)smoke/frr-bgp-peer/daemons(1 hunks)smoke/frr-bgp-peer/frr.conf(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/_init.shsmoke/bgp_frr_test.shsmoke/_init_frr.sh
🧬 Code graph analysis (2)
smoke/bgp_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)start_frr_on_namespace(238-264)smoke/_init.sh (1)
netns_add(96-104)
smoke/_init_frr.sh (1)
smoke/_init.sh (1)
netns_add(96-104)
🪛 Shellcheck (0.11.0)
smoke/bgp_frr_test.sh
[warning] 35-35: Quote this to prevent word splitting.
(SC2046)
[warning] 79-79: tmp is referenced but not assigned.
(SC2154)
smoke/_init_frr.sh
[warning] 243-243: builddir is referenced but not assigned.
(SC2154)
[warning] 246-246: Quote this to prevent word splitting.
(SC2046)
[warning] 247-247: Quote this to prevent word splitting.
(SC2046)
[warning] 252-252: tmp 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). (2)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
🔇 Additional comments (10)
smoke/_init.sh (1)
141-141: LGTM - Cleanup banner is clear.The explicit banner improves test output readability.
smoke/_init_frr.sh (1)
237-264: Function logic is sound.The namespace initialization, FRR setup, and bgpd polling are correctly implemented. The 5-second timeout provides reasonable startup tolerance.
Note: All namespaces share the same frr-bgp-peer configuration. This appears intentional for the test scenario, but consider parameterizing the config source if future tests require namespace-specific configurations.
smoke/frr-bgp-peer/frr.conf (1)
1-20: LGTM - Debug configuration appropriate for testing.The extensive debug directives will provide valuable diagnostic output for troubleshooting BGP peering issues in the smoke test environment.
smoke/frr-bgp-peer/daemons (1)
1-39: LGTM - Minimal daemon configuration for BGP testing.Only essential daemons (zebra, bgpd, staticd) are enabled, which is appropriate for the focused BGP test scenario.
smoke/bgp_frr_test.sh (6)
1-36: LGTM - Excellent documentation.The ASCII topology diagram clearly explains the test architecture, making the subsequent configuration steps easy to follow.
37-52: Interface and Host-B setup is correct.The sequence properly creates interfaces, moves p1 into the ns-b namespace, and configures Host-B networking before establishing IP addressing.
54-74: FRR peer and Host-A setup follows topology correctly.The veth pair creation and namespace assignments match the documented architecture, properly isolating Host-A and the FRR BGP peer.
76-99: BGP peer configuration is correct.AS 43 properly peers with the Grout instance (AS 44) and advertises the Host-A network. Policy relaxations are appropriate for the test environment.
101-141: Grout BGP configuration and loopback setup are correct.The BGP peering (AS 44 ↔ AS 43) is properly configured. The
ip-transparentandinterface gr-loop0directives accommodate Grout's BGP packet handling, and the loopback routes ensure proper connectivity for both the peering and local networks.
142-173: Route validation and connectivity tests are comprehensive.The 10-second BGP convergence timeout is reasonable. The bi-directional ping tests confirm end-to-end functionality. The extensive debugging output (logs, configs, routes, BGP state) will be valuable for troubleshooting failures.
smoke/bgp_frr_test.sh
Outdated
| ip addr add 172.16.0.1/32 dev gr-loop0 | ||
| ip addr add 16.1.0.1/32 dev gr-loop0 | ||
| ip route add 172.16.0.0/24 dev gr-loop0 via 172.16.0.1 | ||
| ip route add 16.1.0.0/24 dev gr-loop0 via 16.1.0.1 |
There was a problem hiding this comment.
ip-transparent should be used for neighbors, instead of setting address manual.
We should not configure route for each network, instead:
ip rule add oif gr-loop0 lookup 100
ip route add default dev gr-loop0 table 100
ip rule add iif gr-loop0 lookup 101
ip route add local default dev gr-loop0 table 101
At the end, this should be done automatically by frr plugin or grout control plane.
There was a problem hiding this comment.
as seen with @christophefontaine I will propose a PR to automatize this directly in zebra.
There was a problem hiding this comment.
ok, so do you prefer to leave this test with the ip addresses for this PR?
Or you want me to update the test to use ip rule ... and ip transparent?
There was a problem hiding this comment.
You can keep it, when I have automatize this, I would update the smoke test in consequence.
There was a problem hiding this comment.
sounds great! thank you for the comment
zeeke
force-pushed
the
frr-bgp-smoke-test
branch
2 times, most recently
from
41ac57c to
6cc79d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
smoke/_init_frr.sh (1)
246-248: Consider validating config file existence.The function copies configuration files but doesn't verify they exist first. If the frr-bgp-peer directory or files are missing, cp will fail with a less clear error.
Add validation before copying:
mkdir -p ${frr_namespace_folder} + if [ ! -f "$(dirname $0)/frr-bgp-peer/daemons" ] || [ ! -f "$(dirname $0)/frr-bgp-peer/frr.conf" ]; then + echo "Required FRR config files not found in frr-bgp-peer/" + exit 1 + fi cp $(dirname $0)/frr-bgp-peer/daemons ${frr_namespace_folder}/daemons cp $(dirname $0)/frr-bgp-peer/frr.conf ${frr_namespace_folder}/frr.conf touch ${frr_namespace_folder}/vtysh.conf
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
smoke/_init.sh(2 hunks)smoke/_init_frr.sh(1 hunks)smoke/bgp_frr_test.sh(1 hunks)smoke/frr-bgp-peer/daemons(1 hunks)smoke/frr-bgp-peer/frr.conf(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- smoke/_init.sh
🧰 Additional context used
📓 Path-based instructions (1)
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/_init_frr.shsmoke/bgp_frr_test.sh
🧬 Code graph analysis (2)
smoke/_init_frr.sh (1)
smoke/_init.sh (1)
netns_add(96-104)
smoke/bgp_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)start_frr_on_namespace(238-264)smoke/_init.sh (1)
netns_add(96-104)
🪛 Shellcheck (0.11.0)
smoke/_init_frr.sh
[warning] 243-243: builddir is referenced but not assigned.
(SC2154)
[warning] 246-246: Quote this to prevent word splitting.
(SC2046)
[warning] 247-247: Quote this to prevent word splitting.
(SC2046)
[warning] 252-252: tmp is referenced but not assigned.
(SC2154)
smoke/bgp_frr_test.sh
[warning] 35-35: Quote this to prevent word splitting.
(SC2046)
[warning] 79-79: tmp 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). (7)
- 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: 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: rpm
- GitHub Check: deb
🔇 Additional comments (8)
smoke/_init_frr.sh (1)
256-263: LGTM with robust polling logic.The bgpd startup polling with a 5-second timeout and clear error message is well-implemented.
smoke/frr-bgp-peer/frr.conf (1)
1-20: LGTM - clean FRR configuration.The configuration appropriately enables debugging for the test environment. The FRR syntax is correct.
smoke/frr-bgp-peer/daemons (1)
22-39: Options for disabled daemons are defined but harmless.Lines 25-26, 28-39 define options for daemons that are disabled. FRR will ignore these, so this doesn't cause issues. This may be intentional for template consistency.
smoke/bgp_frr_test.sh (5)
37-52: LGTM - clear interface and namespace setup.The sequence of creating interfaces, namespaces, and configuring Host-B is logical and well-structured.
76-99: LGTM - comprehensive BGP peer configuration.The FRR BGP peer configuration correctly sets up interfaces, routing, and BGP parameters with appropriate debugging enabled.
102-133: LGTM - thorough Grout FRR configuration.The BGP configuration for the Grout instance includes proper neighbor setup, update-source specification, and network advertisement.
141-150: Good route learning verification with timeout.The 10-second polling loop with clear error message ensures BGP routes are properly exchanged before proceeding.
170-172: LGTM - bidirectional connectivity verification.Testing ping in both directions (Host-A to Host-B and vice versa) properly validates the BGP route exchange.
zeeke
force-pushed
the
frr-bgp-smoke-test
branch
from
6cc79d2 to
0b0d356
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
smoke/bgp_frr_test.sh (2)
171-172: Consider using portable ping flags for cross-platform compatibility.The
-nflag (numeric output) is not universally supported. Standard flags-c(count) and-i(interval) work consistently across systems.-ip netns exec ns-a ping -i0.01 -c3 -n 16.1.0.2 -ip netns exec ns-b ping -i0.01 -c3 -n 16.0.0.2 +ip netns exec ns-a ping -i 0.01 -c 3 16.1.0.2 +ip netns exec ns-b ping -i 0.01 -c 3 16.0.0.2
142-150: Enhance error message for route verification timeout.When BGP route learning fails, the generic error message doesn't show what was found. The debug output follows, but early failure diagnosis would improve troubleshooting.
while ! grcli route show | grep -qE "${expected_route_line}"; do if [ "$SECONDS" -ge "10" ]; then - echo "BGP route not learned" + echo "BGP route not learned. Learned routes:" + grcli route show exit 1 fi sleep 1 done
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
smoke/_init.sh(3 hunks)smoke/_init_frr.sh(1 hunks)smoke/bgp_frr_test.sh(1 hunks)smoke/frr-bgp-peer/daemons(1 hunks)smoke/frr-bgp-peer/frr.conf(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- smoke/frr-bgp-peer/frr.conf
🚧 Files skipped from review as they are similar to previous changes (2)
- smoke/_init.sh
- smoke/frr-bgp-peer/daemons
🧰 Additional context used
📓 Path-based instructions (1)
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/bgp_frr_test.shsmoke/_init_frr.sh
🧬 Code graph analysis (2)
smoke/bgp_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)start_frr_on_namespace(238-264)smoke/_init.sh (1)
netns_add(96-104)
smoke/_init_frr.sh (1)
smoke/_init.sh (1)
netns_add(96-104)
🪛 Shellcheck (0.11.0)
smoke/bgp_frr_test.sh
[warning] 35-35: Quote this to prevent word splitting.
(SC2046)
[warning] 79-79: tmp is referenced but not assigned.
(SC2154)
smoke/_init_frr.sh
[warning] 243-243: builddir is referenced but not assigned.
(SC2154)
[warning] 246-246: Quote this to prevent word splitting.
(SC2046)
[warning] 247-247: Quote this to prevent word splitting.
(SC2046)
[warning] 252-252: tmp 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). (7)
- GitHub Check: rpm
- GitHub Check: deb
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- 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 (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
🔇 Additional comments (1)
smoke/_init_frr.sh (1)
237-263: Implementation looks sound.The function correctly creates a namespace, stages FRR config files, starts FRR, and polls for bgpd startup. Previous indentation and typo issues have been addressed.
zeeke
force-pushed
the
frr-bgp-smoke-test
branch
from
0b0d356 to
76f9fb8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
smoke/frr-bgp-peer/daemons (1)
25-39: Optional: Consider removing options for disabled daemons.Lines 25-39 define options for daemons that are disabled. While harmless and following FRR convention, removing options for disabled daemons (ospfd, ospf6d, ripd, etc.) would reduce clutter.
This is purely cosmetic and can be safely deferred or ignored.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
smoke/_init.sh(3 hunks)smoke/_init_frr.sh(1 hunks)smoke/bgp_frr_test.sh(1 hunks)smoke/frr-bgp-peer/daemons(1 hunks)smoke/frr-bgp-peer/frr.conf(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- smoke/frr-bgp-peer/frr.conf
🧰 Additional context used
📓 Path-based instructions (1)
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/_init_frr.shsmoke/bgp_frr_test.shsmoke/_init.sh
🧬 Code graph analysis (2)
smoke/_init_frr.sh (1)
smoke/_init.sh (1)
netns_add(96-104)
smoke/bgp_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)start_frr_on_namespace(238-264)smoke/_init.sh (1)
netns_add(96-104)
🪛 Shellcheck (0.11.0)
smoke/_init_frr.sh
[warning] 243-243: builddir is referenced but not assigned.
(SC2154)
[warning] 246-246: Quote this to prevent word splitting.
(SC2046)
[warning] 247-247: Quote this to prevent word splitting.
(SC2046)
[warning] 252-252: tmp is referenced but not assigned.
(SC2154)
smoke/bgp_frr_test.sh
[warning] 35-35: Quote this to prevent word splitting.
(SC2046)
[warning] 79-79: tmp 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). (6)
- 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-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: rpm
🔇 Additional comments (11)
smoke/_init.sh (3)
132-132: Verify the necessity of world-writable permissions.The 0777 mode grants full read/write/execute permissions to all users. While this may be needed for namespace-isolated FRR daemons to access the directory, confirm this is the minimum required permission level.
Consider whether 0770 or 0775 would suffice if the FRR processes run under a known group.
141-141: LGTM!The cleanup banner improves test output readability.
201-203: LGTM!The bgpd enablement correctly targets the daemons configuration file.
smoke/_init_frr.sh (1)
237-264: LGTM!The function properly creates a namespace-isolated FRR instance with appropriate cleanup and startup verification. The 5-second bgpd polling timeout is reasonable for smoke tests.
smoke/bgp_frr_test.sh (7)
5-32: LGTM!The ASCII diagram clearly documents the test topology and makes the setup easy to understand.
37-52: LGTM!The interface creation and Host-B configuration correctly establishes the network namespace with appropriate addressing and routing.
54-99: LGTM!The FRR BGP peer setup correctly configures AS 43 with neighbor relationship to the Grout router (AS 44) and advertises the Host-A network.
102-133: LGTM!The extensive BGP debugging configuration is appropriate for a smoke test and will aid in troubleshooting failures.
136-139: LGTM!The gr-loop0 configuration establishes the necessary routing. Note that per previous discussion, this manual route setup will eventually be automated in the FRR plugin or control plane.
141-160: LGTM!The comprehensive debug dump with EXIT trap ensures diagnostic information is available regardless of test outcome.
162-176: LGTM!The route verification polling and bidirectional connectivity tests appropriately validate BGP route exchange and end-to-end reachability.
zeeke
force-pushed
the
frr-bgp-smoke-test
branch
from
76f9fb8 to
3e82cff
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
smoke/_init.sh(3 hunks)smoke/_init_frr.sh(1 hunks)smoke/bgp_frr_test.sh(1 hunks)smoke/frr-bgp-peer/daemons(1 hunks)smoke/frr-bgp-peer/frr.conf(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- smoke/_init.sh
- smoke/frr-bgp-peer/daemons
🧰 Additional context used
📓 Path-based instructions (1)
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/bgp_frr_test.shsmoke/_init_frr.sh
🧬 Code graph analysis (2)
smoke/bgp_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)start_frr_on_namespace(238-264)smoke/_init.sh (1)
netns_add(96-104)
smoke/_init_frr.sh (1)
smoke/_init.sh (1)
netns_add(96-104)
🪛 Shellcheck (0.11.0)
smoke/bgp_frr_test.sh
[warning] 35-35: Quote this to prevent word splitting.
(SC2046)
[warning] 79-79: tmp is referenced but not assigned.
(SC2154)
smoke/_init_frr.sh
[warning] 243-243: builddir is referenced but not assigned.
(SC2154)
[warning] 246-246: Quote this to prevent word splitting.
(SC2046)
[warning] 247-247: Quote this to prevent word splitting.
(SC2046)
[warning] 252-252: tmp 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). (7)
- 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-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: deb
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: rpm
🔇 Additional comments (3)
smoke/frr-bgp-peer/frr.conf (1)
1-20: LGTM!The FRR configuration is well-structured with appropriate debug directives for a test scenario.
smoke/_init_frr.sh (1)
237-264: LGTM!The function correctly creates a namespace-scoped FRR instance with proper cleanup registration and startup verification via polling.
smoke/bgp_frr_test.sh (1)
164-178: LGTM!The route polling logic with timeout and bidirectional connectivity verification properly validates the BGP test scenario.
| # Configure Host-B | ||
| ip link set p1 netns ns-b | ||
| ip -n ns-b link set p1 address ba:d0:ca:ca:00:01 | ||
| ip -n ns-b link set p1 up | ||
| ip -n ns-b addr add 16.1.0.2/24 dev p1 | ||
| ip -n ns-b route add default via 16.1.0.1 | ||
| ip -n ns-b addr show | ||
|
|
||
| set_ip_address p1 16.1.0.1/24 | ||
| set_ip_address p0 172.16.0.1/24 |
There was a problem hiding this comment.
🧩 Analysis chain
Verify interface configuration sequence.
Interface p1 is moved to namespace ns-b at line 44, then configured in that namespace (lines 45-48), but line 51 attempts to configure it via set_ip_address which uses vtysh/Grout. Since p1 has already been moved to ns-b, verify this sequence works as intended. Compare with p0 which is configured via set_ip_address (line 52) before being moved to a namespace (line 58).
🏁 Script executed:
#!/bin/bash
# Verify the order of operations for p1 configuration matches patterns elsewhere
# Check if there are other examples of set_ip_address being called after moving interfaces
rg -nP -C3 'ip link set.*netns' smoke/ | rg -A5 -B5 'set_ip_address'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Directly examine the file and search for patterns
# 1. Check if file exists
echo "=== File check ==="
ls -la smoke/bgp_frr_test.sh 2>&1 | head -5
# 2. View lines 43-52 with line numbers
echo "=== Lines 43-52 ==="
sed -n '43,52p' smoke/bgp_frr_test.sh | cat -n
# 3. Find set_ip_address function definition
echo "=== set_ip_address function ==="
rg -n 'set_ip_address\s*\(' smoke/ -A5 | head -20
# 4. Find all set_ip_address calls with context
echo "=== All set_ip_address calls ==="
rg -n 'set_ip_address' smoke/bgp_frr_test.sh -B2 -A2
# 5. Find all namespace moves in file
echo "=== All namespace moves ==="
rg -n 'ip link set.*netns' smoke/bgp_frr_test.sh -B1 -A1Length of output: 1094
🏁 Script executed:
#!/bin/bash
# View the set_ip_address function implementation
echo "=== set_ip_address function ==="
sed -n '25,45p' smoke/_init_frr.sh
# Check if function references vtysh or namespace context
echo -e "\n=== Checking for vtysh usage ==="
rg -n 'vtysh' smoke/_init_frr.sh -B2 -A2
# View more context around lines 43-62 to understand full initialization
echo -e "\n=== Full context 43-62 ==="
sed -n '43,62p' smoke/bgp_frr_test.sh | cat -nLength of output: 2461
Fix p1 interface configuration order.
Interface p1 is moved to namespace ns-b at line 44, then set_ip_address p1 16.1.0.1/24 is called at line 51. Since set_ip_address uses vtysh (which operates in the main FRR context), it cannot configure an interface that's already in a different namespace. This conflicts with p0's pattern, where set_ip_address p0 172.16.0.1/24 (line 52) is called before p0 is moved to its namespace (line 58). The p1 call at line 51 will fail or be ineffective. Either move set_ip_address p1 16.1.0.1/24 before the namespace move (line 44), or remove it if manual namespace configuration suffices.
🤖 Prompt for AI Agents
In smoke/bgp_frr_test.sh around lines 43–52, the script moves interface p1 into
namespace ns-b (line 44) and only later calls set_ip_address p1 16.1.0.1/24
(line 51) which uses vtysh in the main FRR context and thus cannot configure an
interface that has already been moved; either relocate the set_ip_address p1
16.1.0.1/24 call to before the ip link set p1 netns ns-b line (matching the p0
pattern), or remove the set_ip_address call and rely on the namespace-local
configuration already applied, ensuring the FRR/vtysh configuration runs when
the interface is still in the main namespace.
dfffa06 to
249e497
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
smoke/bgp_frr_test.sh (1)
43-58: Fix interface configuration order for p1 — critical correctness issue.At line 48,
p1is moved to namespacens-b. Then at line 51,set_ip_address p1 16.1.0.1/24is called, which usesvtyshrunning in the main namespace—this cannot configure an interface that's no longer in the main namespace.In contrast,
p0follows the correct pattern:set_ip_address p0(line 52) is called beforep0is moved to a namespace (line 58).Move line 51 (
set_ip_address p1 16.1.0.1/24) to before line 48 so the interface is configured while still in the main namespace:create_interface p1 mac f0:0d:ac:dc:00:01 netns_add ns-a netns_add ns-b -# Configure Host-B -ip link set p1 netns ns-b +set_ip_address p1 16.1.0.1/24 +set_ip_address p0 172.16.0.1/24 + +# Configure Host-B +ip link set p1 netns ns-b ip -n ns-b link set p1 address ba:d0:ca:ca:00:01 ip -n ns-b link set p1 up ip -n ns-b addr add 16.1.0.2/24 dev p1 ip -n ns-b route add default via 16.1.0.1 ip -n ns-b addr show -set_ip_address p1 16.1.0.1/24 -set_ip_address p0 172.16.0.1/24 # Create and start an FRR instance for the BGP peer
🧹 Nitpick comments (2)
smoke/bgp_frr_test.sh (2)
142-162: Extract dump_test_info outside of test flow.The
dump_test_info()function is defined inline but called only on error (line 176). Move the function definition earlier (e.g., after line 135) so the test structure is clearer—setup, test, cleanup flow.
164-174: Polling timeout asymmetry.The BGP route exchange poll waits up to 60 seconds (line 168), while
start_frr_on_namespace()in_init_frr.shwaits only 5 seconds for bgpd to start. If the FRR daemon on the peer is slow to stabilize, the 60-second wait could be masking upstream startup latency. Consider reducing the route poll timeout or clarifying in a comment why such a long grace period is needed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
smoke/_init.sh(3 hunks)smoke/_init_frr.sh(1 hunks)smoke/bgp_frr_test.sh(1 hunks)smoke/frr-bgp-peer/daemons(1 hunks)smoke/frr-bgp-peer/frr.conf(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- smoke/frr-bgp-peer/daemons
- smoke/frr-bgp-peer/frr.conf
- smoke/_init.sh
🧰 Additional context used
📓 Path-based instructions (1)
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/bgp_frr_test.shsmoke/_init_frr.sh
🧬 Code graph analysis (2)
smoke/bgp_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)start_frr_on_namespace(238-264)smoke/_init.sh (1)
netns_add(96-104)
smoke/_init_frr.sh (1)
smoke/_init.sh (1)
netns_add(96-104)
🪛 Shellcheck (0.11.0)
smoke/bgp_frr_test.sh
[warning] 35-35: Quote this to prevent word splitting.
(SC2046)
[warning] 79-79: tmp is referenced but not assigned.
(SC2154)
smoke/_init_frr.sh
[warning] 243-243: builddir is referenced but not assigned.
(SC2154)
[warning] 246-246: Quote this to prevent word splitting.
(SC2046)
[warning] 247-247: Quote this to prevent word splitting.
(SC2046)
[warning] 252-252: tmp 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). (7)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- 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 (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: deb
- GitHub Check: rpm
🔇 Additional comments (1)
smoke/_init_frr.sh (1)
237-264: LGTM — prior issues addressed.The new
start_frr_on_namespace()function looks sound. Prior review comments (indentation and typo) have been corrected. The static analysis warnings forbuilddirandtmpare expected—these are globals provided by the test framework (consistent with usage in_init.sh).
smoke/_init_frr.sh
Outdated
| cp $(dirname $0)/frr-bgp-peer/frr.conf ${frr_namespace_folder}/frr.conf | ||
| touch ${frr_namespace_folder}/vtysh.conf | ||
|
|
||
| ip netns exec $namespace frrinit.sh start $namespace |
There was a problem hiding this comment.
sed -i 's/#watchfrr_options=.*/watchfrr_options="--netns=${frr_namespace_folder}"/' /etc/frr/${frr_namespace_folder}/daemons
frrinit.sh start linux_frr
I am using to start on a different namespace.
I don't need to use ip netns exec $namespace with frrinit.sh.
zeeke
force-pushed
the
frr-bgp-smoke-test
branch
from
249e497 to
b43aad6
Compare
|
@christophefontaine @rjarry I had to work on 3d59872 to make the test work on CI. The problem is that when grout creates the TAP interface I can provide more details (tcpdump, grout traces, ...) if needed |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
modules/infra/control/loopback.c(2 hunks)smoke/_init.sh(3 hunks)smoke/_init_frr.sh(1 hunks)smoke/bgp_frr_test.sh(1 hunks)smoke/frr-bgp-peer/daemons(1 hunks)smoke/frr-bgp-peer/frr.conf(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- smoke/_init.sh
- smoke/frr-bgp-peer/daemons
🧰 Additional context used
📓 Path-based instructions (2)
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/_init_frr.shsmoke/bgp_frr_test.sh
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Don't suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/infra/control/loopback.c
🧠 Learnings (1)
📚 Learning: 2025-08-27T15:33:22.299Z
Learnt from: rjarry
Repo: DPDK/grout PR: 305
File: modules/ip6/control/route.c:407-413
Timestamp: 2025-08-27T15:33:22.299Z
Learning: DPDK rte_rib6_get_nxt() with RTE_RIB6_GET_NXT_ALL flag does not yield the default route ::/0 if configured. The explicit rte_rib6_lookup_exact() call for the default route is necessary to ensure complete route enumeration.
Applied to files:
smoke/bgp_frr_test.sh
🧬 Code graph analysis (3)
smoke/_init_frr.sh (1)
smoke/_init.sh (2)
netns_add(96-104)fail(87-90)
modules/infra/control/loopback.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
smoke/bgp_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)start_frr_on_namespace(238-267)smoke/_init.sh (2)
netns_add(96-104)fail(87-90)
🪛 Shellcheck (0.11.0)
smoke/_init_frr.sh
[warning] 243-243: builddir is referenced but not assigned.
(SC2154)
[warning] 249-249: Quote this to prevent word splitting.
(SC2046)
[warning] 252-252: Quote this to prevent word splitting.
(SC2046)
[warning] 256-256: tmp is referenced but not assigned.
(SC2154)
smoke/bgp_frr_test.sh
[warning] 35-35: Quote this to prevent word splitting.
(SC2046)
[warning] 78-78: tmp 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-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-and-tests (clang-16, 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 (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: build-cross-aarch64
- GitHub Check: deb
- GitHub Check: rpm
🔇 Additional comments (3)
modules/infra/control/loopback.c (1)
155-162: MAC sync update LGTM.
Keeps the cached loopback MAC in step with the tap source; nice safeguard.smoke/frr-bgp-peer/frr.conf (1)
4-19: Debug config OK.
Good to have verbose zebra/BGP logging for smoke troubleshooting.smoke/_init_frr.sh (1)
237-267: Helper reads well.
Namespace bootstrap plus bgpd readiness probe looks coherent.
| SECONDS=0 | ||
| expected_route_line="16.0.0.0/24[[:space:]]+type=L3.*origin=zebra" | ||
| while ! grcli route show | grep -qE "${expected_route_line}"; do | ||
| if [ "$SECONDS" -ge "10" ]; then | ||
| fail "BGP route not learned in Grout" | ||
| fi | ||
| sleep 0.5 | ||
| done | ||
|
|
||
| expected_frr_bgp_peer_route_line="B>\* 16.1.0.0/24" | ||
| while ! vtysh -N $frr_bgp_peer_namespace -c "show ip route" | grep -q "${expected_frr_bgp_peer_route_line}"; do | ||
| if [ "$SECONDS" -ge "10" ]; then | ||
| fail "BGP route not learned in FRR BGP Peer" | ||
| fi | ||
| sleep 0.5 | ||
| done |
There was a problem hiding this comment.
Reset SECONDS before second wait.
SECONDS keeps counting across both loops, so the second wait can abort as soon as the combined time hits 10 s even if the peer would converge. Reset it to zero before the second loop.
Apply this diff to fix it:
while ! grcli route show | grep -qE "${expected_route_line}"; do
if [ "$SECONDS" -ge "10" ]; then
fail "BGP route not learned in Grout"
fi
sleep 0.5
done
+SECONDS=0
expected_frr_bgp_peer_route_line="B>* 16.1.0.0/24"
while ! vtysh -N $frr_bgp_peer_namespace -c "show ip route" | grep -q "${expected_frr_bgp_peer_route_line}"; do📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SECONDS=0 | |
| expected_route_line="16.0.0.0/24[[:space:]]+type=L3.*origin=zebra" | |
| while ! grcli route show | grep -qE "${expected_route_line}"; do | |
| if [ "$SECONDS" -ge "10" ]; then | |
| fail "BGP route not learned in Grout" | |
| fi | |
| sleep 0.5 | |
| done | |
| expected_frr_bgp_peer_route_line="B>\* 16.1.0.0/24" | |
| while ! vtysh -N $frr_bgp_peer_namespace -c "show ip route" | grep -q "${expected_frr_bgp_peer_route_line}"; do | |
| if [ "$SECONDS" -ge "10" ]; then | |
| fail "BGP route not learned in FRR BGP Peer" | |
| fi | |
| sleep 0.5 | |
| done | |
| SECONDS=0 | |
| expected_route_line="16.0.0.0/24[[:space:]]+type=L3.*origin=zebra" | |
| while ! grcli route show | grep -qE "${expected_route_line}"; do | |
| if [ "$SECONDS" -ge "10" ]; then | |
| fail "BGP route not learned in Grout" | |
| fi | |
| sleep 0.5 | |
| done | |
| SECONDS=0 | |
| expected_frr_bgp_peer_route_line="B>\* 16.1.0.0/24" | |
| while ! vtysh -N $frr_bgp_peer_namespace -c "show ip route" | grep -q "${expected_frr_bgp_peer_route_line}"; do | |
| if [ "$SECONDS" -ge "10" ]; then | |
| fail "BGP route not learned in FRR BGP Peer" | |
| fi | |
| sleep 0.5 | |
| done |
🤖 Prompt for AI Agents
In smoke/bgp_frr_test.sh around lines 173 to 188, the script reuses the SECONDS
timer from the first wait loop so the second route-wait can time out
prematurely; reset SECONDS=0 immediately before starting the second while loop
so the FRR BGP peer wait uses a fresh timer, keeping the same timeout logic and
sleep behavior.
modules/infra/control/loopback.c
Outdated
| if (!rte_is_same_ether_addr(ð->src_addr, &lo->mac)) { | ||
| LOG(NOTICE, | ||
| "src_addr(" ETH_F ") differs from interface address(" ETH_F "). updating %s", | ||
| ð->src_addr, | ||
| &lo->mac, | ||
| iface->name); | ||
| loopback_mac_set(iface, ð->src_addr); |
There was a problem hiding this comment.
I am not comfortable with this. If really needed we should subscribe to netlink notifications for that network device and update our internal mac address accordingly.
Also, you should NEVER call any log function in the packet reception path. In this case, we are still in the control path so that is acceptable.
In any case, I think the correct fix is to update the mac address upon notification from the kernel or alternatively, disable systemd-networkd for these interfaces.
There was a problem hiding this comment.
Thank you for your guidance. This is the first time I have put my hands on a DPDK application.
I'll drop this commit and I'll try yo disable the systemd-networkd effect as a workaround.
WRT Subscribing to netlink notifications, I'm not that familiar with this codebase, so I'd let someone else pick this work for the moment.
smoke/_init_frr.sh
Outdated
| done | ||
| } | ||
|
|
||
| # start_frr_on_namespace <namespace> starts an instance of FRR using the given pathspace and running in a network namespace with the same name. |
There was a problem hiding this comment.
nitpick, wrap to 80 columns?
zeeke
force-pushed
the
frr-bgp-smoke-test
branch
from
b43aad6 to
c9fb73d
Compare
| # Avoid systemd-networkd to override the gr-loop0 MAC address | ||
| if [ -d /etc/systemd/network/ ]; then | ||
| cat <<EOF >> /etc/systemd/network/10-gr-loop-no-mac-address.link | ||
| [Match] | ||
| OriginalName=gr-loop* | ||
| [Link] | ||
| MACAddressPolicy=none | ||
| EOF | ||
|
|
||
| udevadm control --reload-rules | ||
| fi |
There was a problem hiding this comment.
That seems quite invasive. Also, if we need this, shouldn't this be done in _init.sh? Maybe we could run the tests in a special environment in the GitHub CI to prevent systemd-networkd or NetworkManager from stepping on our toes.
There was a problem hiding this comment.
That seems quite invasive.
I agree, but it's the bare minimum step list to make this work.
I dug a little and I found the actor who changes the MAC address is udev (part of systemd [1]). So, the above configuration worked on the Ubuntu 24.04 CI (systemd-networkd) and my Fedora 42 (NetworkManager). I know it's not an extensive test, but environments without systemd are very few (maybe only containers).
Do you prefer to freeze this work and wait for a good fix on the code side?
There was a problem hiding this comment.
This makes me think we'll have issues with #373 if udev is trying to rename/control these ports. @christophefontaine did you have a look?
@zeeke: do you think we could run the tests in an isolated way to prevent udev from messing things up? If not, then we should do what you propose but in _init.sh for all tests.
There was a problem hiding this comment.
This will need more digging, as udev modifying the Mac address is not expected !
Is there any log I can have a look at ?
Or any steps to reproduce that ?
There was a problem hiding this comment.
Just don't use systemd ;-)
There was a problem hiding this comment.
with my PR request, the gr-loopX re uses the tun driver instead of the tap. There is no mac address in this cases, no issue with udev.
So maybe we should first finish this PR before this one.
Anyway we will have the issue with the new CP port. These ones are tap driver ;-)
There was a problem hiding this comment.
This will need more digging, as udev modifying the Mac address is not expected ! Is there any log I can have a look at ? Or any steps to reproduce that ?
I created a debug PR to show the problem
zeeke#1
From job-logs.txt, interesting lines about udev changes are
2025-11-05T09:20:05.6760576Z Nov 05 09:19:53 runnervmf2e7y systemd-networkd[694]: gr-loop0: Saved hardware address: c6:09:8e:ae:8c:36
2025-11-05T09:20:05.6781965Z Nov 05 09:19:53 runnervmf2e7y (udev-worker)[28149]: gr-loop0: Using "gr-loop0" as stable identifying information
2025-11-05T09:20:05.6782319Z Nov 05 09:19:53 runnervmf2e7y (udev-worker)[28149]: gr-loop0: Applying persistent MAC address: 16:cd:61:c6:8a:b8
...
2025-11-05T09:20:05.6785504Z Nov 05 09:19:53 runnervmf2e7y systemd-networkd[694]: gr-loop0: Hardware address is changed: c6:09:8e:ae:8c:36 → 16:cd:61:c6:8a:b8
and here is where BGP writes to gr-loop0 using the new MAC, while grout uses the old one:
2025-11-05T09:20:05.6025954Z NOTICE: GROUT: [tx gr-loop0] c6:09:8e:ae:8c:36 > c6:09:8e:ae:8c:36 / IP 172.16.0.2 > 172.16.0.1 ttl=255 proto=TCP(6) / TCP 179 > 42881 flags=SA, (pkt_len=74)
2025-11-05T09:20:05.6026681Z NOTICE: GROUT: [rx gr-loop0] 16:cd:61:c6:8a:b8 > c6:09:8e:ae:8c:36 / IP 172.16.0.1 > 172.16.0.2 ttl=1 proto=TCP(6) / TCP 42881 > 179 flags=S, (pkt_len=74)
There was a problem hiding this comment.
@zeeke: do you think we could run the tests in an isolated way to prevent udev from messing things up? If not, then we should do what you propose but in _init.sh for all tests.
@rjarry I run these tests in a Docker container, where there is no systemd, and we can make /etc as dirty as we want. But making it the default way (or the only way) to run smoke-tests might be too drastic.
Populating files like /etc/systemd/network/10-gr-loop-no-mac-address.link on all developers' hosts is ugly as well (I didn't think about it in the first instance, as I was working on a container).
And sooner or later, we have to find a robust solution to this.
Let me know your preference about how to proceed
There was a problem hiding this comment.
Let's take it as-is for now and we'll think about improving the situation. 👍
Echoing a fixed string makes it easier to look at the test output for the failing command. Signed-off-by: Andrea Panattoni <[email protected]> Reviewed-by: Robin Jarry <[email protected]>
FRR daemons run as non-root user Signed-off-by: Andrea Panattoni <[email protected]> Reviewed-by: Robin Jarry <[email protected]>
The test case ensure basic BGP peering feature works. Two routes are exchanged between the FRR+Grout peer and another, FRR only, BGP peer. Add a `start_frr_on_namespace` method to start an FRR instance in an isolated namespace. Signed-off-by: Andrea Panattoni <[email protected]> Reviewed-by: Robin Jarry <[email protected]>
grout-bot
force-pushed
the
frr-bgp-smoke-test
branch
from
c9fb73d to
e723b5b
Compare
5 participants
The test case ensure basic BGP peering feature works.
Two routes are exchanged between the FRR+Grout peer and another,
FRR only, BGP peer.
The network topology of the test is the following:
Summary by CodeRabbit