Clean up docker containers by default #3629

w-gao · 2021-05-24T01:45:12Z

Make sure Toil removes the docker containers after they are finished running, so we don't have a bunch of "Exited" Docker containers.

Closes #3505.

Changelog Entry

To be copied to the draft changelog by merger:

Clean up docker containers by default. Set remove=False in apiDockerCall() to leave the containers untouched and running.

Reviewer Checklist

Make sure it is coming from issues/XXXX-fix-the-thing in the Toil repo, or from an external repo.
- If it is coming from an external repo, make sure to pull it in for CI with:
```
contrib/admin/test-pr otheruser theirbranchname issues/XXXX-fix-the-thing
```
- If there is no associated issue, create one.
Read through the code changes. Make sure that it doesn't have:
- Addition of trailing whitespace.
- New variable or member names in camelCase that want to be in snake_case.
- New functions without type hints.
- New functions or classes without informative docstrings.
- Changes to semantics not reflected in the relevant docstrings.
- New or changed command line options for Toil workflows that are not reflected in docs/running/cliOptions.rst
- New features without tests.
Comment on the lines of code where problems exist with a review comment. You can shift-click the line numbers in the diff to select multiple lines.
Finish the review with an overall description of your opinion.

Merger Checklist

Make sure the PR passes tests.
Make sure the PR has been reviewed since its last modification. If not, review it.
Merge with the Github "Squash and merge" feature.
- If there are multiple authors' commits, add Co-authored-by to give credit to all contributing authors.
Copy its recommended changelog entry to the Draft Changelog.
Append the issue number in parentheses to the changelog entry.

…move=True` (even if "docker run" fails)

src/toil/lib/docker.py

adamnovak

I have some small complaints, but mostly I'm now suspicious of the whole deferParam system. I think we need to at least re-document it so it makes sense along with the remove flag. As is it's not clear what all the combinations are supposed to do.

A much simpler fix for #3505 might be possible: I don't think the documented default deferParam behavior of RM has been working, which would account for all the left-behind containers from toil-vg.

Do we have any unit tests for the cleanup here? If not, the first step should really be to add some. Then we can be certain of what the combinations of parameters are meant to do, because they will be tested.

adamnovak · 2021-05-25T18:29:07Z

src/toil/lib/docker.py

-                    with open(streamfile, 'w') as f:
-                        f.write(line)
+                    for line in container.logs(stdout=stdout, stderr=stderr, stream=True):
+                        f.write(line.decode() if isinstance(line, bytes) else line)


This is going to raise an error in Toil if a container happens to be sending non-UTF8 data; we often stream binary files in and out of Docker containers in toil-vg. We ought to just pass the bytes along here in binary mode.

src/toil/lib/docker.py

adamnovak · 2021-05-25T18:31:23Z

src/toil/lib/docker.py

@@ -356,27 +362,29 @@ def dockerKill(container_name, gentleKill=False, timeout=365 * 24 * 60 * 60):
        raise create_api_error_from_http_exception(e)


-def dockerStop(container_name):
+def dockerStop(container_name: str, remove: bool = True) -> None:
    """
    Gracefully kills a container.  Equivalent to "docker stop":


This might not be the right way to explain this anymore, since now by default we do more than "docker stop".

Maybe drop the link to the Docker docs and say that this gracefully kills and removes a container?

adamnovak · 2021-05-25T18:31:55Z

src/toil/lib/docker.py

    :param container_name: Name of the container being stopped.
-    :param client: The docker API client object to call.
+    :param remove: If True, remove the container after it exits. (default: True).


Maybe this should be a keep flag everywhere instead of a default-true remove flag? Having flags that are default-true and having to say they =False takes up more brain space than the other way around. Plus, when we document them like this, by saying what happens when they're true, then when you actually go to use them (by setting them to false), you have to invert the sense of the documentation to figure out what you are accomplishing by using the flag, which takes more brain space.

adamnovak · 2021-05-25T18:33:03Z

src/toil/lib/docker.py

+def dockerKill(container_name: str,
+               gentleKill: bool = False,
+               remove: bool = True,
+               timeout: int = 365 * 24 * 60 * 60) -> None:
    """
    Immediately kills a container.  Equivalent to "docker kill":


This might not be the best way to describe this method if it now has a different default behavior than docker kill.

src/toil/lib/docker.py

adamnovak · 2021-05-25T19:19:59Z

@w-gao I've added a bunch of complaints here, but I don't think it would make sense to go through and address all of them. I think we want to do one of:

Not actually change the old remove flag at all and instead test the RM deferParam and make sure it works as documented to clean up containers after the job when not explicitly set.
Refactor remove to actually work through the deferred cleanup system like you have done, give up on apiDockerCall looking quite like the Python Docker API, and then start revising its API to make more sense (maybe deprecating deferParam in favor of remove to control cleanup).

w-gao · 2021-05-25T22:08:43Z

Thanks @adamnovak for the review!

I'm leaning towards fixing the default deferParams behavior for clean up (your first suggestion), because I do want apiDockerCall() to look like the Python Docker API, and I think it would make sense to not change the old remove flag so users can control the underlying client.containers.run() call if needed.

As for unit tests, we actually have these here that test the different combinations of options. Although, I don't think they are tested on the CI. I'll look into the tests and see if the default deferParam behavior of RM is being tested.

adamnovak · 2021-05-25T22:38:25Z

It does look like the dockerTest.py file might not be getting run on CI; maybe we can add it to the Gitlab tests next to other lib tests?

adamnovak

This is definitely simpler now, and I think it should work. @w-gao Are you still adding tests?

w-gao · 2021-06-03T23:52:09Z

Yeah @adamnovak, I'm trying to make sure all combinations work as intended, so this is not quite ready yet.

w-gao · 2021-06-09T00:09:19Z

@adamnovak This should be ready now! I changed a few things since your last review, so this may need a re-review.

For some reason the file permission check doesn't pass on the CI, but works fine locally. I'll take a deeper look into that but that's probably a different issue.

adamnovak

I'm happy with this.

adamnovak · 2021-06-14T16:33:00Z

I think the file permission test fails because we actually are running as root on CI, whereas the test expects us to be running as someone other than root, who it expects to see owning the relevant files.

w-gao added 3 commits May 23, 2021 14:14

Kill and remove docker containers by default

2d38544

Keep the previous behavior where containers are always removed if `re…

