review adding timeouts to Docker API calls

[tgross]

From #9502:

This changeset is a minimal intervention to add the timeout to the Docker API
call only when we have the kill_timeout set. Future work should examine
whether we should be threading contexts through other go-dockerclient API
calls.

[mikenomitch]

Hi to anybody reading this - We think this is a good first issue and good candidate for a community PR. Probably not something the HashiCorp team will get to right away though.

This would involve finding all the places where we call the docker API and adding a timeout there.

For example, for a call like this one could add a go context with a timeout.

[danishprakash]

@tgross sorry for spamming a bunch of issues around here but I think I'll take a shot at this one based on @mikenomitch's comment. Hope that's okay.

[tgross]

Go for it!

[danishprakash]

Hey, I had a question about this. So, we're exclusively talking about attaching context as part of calls to the docker engine made from the docker client, is that correct?

Also, this could change if we're exporting a specific call e.g. driver.Signal() from @tgross's earlier PR in which case, we allow the callers to provide the context (which could be derived from higher up the call stack).

I made a small draft change for the createContainer method here keeping in mind the above two assumptions. Can someone corroborate these? Thanks

[tgross]

Hi @danishprakash!

So, we're exclusively talking about attaching context as part of calls to the docker engine made from the docker client, is that correct?

Also, this could change if we're exporting a specific call e.g. driver.Signal() from @tgross's earlier PR in which case, we allow the callers to provide the context (which could be derived from higher up the call stack).

Yes! We don't want to bubble that all the way up to the callers in client/allocrunner/taskrunner because it'll break the contract with third-party drivers. But inside the drivers/docker package should be fair game.

I made a small draft change for the createContainer method here keeping in mind the above two assumptions. Can someone corroborate these? Thanks

The CREATE: label there effectively is the top of a loop, so I think we want the context/timeout to be created before that label. But other than that, yes this looks like the generally right approach.