https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31074 Olivier Hubert <olivier.hubert@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |olivier.hubert@inlibro.com |ity.org | Status|NEW |Needs Signoff CC| |olivier.hubert@inlibro.com -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31074 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA CC| |david@davidnind.com --- Comment #3 from David Nind <david@davidnind.com> --- I'm getting an error trace after applying the patch and restarting everything (flush_memcached and restart_all, using koha-testing-docker)(step 7): try() encountered an unexpected argument (undef) - perhaps a missing semi-colon before or at /kohadevbox/koha/Koha/Plugins.pm line 251. at /usr/lib/x86_64-linux-gnu/perl-base/Carp.pm line 289 .... Testing with the ByWater Solutions kitchen sink plugin. Before applying the patch, I can install plugins without any problems. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31074 Olivier Hubert <olivier.hubert@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #143445|0 |1 is obsolete| | --- Comment #4 from Olivier Hubert <olivier.hubert@inlibro.com> --- Created attachment 143668 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=143668&action=edit Bug 31074: Reload a plugin's module before installation When using Plack, memcached or another caching mechanism, a plugin's old version is retained in the cache, even when uninstalled. If the plugin is then reinstalled, the old version is used, no matter the version installed. To prevent this, the plugin's module is reloaded every time it is installed. To test: 1) Install a plugin, like the kitchen sink. https://github.com/bywatersolutions/dev-koha-plugin-kitchen-sink/releases 2) Modify the .kpz and in KitchenSink.pm, change MINIMUN_VERSION. 3) Uninstall the plugin. 4) Install the new version. 5) The Minimum Koha version column still lists the old version. 6) Apply patch. 7) Repeat steps 1 through 4. 8) Observe that the Minimum Koha version column lists the correct version. 9) Sign off. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31074 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31074 --- Comment #7 from David Nind <david@davidnind.com> --- (In reply to Olivier Hubert from comment #5)

Patch is updated and working now, sorry about the fluke.

Thanks! Things are now working as per the test plan. Testing notes (using koha-testing-docker): - I used the ByWater Solutions kitchen sink plugin - for the changed version, I changed the minimum version from 21.05 to 20.05. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31074 --- Comment #9 from David Cook <dcook@prosentient.com.au> --- I haven't tested this yet, but I see one potential problem. This refresh code runs in only 1 of the Starman workers. That worker might refresh the plugin code, but it shouldn't affect the other Starman workers. The way to test this would probably be to increase the number of "plack_workers" in koha-conf.xml and try re-loading the page again and again. So far, we might be noticing a coincidence where the refreshed worker is the one serving the plugin page. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31074 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |fridolin.somers@biblibre.co | |m --- Comment #10 from Fridolin Somers <fridolin.somers@biblibre.com> --- Oula indeed, not sure it is a good idea for a perl code to ask for plack refresh. It is well known that plack persistence can show outdated content. Not sure we should even fix it ;) Setting to Failed QA to gather more opinions. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31074 --- Comment #12 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- I don't think Frido was trolling, but adding his 2 cents to the discussion. I agree that we need to make the process for updating/installing/removing plugins more seamless somehow. If the point is that the user should be able to do these actions from the GUI, we need a way to make Plack aware of changes, because right now we expect them to do a server side action, without even telling them to. This is prominent when you install a new version of a plugin: the version shown on the plugin page will not increase, although the plugin was installed correctly, until Plack is restarted. This is very confusing. If the approach taken here, as David noted in comment#10, maybe we could find a way to improve the patch or find another way? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31074 --- Comment #14 from David Cook <dcook@prosentient.com.au> --- (In reply to Fridolin Somers from comment #13)

I must say we at Biblibre do not allow plugins install via UI so we install via script with a refresh of memcached then plack (this order is important).

For many years, I've wanted to disable the plugin install via UI and only handle it via CLI script. Glad to know I'm not the only one! I haven't disabled the UI, but in practice I typically manage plugins for libraries, so I'll upload via the UI, refresh memcached, and then do a graceful reload of Plack.

I know PHP with persistence (PHP-FPM) manages that by knowing changes in sources, I dont know how.

PHP natively uses OpCache which re-validates cached scripts based on file modification timestamps. OpCache also has functions for dealing with its cached scripts as well. It looks like it uses shared memory for the cached code, which I suppose is how it handles the child process scenario... Perl doesn't have a mechanism like it. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31074 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Plugin metadata is kept in |Cached plugin code is used |cache after uninstalling a |in Koha even after changes |plugin |to plugin | |(install/upgrade/uninstall) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31074 --- Comment #17 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- *** Bug 30897 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31074 --- Comment #19 from David Cook <dcook@prosentient.com.au> --- (In reply to Olivier Hubert from comment #16)

