kube-proxy: ratelimit runs of iptables by sync-period flags #46266

thockin · 2017-05-23T05:10:44Z

This bounds how frequently iptables can be synced. It will be no more often than every 10 seconds and no less often than every 1 minute, by default.

@timothysc FYI

@dcbw @freehan FYI

wojtek-t · 2017-05-23T05:25:52Z

@thockin - it seems it already needs rebase :)

wojtek-t

Mostly minor stuff (except from making this work by default in GCI too).

wojtek-t · 2017-05-23T05:30:32Z

pkg/util/async/bounded_frequency_runner.go

+//
+// The function will be run no more often than burstRuns per minInterval,  For
+// example, this can mitigate the impact of expensive operations being called in
+// response to user-initiated operations.  Burts runs are "accumulated" over


s/Burts/Bursts/

wojtek-t · 2017-05-23T05:35:36Z

pkg/util/async/bounded_frequency_runner.go

+	if minInterval == 0 {
+		bfr.limiter = nullLimiter{}
+	} else {
+		// minInterval is a duration, typically in seconds but could be fractional


I'm not sure I understand this comment. I mean I'm not sure how it relates to the line below (which I agree is correct).

wojtek-t · 2017-05-23T05:41:08Z

pkg/util/async/bounded_frequency_runner.go

+		bfr.lastRun = bfr.timer.Now()
+		bfr.fn()
+		bfr.timer.Stop()
+		bfr.timer.Reset(bfr.maxInterval)


I think that it would be good to comment about the semantic of it - I think it isn't obvious:

"lastRun" is the time when the last function call started

"timer" starts ticking when the function actually ended.
So in the situation when noone is calling Run() (or calling it super rarely that it doesn't really matter), but the function call takes significant time comparing to "maxInterval", then we are not really calling it every "maxInterval".

To clarify - if calling a function takes 5s, and maxInterval is also 5s, we will be calling the function every 10s.

I guess, I would do the same (I mean I wouldn't change the logic here), but I think it needs clarification in the comment for NewBoundedFrequencyRunner

Also note, that this semantic is different for user-initiated call, where then "minInterval" is between two consecutive starts of the function call.

That's an excellent point - this is busted. It really should be consistent. My hunch is that it should take the time after the run, so there's always some rest period after a run. This means the named "bounded frequency" is bad. Should be "ThrottledRunner" or something? That doesn't quite convey that is also has a periodic run ...

thoughts?

A possible rename is the only thing left, I think.

Now when I think about it, i think that the semantic that you are suggesting is the good one (I initially though that it should be measure between starts, but that has a bunch of drawbacks).

Regarding naming - I don't have opinion. I was always extremely bad at naming :)

wojtek-t · 2017-05-23T05:47:34Z

cluster/saltbase/salt/kube-proxy/kube-proxy.manifest

 # test_args should always go last to overwrite prior configuration
-{% set params = log_level + " " + feature_gates + " " + test_args -%}
+{% set params = log_level + " " + throttles + " " + feature_gates + " " + test_args -%}


This will not work in GCI. You also need to add appropriate if in:
https://github.com/kubernetes/kubernetes/blob/master/cluster/gce/gci/configure-helper.sh#L822
and
https://github.com/kubernetes/kubernetes/blob/master/cluster/gce/container-linux/configure-helper.sh#L615

wojtek-t · 2017-05-23T05:49:30Z

pkg/proxy/iptables/proxier.go

 }

 func (proxier *Proxier) OnServiceAdd(service *api.Service) {
 	namespacedName := types.NamespacedName{Namespace: service.Namespace, Name: service.Name}
 	if proxier.serviceChanges.update(&namespacedName, nil, service) {
 		// TODO(wojtek-t): If the initial sync of informer either for endpoints or
-		// services is not finished, it doesn't make sense to call syncProxyRules
+		// services is not finished, it doesn't make sense to sync right away


I guess those changes are the one that conficts with my PR that has just been merged.

wojtek-t · 2017-05-23T05:51:46Z

cluster/saltbase/salt/kube-proxy/kube-proxy.manifest

@@ -29,8 +29,10 @@
 {% set feature_gates = "--feature-gates=" + grains.feature_gates -%}
 {% endif -%}

+{% set throttles = "--iptables-sync-period=1m --iptables-min-sync-period=10s" -%}


Can we start with smaller min-sync-period now? E.g. 5s?
After thinking about it, i think this change may surprise people with small cluster where everything worked really fast.

If we decide to increase it later, it would be super trivial change to do.

In general the bursts should allow changes to happen very quickly most of the time, and throttling kicks in only in larger clusters. At least, that's what I expect...

The scenario I was afraid about is that you are setting bursts=2.

So if we are creating a replication controller with 3 replicas, if endpoint controller will be fast enough that it will be adding pods to Endpoints object one by one, the first two will be added quickly, but for the last we will need to wait 10s more.

I'm not sure how big problem it is though, so maybe it fine to leave now. We can tune later.

we can make this even more complex and add a short holdoff (e.g. 1sec). I'd rather do that as a followup, if needed. Or simpler, make bursts higher. Here's my thinking - if the iptables update is fast, a short burst isn't a big deal. If the update is slow, there will be a lot of time between runs anyway to accumulate new pods. So maybe setting burst to 4 or is better - we could do it as a follow up to make that a flag...

I agree that bumping bursts sounds like a good option. I'm obviously fine with doing it in a followup - we will see what will be the impact of this change.

timothysc · 2017-05-23T19:35:50Z

What's the current state of IPVS load-balancing, and doesn't that obviate this to some extent?

thockin · 2017-05-23T23:58:07Z

@timothysc AFAIK IPVS is not done, much less ready to be the default.

thockin · 2017-05-24T05:56:35Z

@k8s-bot pull-kubernetes-unit test this

thockin · 2017-05-24T07:14:37Z

@caesarxuchao I have no idea what staging godeps wants. Help?

wojtek-t · 2017-05-24T10:24:45Z

Just two minor comments (not very actionable though).

@thockin - feel free to self-lgtm after fixing godeps issue

caesarxuchao · 2017-05-24T17:08:34Z

Other staging repos also imports juju,

staging/src/k8s.io/apiserver/Godeps/Godeps.json:482:                    "ImportPath": "github.com/juju/ratelimit",
staging/src/k8s.io/client-go/Godeps/Godeps.json:162:                    "ImportPath": "github.com/juju/ratelimit",
staging/src/k8s.io/kube-aggregator/Godeps/Godeps.json:238:                      "ImportPath": "github.com/juju/ratelimit",
staging/src/k8s.io/kube-apiextensions-server/Godeps/Godeps.json:230:                    "ImportPath": "github.com/juju/ratelimit",
staging/src/k8s.io/sample-apiserver/Godeps/Godeps.json:230:                     "ImportPath": "github.com/juju/ratelimit",

Manually updating them should make the verify script happy.

I'll improve the script after the 1.7 rush.

freehan · 2017-05-24T18:25:29Z

pkg/util/async/bounded_frequency_runner.go

+// completion to next start), except that up to bursts may be allowed.  Burst
+// runs are "accumulated" over time, one per minInterval up to burstRuns total.
+// This can be used, for example, to mitigate the impact of expensive operations
+// being called in response to user-initiated operations. Run requests s that


s/requests\ss/request

This picked up an unrelated but missing change.

This lib manages runs of a function to have min and max frequencies.

thockin · 2017-05-25T03:34:50Z

manually fixed staging godeps. @caesarxuchao that script is my least favorite thing in the whole project right now. It also decided it needed to delete staging/src/k8s.io/client-go/LICENSE - no idea why. and I had to make a clean GOPATH or it just failed in inscrutable ways.

k8s-ci-robot · 2017-05-25T04:01:05Z

@thockin: The following test(s) failed:

Test name	Commit	Details	Rerun command
pull-kubernetes-federation-e2e-gce	`2856fde`	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.

thockin · 2017-05-25T05:08:50Z

self-lgtm

wojtek-t · 2017-05-25T07:23:45Z

/lgtm

@thockin - thanks a lot!

k8s-github-robot · 2017-05-25T07:29:49Z

[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:

~~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

k8s-github-robot · 2017-05-25T13:17:55Z

Automatic merge from submit-queue (batch tested with PRs 44774, 46266, 46248, 46403, 46430)

thockin assigned wojtek-t and dcbw May 23, 2017

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

wojtek-t reviewed May 23, 2017

View reviewed changes

thockin force-pushed the proxy-periodic-runner-2 branch from 3a9c269 to dfd6cab Compare May 24, 2017 04:17

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

thockin force-pushed the proxy-periodic-runner-2 branch 3 times, most recently from 65c3806 to 5b3c9bd Compare May 24, 2017 05:33

thockin force-pushed the proxy-periodic-runner-2 branch from 5b3c9bd to 3da04e1 Compare May 24, 2017 06:27

freehan reviewed May 24, 2017

View reviewed changes

thockin added 5 commits May 24, 2017 20:33

Logging/naming cleanup for service port names

578d9fc

Update godeps for juju ratelimit

3178433

This picked up an unrelated but missing change.

Inject clock through flowcontrol

3153ca2

Add bounded frequency runner

bbb80c2

This lib manages runs of a function to have min and max frequencies.

Use BoundedFrequencyRunner in kube-proxy

2856fde

thockin force-pushed the proxy-periodic-runner-2 branch from 3da04e1 to 2856fde Compare May 25, 2017 03:33

thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2017

k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label May 25, 2017

thockin added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. release-note-label-needed labels May 25, 2017

thockin changed the title ~~kube-proxy: throttle runs of iptables~~ kube-proxy: ratelimit runs of iptables by sync-period flags May 25, 2017

k8s-github-robot merged commit ee671e6 into kubernetes:master May 25, 2017

MrHohn mentioned this pull request May 26, 2017

Failure cluster [3917e9...] failed 85 builds, 8 jobs, and 1 tests over 1 days #46533

Closed

jonjohnsonjr mentioned this pull request Jul 23, 2019

Cold start latency exacerbated by iptables-min-sync-period knative/serving#4902

Closed

thockin deleted the proxy-periodic-runner-2 branch August 14, 2019 17:44

sebgl mentioned this pull request Jan 6, 2020

E2E test Cluster_UUID_should_have_been_preserved is flaky elastic/cloud-on-k8s#2318

Closed

danwinship mentioned this pull request Jul 6, 2020

kube-proxy iptables min-sync-period default 1sec #92836

Merged

robscott mentioned this pull request Aug 5, 2021

Service refuses traffic to endpoints after updating the selected pod label #98570

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kube-proxy: ratelimit runs of iptables by sync-period flags #46266

kube-proxy: ratelimit runs of iptables by sync-period flags #46266

thockin commented May 23, 2017

wojtek-t commented May 23, 2017

wojtek-t left a comment

wojtek-t May 23, 2017

wojtek-t May 23, 2017

wojtek-t May 23, 2017

wojtek-t May 23, 2017

thockin May 24, 2017

thockin May 24, 2017

wojtek-t May 24, 2017

wojtek-t May 23, 2017

wojtek-t May 23, 2017

wojtek-t May 23, 2017

thockin May 24, 2017

wojtek-t May 24, 2017

thockin May 25, 2017

wojtek-t May 25, 2017

timothysc commented May 23, 2017

thockin commented May 23, 2017

thockin commented May 24, 2017

thockin commented May 24, 2017

wojtek-t commented May 24, 2017

caesarxuchao commented May 24, 2017

freehan May 24, 2017

thockin commented May 25, 2017

k8s-ci-robot commented May 25, 2017 •

edited

Loading

thockin commented May 25, 2017

wojtek-t commented May 25, 2017

k8s-github-robot commented May 25, 2017

k8s-github-robot commented May 25, 2017

kube-proxy: ratelimit runs of iptables by sync-period flags #46266

kube-proxy: ratelimit runs of iptables by sync-period flags #46266

Conversation

thockin commented May 23, 2017

wojtek-t commented May 23, 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

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

Choose a reason for hiding this comment

timothysc commented May 23, 2017

thockin commented May 23, 2017

thockin commented May 24, 2017

thockin commented May 24, 2017

wojtek-t commented May 24, 2017

caesarxuchao commented May 24, 2017

Choose a reason for hiding this comment

thockin commented May 25, 2017

k8s-ci-robot commented May 25, 2017 • edited Loading

thockin commented May 25, 2017

wojtek-t commented May 25, 2017

k8s-github-robot commented May 25, 2017

k8s-github-robot commented May 25, 2017

k8s-ci-robot commented May 25, 2017 •

edited

Loading