Make Weave MTU configurable and configure jumbo frame support for new clusters on AWS

[jordanjennings]

Pull request for #2600 to make MTU configurable and to set a good default value.

From the discussion in #1171 it looks like nonMasqueradeCIDR is meant for the overlay CIDR, so I added that to the configuration as well.

New config looks like:

spec:
  networking:
    weave:
      mtu: 8912

I tested the following scenarios with this change:

• Editing an existing cluster and adding mtu: 8912, then running update cluster. The ManagedFile for weave was updated as expected, however I am not sure what the process would be to actually get the settings rolled out.
• Creating a new cluster with command line flags (it sets mtu: 8912 as expected)
• Creating a new cluster with kops create -f mycluster.yml based on https://github.com/kubernetes/kops/blob/master/tests/integration/privateweave/in-v1alpha2.yaml with minor edits

Todo:

• Update docs

Let me know if there's anything else I need to do.

---

This change is 

[k8s-ci-robot]

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.

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. I understand the commands that are listed here.

[k8s-ci-robot]

Hi @jordanjennings. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot 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.

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. I understand the commands that are listed here.

[jordanjennings]

CLA signed

[chrislovecnm]

So the reason that we have not dropped in the change to use NonMasq yet is because there are questions about upgrading. Can you test that upgrading from the old yaml to the new yaml using the NonMasq CIDR works ok? Or just pull it from the PR.

[chrislovecnm]

@k8s-bot ok to test

[jordanjennings]

@chrislovecnm I tested editing an existing cluster and after running update both environment variables were added to k8s-1.6.yaml and pre-k8s-1.6.yaml:

          env:
            - name: IPALLOC_RANGE
              value: 100.64.0.0/10
            - name: WEAVE_MTU
              value: 8912

Does that cover it?

[chrislovecnm]

Where old deployments happy? And new deployments as well?

[jordanjennings]

@chrislovecnm Is there any existing mechanism to replace the CNI DaemonSets with rolling updates? It would be similar to the process for switching CNI providers. Is that a supported change via kops currently?

[chrislovecnm]

A good use case is create a cluster with kops 1.5.x & k8s 1.5.5 and do a rolling-update with your pr version of kops to 1.5.7

We have some issues with 1.5 -> 1.6 k8s upgrades right now, that is why I would recommend 1.5 k8s.

The kops update and kops rolling update will apply the new weave deamonsets.

Have an example app running before and install another example app afterwards. Vadiate both function. And both can ping www.google.com.

This is a bunch of testing work, and if you can do it we very much appreciate it.

[chrislovecnm]

To add some context, this PR is great. If you do not have time to test the CIDR stuff, please remove it and leave the MTU stuff in. Happy to merge the MTU stuff, or if you want to do the above testing we can merge the CIDR and MTU settings.

[jordanjennings]

@chrislovecnm I tested as you mentioned by using kops 1.5.3 with k8s 1.5.5 to deploy a new cluster, which worked fine, deployed sample apps, no issues. I checked in S3 and saw the v1.9.3.yaml file for weave as expected.

Then I used my branch version of kops to edit the cluster and update k8s to 1.5.7 and set the weave MTU to 8912. In the kops update cluster it updated the bootstrap-channel.yaml file to reference the new weave files pre-k8s-1.6.yaml and k8s-1.6.yaml, but didn't actually put those files in the networking.weave folder in S3. I did a rolling update which worked to get k8s to 1.5.7 but did not deploy the new weave files (presumably because they weren't in S3?).

Seems like this is actually an existing issue with kops 1.6.0 and upgrading weave. Suggestions from here?

[chrislovecnm]

You do a rolling update? Ping me on slack on Monday, or we can setup a call

[chrislovecnm]

@k8s-bot ok to test

[k8s-ci-robot]

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.

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. I understand the commands that are listed here.

[jordanjennings]

Found an issue when upgrading a cluster without specifying MTU, it puts nil into the YAML. When someone has created a cluster with an older version of kops they will have a configuration like:

networking:
  weave: {}

With my change, a kops update will set WEAVE_MTU to :

ManagedFile/k8s.fscom.clouddev.thermofisher.net-addons-networking.weave-k8s-1.6
  	Contents            
  	                    	...
  	                    	                cpu: 100m
  	                    	                memory: 200Mi
  	                    	+           env:
  	                    	+             - name: IPALLOC_RANGE
  	                    	+               value: 100.64.0.0/10
  	                    	+             - name: WEAVE_MTU
  	                    	+               value: "<nil>"
  	                    	          - name: weave-npc
  	                    	            image: weaveworks/weave-npc:1.9.4