Note that I was unable to recreate the potential issue with only one worker seeing the changes once Module::Refresh does its job.

I just reproduced the problem on my first try with a standard koha-testing-docker setup with 2 starman workers: [2023/06/19 06:17:49] [WARN] It's a good thing you double-checked this plugin before installing it. 0.0.4 28969 at /var/lib/koha/kohadev/plugins/Koha/Plugin/Prosentient/NoisyPlugin.pm line 48. [2023/06/19 06:17:49] [WARN] You did check this plugin before installing it, right? 0.0.4 28969 at /var/lib/koha/kohadev/plugins/Koha/Plugin/Prosentient/NoisyPlugin.pm line 50. [2023/06/19 06:17:53] [WARN] It's a good thing you double-checked this plugin before installing it. 0.0.3 at /var/lib/koha/kohadev/plugins/Koha/Plugin/Prosentient/NoisyPlugin.pm line 48. [2023/06/19 06:17:53] [WARN] You did check this plugin before installing it, right? 0.0.3 at /var/lib/koha/kohadev/plugins/Koha/Plugin/Prosentient/NoisyPlugin.pm line 50. [2023/06/19 06:18:11] [WARN] It's a good thing you double-checked this plugin before installing it. 0.0.4 28969 at /var/lib/koha/kohadev/plugins/Koha/Plugin/Prosentient/NoisyPlugin.pm line 48. [2023/06/19 06:18:11] [WARN] You did check this plugin before installing it, right? 0.0.4 28969 at /var/lib/koha/kohadev/plugins/Koha/Plugin/Prosentient/NoisyPlugin.pm line 50. [2023/06/19 06:18:13] [WARN] It's a good thing you double-checked this plugin before installing it. 0.0.3 at /var/lib/koha/kohadev/plugins/Koha/Plugin/Prosentient/NoisyPlugin.pm line 48. [2023/06/19 06:18:13] [WARN] You did check this plugin before installing it, right? 0.0.3 at /var/lib/koha/kohadev/plugins/Koha/Plugin/Prosentient/NoisyPlugin.pm line 50. [2023/06/19 06:18:15] [WARN] It's a good thing you double-checked this plugin before installing it. 0.0.4 28969 at /var/lib/koha/kohadev/plugins/Koha/Plugin/Prosentient/NoisyPlugin.pm line 48. [2023/06/19 06:18:15] [WARN] You did check this plugin before installing it, right? 0.0.4 28969 at /var/lib/koha/kohadev/plugins/Koha/Plugin/Prosentient/NoisyPlugin.pm line 50. The lines with 0.0.4 28969 are the one that was refreshed using the Module::Refresh patch, and the lines with 0.0.3 are the 2nd Plack worker which didn't get refreshed, since Module::Refresh didn't run in that process.

Moving forward, if forcing a Plack refresh is not the way to go then I am unsure of the best approach. It seems to me like reading the plugin's main file on every use defeats the purpose of using Plack. Which is why I focused on doing it only when installing / upgrading.

