Skip to content

feat: support injected clock in ConditionSet for testability#204

Merged
DerekFrank merged 1 commit intoawslabs:mainfrom
jamesmt-aws:inject-clock-condition-set
Apr 9, 2026
Merged

feat: support injected clock in ConditionSet for testability#204
DerekFrank merged 1 commit intoawslabs:mainfrom
jamesmt-aws:inject-clock-condition-set

Conversation

@jamesmt-aws
Copy link
Copy Markdown
Contributor

Summary

ConditionSet.Set() uses metav1.Now() for LastTransitionTime, which returns real wall clock time. Controllers that check elapsed time since a condition was set using an injected fake clock get incorrect results because the condition timestamp and the controller clock diverge.

Add a WithClock option to ConditionTypes.For():

// Production (default, no change required)
conditions := status.NewReadyConditions(...).For(object)

// Tests with fake clock
conditions := status.NewReadyConditions(...).For(object, status.WithClock(fakeClock))

Defaults to clock.RealClock{}, so existing callers are unaffected.

Motivation

This fixes a class of test flakes in downstream projects. In kubernetes-sigs/karpenter, the termination controller checks c.clock.Since(condition.LastTransitionTime) using a fake clock, but LastTransitionTime was stamped with metav1.Now(). The test "should not finish draining until minDrainTime has passed" fails deterministically because the fake clock and real clock diverge (kubernetes-sigs/karpenter#2951).

The downstream workaround is to track timestamps separately using the injected clock, duplicating what LastTransitionTime already provides. This PR fixes the root cause.

Changes

  • condition_set.go: Add clock field to ConditionSet, ForOptions/ForOption/WithClock types, now() helper. Replace two metav1.Now() calls with c.now().
  • condition_set_test.go: Test that WithClock controls LastTransitionTime on set, transition, and same-status update.

Test plan

  • All existing tests pass (no behavioral change without WithClock)
  • New test verifies fake clock controls LastTransitionTime

ConditionSet.Set() used metav1.Now() (real clock) for LastTransitionTime.
Controllers that use a fake clock for time-dependent logic (e.g.,
checking elapsed time since a condition was set) get incorrect results
because the condition timestamp and the controller clock diverge.

Add a WithClock option to ConditionTypes.For() that injects a
clock.Clock into the ConditionSet. Defaults to clock.RealClock{}, so
existing callers are unaffected. Tests can pass a fake clock to get
deterministic LastTransitionTime values.

This fixes a class of test flakes in downstream projects (e.g.,
kubernetes-sigs/karpenter#2951) where controllers compare
clock.Since(condition.LastTransitionTime) using a fake clock but the
condition was stamped with real time.
Copy link
Copy Markdown
Contributor

@tzneal tzneal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@DerekFrank DerekFrank merged commit 02ff405 into awslabs:main Apr 9, 2026
1 check passed
jamesmt-aws added a commit to jamesmt-aws/karpenter that referenced this pull request Apr 9, 2026
operatorpkg now supports an injected clock for ConditionSet (awslabs/operatorpkg#204),
so LastTransitionTime can be stamped with a fake clock in tests. Use it.

Replace the drainStartTimes sync.Map workaround with reading
LastTransitionTime directly off the Drained condition. The termination
controller now sets the Drained condition via StatusConditionsWithClock,
which threads its injected clock through to operatorpkg's status.WithClock,
so LastTransitionTime is stamped with the same clock the controller reads
back.

Add NodeClaim.StatusConditionsWithClock as a sibling to StatusConditions().
The variadic shape we'd normally use can't go on StatusConditions() itself
because operatorpkg's status.Object interface requires a non-variadic
signature.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants