Replace time.Now() code with clock.Clock.Now()

[PBundyra]

What would you like to be added:
There are packages like e.g. pkg/scheduler that use time.Now(). As a result this code is hard to test and prone to flakiness as we are not able to directly control the time during tests. It should be replaced with clock.Clock.Now() as it is in WorkloadController

Why is this needed:

Completion requirements:

This enhancement requires the following artifacts:

The artifacts should be linked in subsequent comments.

[PBundyra]

/cc @mbobrovskyi

[TusharMohapatra07]

can i work on this issue ? @PBundyra @mbobrovskyi

[mbobrovskyi]

can i work on this issue ? @PBundyra @mbobrovskyi

Sure, please take it.

[TusharMohapatra07]

@mbobrovskyi thanks! i'm on it

[mbobrovskyi]

/assign @TusharMohapatra07

[TusharMohapatra07]

@mbobrovskyi am i supposed to change the time.Now() instance everywhere inside the project or just those instances which are present inside pkg folder ??

[mbobrovskyi]

Let's focus on pkg/scheduler. This is an example

kueue/pkg/controller/core/workload_controller.go

Line 108 in 74286b4

clock clock.Clock

.

[mimowo]

I would say the end goal is to use it everywhere, but starting with scheduler might be a good idea to see if there are any obstacles, then feel free to follow up in the next PR.

[TusharMohapatra07]

@mimowo understood

[mbobrovskyi]

/reopen

To fix it in other places.

[k8s-ci-robot]

@mbobrovskyi: Reopened this issue.

In response to this:

/reopen

To fix it in other places.

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.

[TusharMohapatra07]

@mbobrovskyi yes, i'm on it

[TusharMohapatra07]

@mbobrovskyi I don't see any significant place to replace time.Now in pkg/workload. Please confirm !

[mbobrovskyi]

What about this one

kueue/pkg/workload/admissionchecks.go

Line 96 in 0f54466

LastTransitionTime: metav1.NewTime(time.Now()),

?

Also here

kueue/pkg/workload/admissionchecks.go

Line 110 in 0f54466

newCheck.LastTransitionTime = metav1.NewTime(time.Now())

.

Could you please check is it possible to propagate clock.Clock? Or maybe just now time.Time.

This is an example

kueue/pkg/workload/admissionchecks.go

Line 32 in 0f54466

func SyncAdmittedCondition(w *kueue.Workload, now time.Time) bool {

[TusharMohapatra07]

@mbobrovskyi Actually, LastTransitionTime takes metav1.Time, so time.Time won't work ig. We could probably pass in the clock.Clock interface inside the function to proceed with this..

[mbobrovskyi]

@mbobrovskyi Actually, LastTransitionTime takes metav1.Time, so time.Time won't work ig. We could probably pass in the clock.Clock interface inside the function to proceed with this..

Why we can't use like this LastTransitionTime: metav1.NewTime(clock.Now()) or LastTransitionTime: metav1.NewTime(now)?

I think just propagation to function is enough for it. Also time.Time is enough.

[TusharMohapatra07]

@mbobrovskyi so, if we propogate it in the function, we have to pass it whenever we're calling the function ?
For ex:-

kueue/pkg/scheduler/preemption/preemption.go

Line 225 in 0f54466

workload.ResetChecksOnEviction(w)

[mbobrovskyi]

Exactly and Preemptor should have clock? If not please add to Preemptor as well.

[TusharMohapatra07]

@mbobrovskyi understood

[TusharMohapatra07]

Hey @mbobrovskyi, sorry for being late but i'm a bit busy with some commitments. I'll restart working on this in a day or two.

[mbobrovskyi]

• [fix]: refactor code to replace time.Now() with Now() in clock package in pkg/scheduler(#3380) #3421
• [cleanup]: refactor code to show the use of fakeClock explicitly in pkg/queue(#3380) #3478
• Replace time.Now() with clock.Now() in pkg/workload #3498
• Replace time.Now() with clock.Now() in pkg/core #3773
• Replace time.Now() with clock.Clock.Now() in pkg/pod #3775
• https://github.com/kubernetes-sigs/kueue/blob/main/pkg/workload/admissionchecks.go
• Replace time.Now() with clock.Clock.Now() in pkg/workload/workload.go #3870