dokku cleanup removed all "exited" containers #1220

Closed
palamccc opened this Issue Jun 7, 2015 · 31 comments

Projects

None yet

8 participants

@palamccc
palamccc commented Jun 7, 2015

dokku should only remove containers created by dokku. It should not remove containers created by others.

I'm running host, with dokku and other docker images. I use 'data-only' container pattern to store data in a 'non-running' container. But dokku removes my non-running data container as well.
http://container42.com/2013/12/16/persistent-volumes-with-docker-container-as-volume-pattern/

https://github.com/progrium/dokku/blob/4aedb6507a3bdec1ff29be2ff60366075186a72e/dokku#L170

@josegonzalez
Member

How are we to detect that you're running a non-dokku provided container?

@3onyc
Contributor
3onyc commented Jun 8, 2015

Possible solution is to label (--label) containers created with Dokku, then modify clean-up to only remove those. Note that any plug-ins need to be modified to also label their containers.

You'd need to modify dokku itself to accomplish this though.

@palamccc
palamccc commented Jun 9, 2015

Is it possible to run all docker build commands with "--rm" option, it will automatically remove container after doing the work. so there won't be any need for cleanup.

@3onyc
Contributor
3onyc commented Jun 9, 2015

That would potentially leave images and containers, for example the Docker daemon crashing would prevent containers (with --rm) from being deleted.

@michaelshobbs
Member

it's also very useful to have the intermediate images/containers around debug deployments. Dokku was never (from what I can tell) specifically written to "play nice" with non-dokku managed images and containers.

Having said that, we can definitely label dokku core containers but as @3onyc mentioned, plugins will also need to be modified.

Anybody else have thoughts on this?

@zimbatm
zimbatm commented Jun 18, 2015

I have just been hit by that issue. Maybe the doc should say more clearly that dokku is not meant to work with other docker containers.

@josegonzalez
Member

@michaelshobbs Not sure I understand your first comment:

it's also very useful to have the intermediate images/containers around debug deployments. Dokku was never (from what I can tell) specifically written to "play nice" with non-dokku managed images and containers.

In any case, I would rather we just state in the docs that dokku doesn't play nice with externally-managed containers. We should definitely not delete data-only containers, though I'm not sure what we can do here. Maybe support a special, dokku-specific label that we'll use to ensure we don't kill certain containers?

@alessio
Contributor
alessio commented Jun 30, 2015

👍 for containers labelling

@josegonzalez josegonzalez added a commit that referenced this issue Sep 6, 2015
@josegonzalez josegonzalez Do not remove any containers that have the label `dokku`. Closes #1220
This can be used to mark data containers as non-removable by the cleanup function
d0c0ffc
@josegonzalez
Member

I recently needed to implement this feature in order to not kill dokku service containers on startup (or whenever dokku cleanup is run). Checkout #1444 for more details.

/cc @Flink

@Cactusbone
Contributor

not fan of the "dokku" label, imo it means it's managed by dokku, when it's not. I don't have another suggestion though :/ anyway this is just what i needed :) any date for next release ? :)

@jazzzz
Contributor
jazzzz commented Sep 7, 2015

it's also very useful to have the intermediate images/containers around debug deployments. Dokku was never (from what I can tell) specifically written to "play nice" with non-dokku managed images and containers.

Having said that, we can definitely label dokku core containers but as @3onyc mentioned, plugins will also need to be modified.

OTOH, now I have to change all the tools I use beside dokku to add a dokku label to every images and container.

I think it would be better if dokku could play nice with non-dokku images and containers.

@josegonzalez
Member

@Cactusbone No, I take that label to mean that it is related to dokku. Personally I assume anything in docker is somehow managed by dokku, and if it's not, then that's up to the user of dokku to figure out. This is just giving you all a simple way to skip those containers.

@jazzzz We could disable the cleanup command and let you self-manage that, but then we'll use more disk space/memory and users will have other issues. Do you have an alternative solution for this problem? Also, what other tools are you using in conjunction with dokku on the same server?

@jazzzz
Contributor
jazzzz commented Sep 8, 2015

@josegonzalez I use many images and containers beside dokku: some are created and started with shells scripts, some with ansible, some directly from Docker Hub, some are manually created... It would make dokku much less useful for me if dokku needs to manage everything in docker.

I think it would be better to tag dokku related containers and images with the dokku instead of the reverse: I would be misleading to see the containers and images not related to dokku with the dokkulabel. And I don't think many plugins would be broken (if any).

Anyway, a plugin with a data-only container will need to set the dokku label on this container but not on the others: it feels strange.

@josegonzalez
Member

@jazzzz I still don't understand how you would want dokku cleanup to work. We can either cleanup containers or we don't. In either case, someone is going to have issues.

@josegonzalez
Member

As well, we are only using this to remove exited containers. So if your container is running,it sticks around.

@jazzzz
Contributor
jazzzz commented Sep 8, 2015

@josegonzalez I would prefer if dokku only remove exited containers with the dokku label, I think it would be safer and cleaner.

With the current implementation, if I use a docker container with a database in a data-only container (with a dokku plugin or not), because this container is not running, dokku will delete this container and I will lose my data.

@michaelshobbs
Member

(Preface: IMHO)
I will re-iterate here that dokku is a docker container manager and there is no way to cleanly have dokku manage its containers and not screw something up for some edge case somewhere. The project has taken a conscious direction of optimizing for the 'simplest' deployment (see DO droplet for example) where dokku manages all docker containers on a given host. Sharing the docker daemon is considered an 'advanced' deployment and is to be handled by the managing user.

