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

datapath: allow specifying cilium_host routes metric #17544

Merged
merged 1 commit into from
Nov 22, 2021

Conversation

Frankkkkk
Copy link
Contributor

The networks used by cilium (pod range, etc.) must be routed to the
nodes. One possibility of announcing the ranges is to add them to the
lo interface. Then, a specific route-map on the local router will announce
all the networks belonging to an interface.

However, when adding an IP or a range to an interface, the kernel
automatically adds a route to it like so:

  2001:my:range::/112 dev lo proto kernel metric 256 pref medium

this metric corresponds to IP6_RT_PRIO_ADDRCONF = 256.

The problem arises the moment cilium adds these routes to the cilium host
device. If a metric is not specified (by default 0), then the kernel will use
IP6_RT_PRIO_USER = 1024, which won't take precedence over the above
route.

We thus introduce the --route-metric param which allows overriding
this metric. If the param is not used, the default metrics won't change.

Fixes: #16539
Signed-off-by: Frank Villaro-Dixon frank.villaro@infomaniak.com

@Frankkkkk Frankkkkk requested review from a team October 6, 2021 11:21
@Frankkkkk Frankkkkk requested a review from a team as a code owner October 6, 2021 11:21
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 6, 2021
@aanm aanm added the release-note/misc This PR makes changes that have no direct user impact. label Oct 18, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 18, 2021
Copy link
Contributor

@joamaki joamaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @brb could you also have a quick look?

Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@brb
Copy link
Member

brb commented Nov 8, 2021

/test

@pchaigno

This comment has been minimized.

@pchaigno
Copy link
Member

You'll need to rebase with latest master for build-commits to pass (I'd recommend to rebase whenever you update the PR branch). The above error is also still present. See Travis CI build for example.

@Frankkkkk Frankkkkk force-pushed the fvd/cilium-route-metric branch 2 times, most recently from 5db3bda to 0748ea7 Compare November 18, 2021 12:55
The networks used by cilium (pod range, etc.) must be routed to the
nodes. One possibility of announcing the ranges is to add them to the
`lo` interface. Then, a specific route-map on the local router will announce
all the networks belonging to an interface.

However, when adding an IP or a range to an interface, the kernel
automatically adds a route to it like so:
```
  2001:my:range::/112 dev lo proto kernel metric 256 pref medium
```
this metric corresponds to `IP6_RT_PRIO_ADDRCONF = 256`.

The problem arises the moment cilium adds these routes to the cilium host
device. If a metric is not specified (by default 0), then the kernel will use
`IP6_RT_PRIO_USER = 1024`, which won't take precedence over the above
route.

We thus introduce the `--route-metric` param which allows overriding
this metric. If the param is not used, the default metrics won't change.

Fixes: cilium#16539
Signed-off-by: Frank Villaro-Dixon <frank.villaro@infomaniak.com>
@pchaigno

This comment has been minimized.

@Frankkkkk
Copy link
Contributor Author

Hi @pchaigno thanks for the test and sorry for the huge delay on my part :(. I don't see how the 2 failed tests could be related to this MR ? Thanks !

@pchaigno
Copy link
Member

pchaigno commented Nov 19, 2021

Seems like the GKE failure was caused by a connectivity blip with kube-apiserver. Restarting to be sure.

/test-gke

The k8s-1.21-kernel-4.19 failure seems to be known flake #12690.

@pchaigno
Copy link
Member

CI job gke-stable failed with known flake #17617, k8s-1.21-kernel-4.19 with #12690. Marking ready to merge.

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 22, 2021
@gandro gandro merged commit e506363 into cilium:master Nov 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

datapath: change metric of route to cilium_host to be < 256
7 participants