== test plan == 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. restart services 7. The Minimum Koha version column is correct now
Created attachment 142517 [details] [review] 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.
Created attachment 143445 [details] [review] 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.
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.
Created attachment 143668 [details] [review] 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.
Patch is updated and working now, sorry about the fluke.
Created attachment 143730 [details] [review] 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. Signed-off-by: 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.
I was surprised to find Module::Refresh in koha-testing-docker, but "apt-cache rdepends --installed libmodule-refresh-perl" shows that it's a dependency of libplack-perl. That's because Plack::Middleware::Refresh comes bundled with Plack: https://metacpan.org/pod/Plack::Middleware::Refresh Reading Plack::Middleware::Refresh documentation, it seems like this middleware is typically only used during development and not production... https://metacpan.org/pod/Module::Refresh also mentions bugs relating to global variables... But then the practice seems normal in an O'Reilly mod_perl book: https://docstore.mik.ua/orelly/weblinux2/modperl/ch06_08.htm
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.
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.
> Not sure we should even fix it ;) are you trolling? If Olivier fixed it, it's because it caused us sufficient headaches during (Koha) upgrades to open/look for a Bz. Our threshold is pretty high in that regard, nowadays.... I understand the discussion is threading toward the old "you can't call the garbage collector from the code", that's valid. But then the bug must be fixed some other way.
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?
(In reply to Blou from comment #11) > > Not sure we should even fix it ;) > > are you trolling? If Olivier fixed it, it's because it caused us sufficient > headaches during (Koha) upgrades to open/look for a Bz. Our threshold is > pretty high in that regard, nowadays.... Sort of trolling yes, dont feed the troll ;) Does not the upgrade script refresh plack + memcached ? 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). I know PHP with persistence (PHP-FPM) manages that by knowing changes in sources, I dont know how.
(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.
I'm getting an idea... Every time we want to use plugins, we check if the plugin is already loaded. If it's already loaded, we check the version/timestamp of the loaded plugin against the version/timestamp in the database. If they differ, then you re-load the plugin. Note that this has to happen in every process. We're already doing some database calls for plugins. Adding more is suboptimal but maybe we could optimize things a bit. (At install/upgrade time, we could set the data in the L2 cache, or we could try to do a more complex SQL query to get more data with only 1 DB call.) If we're going to try to dynamically reload plugin modules, it needs to be done consistently.
I'm reactivating this as I would like to have it solves once and for all. Note that I was unable to recreate the potential issue with only one worker seeing the changes once Module::Refresh does its job. 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.
*** Bug 30897 has been marked as a duplicate of this bug. ***
(In reply to Olivier Hubert from comment #16) > I'm reactivating this as I would like to have it solves once and for all. I'll take another look now, as it would be good to get something done for this.
(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.
Created attachment 152463 [details] [DO NOT PUSH] - Plugin for testing Just increment the VERSION number in the main file so that you can test whether or not the Starman process has updated the plugin code.
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/Plugin/Com/ByWaterSolutions/CoverFlow/lib/perl5
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.
I had a look to see if there was any obvious way to get the instance name or track up to find the parent PID so we could call the HUP call mentioned above.. but I failed so far. 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.
(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.
(In reply to Martin Renvoize from comment #23) > I had a look to see if there was any obvious way to get the instance name or > track up to find the parent PID so we could call the HUP call mentioned > above.. but I failed so far. Is a SIGHUP on all starmans running on a large shared hosting server a paralyzing load? On the occasional plugin install/upgrade/uninstall. Or should it be okay and worth it for correctness sake until something else is found?
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.
(In reply to Victor Grousset/tuxayo from comment #25) > (In reply to Martin Renvoize from comment #23) > > I had a look to see if there was any obvious way to get the instance name or > > track up to find the parent PID so we could call the HUP call mentioned > > above.. but I failed so far. > > Is a SIGHUP on all starmans running on a large shared hosting server a > paralyzing load? On the occasional plugin install/upgrade/uninstall. If you ran ./misc/devel/install_plugins.pl against all instances, it would be a paralyzing load on a large shared hosting server, yes. 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.
(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?
(In reply to Victor Grousset/tuxayo from comment #28) > :( Too bad. Thanks for the real world knowledge. I was hopping the actual > compile would happen on the 1st request. The "plack.psgi" file gets processed each time a worker process is started. If you restart/reload Starman for a large number of instances, you'll end up with a lot of processes trying to use 100% CPU all at the same time (as the API is slow/intensive to launch). > 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) 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. (I have a mod_perl app where I preload my app into the master process, and it makes the app workers start and run very fast.) I think we make quite a few assumptions in the Koha code too which ties each Starman worker to its particular instance. Bug 20630 looks at doing multitenancy with Mojolicious but it stalled a while ago. I think a multi-tenanted Koha would be great, although there is a danger of cross-contamination. You have to manage variable scope and caches very carefully. > 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? 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. 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.
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).
So there's another problem with the concept of automatically reloading Starman on install/upgrade/uninstall. The "background_tasks" hook is run in background_jobs_worker.pl which also loads plugin code. When installing/upgrading/uninstalling a plugin with this hook, you need to restart Starman *and* all affected background job workers. I suppose my idea 1 from https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31074#c19 might be able to take care of that scenario, but it would still be complicated.
(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
(In reply to Victor Grousset/tuxayo from comment #32) > In that case of multi-tenancy with shared workers, allowing plugin > management via UI would be a no anyway, right? Everything would be configuration driven, so you could still have people upload plugins, and Koha could put them in the right place. (Shared workers would all belong to the same Linux system user of course, which would reduce the Linux level auth separation between Koha instances.) > "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 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. (Some day when I have more time I'm going to look again at reducing CPU consumption at startup time for the Starman workers...)
(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
(In reply to Victor Grousset/tuxayo from comment #34) > (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. Ah yes I wasn't thinking clearly. With a multitenant Wordpress, I think the only person who can upload plugins is the Wordpress administrator, so we'd have to do something similar. Which works well for how I like to manage things anyway. > > 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) 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. So then a reload at plugin upload time is determined by the sysadmin anyway, which makes it similar to any software deployment. > > (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 That's the one. It would be very helpful if we used it with Koha, but it's something that I just haven't gotten around to doing yet. (I think my Koha TODO list is nearly infinite.)
(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)
(In reply to Victor Grousset/tuxayo from comment #36) > 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) I would hope that one wouldn't allow plugin upload for a multi-tenant case, since tenant A could upload a malicious plugin that harms tenants B, C, D, etc.
Are tenants not isolated already? Plugins, by default, go into /var/lib/Koha/site/... and run under the instance user.
(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.
(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.
(In reply to Victor Grousset/tuxayo from comment #40) > 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. If they are, then they're using customized code. -- 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.
(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.
(In reply to Martin Renvoize from comment #26) > 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. Yep, it works. `getppid()` got me the PID of the master process. (check from the container if you use one. I didn't know between a docker container and the host, the PIDs would change)
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.
(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.)
PS I don't mean to be a buzz kill and I very much appreciate your contributions, Martin! :D <3
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 ***