While I agree that "orphaned" containers can be a nuisance, I don't think this is the proper way to handle this. Not automatically deleting a volume when deleting a container is (afaik) by design. It allows removing a container without permanently destroying the data generated by that container.

Also, software like fig/docker-compose relies on this when re-creating containers; they destroy the old container (keeping the volume), then create a new container and attach the volume of the old container.

Docker surely needs some improvement to handle "orphaned" volumes, because right now, they are basically invisible, unless you know where to look for them.

There's a proposal that handles this, here: #8484. While that PR has not yet been accepted, the author of that PR also created a utility that does the same, but as a separate tool; you can find it here: https://github.com/cpuguy83/docker-volumes

Ping @cpuguy83: thoughts?

I'd probably change the error message, but I like this.
It helps make sure volumes are cleaned up without just blindly deleting the underlying data.

It may be that fig/compose would need to migrate to using the same volume-container pattern that everyone else uses. But the plain use-case should not be ignored. Using a separate utility or running some maintenance script to cleanup is technical debt. It would be better to fix the problematic behavior directly. This is especially problematic when users are unaware that volumes have been created by a container and potentially filled with data. It is a side effect that leaves a machine in a bad state.

I don't think this is much different than not being allowed to remove a dir that is not empty in the shell (via rm, vs rm -r), this is why I like this solution.

@allingeek agreed (wrt fig/compose), there are also plans to change the "recreate" step in compose; docker/compose#874

I'm still a bit hesitant; I can imagine cases where you'd want to delete a container, but preserve the volume. But yes, you'd probably be able to do so by adding an dummy container using --volumes-from, or exporting the volume.

I'm changing this to code review because I think this design looks good (and sense there's been no movement from anyone else).

@allingeek this one needs a rebase if you're still up for it.
Thanks!

I'll take care of that this morning.

This needs to be very carefully documented. Also I'm still not sure about this. It'll break 99% scripts, because -v option is pretty unknown to community.

I agree that this should be well documented in the release notes. Making people aware of the issue that their scripts are creating is sort of the point. I haven't seen much evidence that -v is unknown though. This is the only way I'm aware of to remove a Docker managed volume without manual intervention.

I haven't seen much evidence that -v is unknown though.

Well, there are lots of reports of "Docker leaking disk space"; many of those turn out to be people that aren't aware that volumes won't be removed when doing docker rm (or docker run --rm ...).

All the more reason:)

All the more reason:)

Agree with you there, but it is a breaking change. Still hoping the volumes-management proposal will make its way back somehow, which will make "dangling" volumes no longer a problem (because they can be managed even without containers attached to them)

Relatively speaking, this is a very polite breaking change. It improves the platform and prevents tools built around it from leaking resources. It also provides a hint for a three character change that adopters can make to fix their integration.

I like volume management as well, but adopting that sort of thing would be a much more significant undertaking. A major augmentation to the UX is maybe a bit drastic just to avoid dealing with a funky interface problem. People will forgive the short term pain.

Looks like I'll need to update the integration tests.

The current integration test that is failing tests container removal with the -f option. The help text for -f reads, "Force the removal of a running container (uses SIGKILL)" I'm going to modify the integration test rather than adjust the functionality because this option is specifically about stopping a running container.

Moving this back to design-review, I think I was being a bit too pushy, need another maintainer to ok design... sorry @allingeek

This needs a closer look from @icecrime and @crosbymichael.

If I understand this correctly, a breaking change is being introduced to expose a possible resource leakage. If there is no other mechanism to expose this to a user (ie list-volumes from #8363), then this would be considered an acceptable tradeoff.

ping @tiborvass @jfrazelle @icecrime @crosbymichael
I'm okay with this change, but we need to decide.

@allingeek @cpuguy83 I'm torn between this is cool for today and this is not what we want long term. I would much rather have volume management like in @cpuguy83's proposal. You should definitely want to be able to remove a container without removing the volume. The problem is that today they are too much linked.

Should we accept this PR because it solves a problem today, knowing that we would want to revert the behavior in the future? Maybe.

I like that it forces someone people to do something with their data.
In cases where a new container is replacing the existing one they can just go ahead and rename the current one, do volumes-from, then delete the old one.
If they truly don't need the data anymore, then do the rm -v.

I don't think that first class volumes fix the UX problem. The real issue here is visibility on orphan volumes. First-class volume operations help with volume management, but do nothing for orphan visibility at container deletion time. Even with the features proposed in #8363, a user should still be presented with a warning when they delete the only container attached to a volume.

@allingeek except that if you had volumes as first class, you could list orphaned ones and delete them.

Yes, you could list them. But unless you are informed that you've created one, or you have regular asynchronous pruning as part of your disciplined workflow, you will never be prompted to perform a cleanup.

@allingeek what about the usecase where you do want orphaned volumes? I guess you could ask for the user to create via the volumes commands instead of creating it when creating a container. But then it's an asymmetry... Idk :S

Yes, I would prefer the prevention of garbage vs forcing someone to clean it up manually.
Also, we don't have a volumes command yet :)
Last I heard we wanted that to be out of process anyway.

Yes, I would prefer the prevention of garbage vs forcing someone to clean it up manually.

Well, from a UX perspective, at least it's consistent with exited containers because users didn't run with --rm and "dangling" images. Whether that's good or bad, that's a different thing :-)

There are two UX issues. First, it is not possible to cleanup orphan volumes with docker. Second, volumes that can be created as invisible side-effects can be orphaned without warning to the user. The proposed change both prevents the creation of orphaned volumes and informs the user. Existing tools that do not depend on the orphan volume behavior can migrate with a two character change. Those that do depend on that behavior can use the same volume-container pattern that everyone else uses.

Maintaining problematic behavior because other parts of the project exhibit similar, but wholly unrelated problematic behavior is not a positive UX trait. I could never imagine a user thinking to themselves, "I'm sure glad Docker didn't merge a fix to this disk leak. It would have really confused me."

ping @docker/core-maintainers Can we get a final decision on this?

First, we need some testcases.
Second, given a choice between today's design (creating orphans w/o knowing it and not being able to clean them up) and this, I'd prefer this PR.

Well, it's in design-review, so let's not ask to add new code when the design is not approved.

oops - always forget to check that :-)

ping @crosbymichael since its a UX change.

ping @docker/core-maintainers

This PR as become totally obsolete, none of the methods used here exist on Docker master anymore.

I'm going to close it for now. This will become easier to implement and understand from an UX point of view after a few more improvements in volume management.

For those following this; top level volume management is now implemented in #14242, which was just merged, and will be in docker 1.9

If you want to test it before that, nightly builds, build from master can be found at https://master.dockerproject.org