ip,ip6,frr: flush routes like zebra on address and iface teardown - #710
ip,ip6,frr: flush routes like zebra on address and iface teardown#710maxime-leroy wants to merge 5 commits into
Conversation
Without any IPv4 address left on an interface, ARP requests can no longer be sent from it: the nexthops reachable through that interface can never be resolved again, and the routes using them are dead weight. Linux flushes them (fib_del_ifaddr) and control planes rely on it. Since FRR 10.7, zebra reacts to the deletion of the last IPv4 address of an interface by dropping such routes from its RIB on its own, without sending anything to the dataplane, which left grout and zebra with two different views for good. Flush them in grout instead, and notify as usual so that zebra follows on every FRR version. Routes installed by a routing daemon are left alone, their owner reevaluates them when the nexthop stops resolving. This also means a lost DHCP lease now removes the IPv4 routes of the interface, as it does on Linux. IPv6 is left untouched: on-link information does not come from the configured addresses there, and the link-local address remains available to source neighbor solicitations. Signed-off-by: Maxime Leroy <maxime@leroys.fr>
zebra removes the kernel routes of an interface from its RIB as soon as the interface goes administratively down, and it does not tell the dataplane about it: rib_update_handle_kernel_route_down_possibility() considers a nexthop dead when its interface is not up, for both address families. Linux behaves the same way (fib_disable_ip flushes, and IPv6 loses its addresses with the link), so the routes never come back. grout keeps them and leaves their nexthops unresolved, which is arguably the better model for a reversible state, but it means the two tables disagree for good as soon as an interface is set down. Add flush-on-iface-down to make grout follow the control plane on that point. It is off by default: grout alone keeps its current behaviour, and the routes of a down interface come back when it goes up again. Carrier loss is deliberately not a trigger, the interface is still configured up and neither Linux nor zebra flush anything in that case. Routes installed by a routing daemon are left alone, their owner reevaluates them on the interface event. Signed-off-by: Maxime Leroy <maxime@leroys.fr>
zebra drops the kernel routes of an interface from its RIB as soon as the interface goes administratively down, for both address families, and sends nothing to the dataplane: it assumes the routes are already gone, as they would be on Linux. grout keeps them, so the two tables diverged for good on every supported FRR version. Enable flush-on-iface-down when connecting to grout, and do it on every connection since a restarted grout comes back with its defaults. Signed-off-by: Maxime Leroy <maxime@leroys.fr>
The routes deleted along with an interface, whether it goes down or is removed, are of no use to a control plane which evicts them on the interface event: zebra has already dropped them from its RIB by the time grout reports them, and each notification costs a full rib_delete() for nothing, plus a failed reinstall attempt for the routes zebra owns. Linux is silent in that case too, and its consumers are expected to react to the interface event. Add skip-events-on-iface-down, off by default, and let the deletion paths say whether they want the events instead of guessing from ambient state. The route deletion which follows the removal of the last IPv4 address of an interface keeps notifying: nothing tells the control plane about it otherwise. The configuration request now carries a set_attrs mask, so that a client setting one attribute does not silently reset the ones it does not know about. The plugin enables both attributes at once. Signed-off-by: Maxime Leroy <maxime@leroys.fr>
The route config set command dispatches to every registered address family with the same parse node, so a family which was not given an argument gets zero. Both handlers rejected it with EINVAL, which made "route config set default rib4-routes N" report a failure although the value had been applied, and contradicted the request documentation which says that zero means unchanged. Treat zero as nothing to change, like the neighbouring FIB fields already do. A set_attrs mask would only be needed if zero were a valid FIB size. Fixes: e1b0bde ("ip,ip6: allow configuring default FIB size") Signed-off-by: Maxime Leroy <maxime@leroys.fr>
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can switch off images and animations for a plain-text comment |
|
The API check failure is not a real incompatibility. Both The check trips because its baseline is the parent commit: which means every breaking commit needs its own bump. So I would rather not bump here: it would put every client and daemon in |
|
See my comment on #711. I understand that in this specific case: adding a new message and modifying it in the same patch series should not cause check-api.sh to fail. However, there is no way to differentiate between a new message being modified mid pull request from other real ABI breaking changes. I really want to keep all API breaking changes self contained (i.e. with their own GR_API_VERSION bump attached). If you can find a way to determine the "correct" reference commit mid pull request, we can drop the hard-coded |
|
Commenting on the code here, why do we need a knob to configure these behaviors? We could just mimic what Linux does (even if it is silly). The point here is to be transparent. If we really want to have this configurable at runtime, I think it would make more sense to use environment variables like for other opt-in things. Lines 17 to 26 in 819435f |
2 participants
zebra drops routes from its RIB on its own, without sending anything to the
dataplane, in two situations:
(zebra: remove kernel route on last address deletion FRRouting/frr#19564)
It assumes the dataplane already dropped them, as Linux does
(fib_disable_ip, fib_del_ifaddr), and calls rib_delete() with
fromkernel=true, so nothing reaches the provider. grout keeps them, so the
two tables disagree for good: grout still forwards on routes zebra no
longer knows, and zebra never re-learns them until the plugin resyncs.
Measured with grout + zebra:
grout now flushes those routes too and notifies, so both tables converge on
every FRR version. The address case is unconditional: without a source
address the remaining nexthops can never be resolved, so keeping them was a
defect of its own. The interface case is a compatibility concession, a down
interface being a reversible state, so it is optional
(flush-on-iface-down, off by default) and the plugin enables it. Carrier
loss is not a trigger, the interface stays configured up. Routes installed
by a routing daemon are never touched, their owner reevaluates them.
The notifications for the routes deleted with an interface are dropped as
well (skip-events-on-iface-down, off by default, enabled by the plugin):
zebra has already purged them, and each one costs a full rib_delete() plus
a failed reinstall attempt for the routes zebra owns.
Two new smoke tests. Both assert that grout and zebra hold the same view
rather than a fixed outcome, so they are version agnostic, and both are red
without the series: