docker container fetch limit is now configurable #513
Conversation
Signed-off-by: Gregory Gulrajani <gregory.gulrajani@nucleusfinancial.com>
|
LGTM. I'm going to merge it and resolve the conflicts which happens because I moved the documentation to asciidoc. |
| @@ -178,7 +182,9 @@ public void execute() throws MojoExecutionException, MojoFailureException { | |||
| DockerAccess access = null; | |||
| try { | |||
| access = createDockerAccess(minimalApiVersion); | |||
| ServiceHub serviceHub = serviceHubFactory.createServiceHub(project, session, access, log, logSpecFactory); | |||
| fetchLimit = resolvedImages.get(0).getRunConfiguration().getFetchLimit() != null | |||
There was a problem hiding this comment.
It doesnt makes sense to pick up only the first image for extracting the fetchLimit. Currently it only makes sense as a globale parameter (as you already put it in). Since I guess it makes also only sense to put it into the global scope (i.e. valid for all images), I'll move it from ServiceHub to DockerAccess and remove the two variants of listImages() (since the list will automatically truncate).
There was a problem hiding this comment.
Well spotted. I meant to ask you about fetching the resolvedImage. I was not happy with the get(0) it's a bit dangerous ..
There was a problem hiding this comment.
Yeah, that was a misunderstanding on my side. A global parameter is good enough.
However, our problem indeed is how we can make the lookup of a container by its name and by its image more effective. Because then we could omit the call to list() altogether. And it would be much more effective, too, especially for a large set of running containers. At the time this was implemented, there was no way with the API todo this direct lookup, but it could be that there is now an alternative API call.
I will go hunting a bit ... ;-)
There was a problem hiding this comment.
Just saw that there are new filters for the list call:
exited=<int>; -- containers with exit code of <int> ;
status=(created|restarting|running|paused|exited|dead)
label=key or label="key=value" of a container label
isolation=(default|process|hyperv) (Windows daemon only)
ancestor=(<image-name>[:<tag>], <image id> or <image@digest>)
before=(<container id> or <container name>)
since=(<container id> or <container name>)
volume=(<volume name> or <mount point destination>)
network=(<network id> or <network name>)
So maybe since and ancestors could be a match for our use case ....
There was a problem hiding this comment.
E.g.
- for fetching by name: since=<container name>&limit=1
- for fetching by images: ancestor=<image name>&limit=1
Hopefully the filter is inclusive. 'will try this out now ...
There was a problem hiding this comment.
gah -- we are on 1.11 -- I will upgrade Nucleus's docker tomorrow to 1.24..
There was a problem hiding this comment.
I'm also only on 1.21, however there is since query param (not as part of the filter), but it seems to eclusive, not inclusive (so the entry with the name itself is not included).
For the name lookup I have to check why I haven't use the /containers/<name>/json call. I remember darkly, there was a reason ;-)
'have to run now, see you tomorrow ...
There was a problem hiding this comment.
Cool, filtering with 'ancestor=' works. Unfortunately only available since 1.23
But with this two features we should get rid of the local iteration of image names (we leave it in nevertheless so that the plugin will work with older clients, too).
Apparently this works for every supported version down to 1.18 (Docker 1.6), since `/containers/<id or name>/json` can use both id and name. We used it only for ids so fart, but this is changed with this commit. Also there is now DockerAccess.getServerApiCVersion() to detect the API version running on the server.
For an API >= 1.23 /containers/ listing with an `ancestor` filter can be used to the the image for a container (which is required mostly only for stopping containers in an external call). For API < 1.23 there is still a call to /containers/ with a limit.
Now when fetching images they are fetched all. This should happen now only when running `docker:stop` or `docker:watch` to find containers for a particular image and when running Docker < 1.11.
Signed-off-by: Gregory Gulrajani gregory.gulrajani@nucleusfinancial.com