@josegonzalez what exactly needed to re-implemented for service containers? if a service container is running it doesn't get stopped or removed and i'd hope that service containers are using docker volumes for persistence in case the container does get 'accidentally' cleaned up by some other (user's or otherwise) action.

@jazzzz
Contributor
jazzzz commented Sep 8, 2015

Not sure if I was clear, I was talking about data volume containers. These containers are not running, thus dokku removes them.

@josegonzalez
Member

@michaelshobbs they persist data and config onto disk (in /var/lib/dokku/services/mysql/service-name for instance). Is that what you mean? I personally like the idea of the same container being started and not being tossed just because the machine rebooted and we are running dokku cleanup.

Re: container-management, I agree that I see dokku as the sole-arbiter of containers on a given host. The simplest thing I can think of is allowing people the ability to disable dokku cleanup and let them handle cleanup on their own.

@jazzzz those containers are running for me?

root@dokku:~# docker create -v /dbdata --name dbdata gliderlabs/herokuish /bin/true
7c4916b095369ba27779fe1d1842215255d563c0f2805fd0125762c11f3ffb98
root@dokku:~# docker ps -a
CONTAINER ID        IMAGE                         COMMAND             CREATED             STATUS              PORTS               NAMES
7c4916b09536        gliderlabs/herokuish:latest   "/bin/true"         2 seconds ago                                               dbdata
@jazzzz
Contributor
jazzzz commented Sep 8, 2015

@josegonzalez for me it's not running, Try with docker ps (without -a). Also STATUS is empty.

@josegonzalez
Member

Interesting, I guess if there is no status, that is treated as exited by docker. What fun!

@josegonzalez
Member

Seems like there isn't a way to check if a container is a data-only container, as they are a convention, not an actual docker construct.

@jazzzz
Contributor
jazzzz commented Sep 8, 2015

Seems like there isn't a way to check if a container is a data-only container.

Yeah, that's why I suggest that dokku creates a dokku label for the containers it creates and only the exited containers with this label. Are there really plugins that depend on dokku cleanup?

@josegonzalez
Member

We don't want to remove the service plugins on boot of the server, hence the current hack.

@josegonzalez josegonzalez added a commit that referenced this issue Sep 8, 2015
@josegonzalez josegonzalez Revert change that whitelisted dokku-labeled containers
Rather than having a one-off hack for "blessed" containers, we should solve the issue properly. As that would take a bit more work, reverting to the previous state is preferred.

Better that we solve a problem problem than with a hack that we'll need to roll back in the future.

Refs #1220

[ci skip]
88bd50a
@josegonzalez josegonzalez added a commit that referenced this issue Sep 8, 2015
@josegonzalez @michaelshobbs josegonzalez + michaelshobbs Revert change that whitelisted dokku-labeled containers
Rather than having a one-off hack for "blessed" containers, we should solve the issue properly. As that would take a bit more work, reverting to the previous state is preferred.

Better that we solve a problem problem than with a hack that we'll need to roll back in the future.

Refs #1220

[ci skip]
f29140c
@josegonzalez josegonzalez added a commit that referenced this issue Sep 9, 2015
@josegonzalez @michaelshobbs josegonzalez + michaelshobbs Revert change that whitelisted dokku-labeled containers
Rather than having a one-off hack for "blessed" containers, we should solve the issue properly. As that would take a bit more work, reverting to the previous state is preferred.

Better that we solve a problem problem than with a hack that we'll need to roll back in the future.

Refs #1220

[ci skip]
f7ebe94
@josegonzalez josegonzalez added a commit that referenced this issue Sep 10, 2015
@josegonzalez @michaelshobbs josegonzalez + michaelshobbs Revert change that whitelisted dokku-labeled containers
Rather than having a one-off hack for "blessed" containers, we should solve the issue properly. As that would take a bit more work, reverting to the previous state is preferred.

Better that we solve a problem problem than with a hack that we'll need to roll back in the future.

Refs #1220

[ci skip]
ce28924
@josegonzalez josegonzalez added a commit that referenced this issue Sep 15, 2015
@josegonzalez @josegonzalez josegonzalez + josegonzalez Revert change that whitelisted dokku-labeled containers
Rather than having a one-off hack for "blessed" containers, we should solve the issue properly. As that would take a bit more work, reverting to the previous state is preferred.

Better that we solve a problem problem than with a hack that we'll need to roll back in the future.

Refs #1220

[ci skip]
0b77bbd
@josegonzalez josegonzalez added a commit that referenced this issue Sep 15, 2015
@josegonzalez @josegonzalez josegonzalez + josegonzalez Revert change that whitelisted dokku-labeled containers
Rather than having a one-off hack for "blessed" containers, we should solve the issue properly. As that would take a bit more work, reverting to the previous state is preferred.

Better that we solve a problem problem than with a hack that we'll need to roll back in the future.

Refs #1220

[ci skip]
aaa7a4e
@josegonzalez josegonzalez reopened this Oct 16, 2015
@josegonzalez
Member

@michaelshobbs we can probably add the dokku label via the docker-options plugin. At a later date - probably 0.5.0? - we can update dokku cleanup to only take those with said label into account when running cleanup?

@michaelshobbs
Member

Yeah works for me

@michaelshobbs
Member

@josegonzalez plugins would need to follow this as well, right?

@josegonzalez
Member

We're only going to cleanup apps started/stopped by dokku. If a plugin wants to opt into that cleanup, up to them.

For example, the official service plugins shouldn't (because we might accidentally cleanup a stopped container during a deploy).

@michaelshobbs
Member

Ok I noted in the docs of #1828 how to do it and why you might want. The language might need to be cleaned up a bit.

@josegonzalez
Member

@michaelshobbs worth adding this to 0.5.x or waiting till 0.6.x?

@michaelshobbs
Member

@josegonzalez your call. PR created 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment