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: Add a flag to set VXLAN and Geneve ports #16874

Merged
merged 3 commits into from Oct 18, 2021

Conversation

errordeveloper
Copy link
Contributor

@errordeveloper errordeveloper commented Jul 14, 2021

This change moves port defaulting logic from the kernel into the
agent and exposes a flag that lets user set a custom port.

Defaults ports remain unchanged.

Towards: #15956

@errordeveloper errordeveloper added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Jul 14, 2021
@errordeveloper errordeveloper requested a review from a team July 14, 2021 11:13
@errordeveloper errordeveloper requested review from a team as code owners July 14, 2021 11:13
@errordeveloper errordeveloper requested a review from a team as a code owner July 14, 2021 11:29
@errordeveloper errordeveloper requested review from a team and nebril July 14, 2021 11:29
bpf/init.sh Show resolved Hide resolved
@errordeveloper

This comment has been minimized.

@errordeveloper

This comment has been minimized.

bpf/init.sh Outdated Show resolved Hide resolved
pkg/datapath/loader/base.go Outdated Show resolved Hide resolved
pkg/option/config.go Outdated Show resolved Hide resolved
@errordeveloper errordeveloper force-pushed the vxlan-port branch 2 times, most recently from 80c813f to 5705342 Compare July 15, 2021 11:01
@errordeveloper errordeveloper temporarily deployed to release-base-images October 11, 2021 13:33 Inactive
@errordeveloper
Copy link
Contributor Author

/test

@errordeveloper errordeveloper temporarily deployed to release-base-images October 12, 2021 11:14 Inactive
@errordeveloper errordeveloper temporarily deployed to release-base-images October 12, 2021 11:53 Inactive
@errordeveloper errordeveloper temporarily deployed to release-base-images October 12, 2021 11:53 Inactive
@errordeveloper errordeveloper temporarily deployed to release-base-images October 12, 2021 12:32 Inactive
@errordeveloper errordeveloper temporarily deployed to release-base-images October 12, 2021 12:32 Inactive
@errordeveloper
Copy link
Contributor Author

/test

@joestringer
Copy link
Member

joestringer commented Oct 12, 2021

Upgrade question, bear with me if anyone already asked and I just missed the answer:

Commit message:

This change makes it possible for user to set a custom port
for VXLAN or Geneve. In order to anable that, the defaulting
logic was introduced in the agent, so kernel defaults are no
longer relied upon.

Default ports remain unchanged.

If we no longer rely on the kernel's default, how do we know that this won't create an upgrade regression impact? Is it because all kernels we support have the same default port? Otherwise, switching from defaulting to the kernel default to an internal value could plausibly change the port. So I think there's two questions, (1), could this change the effective port? (eg on older kernels), and (2) If yes, then what kind of network disruption would we expect during upgrade?

@errordeveloper
Copy link
Contributor Author

Upgrade question, bear with me if anyone already asked and I just missed the answer:

I the topic was touched on briefly, maybe it was on slack. I have a TODO in #15956 (comment) to do document this properly.

Commit message:

This change makes it possible for user to set a custom port
for VXLAN or Geneve. In order to anable that, the defaulting
logic was introduced in the agent, so kernel defaults are no
longer relied upon.
Default ports remain unchanged.

If we no longer rely on the kernel's default, how do we know that this won't create an upgrade regression impact?

There will be impact only if user decide changes the port on upgrade.

Is it because all kernels we support have the same default port?

From my understanding there is no global settable default. The default is per-interface, overriding it requires a very specific ip link invocation with dstport specified. Until now init.sh didn't pass dsport at all, so the default was used. The default port is also assumed by system requirements docs.

And this PR doesn't actually change the default, it only sets what's the current default port is explicitly. It's been assumed until now that the kernel's default won't change, but now if it does actually change, Cilium will keep on using the same port unless user changes or maintainers agree to change it.

Otherwise, switching from defaulting to the kernel default to an internal value could plausibly change the port.

