Bug 25778 - koha-plack puts duplicate entries into PERL5LIB when multiple instances named
Summary: koha-plack puts duplicate entries into PERL5LIB when multiple instances named
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Packaging (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: David Cook
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 26163
  Show dependency treegraph
 
Reported: 2020-06-17 04:52 UTC by David Cook
Modified: 2020-09-20 18:12 UTC (History)
6 users (show)

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


Attachments
Bug 25778: Remove install lib dir from PERL5LIB to prevent dup (1.26 KB, patch)
2020-06-17 07:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25778: [Alternate patch] Fix var scope in koha-plack for PERL5LIB and KOHA_HOME (4.78 KB, patch)
2020-08-14 02:17 UTC, David Cook
Details | Diff | Splinter Review
Bug 25778: Fix var scope in koha-plack for PERL5LIB and KOHA_HOME (4.82 KB, patch)
2020-08-14 16:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25778: Fix var scope in koha-plack for PERL5LIB and KOHA_HOME (4.89 KB, patch)
2020-08-18 11:38 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2020-06-17 04:52:54 UTC
Line 395 in koha-lack "PERL5LIB=$PERL5LIB:$KOHA_HOME/installer:$KOHA_HOME/lib/installer" causes PERL5LIB to grow with duplicate entries for /usr/share/koha/installer and /usr/share/koha/lib/installer when running in production.

Note that this does *not* happen when running in dev, when using koha-testing-docker for instance.
Comment 1 David Cook 2020-06-17 07:12:10 UTC
This is how my @INC looks like on one instance:

/usr/share/koha/lib
/usr/share/koha/installer
/usr/share/koha/lib/installer
/usr/share/koha/installer
/usr/share/koha/lib/installer
/usr/share/koha/installer
/usr/share/koha/lib/installer
/usr/share/koha/installer
/usr/share/koha/lib/installer
/usr/share/koha/installer
/usr/share/koha/lib/installer
/usr/share/koha/installer
/usr/share/koha/lib/installer
/usr/share/koha/installer
/usr/share/koha/lib/installer
/usr/share/koha/installer
/usr/share/koha/lib/installer
/usr/share/koha/installer
/usr/share/koha/lib/installer
/usr/share/koha/installer
/usr/share/koha/lib/installer
/usr/share/koha/installer
/usr/share/koha/lib/installer
/usr/share/koha/installer
/usr/share/koha/lib/installer
/usr/share/koha/installer
/usr/share/koha/lib/installer
/usr/share/koha/installer
/usr/share/koha/lib/installer
/usr/share/koha/installer
/usr/share/koha/lib/installer
/usr/share/koha/installer
/usr/share/koha/lib/installer
/usr/share/koha/installer
/usr/share/koha/lib/installer
/etc/perl
/usr/local/lib/x86_64-linux-gnu/perl/5.26.1
/usr/local/share/perl/5.26.1
/usr/lib/x86_64-linux-gnu/perl5/5.26
/usr/share/perl5
/usr/lib/x86_64-linux-gnu/perl/5.26
/usr/share/perl/5.26
/usr/local/lib/site_perl
/usr/lib/x86_64-linux-gnu/perl-base
Comment 2 Jonathan Druart 2020-06-17 07:17:38 UTC
Created attachment 105934 [details] [review]
Bug 25778: Remove install lib dir from PERL5LIB to prevent dup

In a non dev environment, PERL5LIB will contain duplicate of
$KOHA_HOME/installer and $KOHA_HOME/installer/lib

To prevent that we remove them before appending them.

Test plan:
Try the code in a shell and confirm that it works as expected.

Note: it does not happend in dev because adjust_paths_dev_install reset
PERL5LIB to KOHA_HOME
Comment 3 Tomás Cohen Arazi 2020-08-13 17:27:15 UTC
I don't understand why they accumulate.
Comment 4 David Cook 2020-08-14 00:19:22 UTC
(In reply to Tomás Cohen Arazi from comment #3)
> I don't understand why they accumulate.

The more I think about it, the less I understand as well.

I think that I was wrong about it being due to "koha-plack --restart".

I'm looking at a Koha instance where the server was rebooted last night, but it has 46 pairs of /usr/share/koha/installer and /usr/share/koha/lib/installer.

There is no way that koha-plack was restarted 45 times since last night. 

Interestingly, if I try to cat /proc/<starman PID>/environ, I just get a long blank response with no actual environmental variables inside.
Comment 5 David Cook 2020-08-14 00:24:31 UTC
(In reply to Tomás Cohen Arazi from comment #3)
> I don't understand why they accumulate.

Oh wait I see it now. It's so obvious in retrospect. Look at the scoping of PERL5LIB in the following block of code:

if [ $# -gt 0 ]; then
    # We have at least one instance name
    for name in "$@"; do

        if is_instance $name; then

            adjust_paths_dev_install $name
            export DEV_INSTALL
            export KOHA_HOME
            PERL5LIB=$PERL5LIB:$KOHA_HOME/installer:$KOHA_HOME/lib/installer

If you're specifying more than 1 instance name to koha-plack, it'll append "$KOHA_HOME/installer:$KOHA_HOME/lib/installer" to PERL5LIB for each instance.
Comment 6 Tomás Cohen Arazi 2020-08-14 01:17:02 UTC
So we need to solve it with right scoping.
Comment 7 David Cook 2020-08-14 01:19:03 UTC
(In reply to Tomás Cohen Arazi from comment #6)
> So we need to solve it with right scoping.

Exactly.

The following snippet provides a model that should work:

export THIS
test_function() {
    local THIS=blah
    echo `env`
}
test_function
echo $THIS
Comment 8 David Cook 2020-08-14 01:20:04 UTC
Or better yet...

THIS=ONE
export THIS
test_function() {
    local THIS=blah
    echo `env`
}
test_function
echo $THIS
Comment 9 David Cook 2020-08-14 01:29:51 UTC
I don't have time to write a patch right now, but I reckon we mark PERL5LIB as an export above the per-instance loop, move the per-instance logic into a function, and localize PERL5LIB within the function.
Comment 10 David Cook 2020-08-14 01:32:55 UTC
(In reply to David Cook from comment #9)
> I don't have time to write a patch right now, but I reckon we mark PERL5LIB
> as an export above the per-instance loop, move the per-instance logic into a
> function, and localize PERL5LIB within the function.

Ok maybe just one quick patch... but I won't exhaustively test it right now..
Comment 11 David Cook 2020-08-14 02:17:14 UTC
Created attachment 108221 [details] [review]
Bug 25778: [Alternate patch] Fix var scope in koha-plack for PERL5LIB and KOHA_HOME

Currently, if a koha-plack is given multiple instances, it will create
duplicate $KOHA_HOME/installer and $KOHA_HOME/installer/lib entries
in PERL5LIB as these changes are done at the global rather than local level.

This issue can only be seen in non-dev Koha, since dev Koha installs
rewrite PERL5LIB.

This patch localizes PERL5LIB to each instance.

To test dev installs:
0. cp debian/scripts/koha-plack /usr/sbin/koha-plack
1. koha-plack --restart kohadev
2. Note that nothing breaks

To test non-dev installs:
0. Don't replace /usr/sbin/koha-plack yet
1. Create multiple non-dev installs with plack enabled and running
2. Set up one of these non-dev installs so you can use the web
UI (that is beyong the scope of this test plan)
3. koha-plack --restart test1 test2 test3
4. For test3, go to /cgi-bin/koha/about.pl
5. Note that there are duplicate /usr/share/koha/installer
and /usr/share/koha/lib/installer entries in PERL5LIB
6. cp debian/scripts/koha-plack /usr/sbin/koha-plack
7. koha-plack --restart test1 test2 test3
8. For test3, go to /cgi-bin/koha/about.pl
9. Note that there are no duplicates entries in PERL5LIB
Comment 12 David Cook 2020-08-14 02:18:12 UTC
It would be good to have someone with debugger knowledge (I think that's you, Tomas?) to test my patch. I just tested it with --enable, --start, and --restart
Comment 13 Tomás Cohen Arazi 2020-08-14 14:36:34 UTC
This should be backported down to 19.11 once we agree on the fix.
Comment 14 Tomás Cohen Arazi 2020-08-14 16:06:47 UTC
Created attachment 108290 [details] [review]
Bug 25778: Fix var scope in koha-plack for PERL5LIB and KOHA_HOME

Currently, if a koha-plack is given multiple instances, it will create
duplicate $KOHA_HOME/installer and $KOHA_HOME/installer/lib entries
in PERL5LIB as these changes are done at the global rather than local level.

This issue can only be seen in non-dev Koha, since dev Koha installs
rewrite PERL5LIB.

This patch localizes PERL5LIB to each instance.

To test dev installs:
0. cp debian/scripts/koha-plack /usr/sbin/koha-plack
1. koha-plack --restart kohadev
2. Note that nothing breaks

To test non-dev installs:
0. Don't replace /usr/sbin/koha-plack yet
1. Create multiple non-dev installs with plack enabled and running
2. Set up one of these non-dev installs so you can use the web
UI (that is beyong the scope of this test plan)
3. koha-plack --restart test1 test2 test3
4. For test3, go to /cgi-bin/koha/about.pl
5. Note that there are duplicate /usr/share/koha/installer
and /usr/share/koha/lib/installer entries in PERL5LIB
6. cp debian/scripts/koha-plack /usr/sbin/koha-plack
7. koha-plack --restart test1 test2 test3
8. For test3, go to /cgi-bin/koha/about.pl
9. Note that there are no duplicates entries in PERL5LIB

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Tomás Cohen Arazi 2020-08-14 16:09:06 UTC
David's plugin does the job. I tested on a kohadevbox and printing the PERL5LIB value.

REmote debugging works as expected. About the question on the FIXME, remote debugging won't work with multiple instances at a time, I wouldn't worry about that now.
Comment 16 David Cook 2020-08-16 23:30:53 UTC
(In reply to Tomás Cohen Arazi from comment #15)
> David's plugin does the job. I tested on a kohadevbox and printing the
> PERL5LIB value.
> 
> REmote debugging works as expected. About the question on the FIXME, remote
> debugging won't work with multiple instances at a time, I wouldn't worry
> about that now.

Thanks for testing that, Tomas!
Comment 17 Kyle M Hall 2020-08-18 11:38:18 UTC
Created attachment 108472 [details] [review]
Bug 25778: Fix var scope in koha-plack for PERL5LIB and KOHA_HOME

Currently, if a koha-plack is given multiple instances, it will create
duplicate $KOHA_HOME/installer and $KOHA_HOME/installer/lib entries
in PERL5LIB as these changes are done at the global rather than local level.

This issue can only be seen in non-dev Koha, since dev Koha installs
rewrite PERL5LIB.

This patch localizes PERL5LIB to each instance.

To test dev installs:
0. cp debian/scripts/koha-plack /usr/sbin/koha-plack
1. koha-plack --restart kohadev
2. Note that nothing breaks

To test non-dev installs:
0. Don't replace /usr/sbin/koha-plack yet
1. Create multiple non-dev installs with plack enabled and running
2. Set up one of these non-dev installs so you can use the web
UI (that is beyong the scope of this test plan)
3. koha-plack --restart test1 test2 test3
4. For test3, go to /cgi-bin/koha/about.pl
5. Note that there are duplicate /usr/share/koha/installer
and /usr/share/koha/lib/installer entries in PERL5LIB
6. cp debian/scripts/koha-plack /usr/sbin/koha-plack
7. koha-plack --restart test1 test2 test3
8. For test3, go to /cgi-bin/koha/about.pl
9. Note that there are no duplicates entries in PERL5LIB

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Jonathan Druart 2020-08-18 15:46:48 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 19 Lucas Gass 2020-09-04 15:25:47 UTC
backported to 20.05.x for 20.05.04
Comment 20 Aleisha Amohia 2020-09-10 03:45:19 UTC
backported to 19.11.x for 19.11.10
Comment 21 Victor Grousset/tuxayo 2020-09-20 18:12:22 UTC
Not backported to oldoldstable (19.05.x). Feel free to ask if it's needed.