Skip to content

zebra: fix disable radvd upon interface deletion event #18755

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
pguibert6WIND wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

zebra: fix disable radvd upon interface deletion event #18755

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
25172c0
zebra: fix disable radvd upon interface deletion event
pguibert6WIND Apr 30, 2025
a6bb1a5
zebra: fix reset bgp configuration on if_delete
pguibert6WIND May 7, 2025
258a8a8
topotests: bgp_vpn_5549_route_map, use vlan interface in backbone
pguibert6WIND May 9, 2025
0804384
topotests: bgp_vpn_5549_route_map, add radv test with suppressed if
pguibert6WIND May 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tests/topotests/bgp_vpn_5549_route_map/pe1/ldpd.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ mpls ldp
address-family ipv4
discovery transport-address 10.10.10.10
!
interface pe1-eth1
interface pe1-eth1.100
!
!
!
2 changes: 1 addition & 1 deletion tests/topotests/bgp_vpn_5549_route_map/pe1/ospf6d.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
interface lo
ipv6 ospf6 area 0
!
interface pe1-eth1
interface pe1-eth1.100
ipv6 ospf6 area 0
ipv6 ospf6 hello-interval 1
ipv6 ospf6 dead-interval 3
Expand Down
2 changes: 1 addition & 1 deletion tests/topotests/bgp_vpn_5549_route_map/pe1/zebra.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ interface lo
interface pe1-eth0 vrf RED
ip address 192.168.1.2/24
!
interface pe1-eth1
interface pe1-eth1.100
ip address 10.0.1.1/24
ipv6 address 2001:db8::1/64
!
Expand Down
2 changes: 1 addition & 1 deletion tests/topotests/bgp_vpn_5549_route_map/pe2/ldpd.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ mpls ldp
address-family ipv4
discovery transport-address 10.10.10.20
!
interface pe2-eth0
interface pe2-eth0.100
!
!
!
2 changes: 1 addition & 1 deletion tests/topotests/bgp_vpn_5549_route_map/pe2/ospf6d.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
interface lo
ipv6 ospf6 area 0
!
interface pe2-eth0
interface pe2-eth0.100
ipv6 ospf6 area 0
ipv6 ospf6 hello-interval 1
ipv6 ospf6 dead-interval 3
Expand Down
2 changes: 1 addition & 1 deletion tests/topotests/bgp_vpn_5549_route_map/pe2/zebra.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ interface lo
interface pe2-eth1 vrf RED
ip address 192.168.2.2/24
!
interface pe2-eth0
interface pe2-eth0.100
ip address 10.0.1.2/24
ipv6 address 2001:db8::2/64
!
Expand Down
63 changes: 55 additions & 8 deletions tests/topotests/bgp_vpn_5549_route_map/test_bgp_vpn_5549_route_map.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,15 @@ def setup_module(mod):
pe1.run("ip link set pe1-eth0 master RED")
pe2.run("ip link set pe2-eth1 master RED")

pe1.run("ip link add link pe1-eth1 name pe1-eth1.100 type vlan id 100")
pe1.run("ip link set dev pe1-eth1.100 up")
pe2.run("ip link add link pe2-eth0 name pe2-eth0.100 type vlan id 100")
pe2.run("ip link set dev pe2-eth0.100 up")

pe1.run("sysctl -w net.ipv4.ip_forward=1")
pe2.run("sysctl -w net.ipv4.ip_forward=1")
pe1.run("sysctl -w net.mpls.conf.pe1-eth0.input=1")
pe2.run("sysctl -w net.mpls.conf.pe2-eth1.input=1")
pe1.run("sysctl -w net.mpls.conf.pe1-eth0.100.input=1")
pe2.run("sysctl -w net.mpls.conf.pe2-eth1.100.input=1")

router_list = tgen.routers()

