When a plugin is installed/upgraded, it would be good to send a SIGHUP to the Starman master, so that Koha will gracefully restart itself. As Kyle noted on the listserv, it is especially useful for plugins that add API routes, since the API can't use those until Starman workers are restarted. By doing a graceful restart, no active HTTP requests are interrupted.
We should be able to just run: kill('HUP',$starman_master_pid); However, the tough part is getting $starman_master_pid. As a human, I know the PID for kohadev is stored at /var/run/koha/kohadev/plack.pid but Koha itself doesn't know that. The "koha-plack" script defines the PIDFILE like so: local PIDFILE="/var/run/koha/${instancename}/plack.pid" -- I suppose another option might be to invoke "koha-plack --reload $instance_name" (after bug 21366), but that would assume that it's a Debian package installation and that we have the $instance_name, which we also don't have. -- I suppose the other thing is that this would be Starman specific. I don't know how it would work with Hypnotoad for instance. Based on https://metacpan.org/pod/Mojo::Server::Hypnotoad#MANAGER-SIGNALS it looks like it's graceful restart is done via the USER2 signal rather than the HUP signal.
*** This bug has been marked as a duplicate of bug 31074 ***
Created attachment 164587 [details] [review] Bug 30897: Gracefully restart plack after plugin install/remove
Created attachment 164588 [details] [review] Bug 30897: Gracefully restart plack after plugin install/remove
Thanks for looking into this, Martin. I had forgotten that I'd raised this issue! I'll have to mark it as Failed QA for a few reasons. 1a. If you run install_plugins.pl from the CLI, your bash session will be killed, since that's the parent process. I think that's an unintended consequence. (However, we can probably work around this by checking if we're running in a Plack environment first.) root@kohadevbox:koha$ koha-shell kohadev $ /kohadevbox/koha/misc/devel/install_plugins.pl No plugins found at /var/lib/koha/kohadev/plugins Hangup root@kohadevbox:koha$ 1b. Like 1a, this will cause problems when running unit tests: kohadev-koha@kohadevbox:koha(bug30897)$ perl t/db_dependent/Koha/Plugins/Circulation_hooks.t 1..4 ok 1 - use Koha::Plugins; ok 2 - use Koha::Plugins::Handler; ok 3 - use Koha::Plugin::Test; # Subtest: after_circ_action() hook tests 1..3 Hangup root@kohadevbox:koha$ 2. I think we should wrap this with some configuration. Restarting the Starman on plugin installation is a significant change, so I think we'll want to be able to quickly and easily change between behaviours - at least in the short-term. I think we'll also probably want to default to off at least initially just to be on the safe side. 3. This restart runs even if there are no changes to make to the plugins, which would lead to unnecessary restarts. I think we'd want to be more careful in initiating the restarts. -- Additional questions/comments: 1. Could you speak to why you need to look for the parent pid recursively? I don't see the reason for it. As an aside, do we want to use POSIX::getpid instead of $$ for getting the pid? I suppose POSIX::getpid is easier to read. 2. Relating to my first failure point, do you think perhaps we should interrogate the process we're looking to restart just to be extra careful? At a minimum, I think we'd want to double-check that it is really Starman. I wanted to say we shouldn't try to "kill" pid 1, but I suppose if someone has managed to get Starman running in Docker as the only process, then it could have a pid of 1, and that would be OK...
Created attachment 164654 [details] [review] Bug 30897: Limit to plack environments I had a feeling we'd want to do this when I submitted the proof of concept, though I hadn't appreciated the wider issues that have now been highlighted.. it makes a lot of sense to limit to the plack scope.
Thanks for your review David :). I had a feeling we'd want to wrap it to check we were running as plack but then totally forgot to add it.. follow-up added. Though we might also want to somehow try to let this happen for background workers and I'm not entirely sure how to identify those yet (I think we can probably handle that on another bug). As for the recursive lookup, I must put my hands up here and say I followed some google example from somewhere.. we may well only need to get the single parent.. I can't see that we allow children to spawn more children anywhere here in plack so we're probably safe to reduce that code.
As for $$ vs POSIX... I honestly had just forgotten that $$/$PID/$PROCESS_ID existed.. having said that I feel like the POSIX import makes it very clear what we're doing... I could see devs not immediately understanding the use of $$.
Do you have a method in mind for the double check we're starman/plack you mention? I'm struggling to think of a reliable one
(In reply to Martin Renvoize from comment #9) > Do you have a method in mind for the double check we're starman/plack you > mention? > > I'm struggling to think of a reliable one I was hoping that we could check $0 but it looks like one of the Plack components possibly relating to Plack::App::CGIBin changes it to the CGI script being emulated, so that won't work. I think we'll probably just have to check the environment in that case. C4::Context->psgi_env() will probably be good enough. Another thought about this one... rather than putting it into the InstallPlugins(), we could have a different method for resetting the parent process, and just invoke it in the web controller. That would be safer.
(In reply to Martin Renvoize from comment #8) > As for $$ vs POSIX... I honestly had just forgotten that $$/$PID/$PROCESS_ID > existed.. having said that I feel like the POSIX import makes it very clear > what we're doing... I could see devs not immediately understanding the use > of $$. Yeah, I think so too. While Perl shorthand can be handy, I do like clarity more.
(In reply to Martin Renvoize from comment #7) > I had a feeling we'd want to wrap it to check we were running as plack but > then totally forgot to add it.. follow-up added. Though we might also want > to somehow try to let this happen for background workers and I'm not > entirely sure how to identify those yet (I think we can probably handle that > on another bug). Mmm that's a good point. I bumped into that issue myself recently. I can't remember what it was exactly but yeah the background workers didn't get the update because I didn't think to restart them... > As for the recursive lookup, I must put my hands up here and say I followed > some google example from somewhere.. we may well only need to get the single > parent.. I can't see that we allow children to spawn more children anywhere > here in plack so we're probably safe to reduce that code. Cool I agree
(In reply to David Cook from comment #10) > I think we'll probably just have to check the environment in that case. > C4::Context->psgi_env() will probably be good enough. I just noticed that you already did this in your new patch heh.
Created attachment 164655 [details] [review] Bug 30897: Gracefully restart plack after plugin install/remove == test plan == 1. list your worker processes with something like htop 2. install a plugin https://github.com/bywatersolutions/dev-koha-plugin-kitchen-sink/releases 3. note that the PIDs of the workers stay the same and no sign of restart (no PID changes and no CPU activity that the workers would have if they were starting) 4. apply patches and restart services 5. unistall plugin 6. notice worker restart 7. install plugin 8. notice worker restart 9. downgrade plugin 10. notice worker restart 11. upgrade plugin 12. notice worker restart 13. disable and enable plugin 14. *no restart* Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Oops, mid air collision. Thanks David for finding these issue. So I removed my signoff line. And kept the test plan that I added in the commit message.
Created attachment 164657 [details] [review] Bug 30897: Gracefully restart plack after plugin install/remove == test plan == 1. list your worker processes with something like htop 2. install a plugin https://github.com/bywatersolutions/dev-koha-plugin-kitchen-sink/releases 3. note that the PIDs of the workers stay the same and no sign of restart (no PID changes and no CPU activity that the workers would have if they were starting) 4. apply patches and restart services 5. unistall plugin 6. notice worker restart 7. install plugin 8. notice worker restart 9. downgrade plugin 10. notice worker restart 11. upgrade plugin 12. notice worker restart 13. disable and enable plugin 14. *no restart*
Created attachment 164658 [details] [review] Bug 30897: Limit to plack environments I had a feeling we'd want to do this when I submitted the proof of concept, though I hadn't appreciated the wider issues that have now been highlighted.. it makes a lot of sense to limit to the plack scope.
(In reply to Martin Renvoize from comment #7) > Though we might also want > to somehow try to let this happen for background workers and I'm not > entirely sure how to identify those yet (I think we can probably handle that > on another bug). What kind of issues would a background worker have ? Adding API routes: N/A for background workers. But I suspect that bug 31074 comment 0 would have implication to background workers. +1 for as follow-up ticket.
(In reply to Martin Renvoize from comment #7) > As for the recursive lookup, I must put my hands up here and say I followed > some google example from somewhere.. we may well only need to get the single > parent.. I can't see that we allow children to spawn more children anywhere > here in plack so we're probably safe to reduce that code. Ok, will keep an eye when it's ready to be tested again.
Created attachment 164660 [details] [review] Bug 30897: Gracefully restart plack after plugin install/remove This patch adds a graceful plack restart to the plugin install/uninstall routine to fully allow plugin install to work via the staff client. Test plan 1. list your worker processes with something like htop 2. install a plugin https://github.com/bywatersolutions/dev-koha-plugin-kitchen-sink/releases 3. note that the PIDs of the workers stay the same and no sign of restart (no PID changes and no CPU activity that the workers would have if they were starting) 4. apply patches and restart services 5. unistall plugin 6. notice worker restart 7. install plugin 8. notice worker restart 9. downgrade plugin 10. notice worker restart 11. upgrade plugin 12. notice worker restart 13. disable and enable plugin 14. *no restart* Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 164661 [details] [review] Bug 30897: Limit to plack environments I had a feeling we'd want to do this when I submitted the proof of concept, though I hadn't appreciated the wider issues that have now been highlighted.. it makes a lot of sense to limit to the plack scope.
Created attachment 164662 [details] [review] Bug 30897: Add option to disable automated restart This patch adds the ability to disable the automated plack restart we introduce with this patchset via configuration.
Created attachment 164663 [details] [review] Bug 30897: Remove recursive parent identification loop We believe that plack will only spawn from one parent and children cannot spawn further child processes.. as such we don't need to walk up the tree.
Whilst it's tempting to squash some of these I think the commit notes and history are useful to future devs here. I think I've resolved all the worries here and set the restart to default disabled (I'd love to default to on but understand the concerns with that)
*** Bug 31074 has been marked as a duplicate of this bug. ***
Created attachment 164735 [details] [review] Bug 30897: Gracefully restart plack after plugin install/remove This patch adds a graceful plack restart to the plugin install/uninstall routine to fully allow plugin install to work via the staff client. Test plan 1. list your worker processes with something like htop 2. install a plugin https://github.com/bywatersolutions/dev-koha-plugin-kitchen-sink/releases 3. note that the PIDs of the workers stay the same and no sign of restart (no PID changes and no CPU activity that the workers would have if they were starting) 4. apply patches and restart services 5. unistall plugin 6. notice worker restart 7. install plugin 8. notice worker restart 9. downgrade plugin 10. notice worker restart 11. upgrade plugin 12. notice worker restart 13. disable and enable plugin 14. *no restart* Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 164736 [details] [review] Bug 30897: Limit to plack environments I had a feeling we'd want to do this when I submitted the proof of concept, though I hadn't appreciated the wider issues that have now been highlighted.. it makes a lot of sense to limit to the plack scope. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 164737 [details] [review] Bug 30897: Add option to disable automated restart This patch adds the ability to disable the automated plack restart we introduce with this patchset via configuration. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 164738 [details] [review] Bug 30897: Remove recursive parent identification loop We believe that plack will only spawn from one parent and children cannot spawn further child processes.. as such we don't need to walk up the tree. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
This works well, starts out disabled, nice to see API routes show quickly, excellent!
Created attachment 164758 [details] [review] Bug 30897: (QA follow-up) Enable graceful restart by default This patch enabled the restart by default. After a poll at hackfest24 we opted to enable this by default and the RM requested I add the patch to the bug so we don't forget ;)
Pushed for 24.05! Well done everyone, thank you!
Comment on attachment 164737 [details] [review] Bug 30897: Add option to disable automated restart Review of attachment 164737 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/en/modules/plugins/plugins-upload.tt @@ +45,5 @@ > </div> > [% END %] > + [% IF ( !plugins_restart ) %] > + <div class="dialog alert"> > + <strong>You're system is not configured to automatically refresh on plugin upload, you may need to ask your administrator to complete the plugin installation.</strong> I notice the patch is spelled "You're system" but the patch in master is spelled correctly as "Your system". We've still got a comma splice here, so that comma should be replaced with either a full stop or a semicolon.
Also, one final concern: For DB version 19.06.00.006, we run "Koha::Plugins->new({ enable_plugins => 1 })->InstallPlugins;" in installer/data/mysql/updatedatabase.pl In theory, if the database were being upgraded via the web installer, this would cause the database upgrade to crash I think? I might just test that quickly...
(In reply to David Cook from comment #34) > Also, one final concern: > > I might just test that quickly... So the Starman worker launches a subprocess like the following: sh -c /kohadevbox/koha/installer/data/mysql/updatedatabase.pl >> /var/log/koha/kohadev/updatedatabase_2024-04-12T02:34:37_19.0600004_23.1200017.log 2>> /var/log/koha/kohadev/updatedatabase- error_2024-04-12T02:34:37_19.0600004_23.1200017.log So it's this shell process that gets HUPed, and then init takes over as the parent process: kohadev-koha@kohadevbox:koha(bug_30897_1)$ ps -fww -p 15743 UID PID PPID C STIME TTY TIME CMD kohadev+ 15743 1 13 02:32 ? 00:00:01 /usr/bin/perl /kohadevbox/koha/installer/data/mysql/updatedatabase.pl -- I think this is another unintended consequence, but... it probably won't cause any harm overall, and it would only affect people upgrading from <= 19.05 to >= 24.05. (Note that the upgrade crashed for me at 19.0600011 but that was for unrelated reasons.) -- Note that this is one of the reasons why I think the Starman restart should've been in a separate function to InstallPlugins(). While the psgi detection helped, it's not quite enough. In this case, the environmental variables are inherited from the web process by the shell process.
(In reply to Martin Renvoize from comment #31) > After a poll at hackfest24 we opted to enable this by default and the RM > requested I add the patch to the bug so we don't forget ;) +1 it's better to have good default for libraries without a sysadmin or support provider (the more likely to use the plugin upload UI without knowing about the shortcomings) to have correct behaving plugin management.
Nice find David about the installer, we are lucky it's not that bad. --- Thanks all for managing to squash this issue! :D
> After a poll at hackfest24 we opted to enable this by default Release notes updated. It's for new installs, right? IIUC the absence of <plugins_restart> is the same as it being = 0
Correct and correct, thanks Victor
Not backported to 23.11.x
This feature seems to break searching to install plugins, nothing happens when we try to search for a plugin and click "install"
(In reply to Liz Rea from comment #41) > This feature seems to break searching to install plugins, nothing happens > when we try to search for a plugin and click "install" Looks like it's actually another CSRF regression.. I'll open a new bug and put the fix there.
See bug 37916