There are two clock packages with divergent history

[MikeSpreitzer]

What happened:
I discovered a need to update one of the two clock packages, and preparation for that revealed the divergent history of the two.

One line of development can be seen in

• https://github.com/kubernetes/kubernetes/commits/0fdb33b25e3cb207705f225aec7d30b900dd1e89/staging/src/k8s.io/client-go/util/clock/clock.go , and then
• https://github.com/kubernetes/kubernetes/commits/master/staging/src/k8s.io/apimachinery/pkg/util/clock/clock.go

It looks like early on that was cloned into k8s.io/utils, and from there we have an independent line of development:

• https://github.com/kubernetes/utils/commits/master/clock/testing/fake_clock.go

What you expected to happen:
The two clock packages would be kept in sync for a while and then one removed.

How to reproduce it (as minimally and precisely as possible):

Anything else we need to know?:

Environment:

• Kubernetes version (use kubectl version):
• Cloud provider or hardware configuration:
• OS (e.g: cat /etc/os-release):
• Kernel (e.g. uname -a):
• Install tools:
• Network plugin and version (if this is a network-related bug):
• Others:

[MikeSpreitzer]

@kubernetes/sig-api-machinery-bugs

[lavalamp]

Whoops! Thanks for noticing!

Tagging recent other editors: @munnerz @euank @kevindelgado

Which one should we keep?

[MikeSpreitzer]

I think the one in k8s.io/utils is necessary, because it is consumed by component-base, on which apimachinery depends.

I created kubernetes/utils#190 to start fixing the clock package in utils.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[ash2k]

/lifecycle frozen

[ash2k]

/assign

[MadhavJivrajani]

@MikeSpreitzer - once #105372 and #105375 are merged in, the codebase will be fully migrated away from k8s.io/apimachinery/pkg/util/clock.

How can we proceed with removal of this package? Is there some policy around this?
/assign

[wojtek-t]

@lavalamp @liggitt ^^

[I would personally vote for just removing it - maybe just leaving some "doc" file to point to the new location of that. But let's maybe wait for others.]

[liggitt]

There seem to be a lot of out-of-tree uses of it (https://grep.app/search?q=%22k8s.io/apimachinery/pkg/util/clock%22&filter[lang][0]=Go) ... if it's easy to use type aliases to map to matching k8s.io/utils interfaces/implementations and mark the k8s.io/apimachinery/pkg/util/clock versions deprecated for a release or two, that would ease people updating library versions

If that doesn't work or makes the package unmaintainable in some way, removing with a docs pointer seems ok.

[MadhavJivrajani]

Hey @liggitt 👋🏼
Before making any more changes, I wanted to get your opinion on where we go from here:
Following kubernetes/utils#224, since we cannot modify the interface anymore, one obvious approach that I can think of would be to introduce a new interface in k8s.io/utils/clock

// k8s.io/utils/clock
package clock
...
type WithTickerAndDelayedExecution interface {
    WithTicker
    AfterFunc(d time.Duration, f func()) Timer
}

And in k8s.io/apimachinery/pkg/util/clock (the one to deprecate)

// k8s.io/apimachinery/pkg/util/clock
package clock
clockUtils import "k8s.io/utils/clock"
type PassiveClock = clockUtils.PassiveClock
type Clock = clockUtils.WithTickerAndDelayedExecution
type RealClock = clockUtils.RealClock
// so on for fake clocks as well.

Does this sound OK?

[MadhavJivrajani]

Friendly ping @liggitt ^ :)

[liggitt]

I don't have strong opinions about the transition in k8s.io/apimachinery... using a matching library version there is expected

I'll defer to @wojtek-t on the plan for the utils interface... defining a new interface is the only real way I see to stay backward compatible with the existing exposed go API there. To avoid repeating issues of the past, settle on the final/full interface you need to collapse onto before merging to utils

[MadhavJivrajani]

Hey @liggitt 👋🏼 Before making any more changes, I wanted to get your opinion on where we go from here: Following kubernetes/utils#224, since we cannot modify the interface anymore, one obvious approach that I can think of would be to introduce a new interface in k8s.io/utils/clock

// k8s.io/utils/clock
package clock
...
type WithTickerAndDelayedExecution interface {
    WithTicker
    AfterFunc(d time.Duration, f func()) Timer
}

And in k8s.io/apimachinery/pkg/util/clock (the one to deprecate)

// k8s.io/apimachinery/pkg/util/clock
package clock
clockUtils import "k8s.io/utils/clock"
type PassiveClock = clockUtils.PassiveClock
type Clock = clockUtils.WithTickerAndDelayedExecution
type RealClock = clockUtils.RealClock
// so on for fake clocks as well.

Does this sound OK?

@wojtek-t how does this sound to you wrt unblocking the migration?

[wojtek-t]

@wojtek-t how does this sound to you wrt unblocking the migration?

This sounds good to me.
Given that we migrated everything finally, I think this is ok.

[MadhavJivrajani]

Thanks @wojtek-t!
Based on the above, I tested out how the migration would look locally by making k8s.io/util/clock point to my local clone which has WithTickerAndDelayedExecution and it looks as follows: MadhavJivrajani@2757c57
Could you take a look whenever you get a chance and confirm if this is how we want to proceed? I just want to make sure that we are completely sure before making any more changes :)

[wojtek-t]

Commented there

[MikeSpreitzer]

Hooray! Thanks, @MadhavJivrajani !