Have CNI assume mtu from eth0 #1690

faiq · 2020-06-25T20:57:33Z

As per my conversation with @aojea in the following issue We figured the best path going forward would be to auto configure the MTU of kindnet from the host's device

This pull request replaces the one here
#1686

k8s-ci-robot · 2020-06-25T20:57:42Z

Hi @faiq. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

images/kindnetd/cmd/kindnetd/cni.go

aojea · 2020-06-25T22:33:19Z

images/kindnetd/cmd/kindnetd/cni.go

@@ -28,27 +29,49 @@ import (
 	"k8s.io/utils/net"
 )

+//sensibleMTU value to fall back on - if we're not able to configure from eth0
+const defaultMtu = 1480


why do you think we should change the default?

We could allow the kernel to set the default, as documented here: https://github.com/containernetworking/plugins/tree/master/plugins/main/ptp#example-network-configuration

I've typically found that mtu is configured to 1500, which has been problematic for many people.

Maybe it should be 1400 instead.

I prefer to keep current behaviour, and allow the kernel to set it

aojea · 2020-06-26T19:56:59Z

/ok-to-test

BenTheElder · 2020-06-30T02:18:15Z

explicit +1 on the approach, btw, since I neglected to put that in github anywhere.
will take a closer look soon.

aojea · 2020-06-30T11:21:23Z

ping @faiq , it's only missing not defaulting the mtu to 1480, maybe just not inserting that field in the json if we can't get the MTU value from eth0 is enough.

please disregard my other comment #1690 (comment)

faiq · 2020-06-30T17:06:54Z

Thanks for the ping @aojea. I'll be completing this one today. I believe we just need to template it if we get an error from getting the MTU from eth0. We might need to log this error as well for anyone looking to debug why their MTU isn't being set. Thoughts?

aojea · 2020-06-30T17:28:14Z

Thanks for the ping @aojea. I'll be completing this one today. I believe we just need to template it if we get an error from getting the MTU from eth0. We might need to log this error as well for anyone looking to debug why their MTU isn't being set. Thoughts?

yeah, that sounds nice, what about something like

klog.Infof("Failed to get MTU size from interface eth0, using kernel default MTU size")

aojea · 2020-06-30T19:09:58Z

