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: add context timeouts to docker client calls #13427

Closed

Conversation

danishprakash
Copy link
Contributor

Closes #9503

Signed-off-by: danishprakash [email protected]

@shoenig
Copy link
Member

shoenig commented Sep 22, 2022

@danishprakash did you intend for this PR to still be marked as a Draft? If not, do you mind rebasing with main and marking it Ready for Reveiw?

@danishprakash danishprakash marked this pull request as ready for review October 19, 2022 04:07
@danishprakash
Copy link
Contributor Author

@shoenig yeah, I marked it as a draft since I wanted some inputs from the team and I hadn't gone through the whole codebase with this change. But I think you can go ahead and review it if you can and I can then address the comments.

@shoenig
Copy link
Member

shoenig commented Dec 22, 2022

Sorry for the super slow responses @danishprakash 😬

One key change I think we should make is making the timeout value configurable in the docker plugin configuration.

Similar to how we already have an option for pull_activity_timeout, let's add something like endpoint_timeout which is general for all other docker endpoint interactions.

So we'll want add that, then apply it to all the places we can, and update the docs.

@tgross tgross added the stage/needs-rebase This PR needs to be rebased on main before it can be backported to pick up new BPA workflows label May 17, 2024
@danishprakash
Copy link
Contributor Author

@shoenig sorry for the multi-year delay. Do you still think this is relevant? I can get back on this if it is.

@tgross
Copy link
Member

tgross commented Sep 12, 2024

Hi @danishprakash! We'd love to have this PR if you have time to revive it. It's the sort of thing that's been hard to make bubble up to the top of our priority list.

@tgross
Copy link
Member

tgross commented Sep 25, 2024

@danishprakash we've decided to go ahead with migrating to the official SDK client over here: #23966. The work of making sure all the contexts used in the official SDK have reasonable timeout behaviors will still need to get done, but you'd basically be starting from scratch. So I'm going to close this PR out. If you're still interested in helping out with this once 1.9.0 goes out, we'd be happy to review a new PR.

@tgross tgross closed this Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stage/needs-rebase This PR needs to be rebased on main before it can be backported to pick up new BPA workflows
Projects
Development

Successfully merging this pull request may close these issues.

review adding timeouts to Docker API calls
3 participants