9f5f068

…move=True` (even if "docker run" fails)

Docstring changes.

083cf02

w-gao commented May 24, 2021

View reviewed changes

src/toil/lib/docker.py Outdated Show resolved Hide resolved

adamnovak requested changes May 25, 2021

View reviewed changes

w-gao added 2 commits May 29, 2021 09:48

Merge branch 'master' into issues/3505-auto-remove-docker-containers

5591cc2

Review comments

fc63654

adamnovak approved these changes Jun 3, 2021

View reviewed changes

w-gao marked this pull request as draft June 7, 2021 17:46

Update tests.

e14e9fd

w-gao force-pushed the issues/3505-auto-remove-docker-containers branch from 882a489 to e14e9fd Compare June 9, 2021 00:01

w-gao marked this pull request as ready for review June 9, 2021 00:09

adamnovak self-requested a review June 14, 2021 16:30

adamnovak approved these changes Jun 14, 2021

View reviewed changes

adamnovak merged commit 941228d into master Jun 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up docker containers by default #3629

Clean up docker containers by default #3629

w-gao commented May 24, 2021 •

edited by adamnovak

Loading

adamnovak left a comment

adamnovak May 25, 2021

adamnovak May 25, 2021

adamnovak May 25, 2021

adamnovak May 25, 2021

adamnovak commented May 25, 2021

w-gao commented May 25, 2021

adamnovak commented May 25, 2021

adamnovak left a comment

w-gao commented Jun 3, 2021

w-gao commented Jun 9, 2021

adamnovak left a comment

adamnovak commented Jun 14, 2021

Clean up docker containers by default #3629

Clean up docker containers by default #3629

Conversation

w-gao commented May 24, 2021 • edited by adamnovak Loading

Changelog Entry

Reviewer Checklist

Merger Checklist

adamnovak left a comment

Choose a reason for hiding this comment

adamnovak May 25, 2021

Choose a reason for hiding this comment

adamnovak May 25, 2021

Choose a reason for hiding this comment

adamnovak May 25, 2021

Choose a reason for hiding this comment

adamnovak May 25, 2021

Choose a reason for hiding this comment

adamnovak commented May 25, 2021

w-gao commented May 25, 2021

adamnovak commented May 25, 2021

adamnovak left a comment

Choose a reason for hiding this comment

w-gao commented Jun 3, 2021

w-gao commented Jun 9, 2021

adamnovak left a comment

Choose a reason for hiding this comment

adamnovak commented Jun 14, 2021

w-gao commented May 24, 2021 •

edited by adamnovak

Loading