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

Don't flush iptables chains when using iptables proxy but IPVS is available #75377

Closed
wants to merge 1 commit into
base: master
from

Conversation

@andrewsykim
Copy link
Member

andrewsykim commented Mar 14, 2019

Signed-off-by: Andrew Sy Kim kiman@vmware.com

What type of PR is this?
/kind bug

What this PR does / why we need it:
There's a behavior in the kubelet where if the proxier is set to iptables but the proxy can use IPVS (i.e. the ipvs kernel module is loaded), it tries to flush iptable chains used for IPVS which conflict with chains used for iptables. This impacts traffic through the proxy every time kube-proxy restarts, even though there is no cleanup required. Trying to clean up IPVS rules is nice to have here, but I don't think it's worth flushing iptable chains (especially when the proxier is set to use iptables). This PR changes the proxy to only clean up IPVS rules if --cleanup-ipvs=true.

Which issue(s) this PR fixes:
Fixes #75360

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Don't flush iptable chains when using iptables proxy but IPVS is available
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Mar 14, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: andrewsykim
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: thockin

If they are not already assigned, you can assign the PR to them by writing /assign @thockin in a comment when ready.

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 sig/network and removed needs-sig labels Mar 14, 2019

@@ -175,15 +175,15 @@ func newProxyServer(
serviceEventHandler = proxierIPTables
endpointsEventHandler = proxierIPTables
// No turning back. Remove artifacts that might still exist from the userspace Proxier.
klog.V(0).Info("Tearing down inactive rules.")
klog.V(0).Info("tearing down inactive rules.")
// TODO this has side effects that should only happen when Run() is invoked.

This comment has been minimized.

@andrewsykim

andrewsykim Mar 14, 2019

Author Member

The clean up logic here is a bit confusing and could use a clean up IMO (or maybe just get rid of it?). I can try looking into this in a follow-up PR

This comment has been minimized.

@bowei

bowei Mar 14, 2019

Member

Log messages are usually capitalized.

This comment has been minimized.

@andrewsykim

andrewsykim Mar 14, 2019

Author Member

oops, got this confused with error messages, fixed!

@k8s-ci-robot k8s-ci-robot requested review from mikedanese and thockin Mar 14, 2019

@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Mar 14, 2019

@andrewsykim andrewsykim force-pushed the andrewsykim:fix-ipvs-cleanup branch 2 times, most recently from 94fd40f to b93e521 Mar 14, 2019

@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Mar 14, 2019

/priority important-soon

@@ -175,15 +175,15 @@ func newProxyServer(
serviceEventHandler = proxierIPTables
endpointsEventHandler = proxierIPTables
// No turning back. Remove artifacts that might still exist from the userspace Proxier.
klog.V(0).Info("Tearing down inactive rules.")
klog.V(0).Info("tearing down inactive rules.")
// TODO this has side effects that should only happen when Run() is invoked.

This comment has been minimized.

@bowei

bowei Mar 14, 2019

Member

Log messages are usually capitalized.

klog.Errorf("Error flushing IPVS rules: %v", err)
encounteredError = true
}
func CleanupLeftovers(ipvs utilipvs.Interface, ipt utiliptables.Interface, ipset utilipset.Interface) (encounteredError bool) {

This comment has been minimized.

@bowei

bowei Mar 14, 2019

Member

I would prefer this function just return type error. returning nil already means encounteredError == false. Also, we should get rid of the named return; it makes the code harder to validate

This comment has been minimized.

@andrewsykim

andrewsykim Mar 14, 2019

Author Member

Agreed! Can I do this in a follow-up PR? Want to keep the scope of this change to only prevent chain flushing

ipvs.CleanupLeftovers(ipvsInterface, iptInterface, ipsetInterface, cleanupIPVS)
// IPVS proxier will generate some iptable rules, but we should only clean up those rules if cleanupIPVS
// is true, otherwise we end up flushing iptable chains that are shared between the IPVS and iptables proxier.
// Users should explicitly specify `--clean-ipvs=true` to flush all ipvs rules when kube-proxy starts up.

This comment has been minimized.

@bowei

bowei Mar 14, 2019

Member

these docs probably belong on the command line arg, rather than here? creates comment dependency with command line argument name

This comment has been minimized.

@bowei

bowei Mar 14, 2019

Member

also, the command line arg is actually --cleanup-ipvs

This comment has been minimized.

@andrewsykim

andrewsykim Mar 14, 2019

Author Member

Fixed the command line args. Hard to say if we want to expose this level of detail to users, I personally would not because this is sort of an edge case 🤔 Will defer to your judgement though.

@andrewsykim andrewsykim force-pushed the andrewsykim:fix-ipvs-cleanup branch 2 times, most recently from 24c18e6 to 88ada32 Mar 14, 2019

@thockin
Copy link
Member

thockin left a comment

This whole feature was sort of best-effort. Maybe we should just get rid of the automatic cleanup and only run that if the --cleanup flag is provided.

Basically admit defeat - switching modes has lots of side-effects and should come with a node reboot.

@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Mar 15, 2019

I agree! How do we feel about holding off on that change until v1.15 and getting this patch in for v1.14? iptables chain flushing on every restart is definitely not ideal, especially if the proxier is set to use iptables.

@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Mar 15, 2019

Created #75408 to discuss if we want to simplify the clean up logic.

@bowei bowei added this to Triage in SIG-NETWORK Mar 15, 2019

@thockin
Copy link
Member

thockin left a comment

This is a little convoluted - is it really worth it? I am OK to merge this, but what testing do we need to convnce ourselves it is safe (knowing that we want to change it again for next cycle) ?

@@ -557,7 +557,9 @@ func (s *ProxyServer) Run() error {
if s.CleanupAndExit {
encounteredError := userspace.CleanupLeftovers(s.IptInterface)
encounteredError = iptables.CleanupLeftovers(s.IptInterface) || encounteredError
encounteredError = ipvs.CleanupLeftovers(s.IpvsInterface, s.IptInterface, s.IpsetInterface, s.CleanupIPVS) || encounteredError
if s.CleanupIPVS {

This comment has been minimized.

@thockin

thockin Mar 15, 2019

Member

I don't think we need to check this here - the --cleanup flag is unilateral

This comment has been minimized.

@andrewsykim

andrewsykim Mar 15, 2019

Author Member

good point, fixed this

dont flush iptable chains when using iptables proxy but IPVS is avail…
…able

Signed-off-by: Andrew Sy Kim <kiman@vmware.com>

@andrewsykim andrewsykim force-pushed the andrewsykim:fix-ipvs-cleanup branch from 88ada32 to e4b8835 Mar 15, 2019

@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Mar 15, 2019

This is a little convoluted - is it really worth it? I am OK to merge this, but what testing do we need to convnce ourselves it is safe (knowing that we want to change it again for next cycle) ?

If we're going to fix this properly in the next release then I have no objections closing this PR :). Feel free to re-open if we feel this fix is worthwhile. It seems like this "bug" has existed in the past few releases anyway so waiting for one more release to fix this properly instead of adding more bandaids seems reasonable to me.

/close

SIG-NETWORK automation moved this from Triage to Done Mar 15, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Mar 15, 2019

@andrewsykim: Closed this PR.

In response to this:

This is a little convoluted - is it really worth it? I am OK to merge this, but what testing do we need to convnce ourselves it is safe (knowing that we want to change it again for next cycle) ?

If we're going to fix this properly in the next release then I have no objections closing this PR :). Feel free to re-open if we feel this fix is worthwhile. It seems like this "bug" has existed in the past few releases anyway so waiting for one more release to fix this properly instead of adding more bandaids seems reasonable to me.

/close

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.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Mar 15, 2019

@andrewsykim: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-kubemark-e2e-gce-big e4b8835 link /test pull-kubernetes-kubemark-e2e-gce-big

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

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.