Use of context.TODO()

[onitake]

I noticed that you added context arguments to the k8s API calls in 16109017cec02a8a0c4f80f829acc38ab28a14b2 - but you used context.TODO() instead of propagating a context object.

context.TODO() signals that this will be changed at some point - do have any plans to do so? Otherwise, it would be more appropriate to use context.Background() instead.

[evrardjp]

I think the patch you are referring to was simply the quickest workaround, and we didn't talk about what will it contain yet.

I don't have any strong opinion on that. If you want to switch it, please be sure to include the why in the eventual PR. :)

[github-actions[bot]]

This issue was automatically considered stale due to lack of activity. Please update it and/or join our slack channels to promote it, before it automatically closes (in 7 days).

[evrardjp]

I would like to keep that one open.

[github-actions[bot]]

This issue was automatically considered stale due to lack of activity. Please update it and/or join our slack channels to promote it, before it automatically closes (in 7 days).

[dholbach]

Is there more that needs to be done here?

[onitake]

The current main still has context.TODO() in many places.

There's two ways to solve this: Propagate a context object through all functions that call the k8s API, or use a local context.Background(). I can't tell which one is more appropriate in each case.

In one place, a ctx, cancel := context.WithTimeout(context.Background(), timeout) was used: https://github.com/kubereboot/kured/blob/main/pkg/daemonsetlock/daemonsetlock.go#L152 Would it make sense to have timeouts in other places as well?

[jackfrancis]

I think that the proper path forward here is @onitake's suggestion to audit the code and, where appropriate, establish a durable context object (context.Background() would be fine for this) and then to pass it down the execution flow, updating function signatures as necessary to accept context.Context, and then passing that downstream instead of using a local context.TODO().

The function comments in the go std library for TODO() suggest that this is the appropriate thing to do when you haven't yet done the above.

// TODO returns a non-nil, empty Context. Code should use context.TODO when
// it's unclear which Context to use or it is not yet available (because the
// surrounding function has not yet been extended to accept a Context
// parameter).

Ref:

• https://cs.opensource.google/go/go/+/refs/tags/go1.21.0:src/context/context.go;l=215-218

I don't think we benefit from updating the existing local context.TODO() usages one-by-one with context.Background(). That doesn't seem to offer any benefit, at a (marginal) cost compared to an empty context.Context.

Curious about other thoughts as well here.

[ckotzbauer]

That's a good point, we should pass contexts down the line for better handling. 👍

[leonnicolas]

Hey, if you want to pass down a context, would you also want some kind of mechanism to manage the go routines started in the main func, like https://pkg.go.dev/github.com/oklog/run? There is no point of passing down a context, if you don't cancel it when the application receives a signal, or the metrics server crashes.

[evrardjp]

Kured is so lightweight and simple... I think we need to explain why we really want to do that. I agree with @jackfrancis overall.

Context.background is a step towards a direction, which I can agree with, but I am not entirely sold either...

I commented on @leonnicolas , which is a very good PR to get the ball rolling to "do the right thing". I would prefer if everyone sees and understand the benefits, as it adds a bit of complexity, for a not very big "business" value for kured, IMO. Except if I missed something big?

Explaining said value would be useful to help me review this.