No, as per above, default is per-interface Cilium own the interface anyway.

So I think there's two questions, (1), could this change the effective port? (eg on older kernels), and (2) If yes, then what kind of network disruption would we expect during upgrade?

  1. not expected to happen
  2. yes, and that is to be documented, but only in connection to user intentionally changing the port in an existing installation

@errordeveloper errordeveloper dismissed pchaigno’s stale review October 13, 2021 10:58

concern raised is deemed to be satisfied

@errordeveloper
Copy link
Contributor Author

errordeveloper commented Oct 14, 2021

Travis seems to run into this: 'The job exceeded the maximum log length, and has been terminated.' 🤔

I gets into an infinite loop here:

ake[2]: Entering directory '/home/travis/gopath/src/github.com/cilium/cilium/bpf/mock'
# Pre-pull FROM docker images due to Buildkit sometimes failing to pull them.
grep "^FROM " Dockerfile | cut -d ' ' -f2 | xargs -n1 docker pull
20.04: Pulling from library/ubuntu
Status: Downloaded newer image for ubuntu:20.04
docker.io/library/ubuntu:20.04
tar c Dockerfile \
 | docker build --tag cilium/bpf-mock -
 => [internal] load remote build context                                   0.0s
 => copy /context /                                                        0.1s
 => [internal] load metadata for docker.io/library/ubuntu:20.04            0.0s
 => [internal] load remote build context                                   0.0s
 => copy /context /                                                        0.1s
 => [internal] load metadata for docker.io/library/ubuntu:20.04            0.0s
 => [ 1/10] FROM docker.io/library/ubuntu:20.04                            0.0s
 => => resolve docker.io/library/ubuntu:20.04                              0.0s
 => [ 2/10] RUN apt-get update                                             0.1s
 => [internal] load remote build context                                   0.0s
 => copy /context /                                                        0.1s
 => [internal] load metadata for docker.io/library/ubuntu:20.04            0.0s
 => [ 1/10] FROM docker.io/library/ubuntu:20.04                            0.0s
 => => resolve docker.io/library/ubuntu:20.04                              0.0s
 => [ 2/10] RUN apt-get update                                             0.3s
 => [internal] load remote build context                                   0.0s
 => copy /context /                                                        0.1s
 => [internal] load metadata for docker.io/library/ubuntu:20.04            0.0s
 => [ 1/10] FROM docker.io/library/ubuntu:20.04                            0.0s
 => => resolve docker.io/library/ubuntu:20.04                              0.0s
 => [ 2/10] RUN apt-get update                                             0.4s
 => [internal] load remote build context                                   0.0s
 => copy /context /                                                        0.1s
 => [internal] load metadata for docker.io/library/ubuntu:20.04            0.0s
 => [ 1/10] FROM docker.io/library/ubuntu:20.04                            0.0s
 => => resolve docker.io/library/ubuntu:20.04                              0.0s
 => [ 2/10] RUN apt-get update                                             0.6s
...

This change makes it possible for user to set a custom port
for VXLAN or Geneve. In order to anable that, the defaulting
logic was introduced in the agent, so kernel defaults are no
longer relied upon.

Default ports remain unchanged.

Towards: cilium#15956

Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>
xref cilium/image-tools#136
xref isovalent/iproute2#14

Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>
This is to incorporate new iproute2 image

Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>
@errordeveloper errordeveloper temporarily deployed to release-base-images October 14, 2021 13:38 Inactive
@errordeveloper errordeveloper temporarily deployed to release-base-images October 14, 2021 13:38 Inactive
@errordeveloper
Copy link
Contributor Author

/test

@aanm aanm merged commit c3cac3e into cilium:master Oct 18, 2021
@errordeveloper errordeveloper deleted the vxlan-port branch October 19, 2021 10:39
@llhhbc
Copy link
Contributor

llhhbc commented Dec 22, 2021

good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants