tests: Check if IPv6 MTU change is triggering BGP updates correctly#227
Open
mwinter-osr wants to merge 1 commit intoopensourcerouting:masterfrom
Open
tests: Check if IPv6 MTU change is triggering BGP updates correctly#227mwinter-osr wants to merge 1 commit intoopensourcerouting:masterfrom
mwinter-osr wants to merge 1 commit intoopensourcerouting:masterfrom
Conversation
Test that IPv6 BGP routes are reinstalled after interface MTU is restored above the IPv6 minimum (1280). On Linux, setting MTU < 1280 disables IPv6 on the interface and removes IPv6 kernel routes. When MTU is raised back above 1280, zebra must trigger a RIB re-evaluation so that the BGP routes are reinstalled. Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
Greptile SummaryThis PR adds a new topotest (
|
| Filename | Overview |
|---|---|
| tests/topotests/zebra_mtu_ipv6_change/test_zebra_mtu_ipv6_change.py | New topotest verifying zebra reinstalls IPv6 BGP routes after MTU restoration. Minor issues: unused functools import, unguarded json.loads() in _kernel_route_gone, and the test doesn't validate the BGP RIB state during the MTU-down phase. |
| tests/topotests/zebra_mtu_ipv6_change/r1/frr.conf | iBGP config for r1; no bgp ebgp-requires-policy is a no-op for same-AS sessions and should be removed for clarity. |
| tests/topotests/zebra_mtu_ipv6_change/r2/frr.conf | iBGP config for r2; same spurious no bgp ebgp-requires-policy issue as r1. Otherwise correct: blackhole static route + network statement to originate fdfd::/64. |
| tests/topotests/zebra_mtu_ipv6_change/init.py | Empty init file required by topotest framework; no issues. |
Sequence Diagram
sequenceDiagram
participant r2 as r2 (AS 65000)
participant r1 as r1 (AS 65000)
participant zebra as zebra (r1)
participant kernel as kernel (r1)
r2->>r1: BGP UPDATE IPv6 unicast fdfd::/64 (next-hop: fe80::r2-eth0)
r1->>zebra: install fdfd::/64 via fe80::r2/r1-eth0
zebra->>kernel: ip route add fdfd::/64 dev r1-eth0 proto bgp
Note over kernel: ip link set r1-eth0 mtu 1200
kernel->>zebra: "rtnetlink: MTU < 1280, IPv6 disabled on r1-eth0"
zebra->>kernel: ip route del fdfd::/64 (next-hop unresolvable)
Note over kernel: ip link set r1-eth0 mtu 1500
kernel->>zebra: rtnetlink: MTU restored, IPv6 re-enabled on r1-eth0
zebra->>kernel: ip route add fdfd::/64 dev r1-eth0 proto bgp
Prompt To Fix All With AI
This is a comment left during a code review.
Path: tests/topotests/zebra_mtu_ipv6_change/test_zebra_mtu_ipv6_change.py
Line: 12
Comment:
**Unused import**
`functools` is imported but never referenced anywhere in this test file. Remove it.
```suggestion
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: tests/topotests/zebra_mtu_ipv6_change/r1/frr.conf
Line: 6
Comment:
**`no bgp ebgp-requires-policy` is a no-op for iBGP**
Both r1 and r2 are in AS 65000, so the session is iBGP. `bgp ebgp-requires-policy` only governs eBGP neighbours and has no effect here. The line is harmless but misleading — remove it from both configs for clarity.
```suggestion
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: tests/topotests/zebra_mtu_ipv6_change/test_zebra_mtu_ipv6_change.py
Line: 73-77
Comment:
**`json.loads()` can raise if `ip -j` produces unexpected output**
When IPv6 is fully disabled on `r1-eth0` (MTU < 1280), `ip -6 -j route show fdfd::/64` should return `[]`, but on some kernels or if the subsystem is partially torn down it can emit an error string instead. An uncaught `json.JSONDecodeError` here would crash `run_and_expect` rather than retrying gracefully. Use the same defensive pattern as `_kernel_route_installed` (delegate to `json_cmp`) or wrap in a try/except:
```python
def _kernel_route_gone():
try:
output = json.loads(r1.run("ip -6 -j route show fdfd::/64"))
except json.JSONDecodeError:
return "failed to parse ip -6 -j route output"
if len(output) == 0:
return None
return "fdfd::/64 still in kernel after MTU drop"
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: tests/topotests/zebra_mtu_ipv6_change/test_zebra_mtu_ipv6_change.py
Line: 70-80
Comment:
**Test doesn't verify BGP table withdrawal after MTU drop**
The PR title is "Check if IPv6 MTU change is triggering BGP updates correctly", but the test only checks the kernel FIB — it doesn't verify that FRR's BGP RIB marks `fdfd::/64` as not-installed (or the next-hop as unresolvable) after the MTU drop. A complementary check with `show bgp ipv6 unicast fdfd::/64 json` for `nexthops[0].accessible: false` or `installed: false` would make the test directly validate the BGP→zebra signalling path described in the title, rather than relying solely on kernel state as a proxy.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "tests: Check if IPv6 MTU change is trigg..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| import sys | ||
| import json | ||
| import pytest | ||
| import functools |
There was a problem hiding this comment.
functools is imported but never referenced anywhere in this test file. Remove it.
Suggested change
| import functools |
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/topotests/zebra_mtu_ipv6_change/test_zebra_mtu_ipv6_change.py
Line: 12
Comment:
**Unused import**
`functools` is imported but never referenced anywhere in this test file. Remove it.
```suggestion
```
How can I resolve this? If you propose a fix, please make it concise.| ip address 10.0.0.1/30 | ||
| ! | ||
| router bgp 65000 | ||
| no bgp ebgp-requires-policy |
There was a problem hiding this comment.
no bgp ebgp-requires-policy is a no-op for iBGP
Both r1 and r2 are in AS 65000, so the session is iBGP. bgp ebgp-requires-policy only governs eBGP neighbours and has no effect here. The line is harmless but misleading — remove it from both configs for clarity.
Suggested change
| no bgp ebgp-requires-policy |
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/topotests/zebra_mtu_ipv6_change/r1/frr.conf
Line: 6
Comment:
**`no bgp ebgp-requires-policy` is a no-op for iBGP**
Both r1 and r2 are in AS 65000, so the session is iBGP. `bgp ebgp-requires-policy` only governs eBGP neighbours and has no effect here. The line is harmless but misleading — remove it from both configs for clarity.
```suggestion
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+73
to
+77
| def _kernel_route_gone(): | ||
| output = json.loads(r1.run("ip -6 -j route show fdfd::/64")) | ||
| if len(output) == 0: | ||
| return None | ||
| return "fdfd::/64 still in kernel after MTU drop" |
There was a problem hiding this comment.
json.loads() can raise if ip -j produces unexpected output
When IPv6 is fully disabled on r1-eth0 (MTU < 1280), ip -6 -j route show fdfd::/64 should return [], but on some kernels or if the subsystem is partially torn down it can emit an error string instead. An uncaught json.JSONDecodeError here would crash run_and_expect rather than retrying gracefully. Use the same defensive pattern as _kernel_route_installed (delegate to json_cmp) or wrap in a try/except:
def _kernel_route_gone():
try:
output = json.loads(r1.run("ip -6 -j route show fdfd::/64"))
except json.JSONDecodeError:
return "failed to parse ip -6 -j route output"
if len(output) == 0:
return None
return "fdfd::/64 still in kernel after MTU drop"Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/topotests/zebra_mtu_ipv6_change/test_zebra_mtu_ipv6_change.py
Line: 73-77
Comment:
**`json.loads()` can raise if `ip -j` produces unexpected output**
When IPv6 is fully disabled on `r1-eth0` (MTU < 1280), `ip -6 -j route show fdfd::/64` should return `[]`, but on some kernels or if the subsystem is partially torn down it can emit an error string instead. An uncaught `json.JSONDecodeError` here would crash `run_and_expect` rather than retrying gracefully. Use the same defensive pattern as `_kernel_route_installed` (delegate to `json_cmp`) or wrap in a try/except:
```python
def _kernel_route_gone():
try:
output = json.loads(r1.run("ip -6 -j route show fdfd::/64"))
except json.JSONDecodeError:
return "failed to parse ip -6 -j route output"
if len(output) == 0:
return None
return "fdfd::/64 still in kernel after MTU drop"
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+70
to
+80
| step("Set MTU below IPv6 minimum (1280) — disables IPv6 on the interface") | ||
| r1.run("ip link set r1-eth0 mtu 1200") | ||
|
|
||
| def _kernel_route_gone(): | ||
| output = json.loads(r1.run("ip -6 -j route show fdfd::/64")) | ||
| if len(output) == 0: | ||
| return None | ||
| return "fdfd::/64 still in kernel after MTU drop" | ||
|
|
||
| _, result = topotest.run_and_expect(_kernel_route_gone, None, count=30, wait=1) | ||
| assert result is None, "fdfd::/64 was not removed from kernel after MTU drop" |
There was a problem hiding this comment.
The PR title is "Check if IPv6 MTU change is triggering BGP updates correctly", but the test only checks the kernel FIB — it doesn't verify that FRR's BGP RIB marks fdfd::/64 as not-installed (or the next-hop as unresolvable) after the MTU drop. A complementary check with show bgp ipv6 unicast fdfd::/64 json for nexthops[0].accessible: false or installed: false would make the test directly validate the BGP→zebra signalling path described in the title, rather than relying solely on kernel state as a proxy.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/topotests/zebra_mtu_ipv6_change/test_zebra_mtu_ipv6_change.py
Line: 70-80
Comment:
**Test doesn't verify BGP table withdrawal after MTU drop**
The PR title is "Check if IPv6 MTU change is triggering BGP updates correctly", but the test only checks the kernel FIB — it doesn't verify that FRR's BGP RIB marks `fdfd::/64` as not-installed (or the next-hop as unresolvable) after the MTU drop. A complementary check with `show bgp ipv6 unicast fdfd::/64 json` for `nexthops[0].accessible: false` or `installed: false` would make the test directly validate the BGP→zebra signalling path described in the title, rather than relying solely on kernel state as a proxy.
How can I resolve this? If you propose a fix, please make it concise.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Original PR21500 by ton31337