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
Automated cherry pick of #115620: client-go/cache: fix missing delete event on replace (+ #116623) #115900
Automated cherry pick of #115620: client-go/cache: fix missing delete event on replace (+ #116623) #115900
Conversation
This fixes a race condition when a "short lived" object is created and the create event is still present on the queue when a relist replaces the state. Previously that would lead in the object being leaked. The way this could happen is roughly; 1. new Object is added O, agent gets CREATED event for it 2. watch is terminated, and the agent runs a new list, L 3. CREATE event for O is still on the queue to be processed. 4. informer replaces the old data in store with L, and O is not in L - Since O is not in the store, and not in the list L, no DELETED event is queued 5. CREATE event for O is still on the queue to be processed. 6. CREATE event for O is processed 7. O is <leaked>; its present in the cache but not in k8s. With this patch, on step 4. above it would create a DELETED event ensuring that the object will be removed. Signed-off-by: Odin Ugedal <ougedal@palantir.com> Signed-off-by: Odin Ugedal <odin@uged.al>
…ects This fixes an issue where a relist could result in a DELETED delta with an object wrapped in a DeletedFinalStateUnknown object; and then on the next relist, it would wrap that object inside another DeletedFinalStateUnknown, leaving the user with a "double" layer of DeletedFinalStateUnknown's. Signed-off-by: Odin Ugedal <ougedal@palantir.com> Signed-off-by: Odin Ugedal <odin@uged.al>
Signed-off-by: Odin Ugedal <ougedal@palantir.com> Signed-off-by: Odin Ugedal <odin@uged.al>
This is useful to both reduce the code complexity, and to ensure clients get the "newest" version of an object known when its deleted. This is all best-effort, but for clients it makes more sense giving them the newest object they observed rather than an old one. This is especially useful when an object is recreated. eg. Object A with key K is in the KnownObjects store; - DELETE delta for A is queued with key K - CREATE delta for B is queued with key K - Replace without any object with key K in it. In this situation its better to create a DELETE delta with DeletedFinalStateUnknown with B (with this patch), than it is to give the client an DeletedFinalStateUnknown with A (without this patch). Signed-off-by: Odin Ugedal <ougedal@palantir.com> Signed-off-by: Odin Ugedal <odin@uged.al>
Since the behavior is now changed, and the old behavior leaked objects, this adds a new comment about how Replace works. Signed-off-by: Odin Ugedal <ougedal@palantir.com> Signed-off-by: Odin Ugedal <odin@uged.al>
This cherry pick PR is for a release branch and has not yet been approved by Release Managers. To merge this cherry pick, it must first be approved ( AFTER it has been approved by code owners, please leave the following comment on a line by itself, with no leading whitespace: /cc kubernetes/release-managers (This command will request a cherry pick review from Release Managers and should work for all GitHub users, whether they are members of the Kubernetes GitHub organization or not.) For details on the patch release process and schedule, see the Patch Releases page. 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. |
LGTM label has been added. Git tree hash: 529cf854ebd0493b0790ef0d5f9f4bcdf3446da3
|
/kind bug /lgtm @kubernetes/release-managers - for cherrypick approval |
/lgtm |
Adding a hold while we discuss #115658 (comment). /hold |
@lavalamp per @odinuge last comment on #115658 (comment), I'm happy to add this as a cherry pick for the upcoming patches. Can you please remove the hold tag? thanks! |
Please add #116623 to this cherry pick and then it should be good to go. |
odinuge: sorted out some function signature changes during cherry-picking that caused conflicts. (cherry picked from commit e76dff3) Signed-off-by: Odin Ugedal <odin@uged.al>
876b515
to
a247e48
Compare
/retest |
/hold cancel cc @kubernetes/release-managers for approval |
Mind +1'ing again @lavalap, thanks! |
/lgtm |
LGTM label has been added. Git tree hash: 331712b5c8d7ac3cb213329506a8da1d5298a396
|
Shouldn't the release note be added together as in original PR? |
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.
For RelEng:
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lavalamp, odinuge, wojtek-t, xmudrii 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 |
Cherry pick of #115620 on release-1.25.
#115620: client-go/cache: fix missing delete event on replace
For details on the cherry pick process, see the cherry pick requests page.