Automated cherry pick of #115620: client-go/cache: fix missing delete event on replace (+ #116623) #115899
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>
k8s-ci-robot
added
the
do-not-merge/cherry-pick-not-approved
k8s-ci-robot
added
the
size/L
|
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. |
|
Unknown CLA label state. Rechecking for CLA labels. Send feedback to sig-contributor-experience at kubernetes/community. /check-cla |
k8s-ci-robot
added
the
cncf-cla: yes
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 2f4e0375f7cb0515efe7b666e8f0aadcddf81972
|
|
/kind bug /lgtm @kubernetes/release-managers - for cherrypick approval |
k8s-ci-robot
added
kind/bug
k8s-ci-robot
added
the
approved
|
Adding a hold while we discuss #115658 (comment). /hold |
k8s-ci-robot
added
the
do-not-merge/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. |
k8s-ci-robot
removed
the
lgtm
k8s-ci-robot
added
the
sig/api-machinery
odinuge
force-pushed
the
automated-cherry-pick-of-#115620-upstream-release-1.26
branch
from
53a77a0
to
8e5699f
Compare
k8s-ci-robot
added
the
do-not-merge/invalid-commit-message
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>
odinuge
force-pushed
the
automated-cherry-pick-of-#115620-upstream-release-1.26
branch
from
8e5699f
to
a8d2bc0
Compare
k8s-ci-robot
removed
the
do-not-merge/invalid-commit-message
|
/lgtm I'm not sure why the bot is angry about the commit message? |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 46fce6ababa1b79249613113c45ec02bc5e6a213
|
odinuge
changed the title
|
/hold cancel cc @kubernetes/release-managers for approval |
k8s-ci-robot
removed
the
do-not-merge/hold
|
/cc |
|
Shouldn't the release note be added together as in original PR? |
k8s-ci-robot
added
the
release-note
xmudrii
added
the
cherry-pick-approved
|
[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 |
k8s-ci-robot
removed
the
do-not-merge/cherry-pick-not-approved
Cherry pick of #115620 on release-1.26.
#115620: client-go/cache: fix missing delete event on replace
For details on the cherry pick process, see the cherry pick requests page.