Bug 16579

Summary: [OMNIBUS] Use separate memcached namespace for caching koha-conf.xml
Product: Koha Reporter: Kyle M Hall <kyle>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: abl, chris, jonathan.druart, jweaver, mtj, nick, srdjan, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16770
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15562
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17224
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 6193, 11921, 16166, 17188, 17189    
Bug Blocks:    
Attachments: Bug XXXXX_1: Fix support of several memcached servers
Bug XXXXX_2: Add the ability to define several namespaces - tests
Bug XXXXX_2: Add the ability to define several namespaces - Koha::Caches
Bug XXXXX_2: Add the ability to define several namespaces - replace existing occurrences
Bug XXXXX_3: Restore memcached infos to koha-conf
Bug XXXXX_3: Restore memcached infos to koha-conf - Koha::Config
Bug XXXXX_3: Update remaining occurrences of ->ismemcached
Bug XXXXX_1: Fix support of several memcached servers
Bug XXXXX_2: Add the ability to define several namespaces - tests
Bug XXXXX_2: Add the ability to define several namespaces - Koha::Caches
Bug XXXXX_2: Add the ability to define several namespaces - replace existing occurrences
Bug XXXXX_3: Restore memcached infos to koha-conf
Bug XXXXX_3: Restore memcached infos to koha-conf - Koha::Config
Bug XXXXX_3: Update remaining occurrences of ->ismemcached
Bug XXXXX_2: Add the ability to define several namespaces - Koha::Caches
Bug XXXXX_2: Add the ability to define several namespaces - replace existing occurrences
Bug XXXXX_3: Restore memcached infos to koha-conf
Bug XXXXX_3: Restore memcached infos to koha-conf - Koha::Config
Bug XXXXX_3: Update remaining occurrences of ->ismemcached

Description Kyle M Hall 2016-05-24 18:02:37 UTC
The memcached configuration definitions were moved from koha-conf.xml to environment variables to enable caching of the conf file itself.

This can cause numerous issues and difficulties making sure memcached is enabled for all cron scripts, for plack, etc.

I propose we cache the koha conf file in a separate namespace with the caching still defined in environment variables, but move the memcached configuration for everything else back into the koha conf file itself.

This would allow us to continue caching everything, but even if the cache for the koha-conf file is not configured for a given process, it would allow all other data to still be cached and used. This would greatly improve overall performance I think, especially for cron scripts that use the calendar for holiday handling.
Comment 1 Jesse Weaver 2016-05-25 14:37:17 UTC
I'd like to suggest that we either:

 a) drop the caching for the koha-conf.xml entirely (in my testing, this adds about .14 seconds to an average CGI load) or
 b) just hardcode the server/namespace for caching the koha-conf.xml and use the filename of the .xml as the cache key (kohaconf-/etc/koha/sites/SITE/koha-conf.xml). Using localhost:11211 will work for 99.9% of setups, and if memcached isn't running there, Koha will still work (just a tiny bit slower under CGI).
Comment 2 Kyle M Hall 2016-06-07 16:31:48 UTC
(In reply to Jesse Weaver from comment #1)
> I'd like to suggest that we either:
> 
>  a) drop the caching for the koha-conf.xml entirely (in my testing, this
> adds about .14 seconds to an average CGI load) or
>  b) just hardcode the server/namespace for caching the koha-conf.xml and use
> the filename of the .xml as the cache key
> (kohaconf-/etc/koha/sites/SITE/koha-conf.xml). Using localhost:11211 will
> work for 99.9% of setups, and if memcached isn't running there, Koha will
> still work (just a tiny bit slower under CGI).

I think I'm for option a at this point. The cons seem to far outweigh the pros.
Comment 3 Jonathan Druart 2016-06-12 08:55:46 UTC
I am not in favor of adding 0.15s when we are fighting to gain ms everywhere in the code, so I'd vote for b.

