pass context.Context into potentially blocking operations

[landism]

I work on software that is calling Provider.Create to create a kind cluster. Sometimes this hangs for unclear reasons. e.g., logs show it's been running for 20 minutes and the goroutine is stuck on an exec.Command().Run for a docker run --detach.

What would you like to be added:
I'd like to be able to pass a context.Context to Provider.Create (and really any other public kind api that might block) and have that propagate to any potentially blocking operations so that I can set a timeout on the context, cancel it, and get an error, rather than just blocking forever. (Luckily, for local commands, kind already attaches output to the errors, which will help even more when debugging).

Roughly, I want to make use of #1364 when calling kind as a library.

I am happy to make a PR with this change, but am looking to get confirmation that it'd be welcome before putting in the work.

Why is this needed:
To allow code calling kind as a library to cancel operations (which also makes it easier to debug when operations hang indefinitely)

[aojea]

passing context to operations that can block looks like the right thing to do , cc @BenTheElder

for backwards compatibility, you. may add the Context version in addition to the existing one

[landism]

Thanks!

I've mostly got a branch ready and have hit a backwards compatibility wrinkle with nodes.Node: the Node interface contains several methods whose implementations involve execing commands, so it'd be good to be able to run those with a Context (diff here). The Node interface is public, so adding or modifying methods is technically a breaking change if there is third-party code that implements their own Node types.

The options I see:

1. abandon the whole endeavor
2. don't worry about backwards compatibility for third-party implementations of Node Node since they should really only be created by Providers anyway and that's an internal interface, so, as far as I know, there's not really a good reason for anyone to implement the Node interface themselves.
3. add a NodeWithContext interface with new methods that take contexts (e.g., func IPContext(ctx context.Context)) and then have callers convert / error as needed
4. add a NodeWithContext interface as in (3) but then also add a ProviderWithContext type that explicitly uses nodes.NodeWithContext instead of nodes.Node in its methods.

I'd lean towards (3) if you think a backwards incompatibility for third-party implementations of nodes.Node would be an unacceptable risk for this project, or (2) if you think it'd be ok.

I'd love your opinion, or I can just put it up as a PR and it can be discussed there. Thanks!

[BenTheElder]

I'd like to be able to pass a context.Context to Provider.Create (and really any other public kind api that might block) and have that propagate to any potentially blocking operations so that I can set a timeout on the context, cancel it, and get an error, rather than just blocking forever.

Should be tricky but doable.

If we do this, we should look at signal handling in the kind commands.

(Luckily, for local commands, kind already attaches output to the errors, which will help even more when debugging).

Yeah ... though I've been debating using normal go error wrapping instead of the pkg/errors shim. This hasn't actually been very helpful and we get a LOT of bug reports that just attach a snippet of command output with insufficient context to actually diagnose the problem.

I'd lean towards (3) if you think a backwards incompatibility for third-party implementations of nodes.Node would be an unacceptable risk for this project, or (2) if you think it'd be ok.

Implementations of this would probably be fakes for testing.

People also sometimes do things like a partial local interface with the subset of an interface that they are using.

Unfortunately we do not have good insight into how people are using the library code. Most users are using the CLI directly or scripting it.

Not sure there is any great option. At least with most of the high level user APIs we can just pass context as a new Option without any breaking change.

Don't love adding a ton of redundant FooContext methods, it doesn't read very well. We can bikeshed the names later though, looking at the go stdlib, maybe some of these should be "FooWithContext".

We should search for public usage of nodes.Node and see what turns up when considering just breaking it.

[BenTheElder]

If we didn't think anyone would call/use/implement the node interface we wouldn't have made it public, unfortunately.

Testing fakes, and utilities that call the node APIs were the two I had in mind.

3. is interesting, but messy. we could make a lot of methods type assert upgrading a node to a nodewithcontext and then opportunistically pass the context but that's going to be a lot more complicated/messy for probably not enough gain.

client-go made a change to start requiring context that infamously had rippling breakage throughout the ecosystem, I don't think kind will be that bad but kubernetes is already kind of infamous for that particular form of breakage 🥲

[landism]

At least with most of the high level user APIs we can just pass context as a new Option without any breaking change.

Ah, I was avoiding that because of the guideline against putting Contexts in structs, but I hadn't seen the exception to that guideline for backwards compatibility (which also suggests the duplicating with Context suffix approach). I'm not sure that works very well as an Option, though - in the blog post's example, they added a ctx to a Request, so it's still a per-operation thing. An expiring Context as a Provider Option would be a bit weird - you'd need to make a new provider and invoke one operation every time you wanted to set a timeout. That's probably not the ideal final state, but maybe it's the least bad option (though that only saves us from the *Context methods, and doesn't solve the Node issue).

If we didn't think anyone would call/use/implement the node interface we wouldn't have made it public, unfortunately.

Ah, I was hoping it was only public because it's used in the public Provider API.

It sounds like you aren't happy with any of the options on the table for Node.

One other option is to just not worry about propagating contexts to Node's methods and hope that they'll be relatively unproblematic and/or worry about them if/when there are reports of problems with them. The operations performed by those methods do seem much safer than some of the others.

[BenTheElder]

Ah, I was avoiding that because of the guideline against putting Contexts in structs, but I hadn't seen the exception to that guideline for backwards compatibility (which also suggests the duplicating with Context suffix approach). I'm not sure that works very well as an Option, though - in the blog post's example, they added a ctx to a Request, so it's still a per-operation thing. An expiring Context as a Provider Option would be a bit weird - you'd need to make a new provider and invoke one operation every time you wanted to set a timeout. That's probably not the ideal final state, but maybe it's the least bad option (though that only saves us from the *Context methods, and doesn't solve the Node issue).

Well e.g. cluster.Create() takes ...Option so it's reasonable to supply context optionally in a non-breaking way as an option (that's why we used the option pattern ...), those options are scoped to the Create() call and the methods behind that are internals anyhow.

Unfortunately some "simple" commands that shouldn't block for long but could do (like ListNodes ...), and as you mentioned it should be scoped per request.