Ignore EndpointSlices that are marked for deletion #109624

aryan9600 · 2022-04-22T20:05:54Z

Signed-off-by: Sanskar Jaiswal jaiswalsanskar078@gmail.com

What type of PR is this?

/kind bug

What this PR does / why we need it:

Ignores EndpointSlices that have been marked for deletion, to prevent the controller from getting stuck while calculating stale slices for a service.

Which issue(s) this PR fixes:

Fixes #108779

Special notes for your reviewer:

Related issue: #108267

Does this PR introduce a user-facing change?

EndpointSlices marked for deletion are now ignored during reconciliation.

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

Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>

k8s-ci-robot · 2022-04-22T20:05:56Z

Please note that we're already in Test Freeze for the release-1.24 branch. This means every merged PR has to be cherry-picked into the release branch to be part of the upcoming v1.24.0 release.

k8s-ci-robot · 2022-04-22T20:06:02Z

@aryan9600: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

k8s-ci-robot · 2022-04-22T20:06:03Z

Hi @aryan9600. 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.

aryan9600 · 2022-04-22T20:08:37Z

/sig network

danwinship · 2022-04-25T13:05:46Z

/assign @robscott

robscott

Thanks for the work on this @aryan9600!

robscott · 2022-04-25T17:09:12Z

pkg/controller/util/endpointslice/endpointslice_tracker.go

