Add test for replace events in client-go controller #135665
Conversation
k8s-ci-robot
added
the
release-note-none
|
Please note that we're already in Test Freeze for the Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Mon Dec 8 15:38:13 UTC 2025. |
k8s-ci-robot
added
kind/feature
|
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-sigs/prow repository. |
michaelasp
changed the title
k8s-ci-robot
added
the
needs-priority
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: michaelasp The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/cc @serathius |
k8s-ci-robot
added
sig/api-machinery
| updateCh: make(chan bool, 1), | ||
| } | ||
| fifo := NewRealFIFOWithOptions(RealFIFOOptions{ | ||
| KeyFunction: MetaNamespaceKeyFunc, |
There was a problem hiding this comment.
Why do we use either DeletionHandlingMetaNamespaceKeyFunc or MetaNamespaceKeyFunc? Could we ensure the whole test using same KeyFunc?
There was a problem hiding this comment.
We don't even need to pass the keyfunc to the FIFO, we can use DeletionHandlingMetaNamespaceKeyFunc here but it's not required. I just removed this from being passed to the FIFO.
| } | ||
|
|
||
| select { | ||
| case <-m.updateCh: |
There was a problem hiding this comment.
The more "kosher" way to do that would be using sync.Cond. See
There was a problem hiding this comment.
I was under the assumption that a cvar was more low level, in our case I'd like to wait on the channel but time out if the context is cancelled. Channels let us do that but a cvar would require a bit more work, right? This is a bit less efficient than a raw cvar but gives us more flexibility on the select. Not too picky either way though, especially for a test.
| expectHistory := []eventRecord{ | ||
| {Action: "add", Key: "default/pod-add"}, | ||
| {Action: "delete", Key: "default/pod-del"}, | ||
| {Action: "update", Key: "default/pod-keep"}, |
There was a problem hiding this comment.
Food for thought as a future idea. If replace just just triggered based on watch resynchronization there is technically no reason to trigger update for it if the RV is matching. There is a small change that value was corrupted and we wanted to refresh it in controller, so we might need to design a separate mechanism to detect that, but it would be much cheaper for controllers.
michaelasp
force-pushed
the
replaceTest
branch
from
5aa45ec to
ab33a33
Compare
3 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds additional testing to replace events to ensure that replace events and the underlying store is correct.
Which issue(s) this PR is related to:
KEP: kubernetes/enhancements#5647
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: