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.

Sign up for GitHub

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

Jump to bottom

Configure calico MTU #7480

Merged
merged 2 commits into from
Sep 2, 2019
Merged

Configure calico MTU #7480

merged 2 commits into from
Sep 2, 2019

Conversation

zetaab
Copy link
Member

@zetaab zetaab commented Aug 28, 2019

If I am using 1440 MTU in openstack, it is not working at all. After modifying it to 1430 calico works fine.

Like this doc says the correct value for openstack is 1430:

https://docs.projectcalico.org/v3.7/networking/mtu

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 28, 2019
@zetaab zetaab changed the title Configure mtu for openstack Configure calico MTU for OpenStack Aug 28, 2019
zetaab
zetaab commented Aug 29, 2019
View reviewed changes
upup/models/cloudup/resources/addons/networking.projectcalico.org/k8s-1.12.yaml.template
# Configure the MTU to use
veth_mtu: "1440"
{{- if .Networking.Calico.MTU }}
Copy link
Member Author

@zetaab zetaab Aug 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I did not test this last commit. But if MTU is not defined in cluster spec this is not executed, right? Because MTU is *int32 it is then nil. So if someone wants to configure MTU its now possible - otherwise we use default values

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so but we should probably confirm via a test. I can probably test this in a bit.

@zetaab zetaab changed the title Configure calico MTU for OpenStack Configure calico MTU Aug 29, 2019
@zetaab zetaab force-pushed the feature/mtuopenstack branch from c818819 to 381557c Compare August 29, 2019 17:25
@zetaab
Copy link
Member Author

zetaab commented Aug 29, 2019

/assign @mikesplain

mikesplain
mikesplain reviewed Aug 30, 2019
View reviewed changes
upup/models/cloudup/resources/addons/networking.projectcalico.org/k8s-1.12.yaml.template
# Configure the MTU to use
veth_mtu: "1440"
{{- if .Networking.Calico.MTU }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think so but we should probably confirm via a test. I can probably test this in a bit.

upup/models/cloudup/resources/addons/networking.projectcalico.org/k8s-1.12.yaml.template
{{- if .Networking.Calico.MTU }}
veth_mtu: "{{ .Networking.Calico.MTU }}"
{{- else }}
veth_mtu: "{{- if eq .CloudProvider "openstack" -}}1430{{- else -}}1440{{- end -}}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Part of me wonders if this can all be avoided by doing the logic upstream of setting MTU if not set to either 1440 or 1430 depending on provider. That said I can't think of a straight forward spot to put that right now so this is probably okay.

Copy link
Member Author

@zetaab zetaab Aug 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I have tested this line and it worked well, maybe not the best solution but works

@zetaab
Copy link
Member Author

zetaab commented Aug 30, 2019 •
edited
Loading

I would like to see this in 1.15 alpha - will execute tests later today

@zetaab
Copy link
Member Author

zetaab commented Aug 30, 2019

@mikesplain

Tested with following use cases:

  1. OpenStack MTU not defined in cluster spec (default what most people using) - the MTU was 1430
  2. OpenStack MTU defined in cluster spec - the MTU was what I was waiting for
  3. AWS MTU not defined in cluster spec (default what most people using) - the MTU was 1440
  4. AWS MTU defined in cluster spec - the MTU was what I was waiting for

so everything works as should

@zetaab
Copy link
Member Author

zetaab commented Sep 1, 2019

@justinsb could you check this before doing 1.15 alpha. I just want this to be part of 1.15 alpha :)

chrisz100
chrisz100 approved these changes Sep 2, 2019
View reviewed changes
Copy link
Contributor

@chrisz100 chrisz100 left a comment

Choose a reason for hiding this comment

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

As it’s feature gated and seems important enough to get it into 1.15
/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 2, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrisz100, zetaab

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 2, 2019
@zetaab
Copy link
Member Author

zetaab commented Sep 2, 2019

/retest

@k8s-ci-robot k8s-ci-robot merged commit 41781ae into kubernetes:master Sep 2, 2019
@zetaab zetaab deleted the feature/mtuopenstack branch September 21, 2019 07:32
@zacblazic zacblazic mentioned this pull request Oct 28, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikesplain mikesplain mikesplain left review comments

@chrisz100 chrisz100 chrisz100 approved these changes

@granular-ryanbonham granular-ryanbonham Awaiting requested review from granular-ryanbonham

@robinpercy robinpercy Awaiting requested review from robinpercy

Assignees

@chrisz100 chrisz100

@mikesplain mikesplain

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zetaab @k8s-ci-robot @chrisz100 @mikesplain