zebra: V6 RA not sent anymore after interface up-down-up by soumyar-roy · Pull Request #18451 · FRRouting/frr

soumyar-roy · 2025-03-21T20:29:58Z

zebra: V6 RA not sent anymore after interface up-down-up

Issue:
Once interface is shutdown, the interface is removed from
wheel timer. Now when the interface is up again, current code
won't add the interface to wheel timer again, so it won't send RA
anymore for that interface

Fix:
Moved wheel_add for interface inside rtadv_start_interface_events
This is more common function which gets triggered for both
RA enable and interface up event

Also on any kind of interface activation event, we try to send
RA as soon as possible. This is to satisfy requirement where
quick RA is needed, especially for some convergence, dependent on
RA.

Testing:
Did ineterface up to down to up
Added debug log for RA, checked it is getting advertised preodically
after when up at up state

show bgp summary for 512 bgp peers for bgp bgp unnumbered works fine.

Signed-off-by: Soumya Roy souroy@nvidia.com

mjstapp · 2025-03-21T20:46:54Z

so ... please add a meaningful title and description?

soumyar-roy · 2025-03-21T22:26:44Z

so ... please add a meaningful title and description?

Added now

lib/wheel.c

ton31337 · 2025-03-23T10:04:31Z

lib/wheel.c

+	    list_isempty(wheel->wheel_slot_lists[curr_slot])) {
+		/* Came to back to same slot and that is empty
+		 * so the wheel is empty, puase it
+		*/


Can you fix the comment indentation?

1)This comment is indented w.r.t if (!wheel->run_forever) {. before running git clang-format >>
((((curr_slot + slots_to_skip) % wheel->slots) == curr_slot) &&

list_isempty(wheel->wheel_slot_lists[curr_slot])) {<<<This line is tab indented

/* Came to back to same slot and that is empty

* so the wheel is empty, stop it

*/

if (!wheel->run_forever) {

wheel_stop(wheel);

if (debug_timer_wheel)

zlog_debug("Stopped an empty wheel %p", wheel);

return;

}

}

2)After git clang-format >>
if ((((curr_slot + slots_to_skip) % wheel->slots) == curr_slot) &&

list_isempty(wheel->wheel_slot_lists[curr_slot])) {

list_isempty(wheel->wheel_slot_lists[curr_slot])) {<<<<<This line gets space indented /* Came to back to same slot and that is empty * so the wheel is empty, stop it */

If I dont do step 2) I get style suggestion error curl https://gist.githubusercontent.com/polychaeta/13d7c1b3f9c07b87352be22b5f29ad01/raw/55bb8b7724008c107d333a05c8db9a785a2db0f7/style.diff | git apply -. So restoring back to 1)

no: line 56 is not correct. it looks like it's missing a space to align the comment block

ton31337 · 2025-03-23T10:10:15Z

Can we have a bit of the context when fast/regular wheels are used?

soumyar-roy · 2025-03-23T18:28:58Z

Can we have a bit of the context when fast/regular wheels are used?

Added more context

ton31337 · 2025-03-24T12:21:38Z

Thanks, makes sense now, but please put it inside the commit (not in PR).

mjstapp · 2025-04-16T12:22:45Z

zebra/rtadv.c

-	if (adv_if != NULL)
+	if (adv_if != NULL) {
+		rtadv_send_packet(zvrf->rtadv.sock, zif->ifp, RA_ENABLE);
+		wheel_add_item(zrouter.ra_wheel, zif->ifp);


I'm still concerned about this a bit. I only see the "wheel_remove" in the path where the frr-configured "shut" is processed, but not in other interface "down" processing. are you sure this is correct?

added wheel_remove in interface_down()

soumyar-roy · 2025-04-17T16:14:01Z

Test case added.

mjstapp

the interface module is using some rtadv apis. I think the clearest way to fix these problems is to make sure a) that the rtadv module is notified during the relevant interface events, and b) maybe ensure that it's unambiguous what needs to happen within the rtadv code. a flag, for instance, could indicate whether an interface is rtadv-active or not?

soumyar-roy · 2025-05-01T17:25:16Z

