Skip to content

[FRR] send EOR during GR only when fib install complete#24174

Closed
stepanblyschak wants to merge 5 commits intosonic-net:masterfrom
stepanblyschak:frr-gr-fix
Closed

[FRR] send EOR during GR only when fib install complete#24174
stepanblyschak wants to merge 5 commits intosonic-net:masterfrom
stepanblyschak:frr-gr-fix

Conversation

@stepanblyschak
Copy link
Collaborator

@stepanblyschak stepanblyschak commented Oct 1, 2025

Why I did it

EOR was sent before all routes are advertised when suppress-fib-pending is enabled, this leads to packet loss, due to peer doing route reconciliation for incomplete RIB.

BACKPORT of upstream FRR fix - FRRouting/frr#19522

Work item tracking
  • Microsoft ADO (number only):

How I did it

Backport FRR fix.

How to verify it

Setup topogy like this, setup a regular router interaface between DUT and AUX and setup BGP session between them as well as between switches and IXIA. Run bidirectional traffic towards the prefix advertised by IXIA1. Observe no packet loss during warm-reboot.

IXIA1 <-> DUT <-> AUX <-> IXIA2

Relevant logs:

2025/09/30 12:19:05 BGP: [M67YY-FW795] VRF default: GR route FIB install count incremented to 1 for IPv6 Unicast (prefix: 2000:2:3::/64)
2025/09/30 12:19:05 BGP: [M67YY-FW795] VRF default: GR route FIB install count incremented to 2 for IPv6 Unicast (prefix: 3000:2:3::/64)
2025/09/30 12:19:05 BGP: [M67YY-FW795] VRF default: GR route FIB install count incremented to 3 for IPv6 Unicast (prefix: 3000:2:4::/64)
2025/09/30 12:19:05 BGP: [M67YY-FW795] VRF default: GR route FIB install count incremented to 4 for IPv6 Unicast (prefix: 3000:2:5::/64)
2025/09/30 12:19:05 BGP: [M67YY-FW795] VRF default: GR route FIB install count incremented to 5 for IPv6 Unicast (prefix: 3000:2:6::/64)
2025/09/30 12:19:05 BGP: [M59KS-A3ZXZ] bgp_update_receive: rcvd End-of-RIB for IPv6 Unicast from 2000:1::2 in vrf default


2025/09/30 12:19:05 BGP: [M67YY-FW795] VRF default: GR route FIB install count incremented to 1 for IPv4 Unicast (prefix: 23.23.23.0/24)
2025/09/30 12:19:05 BGP: [M67YY-FW795] VRF default: GR route FIB install count incremented to 2 for IPv4 Unicast (prefix: 24.24.24.0/24)
2025/09/30 12:19:05 BGP: [M67YY-FW795] VRF default: GR route FIB install count incremented to 3 for IPv4 Unicast (prefix: 25.25.25.0/24)
2025/09/30 12:19:05 BGP: [M67YY-FW795] VRF default: GR route FIB install count incremented to 4 for IPv4 Unicast (prefix: 26.26.26.0/24)
2025/09/30 12:19:05 BGP: [M67YY-FW795] VRF default: GR route FIB install count incremented to 5 for IPv4 Unicast (prefix: 100.2.3.0/24)
2025/09/30 12:19:05 BGP: [M59KS-A3ZXZ] bgp_update_receive: rcvd End-of-RIB for IPv4 Unicast from 10.0.2.2 in vrf default


