zebra: fix route deletion during zebra shutdown

[askorichenko]

Restore the order of execution in zebra's main thread:
vrf_terminate();
zebra_dplane_finish();
zebra_dplane_shutdown();
changed in commit 3b0e170.

vrf_terminate() enqueues routes for deletion in dplane thread.
zebra_dplane_shutdown() effectively forces dplane thread
to immediately cease and join (leaving unprocessed routes), while
zebra_dplane_finish() has a condition that prevents
zebra_dplane_shutdown() from running until all routes queued for dplane thread have been deleted.

Closes #14062
Closes #14178

[askorichenko]

This patch fixes issues
#14178
#14062
The bug affects routes installed by different daemons, not only staticd

Bug reproduction (on staticd example) with current master build (9.2):

$ cat >static_254_routes.config<<EOF
ip route 10.0.0.1/32 10.255.0.1
ip route 10.0.0.2/32 10.255.0.1
...
ip route 10.0.0.254/32 10.255.0.1
EOF

# vtysh -f static_254_routes.config
# systemctl stop frr
# ip r 
10.0.0.76 nhid 14 via 10.255.0.1 dev enp1s0 proto 196 metric 20
10.0.0.77 nhid 14 via 10.255.0.1 dev enp1s0 proto 196 metric 20
...
10.0.0.254 nhid 14 via 10.255.0.1 dev enp1s0 proto 196 metric 20

i.e. the tail of the queued deletion events remained unprocessed.

Please review.
Thank you!

[ton31337]

@Mergifyio backport dev/10.0 stable/9.1

[mergify]

backport dev/10.0 stable/9.1

🟠 Waiting for conditions to match

• merged [📌 backport requirement]

[ton31337]

New ASAN failure, see here https://ci1.netdef.org/browse/FRR-PULLREQ3-2001/artifact/ASANP3/AddressSanitizerError/AddressSanitzer.txt. Need to fix this.

[mjstapp]

So the commit you refer to believed it was fixing an ordering problem with vrfs - is that problem still fixed, or will it reappear if this change is merged?

[askorichenko]

1. The currently implemented order aborts route deletion by running zebra_dplane_shutdown() right after vrf_terminate()

zebra_dplane_finish();
vrf_terminate();
zebra_dplane_shutdown();

2. My first version of the patch could delete routes properly, but freed memory too early in vrf_terminate() (thus reintroduced the possible crash fixed by 3b0e170)

vrf_terminate();
zebra_dplane_finish();
zebra_dplane_shutdown();

3. Newer edit suggests to split vrf_terminate() into parts - one that initiates route removal (protected by zebra_dplane_finish() from cancellation as in 2.), and the other releases memory at the final stage (like in 1.). In the main zebra thread the order is:

vrf_disable_all();
zebra_dplane_finish();
vrf_terminate();
zebra_dplane_shutdown();

vrf_terminate() is a library function, it calls disable() (if not already disabled) and delete() (releases resources).
vrf_disable_all() is copied from there but implements only disable() method.

[mergify]

backport dev/10.0 stable/9.1

✅ Backports have been created

• #15569 zebra: fix route deletion during zebra shutdown (backport #15424) has been created for branch dev/10.0
• #15570 zebra: fix route deletion during zebra shutdown (backport #15424) has been created for branch stable/9.1

ASAN fixed.

[riw777]

looks good

[askorichenko]

Added code cleanup requested by @mjstapp.
Agreed this way it looks better. Thank you!

[mjstapp]

This version looks good - but I see that there are two commits here, when really there should just be the one that makes this set of changes. Can you remove or squash the other commit?

[askorichenko]

Pushed in a single commit.
Though there is a failing test in TopoTests Debian 10 amd64 Part 1, but I noticed this particular fail pops out regularly across other PRs, as well, seemed unrelated to the change.

[mjstapp]

so the tests need to pass - but if you look at the results summary, you'll see that there's a button that reruns just the failed tests, which is very useful. I've asked for that rerun - let's see if it's clean.