I am setting the default value to 8912 in create_cluster.go, but if the cluster has already been created then that code doesn't apply.

Two options:

• Don't change the MTU for cluster upgrades unless the configuration is specifically added. This has the benefit of being the least surprising to someone doing an upgrade, but also means they will have to manually discover this change and apply it themselves.
• Somehow always default MTU to 8912 if it isn't set in the cluster configuration. I would need a pointer where to make this code change.

Thoughts?

[k8s-ci-robot]

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.

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. I understand the commands that are listed here.

[chrislovecnm]

Not sure what happened with the CLA on last rebase. Did you can your user info?

Also this fixes #2085 as well

[chrislovecnm]

Let me change my mind. Let's put it in the manifest. Also, do all instance types in aws support that MTU?

[jordanjennings]

Ok, upon further testing I'm dropping the IPALLOC_RANGE part of this pull request. Somehow it worked once, but on every other test it has broken the networking and I'm not sure why. When testing a cluster upgrade it makes it so the pod to pod networking does not work, it just says "No route to host" or "Destination Host Unreachable" for pings or curl requests.

I tested many new cluster creation and cluster upgrade scenarios and in all cases except one the networking was totally broken. I'm not even sure at this point about the one case where I thought it was working, that may have been a bad scenario.

My most recent test was simply creating a new cluster with the default configuration with this branch (meaning MTU and IP CIDR was set) and even in that case pod networking was totally broken. I'm not sure why that is the case, but I am dropping the IP address change and will be testing now with the MTU change only.

[jordanjennings]

@chrislovecnm Per AWS docs:

The following instances support jumbo frames:

Compute optimized: C3, C4, CC2
General purpose: M3, M4, T2
Accelerated computing: CG1, F1, G2, P2
Memory optimized: CR1, R3, R4, X1
Storage optimized: D2, HI1, HS1, I2, I3

Also:

outside of a given AWS region (EC2-Classic), a single VPC, or a VPC peering connection, you will experience a maximum path of 1500 MTU

Should we reduce potential issues by not trying to make this the default? I just want it to be configurable somehow, I don't really care personally about it being the default unless others agree that it's a sensible thing to do for everyone.

[chrislovecnm]

@ottoyiu both you and @jordanjennings are going to need to check what cloud provider a kops cluster is living on, and apply the manifests accordingly. It may be good for both of you to agree upon which code to use for checking the cloud provider, so merging is easier.

[jordanjennings]

OK, final updates pushed. I decided to make the MTU set to jumbo frames only for newly created clusters with this pull request. I am checking for AWS, and in order to do that I had to move down the networking section in create_cluster.go to fall under where it detects the cloud provider. I had to increment the weave version in boostrapchannelbuilder.go in order to get the S3 file updated and DaemonSet updated.

Here is what I tested:

• New cluster with this branch, started correctly, and guestbook working. Validated MTU setting in S3 state store and in weave-net pods, set to 8912 as expected.
• Cluster created with kops 1.6.0 and k8s 1.6.0, cluster started and guestbook validated. Then used this branch to upgrade to k8s 1.6.2 and manually set MTU to 8912. Did an apply and rolling update, then validated S3 state store and weave-net pod, both showed MTU to 8912 as expected. Existing guestbook deployment worked properly, did a new guestbook deployment and it also worked properly.

[chrislovecnm]

A couple of tweaks.

[chrislovecnm]

This is still allowing other cloud providers to use this feature, but we can do a follow-up pr.

[chrislovecnm]

So this is a example how to use --networking cni - maybe we need to rewrite this ;) Please blackout your changes.

[jordanjennings]

Yea I found this networking page pretty confusing as newcomer, was trying to help clarify but maybe it's more confusing now :)

[chrislovecnm]

Let's clean it up in another PR ;)

[chrislovecnm]

Does 1.9.4-kops.1 work?

[jordanjennings]

Per http://semver.org/#spec-item-9 things with labels after the version are considered "pre-release" and have lower precedence than the final version number. So 1.9.4-kops.1 would be treated like a pre-release version of 1.9.4, and 1.9.4 would take precedence.

[chrislovecnm]

@k8s-bot pull-kops-e2e-kubernetes-aws test this

[chrislovecnm]

Thanks

[chrislovecnm]

/lgtm

Pending on a question for the author