2025/09/30 12:20:10 BGP: [P6D1N-F7T05] VRF default: GR route FIB install count decremented to 4 for IPv4 Unicast (prefix: 100.2.3.0/24)
2025/09/30 12:20:10 BGP: [P6D1N-F7T05] VRF default: GR route FIB install count decremented to 4 for IPv6 Unicast (prefix: 2000:2:3::/64)
2025/09/30 12:20:10 BGP: [P6D1N-F7T05] VRF default: GR route FIB install count decremented to 3 for IPv4 Unicast (prefix: 23.23.23.0/24)
2025/09/30 12:20:10 BGP: [P6D1N-F7T05] VRF default: GR route FIB install count decremented to 2 for IPv4 Unicast (prefix: 24.24.24.0/24)
2025/09/30 12:20:10 BGP: [P6D1N-F7T05] VRF default: GR route FIB install count decremented to 1 for IPv4 Unicast (prefix: 25.25.25.0/24)
2025/09/30 12:20:10 BGP: [P6D1N-F7T05] VRF default: GR route FIB install count decremented to 0 for IPv4 Unicast (prefix: 26.26.26.0/24)
2025/09/30 12:20:10 BGP: [KWWZB-VM9GP] VRF default: Triggering GR deferral completion from FIB notification for IPv4 Unicast
2025/09/30 12:20:10 BGP: [ZP3RE-J4Q8C] send End-of-RIB for IPv4 Unicast to 100.2.6.2
2025/09/30 12:20:10 BGP: [ZP3RE-J4Q8C] send End-of-RIB for IPv4 Unicast to 100.2.5.2
2025/09/30 12:20:10 BGP: [ZP3RE-J4Q8C] send End-of-RIB for IPv4 Unicast to 10.0.2.2
2025/09/30 12:20:10 BGP: [ZP3RE-J4Q8C] send End-of-RIB for IPv4 Unicast to 10.0.1.2


2025/09/30 12:20:10 BGP: [P6D1N-F7T05] VRF default: GR route FIB install count decremented to 3 for IPv6 Unicast (prefix: 3000:2:3::/64)
2025/09/30 12:20:10 BGP: [P6D1N-F7T05] VRF default: GR route FIB install count decremented to 2 for IPv6 Unicast (prefix: 3000:2:4::/64)
2025/09/30 12:20:10 BGP: [P6D1N-F7T05] VRF default: GR route FIB install count decremented to 1 for IPv6 Unicast (prefix: 3000:2:5::/64)
2025/09/30 12:20:10 BGP: [P6D1N-F7T05] VRF default: GR route FIB install count decremented to 0 for IPv6 Unicast (prefix: 3000:2:6::/64)
2025/09/30 12:20:10 BGP: [KWWZB-VM9GP] VRF default: Triggering GR deferral completion from FIB notification for IPv6 Unicast
2025/09/30 12:20:10 BGP: [ZP3RE-J4Q8C] send End-of-RIB for IPv6 Unicast to 2000:2:5::2
2025/09/30 12:20:10 BGP: [ZP3RE-J4Q8C] send End-of-RIB for IPv6 Unicast to 2000:2::2
2025/09/30 12:20:10 BGP: [ZP3RE-J4Q8C] send End-of-RIB for IPv6 Unicast to 2000:1::2
2025/09/30 12:20:10 BGP: [ZP3RE-J4Q8C] send End-of-RIB for IPv6 Unicast to 2000:2:6::2

Which release branch to backport (provide reason below if selected)

  • 202205
  • 202211
  • 202305
  • 202311
  • 202405
  • 202411
  • 202505

Tested branch (Please provide the tested image version)

Description for the changelog

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@keboliu keboliu requested a review from dgsudharsan October 9, 2025 02:23
@StormLiangMS
Copy link
Contributor

hi @stepanblyschak @dgsudharsan

I'm trying to assess the impact of this issue. We typically apply FIB suppression on T1 devices when they're in GR (Graceful Restart) and covered. After that, we send the RIB to its peer, but send EOR before transmitting all the route information. This can result in some routes being deleted and re-added, which might cause an unnecessary partial route flap on the peer device, but it shouldn't cause a black hole, correct?

Also, there's a test case (test_bgp_gr_helper) that seems to have failed to catch this issue. Should we consider this a test gap?

Lastly, there appears to be a conflict with the PR. Could you help address it?

@dgsudharsan
Copy link
Collaborator

@StormLiangMS
Yes you are correct. If we perform warm-boot with fib suppression enabled, the deivce after reboot will send EOR before advertising the routes, as it takes additional time to announce routes waiting for the hw install. The result is routes getting deleted at peer device when receiving EOR and getting added back once they are advertised by the device that went warmboot.
Regarding the test gap, we can check and get back but if i am not mistaken warmboot is done only in T0 and suppression is enabled in T1. Do we have both together in any use case?

@volodymyrsamotiy to help resolve the conflict.

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@stepanblyschak
Copy link
Collaborator Author

