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.

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

Remove kube-proxy autocleanup for inactive modes #76109

Merged
merged 6 commits into from Apr 5, 2019

Conversation

@vllry
Copy link
Contributor

commented Apr 3, 2019

What type of PR is this?
/kind bug

What this PR does / why we need it:
kube-proxy attempts to clean up network rules for other modes (EG cleaning up iptables rules when running in IPVS mode). The complexity of this code is prone to bugs, and can delay kube-proxy's readiness between restarts.

This PR see KEP aims to remove auto-cleanup logic for non-current kube-proxy modes. In other words, kube-proxy will only automatically clean up rules relevant to its current mode. Users should use --cleanup or restart the node when switching between kube-proxy modes.

Which issue(s) this PR fixes:
Fixes #75408 (tracking issue)
Fixes #75360 (bug caused by auto-cleanup)

Special notes for your reviewer:
Worth discussing if we gate this behavior with a flag (EG --only-clean-current-mode), or outright GA. Outright GA is currently the plan.

Does this PR introduce a user-facing change?:

kube-proxy no longer automatically cleans up network rules created by running kube-proxy in other modes. If you are switching the mode that kube-proxy is in running in (EG: iptables to IPVS), you will need to run `kube-proxy --cleanup`, or restart the worker node (recommended) before restarting kube-proxy.

If you are not switching kube-proxy between different modes, this change should not require any action.
@vllry

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2019

/priority important-soon
/sig network
/assign @thockin

@thockin
Copy link
Member

left a comment

I keep looking for something missing here, but I can't find it.

We should fix the description in cmd/kube-proxy/app/server.go on the cleanup-ipvs flag: "cleanup ipvs rules before running" is no longer true, right?

Otherwise LGTM

@vllry

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2019

We should fix the description in cmd/kube-proxy/app/server.go on the cleanup-ipvs flag: "cleanup ipvs rules before running" is no longer true, right?

Correct, I'll fix that.

We also need docs changes, which I have no idea how to coordinate.

@thockin

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

@vllry vllry changed the title WIP: Remove kube-proxy autocleanup for inactive modes Remove kube-proxy autocleanup for inactive modes Apr 4, 2019

@vllry

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

/test pull-kubernetes-e2e-gce

@thockin

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

/lgtm
/approve

@andrewsykim

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

Even though it's implied, I think the release note should say something about --cleanup-ipvs being deprecated and no longer having any effect. Should we also mark the release note for this as "action required" since we are being more explicit about users either setting --cleanup or rebooting nodes during a proxy mode switch? cc v1.14 patch release team @aleksandra-malinowska @spiffxp @tpepper since this is going into v1.14.1

@thockin

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

@andrewsykim

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

In IPVS mode --cleanup means "do low-impact cleanup and exit" while --cleanup --cleanup-ipvs means "do full cleanup and exit"

Sorry if I'm missing something, but I'm not seeing the changes in this PR reflect this 🤔 The current changes indicate --cleanup-ipvs has no effect and --cleanup will always do full clean up. Fwiw I prefer the current changes where we just deprecate --cleanup-ipvs but would like to make sure we're on the same page

/hold

@andrewsykim

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

I made the assumption that --cleanup should always do a full cleanup, by comparing to the --cleanup-iptables depreciation. I think making --cleanup do full cleanup makes the most sense. --cleanup --cleanup-ipvs=false seems like a weird scenario.

I agree with this, but I'm realizing now that this changes the scope of what --cleanup is cleaning and we may need to keep --cleanup-ipvs to preserve that. Though if --cleanup is doing cleanup AND exit, I see this unlikely to be a "breaking" change.

@vllry

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

I'm fine with keeping --cleanup-ipvs and kicking that can down the road. @thockin you're in agreement?

@thockin

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

@vllry

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

Gotcha. I'll push up a change shortly, have to run to a meeting.

@k8s-ci-robot k8s-ci-robot removed the lgtm label Apr 5, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: thockin, vllry

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

@vllry vllry force-pushed the vllry:kubeproxy-autocleanup branch from 2ae8544 to 3c99892 Apr 5, 2019

@andrewsykim

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

/lgtm

@andrewsykim

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

/hold cancel

@vllry

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

@thockin @andrewsykim issues resolved + tests finally passed, we're good to go.

@k8s-ci-robot k8s-ci-robot merged commit 71f4c9a into kubernetes:master Apr 5, 2019

17 checks passed

cla/linuxfoundation vllry authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

k8s-ci-robot added a commit that referenced this pull request Apr 5, 2019

Merge pull request #76179 from vllry/automated-cherry-pick-of-#76109-…
…upstream-release-1.14

Automated cherry pick of #76109: Removed cleanup for non-current kube-proxy modes in
@ravilr

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

@vllry @andrewsykim can this be cherry-picked to release-1.13 also please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.