-
Notifications
You must be signed in to change notification settings - Fork 407
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[JENKINS-70132] Remove anonymous volumes when removing the container #286
Conversation
For more context, adding
|
@jglick @rsandell sorry to ping you in advance, I found your names from #280 which is somewhat related to running docker inside of a docker agent. Would you mind taking a look at this PR? I have been using a modified version of this plugin with this change for 2 weeks already, which is working without any issue. I'm asking because I see several other PRs open for years, but I believe this fixes a critical issue. |
Sounds right. Ideally would have test coverage. I am not maintaining this plugin and my standing advice is to uninstall it. #280 was special in that it was fixing something that worked “before”. |
(You would need to check whether this CLI flag was a recent addition.) |
I'll come up with this information. One moment. |
I tested all the way down to $ docker run --rm docker:1.2.0 docker rm --help
Usage: docker rm [OPTIONS] CONTAINER [CONTAINER...]
Remove one or more containers
-f, --force=false Force the removal of a running container (uses SIGKILL)
-l, --link=false Remove the specified link and not the underlying container
-v, --volumes=false Remove the volumes associated with the container |
Something to note, though, is that from 19.03.5 to 19.03.6 the description of the flag was changed from
To
I'm checking here if there was some change in the implementation about it, but judging that this description was changed between a patch level release, no changes in the functionality were introduced. |
Found it, it's just a description update: Therefore I think we are in business with this change, i.e. no concerns related to the docker cli version. |
I'm a bit curious about your suggestion. Is there any replacement for this plugin? I mean, for changing agent in declarative pipelines. |
#105 etc. |
I see. Thank you. I'm working to add a test for this. |
466f8f3
to
11cb2fb
Compare
src/test/java/org/jenkinsci/plugins/docker/workflow/client/DockerClientTest.java
Outdated
Show resolved
Hide resolved
…-plugin into rm-volumes
@jglick thanks a lot for merging this! |
You are welcome, but if you care about this plugin, please consider https://www.jenkins.io/doc/developer/plugin-governance/adopt-a-plugin/ since there is a considerable backlog of unevaluated PRs, many of which lack test coverage or otherwise do not look completely safe to release without a maintainer actively checking for reports of regressions and committing to following up. |
Yes, that's fair. I'll evaluate it with care. |
For example, the following Jenkinsfile:
pipeline { agent { docker { image 'docker:dind' } } }
Will create the container based on
docker:dind
, which declares aVOLUME
that docker creates automatically (this is called anonymous volumes).However, due to the way how Jenkins deletes the container after finishing the build, it will leave the anonymous volume dangling on the agent.
Over time, this can cause issues as these volumes can consume a high amount of disk space and there's no current Jenkins mechanism to delete them other than "manually".
This now aligns with the behavior of
docker run --rm
, which not only deletes the container itself but also its anonymous volumes.Fixes JENKINS-70132