Expand Down Expand Up @@ -138,15 +143,15 @@ def _bgp_verify_v6_global_nexthop_validity():
"complete": True,
"igpMetric": 0,
"pathCount": 2,
"nexthops": [{"interfaceName": "pe2-eth0"}],
"nexthops": [{"interfaceName": "pe2-eth0.100"}],
},
"2001:db8:1::1": {
"valid": True,
"complete": True,
"igpMetric": 10,
"pathCount": 2,
"peer": "2001:db8:1::1",
"nexthops": [{"interfaceName": "pe2-eth0"}],
"nexthops": [{"interfaceName": "pe2-eth0.100"}],
},
}
}
Expand All @@ -168,7 +173,7 @@ def _bgp_verify_v6_global_nexthop_validity():
def check_show_interface_rtadv_params_found(router):
output = json.loads(router.vtysh_cmd("show interface json"))
expected = {
"pe1-eth1": {
"pe1-eth1.100": {
"ndAdvertisedReachableTimeMsecs": 0,
"ndAdvertisedRetransmitIntervalMsecs": 0,
"ndAdvertisedHopCountLimitHops": 64,
Expand All @@ -194,7 +199,7 @@ def test_show_interface_rtadv_params_found():
def check_show_interface_rtadv_params_not_found(router):
output = json.loads(router.vtysh_cmd("show interface json"))
expected = {
"pe1-eth1": {
"pe1-eth1.100": {
"ndAdvertisedReachableTimeMsecs": 0,
"ndAdvertisedRetransmitIntervalMsecs": 0,
}
Expand Down Expand Up @@ -226,7 +231,7 @@ def test_show_interface_rtadv_params_not_found():
def check_show_interface_rtadv_params_found_reapply(router):
output = json.loads(router.vtysh_cmd("show interface json"))
expected = {
"pe1-eth1": {
"pe1-eth1.100": {
"ndAdvertisedReachableTimeMsecs": 0,
"ndAdvertisedRetransmitIntervalMsecs": 0,
}
Expand Down Expand Up @@ -261,7 +266,7 @@ def check_show_interface_rtadv_params_not_found_after_reapply(router):
output = json.loads(router.vtysh_cmd("show interface json"))

expected = {
"pe1-eth1": {
"pe1-eth1.100": {
"ndAdvertisedReachableTimeMsecs": 0,
"ndAdvertisedRetransmitIntervalMsecs": 0,
}
Expand Down Expand Up @@ -290,6 +295,48 @@ def test_show_interface_rtadv_params_not_found_after_reapply():
assert success, "not good"


def test_reapply_bgp_config():
tgen = get_topogen()

router = tgen.gears["pe1"]
router.run("vtysh -f {0}".format(os.path.join(CWD, "pe1/bgpd.conf")))

test_func = functools.partial(
check_show_interface_rtadv_params_found_reapply, router
)
success, _ = topotest.run_and_expect(test_func, None, count=60, wait=0.5)
assert success, "rtadv output is invalid"


def test_remove_vlan_interface_from_pe1():
tgen = get_topogen()

router = tgen.gears["pe1"]
router.run("ip link delete pe1-eth1.100")
output = router.vtysh_cmd("show interface pe1-eth1.100")
print(output)
test_func = functools.partial(
check_show_interface_rtadv_params_not_found_after_reapply, router
)
success, _ = topotest.run_and_expect(test_func, None, count=60, wait=0.5)
assert success, "not good"


def test_readd_vlan_interface_from_pe1():
tgen = get_topogen()

router = tgen.gears["pe1"]
router.run("ip link add link pe1-eth1 name pe1-eth1.100 type vlan id 100")
router.run("ip link set dev pe1-eth1.100 up")
output = router.vtysh_cmd("show interface pe1-eth1.100")
print(output)
test_func = functools.partial(
check_show_interface_rtadv_params_not_found_after_reapply, router
)
success, _ = topotest.run_and_expect(test_func, None, count=60, wait=0.5)
assert success, "not good"


if __name__ == "__main__":
args = ["-s"] + sys.argv[1:]
sys.exit(pytest.main(args))
1 change: 1 addition & 0 deletions zebra/interface.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -764,6 +764,7 @@ void if_delete_update(struct interface **pifp)
memset(&zif->brslave_info, 0,
sizeof(struct zebra_l2info_brslave));
zebra_evpn_mac_ifp_del(ifp);
zebra_rtadv_disable_per_interface(zif);
Copy link
Contributor

@mjstapp mjstapp May 6, 2025

Choose a reason for hiding this comment

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

wouldn't it be better to fix things in rtadv_if_fini(), which is already being called from interface.c ?

Copy link
Member

@donaldsharp donaldsharp May 6, 2025

Choose a reason for hiding this comment

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

Mark is correct I believe

Copy link
Member Author

@pguibert6WIND pguibert6WIND May 6, 2025

Choose a reason for hiding this comment

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

I dont think so. in my case, the interface has configuration on the interface, so the call to if_delete() is not done, and the config is kept until the interface is restored by the system.
That is why I need a separate call.

Copy link
Contributor

@mjstapp mjstapp May 7, 2025

Choose a reason for hiding this comment

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

so ... should the rtadv disable be moved to this place - is this path called first in all interface-delete events?
This api does a few things, but doesn't do all the things that rtadv_if_fini() does. Is that ... correct? Couldn't "delete/create" look the same all the time, where "delete" means "remove everything" and "create" means "reinitialize everything (and if config exists, use the config)" ? I'd prefer to have one consistent path.

Copy link
Member Author

@pguibert6WIND pguibert6WIND May 7, 2025

Choose a reason for hiding this comment

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

rtadv_if_fini() flushes the rtadv configuration. So you lose the config if the interface is deleted in the system, but maintained in the config.

What I did is only to care about the "dynamic" part, which is not related to "static" config.
What I can do is that the if_delete() action will also flush the dynamic part, which was not the case.

Copy link
Contributor

@mjstapp mjstapp May 7, 2025

Choose a reason for hiding this comment

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

yes, we have trouble with the rtadv code for the up/down transitions, and for the delete/create transitions, so it seems like it would be good to figure out how to make the whole interaction between the interface module events and the rtadv module work more clearly.
what is the pattern for "delete"? if an interface is "deleted", it has ifindex of INTERNAL, but if it has configuration, it will be present in the lib's name db, and the if struct will have some config fields set - is that correct? do all the daemons do something similar for their per-daemon data - retain their own object attached to the if struct?

Copy link
Member Author

@pguibert6WIND pguibert6WIND May 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

the solution is northbound config for radvd. I did not check, but I think the config should be removed from the interface in any case, provided that the config is saved in yang backend. this is a different work from what I try to address.

}

if (!ifp->configured) {
Expand Down
28 changes: 16 additions & 12 deletions zebra/rtadv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1560,18 +1560,8 @@ static void zebra_interface_radv_set(ZAPI_HANDLER_ARGS, int enable)
&& !CHECK_FLAG(zif->rtadv.ra_configured,
VTY_RA_INTERVAL_CONFIGURED))
zif->rtadv.MaxRtrAdvInterval = ra_interval * 1000;
} else {
if (CHECK_FLAG(zif->rtadv.ra_configured, BGP_RA_CONFIGURED))
interfaces_configured_for_ra_from_bgp--;

UNSET_FLAG(zif->rtadv.ra_configured, BGP_RA_CONFIGURED);
if (!CHECK_FLAG(zif->rtadv.ra_configured,
VTY_RA_INTERVAL_CONFIGURED))
zif->rtadv.MaxRtrAdvInterval =
RTADV_MAX_RTR_ADV_INTERVAL;
if (!CHECK_FLAG(zif->rtadv.ra_configured, VTY_RA_CONFIGURED))
ipv6_nd_suppress_ra_set(ifp, RA_SUPPRESS);
}
} else
zebra_rtadv_disable_per_interface(zif);
stream_failure:
return;
}
Expand Down Expand Up @@ -1626,6 +1616,18 @@ void rtadv_stop_ra_all(void)
}
}

void zebra_rtadv_disable_per_interface(struct zebra_if *zif)
{
if (CHECK_FLAG(zif->rtadv.ra_configured, BGP_RA_CONFIGURED)) {
interfaces_configured_for_ra_from_bgp--;
UNSET_FLAG(zif->rtadv.ra_configured, BGP_RA_CONFIGURED);
}
if (!CHECK_FLAG(zif->rtadv.ra_configured, VTY_RA_INTERVAL_CONFIGURED))
zif->rtadv.MaxRtrAdvInterval = RTADV_MAX_RTR_ADV_INTERVAL;
if (!CHECK_FLAG(zif->rtadv.ra_configured, VTY_RA_CONFIGURED))
ipv6_nd_suppress_ra_set(zif->ifp, RA_SUPPRESS);
}

void zebra_interface_radv_disable(ZAPI_HANDLER_ARGS)
{
zebra_interface_radv_set(client, hdr, msg, zvrf, 0);
Expand Down Expand Up @@ -2068,6 +2070,8 @@ void rtadv_if_fini(struct zebra_if *zif)
struct rtadv_prefix *rp;
struct pref64_adv *pref64_adv;

zebra_rtadv_disable_per_interface(zif);

rtadv = &zif->rtadv;

while ((rp = rtadv_prefixes_pop(rtadv->prefixes)))
Expand Down
1 change: 1 addition & 0 deletions zebra/rtadv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,7 @@ static inline void rtadv_stop_ra_all(void)

extern void zebra_interface_radv_disable(ZAPI_HANDLER_ARGS);
extern void zebra_interface_radv_enable(ZAPI_HANDLER_ARGS);
extern void zebra_rtadv_disable_per_interface(struct zebra_if *zif);

extern uint32_t rtadv_get_interfaces_configured_from_bgp(void);
extern bool rtadv_compiled_in(void);
Expand Down
Loading