Allow nodeName updates when endPoint is updated. #68575

prameshj · 2018-09-12T19:48:15Z

This change removes a validation check that disallows nodeName change in a service endpoint update.
One scenario where nodeName can change for the same ip address is if the endpoints are in hostNetwork mode and nodes are being added/deleted.
With the current validation check, if endpoints controller misses a pod delete event, future endpoint updates will never succeed. This will result in stale
endpoints for the service.

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #66720
Special notes for your reviewer:

Release note:

Allows changing nodeName in endpoint update.

bowei · 2018-09-12T19:50:23Z

testing?

bowei · 2018-09-12T19:50:31Z

/ok-to-test

bowei · 2018-09-12T19:50:37Z

this needs a release note

prameshj · 2018-09-12T21:17:59Z

Verified via kubectl that nodeNames can now be changed for endpoints. Without my change, I see the error:
'Endpoints "test" is invalid: [subsets[0].addresses[1].nodeName: Forbidden: Cannot change NodeName for ...'

bowei · 2018-09-12T21:29:01Z

/assign @freehan

bowei · 2018-09-12T21:33:32Z

/assign @thockin for approval

freehan · 2018-09-12T21:54:55Z

pkg/apis/core/validation/validation_test.go

@@ -12968,8 +12968,8 @@ func TestEndpointAddressNodeNameUpdateRestrictions(t *testing.T) {
 	// Check that NodeName cannot be changed during update (if already set)


s/cannot/can

freehan · 2018-09-13T17:26:07Z

pkg/apis/core/validation/validation_test.go

@@ -12965,11 +12965,11 @@ func newNodeNameEndpoint(nodeName string) *core.Endpoints {
 func TestEndpointAddressNodeNameUpdateRestrictions(t *testing.T) {
 	oldEndpoint := newNodeNameEndpoint("kubernetes-node-setup-by-backend")
 	updatedEndpoint := newNodeNameEndpoint("kubernetes-changed-nodename")
-	// Check that NodeName cannot be changed during update (if already set)
+	// Check that NodeName can be changed during update


add a comment:
This is to accommodate the case where node IP changed or pod CIDR changed.

nit:add the comment

freehan · 2018-09-13T17:27:15Z

pkg/apis/core/validation/validation.go

@@ -5134,19 +5134,6 @@ func buildEndpointAddressNodeNameMap(subsets []core.EndpointSubset) map[string]s
 	return ipToNodeName


I think you can also get rid of the helper function that constructs ipToNode.

One scenario where nodeName can change for the same ip address is if the endpoints are in hostNetwork mode and nodes are being added/deleted. With the current validation check, if endpoints controller misses a pod delete event, future endpoint updates will never succeed. removed unused helper functions

prameshj · 2018-09-17T15:23:49Z

/retest

freehan · 2018-09-17T18:45:13Z

/lgtm

freehan · 2018-09-17T18:45:31Z

/assign @thockin for approval

k8s-ci-robot · 2018-09-17T18:45:32Z

@freehan: GitHub didn't allow me to assign the following users: for, approval.

Note that only kubernetes members and repo collaborators can be assigned.
For more information please see the contributor guide

In response to this:

/assign @thockin for approval

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.

thockin · 2018-09-17T21:32:45Z

So, to trigger this bug you have to have an pod on node X with IP A, node X goes away, node Y comes up. Controller manager misses the old pod going away. Pod gets rescheduled onto Y and gets address A again?

That seems pretty niche - am I getting it right?

prameshj · 2018-09-17T21:42:23Z

That is one case. You can also trigger it with service endpoints in hostNetwork mode and nodes going up and down - autoscaling or other reasons. If there are a lot of these changes, endpoints controller can miss the endpoint deletion events.

freehan · 2018-09-17T21:50:03Z

This bug triggers more often with preempt-able VMs + pods with host network.

thockin · 2018-09-17T23:08:06Z

Missing events is a MAJOR bug. If we can demonstrate that, we should really REALLY fix that.

That said, I am going to OK this. I think it was pedantic, and should be OK.

/lgtm
/approve

k8s-ci-robot · 2018-09-17T23:08:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: freehan, prameshj, thockin

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:

~~pkg/apis/core/OWNERS~~ [thockin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…75-upstream-release-1.12 Automated cherry pick of #68575: Allow nodeName updates when endPoint is updated.

ottoyiu · 2018-10-05T21:20:06Z

Confirmed to be hitting this by having scale down events (removal of nodes) in v1.10.5. Thanks for the fix!

lfundaro · 2018-10-12T08:09:56Z

Thanks for the fix ! I think this also solves the issue referenced here #69668 . I am not able to confirm as we use kubernetes through GCP and they have not yet released v1.12.1 but looks like the same problem.

liggitt · 2018-11-30T01:39:23Z

Any reason not to pick this to 1.10 and 1.11 as well?

liggitt · 2018-11-30T16:08:31Z

Missing events is a MAJOR bug. If we can demonstrate that, we should really REALLY fix that.

Not reacting to individual events is possible, and I don't think it's a bug. The validation prior to this PR depended on the endpoints controller first deleting the IP+old-node from the endpoints object, then separately adding the IP+new-node. That's an edge-triggered behavior, and we want to be level-triggered, so that the following works:

endpoints controller computes endpoints for service foo to be {pod:a, node:a, ip: 1.2.3.4}
endpoints controller goes down (or drops watch)
pod a is deleted, node a is deleted, node b is created, pod b is created with IP 1.2.3.4
endpoints controller comes up (or relists)
endpoints controller computes endpoints for service foo to be {pod:b, node:b, ip: 1.2.3.4} and tries to do a single update that changes the node name associated with IP 1.2.3.4

To accomodate that, loosening the validation to allow a change is correct.

liggitt · 2018-11-30T16:09:23Z

opened picks to 1.10 and 1.11

…5-upstream-release-1.11 Automated cherry pick of #68575: Allow nodeName updates when endPoint is updated.

…5-upstream-release-1.10 Automated cherry pick of #68575: Allow nodeName updates when endPoint is updated.

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 12, 2018

k8s-ci-robot requested review from jbeda and yujuhong September 12, 2018 19:48

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 12, 2018

prameshj force-pushed the ep-validation branch from 859c0c6 to f59607f Compare September 12, 2018 20:00

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 12, 2018

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Sep 12, 2018

k8s-ci-robot assigned freehan Sep 12, 2018

freehan added the kind/bug Categorizes issue or PR as related to a bug. label Sep 12, 2018

k8s-ci-robot removed the needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Sep 12, 2018

freehan reviewed Sep 12, 2018

View reviewed changes

prameshj force-pushed the ep-validation branch from f59607f to 93fbc8e Compare September 12, 2018 23:13

freehan reviewed Sep 13, 2018

View reviewed changes

prameshj force-pushed the ep-validation branch from 93fbc8e to f252b1d Compare September 14, 2018 19:42

k8s-ci-robot removed the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 14, 2018

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 14, 2018

prameshj force-pushed the ep-validation branch from f252b1d to e588ae5 Compare September 14, 2018 22:36

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 17, 2018

k8s-ci-robot assigned thockin Sep 17, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 17, 2018

k8s-ci-robot merged commit f289353 into kubernetes:master Sep 25, 2018

prameshj mentioned this pull request Oct 1, 2018

Automated cherry pick of #68575: Allow nodeName updates when endPoint is updated. #69297

Merged

k8s-ci-robot added a commit that referenced this pull request Oct 3, 2018

Merge pull request #69297 from prameshj/automated-cherry-pick-of-#685…

a43e3cf

…75-upstream-release-1.12 Automated cherry pick of #68575: Allow nodeName updates when endPoint is updated.

MrHohn mentioned this pull request Oct 11, 2018

Stale node IPs on endpoint resource #69668

Closed

prameshj mentioned this pull request Nov 15, 2018

REQUEST: New membership for prameshj kubernetes/org#243

Closed

6 tasks

liggitt mentioned this pull request Nov 30, 2018

Automated cherry pick of #68575: Allow nodeName updates when endPoint is updated. #71604

Merged

liggitt mentioned this pull request Nov 30, 2018

Automated cherry pick of #68575: Allow nodeName updates when endPoint is updated. #71605

Merged

k8s-ci-robot added a commit that referenced this pull request Dec 3, 2018

Merge pull request #71604 from liggitt/automated-cherry-pick-of-#6857…

19cdb33

…5-upstream-release-1.11 Automated cherry pick of #68575: Allow nodeName updates when endPoint is updated.

k8s-ci-robot added a commit that referenced this pull request Dec 10, 2018

Merge pull request #71605 from liggitt/automated-cherry-pick-of-#6857…

06fa582

…5-upstream-release-1.10 Automated cherry pick of #68575: Allow nodeName updates when endPoint is updated.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow nodeName updates when endPoint is updated. #68575

Allow nodeName updates when endPoint is updated. #68575

prameshj commented Sep 12, 2018 •

edited

Loading

bowei commented Sep 12, 2018

bowei commented Sep 12, 2018

bowei commented Sep 12, 2018

prameshj commented Sep 12, 2018

bowei commented Sep 12, 2018

bowei commented Sep 12, 2018

freehan Sep 12, 2018

freehan Sep 13, 2018

freehan Sep 14, 2018

freehan Sep 13, 2018

prameshj commented Sep 17, 2018

freehan commented Sep 17, 2018

freehan commented Sep 17, 2018

k8s-ci-robot commented Sep 17, 2018

thockin commented Sep 17, 2018

prameshj commented Sep 17, 2018

freehan commented Sep 17, 2018

thockin commented Sep 17, 2018

k8s-ci-robot commented Sep 17, 2018

ottoyiu commented Oct 5, 2018 •

edited

Loading

lfundaro commented Oct 12, 2018

liggitt commented Nov 30, 2018

liggitt commented Nov 30, 2018 •

edited

Loading

liggitt commented Nov 30, 2018

		@@ -12968,8 +12968,8 @@ func TestEndpointAddressNodeNameUpdateRestrictions(t *testing.T) {
		// Check that NodeName cannot be changed during update (if already set)

		@@ -5134,19 +5134,6 @@ func buildEndpointAddressNodeNameMap(subsets []core.EndpointSubset) map[string]s
		return ipToNodeName

Allow nodeName updates when endPoint is updated. #68575

Allow nodeName updates when endPoint is updated. #68575

Conversation

prameshj commented Sep 12, 2018 • edited Loading

bowei commented Sep 12, 2018

bowei commented Sep 12, 2018

bowei commented Sep 12, 2018

prameshj commented Sep 12, 2018

bowei commented Sep 12, 2018

bowei commented Sep 12, 2018

freehan Sep 12, 2018

Choose a reason for hiding this comment

freehan Sep 13, 2018

Choose a reason for hiding this comment

freehan Sep 14, 2018

Choose a reason for hiding this comment

freehan Sep 13, 2018

Choose a reason for hiding this comment

prameshj commented Sep 17, 2018

freehan commented Sep 17, 2018

freehan commented Sep 17, 2018

k8s-ci-robot commented Sep 17, 2018

thockin commented Sep 17, 2018

prameshj commented Sep 17, 2018

freehan commented Sep 17, 2018

thockin commented Sep 17, 2018

k8s-ci-robot commented Sep 17, 2018

ottoyiu commented Oct 5, 2018 • edited Loading

lfundaro commented Oct 12, 2018

liggitt commented Nov 30, 2018

liggitt commented Nov 30, 2018 • edited Loading

liggitt commented Nov 30, 2018

prameshj commented Sep 12, 2018 •

edited

Loading

ottoyiu commented Oct 5, 2018 •

edited

Loading

liggitt commented Nov 30, 2018 •

edited

Loading