Bug 13431 - Shared FastMmap file causes issues
Summary: Shared FastMmap file causes issues
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Packaging (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Jacek Ablewicz
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 13488 (view as bug list)
Depends on: 11842 12800
Blocks:
  Show dependency treegraph
 
Reported: 2014-12-10 15:33 UTC by Martin Renvoize
Modified: 2015-12-03 22:02 UTC (History)
14 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 13431 - Shared FastMmap file causes issues (2.26 KB, patch)
2014-12-11 14:09 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
[SIGNED OFF] Bug 13431 - Shared FastMmap file causes issues (2.33 KB, patch)
2014-12-11 14:47 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13431 - Shared FastMmap file causes issues (3.45 KB, patch)
2014-12-16 12:39 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 13431 [QA Follow-up]: Shared FastMmap file causes issues (2.35 KB, patch)
2014-12-30 10:04 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
[SIGNED OFF] Bug 13431 [QA Follow-up]: Shared FastMmap file causes issues (2.42 KB, patch)
2015-02-08 15:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 13431 - Shared FastMmap file causes issues (3.51 KB, patch)
2015-02-08 15:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 13431 [QA Follow-up]: Shared FastMmap file causes issues (2.42 KB, patch)
2015-02-08 15:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 13431 - Shared FastMmap file causes issues (3.56 KB, patch)
2015-02-11 11:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13431 [QA Follow-up]: Shared FastMmap file causes issues (2.46 KB, patch)
2015-02-11 11:18 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2014-12-10 15:33:53 UTC
+++ This bug was initially created as a clone of Bug #12800 +++

This bug has reared it's head again, but this time it's more serious.

Upon package upgrade on a multi-tenant server, if your tenants do not already have a MEMCACHED_NAMESPACE environment variable defined then all instance will default back to -koha-koha nameing for  the tmp file used for caching.  This inevitably results in the errors shown in the original bug for all but the one instance that crates the file in the first place.

At the very least, we should set this environment variable on a per instance basis at apache vhost conf creation.
Comment 1 Martin Renvoize 2014-12-11 09:49:02 UTC
I'm seeing this issue more and more on IRC as people upgrade their package installs.

The symptom is the appearance of the below error when searching, or looking at MRC Framework pages and a few other places:

Open of share file /tmp/sharefile-koha-koha failed: Permission denied at /usr/lib/perl5/Cache/FastMmap.pm line 640.

The fastest fix I've found to get your system back up and running whilst I work on a longer term fix, is to:

1) Add/Set 'SetEnv MEMCACHED_NAMESPACE="instancename"' per instance you have running in the apache virtual host configuration
2) Delete the existing temp file /tmp/somthing-koha-koha