@StormLiangMS Issue is only with T0 device (T1 is not warm-reboot-able) and only with the following change applied - sonic-net/sonic-swss#3782.

Note, that due to a delay in PortChannel IP configuration, the upstream BGP sessions are established with significant delay.
If there's no delay, then, the offload indication is not populated to zebra and so the device does not send BGP route updates to T1, but instead prematurelly sends EOR causing drops.

Yes, it's a test gap in sonic-mgmt (even with sonic-net/sonic-swss#3782 all sonic-mgmt we run tests pass). It was found by another test.

@StormLiangMS
Copy link
Contributor

@StormLiangMS Issue is only with T0 device (T1 is not warm-reboot-able) and only with the following change applied - sonic-net/sonic-swss#3782.

Note, that due to a delay in PortChannel IP configuration, the upstream BGP sessions are established with significant delay. If there's no delay, then, the offload indication is not populated to zebra and so the device does not send BGP route updates to T1, but instead prematurelly sends EOR causing drops.

Yes, it's a test gap in sonic-mgmt (even with sonic-net/sonic-swss#3782 all sonic-mgmt we run tests pass). It was found by another test.

hi @stepanblyschak Do we observe this issue in the T1 use case? I don't believe we've ever enabled FIB suppress on T0. We could merge this fix into the master branch, but I'd prefer to keep it out of 202505 unless it has an impact on real scenario. Could you also open a GitHub issue to track this test gap?

@stepanblyschak
Copy link
Collaborator Author

@StormLiangMS we don't observe issues with t1 use case, ok not taking to 202505

@eddieruan-alibaba
Copy link
Collaborator

eddieruan-alibaba commented Nov 3, 2025

Should this FRR change be committed to FRR 10.3, 10.4 release branch first ? Since currently 202505 uses FRR 10.3 as FRR base and this patch has none trivial changes in bgp, I would seriously concern about the future maintainability if this commit is in SONiC's 10.3, but not in FRR's 10.3.

For 202511, we are rebasing to 10.4. Again, we can't take this change until this commit is in FRR stable/10.4. That would help us to maintain long term maintainability for release patching.

Copy link
Collaborator

@eddieruan-alibaba eddieruan-alibaba left a comment

Choose a reason for hiding this comment

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

Need to make sure it is committed to FRR stable/10.3, stable/10.4 first, before adding to SONiC patch list.

@liat-grozovik
Copy link
Collaborator

Need to make sure it is committed to FRR stable/10.3, stable/10.4 first, before adding to SONiC patch list.

i believe this should be taken to the FRR community and not to SONiC. once it is there, then the owner of the SONIC FRR maintainer will do that. right?

@StormLiangMS do we have an approval to make it part of master/202511? as branch is out soon lets try to close it this week please.

@StormLiangMS
Copy link
Contributor

I'm ok to take this in 202511, but would like to have a testgap to address this. @stepanblyschak could you help to file one? We can ask community to help.

To have this patch, what Eddie suggested is to have this committed to stable/10.4 FRR, @stepanblyschak could you help to confirm if upgrade to stable/10.4 FRR, do we still need this patch? If so, could we ask FRR community to cherrypick this patch first before adding to SONiC? @dgsudharsan @eddieruan-alibaba @liat-grozovik FYI.

@stepanblyschak
Copy link
Collaborator Author

@StormLiangMS Test gap issue - sonic-net/sonic-mgmt#21249

@StormLiangMS
Copy link
Contributor

Thanks @stepanblyschak would you think we can have this in stable/10.4 FRR as a fix, then we can get it by an upgrade of stable/10.4 which is in progress for 202511 release from FRR to SONiC?

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@stepanblyschak
Copy link
Collaborator Author

@StormLiangMS I am not aware of FRR plans. This is a backport of the change done in master.
I see it is a usual practice to add patches from new FRR relases to sonic FRR, why this one is different?
As for the FRR maybe @dgsudharsan can help.

@eddieruan-alibaba
Copy link
Collaborator

@StormLiangMS I am not aware of FRR plans. This is a backport of the change done in master. I see it is a usual practice to add patches from new FRR relases to sonic FRR, why this one is different? As for the FRR maybe @dgsudharsan can help.

