CODENVY-212: Add DockerContainerCleaner for clean up docker containers #1104
Conversation
| @@ -168,6 +171,41 @@ public Version getVersion() throws IOException { | |||
| } | |||
|
|
|||
| /** | |||
| * Method returns list docker containers which wos filtered by query parameters from {@link ListContainersParams} | |||
|
Add yourself as author |
| verify(dockerConnector, never()).killContainer(containerId2); | ||
| verify(dockerConnector, never()).removeContainer(containerId2, true, true); | ||
| } | ||
| } |
There was a problem hiding this comment.
Please add trailing new line at the end of file
There was a problem hiding this comment.
Trailing new line added
AndrienkoAleksandr
changed the title
|
Pull request updated. @garagatyi @sleshchenko review please. |
|
|
||
| @ScheduleRate(period = 5, initialDelay = 5, unit = TimeUnit.MINUTES)//it will be changed to one per hour. Maybe it should be set in property ? | ||
| @Override | ||
| public void run() { |
There was a problem hiding this comment.
I would recommend add catch (Exception ...) to avoid suppressing of next subsequent executions in case throwing RuntimeException
There was a problem hiding this comment.
Ok, thank you.
|
Pull request updated. Moved job period to the properties file. |
| @@ -70,6 +70,7 @@ docker.registry.auth.password=NULL | |||
| docker.registry.auth.email=NULL | |||
| docker.connection.tcp.connection_timeout_ms=600000 | |||
| docker.connection.tcp.read_timeout_ms=600000 | |||
| docker.clean.up.unused.containers.period_min=5 | |||
There was a problem hiding this comment.
It is good that you've added unit to property name. But naming is not the best. We use dots to separate components and underscores to separate words of one component.
|
Build finished. |
| addQueryParamIfNotNull(connection, "since", params.getSince()); | ||
| addQueryParamIfNotNull(connection, "before", params.getBefore()); | ||
| if (filters != null) { | ||
| connection.query("filters", urlPathSegmentEscaper().escape(JsonHelper.toJson(params.getFilters()))); |
There was a problem hiding this comment.
Either use local variable you've previously set or don't create that variable at all
|
Build finished. |
| return new Object[][] {{"/workspacep2bivvctac5ciwoh_machineri6bxnoj5jq7ll9j_user-name_ws-machine-name", | ||
| new Pair<>("machineri6bxnoj5jq7ll9j", "workspacep2bivvctac5ciwoh")}, | ||
|
|
||
| {"/workspacep2bivvctac5ciwoh_machineri6bxnoj5jq7ll9j_user-name_ws-machine-name", |
There was a problem hiding this comment.
include example with Swarm's node name at the beggining
|
Build finished. |
|
Updated. @garagatyi review please. |
|
Please perform final tests with docker and swarm |
|
LGTM |
| @@ -185,7 +189,48 @@ public Version getVersion() throws IOException { | |||
| if (OK.getStatusCode() != response.getStatus()) { | |||
| throw getDockerException(response); | |||
| } | |||
| return parseResponseStreamAsListAndClose(response.getInputStream()); | |||
| return parseResponseStreamAsListAndClose(response.getInputStream(), new TypeToken<List<Image>>() {}.getType()); | |||
There was a problem hiding this comment.
There is no need to create new instance of TypeToken each time when you list the images
There was a problem hiding this comment.
If we move creation this TypeToken to the field we should do that for all methods, which used method parseResponseStreamAsListAndClose (for now we have two such methods, but it can be more). I think method parseResponseStreamAsListAndClose should be ease of use, so we should not create new fields for TypeToken in the docker connector.
|
Build finished. |
2 similar comments
|
Build finished. |
|
Build finished. |
|
ok |
AndrienkoAleksandr
changed the title
|
Build finished. |
AndrienkoAleksandr
force-pushed
the
CODENVY-212-2
branch
from
d22c7f7 to
25f74c6
Compare
Signed-off-by: Aleksandr Andrienko <aandrienko@codenvy.com>
AndrienkoAleksandr
force-pushed
the
CODENVY-212-2
branch
from
25f74c6 to
0aa802d
Compare
|
Build finished. |
|
Build finished. |
Signed-off-by: Aleksandr Andrienko aandrienko@codenvy.com