This will ensure koha creates a seperate temp file for caching pursposes on a epr instance basis, and the deletion will ensure it happens upon the next request per instance.
Comment 2 Robin Sheat 2014-12-11 11:30:23 UTC
Damn,I thought it was supposed to pick up the username it was running under and include that in the filename. If someone can figure out how to make it do that then I can roll a fixed package. I'm at a conference until Saturday so won't have a chance to look into it, but can make time to do a release.
Comment 3 Jacek Ablewicz 2014-12-11 13:01:19 UTC
Looks like not only multi-tenant setups are affected by this bug: I just stumbled upon the very same thing when trying to import some biblios into the database using misc/migration_tools/bulkmarcimport.pl script.
Comment 4 Jacek Ablewicz 2014-12-11 14:09:15 UTC Comment hidden (obsolete)
Comment 5 Tomás Cohen Arazi 2014-12-11 14:47:31 UTC Comment hidden (obsolete)
Comment 6 Jacek Ablewicz 2014-12-11 18:31:59 UTC
Ouch, after testing this on the production server for a little while I realized that fixing it in that way is not such a good idea after all :(, it has some unforseen side effects. Problem is that with Cache::FastMmap we are caching some things (like MARC frameworks) persistently and indefinitely: they never expire from the cache (like e.g. authorized values do, after 5 seconds AFAIRC). Cached MARC frameworks are only explicitly invalidated and updated to the new values after introducing some changes to them via web interface. So if there is separate copy of the cache file being used by command line scripts, MARC frameworks cached there would sadly never get updated. I dont know how to fix it properly.. perhaps setting 'unlink_on_exit' to 1 (and/or setting expire_time to something other then 0), when GATEWAY_INTERFACE environment variable is not defined, would be somehow acceptable solution (?). As a quick & dirty fix, 'chmod 666 /tmp/sharefile-koha-koha' seems to work as well, but that's probably not a very good idea :).

Also, regarding multi-tenant servers: apparently setting up different namespaces for different tenants may still be much desired thing to do, regardless of this particular bug manifesting itself or not. This error only has a chance to immediately show itself in such setups where there is different UID being used for each tenant (I guess this is how things are set up by default in the packaged version ?). But in manually configured multi-instance servers, when there is a same UID in use on the WWW server side for different tenants - and they don't have their own separate namespaces defined properly yet - after the upgrade, there would be no error visible, but still more and more important per-instance Koha settings (frameworks, authorized values, ...) may possibly end up being unintentionally shared between 2+ tenants.
Comment 7 Martin Renvoize 2014-12-11 18:42:53 UTC
I had a horrible feeling something like that may be the case; but like you I'm unsure of a good solution at present.  Setting a default MEMCACHED_NAMESPACE per instance at koha-create time I think would be fairly trivial (though I'de prefer that pref to be called CACHED_NAMESPACE as the MEMCACHED bit is misleading).  It's running this through the update that I'm really baffled about.. there are similar issues with needing to update koha-conf.xml accross upgrades.
Comment 8 Robin Sheat 2014-12-11 20:51:49 UTC
How about a default name space based on the uid and the database name if one isn't defined in koha-conf
Comment 9 Tomás Cohen Arazi 2014-12-11 20:57:16 UTC
How about completely removing FastMmap as a cache backend? I guess for most of the cases mysql does a better job caching query results.
Comment 10 Marc Véron 2014-12-12 07:21:15 UTC
(In reply to Tomás Cohen Arazi from comment #9)
> How about completely removing FastMmap as a cache backend? I guess for most
> of the cases mysql does a better job caching query results.

+1 for less complexity
Comment 11 Jacek Ablewicz 2014-12-12 10:08:40 UTC
(In reply to Tomás Cohen Arazi from comment #9)
> I guess for most of the cases mysql does a better job caching query results.

It looks that way - fastmmap turns out to be not all that fast after all ;). I did some simple tests (fetching BKS framework with GetMarcStructure() 1000 times in the row):

- not using Koha::Cache, mysql query cache enabled: 27 seconds
- not using Koha::Cache, mysql query cache disabled: 39s
- fastmmap_cache: 35s (!)
- memory_cache: 10s

However, such test case is pretty artificial - things may be different for syspref caching, authorized values caching, how would it behave on heavily loaded servers, what kind of impact from connection latencies we may expect if persistent caching is not in use and mysql is on the different machine (I guess quite a big impact regarding sysprefs and AVs fetching?) etc.

On the other hand, I wonder how many people in the wild may be actually using CACHING_SYSTEM=fastmmap_cache (or are even aware of such possibility)? Interesting thing about fastmmap_cache is that it is currently never used by default nor as a fallback mechanism (at least not since Bug 12041). But it still gets loaded and initialized each and every time, causing possible "Permision denied ..." issues even on such setups which do not use fastmmap cache at all.
Comment 12 Katrin Fischer 2014-12-12 10:11:08 UTC
I think that's one of hte main problems about this - even if you haven't enabled caching or don't intend to use it, it's still being used. And it relies on a parameter that seems unrelated to the itself MEMCACHED_NAMESPACE.
Comment 13 Jacek Ablewicz 2014-12-15 18:29:50 UTC
Not sure how proceed with this one.. I have to admit removing fastmmap caching completely is kinda tempting, but OTOH it's not too hard to imagine some setups where such caching system would be quite beneficial performace-wise. What if we:

1) do not load nor intialize the Cache::FastMmap module, unless such caching system is explicitly requested by the user (CACHING_SYSTEM=fastmmap_cache)
2) also do not load it when $ENV{GATEWAY_INTERFACE} is not defined
3) include _both_ the UID and the database name in the /tmp sharemap file name

