Manually stopping containers before shutdown + --restart unless-stopped

[prontog]

Hello and thanks a lot for this repo,

I've been using it on a Rpi with libreelec for a while now and it's been very helpful. The only issue I had was with the docker stop command in host-trigger-check.sh because I create all my containers using the --restart unless-stopped option. When manually stopping a container created with this option, it will never be started after a reboot (when power comes back).

I know that in your guidelines you specify that custom changes might be needed per use case but I would like to ask you why not simply halt the system? It will try to gracefully stop the docker service which will stop all containers. 'Systemd' usually has a sane timeout (90 secs on most installations I've seen).

Regards,

Panos

[gersilex]

Hey glad to hear this is useful to you. So you basically want to leave out https://github.com/gersilex/apcupsd-docker/blob/5d45df2bb9c8741c3f06a98a7684e8f0aa2bdcd1/host-trigger-check.sh#L15-L20 and hand the responsibility of gracefully stopping the containers over to the init system?

We can certainly try that. I use it on RancherOS and think that it kills quite quickly.

If you would like to try it and create a pull request, I'd certainly try it out and maybe merge it into this project. If not, we can also make it configurable.

[cassiorossi]

Hi, first of all, thanks a lot for you docker! It helped me a lot!

I've cloned your project and made some changes that worked for my reality and would like to send you back those changes so you can see what changes are useful for your reality.

My changes:

1. Created a apccontrol version script in order to handle all apc events;
2. Events can be customized in the host (used netcat for communication between docker and the host)
3. Created a script to start/stop and rebuild docker

Please let me know if want me to create a pull request for you.

Best regards,

Cássio Rossi

[gersilex]

Hey @cassiorossi

Glad you find it useful. Pull requests are always welcome. After reading your changes I still don't quite grasp how it works or what it does, so it might be best to look into the code directly. Unfortunately I could not find the repository in your GitHub account.

So yes, please go and create a pull request so we can look into your changes and see if we can bring them into the project if it might be useful for everyone's reality ;)

[gersilex]

@prontog Pull Request #4 was merged. There are no updates in the README.md, yet but maybe you can already use it if you understand what we discussed there.

The new host-trigger-check does not stop the containers anymore but lets you decide what to do by putting a script file into a folder. Let us know if this is useful to you or you still miss something :)

[prontog]

@gersilex thanks for letting me know. I don't want to disappoint you, but I don't think it was a good idea accepting such a big merge request that is not even documented. If I were to find this repo as it is right now I would pass and look for something else. I liked it before. It was simple and worked (except from the point I raised in this issue). Now it is complex to the point of unreadable. It also does not work out of the box since it will not shutdown without adding you script. Sorry for my late response and again thanks for your work.

[gersilex]

Thank you for your honest comment; really appreciate it. I thought the same yesterday and began to split up the tags on the Docker hub into v1 and v2 to prevent any accidental updates.

I will consult with Cassio and maybe we will maintain his methods in his repository and keep the old method in my repository.

Let's keep this issue open until a final decision has been made.

[cassiorossi]

Hi @prontog, first of all, sorry for using your issue to apply those changes: It was a mistake. My original idea was to create a new issue.

Reading your post I would say that the new solution major problem was not considering the existing installations.

That way, I would say that we could work to create a solution that considers the existing installations and, during this process, we could try to make the code simpler (which is always better).

However, please, my push was only a contribution. Therefore, if its preferable for the project to continue without it, be 100% comfortably to do that way.

Count with me.

Best regards,

Cássio Rossi

[prontog]

Hello @cassiorossi, to tell you the truth, backward compatibility didn't cross my mind (and it should). It is very important and perhaps it could be solved by adding default handlers (in the scripts) that handle each event in the same way the previous version did. That's just an idea. The thing with this project is that if backward compatibility breaks we will, most likely, find it the hard way when the next outage happens...

What is also important, is the documentation. Without it, you can't expect new users to choose this project.

Best Regards and sorry for the delay,

Panos

[gersilex]

Really glad of the honest and sensible conversation. @cassiorossi what about moving the changes from this repo to your repo, so that users can just stay here where it "just works" the way it did before and new users looking for a more flexible way can choose your repository? We can link to each other in our READMEs. What do you think?

[cassiorossi]

Hi @gersilex and @prontog, thanks for your great comments and ideas! Regarding keep a repository with me, I must be honest with you: I am more interesting in contribute with projects than maintain a repository. For professional reasons it would be complicated to me to keep it up to date. I already have a copy of the project in my local git (new version), so please be comfortable to rollback your repository to the old solution or create it in another project.
If you decide to move forward with the @prontog suggestion on this project or in a new one, please count with me to finish the job. Please, stay 100% comfortable to decide the best path to your project and let me know if you need a hand! :) Best regards, Cássio Rossi

[gersilex]

Alright I'll go ahead and put the current state into a branch so we can get back to it whenever we need to and bring the master back to the classic state. I'll also collapse all those messages here so we can re-use this thread for the initial issue.

[gersilex]

Regarding the initial issue @prontog: I understand that stopping the containers ran with --restart=unless-stopped won't start again, yes.

Thinking about it, it really makes sense to not stop the containers before shutting down. I mean, personally, I still have to do it because RancherOS timeout is way below 60 seconds and my fat containers never exit that fast. But - as special use cases may need tinkering with the script - maybe it is me who has the special use case.

I still consider stopping each of those containers more safe but I also understand the side-effect. Maybe I should just comment it out here or make it configurable.