Bug 32561

Summary: background job worker is still running with all the modules in RAM
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: major    
Priority: P5 - low CC: arthur.suzuki, dcook, lucas, m.de.rooy, martin.renvoize, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32558
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.03,22.05.10
Bug Depends on: 28413, 30410    
Bug Blocks:    
Attachments: Bug 32561: Prevent worker to run with unecessary modules in memory
Bug 32561: Add test
Bug 32561: Add test
Bug 32561: Prevent worker to run with unecessary modules in memory
Bug 32561: Add test
Bug 32561: Test loaded modules for background_jobs_worker.pl
Bug 32561: Prevent worker to run with unecessary modules in memory

Description Jonathan Druart 2023-01-04 08:34:46 UTC
Regression of bug 28413, bug 30410 added a "use Koha::Plugins" statement at the top of Koha::BackgroundJob, and so all Koha module are loaded by the worker on startup.
Comment 1 Jonathan Druart 2023-01-04 09:09:19 UTC
Created attachment 144973 [details] [review]
Bug 32561: Prevent worker to run with unecessary modules in memory

Regression of bug 28413, bug 30410 added a "use Koha::Plugins" statement at the top of Koha::BackgroundJob, and so all Koha module are loaded by the worker on startup.

See bug 28413 for more info
Comment 2 Jonathan Druart 2023-01-04 09:15:42 UTC
Created attachment 144974 [details] [review]
Bug 32561: Add test

This test does not seem reliable enough, how could we improve it?

I guess it will fail randomly, especially if ran in parallel with other
processes/tests.
Comment 3 Jonathan Druart 2023-01-04 09:16:43 UTC
Created attachment 144975 [details] [review]
Bug 32561: Add test

This test does not seem reliable enough, how could we improve it?

I guess it will fail randomly, especially if ran in parallel with other
processes/tests.
Comment 4 Kyle M Hall 2023-01-04 11:53:34 UTC
Created attachment 144984 [details] [review]
Bug 32561: Prevent worker to run with unecessary modules in memory

Regression of bug 28413, bug 30410 added a "use Koha::Plugins" statement at the top of Koha::BackgroundJob, and so all Koha module are loaded by the worker on startup.

See bug 28413 for more info

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 5 Kyle M Hall 2023-01-04 11:53:45 UTC
Created attachment 144985 [details] [review]
Bug 32561: Add test

This test does not seem reliable enough, how could we improve it?

I guess it will fail randomly, especially if ran in parallel with other
processes/tests.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 6 Kyle M Hall 2023-01-04 11:53:49 UTC
Created attachment 144986 [details] [review]
Bug 32561: Test loaded modules for background_jobs_worker.pl

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 7 Marcel de Rooy 2023-01-13 10:28:26 UTC
xt/memory_check.t
bash: xt/memory_check.t: Permission denied
root@master:/usr/share/koha# chmod 755 xt/memory_check.t

Argument "/usr/share/koha/misc/background_jobs_worker.pl" isn't numeric in numeric gt (>) at xt/memory_check.t line 31.
ok 1 - background_jobs_worker.pl is consuming /usr/share/koha/misc/background_jobs_worker.pl in memory

Warning needs a fix? The test description of A consumes A is weird.

I would personally only go for pushing the first patch. The two others seem not really needed. But thats just my first impression..
Comment 8 Jonathan Druart 2023-01-13 10:39:16 UTC
(In reply to Marcel de Rooy from comment #7)
> I would personally only go for pushing the first patch. The two others seem
> not really needed. But thats just my first impression..

Yes, same feeling for me. But this problem will come up again if there is no test :-/
Comment 9 David Cook 2023-01-16 04:12:24 UTC
(In reply to Jonathan Druart from comment #8)
> (In reply to Marcel de Rooy from comment #7)
> > I would personally only go for pushing the first patch. The two others seem
> > not really needed. But thats just my first impression..
> 
> Yes, same feeling for me. But this problem will come up again if there is no
> test :-/

That's possible although I think plugin support was one of the last pieces we needed to add? 

Perhaps it would be an idea to add some text to the "use <module>" area like "DO NOT ADD KOHA MODULES HERE OR YOU WILL FACE JONATHAN'S WRATH! See BZ 32561!"
Comment 10 David Cook 2023-01-16 04:18:36 UTC
Like I asked in https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30410#c18, do we have any example plugins that we can use to verify this change?

I don't see any obvious ones listed at https://wiki.koha-community.org/wiki/Koha_plugins
Comment 11 Marcel de Rooy 2023-01-24 12:06:43 UTC
(In reply to Marcel de Rooy from comment #7)
> xt/memory_check.t
> bash: xt/memory_check.t: Permission denied
> root@master:/usr/share/koha# chmod 755 xt/memory_check.t

Please fix

> Argument "/usr/share/koha/misc/background_jobs_worker.pl" isn't numeric in
> numeric gt (>) at xt/memory_check.t line 31.

Please check.

> ok 1 - background_jobs_worker.pl is consuming
> /usr/share/koha/misc/background_jobs_worker.pl in memory
> Warning needs a fix? The test description of A consumes A is weird.

Please adjust.
Comment 12 Jonathan Druart 2023-01-24 12:58:48 UTC
(In reply to Marcel de Rooy from comment #11)
> (In reply to Marcel de Rooy from comment #7)
> > xt/memory_check.t
> > bash: xt/memory_check.t: Permission denied
> > root@master:/usr/share/koha# chmod 755 xt/memory_check.t
> 
> Please fix
> 
> > Argument "/usr/share/koha/misc/background_jobs_worker.pl" isn't numeric in
> > numeric gt (>) at xt/memory_check.t line 31.
> 
> Please check.
> 
> > ok 1 - background_jobs_worker.pl is consuming
> > /usr/share/koha/misc/background_jobs_worker.pl in memory
> > Warning needs a fix? The test description of A consumes A is weird.
> 
> Please adjust.

We removed the tests here.
Comment 13 Marcel de Rooy 2023-01-24 13:56:51 UTC
Created attachment 145617 [details] [review]
Bug 32561: Prevent worker to run with unecessary modules in memory

Regression of bug 28413, bug 30410 added a "use Koha::Plugins" statement at the top of Koha::BackgroundJob, and so all Koha module are loaded by the worker on startup.

See bug 28413 for more info

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2023-01-24 13:57:28 UTC
(In reply to Jonathan Druart from comment #12)
> We removed the tests here.
Good for me.
Comment 15 Tomás Cohen Arazi 2023-01-27 18:22:01 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 16 Matt Blenkinsop 2023-01-31 14:16:18 UTC
Nice work everyone!

Pushed to stable for 22.11.x
Comment 17 Lucas Gass 2023-02-01 16:10:16 UTC
Backported to 22.05.x for upcoming 22.05.10
Comment 18 Arthur Suzuki 2023-02-17 09:24:41 UTC
applied to 21.11.x for 21.11.16
Comment 19 Arthur Suzuki 2023-02-17 09:30:44 UTC
oh, actually this one depends on 30410 which was not applied to 21.11.
Can't backport.