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

Kill unhealthy #349

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

red-avtovo
Copy link

Since docker-compose has restart:unless-stopped option I added self-killing healthcheck

@gynnantonix
Copy link

I don't think this change is a good idea: it introduces unexpected side-effects into the healthcheck.

@red-avtovo
Copy link
Author

It really does, but what is the point of having unhealthy label without automatic recovery?

@red-avtovo
Copy link
Author

As another way of doing it I see adding this guy to a docker-compose.
https://hub.docker.com/r/willfarrell/autoheal/

@gynnantonix
Copy link

Sure. If someone who's using this image explicitly wants a management layer that kills unhealthy containers, that's their business. It'd be inappropriate to force that on them by baking such logic into this image, though.

@luckydonald
Copy link

Yeah https://hub.docker.com/r/willfarrell/autoheal/ is the way to go.
Healthchecks only ever report a status.

@red-avtovo
Copy link
Author

Finally found some time to update my PR. I believe this solution worth to be merged

@luckydonald
Copy link

Looks good to me.
Not sure if AUTOHEAL_CONTAINER_LABEL: all is the best way, or instead the container should get a label instead, so we don't suddenly restart other services on that docker server as well.
Maybe @dperson needs to decide that.

@red-avtovo
Copy link
Author

@dperson any objections?

@luckydonald
Copy link

To elaborate more:
I'd prefer having the restart label on that exact container, so it doesn't accidentally kill other containers which might be there before this.

However that would introduce a change to the configuration to the actual image, which might confuse new users.
They might think that would be needed for running VPN.

@gynnantonix
Copy link

How will killing the container that's hosting the network stack for other containers affect those other containers?

@red-avtovo
Copy link
Author

red-avtovo commented Jan 13, 2021

After you will restart a network container, all dependent containers will loose network at all. In my setup I restart them as well to update network link to net container. So far it works flawlessly

@luckydonald
Copy link

In that case, could it make sense to include that in the docker file as well?

@red-avtovo
Copy link
Author

Now only containers with label autoheal=true will be killed

@luckydonald
Copy link

I like that.
Actually started using autoheal in my own system now as well.

@luckydonald
Copy link

@dperson or someone who is trusted with the merges would need to have a look at it now.

@red-avtovo
Copy link
Author

@dperson is there anything I can do to merge this PR?

@martiGIT
Copy link

any update with that? its a great idea

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants