Pullimage sdk migration #1623
Pullimage sdk migration #1623
Conversation
|
|
||
| pullFinished := make(chan error, 1) | ||
| // TODO Migrate Pull Image once Inactivity Timeout is sorted out | ||
| subCtx, cancelRequest := context.WithCancel(ctx) | ||
|
|
||
| go func() { |
There was a problem hiding this comment.
Is this go routine along with the select statement necessary since we now implement the full go routine with the sdk change? It seems the select statement was only to catch a premature error (if nothing was being pulled), but now we can handle the error within the go routine without the select statement.
There was a problem hiding this comment.
given that images can be large and take long time to download, I think go routine here is necessary so that it is handled in a separate thread. I'm sure that this can be cleaned up, but I'd like to limit changes in behavior between go-dockerclient and SDK, and prefer to come back to this to make any improvements.
There was a problem hiding this comment.
We're still using the goroutine for the pullBegan timeout. You could rework that, but I think its fine to only focus on translating the API for now.
Summary
Migrating pull image to SDK
Implementation details
go-dockerclient used to handle a lot more including inactivity timeout and response "event" object from the image pull. These are handled in agent codebase now, reusing inactivity timeout from stats, and creating custom imagePullResponse object. There's an open issue for handling pull image response event on docker side (docker/engine-api#89) so I didn't want to add premature dependency on it.
Testing
make release)go build -out amazon-ecs-agent.exe ./agent)make test) passgo test -timeout=25s ./agent/...) passmake run-integ-tests) pass.\scripts\run-integ-tests.ps1) passmake run-functional-tests) pass.\scripts\run-functional-tests.ps1) passNew tests cover the changes: yes
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.