skip iptables sync if no endpoint changes #41223

freehan · 2017-02-10T00:06:16Z

Alternative to #41173
fixes: #26637
No need to checksum. Just compare endpoint maps.

k8s-reviewable · 2017-02-10T00:06:22Z

This change is

freehan · 2017-02-10T00:53:40Z

I think this one is cleaner than the checksum one.
I did my best to sort out the original logic and build on top of it. It probably need a major refactor at some point. But since this PR is aiming for cherrypick, so only changing the minimum

bprashanth

/lgtm

bprashanth · 2017-02-10T01:16:04Z

pkg/proxy/iptables/proxier.go

 			activeEndpoints[svcPort] = true
 		}
 	}
-	// Remove endpoints missing from the update.
+	// Gather stale connections to endpoints missing from the update.


nit: suggest rewording to "Check stale connections against endpoints missing from the update"

bprashanth · 2017-02-10T01:24:18Z

pkg/proxy/iptables/proxier.go

 				removedEndpoints := getRemovedEndpoints(curEndpointIPs, newEndpoints)
 				for _, ep := range removedEndpoints {
 					staleConnections[endpointServicePair{endpoint: ep, servicePortName: svcPort}] = true
 				}
-				glog.V(3).Infof("Setting endpoints for %q to %+v", svcPort, newEndpoints)
-				// Once the set operations using the list of ips are complete, build the list of endpoint infos
-				proxier.endpointsMap[svcPort] = proxier.buildEndpointInfoList(portsToEndpoints[portname], newEndpoints)


Comment: The only significant diff is that we were only doing the work to buildEndpointInfoList lazily, but that function only seems as expensive as flattenEndpointsInfo anyway (O(endpoints)), so I don't think we're burning any more cpu.

bprashanth · 2017-02-10T01:25:20Z

pkg/proxy/iptables/proxier.go

 			// record endpoints of unactive service to stale connections
 			for _, ep := range proxier.endpointsMap[svcPort] {
 				staleConnections[endpointServicePair{endpoint: ep.ip, servicePortName: svcPort}] = true
 			}
-
-			glog.V(2).Infof("Removing endpoints for %q", svcPort)


Comment: this version seems cleaner since we're not deleting from the map we're iterating, and there's no residual state as we start with a fresh map everytime.

bprashanth · 2017-02-10T01:26:48Z

/approve

freehan · 2017-02-10T18:04:16Z

Should we get this in and let it soak for some time before cherry-pick into 1.5?

k8s-github-robot · 2017-02-10T18:05:54Z

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: bprashanth, freehan

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

~~pkg/proxy/iptables/OWNERS~~ [bprashanth]

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

freehan · 2017-02-10T18:34:09Z

adding label based on #41223 (review)

k8s-github-robot · 2017-02-10T21:35:40Z

Automatic merge from submit-queue (batch tested with PRs 41223, 40892, 41220, 41207, 41242)

dcbw · 2017-02-11T04:43:16Z

@freehan I've had #39484 sitting around since forever that @thockin hasn't gotten to. That adds a boatload of testcases.

And one of those testcases catches an error in this PR, which is that healthcheck updates are not correctly added anymore because proxier.updateHealthCheckEntries() is no longer called for every svcPort in the new endpoints map too.

I'm happy to fix that up in #39484 if you like, provided I can actually get somebody (@thockin ? @bprashanth ?) to review it.

freehan · 2017-02-12T06:59:30Z

@dcbw Thanks for noticing!

Please go ahead with the fix in #39484.

dcbw · 2017-02-13T16:22:03Z

@freehan I've rebased and repushed #39484, would you mind taking a look at it too? Thanks!

thockin · 2017-02-13T16:47:41Z

Thanks guys,

I have been out for about 2 weeks (combo of vaca, work, sick) and am now trying to get all these PRs accounted for. Minhan can own the review.

I have another set of PRs between me, @timothysc, and @danwinship which make sync be called at a max rate - all together these should be a nice improvement.

Should we do this same transform over Service updates?

dcbw · 2017-02-13T16:48:50Z

@thockin yeah, we probably should do the same for Service updates, but it's not as big of an issue since Service updates are much less frequent than Endpoints.

freehan · 2017-02-13T19:59:09Z

@dcbw
I understand the bug you are referring in #41223 (comment)

I think that should not cause any problem at this point. Here is the theory:

create new service svc1
OnEndpointsUpdate gets called with new service endpoint.
Since updateHealthCheckEntries is only called for previous known service ports in proxier.endpointsMap, healthcheck for the newly added service endpoint will not be included.
OnEndpointsUpdate gets called again with the same of endpoints.
This time, proxier.endpointsMap includes svc1 and healthcheck is updated.

Since OnEndpointsUpdate is called at least twice a second (Due to master election logic in kube-controller-manager and kube-scheduler), having a half second delay to update health check should not cause any problem, right?

I intended to cherry-pick this into 1.5 to help some customers. I think #39484 is too big for cherry-pick. I can add a surgical fix to combine svcPorts from newEndpointMap and proxier.endpointsMap, and trigger updateHealthCheckEntries. For #39484, just need to rebase and delete the surgical fix.

