Apply FRR config before port config on SONiC (work around FRR defect) - #202
Apply FRR config before port config on SONiC (work around FRR defect)#202chbmuc wants to merge 2 commits into
Conversation
Deleting a firewall leaves bgpd with a dangling peer pointer, and the switch crashes with SIGSEGV moments later when the port is bounced. Sonic.Apply() wrote the port and interface configuration first. When a firewall is deleted its port moves from Ports.Firewalls to Ports.Unprovisioned, so configureUnprovisionedPort() calls ensureNotRouted(), which deletes the INTERFACE entry and then waits until the router interface has disappeared from the ASIC. That removes the interface's link-local address, and FRR responds by clearing peer->su. Only afterwards did the FRR configuration withdraw the neighbor. With peer->su already unset, FRR can no longer find the peer's nexthop cache entry while deleting the peer, so bnc->nht_info is left pointing at the freed peer. Moving the port into the PXE vlan bounces the interface immediately afterwards. FRR tracks link-local nexthops through interface events rather than through zebra nexthop registration, so bgp_nht_ifp_up() walks every nexthop cache entry whose ifindex matches the interface and calls evaluate_paths() on each, which dereferences the freed peer. Arming and firing therefore happen seconds apart within a single deprovisioning run. Applying the FRR configuration first withdraws the neighbor while its interface still carries a link-local address, so FRR's cleanup path succeeds and no dangling pointer is left behind. This works around an FRR defect fixed upstream, but not yet in the currently used Sonic Linux release. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
doesn't work: |
The last commit moved the FRR configuration in front of the port configuration to keep bgpd from crashing when a firewall is deleted. The dependency between the two appliers runs both ways, though: a tenant vrf only becomes usable for FRR once the redis applier has written it to the CONFIG_DB and vrfmgrd has created the vrf device. Referencing a vrf that does not exist yet makes frr-reload reject the configuration with "% VRF Vrf<vni> not active", and every interface line below the rejected "interface ... vrf ..." is then parsed at the wrong node and reported as an unknown command. Because Apply() returned on the first FRR error, the redis applier never ran, so the vrf was never created, so the next reload failed for the same reason. reloadFrr() restores the previous frr.conf, so every reconcile run re-rendered the same configuration and failed identically - once a machine was provisioned into a new vrf, the switch never converged again. Ask the redis applier which order a run needs instead of fixing it. A port that is about to be deprovisioned and still carries a routing configuration without a vrf_name is a firewall port whose neighbor has to be withdrawn from FRR before ensureNotRouted() takes its link-local address away - only those runs need the FRR configuration first, everything else keeps the historic order and never references a vrf too early. The decision is made from the CONFIG_DB rather than from a remembered configuration, so it also holds for the first run after a restart. If it cannot be made, FRR goes first: a superfluous frr-reload is cheaper than a crashing bgpd. A run that deprovisions a firewall and adds a vrf at the same time cannot satisfy both requirements. FRR is applied first there as well, but a failure no longer keeps the port configuration from being applied and the FRR configuration is retried once the vrfs exist. The protection against the dangling peer pointer is kept either way, because frr-reload applies deletions before additions, so the neighbor is withdrawn in the first pass even when that pass later fails on the missing vrf. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Unfortunately it gets a bit more complicated in this case (see commit message). |
Where exactly did this occur and what service crashes with SIGSEGV? swss, metal-core or bgpd? I'm not sure I fully understand the problem that this PR is addressing. It introduces some rather high complexity that would need thorough testing in a real environment. And it sets the metal-core up for intended errors in case one firewall gets deleted and a new one gets created within the same reconfiguration. Does the issue at hand justify the complexity? If so I'd like to discuss if there is a solution that doesn't involve letting the metal-core run into errors on purpose. @Sven-Ric is currently evaluating whether the mgmt-framework on edgecore SONiC is mature enough to configure BGP entirely via ConfigDB. If that's possible we can get rid of frr-reload completely. |
|
The reason for this workaround is a known bug in FRR. It causes a SIGSEGV in bgpd when the interface of a underlay device (firewall) goes down before the FRR tables get updated. The consequence is a ~ 70 second network outage for all devices on that leaf switches, as the bug is triggered on both switches of the leaf pair. The issue is critical for one of our customers, but of course a better way to fix this would be updating FRR with the bugfix (FRRouting/frr@51f3216bee). The bugfix is included in all FRR versions > 8.5. (Edgecore-SONiC still uses a patched FRR version 8.1/8.2). |
|
FRR versions cannot be updated on SONiC as far as I know. Only way I can think of is replacing the bgp container by a newer one but that sounds hacky and probably not intended. What I don't understand is why did this bug only occur once and only on one leaf pair. From the description of the FRR bug it should be happening all the time whenever a firewall or machine gets deleted. We have the same SONiC version running on all SONiC leafs. It's a guess we can't really validate but I assume the crash was actually caused by a metal-core timeout that left the ConfigDB in a broken state. After that, the BGPD also crashed. But as we haven't cleaned up the config after the first crash it's possible that the same broken state led to the second crash as well. As long as we don't have a way of reproducing the crash I'd opt for increasing the reconfigure timeout to reduce the risk of breaking the config. |
3 participants
Description
Deleting a firewall leaves bgpd with a dangling peer pointer, and the switch crashes with SIGSEGV moments later when the port is bounced.
Sonic.Apply() wrote the port and interface configuration first. When a firewall is deleted its port moves from Ports.Firewalls to Ports.Unprovisioned, so configureUnprovisionedPort() calls ensureNotRouted(), which deletes the INTERFACE entry and then waits until the router interface has disappeared from the ASIC. That removes the interface's link-local address, and FRR responds by clearing peer->su. Only afterwards did the FRR configuration withdraw the neighbor. With peer->su already unset, FRR can no longer find the peer's nexthop cache entry while deleting the peer, so bnc->nht_info is left pointing at the freed peer.
Moving the port into the PXE vlan bounces the interface immediately afterwards. FRR tracks link-local nexthops through interface events rather than through zebra nexthop registration, so bgp_nht_ifp_up() walks every nexthop cache entry whose ifindex matches the interface and calls evaluate_paths() on each, which dereferences the freed peer. Arming and firing therefore happen seconds apart within a single deprovisioning run.
Applying the FRR configuration first withdraws the neighbor while its interface still carries a link-local address, so FRR's cleanup path succeeds and no dangling pointer is left behind.
This works around an FRR defect fixed upstream, but not yet in the currently used Sonic Linux release.
Used AI-Tools ✨