Skip to content
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

Add volumes cleanup #142

Merged
merged 19 commits into from May 23, 2018

Conversation

@rccrdpccl
Copy link
Contributor

commented Nov 13, 2016

Sorry if I did not ask about this feature before doing the PR, but we are using this feature to extend docker-gc so I had to write it anyway.

@imkin

This comment has been minimized.

Copy link

commented Dec 22, 2016

Is there a timeline on which this would be merged and?

@rccrdpccl

This comment has been minimized.

Copy link
Contributor Author

commented Dec 29, 2016

@imkin This is integrated in my fork https://github.com/rccrdpccl/docker-gc/ , but there is no timeline to merge it in this repo

@sgerrand
Copy link

left a comment

Some typos here and there, but the functionality is useful. We've got workarounds in place to do this on top of docker-gc. It'd be great to have this exist within the app itself.

README.md Outdated
@@ -101,6 +102,14 @@ mariadb-data
drunk_goodall
```

### Excluding Volumes From Gargbage Collection

This comment has been minimized.

Copy link
@sgerrand

sgerrand Feb 28, 2017

🙋 Typo: "Garbage Collection"

README.md Outdated
@@ -101,6 +102,14 @@ mariadb-data
drunk_goodall
```

### Excluding Volumes From Gargbage Collection

There can be occasions where you dont want to remove a dangling volume.

This comment has been minimized.

Copy link
@sgerrand

sgerrand Feb 28, 2017

🙋 Typo: "don't"

README.md Outdated
### Excluding Volumes From Gargbage Collection

There can be occasions where you dont want to remove a dangling volume.
In such case, you can create `/etc/docker-gc-exclude-volumes` (or specify

This comment has been minimized.

Copy link
@sgerrand

sgerrand Feb 28, 2017

"To enable this functionality you can create a file named /etc/docker-gc-exclude-volumes"

@rccrdpccl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2017

@sgerrand Thanks for the feedback, I have fixed the typos now!

@vdice

This comment has been minimized.

Copy link

commented May 25, 2017

Hoping to add support for this feature in the official spotify/docker-gc binary/docker image. Anything stopping getting this in?

fi
else
if [[ -z "$VOLUME_DELETE_ONLY_DRIVER" ]]; then
$DOCKER volume ls --filter "dangling=true" -q | grep -v -f $EXCLUDE_VOLUMES_IDS_FILE > volumes.reap

This comment has been minimized.

Copy link
@vdice

vdice May 25, 2017

My only suggestion would be to either check to make sure $EXCLUDE_VOLUMES_IDS_FILE exists (if not, don't bother grepping) or follow the examples set previously with the image/container excludes logic where if file doesn't exist, filepath just set to /dev/null. (See here)

That way, we can maintain tradition of opting in for volume removal but not having to supply an exclusion file (in such case, we'd assume all unused/dangling volumes are intended to be removed.)

Otherwise, definitely excited for this addition! (Have been testing locally w/ forked ricc/docker-gc image...)

This comment has been minimized.

Copy link
@rccrdpccl

rccrdpccl May 31, 2017

Author Contributor

Ok, I've gone for setting filepath to /dev/null. Please let me know if I can improve it further!

This comment has been minimized.

Copy link
@vdice

vdice May 31, 2017

Great, thanks @rccrdpccl. LGTM -- that was my one suggested change. Now let's get this in! ping @sgerrand

@mattnworb
Copy link
Member

left a comment

looks good to me overall, and thanks for the contribution. I just have two questions below:

set +e
$DOCKER volume ls --filter "dangling=true" -q &> /dev/null
VOLUMES=$?
set -e

This comment has been minimized.

Copy link
@mattnworb

mattnworb May 31, 2017

Member

just curious - why do you need to set and unset e here?

This comment has been minimized.

Copy link
@rccrdpccl

rccrdpccl May 31, 2017

Author Contributor

Here we use set and unset e because docker volume was added in docker 1.9, so I wanted the program to stop and fail. (if I recall correctly this project used to use an older version at the time I have started with this feature). It might be not necessary now, as the image of docker-gc is currently build on a newer version....

docker-gc Outdated
@@ -54,6 +55,9 @@ SYSLOG_LEVEL=${SYSLOG_LEVEL:=info}
SYSLOG_TAG=${SYSLOG_TAG:=docker-gc}
DRY_RUN=${DRY_RUN:=0}
EXCLUDE_DEAD=${EXCLUDE_DEAD:=0}
REMOVE_VOLUMES=${REMOVE_VOLUMES:=0}
EXCLUDE_VOLUMES_IDS_FILE=${EXCLUDE_VOLUMES_IDS_FILE:=/etc/docker-gc-exclude-volumes}
VOLUME_DELETE_ONLY_DRIVER=${VOLUME_DELETE_ONLY_DRIVER:=locale}

This comment has been minimized.

Copy link
@mattnworb

mattnworb May 31, 2017

Member

why does this default to locale?

This comment has been minimized.

Copy link
@rccrdpccl

rccrdpccl May 31, 2017

Author Contributor

Oh, good question: it should be local. This was a pretty nasty bug too, because if you would not specify any value for VOLUME_DETE_ONLY_DRIVER it would not actually remove any volume... It is now fixed.

@mvisonneau

This comment has been minimized.

Copy link

commented Sep 14, 2017

any other reason not to merge this one ?

@paulRbr

This comment has been minimized.

Copy link

commented Sep 15, 2017

The changes on this MR looks good to me. Would me great to have a maintainer taking a look at it :)

@paulRbr

This comment has been minimized.

Copy link

commented Sep 15, 2017

@rccrdpccl could you squash your commits in understandable unit of work please?

@rccrdpccl

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2017

@paulRbr I agree with you that messages should have been cleaned up, however this is not my private tree anymore. I would rather squash it on merge: although there are a few messages, I think this feature is small enough to be collapsed to one single commit. What do you think?

@paulRbr

This comment has been minimized.

Copy link

commented Sep 18, 2017

@rccrdpccl yes one single commit seems reasonable 👍

@Constantin07

This comment has been minimized.

Copy link

commented Mar 27, 2018

when this PR will be merged ?

@lainekendall

This comment has been minimized.

Copy link

commented Apr 25, 2018

is there an update on the status of this? would like to see it!

@davidxia davidxia merged commit 7ce3b44 into spotify:master May 23, 2018

@davidxia

This comment has been minimized.

Copy link
Member

commented May 23, 2018

Thanks, @rccrdpccl. Sorry for the delay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.