@@ -100,6 +100,9 @@ func (est *EndpointSliceTracker) StaleSlices(service *v1.Service, endpointSlices
 		providedSlices[endpointSlice.UID] = endpointSlice.Generation
 		g, ok := gfs[endpointSlice.UID]
 		if ok && (g == deletionExpected || g > endpointSlice.Generation) {
+			if endpointSlice.DeletionTimestamp != nil {


I think this just pushes the problem further down. This will allow the sync to continue which is good, but we also need a change in the reconciler (or similar area) to ensure that the reconcile logic also ignores EndpointSlices that are terminating, otherwise we'll be in a worse place than when we started.

robscott · 2022-04-25T17:44:48Z

/ok-to-test

aojea · 2022-04-25T19:50:30Z

/cc

pkg/controller/endpointslice/endpointslice_controller.go

aojea · 2022-04-28T07:55:04Z

pkg/controller/endpointslice/endpointslice_controller.go

@@ -557,3 +560,14 @@ func trackSync(err error) {
 	}
 	endpointslicemetrics.EndpointSliceSyncs.WithLabelValues(metricLabel).Inc()
 }
+
+func dropEndpointSlicesPendingDeletion(endpointSlices []*discovery.EndpointSlice) []*discovery.EndpointSlice {


forget my previous comment, this is right

aojea · 2022-04-28T08:00:59Z

pkg/controller/endpointslice/endpointslice_controller.go

@@ -364,6 +364,9 @@ func (c *Controller) syncService(key string) error {
 		return err
 	}

+	// Drop EndpointSlices that have been marked for deletion to prevent the controller from getting stuck.
+	endpointSlices = dropEndpointSlicesPendingDeletion(endpointSlices)
+
 	if c.endpointSliceTracker.StaleSlices(service, endpointSlices) {


@robscott I can't remember all the details now of the tracker, is there anything else we should do in the endpointSliceTracker?

Commenting for future us - this is right, explained by https://github.com/kubernetes/kubernetes/pull/109624/files#r860592618.

robscott

Thanks @aryan9600! This change LGTM, just a couple tiny nits but will approve after those are addressed.

robscott · 2022-06-02T19:03:49Z

pkg/controller/endpointslice/endpointslice_controller_test.go

+	// The EndpointSlice marked for deletion should be ignored by the controller, and thus
+	// should not result in only one action from the client (on the initial EndpointSlice).


It would be helpful to explain which action we're actually expecting here. I'm assuming it's just a read from the controller?

Yes sure. It's not a read, it's an update where the non-terminating endpointslice's trigger time annotation is removed.

robscott · 2022-06-03T00:35:32Z

pkg/controller/endpointslice/endpointslice_controller_test.go

+	assert.Len(t, result, 2)
+	for _, endpointSlice := range endpointSlices {
+		if endpointSlice.Name == "epSlice1" {
+			t.Errorf("Expected EndpointSlice marked for deletion to be dropped.")
+		}
+	}


This is more complicated, but it would be nice to have some kind of test that actually verified the precise contents of the remaining list to be sure there's nothing funny happening like the objects being emptied out. (I know that's impossible with the current code, just would likely slightly more thorough tests here).

Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>

robscott · 2022-06-09T05:15:22Z

Thanks @aryan9600! I think this bug fix is worth cherry picking back to previous versions as well - I can approve if you create the cherry picks.

/lgtm
/approve

k8s-ci-robot · 2022-06-09T05:15:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aryan9600, robscott

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/controller/endpointslice/OWNERS~~ [robscott]
~~pkg/controller/util/endpointslice/OWNERS~~ [robscott]

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

aryan9600 · 2022-06-09T06:28:55Z

/retest-required

…09624-upstream-release-1.24 Automated cherry pick of #109624: Ignore EndpointSlices that are already marked for deletion

…09624-upstream-release-1.22 Automated cherry pick of #109624: Ignore EndpointSlices that are already marked for deletion

…09624-upstream-release-1.23 Automated cherry pick of #109624: Ignore EndpointSlices that are already marked for deletion

…/fix-endpointslice-deletion Ignore EndpointSlices that are marked for deletion Signed-off-by: Damien Grisonnet <dgrisonn@redhat.com>

Ignore EndpointSlices that are already marked for deletion

7d8048d

Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Apr 22, 2022

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 22, 2022

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Apr 22, 2022

k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 22, 2022

k8s-ci-robot requested review from danwinship and freehan April 22, 2022 20:11

k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Apr 22, 2022

k8s-ci-robot assigned robscott Apr 25, 2022

robscott reviewed Apr 25, 2022

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 25, 2022

k8s-ci-robot requested a review from aojea April 25, 2022 19:50

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

aryan9600 force-pushed the fix-endpointslice-deletion branch 2 times, most recently from ac8d9f6 to 3e5ef5a Compare April 26, 2022 07:03

aryan9600 requested a review from robscott April 26, 2022 19:50

robscott reviewed Apr 27, 2022

View reviewed changes

pkg/controller/endpointslice/endpointslice_controller.go Outdated Show resolved Hide resolved

aryan9600 force-pushed the fix-endpointslice-deletion branch 2 times, most recently from f28b044 to b473a9e Compare April 27, 2022 18:58

aojea reviewed Apr 28, 2022

View reviewed changes

robscott reviewed Jun 3, 2022

View reviewed changes

move the ignore logic higher up to the reconciler

4314e58

Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>

aryan9600 force-pushed the fix-endpointslice-deletion branch from b473a9e to 4314e58 Compare June 6, 2022 04:20

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 Jun 6, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 9, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 9, 2022

k8s-ci-robot merged commit e8d6b76 into kubernetes:master Jun 9, 2022

k8s-ci-robot added this to the v1.25 milestone Jun 9, 2022

aryan9600 mentioned this pull request Jun 9, 2022

Automated cherry pick of #109624: Ignore EndpointSlices that are already marked for deletion #110482

Merged

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 Jun 9, 2022

This was referenced Jun 9, 2022

Automated cherry pick of #109624: Ignore EndpointSlices that are already marked for deletion #110483

Merged

Automated cherry pick of #109624: Ignore EndpointSlices that are already marked for deletion #110484

Merged

k8s-ci-robot added a commit that referenced this pull request Jun 9, 2022

Merge pull request #110484 from aryan9600/automated-cherry-pick-of-#1…

fb3d64a

…09624-upstream-release-1.24 Automated cherry pick of #109624: Ignore EndpointSlices that are already marked for deletion

k8s-ci-robot added a commit that referenced this pull request Jun 9, 2022

Merge pull request #110482 from aryan9600/automated-cherry-pick-of-#1…

eeba55b

…09624-upstream-release-1.22 Automated cherry pick of #109624: Ignore EndpointSlices that are already marked for deletion

k8s-ci-robot added a commit that referenced this pull request Jun 9, 2022

Merge pull request #110483 from aryan9600/automated-cherry-pick-of-#1…

3943991

…09624-upstream-release-1.23 Automated cherry pick of #109624: Ignore EndpointSlices that are already marked for deletion

openshift-ci-robot mentioned this pull request Sep 2, 2022

UPSTREAM: 110639: endpointslices: node missing on Pod scenario openshift/kubernetes#1359

Merged

aryan9600 mentioned this pull request Sep 7, 2022

REQUEST: New membership for aryan9600 kubernetes/org#3664

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore EndpointSlices that are marked for deletion #109624

Ignore EndpointSlices that are marked for deletion #109624

aryan9600 commented Apr 22, 2022 •

edited

k8s-ci-robot commented Apr 22, 2022

k8s-ci-robot commented Apr 22, 2022

k8s-ci-robot commented Apr 22, 2022

aryan9600 commented Apr 22, 2022

danwinship commented Apr 25, 2022

robscott left a comment

robscott Apr 25, 2022

robscott commented Apr 25, 2022

aojea commented Apr 25, 2022

aojea Apr 28, 2022

aojea Apr 28, 2022

robscott Jun 9, 2022

robscott left a comment

robscott Jun 2, 2022

aryan9600 Jun 5, 2022

robscott Jun 3, 2022

robscott commented Jun 9, 2022 •

edited

k8s-ci-robot commented Jun 9, 2022

aryan9600 commented Jun 9, 2022

		// The EndpointSlice marked for deletion should be ignored by the controller, and thus
		// should not result in only one action from the client (on the initial EndpointSlice).

Ignore EndpointSlices that are marked for deletion #109624

Ignore EndpointSlices that are marked for deletion #109624

Conversation

aryan9600 commented Apr 22, 2022 • edited

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 Apr 22, 2022

k8s-ci-robot commented Apr 22, 2022

k8s-ci-robot commented Apr 22, 2022

aryan9600 commented Apr 22, 2022

danwinship commented Apr 25, 2022

robscott left a comment

Choose a reason for hiding this comment

robscott Apr 25, 2022

Choose a reason for hiding this comment

robscott commented Apr 25, 2022

aojea commented Apr 25, 2022

aojea Apr 28, 2022

Choose a reason for hiding this comment

aojea Apr 28, 2022

Choose a reason for hiding this comment

robscott Jun 9, 2022

Choose a reason for hiding this comment

robscott left a comment

Choose a reason for hiding this comment

robscott Jun 2, 2022

Choose a reason for hiding this comment

aryan9600 Jun 5, 2022

Choose a reason for hiding this comment

robscott Jun 3, 2022

Choose a reason for hiding this comment

robscott commented Jun 9, 2022 • edited

k8s-ci-robot commented Jun 9, 2022

aryan9600 commented Jun 9, 2022

aryan9600 commented Apr 22, 2022 •

edited

robscott commented Jun 9, 2022 •

edited