What do you think?

Automatic merge from submit-queue fix healthcheck update problem introduced by #41223 ref: #41223 surgical fix for #41223 (comment)

…#41357-upstream-release-1.5 Automatic merge from submit-queue Automated cherry pick of #41223 #41357 Cherry pick of #41223 #41357 on release-1.5. #41223: skip iptables sync if no endpoint changes #41357: fix healthcheck update problem introduced by #41223

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

freehan assigned thockin and bprashanth Feb 10, 2017

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Feb 10, 2017

freehan mentioned this pull request Feb 10, 2017

[CRI] Add HostPort Support for Dockershim #35457

Closed

bprashanth reviewed Feb 10, 2017

View reviewed changes

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 10, 2017

skip iptables sync if no endpoint changes

87fe4dc

freehan force-pushed the kube-proxy-skip branch from 88c1f2f to 87fe4dc Compare February 10, 2017 18:03

freehan added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 10, 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 Feb 10, 2017

freehan added release-note-none Denotes a PR that doesn't merit a release note. 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 Feb 10, 2017

freehan added this to the v1.5 milestone Feb 10, 2017

k8s-github-robot merged commit fad9408 into kubernetes:master Feb 10, 2017

freehan mentioned this pull request Feb 10, 2017

skip endpointUpdate if endpoint checksum stays the same #41173

Closed

thockin mentioned this pull request Feb 13, 2017

Proxy defer on update events #41022

Merged

freehan added a commit to freehan/kubernetes that referenced this pull request Feb 13, 2017

fix healthcheck update problem introduced by kubernetes#41223

572e3be

freehan mentioned this pull request Feb 13, 2017

fix healthcheck update problem introduced by #41223 #41357

Merged

k8s-github-robot pushed a commit that referenced this pull request Feb 14, 2017

Merge pull request #41357 from freehan/kube-proxy-skip

b1e0d0e

Automatic merge from submit-queue fix healthcheck update problem introduced by #41223 ref: #41223 surgical fix for #41223 (comment)

freehan mentioned this pull request Feb 16, 2017

Automated cherry pick of #41223 #41357 #41578

Merged

freehan added a commit to freehan/kubernetes that referenced this pull request Feb 16, 2017

fix healthcheck update problem introduced by kubernetes#41223

21efcaf

ahakanbaba pushed a commit to ahakanbaba/kubernetes that referenced this pull request Feb 17, 2017

fix healthcheck update problem introduced by kubernetes#41223

00d9535

eden mentioned this pull request Mar 3, 2017

kube-proxy constantly syncing/restoring iptables rules, consuming CPU resources kubernetes/minikube#1158

Closed

databus23 pushed a commit to sapcc/kubernetes that referenced this pull request Mar 28, 2017

fix healthcheck update problem introduced by kubernetes#41223

ed56823

databus23 mentioned this pull request Mar 28, 2017

Fix kube proxy reload frenzy sapcc/kubernetes#2

Open

databus23 pushed a commit to sapcc/kubernetes that referenced this pull request May 15, 2017

fix healthcheck update problem introduced by kubernetes#41223

8a08948

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

skip iptables sync if no endpoint changes #41223

skip iptables sync if no endpoint changes #41223

freehan commented Feb 10, 2017 •

edited

k8s-reviewable commented Feb 10, 2017

freehan commented Feb 10, 2017

bprashanth left a comment

bprashanth Feb 10, 2017

bprashanth Feb 10, 2017

bprashanth Feb 10, 2017

bprashanth commented Feb 10, 2017

freehan commented Feb 10, 2017

k8s-github-robot commented Feb 10, 2017

freehan commented Feb 10, 2017

k8s-github-robot commented Feb 10, 2017

dcbw commented Feb 11, 2017

freehan commented Feb 12, 2017

dcbw commented Feb 13, 2017

thockin commented Feb 13, 2017

dcbw commented Feb 13, 2017

freehan commented Feb 13, 2017 •

edited

skip iptables sync if no endpoint changes #41223

skip iptables sync if no endpoint changes #41223

Conversation

freehan commented Feb 10, 2017 • edited

k8s-reviewable commented Feb 10, 2017

freehan commented Feb 10, 2017

bprashanth left a comment

Choose a reason for hiding this comment

bprashanth Feb 10, 2017

Choose a reason for hiding this comment

bprashanth Feb 10, 2017

Choose a reason for hiding this comment

bprashanth Feb 10, 2017

Choose a reason for hiding this comment

bprashanth commented Feb 10, 2017

freehan commented Feb 10, 2017

k8s-github-robot commented Feb 10, 2017

freehan commented Feb 10, 2017

k8s-github-robot commented Feb 10, 2017

dcbw commented Feb 11, 2017

freehan commented Feb 12, 2017

dcbw commented Feb 13, 2017

thockin commented Feb 13, 2017

dcbw commented Feb 13, 2017

freehan commented Feb 13, 2017 • edited

freehan commented Feb 10, 2017 •

edited

freehan commented Feb 13, 2017 •

edited