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

Jump to bottom

[Release 3.11] Bug 1795416: Multiple iptables improvements for kubelet and kube-proxy #24748

Merged
merged 12 commits into from Sep 3, 2020
Merged

[Release 3.11] Bug 1795416: Multiple iptables improvements for kubelet and kube-proxy #24748

merged 12 commits into from Sep 3, 2020

Conversation

JacobTanenbaum
Copy link
Contributor

@JacobTanenbaum JacobTanenbaum commented Mar 23, 2020 •
edited

A series of backported commits that improves kubelet and kube-proxy performance

https://bugzilla.redhat.com/show_bug.cgi?id=1795416

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 23, 2020
@danwinship
Copy link
Contributor

manually add the file vendor/k8s.io/apimachinery/pkg/util/version/version.go to iptables

oh, it's just in a different place in 3.11; k8s.io/kubernetes/pkg/util/version

@juanluisvaladas
Copy link
Contributor

The only thing that I find unexpected is: d4fe81c but I'm guessing this was done to avoid conflicts and seems safe, so looks good.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 24, 2020
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 25, 2020
@JacobTanenbaum
Copy link
Contributor Author

@danwinship removed the manual addition of the apimachinery code in favour of the code in k8s.io/kubernetes/pkg/util/version

@danwinship
Copy link
Contributor

from the unit test failure:

[WARNING] `go test` had the following output to stderr:
[WARNING] # github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/proxy/ipvs
[WARNING] vendor/k8s.io/kubernetes/pkg/proxy/ipvs/proxier_test.go:657:23: too many arguments in call to NewFakeProxier
[WARNING] 	have (*"github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/util/iptables/testing".FakeIPTables, *"github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/util/ipvs/testing".FakeIPVS, *"github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/util/ipset/testing".FakeIPSet, nil, nil, bool)
[WARNING] 	want (iptables.Interface, ipvs.Interface, ipset.Interface, []"net".IP)

@danwinship
Copy link
Contributor 

UPSTREAM 80368: kube-proxy: drop iptables version check
...
MANUAL EDITS: did not change
    vendor/k8s.io/kubernetes/pkg/proxy/iptables/BUILD

couldn't figure out how the changes applied...

The upstream patch removes the line

