task engine: centralize getting docker ID, check it's not blank #2608
Conversation
| func (engine *DockerTaskEngine) inspectContainerByName(taskArn, containerName string) (*types.ContainerJSON, error) { | ||
| containers, ok := engine.state.ContainerMapByArn(taskArn) | ||
| if !ok { | ||
| return nil, errors.New("engine: failed to find the pause container, no containers in the task") |
There was a problem hiding this comment.
also fixes this inaccurate error message, seems like maybe this function was originally written to only inspect the pause container.
sparrc
force-pushed
the
more-resilient-docker-id
branch
2 times, most recently
from
e87342c to
5d562ce
Compare
agent/engine/docker_task_engine.go
Outdated
| pauseContainer, ok := containers[containerName] | ||
| if !ok { | ||
| return nil, errors.New("engine: failed to find the pause container") | ||
| func (engine *DockerTaskEngine) inspectContainer(taskArn, container *apicontainer.Container) (*types.ContainerJSON, error) { |
There was a problem hiding this comment.
taskArn should be a string?
agent/engine/docker_task_engine.go
Outdated
| } | ||
| containerMap, ok := engine.state.ContainerMapByArn(task.Arn) | ||
| if !ok { | ||
| return "", errors.Errorf("Container name=%s belongs to unrecognized task taskArn=%s", container.Name, task.Arn) |
There was a problem hiding this comment.
error message should start with lower case, same below
| if !ok { | ||
| return nil, errors.New("engine: failed to find the pause container") | ||
| func (engine *DockerTaskEngine) inspectContainer(taskArn, container *apicontainer.Container) (*types.ContainerJSON, error) { | ||
| dockerID, err := engine.getDockerID(task, container) |
There was a problem hiding this comment.
Not sure about this one. I think this is misleading. getDockerID sometimes returns name?
Also, I am not certain that the logic isn't changed here. This code used to retrieve the container using the name of the container, but now by invoking getDockerID it will try to first attempt to get the docker runtime id. So, if pauseContainer also has a runtime id (now or in the future) this will break right?
There was a problem hiding this comment.
Yes it's true, getDockerID could sometimes return the name, I'm not sure how to call the function to indicate that.
You are correct that this would change to mostly calling the docker API using the docker runtime ID. The docker API f is very liberal in what it accepts as a "docker id". You can pass in the name, full runtime id, short name, or short runtime ID. That is part of why I wanted to make this change, so that it is more standardized on using a single type of "docker id".
There was a problem hiding this comment.
for example, previous to this PR we were called RemoveContainer using DockerName, but if you look at the implementation it is referred to as DockerID in our code, and then ultimately containerID in the docker client codebase:
There was a problem hiding this comment.
I see. I'd prefer to either only use DockerConainer everywhere or to add these attributes directly to the container struct. But that's a much larger change. I guess this is a good iteration
sparrc
force-pushed
the
more-resilient-docker-id
branch
from
5d562ce to
791089a
Compare
sparrc
force-pushed
the
more-resilient-docker-id
branch
2 times, most recently
from
3ddd3ab to
e865e9c
Compare
sparrc
force-pushed
the
more-resilient-docker-id
branch
from
e865e9c to
96614ac
Compare
sparrc
changed the title
Summary
In 5 different places we were mapping from our internal apicontainer object to the dockercontainer object in order to obtain the docker ID, though never in exactly the same way. This centralizes it into a single function (getDockerID) that also tries to first just grab the dockerID from the apicontainer object if it exists.
Will also fallback to the DockerName attribute on the dockercontainer obj if DockerID doesnt exist.
New tests cover the changes: no
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.