Reduce memory allocations in kube proxy #46033

wojtek-t · 2017-05-18T14:45:17Z

Memory allocation (and Go GarbageCollection) seems to be one of the most expensive operations in kube-proxy (I've seen profiles where it was more than 50%).

The commits are mostly independent from each other and all of them are mostly about reusing already allocated memory.

This PR is reducing memory allocation by ~5x (results below from 100-node load test):

before:

(pprof) top
38.64GB of 39.11GB total (98.79%)
Dropped 249 nodes (cum <= 0.20GB)
Showing top 10 nodes out of 61 (cum >= 0.20GB)
      flat  flat%   sum%        cum   cum%
   15.10GB 38.62% 38.62%    15.10GB 38.62%  bytes.makeSlice
    9.48GB 24.25% 62.87%     9.48GB 24.25%  runtime.rawstringtmp
    8.30GB 21.21% 84.07%    32.47GB 83.02%  k8s.io/kubernetes/pkg/proxy/iptables.(*Proxier).syncProxyRules
    2.08GB  5.31% 89.38%     2.08GB  5.31%  fmt.(*fmt).padString
    1.90GB  4.86% 94.24%     3.82GB  9.77%  strings.Join
    0.67GB  1.72% 95.96%     0.67GB  1.72%  runtime.hashGrow
    0.36GB  0.92% 96.88%     0.36GB  0.92%  runtime.stringtoslicebyte
    0.31GB  0.79% 97.67%     0.62GB  1.58%  encoding/base32.(*Encoding).EncodeToString
    0.24GB  0.62% 98.29%     0.24GB  0.62%  strings.genSplit
    0.20GB   0.5% 98.79%     0.20GB   0.5%  runtime.convT2E

after:

7.94GB of 8.13GB total (97.75%)
Dropped 311 nodes (cum <= 0.04GB)
Showing top 10 nodes out of 65 (cum >= 0.11GB)
      flat  flat%   sum%        cum   cum%
    3.32GB 40.87% 40.87%     8.05GB 99.05%  k8s.io/kubernetes/pkg/proxy/iptables.(*Proxier).syncProxyRules
    2.85GB 35.09% 75.95%     2.85GB 35.09%  runtime.rawstringtmp
    0.60GB  7.41% 83.37%     0.60GB  7.41%  runtime.hashGrow
    0.31GB  3.76% 87.13%     0.31GB  3.76%  runtime.stringtoslicebyte
    0.28GB  3.43% 90.56%     0.55GB  6.80%  encoding/base32.(*Encoding).EncodeToString
    0.19GB  2.29% 92.85%     0.19GB  2.29%  strings.genSplit
    0.18GB  2.17% 95.03%     0.18GB  2.17%  runtime.convT2E
    0.10GB  1.28% 96.31%     0.71GB  8.71%  runtime.mapassign
    0.10GB  1.21% 97.51%     0.10GB  1.21%  syscall.ByteSliceFromString
    0.02GB  0.23% 97.75%     0.11GB  1.38%  syscall.SlicePtrFromStrings

wojtek-t · 2017-05-18T18:11:57Z

We can probably optimize much more, but I would like to wait with that for all other changes to be merged first to see where we will be (in particular periodic runner)

thockin

Nice. Sort of sad how easy it is to write slice-abusive code.

Just a few nits. Will approve, you can fixup and self-lgtm

thockin · 2017-05-19T18:22:42Z

pkg/proxy/iptables/proxier.go

@@ -417,6 +425,11 @@ func NewProxier(ipt utiliptables.Interface,
 		recorder:         recorder,
 		healthChecker:    healthChecker,
 		healthzServer:    healthzServer,
+		iptablesLines:    bytes.NewBuffer(nil),


nit: This isn't really parsed into lines yet - call it iptablesData or iptablesRaw ?

thockin · 2017-05-19T18:23:17Z

pkg/proxy/iptables/proxier.go

-	filterRules := bytes.NewBuffer(nil)
-	natChains := bytes.NewBuffer(nil)
-	natRules := bytes.NewBuffer(nil)
+	// Reset all buffers used later.


Comment that this is to avoid re-allocations

thockin · 2017-05-19T18:26:50Z

pkg/kubelet/network/hostport/fake_iptables.go

@@ -228,36 +228,27 @@ func saveChain(chain *fakeChain, data *bytes.Buffer) {
 }

 func (f *fakeIPTables) Save(tableName utiliptables.Table) ([]byte, error) {


Do we still need plain Save() any more?

We are still using it in few places in kubelet. Though I'm happy to send a PR migrating it to SaveInto (should be pretty simple). But I would prefer to do it in a separate one. Will send one on Monday.

thockin · 2017-05-19T18:29:49Z

pkg/util/iptables/iptables.go

+	args := []string{"-t", string(table)}
+	glog.V(4).Infof("running iptables-save %v", args)
+	cmd := runner.exec.Command(cmdIPTablesSave, args...)
+	cmd.SetStdout(buffer)


Add a comment on this vs CombinedOutput() ?

thockin · 2017-05-19T18:31:16Z

/approve

k8s-github-robot · 2017-05-19T18:39:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: thockin, wojtek-t

Needs approval from an approver in each of these OWNERS Files:

~~pkg/OWNERS~~ [thockin,wojtek-t]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

wojtek-t

Thanks Tim - comments applied.

wojtek-t · 2017-05-19T18:37:10Z

pkg/kubelet/network/hostport/fake_iptables.go

@@ -228,36 +228,27 @@ func saveChain(chain *fakeChain, data *bytes.Buffer) {
 }

 func (f *fakeIPTables) Save(tableName utiliptables.Table) ([]byte, error) {


We are still using it in few places in kubelet. Though I'm happy to send a PR migrating it to SaveInto (should be pretty simple). But I would prefer to do it in a separate one. Will send one on Monday.

wojtek-t · 2017-05-19T18:44:36Z

pkg/proxy/iptables/proxier.go

@@ -417,6 +425,11 @@ func NewProxier(ipt utiliptables.Interface,
 		recorder:         recorder,
 		healthChecker:    healthChecker,
 		healthzServer:    healthzServer,
+		iptablesLines:    bytes.NewBuffer(nil),


wojtek-t · 2017-05-19T18:45:23Z

pkg/proxy/iptables/proxier.go

-	filterRules := bytes.NewBuffer(nil)
-	natChains := bytes.NewBuffer(nil)
-	natRules := bytes.NewBuffer(nil)
+	// Reset all buffers used later.


wojtek-t · 2017-05-19T18:49:12Z

pkg/util/iptables/iptables.go

+	args := []string{"-t", string(table)}
+	glog.V(4).Infof("running iptables-save %v", args)
+	cmd := runner.exec.Command(cmdIPTablesSave, args...)
+	cmd.SetStdout(buffer)


wojtek-t · 2017-05-19T18:50:23Z

Applying label based on Tim`s comment above.

wojtek-t · 2017-05-19T19:06:19Z

Just fixed compile errors that appeared (didn't rename all occurences of variable).

wojtek-t · 2017-05-19T19:34:22Z

Sorry - fixed gofmt.

freehan

just nits. Looks good overall.

freehan · 2017-05-19T23:13:13Z

pkg/proxy/iptables/proxier.go


-	glog.V(3).Infof("Restoring iptables rules: %s", lines)
-	err = proxier.iptables.RestoreAll(lines, utiliptables.NoFlushTables, utiliptables.RestoreCounters)
+	if glog.V(4) {


remove if condition here?

Sorry - initially it was different in my code.
So it sometimes makes sense to have such iffs, if the arguments of glog are expensive to compute. So imagine that you have the following line:

glog.V(4).Infof("xxxL: %v", callFunction(args))

Then callFunction(args) will be called no matter what the verbosity is and whether the log will be printed or not. In case of expensive computations, it's simply waste of time.

This is no longer the case here, so will send a PR to remove it (or will integrate it to one of other PRs I already have).

freehan · 2017-05-19T23:15:22Z

pkg/proxy/iptables/proxier.go

@@ -1497,8 +1497,8 @@ func (proxier *Proxier) syncProxyRules(reason syncReason) {
 	proxier.iptablesLines.Write(proxier.natChains.Bytes())
 	proxier.iptablesLines.Write(proxier.natRules.Bytes())

-	if glog.V(4) {


why? I feel like it has to be 4 at least. Otherwise, there is no enough actionable information to debug e2e tests.

This is extremely expensive (in my tests in just 100-node cluster, this line is responsible for ~10% of whole cpu usage and ~20% of all memory allocations and GC).

I think that we can just bump the verbosity in tests in case of problems.

k8s-ci-robot · 2017-05-20T06:01:30Z

@wojtek-t: The following test(s) failed:

Test name	Commit	Details	Rerun command
pull-kubernetes-federation-e2e-gce	`a3da8d7`	link	`@k8s-bot pull-kubernetes-federation-e2e-gce test this`

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.

k8s-github-robot · 2017-05-20T06:21:49Z

Automatic merge from submit-queue

thockin · 2017-05-21T04:15:12Z

pkg/proxy/iptables/proxier.go


-	glog.V(3).Infof("Restoring iptables rules: %s", lines)
-	err = proxier.iptables.RestoreAll(lines, utiliptables.NoFlushTables, utiliptables.RestoreCounters)
+	if glog.V(5) {


weird constructs like this warrant a comment

In this case it's not longer needed - I will send a PR removing it on Monday.

#46201 is already out for review

Automatic merge from submit-queue (batch tested with PRs 45534, 37212, 46613, 46350) Speed up and reduce number of memory allocations in kube-proxy This is a second (and last PR) in this series - this solves all very-low-hanging fruits. This PR: - reduces cpu usage by ~25% - reduces memory allocations by ~3x (together with #46033 by 10-12x) Without this PR: ``` (pprof) top 8.59GB of 8.79GB total (97.75%) Dropped 238 nodes (cum <= 0.04GB) Showing top 10 nodes out of 64 (cum >= 0.11GB) flat flat% sum% cum cum% 3.66GB 41.60% 41.60% 8.72GB 99.17% k8s.io/kubernetes/pkg/proxy/iptables.(*Proxier).syncProxyRules 3.07GB 34.96% 76.56% 3.07GB 34.96% runtime.rawstringtmp 0.62GB 7.09% 83.65% 0.62GB 7.09% runtime.hashGrow 0.34GB 3.82% 87.46% 0.34GB 3.82% runtime.stringtoslicebyte 0.29GB 3.24% 90.71% 0.58GB 6.61% encoding/base32.(*Encoding).EncodeToString 0.22GB 2.47% 93.18% 0.22GB 2.47% strings.genSplit 0.18GB 2.04% 95.22% 0.18GB 2.04% runtime.convT2E 0.11GB 1.22% 96.44% 0.73GB 8.36% runtime.mapassign 0.10GB 1.08% 97.52% 0.10GB 1.08% syscall.ByteSliceFromString 0.02GB 0.23% 97.75% 0.11GB 1.25% syscall.SlicePtrFromStrings ``` with this PR: ``` (pprof) top 2.98GB of 3.08GB total (96.78%) Dropped 246 nodes (cum <= 0.02GB) Showing top 10 nodes out of 70 (cum >= 0.10GB) flat flat% sum% cum cum% 1.99GB 64.60% 64.60% 1.99GB 64.60% runtime.rawstringtmp 0.58GB 18.95% 83.55% 0.58GB 18.95% runtime.hashGrow 0.10GB 3.40% 86.95% 0.69GB 22.47% runtime.mapassign 0.09GB 2.86% 89.80% 0.09GB 2.86% syscall.ByteSliceFromString 0.08GB 2.63% 92.44% 0.08GB 2.63% runtime.convT2E 0.03GB 1.13% 93.56% 0.03GB 1.13% syscall.Environ 0.03GB 0.99% 94.56% 0.03GB 0.99% bytes.makeSlice 0.03GB 0.97% 95.52% 0.03GB 1.06% os.Stat 0.02GB 0.65% 96.18% 3.01GB 97.79% k8s.io/kubernetes/pkg/proxy/iptables.(*Proxier).syncProxyRules 0.02GB 0.6% 96.78% 0.10GB 3.35% syscall.SlicePtrFromStrings ```

wojtek-t added area/kube-proxy release-note-none Denotes a PR that doesn't merit a release note. labels May 18, 2017

wojtek-t assigned freehan, thockin and MrHohn May 18, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 18, 2017

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 18, 2017

wojtek-t force-pushed the reduce_memory_allocations_in_kube_proxy branch from 3dd0222 to 524c13a Compare May 18, 2017 15:59

thockin reviewed May 19, 2017

View reviewed changes

wojtek-t added 6 commits May 19, 2017 20:44

Remove SaveAll from iptables interface

028ac80

Extend Iptables interface with SaveInto

bcfae7e

Reuse buffer for getting iptables contents

5464c39

Avoid strings.Join which is expensive

4d29c86

Reuse buffers for generated iptables rules

e3bb755

Descrese logs verbosity for iptables

7d44f83

wojtek-t force-pushed the reduce_memory_allocations_in_kube_proxy branch from 524c13a to 919f931 Compare May 19, 2017 18:49

wojtek-t commented May 19, 2017

View reviewed changes

wojtek-t added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 19, 2017

wojtek-t force-pushed the reduce_memory_allocations_in_kube_proxy branch from 919f931 to f53440e Compare May 19, 2017 19:05

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 19, 2017

wojtek-t added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 19, 2017

Fix naming and comments in kube-proxy.

a3da8d7

wojtek-t force-pushed the reduce_memory_allocations_in_kube_proxy branch from f53440e to a3da8d7 Compare May 19, 2017 19:34

wojtek-t added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 19, 2017

freehan reviewed May 19, 2017

View reviewed changes

k8s-github-robot merged commit 3456d4d into kubernetes:master May 20, 2017

thockin reviewed May 21, 2017

View reviewed changes

wojtek-t deleted the reduce_memory_allocations_in_kube_proxy branch May 23, 2017 09:59

wojtek-t mentioned this pull request May 24, 2017

Speed up and reduce number of memory allocations in kube-proxy #46350

Merged

danwinship mentioned this pull request Apr 14, 2022

Need benchmarks for iptables proxy golang performance hacks #109481

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce memory allocations in kube proxy #46033

Reduce memory allocations in kube proxy #46033

wojtek-t commented May 18, 2017

wojtek-t commented May 18, 2017

thockin left a comment

thockin May 19, 2017

wojtek-t May 19, 2017

thockin May 19, 2017

wojtek-t May 19, 2017

thockin May 19, 2017

wojtek-t May 19, 2017

thockin May 19, 2017

wojtek-t May 19, 2017

thockin commented May 19, 2017

k8s-github-robot commented May 19, 2017

wojtek-t left a comment

wojtek-t May 19, 2017

wojtek-t May 19, 2017

wojtek-t May 19, 2017

wojtek-t May 19, 2017

wojtek-t commented May 19, 2017

wojtek-t commented May 19, 2017

wojtek-t commented May 19, 2017

freehan left a comment

freehan May 19, 2017

wojtek-t May 20, 2017

freehan May 19, 2017

wojtek-t May 20, 2017

k8s-ci-robot commented May 20, 2017 •

edited

k8s-github-robot commented May 20, 2017

thockin May 21, 2017

wojtek-t May 21, 2017

wojtek-t May 22, 2017

		@@ -228,36 +228,27 @@ func saveChain(chain fakeChain, data bytes.Buffer) {
		}

		func (f *fakeIPTables) Save(tableName utiliptables.Table) ([]byte, error) {

Reduce memory allocations in kube proxy #46033

Reduce memory allocations in kube proxy #46033

Conversation

wojtek-t commented May 18, 2017

wojtek-t commented May 18, 2017

thockin left a 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

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

thockin commented May 19, 2017

k8s-github-robot commented May 19, 2017

wojtek-t left a 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

wojtek-t commented May 19, 2017

wojtek-t commented May 19, 2017

wojtek-t commented May 19, 2017

freehan left a 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

k8s-ci-robot commented May 20, 2017 • edited

k8s-github-robot commented May 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 20, 2017 •

edited