cache: Reflector should have the same injected clock as its informer #115077
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
size/S
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: smarterclayton 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
added
the
approved
While refactoring the backoff manager to simplify and unify the code in wait a race condition was encountered in TestSharedInformerWatchDisruption. The new implementation failed because the fake clock was not propagated to the backoff managers when the reflector was used in a controller. After ensuring the mangaers, reflector, controller, and informer shared the same clock the test needed was updated to avoid the race condition by advancing the fake clock and adding real sleeps to wait for asynchronous propagation of the various goroutines in the controller. Due to the deep structure of informers it is difficult to inject hooks to avoid having to perform sleeps. At a minimum the FakeClock interface should allow a caller to determine the number of waiting timers (to avoid the first sleep).
k8s-ci-robot
added
sig/api-machinery
smarterclayton
force-pushed
the
reflector_mock_clock
branch
from
b78d9f2
to
91b3a81
Compare
k8s-ci-robot
added
release-note-none
aojea
reviewed
Jan 15, 2023
| @@ -348,6 +348,18 @@ func TestSharedInformerWatchDisruption(t *testing.T) { | |||
| // Simulate a connection loss (or even just a too-old-watch) | |||
| source.ResetWatch() | |||
|
|
|||
| // Wait long enough for the reflector to exit and the backoff function to start waiting | |||
There was a problem hiding this comment.
just curios, what is "long enough" in this context? the time to execute all these actions?
There was a problem hiding this comment.
In this test long enough is "for the goscheduler to kick in, switch to the waiting goroutine, and then run up until the point we try to get the timer channel from timer, which registers us with the fake clock so 'Step' actually does something". I.e. on the order of nano seconds but we don't have a way currently to inject a deterministic "wait until a timer is created on the fake clock and passes this argument".
|
/test pull-kubernetes-node-e2e-containerd unrelated failure
/lgtm test doesn't flake with current sleeps |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 4b275cc1517278d2e8a3ea8e7afc723475dad0dc
|
|
TFTR! |
This was referenced Jan 17, 2023
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NOTE: This pull is part of a series of changes that introduce new context-cancellation-aware Poll methods, reduce the surface area of the wait package to a smaller set of functions (if unused, marked private, if consolidating, marked deprecated), unify the underlying loop implementation with better testing, consolidate the backoff manager code into a smaller chunk, and in general address a number of outstanding issues. See #115077, #115116, #115113, #115140, #115064, and #107826.
While refactoring the backoff manager to simplify and unify the code in wait a race condition was encountered in TestSharedInformerWatchDisruption. The new implementation failed because the fake clock was not propagated to the reflector AND backoff managers (right now the backoff managers in tests would be using a real clock). After ensuring the reflector, controller, and informer shared the same clock the test needed to be updated to avoid the race condition by advancing the fake clock and adding real sleeps to wait for asynchronous propagation of the various goroutines in the controller.
Due to the deep structure of informers it is difficult to inject hooks to avoid having to perform sleeps. At a minimum the FakeClock interface should allow a caller to determine the number of waiting timers (to avoid the first sleep).
Included in #115064 but called out separately here for independent tests.
/kind cleanup
/kind flake