Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ospf6d: Fix summary LSA removal #18345

Draft
wants to merge 4 commits into
base: stable/10.2
Choose a base branch
from

Conversation

gromit1811
Copy link
Contributor

Finally, a first attempt to fix #16197

The main fixes are from the following 2 commits (other commits are mostly cleanup/optimization):

commit 9eb0492

ospf6d: Fix old summary route lookup

When orignating summaries, we perform a lookup to find possible previous
summaries. Don't just compare the prefix in that case, but also route and
path properties: We might have several summary entries for the same
destination (e.g. with a intra- and inter-area path) at the same time (at
least temporarily) and we need to deal with the right one.

commit 6f8df0c

ospf6d: Fix summary LSA removal by using ospf6_abr_task()

Previously, we checked each individual route to see whether a summary LSA
needs to be originated or purged. The problem is that this decision can't be
made reliably by looking at a single route, because there might be several
routes for a destination (e.g. one with a intra- and one with an inter-area
path) and one might cause origination of a summary LSA while the other
causes it to be purged. This causes unstable and/or missing LSAs as
described in https://github.com/FRRouting/frr/issues/16197

Instead, ospf6_abr_originate_summary_to_area() now only originates new LSAs
or approves existing ones. Removal is handled in ospf6_abr_task() after the
whole routing table has been iterated. LSAs which have not been approved by
any call to ospf6_abr_originate_summary_to_area() are then removed. This
matches the behaviour of OSPFv2.

Note that this isn't finished/complete yet and is intended mainly for feedback or to stop me in case I'm moving in a completely wrong direction 😉

Open issues:

  • Check topotest failures: ospf6_gr_topo1 ospf6_topo2
  • Routing table hooks no longer make sense the way they're implemented (directly calling ospf6_abr_originate_summary_to_area()), just schedule ospf6_abr_task() instead?
  • Right now we actually check routes twice, once via hooks and once via ospf6_abr_task(). Doesn't seem to hurt, but isn't really efficient.
  • Check whether there are other cases where we might have to schedule ospf6_abr_task() to immediately remove a summary LSA.
  • ospf6_abr_task() is hidden in ospf6_nssa.c even though it is not at all related to NSSA. Guess it was put there when adding NSSA support and that needed a small part of the ospf6_abr_task() functionality. Move non-NSSA functions from ospf6_nssa.c to ospf6_abr.c.

No point in doing this for route types we're not interested in at all.

Signed-off-by: Martin Buck <[email protected]>
When orignating summaries, we perform a lookup to find possible previous
summaries. Don't just compare the prefix in that case, but also route and
path properties: We might have several summary entries for the same
destination (e.g. with a intra- and inter-area path) at the same time (at
least temporarily) and we need to deal with the right one.

Signed-off-by: Martin Buck <[email protected]>
Previously, we checked each individual route to see whether a summary LSA
needs to be originated or purged. The problem is that this decision can't be
made reliably by looking at a single route, because there might be several
routes for a destination (e.g. one with a intra- and one with an inter-area
path) and one might cause origination of a summary LSA while the other
causes it to be purged. This causes unstable and/or missing LSAs as
described in FRRouting#16197

Instead, ospf6_abr_originate_summary_to_area() now only originates new LSAs
or approves existing ones. Removal is handled in ospf6_abr_task() after the
whole routing table has been iterated. LSAs which have not been approved by
any call to ospf6_abr_originate_summary_to_area() are then removed. This
matches the behaviour of OSPFv2.

Signed-off-by: Martin Buck <[email protected]>
We slightly abuse summary routes to store information about LSAs that have
been originated for them. Most other route parameters are not used for
anything. So don't update them to avoid giving the impression that we've
got real routes here and to reduce confusion.

Signed-off-by: Martin Buck <[email protected]>
@frrbot frrbot bot added the ospfv3 label Mar 7, 2025
@donaldsharp
Copy link
Member

Hey can we submit the PR against master not 10.2? All code must go into master first.

@gromit1811
Copy link
Contributor Author

Sure. I'm currently testing on 10.2.1 to have a stable base, but of course I'll rebase to master when the PR is complete and before removing the draft status

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

Successfully merging this pull request may close these issues.

2 participants