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

flushing load image response before returning success #1761

Merged
merged 1 commit into from
Dec 26, 2018

Conversation

yhlee-aws
Copy link
Contributor

@yhlee-aws yhlee-aws commented Dec 26, 2018

Summary

go-dockerclient's stream client used to handle the stream response before returning success for the API call. Imitating the same behavior for ImageLoad to make sure we don't run into the race condition between HTTP response reporting success and image load successfully completing.

Implementation details

flushing response.Body from ImageLoad call before returning success

Testing

  • Builds on Linux (make release)
  • Builds on Windows (go build -out amazon-ecs-agent.exe ./agent)
  • Unit tests on Linux (make test) pass
  • Unit tests on Windows (go test -timeout=25s ./agent/...) pass
  • Integration tests on Linux (make run-integ-tests) pass
  • Integration tests on Windows (.\scripts\run-integ-tests.ps1) pass
  • Functional tests on Linux (make run-functional-tests) pass
  • Functional tests on Windows (.\scripts\run-functional-tests.ps1) pass

New tests cover the changes: N/A

Description for the changelog

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@yhlee-aws
Copy link
Contributor Author

If close() mandates waiting for stream EOF, copy isn't necessary. However Closer interface does not dictate any such behavior, and this duplicates the previous behavior with go-dockerclient.

@yhlee-aws yhlee-aws merged commit 684a5de into aws:dev Dec 26, 2018
@yhlee-aws yhlee-aws deleted the loadimage_test branch December 26, 2018 23:34
@yhlee-aws yhlee-aws added this to the 1.24.0 milestone Jan 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants