Bug 17814 - koha-plack --stop should make sure that Plack really stop
Summary: koha-plack --stop should make sure that Plack really stop
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Pongtawat
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-12-26 10:43 UTC by Pongtawat
Modified: 2017-05-10 09:29 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Unverified patch (827 bytes, patch)
2016-12-26 10:54 UTC, Pongtawat
Details | Diff | Splinter Review
Add retry to koha-plack stop (827 bytes, patch)
2017-04-04 16:13 UTC, Pongtawat
Details | Diff | Splinter Review
Bug 17814: koha-plack --stop should make sure that Plack really stop (986 bytes, patch)
2017-04-05 20:49 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 17814: koha-plack --stop should make sure that Plack really stop (2.10 KB, patch)
2017-04-19 19:49 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pongtawat 2016-12-26 10:43:59 UTC
koha-plack --stop doesn't ensure that Plack was really stopped before returning. 

So in case that koha-plack --stop was quickly follows by koha-plack --start (e.g. logrotate), it could leave Plack in stop state. This is due to koha-plack --start think that Plack was already started, while it actually is being stopped.

For me I think this is the cause why I got random Plack stop when logrotate is run. 

It should be similar to the case of Zebra in Bug #16885, and the solution might be the same: adding "--retry=TERM/30/KILL/5;" to start-stop-daemon command.

I just try that on our server and will see if the problem goes away.
Comment 1 Pongtawat 2016-12-26 10:54:26 UTC
Created attachment 58427 [details] [review]
Unverified patch

A simple patch follows that in #16885. If it work there, it should work here too. But not yet verified.
Comment 2 Magnus Enger 2016-12-30 14:19:20 UTC
Should this be "Needs signoff"? Or is it too early yet?
Comment 3 Barton Chittenden 2017-01-09 18:35:13 UTC
I think that --restart also has this issue. Here's a paste from a recent restart:

sudo koha-plack --restart glenoaks
[....] Restarting Plack daemon for glenoaks:[....] Stopping Plack daemon for gle[ ok :.
[FAIL] Error: Plack already running for glenoaks: failed!
 failed!
Comment 4 Pongtawat 2017-01-10 03:42:55 UTC
Won't --restart work properly if stop work properly? 
From the code, it seems that --restart just call stop and then start.
Comment 5 Pongtawat 2017-01-10 03:58:03 UTC
TEST PLAN
---------
1) Login to staff client and do something that will hold connection for a long time, e.g. a batch import or a slow report.
2) sudo koha-plack --stop mykoha
3) ps aux | grep plack   <-- a Plack process will still running work in 1)
4) wait for 1) to finish and all Plack processes exit
5) sudo koha-plack --start mykoha
6) apply the patch
7) repeat step 1)-2)
8) ps aux | grep plack   <-- There should be no Plack process running now
9) Note that work in step 1) might get terminated midway. Since we force Plack to stop after some wait.

Alternatively, I think we can test with --restart too. Before patch, it should behave like in comment 3. After patch, it should restart properly.
Comment 6 Pongtawat 2017-02-22 17:05:50 UTC
With this patch, I'm running without problem for a month. So, just hope this get merged in.
Comment 7 Pongtawat 2017-04-04 16:13:12 UTC
Created attachment 61845 [details] [review]
Add retry to koha-plack stop
Comment 8 Marc Véron 2017-04-05 20:49:18 UTC
Created attachment 61927 [details] [review]
Bug 17814: koha-plack --stop should make sure that Plack really stop

Followed test plan in comment #5.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 9 Jonathan Druart 2017-04-19 19:49:53 UTC
Created attachment 62421 [details] [review]
Bug 17814: koha-plack --stop should make sure that Plack really stop

koha-plack --stop doesn't ensure that Plack was really stopped before
returning

So in case that koha-plack --stop was quickly follows by koha-plack
--start (e.g. logrotate), it could leave Plack in stop state. This is
due to koha-plack --start think that Plack was already started, while it
actually is being stopped.

For me I think this is the cause why I got random Plack stop when
logrotate is run.

It should be similar to the case of Zebra in Bug #16885, and the
solution might be the same: adding "--retry=TERM/30/KILL/5;" to
start-stop-daemon command.

TEST PLAN
---------
1) Login to staff client and do something that will hold connection for
a long time, e.g. a batch import or a slow report.
2) sudo koha-plack --stop mykoha
3) ps aux | grep plack   <-- a Plack process will still running work in
1)
4) wait for 1) to finish and all Plack processes exit
5) sudo koha-plack --start mykoha
6) apply the patch
7) repeat step 1)-2)
8) ps aux | grep plack   <-- There should be no Plack process running
now
9) Note that work in step 1) might get terminated midway. Since we force
Plack to stop after some wait.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Kyle M Hall 2017-04-21 11:38:30 UTC
Pushed to master for 17.05, thanks Pongtawat!
Comment 11 Katrin Fischer 2017-04-21 22:23:57 UTC
Thx Pongtawat!
Comment 12 Katrin Fischer 2017-04-21 22:24:17 UTC
This patch has been pushed to 16.11.x and will be in 16.11.07.
Comment 13 Mason James 2017-05-03 04:33:48 UTC
Pushed to 16.05.x, for 16.05.12 release
Comment 14 Julian Maurice 2017-05-10 09:29:15 UTC
Pushed to 3.22.x for 3.22.21