Unify all the RouteTables that point at the main kernel table #8418
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
marvin-tigera
added
release-note-required
fasaxc
force-pushed
the
route-unification
branch
from
f67efab to
ae7138b
Compare
3 tasks
fasaxc
force-pushed
the
route-unification
branch
3 times, most recently
from
5622c58 to
ff922e2
Compare
fasaxc
added
docs-not-required
fasaxc
force-pushed
the
route-unification
branch
8 times, most recently
from
351a716 to
a7caec2
Compare
fasaxc
force-pushed
the
route-unification
branch
3 times, most recently
from
3f7cadc to
d4276c4
Compare
fasaxc
force-pushed
the
route-unification
branch
2 times, most recently
from
ddaff28 to
937977c
Compare
3 tasks
fasaxc
force-pushed
the
route-unification
branch
from
fbf4100 to
02a009f
Compare
3 tasks
fasaxc
force-pushed
the
route-unification
branch
from
a050676 to
0dece97
Compare
3 tasks
fasaxc
force-pushed
the
route-unification
branch
from
0dece97 to
4ea7466
Compare
fasaxc
changed the title
|
Still filling in the last bits of coverage but I think this is ready for a first pass now... |
fasaxc
force-pushed
the
route-unification
branch
from
44cbc45 to
91c4262
Compare
fasaxc
force-pushed
the
route-unification
branch
from
91c4262 to
4860486
Compare
Replace the plethora of RouteTable instances that we used to program routes into the main kernel routing table with a single unified RouteTable. - Instead of handling conflicts by trying to program both routes into the kernel, detect conflicts in user space and program only the winning route in the kernel. The previous behaviour resulted in a messy retry loop, spewing errors until the conflict was resolved. - Given each class of routes its own `RouteClass` with an associated priority to allow clean conflict resolution. for example, a local workload route has a higher priority than an IPAM block "blackhole" route. - Align route programming with the indexing approach in the kernel. In the kernel, routes are indexed on table, CIDR, ToS and metric. Previously, we would read back routes and only take account of the CIDR. This resulted in the possibility of aliasing: we would read back two routes and only see one CIDR. - Identifying which routes belong to Calico in the main routing table is very messy and requires heuristics that were spread across several components. Unify those heuristics into a single "ownership policy" abstraction. The main routing table gets its own ownership policy. - Fix that conntrack cleanup wasn't properly handled when routes moved between RouteTables. conntrack cleanup would only be sequenced correctly when routes moved within a RouteTable. For example, if a route moved from one workload to another, we'd correctly sequence removal of the route, conntrack cleanup and adding the new route. However, if a route moved from local workload to remote VXLAN via the tunnel, we'd do the cleanup in parallel with programming the new route resulting in potential glitches. - Move conntrack cleanup to a new object to encapsulate the fiddly calculation. To align with Conntrack, this is still CIDR based instead of taking into account the ToS and Priority. For now, we assume that cleanups may happen for alternate-priority routes but that the RouteTable won't program more than one route per CIDR.
fasaxc
force-pushed
the
route-unification
branch
from
4860486 to
83881c6
Compare
fasaxc
force-pushed
the
route-unification
branch
from
83881c6 to
d01fa34
Compare
caseydavenport
approved these changes
Aug 7, 2024
| // StartConntrackCleanupAndReset. | ||
| // | ||
| // No cleanup is triggered if there is a desired route for the given CIDR | ||
| // and that desirec route is staying on this interface. |
| for { | ||
| select { | ||
| case <-ch: | ||
| logrus.WithField("ip", ipAddr).Info("Done waiting for pending conntrack deletion to finish") |
There was a problem hiding this comment.
Might be worth including how long it took in both of these logs?
|
/merge-when-ready |
|
OK, I will merge the pull request when it's ready, leave the commits as is when I merge it, and leave the branch after I've merged it. |
fasaxc
force-pushed
the
route-unification
branch
from
813c9b6 to
115f80e
Compare
|
Removing "merge-when-ready" label due to new commits |
fasaxc
force-pushed
the
route-unification
branch
from
115f80e to
5cfb93f
Compare
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Replace the plethora of RouteTable instances that we used to program routes into the main kernel routing table with a single unified RouteTable.
RouteClasswith an associated priority to allow clean conflict resolution. for example, a local workload route has a higher priority than an IPAM block "blackhole" route. Bridge the gap from old API to new by introducing a ClassView object that exposes the old API to the "manager" layer objects. They each act as if they have their own RouteTable but the ClassView adds the RouteClass in order to disambiguate routes.Related issues/PRs
CORE-9902
CORE-10126
CI-1357
Todos
Release Note
Reminder for the reviewer
Make sure that this PR has the correct labels and milestone set.
Every PR needs one
docs-*label.docs-pr-required: This change requires a change to the documentation that has not been completed yet.docs-completed: This change has all necessary documentation completed.docs-not-required: This change has no user-facing impact and requires no docs.Every PR needs one
release-note-*label.release-note-required: This PR has user-facing changes. Most PRs should have this label.release-note-not-required: This PR has no user-facing changes.Other optional labels:
cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.