New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ignore EndpointSlices that are marked for deletion #109624
Ignore EndpointSlices that are marked for deletion #109624
Conversation
Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
Please note that we're already in Test Freeze for the |
@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 The 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. |
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 Once the patch is verified, the new status will be reflected by the 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. |
/sig network |
/assign @robscott |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work on this @aryan9600!
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
/ok-to-test |
/cc |
ac8d9f6
to
3e5ef5a
Compare
f28b044
to
b473a9e
Compare
@@ -557,3 +560,14 @@ func trackSync(err error) { | |||
} | |||
endpointslicemetrics.EndpointSliceSyncs.WithLabelValues(metricLabel).Inc() | |||
} | |||
|
|||
func dropEndpointSlicesPendingDeletion(endpointSlices []*discovery.EndpointSlice) []*discovery.EndpointSlice { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forget my previous comment, this is right
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@robscott I can't remember all the details now of the tracker, is there anything else we should do in the endpointSliceTracker
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commenting for future us - this is right, explained by https://github.com/kubernetes/kubernetes/pull/109624/files#r860592618.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @aryan9600! This change LGTM, just a couple tiny nits but will approve after those are addressed.
// 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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be helpful to explain which action we're actually expecting here. I'm assuming it's just a read from the controller?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes sure. It's not a read, it's an update where the non-terminating endpointslice's trigger time annotation is removed.
assert.Len(t, result, 2) | ||
for _, endpointSlice := range endpointSlices { | ||
if endpointSlice.Name == "epSlice1" { | ||
t.Errorf("Expected EndpointSlice marked for deletion to be dropped.") | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
b473a9e
to
4314e58
Compare
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 |
[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:
Approvers can indicate their approval by writing |
/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>
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?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: