[27.x backport] fix: ctx on run image pull #5654
Merged
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
docker rundoesn't cancel gracefully #5639After some digging I found that we don't actually explicitly return an error on the
pullImagefunction here. Once the context is cancelled the response is closed since the API client useshttp.NewRequestWithContextwhich then exits thejsonmessage.DisplayJSONMessagesToStream(responseBody, out, nil)without any errors.The following API request to
ContainerCreatethen immediately fails since the context is closed, which then returns an error.See this section of the code, it fails to find the image, then pulls the image, and continues to retry creating a container - on this second retry it only then returns with the context error.
https://github.com/docker/cli/blob/master/cli/command/container/create.go#L271-L289
Writing a test for this means that we are building into the test an assumption that we won't error on the
pullImagefunction, but rather on the second call toContainerCreate. I'll push what I have, but I honestly think that we will get tripped up by thepullImagefunction again since it's not context aware - actually more so thejsonmessagepackage it is using is actually the root cause found in Moby here. It's a bit of a hornets nest since it gets used in many places and by refactoring/improving this it could cause edge case bugs.- What I did
Delay stripping the
cancelfrom the functionctxso we can still cancel thecreateContainerfunction.cli/cli/command/container/run.go
Line 142 in e4e0ea2
- How I did it
- How to verify it
Unit test
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)