Bug 11921 - Move memcached configuration back to koha-conf.xml
Summary: Move memcached configuration back to koha-conf.xml
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
: 17224 (view as bug list)
Depends on: 6193 17188 17189
Blocks: 16579 17261 17332
  Show dependency treegraph
 
Reported: 2014-03-10 22:08 UTC by Galen Charlton
Modified: 2018-06-04 20:11 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
The configuration settings for specifying the memcached server is moved back to koha-conf.xml. Passing that information via an environment variable breaks badly on Plack. It would allow command line scripts and cron jobs to also make use of caching. Not only plack. ATTENTION on update: if you update from a config with memcached in koha-http.conf without Plack and you want to use Plack, you need to port memcached conf in koha-conf.xml by hand. If you don't use Plack, you can leave memcached conf in koha-http.conf, but for the future is still better to migrate the conf by hand.
Version(s) released in:


Attachments
Bug 11921: Restore memcached infos to koha-conf (4.69 KB, patch)
2016-08-24 11:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11921: Restore memcached infos to koha-conf - Koha::Config (10.61 KB, patch)
2016-08-24 11:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11921: Update remaining occurrences of ->ismemcached (2.54 KB, patch)
2016-08-24 11:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11921: Restore memcached infos to koha-conf (4.77 KB, patch)
2016-09-05 10:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11921: Restore memcached infos to koha-conf - Koha::Config (10.61 KB, patch)
2016-09-05 10:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11921: Update remaining occurrences of ->ismemcached (2.54 KB, patch)
2016-09-05 10:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11921: C4::Context->memcached does no longer exist (1.08 KB, patch)
2016-09-06 14:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11921: Fix logic in C4::Context->new to init the config (1.51 KB, patch)
2016-09-06 14:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11921: Restore memcached infos to koha-conf (4.77 KB, patch)
2016-09-06 22:55 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 11921: Restore memcached infos to koha-conf - Koha::Config (10.67 KB, patch)
2016-09-06 22:56 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 11921: Update remaining occurrences of ->ismemcached (2.59 KB, patch)
2016-09-06 22:57 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 11921: C4::Context->memcached does no longer exist (1.13 KB, patch)
2016-09-06 22:57 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 11921: Fix logic in C4::Context->new to init the config (1.57 KB, patch)
2016-09-06 22:58 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 11921: Restore memcached infos to koha-conf (4.82 KB, patch)
2016-09-07 15:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11921: Restore memcached infos to koha-conf - Koha::Config (10.72 KB, patch)
2016-09-07 15:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11921: Update remaining occurrences of ->ismemcached (2.65 KB, patch)
2016-09-07 15:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11921: C4::Context->memcached does no longer exist (1.18 KB, patch)
2016-09-07 15:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11921: Fix logic in C4::Context->new to init the config (1.62 KB, patch)
2016-09-07 15:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11921: (QA followup) Remove remaining unused var (4.07 KB, patch)
2016-09-07 15:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11921: (followup) Don't die on non-existent koha-conf.xml (3.19 KB, patch)
2016-09-09 14:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Galen Charlton 2014-03-10 22:08:50 UTC
The configuration settings for specifying the memcached server should be moved back to koha-conf.xml.  The current scheme of passing that information via an environment variable breaks badly on Plack without requiring significant machinations -- and if you're using Plack, there is no need to cache the base Koha configuration information in memcached.
Comment 1 Chris Cormack 2014-03-10 22:13:16 UTC
I agree totally, the config file is a much more logical place to have this. It would allow command line scripts and cron jobs to also make use of caching. Not only plack.

With Koha::Cache we are able to make use of a lot of caching mechanism, not just memcached, having examples of these commented out in the koha-conf.xml would make it much easier for others to use them.
Comment 2 Jonathan Druart 2016-08-24 11:43:05 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2016-08-24 11:43:09 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2016-08-24 11:43:12 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2016-08-24 11:44:46 UTC
See bug 16579 for more information on these patches.
Comment 6 Jonathan Druart 2016-09-05 10:16:19 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2016-09-05 10:16:22 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2016-09-05 10:16:25 UTC Comment hidden (obsolete)
Comment 9 Jacek Ablewicz 2016-09-06 13:04:09 UTC
in C4/Contex.pm ->new():