That way it:
- would still be a trival patch,
- this feature will remain available for the people who really want/need it
- and (unless I'm very much mistaken) it should cover almost 100% of the problematic cases (?).
Comment 14 Chris Cormack 2014-12-15 19:38:07 UTC
(In reply to Jacek Ablewicz from comment #13)
> Not sure how proceed with this one.. I have to admit removing fastmmap
> caching completely is kinda tempting, but OTOH it's not too hard to imagine
> some setups where such caching system would be quite beneficial
> performace-wise. What if we:
> 
> 1) do not load nor intialize the Cache::FastMmap module, unless such caching
> system is explicitly requested by the user (CACHING_SYSTEM=fastmmap_cache)
> 2) also do not load it when $ENV{GATEWAY_INTERFACE} is not defined
> 3) include _both_ the UID and the database name in the /tmp sharemap file
> name
> 
> That way it:
> - would still be a trival patch,
> - this feature will remain available for the people who really want/need it
> - and (unless I'm very much mistaken) it should cover almost 100% of the
> problematic cases (?).

That is a good start, and couple it with the moving the cache conf back to koha-conf.xml and out of env variables (bug 11921) and we would have a less error prone and less confusing set up.

But doing a patch like you suggest, will fix this bug, and then 11921 can follow on
Comment 15 Jacek Ablewicz 2014-12-16 12:39:57 UTC Comment hidden (obsolete)
Comment 16 Chris Cormack 2014-12-16 21:32:19 UTC
Comment on attachment 34453 [details] [review]
Bug 13431 - Shared FastMmap file causes issues

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

::: Koha/Cache.pm
@@ +40,4 @@
>  use Carp;
>  use Module::Load::Conditional qw(can_load);
>  use Koha::Cache::Object;
> +use C4::Context;

We can't do this. We just added a big circular dependency if we do.

Because C4::Context uses Koha::Cache

@@ +92,5 @@
>      }
>  
> +    if ( $self->{'default_type'} eq 'fastmmap'
> +      && defined( $ENV{GATEWAY_INTERFACE} )
> +      && can_load( modules => { 'Cache::FastMmap' => undef } ) ) {

This change seems good.

@@ +162,5 @@
>  sub _initialize_fastmmap {
>      my ($self) = @_;
> +    my $share_file = join( '-',
> +        "/tmp/sharefile-koha", $self->{'namespace'},
> +        C4::Context->config('hostname'), C4::Context->config('database'),

A better way to do this is is to change the places that call Koha::Cache->new()  
To set the namespace. 

EG my $cache = Koha::Cache-new({cache_type => 'memcached_cache', namespace => C4::Context->config('hostname') . '-' .C4::Context->config('database')});

I reread the code, and there isn't a bug in Koha::Cache, there is a bug in how we are calling it.
$self->{'namespace'} ||= $ENV{MEMCACHED_NAMESPACE} || 'koha'; 
Is what is in the module, we just are never setting namespace, so it defaults to what is in the ENV, or just koha
Comment 17 Robin Sheat 2014-12-16 22:40:01 UTC
(In reply to Jacek Ablewicz from comment #11)
> However, such test case is pretty artificial - things may be different for
> syspref caching, authorized values caching, how would it behave on heavily

Latency too, i.e. if you are using a separate MySQL server (which is a recommended configuration, imo) then having things local may be an improvement.

> On the other hand, I wonder how many people in the wild may be actually
> using CACHING_SYSTEM=fastmmap_cache (or are even aware of such possibility)?
> Interesting thing about fastmmap_cache is that it is currently never used by
> default nor as a fallback mechanism (at least not since Bug 12041). But it

Yeah, it's not recommended to use fastmmap_cache unless you have good cause to due to it not supporting expiry. I'd be quite OK with disabling the init of it and starting the functions with a 'confess "not implemented due to bug 13431"' if an attempt to use it is made. We can always fix it up later should we discover a need.
Comment 18 Jacek Ablewicz 2014-12-18 19:44:16 UTC
(In reply to Chris Cormack from comment #16)

> > +use C4::Context;
> 
> We can't do this. We just added a big circular dependency if we do.
> 
> Because C4::Context uses Koha::Cache

Not in the current master (?), but it indeed will be used after bug 11998 (I was under the impression that bug got already pushed). But it looks like this line is not even necessary: in all places where Koha::Cache is used so far, C4::Context is already loaded. Making such assumption is probably not very good practice, but this code part (which includes database name etc. in the sharefile name) is intended only as temporary (= until namespace configuration issue gets dealt with properly) solution for severely misconfigured multi-instance setups. So maybe we can live with that ;) if some extra precautions will be taken (like putting that code part in the eval {} block + some warn "Unable to determine database name due to .." maybe ?).

> @@ +162,5 @@
> >  sub _initialize_fastmmap {
> >      my ($self) = @_;
> > +    my $share_file = join( '-',
> > +        "/tmp/sharefile-koha", $self->{'namespace'},
> > +        C4::Context->config('hostname'), C4::Context->config('database'),
> 
> A better way to do this is is to change the places that call
> Koha::Cache->new()  
> To set the namespace. 
> 
> EG my $cache = Koha::Cache-new({cache_type => 'memcached_cache', namespace
> => C4::Context->config('hostname') . '-' .C4::Context->config('database')});

That would make Koha::Cache usage a bit inconvenient IMO, especially as Koha::Cache->new() is currently never called outside of Koha::Cache itself, Koha::Cache->get_instance() is always used instead.
Comment 19 Jacek Ablewicz 2014-12-18 20:33:53 UTC
(In reply to Robin Sheat from comment #17)

> Yeah, it's not recommended to use fastmmap_cache unless you have good cause
> to due to it not supporting expiry.

That comment in Koha/Cache.pm "fastmmap .. doesn't support expiry in a useful way" is quite puzzling; I'm unable to find anything specific anywhere regarding what exactly is wrong with it's expiry mechanism - ?
Comment 20 Robin Sheat 2014-12-19 01:52:25 UTC
(In reply to Jacek Ablewicz from comment #19)
> That comment in Koha/Cache.pm "fastmmap .. doesn't support expiry in a
> useful way" is quite puzzling; I'm unable to find anything specific anywhere
> regarding what exactly is wrong with it's expiry mechanism - ?

I can't remember now, there is/was something it doesn't do that we needed for consistency, but I'm unsure of the details. Looking at Cache::FastMmap now, it might be that you can't set the expiry timeout on individual writes, you can only set it globally.
Comment 21 Jacek Ablewicz 2014-12-19 05:44:45 UTC
(In reply to Robin Sheat from comment #20)

> Looking at Cache::FastMmap now, it might be that you can't set the expiry
> timeout on individual writes, you can only set it globally.

There seems to be a lot of confusing reports on the net regarding this subject, but setting expiry timeout separately for individual cache entries is supported since version 1.23 or so (although this particular feature is still apparently not documented in any meaningful way).
Comment 22 Jonathan Druart 2014-12-19 10:42:27 UTC
I don't know if it is relevant, but maybe should we provide a quick fix like:

     if ( can_load( modules => { 'Cache::FastMmap' => undef } ) ) {
-        _initialize_fastmmap($self);
-        if ( $self->{'default_type'} eq 'fastmmap'
-            && defined( $self->{'fastmmap_cache'} ) )
-        {
-            $self->{'cache'} = $self->{'fastmmap_cache'};
+        eval {
+            _initialize_fastmmap($self);
+            if ( $self->{'default_type'} eq 'fastmmap'
+                && defined( $self->{'fastmmap_cache'} ) )
+            {
+                $self->{'cache'} = $self->{'fastmmap_cache'};
+            }
+        };
+        if ( $@ ) {
+            warn "Cannot initialize FastMmap: $@";
         }
     }

In any case the application should crash if the cache is not correctly initialized.
Comment 23 Jacek Ablewicz 2014-12-30 10:04:38 UTC Comment hidden (obsolete)
Comment 24 Katrin Fischer 2015-02-08 15:44:57 UTC
Tested with both patches applied after running into the FastMmap issue again. This seems to work fine, but there is a small discrepancy to the test plan:

>3) Do not include UID in the sharefile name (it will be constructed
>using namespace + database name + database host instead).
>
>Test plan addendum:
>
>   s/and UID//

My files were created in:

/tmp/sharefile-koha-koha-localhost-koha

So it looks like the UID is still present?
Comment 25 Katrin Fischer 2015-02-08 15:49:15 UTC
Maybe just a documentation issue?

Test plan says:
namespace + database name + database host

But code looks more like:

namespace + database host + database name

+        $share_file = join( '-',
+            "/tmp/sharefile-koha", $self->{'namespace'},
+            C4::Context->config('hostname'), C4::Context->config('database') );

Signing off - please check my assumptions are correct.
Comment 26 Katrin Fischer 2015-02-08 15:51:11 UTC Comment hidden (obsolete)
Comment 27 Katrin Fischer 2015-02-08 15:52:10 UTC Comment hidden (obsolete)
Comment 28 Katrin Fischer 2015-02-08 15:52:14 UTC Comment hidden (obsolete)
Comment 29 Katrin Fischer 2015-02-08 15:53:50 UTC
One last note: How can we best document the new CACHING_SYSTEM=fastmmap?
Comment 30 Katrin Fischer 2015-02-08 15:54:33 UTC
*** Bug 13488 has been marked as a duplicate of this bug. ***
Comment 31 Jacek Ablewicz 2015-02-09 17:10:46 UTC
(In reply to Katrin Fischer from comment #25)
> Maybe just a documentation issue?
> 
> Test plan says:
> namespace + database name + database host
> 
> But code looks more like:
> 
> namespace + database host + database name
> 
> +        $share_file = join( '-',
> +            "/tmp/sharefile-koha", $self->{'namespace'},
> +            C4::Context->config('hostname'),
> C4::Context->config('database') );
> 
> Signing off - please check my assumptions are correct.

Yes, the database host in file name is before the database name. Including database name & host is probably a bit of overkill - as a result, file name will be kind of funny looking.. But after this patch, this file will only be created on such rare setups which are consciously using this particular cache system (I'm estimating there are maybe 2-5 such setups existing in the wild ;)
Comment 32 Jacek Ablewicz 2015-02-09 17:43:22 UTC
(In reply to Katrin Fischer from comment #29)
> One last note: How can we best document the new CACHING_SYSTEM=fastmmap?

I'm thinking that on this stage it may be better to leave it as is (ie., as undocumented feature, for those who are willing to test / use it in purely experimental way) - it's not clear if it is efficient enough to be worth a trouble (proverbial jury is still out on this one), and it seems to have some not yet fully evaluated potential issues (e.g., apparently it is not thread-safe).
Comment 33 Tomás Cohen Arazi 2015-02-09 18:10:17 UTC
I'd vote for disabling the use of fastmmap, as it is not clear how "efficient" it is. Some people did benchmark it and found it was counter productive.
Comment 34 Katrin Fischer 2015-02-09 18:41:59 UTC
I think Jacek patch will deactivate it - I am also ok with removing it, but would like to see this resolved soon
Comment 35 Chris Cormack 2015-02-09 19:01:41 UTC
Jacek's patch does the right thing, disables it unless people explicitly turn it on. Which is the best of both worlds.

Can someone else in the QA team please take a look, it'd be great to get this in 3.18.4
Comment 36 Jonathan Druart 2015-02-11 11:18:43 UTC
Created attachment 35823 [details] [review]
Bug 13431 - Shared FastMmap file causes issues

Koha::Cache package does not take into account that, when using
fastmmap caching variant, mmaped cache file created in /tmp
(typically: /tmp/sharefile-koha-koha), would only be further
accessible to the one given OS user - the one which created it.
In many Koha setups, in the circumstances when various system scripts
are executed by 2+ users with diffrent UIDs (like multi-tenant servers,
for example) this may cause many kinds of issues. Observable symptom
is usually the appearance of the below error when searching, or looking
at MARC Framework pages and a few other places:

Open of share file /tmp/sharefile-koha-koha failed: Permission denied
at /usr/lib/perl5/Cache/FastMmap.pm line 640.

This patch:
- disables initialisation of fastmmap caching subsystem unless it is
explicitly requested by the user (CACHING_SYSTEM=fastmmap)
- disables fastmmap cache usage for command line scripts
(i.e. when GATEWAY_INTERFACE environment variable is not defined)
- adds the database name, host name and an ID of the OS user to the
mmaped file name created in /tmp, to prevent various kinds of
unintentional conflicts and/or permission problems from happening

To test:

1) remove the /tmp/sharefile-koha-* file[s] (if any)
2) do something which would lead to its re-creation (e.g., performing
any search in OPAC should be sufficient to cause that)
3) observe that /tmp/sharefile-koha-koha got created
4) remove it
5) apply patch
6) redo step 2)
7) observe that aforementioned file is no longer created in /tmp
8) set CACHING_SYSTEM environment variable to 'fastmmap'
9) redo step 2), observe that /tmp/sharefile-koha-* file got created
and that it's name now contains hostname, database name and UID
10) ensure that everything still works like it should and that there
are no regressions of any kinds anywhere in the system ;)

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 37 Jonathan Druart 2015-02-11 11:18:52 UTC
Created attachment 35824 [details] [review]
Bug 13431 [QA Follow-up]: Shared FastMmap file causes issues

