Watchtower: SIGTERM sent to container before lifecycle-hooks "pre-update" finishes

[Jacobh2]

Describe the bug
Seems like the container is killed before the pre-update script has time to finish. Does it wait for the pre script to exit before sending SIGTERM?

To Reproduce
Create a script that takes longer to finish than what the container takes to finish. For example put a sleep for some seconds and then print to a file after the sleep, and the print will never happen.

Expected behavior
I expect my pre-update script to completely finish until the SIGTERM signal is being sent by watchtower. In my case I am doing a curl in order to tell my server that my client is updating a container

Environment
Running docker via docker compose on raspberry pi 3b+ and watchtower built from master targeted to arm:
Linux xyz 4.14.34-hypriotos-v7+ #1 SMP Sun Apr 22 14:57:31 UTC 2018 armv7l GNU/Linux
watchtower built using the standalone dockerfile with GOOS=linux GOARCH=arm

Logs from running watchtower with the --debug option

time="2019-08-12T08:49:54Z" level=debug msg="Pulling <image> for /<container-name>"
time="2019-08-12T08:49:54Z" level=debug msg="Loaded auth credentials <json blob> from /config.json"
time="2019-08-12T08:49:54Z" level=debug msg="Got auth value: <value>"
time="2019-08-12T08:49:54Z" level=debug msg="Got image name: <image>"
time="2019-08-12T08:50:04Z" level=info msg="Found new <image> image (sha256:<hash>)"
time="2019-08-12T08:50:19Z" level=info msg="Executing pre-update command."
time="2019-08-12T08:50:19Z" level=info msg="Stopping /<container-name> (<container-id>) with SIGTERM"
time="2019-08-12T08:50:20Z" level=debug msg="Skipped another update already running."
time="2019-08-12T08:50:20Z" level=debug msg="Scheduled next run: 2019-08-12 08:50:50 +0000 UTC"
time="2019-08-12T08:50:22Z" level=debug msg="Removing container <container-id>"
time="2019-08-12T08:50:22Z" level=info msg="Creating /<container-name>"
time="2019-08-12T08:50:22Z" level=debug msg="Starting container /<container-name> (<new-container-id>)"
time="2019-08-12T08:50:27Z" level=info msg="Executing post-update command."
time="2019-08-12T08:50:27Z" level=info msg="Removing image sha256:<old-hash>"
time="2019-08-12T08:50:27Z" level=debug msg="Scheduled next run: 2019-08-12 08:50:50 +0000 UTC"

Additional context
Watchtower command: command: --interval 30 --cleanup --enable-lifecycle-hooks --debug

[zoispag]

Hey @simskij, can we bump the priority on this?

I am looking for a way to check that no job is currently running and even though I could do it with a different SIGTERM, using a pre-update script, seems the most straightforward and easy way.

[simskij]

Sure! Seems reasonable! I’ve been doing a lot of conferences and instructor gigs lately which have affected my ability to work on watchtower. Things are starting to look a bit calmer now though! Bumping this as it’s an error in an existing feature.

[simskij]

A quick update: I've got started on this one. Expecting it to be done soon.

[simskij]

Another update:

The feature is now done, I hope. It's pushed to branch feature/367 for some additional testing before merging it. If all looks good, it will be part of the next release.

[lmussier]

Hi,
Just to be sure my understanding id good.
The desired behavior is, all operations having a hook (not only preupdate) wait for the hook to finish before going further?
I'm intressting in a use case when somebody/something have to validate the update (more or less what @zoispag want to do)

Ps: Thanks for the great work.

[simskij]

The desired behavior is, all operations having a hook (not only preupdate) wait for the hook to finish before going further?

In my PR, it's only the pre-update hook, as that one is the only one that might get interrupted/terminated by the container being replaced.

[simskij]

I know that @zoispag has been extremely busy with other duties, which is totally understandable. Would anyone else like to have a stab at evaluating these changes? Maybe @victorcmoura or @codelica?

[victorcmoura]

@simskij, sure! I'll take a look at it

[victorcmoura]

@simskij, the changes seem to work. Unfortunately, it is hard to include some testing due to ExecuteCommand mock, it will never return an error. Looks good enough to me.