Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers/docker: refactor use of clients in docker driver #17731

Merged
merged 2 commits into from
Jun 26, 2023

Conversation

shoenig
Copy link
Member

@shoenig shoenig commented Jun 26, 2023

This PR refactors how we manage the two underlying clients used by the
docker driver for communicating with the docker daemon. We keep two clients

One with a hard-coded timeout that applies to all operations no matter
what, intended for use with short lived / async calls to docker.

The other has no timeout and is the responsibility of the caller to set a
context that will ensure the call eventually terminates (except in the .Wait case).

The use of these two clients has been confusing and mistakes were made
in a number of places where calls were making use of the wrong client.

This PR makes it so that a user must explicitly call a function to get
the client that makes sense for that use case.

Fixes #17023

This PR refactors how we manage the two underlying clients used by the
docker driver for communicating with the docker daemon. We keep two clients
- one with a hard-coded timeout that applies to all operations no matter
what, intended for use with short lived / async calls to docker. The other
has no timeout and is the responsibility of the caller to set a context
that will ensure the call eventually terminates.

The use of these two clients has been confusing and mistakes were made
in a number of places where calls were making use of the wrong client.

This PR makes it so that a user must explicitly call a function to get
the client that makes sense for that use case.

Fixes #17023
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

drivers/docker/driver.go Outdated Show resolved Hide resolved
drivers/docker/handle.go Outdated Show resolved Hide resolved
drivers/docker/handle.go Show resolved Hide resolved
drivers/docker/handle.go Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.3.x backport to 1.3.x release line backport/1.4.x backport to 1.4.x release line backport/1.5.x backport to 1.5.x release line
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docker task driver overrides max_kill_timeout
2 participants