Fetch MTU from the nodeIP interface by manuelbuil · Pull Request #10635 · cilium/cilium

manuelbuil · 2020-03-19T15:44:10Z

The detected MTU is always the one belonging to the interface that acts
as gateway. That interface does not necessarily need to be the one that
holds nodeIP and thus the one that Cilium will use to send traffic
inside the k8s cluster. This patch fixes this and detects the mtu of the
interface used for cluster communications

Fixes: #10309
Signed-off-by: Manuel Buil mbuil@suse.com

Please ensure your pull request adheres to the following guidelines:

For first time contributors, read Submitting a pull request
All code is covered by unit and/or runtime tests where feasible.
All commits contain a well written commit description including a title,
description and a Fixes: #XXX line if the commit addresses a particular
GitHub issue.
All commits are signed off. See the section Developer’s Certificate of Origin
Provide a title or release-note blurb suitable for the release notes.
Thanks for contributing!

The detected MTU is always the one belonging to the interface that acts
as gateway. That interface does not necessarily need to be the one that
holds nodeIP and thus the one that Cilium will use to send traffic
inside the k8s cluster. This patch fixes this and detects the mtu of the
interface used for cluster communications

Fixes: #10309

Autodetection of the mtu correctly detects the mtu of the interface used for the kubernetes cluster communication. The mtu was incorrectly detected in cases where multiple interfaces were present and the gateway interface was not the one used for kubernetes cluster communication

maintainer-s-little-helper · 2020-03-19T15:44:13Z

Please set the appropriate release note label.

joestringer · 2020-03-19T16:15:53Z

Travis failure seems relevant:

