Bug 21366 - Add Plack reload
Summary: Add Plack reload
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Packaging (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Cook
QA Contact: Marcel de Rooy
URL:
Keywords: release-notes-needed
Depends on:
Blocks:
 
Reported: 2018-09-16 22:25 UTC by Fridolin Somers
Modified: 2023-06-08 22:26 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00


Attachments
Bug 21366: Add koha-plack --reload to gracefully restart Plack/Starman (2.35 KB, patch)
2022-08-11 05:28 UTC, David Cook
Details | Diff | Splinter Review
Bug 21366: Add koha-plack --reload to gracefully restart Plack/Starman (2.43 KB, patch)
2022-08-11 07:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21366: Add --reload option to koha-plack help/manual (2.18 KB, patch)
2022-08-12 02:13 UTC, David Cook
Details | Diff | Splinter Review
Bug 21366: Add --reload option to koha-plack help/manual (2.40 KB, patch)
2022-08-22 01:51 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 21366: Add koha-plack --reload to gracefully restart Plack/Starman (2.52 KB, patch)
2022-08-23 11:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21366: Add --reload option to koha-plack help/manual (2.49 KB, patch)
2022-08-23 11:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2018-09-16 22:25:08 UTC
Starman daemon as an option to ask for "reload" meaning the renew of all workers, like Apache graceful restart.
Starman restart will kill at once the master process and workers.
Starman reload will not impact the master process, it will kill and recreate idle workers, it will wait for busy workers to end before renew them.

It is a nice way to refresh the plack middleware without impacting any running page, mostly useful for production servers.
Comment 1 Fridolin Somers 2018-09-16 22:28:03 UTC
To trigger this reload, juste run :

/bin/kill -HUP $MASTER_PID

MASTER_PID beeing the pid of the starman master process, can be fetched in pid file.

Source :
https://metacpan.org/pod/distribution/Starman/script/starman#SIGNALS
Comment 2 Joonas Kylmälä 2020-06-05 11:47:52 UTC
In koha-suomi fork we use start-stop-daemon to send the HUP signal, that's probably the preferable way since koha-plack uses start-stop-daemon already to send the process kill signal.
Comment 3 David Cook 2020-06-09 03:55:30 UTC
This would be great
Comment 4 David Cook 2022-05-20 01:16:51 UTC
(In reply to Joonas Kylmälä from comment #2)
> In koha-suomi fork we use start-stop-daemon to send the HUP signal, that's
> probably the preferable way since koha-plack uses start-stop-daemon already
> to send the process kill signal.

I'm only seeing "start-stop-daemon" for koha-plack --stop for some reason...
Comment 5 David Cook 2022-05-20 01:18:12 UTC
(In reply to Fridolin Somers from comment #1)
> To trigger this reload, juste run :
> 
> /bin/kill -HUP $MASTER_PID
> 
> MASTER_PID beeing the pid of the starman master process, can be fetched in
> pid file.
> 
> Source :
> https://metacpan.org/pod/distribution/Starman/script/starman#SIGNALS

I'd forgotten about this comment, but I just used the following today in prod:

kill -SIGHUP $(cat /var/run/koha/<instance name>/plack.pid)

I wanted to enable plugins and then reload after installing the plugin, and it was so elegant.
Comment 6 David Cook 2022-05-20 01:25:56 UTC
I don't know if we could use it with Koha package installs, as I think the koha-common service is stopped and started, although I'm not 100% sure of that mechanism at this point...
Comment 7 David Cook 2022-05-20 01:30:15 UTC
Question:

- Do we want "koha-plack --reload" or do we want "koha-plack --graceful" or both?

I think Apache treats reload/graceful/force-reload as the same.
Comment 8 Andreas Jonsson 2022-05-25 07:11:13 UTC
Would it be too risky to send a HUP-signal to the plack process automatically when a plugin is reuploaded?

I think that this would be very nice, as it would make plugins upgradable without command line access.
Comment 9 David Cook 2022-05-26 00:36:47 UTC
(In reply to Andreas Jonsson from comment #8)
> Would it be too risky to send a HUP-signal to the plack process
> automatically when a plugin is reuploaded?
> 
> I think that this would be very nice, as it would make plugins upgradable
> without command line access.

You know... I've been thinking about that idea for years but I never actually tried it. 

I just tried it now and it worked well!

With the Debian packages, each instance is run using a specific Linux user, and it looks like that Linux user can HUP processes it owns, so yeah... it works.

--

In terms of risk, I suppose allowing a web user to affect background Linux processes is not ideal. I have noticed that the REST API can be slow to startup and can consume a lot of CPU, so I suppose the risk would be that an accident or an attack could lead to a denial of service. But I think that's a fairly low risk. After all, you can already do a DDOS by attacking the OPAC search function.

So I think the risk is worth the reward.
Comment 10 David Cook 2022-06-03 01:43:14 UTC
(In reply to Andreas Jonsson from comment #8)
> Would it be too risky to send a HUP-signal to the plack process
> automatically when a plugin is reuploaded?
> 
> I think that this would be very nice, as it would make plugins upgradable
> without command line access.

I've opened bug 30897 for this one but it's easier said than done...
Comment 11 Jonathan Druart 2022-08-09 08:44:43 UTC
This bug report is assigned, can we expect a patch soon?
Comment 12 David Cook 2022-08-10 03:46:40 UTC
(In reply to Joonas Kylmälä from comment #2)
> In koha-suomi fork we use start-stop-daemon to send the HUP signal, that's
> probably the preferable way since koha-plack uses start-stop-daemon already
> to send the process kill signal.

Would that look like the following?

start-stop-daemon --pidfile ${PIDFILE} --user="${instancename}-koha" --stop --signal HUP;
Comment 13 David Cook 2022-08-10 03:47:18 UTC
(In reply to Jonathan Druart from comment #11)
> This bug report is assigned, can we expect a patch soon?

It should be a simple patch. I feel like I'd probably be more useful in test/QA for this one, but I could write it if no one else is going to...
Comment 14 David Cook 2022-08-11 05:28:47 UTC
Created attachment 139011 [details] [review]
Bug 21366: Add koha-plack --reload to gracefully restart Plack/Starman

This patch adds a "--reload" option to koha-plack, which allows
a sysadmin to gracefully restart Koha.

This is very useful when installing Koha plugins or deploying a hot fix
where you need to change code but don't want to interrupt anyone's
usage of Koha.

0. Apply patch
1. cp /usr/sbin/koha-plack /usr/sbin/koha-plack.bak
2. cp debian/scripts/koha-plack /usr/sbin/koha-plack
3. ps -efww | grep "starman"
4. Now at roughly the same time do the following two actions:
    4a. Go to http://localhost:8081/cgi-bin/koha/about.pl
    4b. koha-plack --reload kohadev
5. Note that the web request completes successfully
6. ps -efww | grep "starman"
7. Note that the "starman master" process stays alive, but the "starman worker" processes have been restarted
Comment 15 David Cook 2022-08-11 05:29:46 UTC
Ok patch prepared. Looking at Fridolin and Joonas for sign off and QA!
Comment 16 Jonathan Druart 2022-08-11 07:16:18 UTC
Created attachment 139015 [details] [review]
Bug 21366: Add koha-plack --reload to gracefully restart Plack/Starman

This patch adds a "--reload" option to koha-plack, which allows
a sysadmin to gracefully restart Koha.

This is very useful when installing Koha plugins or deploying a hot fix
where you need to change code but don't want to interrupt anyone's
usage of Koha.

0. Apply patch
1. cp /usr/sbin/koha-plack /usr/sbin/koha-plack.bak
2. cp debian/scripts/koha-plack /usr/sbin/koha-plack
3. ps -efww | grep "starman"
4. Now at roughly the same time do the following two actions:
    4a. Go to http://localhost:8081/cgi-bin/koha/about.pl
    4b. koha-plack --reload kohadev
5. Note that the web request completes successfully
6. ps -efww | grep "starman"
7. Note that the "starman master" process stays alive, but the "starman worker" processes have been restarted

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 17 Jonathan Druart 2022-08-11 07:16:52 UTC
Note that the help is missing the new option.
Comment 18 David Cook 2022-08-11 23:28:38 UTC
(In reply to Jonathan Druart from comment #17)
> Note that the help is missing the new option.

Believe it or not, that thought crossed my mind as I was falling asleep last night haha. I'll look at that.
Comment 19 David Cook 2022-08-12 02:13:42 UTC
Created attachment 139035 [details] [review]
Bug 21366: Add --reload option to koha-plack help/manual
Comment 20 David Cook 2022-08-12 02:29:43 UTC
(Side note, I've opened bug 31348 to handle graceful stopping of Starman/Plack as well.)
Comment 21 Victor Grousset/tuxayo 2022-08-22 01:51:00 UTC
Created attachment 139565 [details] [review]
Bug 21366: Add --reload option to koha-plack help/manual
Comment 22 Victor Grousset/tuxayo 2022-08-22 01:53:13 UTC
Last patch amended to explain what reloading means.

Reload the Plack daemon for the desired Koha instances.
↓↓↓
Reload the Plack daemon for the desired Koha instances, letting the busy workers finish processing their requests before restarting them.


Tell me if the phrasing can be improved.
Comment 23 Marcel de Rooy 2022-08-22 07:46:19 UTC
Reading this on CPAN:
Sending HUP signal to the master process will restart all the workers gracefully (meaning the currently running requests will shut down once the request is complete), and by default, the workers will pick up the code changes you make by reloading the application.

If you enable --preload-app option, however, the code will be only loaded in the startup process and will not pick up the code changes you made. If you want to preload the app and do graceful restarts by reloading the code changes, you're recommended to use Server::Starter, configured to send QUIT signal when superdaemon received HUP ...
[End of quote]

While testing with about and inserting a small code change, I notice that while I started the about web request before the plack reload, the code change already is visible in the web request.
I think that this is not what we want here ?
Comment 24 Victor Grousset/tuxayo 2022-08-22 23:26:39 UTC
(In reply to Marcel de Rooy from comment #23)
> If you enable --preload-app option, however, the code will be only loaded in the startup process and will not pick up the code changes you made. If you want to preload the app and do graceful restarts by reloading the code changes, you're recommended to use Server::Starter, configured to send QUIT signal when superdaemon received HUP ...
> [End of quote]
> While testing with about and inserting a small code change, I notice that
> while I started the about web request before the plack reload, the code
> change already is visible in the web request.
> I think that this is not what we want here ?

What do you mean? The patch doesn't use --preload-app.

I tested changing something in about.pl: once over the ~10 tries the change was reflected only by reloading the page. Otherwise I had to run `pkill starman --signal SIGHUP` to get the change.

Your plack_max_requests isn't 1 is it? That would be an explanation of what you observed.
Comment 25 David Cook 2022-08-22 23:31:18 UTC
(In reply to Marcel de Rooy from comment #23)
> If you enable --preload-app option, however, the code will be only loaded in
> the startup process and will not pick up the code changes you made. If you
> want to preload the app and do graceful restarts by reloading the code
> changes, you're recommended to use Server::Starter, configured to send QUIT
> signal when superdaemon received HUP ...
> [End of quote]

We don't use --preload-app, so we don't need to worry about it.

Even if we did use --preload-app, I think some of the text is misleading. While code changes to code that is preloaded wouldn't come through, changes to code that is lazy loaded after forking the children would still come through.

I use mod_perl for a different Perl app where I preload the core Catalyst application into the httpd master process so that slow setup like the ORM just takes place 1 time, and then all the lazy loads for function specific things happen within the child processes. 

> While testing with about and inserting a small code change, I notice that
> while I started the about web request before the plack reload, the code
> change already is visible in the web request.
> I think that this is not what we want here ?

If you want to test a small code change, you'd need to first make sure that all Starman workers have already cached the about.pl page in their CGI to PSGI cache. Once they're cached, then you can make your code change, confirm that the change isn't visible, do the reload, and then note that the change is visible.

I chose the about.pl test somewhat arbitrarily because koha-testing-docker is such a small database. In production, where you really see this being useful is in search.pl and opac-search.pl where a search might take 5+ seconds. Using koha-plack --reload will wait for that long search request to complete, and then it will restart that worker.

--

Does that answer your questions? I will be online a bit tonight (ie European morning) so might be able to answer more on IRC :)
Comment 26 Marcel de Rooy 2022-08-23 06:53:03 UTC
(In reply to Victor Grousset/tuxayo from comment #24)
> (In reply to Marcel de Rooy from comment #23)
> What do you mean? The patch doesn't use --preload-app.
I did not say that. Please read the comment. It is a quote.

> I tested changing something in about.pl: once over the ~10 tries the change
> was reflected only by reloading the page. Otherwise I had to run `pkill
> starman --signal SIGHUP` to get the change.
I had the change reflected all tries. The comment of David explains it, I guess.

> Your plack_max_requests isn't 1 is it? That would be an explanation of what
> you observed.
No it is not :)
Comment 27 Marcel de Rooy 2022-08-23 06:59:14 UTC
(In reply to David Cook from comment #25)
> We don't use --preload-app, so we don't need to worry about it.
I know that. Quoting etc.

> Even if we did use --preload-app, I think some of the text is misleading.
> While code changes to code that is preloaded wouldn't come through, changes
> to code that is lazy loaded after forking the children would still come
> through.
This explains my testing. Lazy brain :) All workers need to cache the old code first obviously. I just got the wrong worker a few times. Inspired by Victor, I will set the number of workers as low as possible and test again..

> I chose the about.pl test somewhat arbitrarily because koha-testing-docker
> is such a small database. In production, where you really see this being
> useful is in search.pl and opac-search.pl where a search might take 5+
> seconds. Using koha-plack --reload will wait for that long search request to
> complete, and then it will restart that worker.
I do not use k-t-d, but your point is clear.

Resuming QA scheduled for this afternoon.
Comment 28 David Cook 2022-08-23 07:11:24 UTC
(In reply to Marcel de Rooy from comment #27)
> > I chose the about.pl test somewhat arbitrarily because koha-testing-docker
> > is such a small database. In production, where you really see this being
> > useful is in search.pl and opac-search.pl where a search might take 5+
> > seconds. Using koha-plack --reload will wait for that long search request to
> > complete, and then it will restart that worker.
> I do not use k-t-d, but your point is clear.
> 
> Resuming QA scheduled for this afternoon.

I'll be around on and off for the next 5-7 hours, so shoot me any questions you might have. 

For what it's worth, I have a "reload_koha.sh" script I use in production all the time, especially when updating plugins, and it's wonderful hehe.
Comment 29 Marcel de Rooy 2022-08-23 11:42:30 UTC
(In reply to Marcel de Rooy from comment #27)
> > seconds. Using koha-plack --reload will wait for that long search request to
> > complete, and then it will restart that worker.

koha-plack does not wait for the request to complete but it does the job as expected: The current request gave me back the old contents, but Starman closed the old worker afterwards since a new request gives me the new contents.

Excellent.
Comment 30 Marcel de Rooy 2022-08-23 11:43:59 UTC
Created attachment 139668 [details] [review]
Bug 21366: Add koha-plack --reload to gracefully restart Plack/Starman

This patch adds a "--reload" option to koha-plack, which allows
a sysadmin to gracefully restart Koha.

This is very useful when installing Koha plugins or deploying a hot fix
where you need to change code but don't want to interrupt anyone's
usage of Koha.

0. Apply patch
1. cp /usr/sbin/koha-plack /usr/sbin/koha-plack.bak
2. cp debian/scripts/koha-plack /usr/sbin/koha-plack
3. ps -efww | grep "starman"
4. Now at roughly the same time do the following two actions:
    4a. Go to http://localhost:8081/cgi-bin/koha/about.pl
    4b. koha-plack --reload kohadev
5. Note that the web request completes successfully
6. ps -efww | grep "starman"
7. Note that the "starman master" process stays alive, but the "starman worker" processes have been restarted

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 31 Marcel de Rooy 2022-08-23 11:44:03 UTC
Created attachment 139669 [details] [review]
Bug 21366: Add --reload option to koha-plack help/manual

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 32 Tomás Cohen Arazi 2022-08-26 19:05:13 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 33 Victor Grousset/tuxayo 2022-08-28 01:12:23 UTC
(In reply to Marcel de Rooy from comment #26)
> (In reply to Victor Grousset/tuxayo from comment #24)
> > (In reply to Marcel de Rooy from comment #23)
> > What do you mean? The patch doesn't use --preload-app.
> I did not say that. Please read the comment. It is a quote.

That confusion was after reading it several time before writing the message because I wasn't sure what you meant by this quote. And the best I could come up with is that we were using --preload-app
So still confused but glad it got sorted out in this end ^o^
Comment 34 Lucas Gass 2022-10-31 17:37:25 UTC
Enhancement will not be backported to 22.05.x series