There are two main reasons for adopting this approach:

1 Expertise in PR Review: The FRR community is better positioned to review this PR, given their deeper familiarity with the codebase. For this particular PR, since the changes are non-trivial, it is better to get bless from FRR team for adding confidence in the quality and correctness.

2 Simplified Future Cherry-Picks: Submitting the change upstream to FRR would streamline the process of backporting it to SONiC later. If conflicts arise during cherry-picking from an FRR release branch, resolving them will be easier when the change already exists in the upstream repository.

Therefore, we put the following statement in https://github.com/sonic-net/SONiC/blob/master/tsc/frr/sonic_frr_update_process.md

"Note:

Currently, SONiC FRR maintainers are NOT responsible for cherry-picking patches across different SONiC releases. For example, applying critical patches from the FRR 10.4 branch to the SONiC 202505 branch. Such patches must first be merged upstream into FRR."

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@stepanblyschak
Copy link
Collaborator Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command.

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@stepanblyschak
Copy link
Collaborator Author

Seems unrelated error:

>           raise RunAnsibleModuleFail("run module {} failed".format(self.module_name), hostname_res)
E           tests.common.errors.RunAnsibleModuleFail: run module command failed, Ansible Results =>
E           failed = True
E           changed = True
E           rc = 1
E           cmd = ['python', '/tmp/loganalyzer.py', '--action', 'init', '--run_id', 'test_orchagent_heartbeat_checker.2026-01-28-14:33:03']
E           start = 2026-01-28 14:33:03.350364
E           end = 2026-01-28 14:33:03.479623
E           delta = 0:00:00.129259
E           msg = non-zero return code
E           invocation = {'module_args': {'_raw_params': 'python /tmp/loganalyzer.py --action init --run_id test_orchagent_heartbeat_checker.2026-01-28-14:33:03', '_uses_shell': False, 'expand_argument_vars': True, 'stdin_add_newline': True, 'strip_empty_ends': True, 'argv': None, 'chdir': None, 'executable': None, 'creates': None, 'removes': None, 'stdin': None}}
E           _ansible_no_log = False
E           stdout =
E           stderr =
E           /tmp/loganalyzer.py:18: SyntaxWarning: invalid escape sequence '\ '
E             -m /home/hrachya/projects/loganalyzer/match.file.1.log,/home/hrachya/projects/loganalyzer/match.file.2.log \    # noqa: E501 W605
E           Traceback (most recent call last):
E             File "/tmp/loganalyzer.py", line 877, in <module>
E               main(sys.argv[1:])
E               ~~~~^^^^^^^^^^^^^^
DEBUG:tests.conftest:[log_custom_msg] item: <Function test_orchagent_heartbeat>
DEBUG:tests.conftest:append custom_msg: {'dut_check_result': {'core_dump_check_failed': True, 'config_db_check_failed': False}}
INFO:root:Can not get Allure report URL. Please check logs
E             File "/tmp/loganalyzer.py", line 830, in main
E               analyzer.place_marker(log_file_list, analyzer.create_start_marker())
E               ~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
E             File "/tmp/loganalyzer.py", line 259, in place_marker
E               self.place_marker_to_syslog(marker)
E               ~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^
E             File "/tmp/loganalyzer.py", line 208, in place_marker_to_syslog
E               self.flush_rsyslogd()
E               ~~~~~~~~~~~~~~~~~~~^^
E             File "/tmp/loganalyzer.py", line 172, in flush_rsyslogd
E               out = str(subprocess.check_output("systemctl status rsyslog", shell=True))
E                         ~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
E             File "/usr/lib/python3.13/subprocess.py", line 472, in check_output
E               return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
E                      ~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
E                          **kwargs).stdout
E                          ^^^^^^^^^
E             File "/usr/lib/python3.13/subprocess.py", line 577, in run
E               raise CalledProcessError(retcode, process.args,
E                                        output=stdout, stderr=stderr)
E           subprocess.CalledProcessError: Command 'systemctl status rsyslog' returned non-zero exit status 3.

@stepanblyschak
Copy link
Collaborator Author

@dgsudharsan to take over this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants