Bug 17262 - Plack on packages is not having memcached set properly
Summary: Plack on packages is not having memcached set properly
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Packaging (show other bugs)
Version: 16.05
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-06 19:42 UTC by Tomás Cohen Arazi
Modified: 2019-06-27 09:24 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 17262: Inject MEMCACHED_* variables into Plack's env (2.21 KB, patch)
2016-09-06 19:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17262: Inject MEMCACHED_* variables into Plack's env (2.36 KB, patch)
2016-09-21 18:05 UTC, Frédéric Demians
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2016-09-06 19:42:49 UTC
Using the patches from bug 17261, you will notice the running Plack backends are not getting the MEMCACHED_SERVERS and MEMCACHED_NAMESPACE variables filled as they should.

Steps to reproduce:
- Be on top of bug 17261
- Have a working memcached setup, plack disbaled (default in kohadevbox)
=> About page shows:
  Memcached: 	Servers: 127.0.0.1:11211 | Namespace: koha_kohadev | Status: running
- Enable Plack:
  sudo koha-plack --enable kohadev
  sudo koha-plack --start kohadev
  sudo service apache2 restart
- Reload about page
=> FAIL: You get:
  Memcached: 	Servers: undefined| Namespace: undefined | Status: unknown
Comment 1 Tomás Cohen Arazi 2016-09-06 19:51:27 UTC
Created attachment 55266 [details] [review]
Bug 17262: Inject MEMCACHED_* variables into Plack's env

This patch adds two functions to koha-functions.sh to read the MEMCACHED_*
variables from configured instances' apache files (the only source for that
info for now).

It then tweaks koha-plack's start_plack() function so it exports the variables
to env before launching starman.

The patch seems trivial, but it highlights a bigger problem with our Plack integration
as the starman server seems to hang if those environment variables are defined!

Sponsored-by: ByWater Solutions
Comment 2 Tomás Cohen Arazi 2016-09-06 19:52:49 UTC
Sorry for adding you all to the CC for this bug, but I think it deserves some urgent attention.

Note: The patch itself cannot be pushed without solving the bigger problem.
Comment 3 Chris Cormack 2016-09-06 20:40:39 UTC
Have you seen bug 11921

That shifts the config back out of ENV variables. I feel like signing that off and getting that in will be better than bandaiding what we do now.

What do you think?
Comment 4 Jacek Ablewicz 2016-09-07 09:52:11 UTC
This is surprising (+ that there were no apparent complains related to this so far from people running this in production), plack + Cache::Memory is a really bad combination for so many reasons.. I thought that only the CLI scripts like updatedatabase.pl etc. were affected.
Comment 5 Mirko Tietgen 2016-09-09 19:55:48 UTC
(In reply to Chris Cormack from comment #3)
> Have you seen bug 11921
> 
> That shifts the config back out of ENV variables. I feel like signing that
> off and getting that in will be better than bandaiding what we do now.
> 
> What do you think?

With 11921 pushed, is this obsolete now? Still needed? Needs adjustment?
Comment 6 Jonathan Druart 2016-09-12 16:01:07 UTC
(In reply to Mirko Tietgen from comment #5)
> (In reply to Chris Cormack from comment #3)
> > Have you seen bug 11921
> > 
> > That shifts the config back out of ENV variables. I feel like signing that
> > off and getting that in will be better than bandaiding what we do now.
> > 
> > What do you think?
> 
> With 11921 pushed, is this obsolete now? Still needed? Needs adjustment?

Still needed for stable branches, not master.
Comment 7 Frédéric Demians 2016-09-21 18:05:33 UTC
Created attachment 55742 [details] [review]
Bug 17262: Inject MEMCACHED_* variables into Plack's env

This patch adds two functions to koha-functions.sh to read the MEMCACHED_*
variables from configured instances' apache files (the only source for that
info for now).

It then tweaks koha-plack's start_plack() function so it exports the variables
to env before launching starman.

The patch seems trivial, but it highlights a bigger problem with our Plack integration
as the starman server seems to hang if those environment variables are defined!

Sponsored-by: ByWater Solutions
Signed-off-by: Frédéric Demians <f.demians@tamil.fr>
Comment 8 Tomás Cohen Arazi 2016-09-22 16:49:30 UTC
This patch has been pushed to 16.05.
Comment 9 Julian Maurice 2016-09-23 06:43:00 UTC
Pushed to 3.22.x, will be in 3.22.11
Comment 10 Mirko Tietgen 2016-09-30 20:17:00 UTC
This patch was reverted from the 16.05.04.01 package by request because it would break instances running Plack. I will skip it for 3.22.11 too. Please sort this out until next release, either by a followup that fixes the problem or by reverting it officially from stable and oldstable.
Comment 11 Jonathan Druart 2016-10-03 07:48:36 UTC
(In reply to Mirko Tietgen from comment #10)
> This patch was reverted from the 16.05.04.01 package by request because it
> would break instances running Plack. I will skip it for 3.22.11 too. Please
> sort this out until next release, either by a followup that fixes the
> problem or by reverting it officially from stable and oldstable.

Could you detail what is "the problem"?
Comment 12 Josef Moravec 2016-10-03 07:52:23 UTC
I think the problem is described in bug 17342
Comment 13 Mirko Tietgen 2016-10-03 08:48:39 UTC
(In reply to Jonathan Druart from comment #11)
> Could you detail what is "the problem"?

Tomás said there is a circular dependency IIRC, and asked me to remove it from 16.05.04. Which I did and uploaded a new 16.05.04.01 while I was travelling, I had no time to look into the actual problem. He should be able to explain.
Comment 14 Frédéric Demians 2016-10-03 11:11:28 UTC
It's either not clear to me whether this patch has to be removed from 16.05 or completed.
Comment 15 Jonathan Druart 2016-10-03 11:20:26 UTC
(In reply to Frédéric Demians from comment #14)
> It's either not clear to me whether this patch has to be removed from 16.05
> or completed.

It would be better to fix it, see bug 17342 for a patch.