Remove the flannel experimental overlay #33862

luxas · 2016-09-30T20:33:22Z

What this PR does / why we need it:

It removes the deprecated flannel overlay integration in kubelet.
It's completely unnecessary now with CNI which can handle everything smoothly when flannel is running in a daemonset.

Which issue this PR fixes (optional, in fixes #<issue number>(, #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #17795, #30589

cc @kubernetes/sig-network @thockin @freehan @bprashanth @yujuhong @dchen1107

This change is

k8s-ci-robot · 2016-09-30T20:37:06Z

Jenkins GCE e2e failed for commit c99f586. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-09-30T20:37:06Z

Jenkins GCI GCE e2e failed for commit c99f586. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-09-30T20:37:47Z

Jenkins GCE Node e2e failed for commit c99f586. Full PR test history.

The magic incantation to run this job again is @k8s-bot node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

luxas · 2016-09-30T20:52:23Z

I can also remove the proposal if you like

yujuhong · 2016-09-30T21:01:17Z

@luxas you need to run hack/update-all.sh to update the generated files.

luxas · 2016-09-30T21:04:10Z

Yes, I know :)
I just uploaded the first commit while hack/update-all.sh was running.

@yujuhong PTAL at the first commit

yujuhong · 2016-09-30T21:22:39Z

I just uploaded the first commit while hack/update-all.sh was running.

Fair enough. It does take quite a while to run.

luxas · 2016-09-30T21:24:07Z

@yujuhong What do you think about the PR itself?

yujuhong · 2016-09-30T21:29:10Z

Salt needs some cleanup too:

kubernetes/cluster/saltbase/salt/kubelet/default

Line 107 in 51198f5

{% set experimental_flannel_overlay = "--experimental-flannel-overlay=true" %}

freehan · 2016-09-30T21:29:39Z

@luxas Thanks for the PR!

Just the quota exceeded.

ERROR: (gcloud.compute.addresses.create) Some requests did not succeed:
 - Quota 'STATIC_ADDRESSES' exceeded. Limit: 20.0

k8s-ci-robot · 2016-09-30T21:29:44Z

Jenkins verification failed for commit b251dd5. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

yujuhong · 2016-09-30T21:32:38Z

@yujuhong What do you think about the PR itself?

I'm fine with removing this flag, and in general happy to see dead code go. Someone from the cluster team may want to chime in.

luxas · 2016-09-30T21:33:19Z

Should I remove the old proposal as well: https://github.com/kubernetes/kubernetes/blob/master/docs/proposals/flannel-integration.md?

luxas · 2016-09-30T21:48:38Z

I'm testing now to remove the old flannel code in kube-up as well.

cc @roberthbailey @mikedanese @zmerlynn from the cluster team, PTAL

yujuhong · 2016-09-30T22:02:06Z

pkg/kubelet/kubelet.go

-		containerManager:           kubeDeps.ContainerManager,
-		nodeIP:                     net.ParseIP(kubeCfg.NodeIP),
-		clock:                      clock.RealClock{},
+		os:                kubeDeps.OSInterface,


why changing this?

It's just gofmt. I removed the two flannel overlay-specific fields

k8s-ci-robot · 2016-09-30T22:03:11Z

Jenkins Kubemark GCE e2e failed for commit a6ab75d. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-09-30T22:03:41Z

Jenkins GCI GKE smoke e2e failed for commit a6ab75d. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-09-30T22:04:11Z

Jenkins GKE smoke e2e failed for commit a6ab75d. Full PR test history.

The magic incantation to run this job again is @k8s-bot gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

yujuhong · 2016-09-30T22:09:17Z

Should I remove the old proposal as well: https://github.com/kubernetes/kubernetes/blob/master/docs/proposals/flannel-integration.md?

I don't know the policy on proposals, but I think removing a no-longer-relevant proposal is good.

mikedanese · 2016-09-30T22:34:14Z

pkg/kubelet/kubelet_network.go

