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.

Sign up for GitHub

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

Jump to bottom

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

Open
prontog opened this issue Aug 28, 2020 · 12 comments
Open

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

prontog opened this issue Aug 28, 2020 · 12 comments

Comments

@prontog
Copy link

prontog commented Aug 28, 2020

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
Copy link
Owner

gersilex commented Aug 28, 2020 •
edited
Loading

Hey glad to hear this is useful to you. So you basically want to leave out

apcupsd-docker/host-trigger-check.sh

Lines 15 to 20 in 5d45df2

# Plan shutdown in 5 minutes, if supported by shutdown script
shutdown -P +5 || true
echo "Stopping all Docker containers..."
docker ps -q | xargs --no-run-if-empty docker stop --time 300
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

This comment has been minimized.

Sign in to view
@gersilex

This comment has been minimized.

Sign in to view
cassiorossi added a commit to cassiorossi/apcupsd-docker that referenced this issue Sep 21, 2020
@cassiorossi
Pull Request - gersilex#3
da43f8a 
1. Changed the way the host-trigger-check.sh check hook script result
2. Correction of the variable DATE_MASK
3. Correction of IFS compability
gersilex pushed a commit that referenced this issue Oct 12, 2020
@cassiorossi
Use netcat/ncat as communication channel, allow multiple arbitrary sc…
c41fd2d 
…ripts in a folder

* 1. Changed to use nc to listen all UPS triggers
2. Changed the listner so it will be able to run scripts from other locations
3. Created a script to start, stop and rebuild the docker

* Changes from Pull Request

1. Removed "&" from CMD Dockerfile
2. Changed the proccess to use only apc event as parameter
3. Correction on docker_install.sh - move stop option position to first
4. Changed APC_SCR_DIR default value to "/etc/apcupsd/scripts.d"
5. Created a new env variable DATE_MASK to be used in the output messages
6. Handled output result from apc hook script
7. Handled apc hook script not exist warning
8. Changed IFS value
9. Removed other parameters value handle from acpccontrol
10. Changed netcat loop, more secure way was implemented.

* Pull Request - #3

1. Changed the way the host-trigger-check.sh check hook script result
2. Correction of the variable DATE_MASK
3. Correction of IFS compability

* Pull Resquest - New Features

1. Changed the netcat solution
	- Created a fifo in the docker
	- Send all events to it
	- Read it with tail to netcat server
	- Changed the netcat server aproach to stay reading all the time (don not miss events)

2. Created more log entries

* Pull requests adjusts

1 - Accepted the Dockerfile CMD option
2 - Removed the "-e" and accepted the hooked script execution approach
3 - Changed the way of stopping the proccess - however still kept the "send quit message" approach

* Header typo corretion
@gersilex

This comment has been minimized.

Sign in to view
@prontog

This comment has been minimized.

Sign in to view
@gersilex

This comment has been minimized.

Sign in to view
@cassiorossi

This comment has been minimized.

Sign in to view
@prontog

This comment has been minimized.

Sign in to view
@gersilex

This comment has been minimized.

Sign in to view
@cassiorossi

This comment has been minimized.

Sign in to view
@gersilex

This comment has been minimized.

Sign in to view
@gersilex
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@gersilex @prontog @cassiorossi