Unifying MTU settings in ConfigMap #1770

ketkulka · 2018-03-18T08:37:44Z

Have a single point of MTU definition
CNI_MTU variable will set mtu for cni as well as
FELIX_IPINIPMTU
For more control, both var can still be set individually

Description

Fixes Issue #1482

Todos

Tests
Documentation
Release note

Release note

veth MTU is now configurable via the configmap in the Kubernetes self-hosted manifests

caseydavenport

A few comments, thanks!

caseydavenport · 2018-03-23T00:05:40Z

master/getting-started/kubernetes/installation/hosted/calico.yaml

@@ -21,6 +21,9 @@ data:
  # Configure the Calico backend to use.
  calico_backend: "bird"

+  # Configure the MTU to use
+  cni_mtu: "1500"


I think we just want to call this "mtu" or perhaps "veth_mtu" since it's used by both CNI and Felix.

Also, we should default to the lowest common denominator - from this page it looks like that's 1440.

It would also be great to update the command above with a link for more information.

e.g.:
"# Configure the MTU to use for container veths. See here for more information: https://docs.projectcalico.org/v3.0/usage/configuration/mtu"

caseydavenport · 2018-03-23T00:09:45Z

master/usage/configuration/mtu.md

-common network MTU size.  The default MTU for the IP-in-IP tunnel device
-is 1440 to match the value needed in GCE.
+common network MTU size.  Moreover, the default MTU for the IP-in-IP tunnel
+device is also set to 1500. For GCE deployments, `FELIX_IPINIPMTU` should


I think we want to leave this at 1440 - that's going to work in the most environments by default. For other environments, users can adjust as desired.

caseydavenport · 2018-03-23T00:10:38Z

master/usage/configuration/mtu.md

@@ -83,6 +84,9 @@ Example CNI configuration
 }
 ```

+By default, CNI configuration mtu is set to 1500 by virtue of `"mtu": __CNI_MTU__` variable.


This isn't quite right - the __CNI_MTU__ variable only affects the CNI plugin. However, the "mtu" option in the ConfigMap is passed to both the CNI plugin and Felix as well.

@caseydavenport thanks for your time for review. I have pushed the change taking care of these comments.

ketkulka

@caseydavenport thanks for your time for review. I have pushed the change taking care of these comments.

ketkulka · 2018-03-29T02:34:15Z

@caseydavenport squashed the commits here too.

caseydavenport · 2018-03-29T16:55:12Z

master/usage/configuration/mtu.md

@@ -83,6 +83,9 @@ Example CNI configuration
 }
 ```

+By default, CNI MTU and `FELIX_IPINIPMTU` derive their value from `veth_mtu` ConfigMap


I think we need a bit that says "If using a Kubernetes self-hosted manifest to install Calico, ... "

This section isn't kubernetes specific, so by adding Kubernetes specific data we need to make that clear. To be honest, we probably don't need this section at all - I think the comments in the manifests themselves should make this clear. wdyt?

caseydavenport · 2018-03-29T17:10:12Z

master/usage/configuration/mtu.md

-common network MTU size.  The default MTU for the IP-in-IP tunnel device
-is 1440 to match the value needed in GCE.
+common network MTU size. The default MTU for the IP-in-IP tunnel device
+-is 1440 to match the value needed in GCE.


is this a typo? I don't think we should have a - here.

- Have a single point of MTU definition - veth_mtu configmap variable will set mtu for cni as well as FELIX_IPINIPMTU - For more control, both var can still be set individually

ketkulka · 2018-03-29T17:52:23Z

