Fix FakeClock::Reset to always succeed #94317
Conversation
k8s-ci-robot
added
release-note-none
|
Hi @kevindelgado. 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. |
k8s-ci-robot
added
the
needs-ok-to-test
|
This PR came about while trying to write the unit test for #94235 using a fake clock instead of a real clock. The way the backoff manager resets its timer after successive iterations exposed the fact that the current fake clock implementation does not allow for resetting fake timers that have already fired. |
|
One outstanding question I have is whether this conflicts with the behavior previously fixed by 3e898d6#diff-2feab94c4d7d3f50c86be9853504ace6L313-L319. It seems to me that previously Reset() would reset the timer if already fired (while returning false). I’m unclear on the context of that change and if this is reverting something that was intentionally modified (albeit, it was modified to a behavior misaligned with the behavior of real clock). |
|
/cc @lavalamp |
|
/assign @caesarxuchao |
|
/ok-to-test Let's see if this breaks anything :) |
k8s-ci-robot
added
ok-to-test
|
looks like it passed, PTAL @lavalamp |
Current FakeClock::Reset only successfully resets the timer and returns true when the timer has neither fired nore been stopped. This is incorrect behavior as real clocks allow for reseting in both of those situations (as long has the channel has been drained). This is useful in tests that use a fake clock to test wait.BackofManager, as the timer resets after each iteration of Backoff() after the timer has already fired.
kevindelgado
force-pushed
the
draft/fix-fakeclock-reset
branch
from
5a7404a to
7e7ee90
Compare
|
/test pull-kubernetes-e2e-kind |
|
/lgtm thanks a ton for fixing this! I don't think this should break e2e tests, only unit tests. So should be good to go. |
|
@lavalamp: The provided milestone is not valid for this repository. Milestones in this repository: [ Use In response to this:
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. |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kevindelgado, lavalamp 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
|
/milestone v1.20 Missing the v, I guess that's v for "very sure I want 1.20". |
|
/remove-needs-sig |
justaugustus
removed
the
needs-sig
5 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
Currently, FakeClock::Reset only successfully resets the timer and returns true when the timer has neither fired nor been stopped.
This is incorrect behavior as real clocks allow for resetting in both of those situations (as long has the channel has been drained).
This is useful in tests that use a fake clock to test wait.BackofManager, as the timer resets after each iteration of Backoff() after the timer has already fired.
Does this PR introduce a user-facing change?: