Remove last endpoint for kubernetes Service during graceful shutdown of final kube-apiserver #116685

nayihz · 2023-03-16T15:02:32Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

The IP of the last one apiserver isn't removed from k8s svc endpoints on shutdown. Clients (using in-cluster API mode) will try to continue talking/connecting to that instance even after the apiserver is dead.

Which issue(s) this PR fixes:

Fixes #115804

Special notes for your reviewer:

Does this PR introduce a user-facing change?

kube-apiserver always removes its endpoint from kubernetes service during graceful shutdown (even if it's the only/last one)

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2023-03-16T15:02:43Z

Hi @czybjtu. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

nayihz · 2023-03-17T07:13:43Z

fix it according to your suggestion, please take a look when you have time. @aojea

pkg/controlplane/reconcilers/lease.go

sftim · 2023-03-17T13:15:16Z

pkg/controlplane/reconcilers/lease_test.go

@@ -503,6 +503,16 @@ func TestLeaseRemoveEndpoints(t *testing.T) {
 			expectUpdate:  makeEndpointsArray("foo", []string{"4.3.2.2", "4.3.2.3", "4.3.2.4"}, []corev1.EndpointPort{{Name: "foo", Port: 8080, Protocol: "TCP"}}),
 			expectLeases:  []string{"4.3.2.2", "4.3.2.3", "4.3.2.4"},
 		},
+		{
+			testName:      "the last apiserver was shutdown",


Suggested change

testName: "the last apiserver was shutdown",

testName: "the last API server was shut down cleanly",

sftim · 2023-03-17T13:17:08Z

pkg/controlplane/reconcilers/lease.go

💭 if we know that this is the last kube-apiserver in a cluster, which should we do:

remove the endpoint(s) representing us from the relevant EndpointSlice(s)

delete those EndpointSlice(s) (the first API server to start up would need to make a new one)

From an architectural / philosophical point of view, I'm not sure which feels more appropriate.

remove the endpoint(s) representing us from the relevant EndpointSlice(s)

delete those EndpointSlice(s) (the first API server to start up would need to make a new one)

the former, because you can never know if your are really the last one, or you are restarting, or .... I think that each apiserver should modify only its own IP, that is the one he is authoritative

sftim · 2023-03-17T13:18:17Z

This change is visible to end users. We should provide a release note.

sftim · 2023-03-17T13:19:17Z

/retitle Remove last endpoint for kubernetes Service during graceful shutdown of final kube-apiserver

aojea · 2023-03-19T13:53:08Z

pkg/controlplane/reconcilers/lease_test.go

+			r.StopReconciling()
 			err = r.RemoveEndpoints(test.serviceName, netutils.ParseIPSloppy(test.ip), test.endpointPorts)


does this no affect the other tests?
we may add more test cases to exercise the races you mentioned during the review

r.StopReconciling() has no side effect to current tests because the precondition for executing TestLeaseRemoveEndpoints function is that the apiserver has already been shutdown. It's better to add test case for the scenario of apiserver start up.

we may add more test cases to exercise the races

Yes, I'll do this later.

aojea · 2023-03-19T13:54:05Z

/lgtm cancel

the tests can be improved, also I couldn't look deeper to see if modifying the existing one can cause issues

nayihz · 2023-03-21T01:13:55Z

/retest-required

cici37 · 2023-03-21T20:02:08Z

/triage accepted

nayihz · 2023-04-27T13:57:38Z

kindly ping @aojea

aojea · 2023-04-27T14:02:25Z

kindly ping @aojea

yeah, if you don't ping me I would nto remember sorry, let me check it

aojea · 2023-04-27T22:03:16Z

/lgtm
/assign @wojtek-t

k8s-ci-robot · 2023-04-27T22:03:22Z

LGTM label has been added.

Git tree hash: 0394723b279d416652ed5f1ee1d73fc921ceee6f

wojtek-t · 2023-04-28T12:02:46Z

@aojea I have added release note - PTAL.

Overall this looks good - thanks for making this happen.

/lgtm
/approve

k8s-ci-robot · 2023-04-28T12:03:06Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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/controlplane/OWNERS~~ [wojtek-t]

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

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 16, 2023

k8s-ci-robot requested review from caesarxuchao and enj March 16, 2023 15:03

aojea reviewed Mar 17, 2023

View reviewed changes

pkg/controlplane/reconcilers/lease.go Show resolved Hide resolved

aojea reviewed Mar 17, 2023

View reviewed changes

pkg/controlplane/reconcilers/lease.go Show resolved Hide resolved

aojea reviewed Mar 17, 2023

View reviewed changes

pkg/controlplane/reconcilers/lease.go Show resolved Hide resolved

aojea reviewed Mar 17, 2023

View reviewed changes

pkg/controlplane/reconcilers/lease.go Outdated Show resolved Hide resolved

nayihz force-pushed the fix_lease_remove_endpoints branch from b99e83c to 3600d59 Compare March 17, 2023 12:36

nayihz marked this pull request as ready for review March 17, 2023 12:38

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 17, 2023

k8s-ci-robot requested review from deads2k and justinsb March 17, 2023 12:38

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 17, 2023

sftim reviewed Mar 17, 2023

View reviewed changes

k8s-ci-robot changed the title ~~remove last ip when apiserver was shut down~~ Remove last endpoint for kubernetes Service during graceful shutdown of final kube-apiserver Mar 17, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 19, 2023

aojea reviewed Mar 19, 2023

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 19, 2023

k8s-ci-robot requested review from aojea and wojtek-t March 19, 2023 13:54

nayihz force-pushed the fix_lease_remove_endpoints branch from 36ccb20 to 35519b1 Compare March 20, 2023 12:16

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 20, 2023

fix: remove last ip when apiserver was shut down

e567490

nayihz force-pushed the fix_lease_remove_endpoints branch from 35519b1 to e567490 Compare March 20, 2023 13:15

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 21, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 27, 2023

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 Apr 28, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 28, 2023

k8s-ci-robot merged commit f66e1a3 into kubernetes:master Apr 28, 2023

k8s-ci-robot added this to the v1.28 milestone Apr 28, 2023

nayihz deleted the fix_lease_remove_endpoints branch April 28, 2023 13:25

rtheis mentioned this pull request Jun 20, 2023

Restore Kubernetes API service endpoint during HA rolling restarts #118777

Closed

aojea mentioned this pull request Jul 10, 2023

cmd/kube-apiserver: turn core (legacy) rest storage into standard RESTStorageProvider #119042

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove last endpoint for kubernetes Service during graceful shutdown of final kube-apiserver #116685

Remove last endpoint for kubernetes Service during graceful shutdown of final kube-apiserver #116685

nayihz commented Mar 16, 2023 •

edited by wojtek-t

Loading

k8s-ci-robot commented Mar 16, 2023

nayihz commented Mar 17, 2023

sftim Mar 17, 2023

sftim Mar 17, 2023 •

edited

Loading

aojea Mar 17, 2023

sftim commented Mar 17, 2023

sftim commented Mar 17, 2023

aojea Mar 19, 2023

nayihz Mar 20, 2023 •

edited

Loading

aojea commented Mar 19, 2023

nayihz commented Mar 21, 2023

cici37 commented Mar 21, 2023

nayihz commented Apr 27, 2023

aojea commented Apr 27, 2023

aojea commented Apr 27, 2023

k8s-ci-robot commented Apr 27, 2023

wojtek-t commented Apr 28, 2023

k8s-ci-robot commented Apr 28, 2023

	testName: "the last apiserver was shutdown",
	testName: "the last API server was shut down cleanly",

		r.StopReconciling()
		err = r.RemoveEndpoints(test.serviceName, netutils.ParseIPSloppy(test.ip), test.endpointPorts)

Remove last endpoint for kubernetes Service during graceful shutdown of final kube-apiserver #116685

Remove last endpoint for kubernetes Service during graceful shutdown of final kube-apiserver #116685

Conversation

nayihz commented Mar 16, 2023 • edited by wojtek-t Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Mar 16, 2023

nayihz commented Mar 17, 2023

sftim Mar 17, 2023

Choose a reason for hiding this comment

sftim Mar 17, 2023 • edited Loading

Choose a reason for hiding this comment

aojea Mar 17, 2023

Choose a reason for hiding this comment

sftim commented Mar 17, 2023

sftim commented Mar 17, 2023

aojea Mar 19, 2023

Choose a reason for hiding this comment

nayihz Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

aojea commented Mar 19, 2023

nayihz commented Mar 21, 2023

cici37 commented Mar 21, 2023

nayihz commented Apr 27, 2023

aojea commented Apr 27, 2023

aojea commented Apr 27, 2023

k8s-ci-robot commented Apr 27, 2023

wojtek-t commented Apr 28, 2023

k8s-ci-robot commented Apr 28, 2023

nayihz commented Mar 16, 2023 •

edited by wojtek-t

Loading

sftim Mar 17, 2023 •

edited

Loading

nayihz Mar 20, 2023 •

edited

Loading