Refactor away from wait.Poll calls

[yiannistri]

We make heavy use of https://pkg.go.dev/k8s.io/apimachinery/pkg/util/wait#Poll which is now deprecated. We should refactor away from these calls.

[yitsushi]

Notes

Deprecated: This method does not return errors from context, use PollWithContextTimeout. Note that the new method will no longer return ErrWaitTimeout and instead return errors defined by the context package. Will be removed in a future release.

It says to use PollWithContextTimeout, but as I see there is no function like that, but it has PollUntilContextTimeout which has this description:

The deadline context will be cancelled if the Poll succeeds before the timeout, simplifying inline usage. All other behavior is identical to PollWithContextTimeout.

So I guess the right way to replace wait.Poll calls is to use wait.PollUntilContextTimeout which is very similar to wait.Poll:

// snippet from pkg/run/session/connect/utils.go
wait.PollImmediate(
  time.Second,
  time.Minute*10,
  func() (done bool, err error) { ... },
)

// can be replaced with
timeoutCtx, timeoutCancel := context.WithTimeout(context.Background(), time.Minute*10)
wait.PollUntilContextTimeout(
  timeoutCtx,
  time.Second,
  func(ctx context.Context) (done bool, err error) { ... },
)

Basically the difference is a context.WithTimeout instead of a time.Duration and the condition function has a context.Context argument.

[opudrovs]

@yitsushi @yiannistri So, after running

go get -u k8s.io/apimachinery                                             
go mod tidy

to upgrade k8s.io/apimachinery to the version which has method wait.PollUntilContextTimeout (added in v0.27.0 of apimachinery, and we are currently on version v0.26.3 in OSS), I am getting the following errors for make all:

# github.com/weaveworks/weave-gitops/core/clustersmngr/cluster
core/clustersmngr/cluster/delegating_cache.go:44:48: not enough arguments in call to apiutil.NewDiscoveryRESTMapper
	have (*rest.Config)
	want (*rest.Config, *http.Client)
core/clustersmngr/cluster/delegating_cache.go:59:34: undefined: client.NewDelegatingClient
core/clustersmngr/cluster/delegating_cache.go:59:61: undefined: client.NewDelegatingClientInput
core/clustersmngr/cluster/single.go:58:48: not enough arguments in call to apiutil.NewDiscoveryRESTMapper
	have (*rest.Config)
	want (*rest.Config, *http.Client)
# github.com/fluxcd/pkg/runtime/client
/Users/olga/go/pkg/mod/github.com/fluxcd/pkg/[email protected]/client/impersonator.go:135:50: not enough arguments in call to apiutil.NewDynamicRESTMapper
	have (*rest.Config)
	want (*rest.Config, *http.Client)
/Users/olga/go/pkg/mod/github.com/fluxcd/pkg/[email protected]/client/impersonator.go:167:50: not enough arguments in call to apiutil.NewDynamicRESTMapper
	have (*rest.Config)
	want (*rest.Config, *http.Client)
# github.com/weaveworks/tf-controller/tfctl
/Users/olga/go/pkg/mod/github.com/weaveworks/tf-controller/[email protected]/break_glass.go:80:13: undefined: infrav1.BreakTheGlassAnnotation
/Users/olga/go/pkg/mod/github.com/weaveworks/tf-controller/[email protected]/break_glass.go:83:16: undefined: infrav1.BreakTheGlassAnnotation
/Users/olga/go/pkg/mod/github.com/weaveworks/tf-controller/[email protected]/break_glass.go:100:24: undefined: infrav1.BreakTheGlassAnnotation

It looks like almost the same issue as upgrading TF controller in OSS in general, right?

Possibly, the following, unless we have a separate issue for upgrading TF controller (I remember, we talked about adding an issue for this):
#3746

Upgrading apimachinery to v0.27.0 does not help too, because after running go mod tidy to fix some errors, apimachinery is auto-upgraded to v0.27.2.

[opudrovs]

I can create a http.Client which apiutil.NewDynamicRESTMapper now requires with

rest.HTTPClientFor(c.restConfig)

if it works, but what to do with TF controller?

[opudrovs]

And after upgrade there is also error on client.NewDelegatingClient(client.NewDelegatingClientInput{ method undefined.

[yitsushi]

This builds on top of #3811

[opudrovs]

@yitsushi @yiannistri

There are several cases like the following wait.Poll call within the ReconcileDashboard function.
https://github.com/weaveworks/weave-gitops/blob/main/pkg/run/install/install_dashboard.go#L235

When we replace the wait.Poll call with wait.PollUntilContextTimeout, we need to pass a context to wait.PollUntilContextTimeout. There is a ctx context.Context argument, which was already created with context.WithTimeout, passed to the ReconcileDashboard function .

So, can we pass the context passed to the ReconcileDashboard function to the wait.PollUntilContextTimeout function (called by ReconcileDashboard)?

Or we cannot rely on that context being created by context.WithTimeout and should create another context within ReconcileDashboard specifically to pass to wait.PollUntilContextTimeout (as in the example above)?

timeoutCtx, timeoutCancel := context.WithTimeout(ctx, timeout)
defer timeoutCancel()

if err := wait.PollUntilContextTimeout(timeoutCtx, interval, timeout, false, func(ctx context.Context) (bool, error) {

[yitsushi]

We can use an existing context if we have one either the context itself or as the "parent" of context.WithTimeout(ctx, timeout)

[yiannistri]

This is blocked until we update controller-runtime to 0.15.0.

[yitsushi]

in the go.mod file:

	sigs.k8s.io/controller-runtime v0.15.0

BUT this is also in the go.mod file:

// Replace k8s.io packages v0.26 to downgrade controller-runtime to v0.14.6
replace (
....
	sigs.k8s.io/controller-runtime => sigs.k8s.io/controller-runtime v0.14.6
)

[yitsushi]

@yitsushi : so is it still blocked?
@yiannistri : I don't think so, I would attempt to bump the controller-runtime version and see what breaks, if anything