1) Removed 'use C4::Context;' because it can lead to introduction
of circular reference in the near future
2) Put fastmmap initialization code into an eval {} block, to catch
various kinds of errors which can still occur during it's init in
some [less usual] Koha setups and/or more unusual circumstances
3) Do not include UID in the sharefile name (it will be constructed
using namespace + database name + database host instead).

Test plan addendum:

   s/and UID//

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 38 Tomás Cohen Arazi 2015-02-12 18:30:15 UTC
Patches pushed to master.

Thanks Jacek!
Comment 39 Chris Cormack 2015-02-14 07:38:13 UTC
Pushed to 3.18.x will be in 3.18.4
Comment 40 Lennon Mazonde 2015-02-19 11:47:58 UTC
(In reply to Martin Renvoize from comment #1)
> I'm seeing this issue more and more on IRC as people upgrade their package
> installs.
> 
> The symptom is the appearance of the below error when searching, or looking
> at MRC Framework pages and a few other places:
> 
> Open of share file /tmp/sharefile-koha-koha failed: Permission denied at
> /usr/lib/perl5/Cache/FastMmap.pm line 640.
> 
> The fastest fix I've found to get your system back up and running whilst I
> work on a longer term fix, is to:
> 
> 1) Add/Set 'SetEnv MEMCACHED_NAMESPACE="instancename"' per instance you have
> running in the apache virtual host configuration
> 2) Delete the existing temp file /tmp/somthing-koha-koha
> 
> This will ensure koha creates a seperate temp file for caching pursposes on
> a epr instance basis, and the deletion will ensure it happens upon the next
> request per instance.

Hello, this seems to have worked for me. I have two instances of koha running on my server. After making the adjustments as suggested above, two temp files are now being created- sharefile-koha-xxa, sharefile-koha-xxb. Thanks!
Comment 41 Mason James 2015-02-26 04:55:47 UTC
(In reply to Chris Cormack from comment #39)
> Pushed to 3.18.x will be in 3.18.4

Pushed to 3.16.x, will be in 3.16.8