"//staging/src/k8s.io/apimachinery/pkg/util/version:go_default_library",`

you want to remove the line:

"//pkg/util/version:go_default_library",

UPSTREAM 80368: iptables: simplify version handling

Likewise with vendor/k8s.io/kubernetes/pkg/util/iptables/BUILD here

UPSTREAM: 67690: Kubelet: only sync iptables on linux

I would drop this one; it should be easy enough to just move the fixes in the later PRs from kubelet_network_linux.go to kubelet_network.go

UPSTREAM 64831: Address a TODO, move to lazy initialization of the firewallD signal handler

and you can just squash this one into "Drop iptables firewalld monitoring support"

UPSTREAM 78547: Made IPVS and iptables modes of kube-proxy fully randomize masquerading if possible

I would also drop this. That's a functionality change which is nice and all but we didn't intend to be backporting it and we shouldn't backport it just to make the remaining PRs merge more cleanly.

UPSTREAM 81517: Add iptables.Monitor, use it from kubelet and
 kube-proxy

(please fix to keep the whole title on one line so it shows up right in git log --oneline etc)

@JacobTanenbaum
Copy link
Contributor Author

/retest

@JacobTanenbaum JacobTanenbaum changed the title [WIP] [Release 3.11] Mutliple iptables improvements for kubelet and kube-proxy [Release 3.11] Bug 1727441: Mutliple iptables improvements for kubelet and kube-proxy Mar 30, 2020
@openshift-ci-robot openshift-ci-robot added bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Mar 30, 2020
@openshift-ci-robot
Copy link

@JacobTanenbaum: This pull request references Bugzilla bug 1727441, which is invalid:

  • expected the bug to be open, but it isn't
  • expected the bug to target the "3.11.z" release, but it targets "4.4.0" instead
  • expected the bug to be in one of the following states: NEW, ASSIGNED, ON_DEV, POST, POST, but it is CLOSED (DUPLICATE) instead
  • expected Bugzilla bug 1727441 to depend on a bug in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but no dependents were found

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

[Release 3.11] Bug 1727441: Mutliple iptables improvements for kubelet and kube-proxy

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.

1 similar comment
@openshift-ci-robot
Copy link

@JacobTanenbaum: This pull request references Bugzilla bug 1727441, which is invalid:

  • expected the bug to be open, but it isn't
  • expected the bug to target the "3.11.z" release, but it targets "4.4.0" instead
  • expected the bug to be in one of the following states: NEW, ASSIGNED, ON_DEV, POST, POST, but it is CLOSED (DUPLICATE) instead
  • expected Bugzilla bug 1727441 to depend on a bug in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but no dependents were found

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

[Release 3.11] Bug 1727441: Mutliple iptables improvements for kubelet and kube-proxy

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.

@JacobTanenbaum JacobTanenbaum changed the title [Release 3.11] Bug 1727441: Mutliple iptables improvements for kubelet and kube-proxy [Release 3.11] Bug 1795416 : Mutliple iptables improvements for kubelet and kube-proxy Mar 30, 2020
@openshift-ci-robot openshift-ci-robot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Mar 30, 2020
@openshift-ci-robot
Copy link

@JacobTanenbaum: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

[Release 3.11] Bug 1795416 : Mutliple iptables improvements for kubelet and kube-proxy

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.

@JacobTanenbaum JacobTanenbaum changed the title [Release 3.11] Bug 1795416 : Mutliple iptables improvements for kubelet and kube-proxy [Release 3.11] Bug 1795416: Mutliple iptables improvements for kubelet and kube-proxy Mar 30, 2020
@openshift-ci-robot
Copy link

@JacobTanenbaum: This pull request references Bugzilla bug 1795416, which is invalid:

  • expected Bugzilla bug 1795416 to depend on a bug in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but no dependents were found

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

[Release 3.11] Bug 1795416: Mutliple iptables improvements for kubelet and kube-proxy

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.

@openshift-ci-robot openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Mar 30, 2020
@danwinship
Copy link
Contributor

/retitle [WIP] [Release 3.11] Multiple iptables improvements for kubelet and kube-proxy

test failures seem mostly related to recent CI changes. It's good that e2e-gcp passed.
/retest

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 2, 2020
@danwinship
Copy link
Contributor

/lgtm
🤞

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 2, 2020
@danwinship
Copy link
Contributor

/test unit
flake from #25027 @juanluisvaladas

vxlan_monitor_test.go:263: Expected update[0] to be 192.168.1.1: []node.egressVXLANNode{node.egressVXLANNode{nodeIP:"192.168.1.3", offline:true, in:0x3, out:0xe, retries:0}, node.egressVXLANNode{nodeIP:"192.168.1.1", offline:true, in:0x4, out:0x7, retries:0}}

@JacobTanenbaum
Copy link
Contributor Author

/retest

@JacobTanenbaum
UPSTREAM 80368: iptables: don't do feature detection on the iptables-…
6b9e156 
…restore binary

Origin-commit: 8bced9b

The iptables code was doing version detection on the iptables binary
but feature detection on the iptables-restore binary, to try to
support the version of iptables in RHEL 7, which claims to be 1.4.21
but has certain features from iptables 1.6.

The problem is that this particular set of versions and checks
resulted in the code passing "-w" ("wait forever for the lock") to
iptables, but "-w 5" ("wait at most 5 seconds for the lock") to
iptables-restore. On systems with very very many iptables rules, this
could result in the kubelet periodic resyncs (which use "iptables")
blocking kube-proxy (which uses "iptables-restore") and causing it to
time out.

We already have code to grab the lock file by hand when using a
version of iptables-restore that doesn't support "-w", and it works
fine. So just use that instead, and only pass "-w 5" to
iptables-restore when iptables reports a version that actually
supports it.

NOTE: MANUALLY EDITED, upstream switched to from glog -> klog
@JacobTanenbaum
UPSTREAM 80368: kube-proxy: drop iptables version check
40fc424 
Origin-commit: a735c97

Kube-proxy's iptables mode used to care whether utiliptables's
EnsureRule was able to use "iptables -C" or if it had to implement it
hackily using "iptables-save". But that became irrelevant when
kube-proxy was reimplemented using "iptables-restore", and no one ever
noticed. So remove that check.

MANUAL EDITS:
   vendor/k8s.io/kubernetes/pkg/proxy/iptables/BUILD removed line //pkg/util/version:go_default_library upstream removes //staging/src/k8s.io/apimachinery/pkg/util/version:go_default_library
@JacobTanenbaum
UPSTREAM 80368: iptables: simplify version handling
977b88e 
Origin-commit: 81cd27a

MANUAL EDITS:
  vendor/k8s.io/kubernetes/pkg/util/iptables/iptables.go - parts needed to be manually applied
  change import k8s.io/apimachinery/pkg/util/version/version.go -> k8s.io/kubernetes/pkg/util/version
  in vendor/k8s.io/kubernetes/pkg/util/iptables/BUILD manually remove "//pkg/util/version:go_default_library"
@JacobTanenbaum
UPSTREAM 82602: Fix iptables version detection code to handle RHEL 7 …
43dcb4f 
…correctly

ORIGIN PR: 7588807

NOTES:
in vendor/k8s.io/kubernetes/pkg/util/iptables/iptables.go needed to change klog -> glog
@JacobTanenbaum
UPSTREAM 81517: Drop iptables firewalld monitoring support
1af8f61 
ORIGIN PR: b6c3d54

The firewalld monitoring code was not well tested (and not easily
testable), would never be triggered on most platforms, and was only
being taken advantage of from one place (kube-proxy), which didn't
need it anyway since it already has its own resync loop.

Since the firewalld monitoring was the only consumer of pkg/util/dbus,
we can also now delete that.

NOTES:
  cannot actually delete the dbus code pkg/dns relies on it

  manually removed dbus code from: vendor/k8s.io/kubernetes/cmd/kube-proxy/app/BUILD
                                   vendor/k8s.io/kubernetes/pkg/kubelet/BUILD
                                   vendor/k8s.io/kubernetes/pkg/util/BUILD

 manually applied from: vendor/k8s.io/kubernetes/pkg/kubelet/kubelet.go
                        vendor/k8s.io/kubernetes/pkg/util/iptables/iptables.go

does not compile, needs future commits
@JacobTanenbaum
UPSTREAM 81517: Add iptables.Monitor, use it from kubelet and kube-proxy
8a2c994 
ORIGIN PR: 3948f16

Kubelet and kube-proxy both had loops to ensure that their iptables
rules didn't get deleted, by repeatedly recreating them. But on
systems with lots of iptables rules (ie, thousands of services), this
can be very slow (and thus might end up holding the iptables lock for
several seconds, blocking other operations, etc).

The specific threat that they need to worry about is
firewall-management commands that flush *all* dynamic iptables rules.
So add a new iptables.Monitor() function that handles this by creating
iptables-flush canaries and only triggering a full rule reload after
noticing that someone has deleted those chains.

NOTES:
  klog->glog in vendor/k8s.io/kubernetes/pkg/util/iptables/iptables.go
  manual application of patches in vendor/k8s.io/kubernetes/pkg/util/iptables/iptables.go
                                   vendor/k8s.io/kubernetes/pkg/util/iptables/BUILD
                                   vendor/k8s.io/kubernetes/pkg/proxy/iptables/proxier.go

the changes to vendor/k8s.io/kubernetes/pkg/kubelet/kubelet_network_linux.go had to be manually applied to vendor/k8s.io/kubernetes/pkg/kubelet/kubelet_network.go

does not compile needs a future commit
@JacobTanenbaum
UPSTREAM 81517: e2e: test that both kube-proxy and kubelet recover af…
6b46aef 
…ter iptables flush

Notes
  manually changed refereances to the imports
    e2eservice "k8s.io/kubernetes/test/e2e/framework/service"
    e2essh "k8s.io/kubernetes/test/e2e/framework/ssh"

    to framework because that is there the functions reside

    had to do a bring in clientset because StopServeHostname() and StartServeHostName
     take a internalClientset.interface instead of Service

    manually performed the operations of ssh.LogResult()

    correct the arguments to StartServeHostnameService and StopServeHostnameService
@JacobTanenbaum
node: Update for utiliptables API changes
351896f 
SDN COMMIT: 3b71b6bba38c1eeb0b92155ee97d20e9e9b9b199

NOTE:
  because of the split the whole patch had to be manually applied
    pkg/network/node/iptables.go and pkg/network/node/pod.go had the functionality in pkg/network/node/iptables.go

   pkg/cmd/server/kubernetes/network/network.go has the functionality of pkg/openshift-sdn/proxy.go
@JacobTanenbaum
UPSTREAM 83402: iptables.Monitor: don't be fooled by "could not get l…
dc9bd07 
…ock" errors

ORIGIN PR: 2f89c03

NOTES:
had to manually apply the patch because of klog -> glog
@JacobTanenbaum
Fix hybrid proxier for iptables.Monitor
d69c104 
backport of sdn commit 326354dc2f29bafd41394ea8807d75d23288e20e
very heavliy modified in order to function

The iptables proxier has been fixed to not constantly resync its rules
when not needed... except that the hybrid proxier was forcing it to do
it anyway. Fix that by moving the NodeIPTables monitor up out to the
top level of openshift-sdn-node and sharing it between the node and
proxy code.
@JacobTanenbaum
Bug 1820778: fix health check in standalone kube-proxy
96972ee
@JacobTanenbaum
UPSTREAM: <drop>: preserve old health/metrics behavior
0c9e44a 
Tweak the upstream commits to preserve the old behavior for kube-proxy
health and metrics. In 4.4 we will get the upstream fix for health,
and will need to ensure that our monitoring is fixed to use different
metrics.

MANUAL EDIT:
the metric metrics.SyncProxyRulesLastTimestamp does not exist in the version of kube in 3.11 so I need
to remove it

change SyncProxyRules to forceSyncProxyRules() in proxier_test.go
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 2, 2020
@openshift-ci-robot
Copy link

@JacobTanenbaum: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-gcp-crio 0c9e44a link /test e2e-gcp-crio

Full PR test history. Your PR dashboard.

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.

@danwinship
Copy link
Contributor

/assign @knobunc eparis

@danwinship
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 2, 2020
@knobunc
Copy link
Contributor

knobunc commented Sep 3, 2020

/approve

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, JacobTanenbaum, juanluisvaladas, knobunc

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 3, 2020
@openshift-merge-robot openshift-merge-robot merged commit f2fd065 into openshift:release-3.11 Sep 3, 2020
@openshift-ci-robot
Copy link

@JacobTanenbaum: All pull requests linked via external trackers have merged:

Bugzilla bug 1795416 has been moved to the MODIFIED state.

In response to this:

[Release 3.11] Bug 1795416: Multiple iptables improvements for kubelet and kube-proxy

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danwinship danwinship danwinship left review comments

@knobunc knobunc Awaiting requested review from knobunc

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

Assignees

@danwinship danwinship

@juanluisvaladas juanluisvaladas

@eparis eparis

@knobunc knobunc

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-urgent Referenced Bugzilla bug's severity is urgent for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@JacobTanenbaum @danwinship @juanluisvaladas @openshift-ci-robot @abhat @dcbw @knobunc @eparis @openshift-merge-robot