dockerapi: Migrated VolumeRemove and APIVersion to Docker SDK #1468
Conversation
voorka
changed the title
| if err != nil { | ||
| return &CannotGetDockerClientError{version: dg.version, err: err} | ||
| } | ||
|
|
||
| ok := client.RemoveVolume(name) | ||
| ok := client.VolumeRemove(ctx, name, false) |
There was a problem hiding this comment.
What does the third argument indicate?
There was a problem hiding this comment.
It's means force remove: https://godoc.org/github.com/docker/docker/client#Client.VolumeRemove
The previous docker client that we use doesn't specify force remove: https://github.com/fsouza/go-dockerclient/blob/05828842f94da0aef6ad140aa3fe777d23d17139/volume.go#L123
The default is false from docker api doc:
https://docs.docker.com/engine/api/v1.36/#operation/VolumeDelete
So if the purpose is to achieve the same functionality, make it false is safe here.
There was a problem hiding this comment.
If removal fails for a task scoped volume, force removing it seems like the way to go. But this PR is not the right place for making this decision. We'll leave this as it is for now.
There was a problem hiding this comment.
I'll add this as a TODO to my development plan for this project.
| @@ -1266,11 +1266,11 @@ func (dg *dockerGoClient) listPlugins(ctx context.Context) ListPluginsResponse { | |||
|
|
|||
| // APIVersion returns the client api version | |||
| func (dg *dockerGoClient) APIVersion() (dockerclient.DockerVersion, error) { | |||
| client, err := dg.dockerClient() | |||
| client, err := dg.sdkDockerClient() | |||
There was a problem hiding this comment.
Since we have two different clients, how do we know if APIVersion() is returning the sdk client's version?
There was a problem hiding this comment.
sdkDockerClient() returns the client generated by the docker/docker library. The function, FindClientAPIVersion() was changed from a for loop over all clients to an API call and therefore, FindClientAPIVersion will be called on the sdkClient. We can test this because if the go-dockerclient tried to make the API call ClientVersion() in FindClientAPIVersion, an error would be thrown because go-dockerclient does not implement this function.
Summary
Migration for VolumeRemove and APIVersion from go-dockerclient to the DockerSDK
Implementation details
Changes were implemented by swapping API calls to new SDK Client. All changes could be done under the DockerClient interface making this migration fairly straightforward.
The VolumeRemove API call takes in a boolean for the force flag to remove a volume. Go-dockerclient did not support this flag, so 'false' is used as a placeholder.
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
Enhancement - VolumeRemove and APIVersion migrated to Docker SDKLicensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.