251: unless ( %$self ) {
         $self = Koha::Config->read_from_file($conf_fname);
     }

$self is always an empty hash at this point, so config file would be rereaded and reparsed no matter what?
Comment 10 Jacek Ablewicz 2016-09-06 13:10:06 UTC
C4/Auth.pm, in get_session():

1731:    elsif ( $storage_method eq 'memcached' && Koha::Caches->get_instance->memcached_cache ) {
         $session = new CGI::Session( "driver:memcached;serializer:yaml;id:md5", $sessionID, { Memcached => C4::Context->memcached } );

looks like ->memcached method doesn't exist in C4::Context anymore
Comment 11 Jonathan Druart 2016-09-06 14:21:19 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2016-09-06 14:21:31 UTC Comment hidden (obsolete)
Comment 13 Chris Cormack 2016-09-06 22:31:34 UTC
Trap for young testers

Bug 17189 updates plack.psgi .. make sure you are running that version before you test (you will probably need to manually copy it)
Comment 14 Chris Cormack 2016-09-06 22:49:22 UTC
It takes a bit to get set up to test, the main thing is make sure you are using the latest plack.psgi.

But once it is set up it seems to work well, plack is now running using memcached using the variables from the config file.

I will sign off.
Comment 15 Chris Cormack 2016-09-06 22:55:47 UTC Comment hidden (obsolete)
Comment 16 Chris Cormack 2016-09-06 22:56:48 UTC Comment hidden (obsolete)
Comment 17 Chris Cormack 2016-09-06 22:57:23 UTC Comment hidden (obsolete)
Comment 18 Chris Cormack 2016-09-06 22:57:55 UTC Comment hidden (obsolete)
Comment 19 Chris Cormack 2016-09-06 22:58:17 UTC Comment hidden (obsolete)
Comment 20 Tomás Cohen Arazi 2016-09-07 15:52:31 UTC
Created attachment 55310 [details] [review]
Bug 11921: Restore memcached infos to koha-conf

MEMCACHED_SERVERS and MEMCACHED_NAMESPACE should be moved back to
koha-conf instead of an ENV var to let command line scripts use
the cache mechanism.

TODO:
- Add the entries to koha-conf.xml
- Remove occurrences of MEMCACHED_SERVERS in package, apache and plack files

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 21 Tomás Cohen Arazi 2016-09-07 15:52:40 UTC
Created attachment 55311 [details] [review]
Bug 11921: Restore memcached infos to koha-conf - Koha::Config

Move C4::Context->read_config_file to Koha::Config
That permits to reuse it from Koha::Cache without needing C4::Context
(and introduce a circular deps).

TODO: Add decent POD to Koha::Config
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 22 Tomás Cohen Arazi 2016-09-07 15:52:48 UTC
Created attachment 55312 [details] [review]
Bug 11921: Update remaining occurrences of ->ismemcached

TODO: Would be better to provide a better way to know if memcache is correctly set
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Tomás Cohen Arazi 2016-09-07 15:52:57 UTC
Created attachment 55313 [details] [review]
Bug 11921: C4::Context->memcached does no longer exist

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Tomás Cohen Arazi 2016-09-07 15:53:09 UTC
Created attachment 55314 [details] [review]
Bug 11921: Fix logic in C4::Context->new to init the config

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 25 Tomás Cohen Arazi 2016-09-07 15:53:20 UTC
Created attachment 55315 [details] [review]
Bug 11921: (QA followup) Remove remaining unused var

This patch removes an occurence of an unused var in Context.pm and
also prevents a warning if memcached_servers is not set:

Use of uninitialized value in split at /home/vagrant/kohaclone/Koha/Cache.pm line 91.

t also tidies small things.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 26 Tomás Cohen Arazi 2016-09-07 15:54:05 UTC
Works as expected, backwards compatible (ENV overloads koha-conf.xml)
Comment 27 Kyle M Hall 2016-09-08 11:39:26 UTC
Pushed to master for 16.11, thanks Jonathan!
Comment 28 Tomás Cohen Arazi 2016-09-09 14:54:30 UTC
Created attachment 55441 [details] [review]
Bug 11921: (followup) Don't die on non-existent koha-conf.xml

This patch wraps the XMLin call on an eval block. It also adds unit
tests (mocked) for Koha::Config->read_from_file()

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 29 Jacek Ablewicz 2016-09-13 09:19:35 UTC
To work in (so-so) reliable way, this feature (using memcached for storing CGI sessions)

    $session = new CGI::Session( "driver:memcached;serializer:yaml;id:md5", $sessionID, { Memcached => $memcached } );

desperately needs a possibility to use it's own separately configured memcached server instance for that task. Otherwise the risk for user sessions being occasionally kicked out in semi-random fashion is pretty substantial.
Comment 30 Tomás Cohen Arazi 2016-09-13 14:00:01 UTC
(In reply to Jacek Ablewicz from comment #29)
> To work in (so-so) reliable way, this feature (using memcached for storing
> CGI sessions)
> 
>     $session = new CGI::Session( "driver:memcached;serializer:yaml;id:md5",
> $sessionID, { Memcached => $memcached } );
> 
> desperately needs a possibility to use it's own separately configured
> memcached server instance for that task. Otherwise the risk for user
> sessions being occasionally kicked out in semi-random fashion is pretty
> substantial.

Jacek, are you implying there's a bug on master now this has been pushed?
Comment 31 Jacek Ablewicz 2016-09-13 15:19:05 UTC
(In reply to Tomás Cohen Arazi from comment #30)
> (In reply to Jacek Ablewicz from comment #29)
> > To work in (so-so) reliable way, this feature (using memcached for storing
> > CGI sessions)
> > 
> >     $session = new CGI::Session( "driver:memcached;serializer:yaml;id:md5",
> > $sessionID, { Memcached => $memcached } );
> > 
> > desperately needs a possibility to use it's own separately configured
> > memcached server instance for that task. Otherwise the risk for user
> > sessions being occasionally kicked out in semi-random fashion is pretty
> > substantial.
> 
> Jacek, are you implying there's a bug on master now this has been pushed?

Sorry for the confusing comment - what I meant is that storing sessions in the same memcached server instance which is used as L2 caching system for everything else (& their dogs) has potential reliability problems (extra fun / unpredictability involved if the same memcached instance is used by the multiple tenants) - but it's not the fault of this patch set, more like a side note.
Comment 32 Jonathan Druart 2016-09-14 10:34:31 UTC
*** Bug 17224 has been marked as a duplicate of this bug. ***
Comment 33 Frédéric Demians 2016-09-22 06:00:33 UTC
Jonathan, don't you think pushing those patches to 16.05 would be highly
risky? I can see the issue in 16.05 concerning the way memcached server is
retrieved. With Plack enabled, memcached server is just not used whether you
don't export MEMCACHED_SERVERS in shell environment before starting Plack. But
isn't the patch for bug 17262 more secure for 16.05? because it doesn't affect
that much code in Koha inner modules?
Comment 34 Jonathan Druart 2016-09-27 07:37:31 UTC
(In reply to Frédéric Demians from comment #33)
> Jonathan, don't you think pushing those patches to 16.05 would be highly
> risky? I can see the issue in 16.05 concerning the way memcached server is
> retrieved. With Plack enabled, memcached server is just not used whether you
> don't export MEMCACHED_SERVERS in shell environment before starting Plack.
> But
> isn't the patch for bug 17262 more secure for 16.05? because it doesn't
> affect
> that much code in Koha inner modules?

Yes, I don't think these patches should be backported.
Comment 35 Mason James 2016-11-30 08:15:50 UTC
enhancement, passing for 16.05