zebra: fix disable radvd upon interface deletion event#18755
zebra: fix disable radvd upon interface deletion event#18755pguibert6WIND wants to merge 4 commits intoFRRouting:masterfrom
Conversation
ton31337
left a comment
ton31337
left a comment
There was a problem hiding this comment.
LGTM, just not sure why do we need touching MaxRtrAdvInterval?
pguibert6WIND
force-pushed
the
rtadv_interface_remove
branch
2 times, most recently
from
23240a6 to
f0af351
Compare
because zebra_interface_radv_set() ZAPI call does the same thing. |
|
does this PR relate to #18451 ? |
no, here, it deals with interface deletion. |
|
ci:rerun |
| memset(&zif->brslave_info, 0, | ||
| sizeof(struct zebra_l2info_brslave)); | ||
| zebra_evpn_mac_ifp_del(ifp); | ||
| zebra_rtadv_disable_per_interface(zif); |
There was a problem hiding this comment.
wouldn't it be better to fix things in rtadv_if_fini(), which is already being called from interface.c ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
pguibert6WIND
force-pushed
the
rtadv_interface_remove
branch
from
f0af351 to
2bf4edd
Compare
If the radvd service is used on a physical interface, the service is not stopped and may be reused upon re-creation of that physical interface. Fix this by stopping the radvd service. Signed-off-by: Philippe Guibert <[email protected]>
When an interface is removed from the configuration, the configuration is flushed in the rtadv_if_fini() function, but the dynamic part related to bgp registration of radvd is not done. Signed-off-by: Philippe Guibert <[email protected]>
To ease the removal of interfaces during the test, let us ease vlan interfaces. The physical interface that link pe1 and pe2 is replaced by a vlan interface with vlan id 100. Signed-off-by: Philippe Guibert <[email protected]>
Add a test that removes and re-adds the vlan interface, where RADV was enabled. The expectation is that radv from BGP is really disabled at recovery. Signed-off-by: Philippe Guibert <[email protected]>
pguibert6WIND
force-pushed
the
rtadv_interface_remove
branch
from
2bf4edd to
0804384
Compare
|
Added a test that demonstrates this change of code. |
|
This PR is stale because it has been open 180 days with no activity. Comment or remove the |
4 participants
If the radvd service is used on a physical interface, the service is not stopped and may be reused upon re-creation of that physical interface.
Fix this by stopping the radvd service.