@@ -121,7 +121,7 @@ func (kl *Kubelet) providerRequiresNetworkingConfiguration() bool {
 	// is used or whether we are using overlay networking. We should return
 	// true for cloud providers if they implement Routes() interface and
 	// we are not using overlay networking.
-	if kl.cloud == nil || kl.cloud.ProviderName() != "gce" || kl.flannelExperimentalOverlay {
+	if kl.cloud == nil || kl.cloud.ProviderName() != "gce" {


we still want a flag that does this probably. flannelExperimentalOverlay is the wrong name.

One can use the CNI flannel plugin. But that needs extra setup for flannel daemon and etc.

I think that is much cleaner than having a flag for flannel.

At least, this isn't a regression.
Nobody (at least not many) has used the experimental flannel overlay, and if we want to add a new flag for this specific case, we can do it later

@freehan I think we have the same problem for not just flannel. If I'm using the gce cloud provider with weave and I don't want to configure routes, kubelet will report not ready. There is no way of opting out of gce routes with the gce cloud provider. Is that an issue?

@mikedanese what does kubelet complain in the weave case? I do not recall kubelet checks podCIDR unless it comes from network plugin. I think the assigned route can be ignored by configuration.

I think you should open a separate issue for that discussion.
Please do that and keep focusing on the PR itself

I brought this up in #33573 (comment). It's a problem for OpenShift when run on GCE too, as we use an overlay and don't need Routes created for pod<->pod communication, but the NetworkUnavailable condition is set anyway.

But I think this should be solved differently and more cleanly than repurposing the flannelExperimentalOverlay flag.

mikedanese · 2016-09-30T22:35:21Z

Should I remove the old proposal as well:

No. You can just leave it as is or make a not that it's deprecated.

luxas · 2016-10-03T19:38:31Z

@freehan Any concerns/comments?
If this looks good to you and @yujuhong I'll just rebase and make it ready to merge

freehan · 2016-10-03T23:10:06Z

LGTM
Thanks!

yujuhong · 2016-10-03T23:14:11Z

LGTM. Please rebase.

freehan · 2016-10-04T00:19:23Z

Feel free to rebase and self apply

k8s-ci-robot · 2016-10-04T10:09:30Z

Jenkins unit/integration failed for commit 4e52f84. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

luxas · 2016-10-04T10:30:32Z

@k8s-bot unit test this

k8s-github-robot · 2016-10-05T04:03:06Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2016-10-05T04:39:20Z

Automatic merge from submit-queue

googlebot added the cla: yes label Sep 30, 2016

luxas added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Sep 30, 2016

k8s-github-robot assigned yujuhong Sep 30, 2016

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 30, 2016

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 30, 2016

yujuhong reviewed Sep 30, 2016

View reviewed changes

mikedanese reviewed Sep 30, 2016

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 1, 2016

yujuhong assigned freehan Oct 3, 2016

luxas added 3 commits October 4, 2016 11:53

Remove the flannel experimental overlay

348717c

Remove the old flannel network mode in kube-up

950e237

Run hack/update-all.sh

4e52f84

luxas force-pushed the remove_experimental_flannel branch from a6ab75d to 4e52f84 Compare October 4, 2016 09:45

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 4, 2016

luxas added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2016

k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 4, 2016

k8s-github-robot merged commit 457d378 into kubernetes:master Oct 5, 2016

bboreham mentioned this pull request Oct 6, 2016

AWS: network providers required to update NetworkUnavailable #33573

Closed

Remove the flannel experimental overlay #33862

Remove the flannel experimental overlay #33862

Conversation

luxas commented Sep 30, 2016 • edited by k8s-oncall Loading

k8s-ci-robot commented Sep 30, 2016

k8s-ci-robot commented Sep 30, 2016

k8s-ci-robot commented Sep 30, 2016

luxas commented Sep 30, 2016

yujuhong commented Sep 30, 2016

luxas commented Sep 30, 2016

yujuhong commented Sep 30, 2016

luxas commented Sep 30, 2016

yujuhong commented Sep 30, 2016

freehan commented Sep 30, 2016

k8s-ci-robot commented Sep 30, 2016

yujuhong commented Sep 30, 2016

luxas commented Sep 30, 2016

luxas commented Sep 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 30, 2016

k8s-ci-robot commented Sep 30, 2016

k8s-ci-robot commented Sep 30, 2016

yujuhong commented Sep 30, 2016

Choose a reason for hiding this comment

freehan Sep 30, 2016 • edited Loading

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

Choose a reason for hiding this comment

mikedanese commented Sep 30, 2016

luxas commented Oct 3, 2016

freehan commented Oct 3, 2016

yujuhong commented Oct 3, 2016

freehan commented Oct 4, 2016

k8s-ci-robot commented Oct 4, 2016

luxas commented Oct 4, 2016

k8s-github-robot commented Oct 5, 2016

k8s-github-robot commented Oct 5, 2016

luxas commented Sep 30, 2016 •

edited by k8s-oncall

Loading

freehan Sep 30, 2016 •

edited

Loading