# github.com/cilium/cilium/daemon/cmd
vet: daemon/cmd/status_test.go:83:96: too few arguments in call to mtu.NewConfiguration
make[1]: *** [govet] Error 2
make[1]: Leaving directory `/home/travis/gopath/src/github.com/cilium/cilium'
make: *** [unit-tests] Error 2

vadorovsky · 2020-03-19T16:21:52Z

test-me-please

coveralls · 2020-03-19T16:56:23Z

Coverage increased (+0.03%) to 36.931% when pulling 9d985f1 on manuelbuil:mtuSolution into 601852b on cilium:master.

vadorovsky

One nit. Otherwise looks good.

aanm

@jrfastab PTAL as well

manuelbuil · 2020-03-20T11:53:01Z

test-me-please

aanm · 2020-03-20T14:10:59Z

test-me-please

vadorovsky · 2020-03-20T14:30:29Z

test-me-please

vadorovsky · 2020-03-20T14:31:35Z

Travis hit the flake #10615. we can ignore it

vadorovsky · 2020-03-23T10:40:59Z

test-me-please

vadorovsky · 2020-04-14T14:22:55Z

test-me-please

vadorovsky · 2020-04-15T08:14:31Z

Hitting the flake #10882

vadorovsky · 2020-04-15T08:14:38Z

test-me-please

vadorovsky

Looks good, thanks!

vadorovsky · 2020-04-15T10:42:08Z

@tgraf PTAL

vadorovsky · 2020-04-15T16:51:54Z

restart-ginkgo

joestringer · 2020-04-15T17:31:09Z

@mrostecki Be aware there are at least three different types of issues with some of the services /nodeports tests , make sure you check the exit code and ensure it matches the corresponding issue. I can't tell if the failure seen before was #10882 because there's no link any more, but that one saw exit code 7 and was only seen in PRs that updated the cilium-runtime image so I suspect this saw a different issue, such as #10942 .

joestringer · 2020-05-20T19:42:30Z

Yes, this looks to be the case:
https://jenkins.cilium.io/job/Cilium-PR-K8s-oldest-net-next/295/

Timeout has been exceeded

I'm aware there has been some provider issues combined with this particular job having more tests than the others which can lead to this situation. Given the code changes here I think it's fairly unlikely to actually introduce a kernel-specific regression on net-next; it's also passed all the other jobs so as long as we're fine with the latest code changes then this should be good to merge.

joestringer · 2020-05-20T19:43:14Z

@manuelbuil are you still making any changes? If not, please mark "ready to review".

manuelbuil · 2020-05-22T09:19:55Z

@manuelbuil are you still making any changes? If not, please mark "ready to review".

No, patch is ready to review. I can't add or remove any label, so not sure how to mark it as ready to review or change the draft status. Can you help me?

maintainer-s-little-helper · 2020-05-25T09:17:10Z

Commit ac4bb90ab113c26e642764890e7d8522d75c70a1 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

vadorovsky · 2020-05-25T09:25:19Z

test-me-please

vadorovsky · 2020-05-25T13:59:50Z

https://jenkins.cilium.io/job/Cilium-PR-K8s-newest-kernel-4.9/358 failed with

12:38:10  • Failure [298.763 seconds]
12:38:10  K8sDatapathConfig
12:38:10  /home/jenkins/workspace/Cilium-PR-K8s-newest-kernel-4.9/k8s-1.18-gopath/src/github.com/cilium/cilium/test/ginkgo-ext/scopes.go:436
12:38:10    Etcd
12:38:10    /home/jenkins/workspace/Cilium-PR-K8s-newest-kernel-4.9/k8s-1.18-gopath/src/github.com/cilium/cilium/test/ginkgo-ext/scopes.go:436
12:38:10      Check connectivity [It]
12:38:10      /home/jenkins/workspace/Cilium-PR-K8s-newest-kernel-4.9/k8s-1.18-gopath/src/github.com/cilium/cilium/test/ginkgo-ext/scopes.go:471
12:38:10  
12:38:10      cilium pre-flight checks failed
[2020-05-25T10:38:10.986Z]     Expected
[2020-05-25T10:38:10.986Z]         <*errors.errorString | 0xc00025e130>: {
[2020-05-25T10:38:10.986Z]             s: "Cilium validation failed: 4m0s timeout expired: Last polled error: connectivity health is failing: Cluster connectivity is unhealthy on 'cilium-s7rdp': Exitcode: 255 \nStdout:\n \t \nStderr:\n \t Error: Cannot get status/probe: Put \"http://%2Fvar%2Frun%2Fcilium%2Fhealth.sock/v1beta/status/probe\": context deadline exceeded\n\t \n\t command terminated with exit code 255\n\t \n",
[2020-05-25T10:38:10.986Z]         }
[2020-05-25T10:38:10.986Z]     to be nil
12:38:10  
12:38:10      /home/jenkins/workspace/Cilium-PR-K8s-newest-kernel-4.9/k8s-1.18-gopath/src/github.com/cilium/cilium/test/helpers/manifest.go:313

joestringer

Overall the idea of taking the externalIP from k8s and using that to determine which device provides 'external' connectivity (and hence governs MTU) seems like a reasonable approach. I wish there were a better way to figure this out than iterating all IPs associated with all devices, but I'm not sure the kernel gives us any better APIs.

I have a bunch of minor nits that should be easy to address below.

manuelbuil

Thanks for the review @joestringer

vadorovsky · 2020-05-29T10:19:22Z

test-me-please

vadorovsky · 2020-06-02T12:51:55Z

test-me-please

The detected MTU is always the one belonging to the interface that acts as gateway. That interface does not necessarily need to be the one that holds nodeIP and thus the one that Cilium will use to send traffic inside the k8s cluster. This patch fixes this and detects the mtu of the interface used for cluster communications The extra calculation of the correct MTU must be done before the daemon is created and thus part of the K8s initialization code must be moved before the daemon creation. The reason is that the Kubernetes IP for the node is calculated during the K8s initialization. Fixes: #10309 Signed-off-by: Manuel Buil <mbuil@suse.com>

vadorovsky · 2020-06-02T14:05:17Z

test-me-please

vadorovsky · 2020-06-03T13:29:34Z

I guess that the GKE test failure can be ignored.

manuelbuil requested review from a team March 19, 2020 15:44

maintainer-s-little-helper Bot added the dont-merge/needs-release-note label Mar 19, 2020

joestringer added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Mar 19, 2020

maintainer-s-little-helper Bot removed the dont-merge/needs-release-note label Mar 19, 2020

joestringer added dont-merge/needs-release-note labels Mar 19, 2020

maintainer-s-little-helper Bot removed the dont-merge/needs-release-note label Mar 19, 2020

vadorovsky suggested changes Mar 19, 2020

View reviewed changes

Comment thread pkg/mtu/detect_linux.go Outdated

aanm requested changes Mar 20, 2020

View reviewed changes

Comment thread pkg/mtu/detect_linux.go Outdated

aanm requested a review from jrfastab March 20, 2020 09:31

aanm approved these changes Mar 20, 2020

View reviewed changes

tgraf suggested changes Mar 24, 2020

View reviewed changes

Comment thread daemon/cmd/daemon.go Outdated

vadorovsky suggested changes Mar 30, 2020

View reviewed changes

Comment thread daemon/cmd/daemon.go Outdated

vadorovsky approved these changes Apr 15, 2020

View reviewed changes

manuelbuil marked this pull request as ready for review May 25, 2020 09:16

maintainer-s-little-helper Bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. and removed dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. labels May 25, 2020

joestringer requested changes May 27, 2020

View reviewed changes

Comment thread pkg/mtu/detect_linux.go Outdated

Comment thread pkg/mtu/detect_linux.go Outdated

Comment thread pkg/mtu/detect_linux.go Outdated

Comment thread pkg/mtu/mtu.go Outdated

Comment thread pkg/mtu/detect_linux.go Outdated

Comment thread daemon/cmd/daemon.go Outdated

manuelbuil commented May 29, 2020

View reviewed changes

Comment thread pkg/mtu/mtu.go Outdated

Comment thread daemon/cmd/daemon.go Outdated

Comment thread pkg/mtu/detect_linux.go Outdated

manuelbuil requested a review from a team as a code owner May 29, 2020 08:13

aanm requested a review from joestringer May 29, 2020 13:59

vadorovsky approved these changes Jun 2, 2020

View reviewed changes

vadorovsky added kind/bug This is a bug in the Cilium logic. area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. labels Jun 3, 2020

joestringer approved these changes Jun 3, 2020

View reviewed changes

maintainer-s-little-helper Bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 3, 2020

joestringer added the needs-backport/1.8 label Jun 3, 2020

joestringer merged commit 507d9b8 into cilium:master Jun 3, 2020

tklauser mentioned this pull request Jun 4, 2020

v1.8 backports 2020-06-04 #11893

Merged

tklauser added backport-pending/1.8 and removed needs-backport/1.8 labels Jun 4, 2020

borkmann added backport-done/1.8 and removed backport-pending/1.8 labels Jun 5, 2020

Conversation

manuelbuil commented Mar 19, 2020 • edited by joestringer Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maintainer-s-little-helper Bot commented Mar 19, 2020

Uh oh!

joestringer commented Mar 19, 2020

Uh oh!

vadorovsky commented Mar 19, 2020

Uh oh!

coveralls commented Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vadorovsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aanm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

manuelbuil commented Mar 20, 2020

Uh oh!

aanm commented Mar 20, 2020

Uh oh!

vadorovsky commented Mar 20, 2020

Uh oh!

vadorovsky commented Mar 20, 2020

Uh oh!

vadorovsky commented Mar 23, 2020

Uh oh!

Uh oh!

Uh oh!

vadorovsky commented Apr 14, 2020

Uh oh!

vadorovsky commented Apr 15, 2020

Uh oh!

vadorovsky commented Apr 15, 2020

Uh oh!

vadorovsky left a comment

Choose a reason for hiding this comment

Uh oh!

vadorovsky commented Apr 15, 2020

Uh oh!

vadorovsky commented Apr 15, 2020

Uh oh!

joestringer commented Apr 15, 2020

Uh oh!

joestringer commented May 20, 2020

Uh oh!

joestringer commented May 20, 2020

Uh oh!

manuelbuil commented May 22, 2020

Uh oh!

maintainer-s-little-helper Bot commented May 25, 2020

Uh oh!

vadorovsky commented May 25, 2020

Uh oh!

vadorovsky commented May 25, 2020

Uh oh!

joestringer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

manuelbuil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vadorovsky commented May 29, 2020

Uh oh!

vadorovsky commented Jun 2, 2020

Uh oh!

vadorovsky commented Jun 2, 2020

manuelbuil commented Mar 19, 2020 •

edited by joestringer

Loading

coveralls commented Mar 19, 2020 •

edited

Loading