images/kindnetd/cmd/kindnetd/cni.go

 }

 // ComputeCNIConfigInputs computes the template inputs for CNIConfigWriter
 func ComputeCNIConfigInputs(node corev1.Node) CNIConfigInputs {
 	podCIDR := node.Spec.PodCIDR
 	defaultRoute := "0.0.0.0/0"
+	mtu, err := computeBridgeMTU()


@BenTheElder if I read it correctly this is going to be executed periodically, can you confirm?

I don't like the idea of iterating over the interfaces periodically, it can cause issues, I had issue with the net library before.
Should be better to try to get the mtu only once? in the main loop? and pass it as a parameter of ComputeCNIConfigInputs?

Actually, yes it will be computed multiple times and I think it's a more sound approach to compute this once.

faiq · 2020-07-01T03:13:24Z

/retest

BenTheElder · 2020-07-01T04:34:12Z

images/kindnetd/cmd/kindnetd/cni.go

+
+var mtu int
+
+func init() {


init() and global data is a big anti-pattern.
mtu should be plumbed down from main.

I didn't realize this was an anti-pattern, sorry. If you have a quick article on best practices, I'd love to read it! If not, no sweat.

I can pull it down from main, but I think it would need to go in makeNodesReconciler as an extra parameter which then propagates to the ComputeCNIConfigInputs function.

Alternatively we can put mtu as a field in CNIConfigWriter and in this method func (c *CNIConfigWriter) Write(inputs CNIConfigInputs)

we can have inputs.Mtu = c.mtu

Let me know what you would prefer.

I lean to the CNIConfigWriter since mtu only matter to that type.
what do you think Ben?

no sweat :-)
I would say "avoid globals" is a good practice for all languages, I don't have a reference handy just this moment though, sorry.

init is best avoided in code because it's both global behavior and unpredictable import side-effects (importing the package causes this code to be run during the binary startup, in some random order versus other packages)

CNIConfigWriter makes sense to me 👍

https://stackoverflow.com/a/56039373 is a pretty decent short reference on this point specifically 😅

faiq · 2020-07-01T19:31:57Z

/retest

aojea · 2020-07-01T20:14:48Z

images/kindnetd/cmd/kindnetd/cni.go

@@ -76,6 +94,9 @@ const cniConfigTemplate = `
 			]
 		]
 		}
+		{{if .Mtu}},


if the eth0 interface is not found mtu will be 0,
and {{if .Mtu}}, will skip this based on the go template docs

{{if pipeline}} T1 {{end}} If the value of the pipeline is empty, no output is generated; otherwise, T1 is executed. The empty values are false, 0, any nil pointer or interface value, and any array, slice, map, or string of length zero. Dot is unaffected.

Yes, and then it will default to the ptp behavior of being set by the kernel. I thought that's what we wanted #1690 (comment)

In the docs it says it is an optional field and will be set by Kernel if its not there: https://github.com/containernetworking/plugins/tree/master/plugins/main/ptp#network-configuration-reference

Network configuration reference name (string, required): the name of the network type (string, required): "ptp" ipMasq (boolean, optional): set up IP Masquerade on the host for traffic originating from ip of this network and destined outside of this network. Defaults to false. mtu (integer, optional): explicitly set MTU to the specified value. Defaults to value chosen by the kernel. ipam (dictionary, required): IPAM configuration to be used for this network. dns (dictionary, optional): DNS information to return as described in the Result.

right, perfect

aojea · 2020-07-01T20:21:28Z

this looks good now, thanks for your patience.
Do you mind squashing all commits?

faiq · 2020-07-01T20:24:35Z

I can squash all the commits, sure.

aojea · 2020-07-01T20:58:08Z

/retest
/lgtm
thanks

faiq · 2020-07-02T01:24:21Z

/retest

aojea · 2020-07-02T08:02:48Z

/retest

aojea · 2020-07-02T08:56:46Z

tested locally and works

3: eth0@if5: <BROADCAST,MULTICAST,UP,LOWER_UP,M-DOWN> mtu 1400 qdisc noqueue

root@kind-control-plane:/# crictl logs f438b5bc17747
I0702 08:52:26.361506       1 main.go:65] hostIP = 172.18.0.2
podIP = 172.18.0.2
I0702 08:52:26.361748       1 main.go:74] setting mtu 1400 for CNI

BenTheElder · 2020-07-08T02:00:56Z

Sorry I was out the end of last week (holidays at employer / USA). Looking at this again now.

BenTheElder · 2020-07-08T02:39:30Z

rebased to HEAD and pushed a kindnetd image
/lgtm
/approve

k8s-ci-robot · 2020-07-08T02:39:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, faiq

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [BenTheElder]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

BenTheElder · 2020-07-08T03:27:02Z

/retest
Test pod didn't start. CI issue.

BenTheElder · 2020-07-08T04:17:18Z

thanks for this!

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 25, 2020

k8s-ci-robot requested review from aojea and munnerz June 25, 2020 20:57

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 25, 2020

aojea reviewed Jun 25, 2020

View reviewed changes

images/kindnetd/cmd/kindnetd/cni.go Show resolved Hide resolved

BenTheElder assigned BenTheElder and aojea Jun 25, 2020

aojea reviewed Jun 25, 2020

View reviewed changes

images/kindnetd/cmd/kindnetd/cni.go Show resolved Hide resolved

aojea reviewed Jun 25, 2020

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 26, 2020

aojea reviewed Jun 30, 2020

View reviewed changes

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 30, 2020

BenTheElder reviewed Jul 1, 2020

View reviewed changes

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 1, 2020

aojea reviewed Jul 1, 2020

View reviewed changes

faiq force-pushed the configure-cni-mtu branch from cb1d931 to d446edc Compare July 1, 2020 20:25

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 1, 2020

faiq requested a review from BenTheElder July 7, 2020 17:46

faiq and others added 2 commits July 7, 2020 19:17

Have CNI assume mtu from eth0

3aec2c7

bump kindnetd

a0e2d0e

BenTheElder force-pushed the configure-cni-mtu branch from d446edc to a0e2d0e Compare July 8, 2020 02:37

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 8, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 8, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 8, 2020

k8s-ci-robot merged commit 52d5c77 into kubernetes-sigs:master Jul 8, 2020

BenTheElder mentioned this pull request Jul 8, 2020

Allow users to configure MTU on kindnet #1468

Closed

faiq mentioned this pull request Feb 21, 2023

REQUEST: New membership for faiq kubernetes/org#4031

Closed

9 tasks

faiq mentioned this pull request Feb 5, 2024

REQUEST: New membership for faiq kubernetes/org#4733

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Have CNI assume mtu from eth0 #1690

Have CNI assume mtu from eth0 #1690

faiq commented Jun 25, 2020

k8s-ci-robot commented Jun 25, 2020

aojea Jun 25, 2020

faiq Jun 25, 2020

aojea Jun 26, 2020

aojea commented Jun 26, 2020

BenTheElder commented Jun 30, 2020

aojea commented Jun 30, 2020

faiq commented Jun 30, 2020

aojea commented Jun 30, 2020

aojea Jun 30, 2020

faiq Jun 30, 2020

faiq commented Jul 1, 2020

BenTheElder Jul 1, 2020

faiq Jul 1, 2020

aojea Jul 1, 2020

BenTheElder Jul 1, 2020

BenTheElder Jul 1, 2020

faiq commented Jul 1, 2020

aojea Jul 1, 2020

faiq Jul 1, 2020 •

edited

Loading

aojea Jul 1, 2020

aojea commented Jul 1, 2020

faiq commented Jul 1, 2020

aojea commented Jul 1, 2020

faiq commented Jul 2, 2020

aojea commented Jul 2, 2020

aojea commented Jul 2, 2020

BenTheElder commented Jul 8, 2020

BenTheElder commented Jul 8, 2020

k8s-ci-robot commented Jul 8, 2020

BenTheElder commented Jul 8, 2020

BenTheElder commented Jul 8, 2020

Have CNI assume mtu from eth0 #1690

Have CNI assume mtu from eth0 #1690

Conversation

faiq commented Jun 25, 2020

k8s-ci-robot commented Jun 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Jun 26, 2020

BenTheElder commented Jun 30, 2020

aojea commented Jun 30, 2020

faiq commented Jun 30, 2020

aojea commented Jun 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faiq commented Jul 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faiq commented Jul 1, 2020

Choose a reason for hiding this comment

faiq Jul 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Jul 1, 2020

faiq commented Jul 1, 2020

aojea commented Jul 1, 2020

faiq commented Jul 2, 2020

aojea commented Jul 2, 2020

aojea commented Jul 2, 2020

BenTheElder commented Jul 8, 2020

BenTheElder commented Jul 8, 2020

k8s-ci-robot commented Jul 8, 2020

BenTheElder commented Jul 8, 2020

BenTheElder commented Jul 8, 2020

faiq Jul 1, 2020 •

edited

Loading