There are ways... 1. In the "plugin_data" table, we have __INSTALLED_VERSION__. During get_enabled_plugins() and GetPlugins() and Koha::Plugins::Handler->run, and probably other places..., we can compare $class->{metadata}->{version} against __INSTALLED_VERSION__ (note this is a class lookup and not an object lookup, so it would be called before calling the new() constructor to create an object). You could fetch all values for the key __INSTALLED_VERSION__ in 1 database call to reduce overhead. (As for reading the plugin's main file on every use - the file is already loaded into the process's memory. So the plugin's coded information is already available.) 2. A Debian package friendly solution could be running "kill('HUP',$starman_master_pid);" after installing/upgrading a plugin. I talk about this at https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30897#c1 It's a bit of a nuclear option but it would be the cleanest. 3. We could do a Plack middleware, although that would run for every single web request. That's probably not necessary. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31074 --- Comment #21 from David Cook <dcook@prosentient.com.au> --- Actually, another problem with this whole proposal is that it only takes into account the main plugin .pm file. A plugin can contain many Perl modules, which also need to be updated. Often, these additional modules will be in the same plugin namespace, but they don't have to be. Consider Bywater's Coverflow plugin: https://github.com/bywatersolutions/koha-plugin-coverflow/tree/master/Koha/P... -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31074 --- Comment #22 from David Cook <dcook@prosentient.com.au> --- While Plack::Middleware::Refresh wouldn't work for bug 31729, it could potentially cover the plugin scenario, since we don't use .pl scripts in plugins. But it is designed for development and not for production. That said, since it's a Middleware, it's easy for people to inject it into their own .psgi files without it being an out-of-the-box configuration. Or maybe it could be an out-of-the-box configuration and people with backend support could remove it. I'm still thinking about disabling the web upload of plugins and just managing it via the CLI. But I know there's a lot of people in different Koha support situations. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31074 --- Comment #24 from David Cook <dcook@prosentient.com.au> --- (In reply to Martin Renvoize from comment #23)

I'm considering coming back to option 1 in David's suggestions... do you think you're likely to have any time to work through this early next cycle David? I'm a little out of my depth, but would love to see this moving forward.

Probably not :/. I've away a lot in January and then I'll have a lot of work to catch up on in Feb and March. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31074 --- Comment #26 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Adding this here for reference so I don't forget.. we might be able to use https://perldoc.perl.org/functions/getppid to find the Starman master pid.. I'm going to run a few tests using that technique. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31074 --- Comment #28 from Victor Grousset/tuxayo <victor@tuxayo.net> --- (In reply to David Cook from comment #27)

If you ran ./misc/devel/install_plugins.pl against all instances, it would be a paralyzing load on a large shared hosting server, yes.

:( Too bad. Thanks for the real world knowledge. I was hopping the actual compile would happen on the 1st request. Also, are there cases of a single main starman for all workers of a large shared hosting? Or would that be a bad setup and we don't have the duty to worry about instances that went out of their way to achieve this? And for such a setup, instance wide reload might not be a paralyzing load because *hopefully?* when spawning many workers at the same time from the *same* main starman process, it might not compile 100 times Koha's minimal base code but only once and they fork 100 time almost for free. (the last is suppositions, if not waiting for requests to do the compiling, then it would make sense to benefit for mutualising compiling work)

If it was just on a web upload or a single install for ./misc/devel/install_plugins.pl then it should be fine since it would just be restarting 1 starman master process.

Certainly something to be careful with...

I'm a bit apprehensive about giving a web user the ability to restart Starman.

It still happens on it's own every 100 queries by default (50 X the number of workers, 2 by default) on most Koha's right? And can be triggered by someone knowing that it existing like as a way to mitigate this very issue. install/upgrade/uninstall a plugin and spam F5 on a light page to make the operation effective ^^" What bad stuff could happen if an end user (managing plugins) can trigger a worker reload? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31074 --- Comment #30 from David Cook <dcook@prosentient.com.au> --- Personally, I've moved to only allowing plugins installed via the CLI, which makes managing this issue fairly trivial. I'll install a plugin and start the necessary processes for the necessary instance(s). -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31074 --- Comment #32 from Victor Grousset/tuxayo <victor@tuxayo.net> --- (In reply to David Cook from comment #29)

We'd need to "preload" the base code, which we don't currently do in Koha, in order to get that benefit of compiling once and then forking many times for a cheaper cost.

Thanks for all the info :)

...

What bad stuff could happen if an end user (managing plugins) can trigger a worker reload?

It would make for a very very easy denial of service, but it's true that someone can still do a DOS by just sending a high volume of high impact requests anyway.

Or by sending a high impact sabotaging plugin ^^" So it doesn't seem like an additional concern.

This is another case where multi-tenancy would be a problem. If one end user could trigger a worker reload on a multi-tenanted server, it would cut out for everyone. At least in this case it's isolated to their own Koha instance.

In that case of multi-tenancy with shared workers, allowing plugin management via UI would be a no anyway, right? "it would cut out for everyone" => just a few seconds of wait while the system restarts many workers but other than that it's said to be graceful so no aborted request processing IIUC -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31074 --- Comment #34 from Victor Grousset/tuxayo <victor@tuxayo.net> --- (In reply to David Cook from comment #33)

Everything would be configuration driven, so you could still have people upload plugins, and Koha could put them in the right place.

If the workers are shared, then all plugins also, right? So before triggering a too wide reload the issue would be simply letting tenants installing plugins because that would affect the other. ---

Yes, it should be graceful, so no aborted request processing, but the length of the wait is variable.

For instance, if you have 8 CPUs and 400 workers, and each worker needs 100% CPU for a few seconds at startup then you're going to have a long wait before the full system is operational.

The ratio between threads and workers should never be like this, right? Because the point of workers is to have parallel request processing when having multiple threads. So 8 CPUs could never process 400 queries at the same time. (even if a lot of time is DB wait) So the practical number of workers would be much lower. And that also caps the reload time. Like a few seconds a most? Beyond means not having enough threads to be able to use that much workers anyway. All this is to be sure whether or not the possibility of triggering a reload is viable or not. (option 2 of comment 19) ---

(Some day when I have more time I'm going to look again at reducing CPU consumption at startup time for the Starman workers...)

Just in case that might help, you mentioned preloading earlier, is that --preload-app ? https://metacpan.org/dist/Starman/view/script/starman#-preload-app -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31074 --- Comment #36 from Victor Grousset/tuxayo <victor@tuxayo.net> --- (In reply to David Cook from comment #35)

8 CPUs and 400 workers is an extreme example.

If you're multi-tenanted, you'll have fewer workers. But if you're not multi-tenanted, you could easily have hundreds of workers even with a small number of CPUs, if many of the instances don't carry much load.

Philippe has an example with 100-200 Koha instances I believe. They're mostly going to be idle, but you'd need 200-400 workers just to have a minimum amount of availability.

If you were multi-tenanted, you would have a more reasonable number of workers. But then as per the above you wouldn't allow Koha instances to upload plugins anyway.

Ok since the multi-tenant case isn't a problem here, one Koha instance will always have a reasonable number of workers vs the number of threads. So a Koha SIGHUPing it's starman master process shouldn't cause enough trouble to make option 2 of comment 19 non-viable. (IIUC) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31074 --- Comment #38 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Are tenants not isolated already? Plugins, by default, go into /var/lib/Koha/site/... and run under the instance user. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31074 --- Comment #40 from Victor Grousset/tuxayo <victor@tuxayo.net> --- (In reply to David Cook from comment #39)

(In reply to Martin Renvoize from comment #38)

...

Are tenants not isolated already? Plugins, by default, go into /var/lib/Koha/site/... and run under the instance user.

Yeah, currently they're separated. Maybe I'm misunderstanding what Victor was saying. I thought he was talking about a scenario with plugins in a bug 20630 world where they wouldn't be separated.

I didn't know that bug 20630 would be necessary. I though it was already done in the wild by some providers tweaking their setup to manage to have multiple instances sharing the same workers. Actually even in that case, their number of workers would still be reasonable vs their number of CPU threads. So there wouldn't even be a blocker here. That, in addition to the earlier point of plugins being (unavoidably? no matter how Koha evolves?) shared if workers are shared so it's nonviable to allow tenants to manage them. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31074 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |normal --- Comment #42 from Victor Grousset/tuxayo <victor@tuxayo.net> --- (In reply to David Cook from comment #41)

Overall, if folk want to keep going down this path, then by all means they should. All I would ask is that they have a configuration option for it in koha-conf.xml which defaults to off.

No correctness by default when managing plugins from the UI? (and oops, I created the ticket as enhancement when it's in fact about fixing bugs) And before the default value, why needing a way to disable it? What's the remaining issues? If an instance has plugins UI enabled, then in which case is that a problem that on plugin install/upgrade/uninstall, a reload is triggered? It seems and in every case, even in unsupported ones, there will be always a reasonable number of workers linked to an intance vs the number of threads so reload time shouldn't be too high. Should the plugin UI warn that install/upgrade/uninstall operation shouldn't be done on peak load. Because yes, that's the times even reasonable request stalls are not a good idea. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31074 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=23303 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31074 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |koha@trust-box.at -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31074 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=30897 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31074 --- Comment #45 from David Cook <dcook@prosentient.com.au> --- (In reply to Martin Renvoize from comment #44)

I've added an alternative approach to bug 30897 that uses the proposed getppid and kill approach.

It's not tested yet, but hopefully can get this one moving again.

Martin, I think you've gone with a pretty good approach. I did find some problems with it, but they should be fairly easily fixed I think. Personally, I'd probably never want to automatically restart Starman, but I think it would be a very useful feature for a lot of people, and really essential for people with minimal sysadmin support. (We might find that we add this to the Identity Provider section as well, since that requires Koha restarts in some cases.) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31074 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |RESOLVED Resolution|--- |DUPLICATE --- Comment #47 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Thanks for taking the time to look at this David, I really appreciate the feedback on the other bug. I think we can reverse the duplicate at this point and mark this one resolved as Duplicate as the testing here at Hackfest and your feedback seem to be pointing to that one being the preferred approach. *** This bug has been marked as a duplicate of bug 30897 *** -- You are receiving this mail because: You are watching all bug changes.