Created attachment 74671 [details] [review]
Bug 20630: An attempt at multitenancy with Mojolicious

This patch tries to achieve the same goal as bug 15562, but with
Mojolicious and with minimal changes.
It does so by accepting a X-Koha-Conf header containing the path to an
existing koha-conf.xml

Test plan:
1. Apply bug 20582
2. Apply this patch
3. Use etc/nginx.conf as an example and create two server blocks. Make
   sure you have different paths for X-Koha-Conf and the files differ at
   least for the database name (and memcached namespace if memcached is
   enabled)
4. Run `env KOHA_CONF=a-third-koha-conf.xml morbo bin/koha`
5. Try to access the first server block, change some visible sysprefs
   like IntranetUserCss and OpacUserCss
6. Do the same with the second server block, and again with
   http://yourserverip:3000/
7. Confirm that everything is working normally for each instance
8. Test with and without memcached
9. Restart morbo without KOHA_CONF (env KOHA_CONF= morbo bin/koha) and
   confirm that the two first sites still work

Thanks for this, Julian!

I'm trying to think of other places where Koha would leak data other than system preferences/caches...

Created attachment 76218 [details] [review]
bug 20630: Various improvements

Thank you for this patch. I've done some testing with it and it appears to work great. Compared to Plack, this method is much more efficient and less memory-hungry when running multiple instances. I'm currently using it to run 20 instances on a VM with 4GB of RAM, and will soon test with 120 instances on the same host. I'll be sure to report any further issues and results here.

In the mean time, I'd like to share a patch which applies on top of the initial one, improving it in the following ways:

* Use absolute paths instead of relative paths to allow deployment in the context of (real-world) package-based installations
* Read a configuration file for the Mojo app
* Provide a systemd service for running the app using the optimized hypnotoad server
* Provide a catch-all route for general 404 errors
* Add headers to instruct clients to cache static resources

I'm absolutely new to Mojolicious deployments so there might be better ways to achieve these goals but I thought it would probably be better to share this than let it soak in my local repository.

(In reply to Jerome Charaoui from comment #3)
> Created attachment 76218 [details] [review] [review]
> bug 20630: Various improvements
> 
> Thank you for this patch. I've done some testing with it and it appears to
> work great. Compared to Plack, this method is much more efficient and less
> memory-hungry when running multiple instances. I'm currently using it to run
> 20 instances on a VM with 4GB of RAM, and will soon test with 120 instances
> on the same host. I'll be sure to report any further issues and results here.
> 

Hi Jerome, how has your system been going for these last 5 months?

Ha David, I've been reporting my progress mainly on bug 20582. In short: it works well, although there are some quirks to work out.

Comment on attachment 74671 [details] [review]
Bug 20630: An attempt at multitenancy with Mojolicious

Review of attachment 74671 [details] [review]:
-----------------------------------------------------------------

::: Koha/App/Koha.pm
@@ +73,5 @@
> +    my $caches = $Koha::Caches::singleton_caches;
> +    if ($caches) {
> +        foreach my $key (keys %$caches) {
> +            my $cache = $caches->{$key};
> +            if (ref $cache->{cache} eq 'Cache::Memory') {

This should not be needed, as of Cache::Memory was removed by bug 21955

Comment on attachment 74671 [details] [review]
Bug 20630: An attempt at multitenancy with Mojolicious

Review of attachment 74671 [details] [review]:
-----------------------------------------------------------------

::: Koha/App/Koha.pm
@@ +73,5 @@
> +    my $caches = $Koha::Caches::singleton_caches;
> +    if ($caches) {
> +        foreach my $key (keys %$caches) {
> +            my $cache = $caches->{$key};
> +            if (ref $cache->{cache} eq 'Cache::Memory') {

This should not be needed, as of Cache::Memory was removed by bug 21955

*** Bug 15562 has been marked as a duplicate of this bug. ***

This would still be interesting to explore, although I think we'd need to make more progress running Koha as a Mojolicious app before it would be worthwhile?

I have renewed interest in multitenanting Koha using Mojolicious.

At a glance, if these patches were rebased, they could actually be merged into Koha without affecting prod Koha driven by Starman. 

I'd be tempted to run a test system on a multitenanted Koha, and get folk to try it out and report if there were any problems. 

--

That said, I suppose in the back of my mind, I'd always be a bit bothered by what we'd lose by not having the processes isolated from each other. A mistake in variable scoping would have a larger impact on a multitenanted Koha.

Plugins could lead to cross tenant exposures with this more easily I think.  Also worth some consideration

(In reply to Martin Renvoize from comment #11)
> Plugins could lead to cross tenant exposures with this more easily I think. 
> Also worth some consideration

Agreed. I think there's another bug report or IRC chat where I talk about how plugins would be a big problem for multi-tenancy. 

I'm not sure how practical it's going to be for Koha, even if I would love it from a resource perspective...