the interface module is using some rtadv apis. I think the clearest way to fix these problems is to make sure a) that the rtadv module is notified during the relevant interface events, and b) maybe ensure that it's unambiguous what needs to happen within the rtadv code. a flag, for instance, could indicate whether an interface is rtadv-active or not?

Changed code.

mjstapp · 2025-05-08T14:37:48Z

zebra/interface.c

 #include "log.h"
 #include "zclient.h"
 #include "vrf.h"
+#include "wheel.h"


don't need this change, right?

mjstapp · 2025-05-08T14:42:52Z

zebra/rtadv.c

 	wheel_remove_item(zrouter.ra_wheel, ifp);

+	if (if_down_event) {
+		/* Nothing to do more, return */


I guess I understand that you don't want to try to send a packet, but shouldn't you stop the "join_timer" ?

Sure, I was trying to limit existing behavior part of this change. I have accommodated it now, join_timer is turned off.

mjstapp

I think the zebra changes are ok now

ton31337 · 2025-05-15T06:19:30Z

tests/topotests/high_ecmp/test_high_ecmp_unnumbered.py

+
+    # Take two snap shots for RA status, it should not change
+    # Give enough time, RA adv timer to expire
+    sleep(2)


This is not really deterministic (with sleeps)...

I have removed hardcoding for sleep, parsed ndRouterAdvertisementsIntervalSecs and used that for sleep time. Please check new code. But still we need to wait/sleep for some amount of time, to get more correct result. We need to wait for at-least ndRouterAdvertisementsIntervalSecs + buffer time(1 sec) to avoid situation where any event was delayed or due to any processing delay, the RA state might get updated later, if we checked early, test may fail to catch a wrong state, it may indicate false PASS case.

ton31337 · 2025-05-16T06:03:54Z

tests/topotests/high_ecmp/test_high_ecmp_unnumbered.py

+
+    _, result = topotest.run_and_expect(_check_interface_down, True, count=10, wait=1)
+    if result is not True:
+        sys.stderr.write("Interface did not go down after shutdown command\n")


Why do we need this stderr print if we assert below?

ton31337 · 2025-05-16T06:04:10Z

tests/topotests/high_ecmp/test_high_ecmp_unnumbered.py

+        output = tgen.gears["r1"].vtysh_cmd("show interface r1-eth200 json")
+        return True if '"administrativeStatus":"down"' in output else False
+
+    _, result = topotest.run_and_expect(_check_interface_down, True, count=10, wait=1)


Set at least 15 seconds (this is a minimum)

ton31337 · 2025-05-16T06:24:59Z

tests/topotests/high_ecmp/test_high_ecmp_unnumbered.py

+
+    # Verify RA state didn't change when interface is down
+    if rtadv_output1 != rtadv_output2:
+        sys.stderr.write(


Shouldn't we assert here?

changed code.

ton31337 · 2025-05-20T10:12:33Z

Why do you close and reopen all the time?

Issue: Once interface is shutdown, the interface is removed from wheel timer. Now when the interface is up again, current code won't add the interface to wheel timer again, so it won't send RA anymore for that interface Fix: Moved wheel_add for interface inside rtadv_start_interface_events This is more common function which gets triggered for both RA enable and interface up event Also on any kind of interface activation event, we try to send RA as soon as possible. This is to satisfy requirement where quick RA is needed, especially for some convergence, dependent on RA. Testing: Did ineterface up to down to up Added debug log for RA, checked it is getting advertised preodically after when up at up state show bgp summary for 512 bgp peers for bgp bgp unnumbered works fine. Signed-off-by: Soumya Roy <souroy@nvidia.com>

Added test cases with interface down/up/shutdown to verify RA state of an interface Signed-off-by: Soumya Roy <souroy@nvidia.com>

frrbot bot added libfrr zebra labels Mar 21, 2025

soumyar-roy marked this pull request as draft March 21, 2025 20:30

github-actions bot added master size/M rebase PR needs rebase labels Mar 21, 2025

soumyar-roy force-pushed the soumya/fastra branch from 2ec3ae2 to fb482ba Compare March 21, 2025 20:52

soumyar-roy changed the title ~~Soumya/fastra~~ zebra: send v6 fast RA at faster interval Mar 21, 2025

soumyar-roy force-pushed the soumya/fastra branch 2 times, most recently from f4eedc5 to 7095a84 Compare March 21, 2025 21:51

soumyar-roy force-pushed the soumya/fastra branch from 7095a84 to fe352cb Compare March 22, 2025 02:27

soumyar-roy closed this Mar 22, 2025

soumyar-roy reopened this Mar 22, 2025

soumyar-roy closed this Mar 22, 2025

soumyar-roy reopened this Mar 22, 2025

soumyar-roy force-pushed the soumya/fastra branch from fe352cb to 0e7d05c Compare March 23, 2025 00:51

github-actions bot added size/L and removed size/M labels Mar 23, 2025

soumyar-roy force-pushed the soumya/fastra branch from 0e7d05c to 568f5fe Compare March 23, 2025 05:38

ton31337 reviewed Mar 23, 2025

View reviewed changes

lib/wheel.c Outdated Show resolved Hide resolved

ton31337 reviewed Mar 23, 2025

View reviewed changes

soumyar-roy force-pushed the soumya/fastra branch 2 times, most recently from c13b1c3 to 3a9feb0 Compare March 23, 2025 18:08

soumyar-roy marked this pull request as ready for review March 24, 2025 00:53

soumyar-roy force-pushed the soumya/fastra branch from 3a9feb0 to 11dd5a0 Compare March 24, 2025 16:02

soumyar-roy reopened this Apr 15, 2025

soumyar-roy closed this Apr 15, 2025

soumyar-roy reopened this Apr 15, 2025

soumyar-roy closed this Apr 15, 2025

soumyar-roy reopened this Apr 15, 2025

soumyar-roy force-pushed the soumya/fastra branch from efd20b7 to 3ff752f Compare April 16, 2025 00:40

soumyar-roy closed this Apr 16, 2025

soumyar-roy reopened this Apr 16, 2025

mjstapp reviewed Apr 16, 2025

View reviewed changes

soumyar-roy force-pushed the soumya/fastra branch from 3ff752f to 914dee7 Compare April 16, 2025 22:40

github-actions bot added size/M and removed size/XS labels Apr 16, 2025

soumyar-roy force-pushed the soumya/fastra branch from 914dee7 to 6147ccc Compare April 16, 2025 22:47

mjstapp reviewed Apr 18, 2025

View reviewed changes

This was referenced Apr 21, 2025

[FRR] Add support for 514 BGP sessions sonic-net/sonic-buildimage#22390

Merged

[202412] [FRR] Add support for 514 BGP sessions Azure/sonic-buildimage-msft#1034

Merged

mjstapp mentioned this pull request May 5, 2025

zebra: fix disable radvd upon interface deletion event #18755

Draft

mjstapp reviewed May 8, 2025

View reviewed changes

mjstapp approved these changes May 14, 2025

View reviewed changes

ton31337 reviewed May 15, 2025

View reviewed changes

ton31337 reviewed May 16, 2025

View reviewed changes

soumyar-roy added 2 commits May 20, 2025 13:50

test: Add test case to verify RA adv

cc4fffa

Added test cases with interface down/up/shutdown to verify RA state of an interface Signed-off-by: Soumya Roy <souroy@nvidia.com>

ton31337 approved these changes May 20, 2025

View reviewed changes

mssonicbld mentioned this pull request May 30, 2025

[action] [PR:22390] [FRR] Add support for 514 BGP sessions sonic-net/sonic-buildimage#22786

Merged

11 tasks

Conversation

soumyar-roy commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjstapp commented Mar 21, 2025

Uh oh!

soumyar-roy commented Mar 21, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ton31337 commented Mar 23, 2025

Uh oh!

soumyar-roy commented Mar 23, 2025

Uh oh!

ton31337 commented Mar 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soumyar-roy commented Apr 17, 2025

Uh oh!

mjstapp left a comment

Choose a reason for hiding this comment

Uh oh!

soumyar-roy commented May 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjstapp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ton31337 commented May 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

soumyar-roy commented Mar 21, 2025 •

edited

Loading