my bad :( @caseydavenport. latest commit fixes the typo. Thanks again for being patient with too many commits.

…

On Thu, Mar 29, 2018 at 10:10 AM, Casey Davenport ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In master/usage/configuration/mtu.md <#1770 (comment)>: > @@ -28,8 +28,8 @@ packet. #### Default MTU sizes The default MTU for workload interfaces is 1500, this is to match the most -common network MTU size. The default MTU for the IP-in-IP tunnel device -is 1440 to match the value needed in GCE. +common network MTU size. The default MTU for the IP-in-IP tunnel device +-is 1440 to match the value needed in GCE. is this a typo? I don't think we should have a - here. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1770 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA_K0gHXe5RR2gaZeGDk6afHtBWE9ws5ks5tjRV1gaJpZM4SvHbE> .

caseydavenport · 2018-03-29T20:30:32Z

@ketkulka no worries, thanks for running with this!

* Most upstream changes were buried in calico#1884 which switched from non-templated manifests to templating * projectcalico/calico#1884 * projectcalico/calico#1853 * projectcalico/calico#2069 * projectcalico/calico#2032 * projectcalico/calico#1841 * projectcalico/calico#1770

ketkulka mentioned this pull request Mar 18, 2018

Exposing the IPIP tunnel value in the ConfigMap instead hardcoding in the DaemonSet #1482

Closed

ketkulka changed the title ~~Issue #1482: Unifying MTU settings in ConfigMap~~ Unifying MTU settings in ConfigMap Mar 19, 2018

caseydavenport requested changes Mar 23, 2018

View reviewed changes

ketkulka commented Mar 27, 2018

View reviewed changes

caseydavenport added this to the Calico v3.2.0 milestone Mar 27, 2018

ketkulka force-pushed the master branch from 826cefe to dfab055 Compare March 29, 2018 02:33

caseydavenport reviewed Mar 29, 2018

View reviewed changes

Unifying MTU settings in ConfigMap

c5f432f

- Have a single point of MTU definition - veth_mtu configmap variable will set mtu for cni as well as FELIX_IPINIPMTU - For more control, both var can still be set individually

ketkulka force-pushed the master branch from dfab055 to c5f432f Compare March 29, 2018 17:50

caseydavenport approved these changes Mar 29, 2018

View reviewed changes

caseydavenport added the release-note-required Change has user-facing impact (no matter how small) label Mar 29, 2018

caseydavenport merged commit 6d88a15 into projectcalico:master Mar 29, 2018

This was referenced Aug 15, 2018

Update Calico from v3.1.3 to v3.2.0 poseidon/terraform-render-bootstrap#74

Merged

Update Calico from v3.1.3 to v3.2.1 poseidon/typhoon#278

Merged

databus23 mentioned this pull request Sep 24, 2018

Calico v3.2.1 gardener/gardener#380

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unifying MTU settings in ConfigMap #1770

Unifying MTU settings in ConfigMap #1770

ketkulka commented Mar 18, 2018 •

edited by caseydavenport

Loading

caseydavenport left a comment

caseydavenport Mar 23, 2018

caseydavenport Mar 23, 2018

caseydavenport Mar 23, 2018

caseydavenport Mar 23, 2018

ketkulka Mar 27, 2018

ketkulka left a comment

ketkulka commented Mar 29, 2018

caseydavenport Mar 29, 2018

caseydavenport Mar 29, 2018

ketkulka commented Mar 29, 2018 via email

caseydavenport commented Mar 29, 2018

Unifying MTU settings in ConfigMap #1770

Unifying MTU settings in ConfigMap #1770

Conversation

ketkulka commented Mar 18, 2018 • edited by caseydavenport Loading

Description

Todos

Release note

caseydavenport left a comment

Choose a reason for hiding this comment

caseydavenport Mar 23, 2018

Choose a reason for hiding this comment

caseydavenport Mar 23, 2018

Choose a reason for hiding this comment

caseydavenport Mar 23, 2018

Choose a reason for hiding this comment

caseydavenport Mar 23, 2018

Choose a reason for hiding this comment

ketkulka Mar 27, 2018

Choose a reason for hiding this comment

ketkulka left a comment

Choose a reason for hiding this comment

ketkulka commented Mar 29, 2018

caseydavenport Mar 29, 2018

Choose a reason for hiding this comment

caseydavenport Mar 29, 2018

Choose a reason for hiding this comment

ketkulka commented Mar 29, 2018 via email

caseydavenport commented Mar 29, 2018

ketkulka commented Mar 18, 2018 •

edited by caseydavenport

Loading