(In reply to Kyle M Hall from comment #0)
> This can cause numerous issues and difficulties making sure memcached is
> enabled for all cron scripts, for plack, etc.

Could you detail the issues and difficulties? Why is it hard to set the env var?
Comment 4 Jonathan Druart 2016-06-12 09:00:00 UTC
But b is not ideal neither.
You expect that memcached won't be use if the env var is not defined.
We can imagine a sysop wanting to disable the cache (because a bug has been found). He does not necessarily want to stop memcached.
Comment 5 Jacek Ablewicz 2016-06-13 13:36:39 UTC
140 miliseconds? Quite a lot - I know config file is currently parsed 3 times by an average script (for no apparent reason), but on my test setup, C4::Context->new() call only takes ~5.1 miliseconds. Memcache not enabled. And I have three Z39.50 servers defined in there.

With

   Cache => 'memcopy'

added to XMLin() in C4/Context.pm, its down to 92 microseconds per call.
Comment 6 Jacek Ablewicz 2016-06-13 13:49:34 UTC
Either way, I have a feeling that implementing it (fetching memcache settings from Koha config) is not going to be easy. Fastmmap caching system is still broken, because it wasn't easy in that case (see Bug 16067 & Bug 11998 comment #114 +).
Comment 7 Jacek Ablewicz 2016-06-13 13:59:14 UTC
(In reply to Jonathan Druart from comment #3)
> Why is it hard to set the env var?

Maybe it's not that hard, but it needs to be done in many places, and somehow it gets often forgotten. E.g., in 3.22.6 (or 3.22.7) installed from packages, memcached is enabled by default for CGI/plack, but memcached environment variables are not defined for shell / maintenance scripts. After installer/data/mysql/updatedatabase.pl DB update is done from command line, GUI is still prompting for an update, because it fetches (outdated) version preference from the memcache.
Comment 8 Kyle M Hall 2016-06-13 18:31:21 UTC
I think bug 11921 is a far more sane solution than this. Far fewer moving parts, less prone to regressions. Setting this to WONTFIX for the time being.
Comment 9 Jacek Ablewicz 2016-06-14 13:40:36 UTC
To make things a bit more complicated, this separate cache is used not just for caching koha config in C4/Context; e.g. in C4/Languages.pm:

    if (C4::Context->ismemcached) {
        require Memoize::Memcached;
        import Memoize::Memcached qw(memoize_memcached);

        memoize_memcached('getTranslatedLanguages',
            memcached => memcached => C4::Context->memcached);
        memoize_memcached('getFrameworkLanguages',
            memcached => memcached => C4::Context->memcached);
        memoize_memcached('getAllLanguages',
            memcached => memcached => C4::Context->memcached);
    }

Looks like an excellent material for a bunch of the brand new '... is not plack safe' bug reports ;)
Comment 10 Jonathan Druart 2016-06-18 14:19:34 UTC
(In reply to Jacek Ablewicz from comment #9)
> To make things a bit more complicated, this separate cache is used not just
> for caching koha config in C4/Context; e.g. in C4/Languages.pm:

At least not in C4/Languages, this bunch of code is buggy. See bug 16770.
Comment 11 Jonathan Druart 2016-06-18 15:20:00 UTC
Maybe it's silly but I have another option:
c) move the memcached config to a syspref and do not fetch this pref from the cache (like we do for OVERRIDE_SYSPREF_*)

It will 1/ avoid to parse the koha conf file, 2/ keep the ability to cache it, 3/ add the ability to use the cache from anywhere and 4/ remove the need to export env vars.

I have to admit it's not its place and the cache config should not be stored in the DB, but it looks like a tempting solution. Do I miss something?
Comment 12 Jacek Ablewicz 2016-06-19 15:02:34 UTC
(In reply to Jonathan Druart from comment #11)

> c) move the memcached config to a syspref and do not fetch this pref from
> the cache (like we do for OVERRIDE_SYSPREF_*)

You mean, to fetch memcached setup directly from the database? But to do it you'll need to parse koha config file first, to obtain database connection config.

> It will 1/ avoid to parse the koha conf file

I don't see how this step can be avoided in such scenario (?).

2nd problem with this scenario is that plack startup script will initialize DB connection (1st 'use C4::Context;' will trigger it) and without some extra care this single DB connection will then be shared between multiple plack workers. But this is not really a big problem, just a matter of adding e.g.

{
    ## pre-init DBI / DBIx database connection
    my $schema = Koha::Database->new()->schema();
    my $dbh = C4::Context->dbh;
    $dbh->do('SELECT * FROM systempreferences WHERE 1 = 0');
    Koha::Config::SysPrefs->find('marcflavour');

    ## trigger lazy-loaded modules compilation (conditional
    ## requires etc.) in Template::*, CGI and Koha::Dateutils
    # Template::Config->preload();
    # CGI->compile(':all');
    ## initializes locale and timezone related stuff
    # dt_from_string();

    ## disconnect from the database, otherwise the single database
    ## connection will be shared between multiple plack workers
    $schema->storage()->disconnect();
}

at the end of the startup script. Actually, this may have some extra adventages as it will allow safe preloading of many more modules at startup (good for performance and really good for memory footprint, due to copy-on-write), including some problematic ones like e.g. C4/Auth.pm (which calls ->preference() in BEGIN block).
Comment 13 Jonathan Druart 2016-06-23 11:39:51 UTC
(In reply to Jacek Ablewicz from comment #12)
> (In reply to Jonathan Druart from comment #11)
> 
> > c) move the memcached config to a syspref and do not fetch this pref from
> > the cache (like we do for OVERRIDE_SYSPREF_*)
> 
> You mean, to fetch memcached setup directly from the database? But to do it
> you'll need to parse koha config file first, to obtain database connection
> config.
> 
> > It will 1/ avoid to parse the koha conf file
> 
> I don't see how this step can be avoided in such scenario (?).

No of course we will have to parse it :)
I meant we will just need to parse it once.
Once parsed, we could cache it and avoid to parse it for each request.
But actually it does not make sense to move it to syspref, there is no additional advantage compared to koha-conf (it's even worst, we need the dbh).

I have worked on a proposal, patches are coming
Comment 14 Jonathan Druart 2016-06-23 11:46:33 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2016-06-23 11:48:56 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2016-06-23 11:49:29 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2016-06-23 11:51:34 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2016-06-23 11:52:36 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2016-06-23 11:54:56 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2016-06-23 11:55:48 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2016-07-03 15:11:18 UTC
Could someone take a look at these patches?
Comment 22 Chris Cormack 2016-07-04 21:23:52 UTC
Applying: Bug XXXXX_2: Add the ability to define several namespaces - tests
fatal: sha1 information is lacking or useless (t/Cache.t).
error: could not build fake ancestor
Patch failed at 0001 Bug XXXXX_2: Add the ability to define several namespaces - tests
Comment 23 Jonathan Druart 2016-07-06 13:27:23 UTC
(In reply to Chris Cormack from comment #22)
> Applying: Bug XXXXX_2: Add the ability to define several namespaces - tests
> fatal: sha1 information is lacking or useless (t/Cache.t).
> error: could not build fake ancestor
> Patch failed at 0001 Bug XXXXX_2: Add the ability to define several
> namespaces - tests

Sorry, I forgot to add the dependent bug. It depends on bug 16166.
Comment 24 Jacek Ablewicz 2016-07-14 12:30:39 UTC
(In reply to Jonathan Druart from comment #21)
> Could someone take a look at these patches?

Overall they look (mostly) fine to me, or at least a move in the right direction[s]. They are a bit hard to digest though, lots of collateral fixes included - if some of them could be (?) moved to the separate reports, signing off and Q&A will probably have a much better chance to occur in foreseeable future ;).

Not sure if I fully understand a concept of namespaces / sub-namespaces introduced with this patch set (and what are they needed for exactly, especially in such particular form). E.g. L1 storage hash still remains a shared singleton (sort of) - moving L1 storage to the cache objects instead (and ideally tying those objects to the Koha contexts in 1:1 relationship) seems to be much less headache-prone long term solution IMO.

By the look of it, in regards of cache architectural changes, there will be some overlap with Bug 15562, which in parts does somehow similar things with the cache (and a lot of other interesting things as well, but it's huge and I don't really understand ~half of it). Are there any chances of coordinated effort of some sort, so they both don't clash too badly with each other?
Comment 25 Srdjan Jankovic 2016-07-15 03:32:18 UTC
(In reply to Jacek Ablewicz from comment #24)
> By the look of it, in regards of cache architectural changes, there will be
> some overlap with Bug 15562, which in parts does somehow similar things with
> the cache (and a lot of other interesting things as well, but it's huge and
> I don't really understand ~half of it). Are there any chances of coordinated
> effort of some sort, so they both don't clash too badly with each other?

That's terrible. Next time I rebase I'll try to make it more readable. Apologies.

Changes presented here are almost identical to the changes 15562, except that they are a bit better:
* I fell short of introducing Koha::Config which I should have (I chickened out)
* Koha::Caches - maybe we need to mandate the namespace when instantiating Koha::Cache in order for this to be more robust?

At any rate, I did my patch in order to make plack multi-host going, and to be brutally honest I was expecting things in the Cache/Context land to change a lot by the time my patch gets some love. The issue is very complex, so it is probably good to have independent efforts at early stages. Helps with discovering unforeseen situations.

One thing that might be worth thinking about is that the very same problem exists with the database connections. So maybe some über class, Koha::Storage or something that would have a slot for everything we use, and then Koha::Storages collection keyed by the namespace?
Comment 26 Jonathan Druart 2016-08-03 13:58:04 UTC
Hi Jacek and Srdjan!
Thanks for taking a look and sorry for the delay to get back to you.

(In reply to Jacek Ablewicz from comment #24)
> (In reply to Jonathan Druart from comment #21)
> > Could someone take a look at these patches?
> 
> Overall they look (mostly) fine to me, or at least a move in the right
> direction[s]. They are a bit hard to digest though, lots of collateral fixes
> included - if some of them could be (?) moved to the separate reports,
> signing off and Q&A will probably have a much better chance to occur in
> foreseeable future ;).

That's why I have prefixed the commit messages with Bug XXXXX_1|2|3.
Patches with "Bug XXXXX_1" should go on their own bug report, same with _2 and _3.
I plan to move them, but I wanted to know if the different changes were appreciated before splitting them.

> Not sure if I fully understand a concept of namespaces / sub-namespaces
> introduced with this patch set (and what are they needed for exactly,
> especially in such particular form). E.g. L1 storage hash still remains a
> shared singleton (sort of) - moving L1 storage to the cache objects instead
> (and ideally tying those objects to the Koha contexts in 1:1 relationship)
> seems to be much less headache-prone long term solution IMO.

Yes indeed, it was the easy and lazy approach. My first idea was to manage it from Koha::Caches which makes more sense, but it was not so easy.
I'd be happy to improve that if you have a good idea on how to do it.

(In reply to Srdjan Jankovic from comment #25)
> (In reply to Jacek Ablewicz from comment #24)
> > By the look of it, in regards of cache architectural changes, there will be
> > some overlap with Bug 15562, which in parts does somehow similar things with
> > the cache (and a lot of other interesting things as well, but it's huge and
> > I don't really understand ~half of it). Are there any chances of coordinated
> > effort of some sort, so they both don't clash too badly with each other?
> 
> That's terrible. Next time I rebase I'll try to make it more readable.
> Apologies.
> 
> Changes presented here are almost identical to the changes 15562, except
> that they are a bit better:
> * I fell short of introducing Koha::Config which I should have (I chickened
> out)
> * Koha::Caches - maybe we need to mandate the namespace when instantiating
> Koha::Cache in order for this to be more robust?

Does that mean you would agree to let these patches make his own way then rebase yours? :)

> One thing that might be worth thinking about is that the very same problem
> exists with the database connections. So maybe some über class,
> Koha::Storage or something that would have a slot for everything we use, and
> then Koha::Storages collection keyed by the namespace?

I think we are dealing pretty well with our cache improvements: we improve it a lot without breaking anything (so far!), using small steps.
So I'd suggest to continue like that and keep in mind these Koha::Storage[s] classes

What's next? :)
Comment 27 Jonathan Druart 2016-08-03 14:24:57 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2016-08-03 14:25:08 UTC Comment hidden (obsolete)
Comment 29 Jonathan Druart 2016-08-03 14:25:29 UTC Comment hidden (obsolete)
Comment 30 Jonathan Druart 2016-08-03 14:25:43 UTC Comment hidden (obsolete)
Comment 31 Jonathan Druart 2016-08-03 14:25:55 UTC Comment hidden (obsolete)
Comment 32 Jonathan Druart 2016-08-03 14:26:03 UTC Comment hidden (obsolete)
Comment 33 Jonathan Druart 2016-08-03 14:26:11 UTC Comment hidden (obsolete)
Comment 34 Jonathan Druart 2016-08-03 14:32:02 UTC Comment hidden (obsolete)
Comment 35 Jonathan Druart 2016-08-03 14:32:12 UTC Comment hidden (obsolete)
Comment 36 Jonathan Druart 2016-08-03 14:32:18 UTC Comment hidden (obsolete)
Comment 37 Jonathan Druart 2016-08-03 14:32:24 UTC Comment hidden (obsolete)
Comment 38 Jonathan Druart 2016-08-03 14:32:30 UTC Comment hidden (obsolete)
Comment 39 Srdjan Jankovic 2016-08-04 00:40:44 UTC
> (In reply to Srdjan Jankovic from comment #25)
> Does that mean you would agree to let these patches make his own way then
> rebase yours? :)

Absolutely. It affects only one of my patches, and if lucky enough I may even be able to throw it away. It is a pre-patch anyway, to prepare the grounds for the real issue.
Comment 40 Nick Clemens 2016-08-12 19:10:20 UTC
I haven't fully tested, but initial read through and applying looks good. This makes it much easier to get memcached functioning with plack and is a good improvement
Comment 41 Kyle M Hall 2016-08-15 13:14:34 UTC
(In reply to Nick Clemens from comment #40)
> I haven't fully tested, but initial read through and applying looks good.
> This makes it much easier to get memcached functioning with plack and is a
> good improvement

I agree! This is a huge improvement and greatly simplifies the setup.

Jacek, what do you think?
Comment 42 Jacek Ablewicz 2016-08-19 18:44:37 UTC
(In reply to Kyle M Hall from comment #41)
> (In reply to Nick Clemens from comment #40)
> > I haven't fully tested, but initial read through and applying looks good.
> > This makes it much easier to get memcached functioning with plack and is a
> > good improvement
> 
> I agree! This is a huge improvement and greatly simplifies the setup.
> 
> Jacek, what do you think?

I think generally it's good, and certainly good enough for the 'promotion' from "In discussion" to "Needs sign-off". I have some doubts re/ certain parts (mostly about an introduction of that 'namespace' thingy to the Koha caching system - not entirely sure if it's strictly necessary, and - in case it is - if doing it in this particular way is the best way possible).

But it's more the matter of opinion / personal preferences (and/or my lack of understanding why it's needed to be done, and/or why in that way) than any real concerns.

So far I haven't spotted any regressions in this patch set - looks like there may be some (= most likely very-very slight) performance hits involved for the caching system, because of the 'namespaces' introduction, but IMO sorting out inconsistencies of the memcached config / setups problems in the systemic way is _much_ more important then that.
Comment 43 Jonathan Druart 2016-08-24 11:34:30 UTC
 Bug XXXXX_1: Fix support of several memcached servers 
moved to bug 17188.
Comment 44 Jonathan Druart 2016-08-24 11:40:21 UTC
 Bug XXXXX_2: Add the ability to define several namespaces - tests
 Bug XXXXX_2: Add the ability to define several namespaces - Koha::Caches 
 Bug XXXXX_2: Add the ability to define several namespaces - replace existing occurrences

Moved to bug 17189.
Comment 45 Jonathan Druart 2016-08-24 11:43:37 UTC
 Bug XXXXX_3: Restore memcached infos to koha-conf
 Bug XXXXX_3: Restore memcached infos to koha-conf - Koha::Config
 Bug XXXXX_3: Update remaining occurrences of ->ismemcached

Moved to bug 11921
Comment 46 Jonathan Druart 2016-08-24 11:49:31 UTC
(In reply to Jacek Ablewicz from comment #42)
> (In reply to Kyle M Hall from comment #41)
> > (In reply to Nick Clemens from comment #40)
> > > I haven't fully tested, but initial read through and applying looks good.
> > > This makes it much easier to get memcached functioning with plack and is a
> > > good improvement
> > 
> > I agree! This is a huge improvement and greatly simplifies the setup.
> > 
> > Jacek, what do you think?
> 
> I think generally it's good, and certainly good enough for the 'promotion'
> from "In discussion" to "Needs sign-off". I have some doubts re/ certain
> parts (mostly about an introduction of that 'namespace' thingy to the Koha
> caching system - not entirely sure if it's strictly necessary, and - in case
> it is - if doing it in this particular way is the best way possible).

I agree, that's why I have added to FIXME in the commit message since the first submission :)

    FIXME: There is an architecture problem here: the L1 cache should be
    defined in Koha::Cache

All the patches have been splited into 3 bug reports.
Letting this one for the general discussion about these patches.
Comment 47 Jonathan Druart 2016-09-14 08:31:35 UTC
The 3 bugs 11921, 16166 and 17189 have been pushed to master.
Closing this one.