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: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Packaging (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor
Assignee: David Cook
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 26163
  Show dependency tree / graph
 
Reported: 2020-06-17 04:52 UTC by David Cook
Modified: 2021-06-14 21:31 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.04, 19.11.10
Circulation function:

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 (tcohen) 		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 (khall) 		Details | Diff | Splinter Review
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

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 (tcohen) 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 (tcohen) 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 (tcohen) 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 (tcohen) 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 (tcohen) 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 (khall) 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 (lukeg) 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.