Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Testing with time #589

Open
andrew-su opened this issue Feb 8, 2025 · 4 comments
Open

Testing with time #589

andrew-su opened this issue Feb 8, 2025 · 4 comments

Comments

@andrew-su
Copy link
Contributor

We have a reconciler which does some heavy processing and may take a lot of time. After it completes its tasks we want to stamp out the completion time + 5 minutes in the status for example.

During tests, this happens quick but still it's off by a few nanoseconds which we can fix by using the provided time package and setting Now in the tests. However this means we need to switch from using regular time package to the one provided by RR. This is a problem because it won't be accurate to when the task completed + 5m during a real reconcile. I see that the ResourceReconciler stashes the current time at the beginning of the reconcile.

ctx = rtime.StashNow(ctx, time.Now())

Maybe for time testing a way to provide more gocmp options (approximating time) or to have a way to disable the auto now time stashing for ResourceReconciler.

Do you have another idea on steps to work around this?

@mamachanko
Copy link
Contributor

mamachanko commented Feb 8, 2025

@andrew-su Have you considered to inject a clock abstraction into your reconciler? That way you get precise control over the passage of time in your tests. ⏱️

For example, you could use k8s.io/utils/clock's RealClock in your live reconciler and PassiveClock in your tests.

@mamachanko
Copy link
Contributor

@andrew-su Have you considered to inject a clock abstraction into your reconciler? That way you get precise control over the passage of time in your tests. ⏱️

For example, you could use k8s.io/utils/clock's RealClock in your live reconciler and PassiveClock in your tests.

🤔 This may not work, because (sub)reconciler tests don't give you a hook where you would call "advance time by 3m 45s", say.

However, you may be successful with injecting a mock clock. Before your test runs you say what time should be returned for each invocation. It couples your implementation to how often the time is retrieved though.

@scothis
Copy link
Member

scothis commented Feb 8, 2025

The intent behind rtime was to normalize the timestamp in conditions. It also helps with testing since you can deterministically know the time that will be set into the status. Capturing the time at the start of the reconcile is intended to represent the time of the state being reconciled.

Setting a timestamp, absent other meaningful changes, can cause a reconcile loop as the resource will be updated with each reconcile and never reach a steady state. This is why condition timestamps mark the last change of the condition, and not the last time the condition's value was affirmed.

Dealing with actual time always get tricky because of drift, machine speed, breakpoint pauses, etc. Defining a custom cmp comparer or ignore the value with a filter would help manage drift. We'd need a way to add custom opts for cmp.Diff() calls. I thought we added this ability in the past, but I don't see it.

@mamachanko
Copy link
Contributor

[...] We'd need a way to add custom opts for cmp.Diff() calls. I thought we added this ability in the past, but I don't see it.

@scothis The last thing I recall being related was VerifyStashedValue d526d74 which does not expose cmp options directly but a wat to set a custom assertion function.

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

No branches or pull requests

3 participants