Bug 32558 - Allow background_jobs_worker.pl to process multiple jobs simultaneously up to a limit
Summary: Allow background_jobs_worker.pl to process multiple jobs simultaneously up to...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords: dependency
Depends on: 32573
Blocks:
  Show dependency treegraph
 
Reported: 2023-01-03 15:35 UTC by Kyle M Hall
Modified: 2024-01-26 13:31 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.04,22.05.11


Attachments
Bug 32558: Add ability for background_jobs_worker.pl to process multiple jobs simultaneously up to a limit (4.96 KB, patch)
2023-01-03 15:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32558: Add ability for background_jobs_worker.pl to process multiple jobs simultaneously up to a limit (2.81 KB, patch)
2023-01-03 16:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32558: Add ability for background_jobs_worker.pl to process multiple jobs simultaneously up to a limit (2.86 KB, patch)
2023-01-03 19:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32558: (follow-up) Set default max processes to 1 (742 bytes, patch)
2023-01-03 22:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32558: (follow-up) Allow configuration of max processes via env or koha conf (2.26 KB, patch)
2023-01-04 19:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32558: (follow-up) Ack the stomp message before forking (1.00 KB, patch)
2023-01-05 14:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32558: Add ability for background_jobs_worker.pl to process multiple jobs simultaneously up to a limit (4.27 KB, patch)
2023-01-05 16:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32558: Add ability for background_jobs_worker.pl to process multiple jobs simultaneously up to a limit (3.78 KB, patch)
2023-01-31 17:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32558: Add ability for background_jobs_worker.pl to process multiple jobs simultaneously up to a limit (3.74 KB, patch)
2023-01-31 17:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32558: Add ability for background_jobs_worker.pl to process multiple jobs simultaneously up to a limit (4.56 KB, patch)
2023-02-01 14:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32558: Add ability for background_jobs_worker.pl to process multiple jobs simultaneously up to a limit (5.07 KB, patch)
2023-02-02 16:50 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 32558: Add ability for background_jobs_worker.pl to process multiple jobs simultaneously up to a limit (5.16 KB, patch)
2023-02-17 08:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32558: (QA follow-up) Leave default to 1, remove extra fork (1.07 KB, patch)
2023-02-17 08:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32558: (QA follow-up) Leave default to 1, remove extra fork (1.13 KB, patch)
2023-03-06 13:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32558: (QA follow-up) Move ForkManager to required (1.19 KB, patch)
2023-03-07 12:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2023-01-03 15:35:38 UTC
Right now background_jobs_worker.pl only processes jobs in serial. It would make sense to handle jobs in parallel up to a user definable limit.
Comment 1 Kyle M Hall 2023-01-03 15:43:56 UTC
Created attachment 144946 [details] [review]
Bug 32558: Add ability for background_jobs_worker.pl to process multiple jobs simultaneously up to a limit

Right now background_jobs_worker.pl only processes jobs in serial. It would make sense to handle jobs in parallel up to a user definable limit.

Test Plan:
1) Apply this patch
2) Stop background_jobs_worker.pl
3) Generate some background jobs by editing records, placing holds, etc
4) Watch processes in a new terminal: watch -n 0.1 'ps aux | grep background_jobs_worker.pl'
5) Run background_jobs_worker.pl
6) Note the multiple forked processes in the ps output
Comment 2 Jonathan Druart 2023-01-03 15:55:27 UTC
Hi Kyle, you embedded unrelated changes (sip).
This would make sense in some cases (job types), but I think it should default to 1 to keep the existing behaviour (we don't want 10 long tasks/batch mod running at the same time).
Comment 3 Kyle M Hall 2023-01-03 16:24:47 UTC
Created attachment 144947 [details] [review]
Bug 32558: Add ability for background_jobs_worker.pl to process multiple jobs simultaneously up to a limit

Right now background_jobs_worker.pl only processes jobs in serial. It would make sense to handle jobs in parallel up to a user definable limit.

Test Plan:
1) Apply this patch
2) Stop background_jobs_worker.pl
3) Generate some background jobs by editing records, placing holds, etc
4) Watch processes in a new terminal: watch -n 0.1 'ps aux | grep background_jobs_worker.pl'
5) Run background_jobs_worker.pl
6) Note the multiple forked processes in the ps output
Comment 4 Kyle M Hall 2023-01-03 19:49:58 UTC
Created attachment 144960 [details] [review]
Bug 32558: Add ability for background_jobs_worker.pl to process multiple jobs simultaneously up to a limit

Right now background_jobs_worker.pl only processes jobs in serial. It would make sense to handle jobs in parallel up to a user definable limit.

Test Plan:
1) Apply this patch
2) Stop background_jobs_worker.pl
3) Generate some background jobs by editing records, placing holds, etc
4) Watch processes in a new terminal: watch -n 0.1 'ps aux | grep background_jobs_worker.pl'
5) Run background_jobs_worker.pl
6) Note the multiple forked processes in the ps output
Comment 5 Kyle M Hall 2023-01-03 22:44:28 UTC
(In reply to Jonathan Druart from comment #2)
> Hi Kyle, you embedded unrelated changes (sip).
> This would make sense in some cases (job types), but I think it should
> default to 1 to keep the existing behaviour (we don't want 10 long
> tasks/batch mod running at the same time).

Yep, that was an accident. It just took me a few minutes to untangle ;)
Comment 6 Kyle M Hall 2023-01-03 22:45:58 UTC
Created attachment 144961 [details] [review]
Bug 32558: (follow-up) Set default max processes to 1
Comment 7 David Cook 2023-01-03 23:01:45 UTC
An alternative would be to launch more background_jobs_worker.pl processes, although that would require updating the koha-worker script.

Speaking of the koha-worker script, how would the max processes be configured for it?

Perhaps something in debian/koha-common.init that can be overridden in /etc/default/koha-common?
Comment 8 Jonathan Druart 2023-01-04 08:26:06 UTC
(In reply to David Cook from comment #7)
> An alternative would be to launch more background_jobs_worker.pl processes,
> although that would require updating the koha-worker script.

Yes, that's exactly what I thought later yesterday night. Why should we need this flag instead of letting sysops start the number of workers they want?

The first argument could be the memory footprint, but with bug 28413 it should not be a problem.
Comment 9 Jonathan Druart 2023-01-04 08:35:15 UTC
(In reply to Jonathan Druart from comment #8)
> The first argument could be the memory footprint, but with bug 28413 it
> should not be a problem.

And just noticed that it's broken, see bug 32561.
Comment 10 Kyle M Hall 2023-01-04 11:29:32 UTC
(In reply to David Cook from comment #7)
> An alternative would be to launch more background_jobs_worker.pl processes,
> although that would require updating the koha-worker script.

Would that work with the database polling method? I think that would require some re-engineering.
Comment 11 Kyle M Hall 2023-01-04 15:30:58 UTC
(In reply to Jonathan Druart from comment #9)
> (In reply to Jonathan Druart from comment #8)
> > The first argument could be the memory footprint, but with bug 28413 it
> > should not be a problem.
> 
> And just noticed that it's broken, see bug 32561.

I had another thought triggered by bug 32561. Your solution on that bug only keeps the memory footprint down until the first time that the require for Koha::Plugins is evaluated. At that time the memory usage will jump back up and remain there. With this patch, those inflated processes will go away after processing that job. I think both techniques should be implemented for maximum flexibility. For example, someone may want to deploy a fast queue with 20 max processes, and a slow queue with 10 max processes.
Comment 12 Kyle M Hall 2023-01-04 15:32:23 UTC
(In reply to Kyle M Hall from comment #11)
> (In reply to Jonathan Druart from comment #9)
> > (In reply to Jonathan Druart from comment #8)
> > > The first argument could be the memory footprint, but with bug 28413 it
> > > should not be a problem.
> > 
> > And just noticed that it's broken, see bug 32561.
> 
> I had another thought triggered by bug 32561. Your solution on that bug only
> keeps the memory footprint down until the first time that the require for
> Koha::Plugins is evaluated. At that time the memory usage will jump back up
> and remain there. With this patch, those inflated processes will go away
> after processing that job. I think both techniques should be implemented for
> maximum flexibility. For example, someone may want to deploy a fast queue
> with 20 max processes, and a slow queue with 10 max processes.

Just remembered that the process forks, so I think my memory concern is invalid. I do think the rest of my comment stands though ;)
Comment 13 Kyle M Hall 2023-01-04 19:22:02 UTC
Created attachment 145043 [details] [review]
Bug 32558: (follow-up) Allow configuration of max processes via env or koha conf
Comment 14 Jonathan Druart 2023-01-05 13:18:03 UTC
From https://www.grzegorowski.com/rabbitmq-406-channel-closed-precondition-failed

"You have to ACK messages in same order as they arrive to your system"

If this is correct, forking is not an option here.

I have not found something similar in rabbitmq doc however.
Comment 15 Kyle M Hall 2023-01-05 14:20:38 UTC
(In reply to Jonathan Druart from comment #14)
> From
> https://www.grzegorowski.com/rabbitmq-406-channel-closed-precondition-failed
> 
> "You have to ACK messages in same order as they arrive to your system"
> 
> If this is correct, forking is not an option here.
> 
> I have not found something similar in rabbitmq doc however.

You are correct. I've been reading a lot about ACK messages, and we are using them wrong. ACK isn't suppose to indicate a process has completed. It's only meant to indicate the a message has been received. In fact, sending ACK messages appears to be optional.

This is from the O'Reilly book Mobile and Web Messaging:
---
By default, the STOMP broker will consider the message automatically acknowledged when it is delivered to the consumer.

However, there are cases in which the consumer may prefer to explicitly acknowledge the message. It leaves a window of opportunity to determine whether it can handle the message or not. For example, the client needs to write the message payload in a data store. There may be issues with opening a connection to the data store and the client could choose to acknowledge the message only after having successfully written its body to the data store. In case of failure, it will instead nack the message (explicitly refuse to take ownership of it). When the STOMP broker is informed of this negative acknowledgment, it may then decide to deliver the message to another consumer subscribed to the destination or try again some time later depending on its configuration.
---

I will submit a following so that we ACK the message before forking.
Comment 16 Kyle M Hall 2023-01-05 14:22:41 UTC
Created attachment 145062 [details] [review]
Bug 32558: (follow-up) Ack the stomp message before forking
Comment 17 Jonathan Druart 2023-01-05 15:26:17 UTC
This contradicts https://metacpan.org/pod/Net::Stomp#ack

"""
This acknowledges that you have received and processed a frame and all frames before it (if you are using client acknowledgements)
"""
Comment 18 Kyle M Hall 2023-01-05 16:21:20 UTC
(In reply to Jonathan Druart from comment #17)
> This contradicts https://metacpan.org/pod/Net::Stomp#ack
> 
> """
> This acknowledges that you have received and processed a frame and all
> frames before it (if you are using client acknowledgements)
> """

That interpretation contradicts the STOMP protocol specification: https://stomp.github.io/stomp-specification-1.2.html#ACK

I think the problem here lay in the vagueness of the phrase "processed a frame". That doesn't mean "completed a task". It simply means the frame was successfully read by the client, and that we were able to decode the data within it, and nothing more.
Comment 19 Kyle M Hall 2023-01-05 16:33:54 UTC
(In reply to Kyle M Hall from comment #18)
> (In reply to Jonathan Druart from comment #17)
> > This contradicts https://metacpan.org/pod/Net::Stomp#ack
> > 
> > """
> > This acknowledges that you have received and processed a frame and all
> > frames before it (if you are using client acknowledgements)
> > """
> 
> That interpretation contradicts the STOMP protocol specification:
> https://stomp.github.io/stomp-specification-1.2.html#ACK
> 
> I think the problem here lay in the vagueness of the phrase "processed a
> frame". That doesn't mean "completed a task". It simply means the frame was
> successfully read by the client, and that we were able to decode the data
> within it, and nothing more.

Moving this discussion and patch to Bug 32573 to avoid scope creep.
Comment 20 Kyle M Hall 2023-01-05 16:40:53 UTC
Created attachment 145067 [details] [review]
Bug 32558: Add ability for background_jobs_worker.pl to process multiple jobs simultaneously up to a limit

Right now background_jobs_worker.pl only processes jobs in serial. It would make sense to handle jobs in parallel up to a user definable limit.

Test Plan:
1) Apply this patch
2) Stop background_jobs_worker.pl
3) Generate some background jobs by editing records, placing holds, etc
4) Watch processes in a new terminal: watch -n 0.1 'ps aux | grep background_jobs_worker.pl'
5) Run background_jobs_worker.pl with parameter -p 3 or some other
   number of max processes
6) Note the multiple forked processes in the ps output
Comment 21 David Cook 2023-01-08 23:57:57 UTC
(In reply to Jonathan Druart from comment #14)
> From
> https://www.grzegorowski.com/rabbitmq-406-channel-closed-precondition-failed
> 
> "You have to ACK messages in same order as they arrive to your system"
> 
> If this is correct, forking is not an option here.

Or rather, Kyle's patch is just forking at the wrong point.

I've just reviewed the patches again, and I notice that this is a forking job server style, but we need a pre-forking job server style. 

That is, following GetOptions, we should fork, and each child process gets its own connection with RabbitMQ. Then each child process ACKs the messages in the same order it receives on its own separate connection (and channel).

That way, it's functionally the same thing as running 4 separate background job worker processes.
Comment 22 David Cook 2023-01-09 01:19:20 UTC
Was tempted to Fail QA this one, but I haven't actually tested if the current patch won't work in practice. 

But I think moving the forking up as I describe above would be a wise move.
Comment 23 David Cook 2023-01-09 05:11:01 UTC
(In reply to Kyle M Hall from comment #10)
> Would that work with the database polling method? I think that would require
> some re-engineering.

Upon review, that won't work with having multiple pre-forked processes either.
Comment 24 David Cook 2023-01-09 23:31:33 UTC
What happens if you have a background_jobs_worker.pl running 4 child processes, they all ACK early, and then 1 child process crashes?

That job will be stuck as "new" or "started" until the background_jobs_worker.pl is restarted and does some database polling, yes?
Comment 25 Emily Lamancusa 2023-01-30 21:26:13 UTC
The discussion on ACK early / ACK late seems to have been settled for now in bug 32573, so is it fair to conclude that we're ready to move forward with this one?

If so, this patch needs to be rebased.

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 32558: Add ability for background_jobs_worker.pl to process multiple jobs simultaneously up to a limit
error: sha1 information is lacking or useless (misc/background_jobs_worker.pl).
error: could not build fake ancestor
Patch failed at 0001 Bug 32558: Add ability for background_jobs_worker.pl to process multiple jobs simultaneously up to a limit
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-32558-Add-ability-for-backgroundjobsworkerpl-t-cxrOEt.patch
Comment 26 Kyle M Hall 2023-01-31 17:18:43 UTC
Created attachment 145900 [details] [review]
Bug 32558: Add ability for background_jobs_worker.pl to process multiple jobs simultaneously up to a limit

Right now background_jobs_worker.pl only processes jobs in serial. It would make sense to handle jobs in parallel up to a user definable limit.

Test Plan:
1) Apply this patch
2) Stop background_jobs_worker.pl
3) Generate some background jobs by editing records, placing holds, etc
4) Watch processes in a new terminal: watch -n 0.1 'ps aux | grep background_jobs_worker.pl'
5) Run background_jobs_worker.pl with parameter -p 3 or some other
   number of max processes
6) Note the multiple forked processes in the ps output
Comment 27 Kyle M Hall 2023-01-31 17:19:46 UTC
Created attachment 145901 [details] [review]
Bug 32558: Add ability for background_jobs_worker.pl to process multiple jobs simultaneously up to a limit

Right now background_jobs_worker.pl only processes jobs in serial. It would make sense to handle jobs in parallel up to a user definable limit.

Test Plan:
1) Apply this patch
2) Stop background_jobs_worker.pl
3) Generate some background jobs by editing records, placing holds, etc
4) Watch processes in a new terminal: watch -n 0.1 'ps aux | grep background_jobs_worker.pl'
5) Run background_jobs_worker.pl with parameter -p 3 or some other
   number of max processes
6) Note the multiple forked processes in the ps output
Comment 28 Emily Lamancusa 2023-01-31 22:30:34 UTC
Test notes:

-Step 5 should say parameter -m 3 instead of parameter -d 3

-The above should also be documented in the help section of the script

-Is the process limit still meant to be configurable via configuration files? If so, that should be added to the test plan. If it's meant to be a command-line option only, then should the <background_jobs_worker> sections of koha-conf.xml and koha-conf-site.xml.in be removed from the patch?
(I don't really know much about those files yet, so hopefully that question makes sense)
Comment 29 Emily Lamancusa 2023-01-31 22:32:36 UTC
*-Step 5 should say parameter -m 3 instead of parameter -p 3

Apparently I can't type it correctly either!
Comment 30 Kyle M Hall 2023-02-01 14:35:46 UTC
Created attachment 145950 [details] [review]
Bug 32558: Add ability for background_jobs_worker.pl to process multiple jobs simultaneously up to a limit

Right now background_jobs_worker.pl only processes jobs in serial. It would make sense to handle jobs in parallel up to a user definable limit.

Test Plan:
1) Apply this patch
2) Stop background_jobs_worker.pl
3) Generate some background jobs by editing records, placing holds, etc
4) Watch processes in a new terminal: watch -n 0.1 'ps aux | grep background_jobs_worker.pl'
5) Run background_jobs_worker.pl with parameter -m 3 or some other
   number of max processes
6) Note the multiple forked processes in the ps output
Comment 31 Kyle M Hall 2023-02-01 14:36:19 UTC
(In reply to Emily Lamancusa from comment #29)
> *-Step 5 should say parameter -m 3 instead of parameter -p 3
> 
> Apparently I can't type it correctly either!

Thanks! I've updated the test plan in the patch!
Comment 32 Emily Lamancusa 2023-02-02 16:50:28 UTC
Created attachment 146020 [details] [review]
Bug 32558: Add ability for background_jobs_worker.pl to process multiple jobs simultaneously up to a limit

Right now background_jobs_worker.pl only processes jobs in serial. It would make sense to handle jobs in parallel up to a user definable limit.

Test Plan:
1) Apply this patch
2) Stop background_jobs_worker.pl
3) Generate some background jobs by editing records, placing holds, etc
4) Watch processes in a new terminal: watch -n 0.1 'ps aux | grep background_jobs_worker.pl'
5) Run background_jobs_worker.pl with parameter -m 3 or some other
   number of max processes
6) Note the multiple forked processes in the ps output

Test notes - also tested the following on KTD:
1. Stop background_jobs_worker.pl
2. Edit /etc/koha/sites/kohadev/koha-conf.xml - set max_processes to 10
3. Generate some background jobs
4. Watch processes in a new terminal: watch -n 0.1 'ps aux | grep background_jobs_worker.pl'
5. Restart all
6. Confirm multiple forked processes in the ps output

Both methods work as expected and generate multiple forked processes
based on the value set for max processes.

Signed-off-by: emlam <emily.lamancusa@montgomerycountymd.gov>
Comment 33 Marcel de Rooy 2023-02-03 10:10:01 UTC
Looking here now
Comment 34 Marcel de Rooy 2023-02-03 10:20:26 UTC
+Max processes will be set from the command line option, the environment variable MAX_PROCESSES, or the koha-conf file, in that order of precedence.
+By default the script will only run one job at a time.

I think this is not needed. Confusing too.
It is already simpler if we remove the koha-conf option here.
I would opt for commandline option or default 1. Setting MAX_PROCESSES or --max_processes is the same amount of work?
Comment 35 Marcel de Rooy 2023-02-03 10:28:10 UTC
Not sure (yet) but should the default not be zero? Just do not fork like we currently do?
Comment 36 Marcel de Rooy 2023-02-03 10:31:40 UTC
Theoretical add a wait_all_children:

diff --git a/misc/background_jobs_worker.pl b/misc/background_jobs_worker.pl
index 1b2d6309d9..89f51042cf 100755
--- a/misc/background_jobs_worker.pl
+++ b/misc/background_jobs_worker.pl
@@ -156,6 +156,7 @@ while (1) {
         sleep 10;
     }
 }
+$pm->wait_all_children;
 $conn->disconnect;

 sub process_job {
Comment 37 Marcel de Rooy 2023-02-03 10:32:09 UTC
Changing status to reflect need for feedback
Comment 38 Marcel de Rooy 2023-02-03 10:32:35 UTC
(In reply to Marcel de Rooy from comment #34)
> +Max processes will be set from the command line option, the environment
> variable MAX_PROCESSES, or the koha-conf file, in that order of precedence.
> +By default the script will only run one job at a time.
> 
> I think this is not needed. Confusing too.
> It is already simpler if we remove the koha-conf option here.
> I would opt for commandline option or default 1. Setting MAX_PROCESSES or
> --max_processes is the same amount of work?

default 0
Comment 39 David Cook 2023-02-05 22:17:54 UTC
(In reply to Marcel de Rooy from comment #34)
> +Max processes will be set from the command line option, the environment
> variable MAX_PROCESSES, or the koha-conf file, in that order of precedence.
> +By default the script will only run one job at a time.
> 
> I think this is not needed. Confusing too.
> It is already simpler if we remove the koha-conf option here.
> I would opt for commandline option or default 1. Setting MAX_PROCESSES or
> --max_processes is the same amount of work?

If you wanted to set different options for different instances, koha-conf would be the only option that would work.
Comment 40 David Cook 2023-02-05 22:19:21 UTC
(In reply to Marcel de Rooy from comment #35)
> Not sure (yet) but should the default not be zero? Just do not fork like we
> currently do?

The job needs to run in a forked process. The memory used by the job can be reclaimed when the child process ends - whereas the memory will never be reclaimed by the OS if the process doesn't fork. (The memory can be re-used if it's internally freed within Perl but can't be reclaimed by the OS.)
Comment 41 Marcel de Rooy 2023-02-06 06:34:40 UTC
(In reply to David Cook from comment #40)
> (In reply to Marcel de Rooy from comment #35)
> > Not sure (yet) but should the default not be zero? Just do not fork like we
> > currently do?
> 
> The job needs to run in a forked process. The memory used by the job can be
> reclaimed when the child process ends - whereas the memory will never be
> reclaimed by the OS if the process doesn't fork. (The memory can be re-used
> if it's internally freed within Perl but can't be reclaimed by the OS.)

Probably you misread my comment ;) My formulations are compact.
Note that this patch leaves the original fork in the process_job sub. To keep current behavior (parent process, 1 fork in process_job) the default should be zero. Not one.
"Just do not fork" refers to the extra fork that we introduce here around calling process_job. And that actually introduces a double fork that we might want to get rid of too..
So the default could be 1 IF we remove the fork in the sub.
Comment 42 Marcel de Rooy 2023-02-06 06:42:28 UTC
(In reply to David Cook from comment #39)
> (In reply to Marcel de Rooy from comment #34)
> > +Max processes will be set from the command line option, the environment
> > variable MAX_PROCESSES, or the koha-conf file, in that order of precedence.
> > +By default the script will only run one job at a time.
> > 
> > I think this is not needed. Confusing too.
> > It is already simpler if we remove the koha-conf option here.
> > I would opt for commandline option or default 1. Setting MAX_PROCESSES or
> > --max_processes is the same amount of work?
> 
> If you wanted to set different options for different instances, koha-conf
> would be the only option that would work.

Okay. That would be an easier solution for that scenario. (We could even add a pref, haha.) If it will be really used like that, is still a question for me though. So, lets keep all options then, no blocker.
Comment 43 Jonathan Druart 2023-02-06 16:14:07 UTC
Could you remind me why we need this? Why don't you just run another worker? This area has not been very stable in the last releases and I am not sure we should play with more forking and introduce possible regressions when an alternative (run more workers) is already available.
Comment 44 David Cook 2023-02-06 22:21:50 UTC
(In reply to Jonathan Druart from comment #43)
> Could you remind me why we need this? Why don't you just run another worker?
> This area has not been very stable in the last releases and I am not sure we
> should play with more forking and introduce possible regressions when an
> alternative (run more workers) is already available.

I could be wrong but I think the premise of this change is that it would be more difficult to update koha-worker and koha-common.init to allow for a configurable number of workers than it would be to add more forks.

However, I agree that it would be preferable to just run more workers. Even if it's easier to fork, I don't think this patch goes about it quite the right way...
Comment 45 Emily Lamancusa 2023-02-10 15:16:10 UTC
I'm upping the severity of this bug as well as bug 32572, because being limited to 2 background worker processes is causing us some pretty significant issues with the volume of background jobs we generate - we've had to turn the real time holds queue off because the workers couldn't keep up.
Comment 46 Kyle M Hall 2023-02-16 18:10:58 UTC
(In reply to David Cook from comment #44)
> (In reply to Jonathan Druart from comment #43)
> > Could you remind me why we need this? Why don't you just run another worker?
> > This area has not been very stable in the last releases and I am not sure we
> > should play with more forking and introduce possible regressions when an
> > alternative (run more workers) is already available.
> 
> I could be wrong but I think the premise of this change is that it would be
> more difficult to update koha-worker and koha-common.init to allow for a
> configurable number of workers than it would be to add more forks.
> 
> However, I agree that it would be preferable to just run more workers. Even
> if it's easier to fork, I don't think this patch goes about it quite the
> right way...

I think these are both great ideas, and there is no reason we can't have both :)

That way we can choose how many workers we have for each queue, and how many parallel processes each worker can handle.
Comment 47 Marcel de Rooy 2023-02-17 07:03:49 UTC
Nothing changed, backed to FQA
Comment 48 Marcel de Rooy 2023-02-17 07:14:04 UTC
Note: Kyle, you did not even respond to any of the comments !
Comment 49 Marcel de Rooy 2023-02-17 08:00:39 UTC
When I submit 4 stage marc imports with 3 processes (removing the extra fork in process_job), I get this:

285042 at /usr/share/koha/misc/background_jobs_worker.pl line 93.
285043 82 at /usr/share/koha/misc/background_jobs_worker.pl line 136.
285044 83 at /usr/share/koha/misc/background_jobs_worker.pl line 136.
285045 84 at /usr/share/koha/misc/background_jobs_worker.pl line 136.
C4::ImportBatch::_update_batch_record_counts(): DBI Exception: DBD::mysql::st execute failed: Deadlock found when trying to get lock; try restarting transaction at /usr/share/koha/C4/ImportBatch.pm line 392
C4::ImportBatch::_update_batch_record_counts(): DBI Exception: DBD::mysql::st execute failed: Deadlock found when trying to get lock; try restarting transaction at /usr/share/koha/C4/ImportBatch.pm line 392
285115 85 at /usr/share/koha/misc/background_jobs_worker.pl line 136.

42 is the pid of the main process; it starts 43, 44 and 45 for three jobs. Job 82 finishes, but 83 and 84 fail concurrently on a deadlock.
Job 85, the fourth import, starts later (which is fine) and finishes ok.

So, this is not a problem of background worker itself, but the transaction in StageMarcForImport does not (yet) support this operation concurrently.
Comment 50 Marcel de Rooy 2023-02-17 08:27:17 UTC
Refactored the code in ImportBatch, new results:

286505 at /usr/share/koha/misc/background_jobs_worker.pl line 93.
286506 90 at /usr/share/koha/misc/background_jobs_worker.pl line 136.
286507 91 at /usr/share/koha/misc/background_jobs_worker.pl line 136.
286508 92 at /usr/share/koha/misc/background_jobs_worker.pl line 136.
286567 93 at /usr/share/koha/misc/background_jobs_worker.pl line 136.

| 90 | finished | 2023-02-17 08:16:14 | 2023-02-17 08:17:14 |
| 91 | finished | 2023-02-17 08:16:14 | 2023-02-17 08:17:14 |
| 92 | finished | 2023-02-17 08:16:14 | 2023-02-17 08:17:14 |
| 93 | finished | 2023-02-17 08:17:17 | 2023-02-17 08:18:17 |

This looks good !
Comment 51 Marcel de Rooy 2023-02-17 08:41:26 UTC
Opened bug 32990 to address the deadlock.
Please signoff, Kyle !
Comment 52 Marcel de Rooy 2023-02-17 08:43:52 UTC
Created attachment 146753 [details] [review]
Bug 32558: Add ability for background_jobs_worker.pl to process multiple jobs simultaneously up to a limit

Right now background_jobs_worker.pl only processes jobs in serial. It would make sense to handle jobs in parallel up to a user definable limit.

Test Plan:
1) Apply this patch
2) Stop background_jobs_worker.pl
3) Generate some background jobs by editing records, placing holds, etc
4) Watch processes in a new terminal: watch -n 0.1 'ps aux | grep background_jobs_worker.pl'
5) Run background_jobs_worker.pl with parameter -m 3 or some other
   number of max processes
6) Note the multiple forked processes in the ps output

Test notes - also tested the following on KTD:
1. Stop background_jobs_worker.pl
2. Edit /etc/koha/sites/kohadev/koha-conf.xml - set max_processes to 10
3. Generate some background jobs
4. Watch processes in a new terminal: watch -n 0.1 'ps aux | grep background_jobs_worker.pl'
5. Restart all
6. Confirm multiple forked processes in the ps output

Both methods work as expected and generate multiple forked processes
based on the value set for max processes.

Signed-off-by: emlam <emily.lamancusa@montgomerycountymd.gov>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 53 Marcel de Rooy 2023-02-17 08:43:56 UTC
Created attachment 146754 [details] [review]
Bug 32558: (QA follow-up) Leave default to 1, remove extra fork

The default of 1 resembles the old behavior: 1 fork for the job.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 54 Marcel de Rooy 2023-02-17 08:51:21 UTC
See former comments.
I am passing QA on this patch set, while noting the concerns of Jonathan. This allows for more processes in the script, but cannot be set via koha-worker which could be done on a follow-up.
Added a new report to address deadlock problem when running stage import concurrently.

And in general: Please respond to comments and do not change the status back if you did not provide any feedback or code changes. It does not feel good.
Comment 55 Kyle M Hall 2023-03-06 13:26:49 UTC
Created attachment 147784 [details] [review]
Bug 32558: (QA follow-up) Leave default to 1, remove extra fork

The default of 1 resembles the old behavior: 1 fork for the job.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 56 Tomás Cohen Arazi 2023-03-06 17:04:55 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 57 Martin Renvoize 2023-03-06 18:34:16 UTC
Should we have moved the Parallel::ForkManager dependency from 'recommends' to 'requires' in this patchset?  Prior to now, it looks like it was only used in tests.
Comment 58 Marcel de Rooy 2023-03-07 12:15:50 UTC
Created attachment 147828 [details] [review]
Bug 32558: (QA follow-up) Move ForkManager to required

Thx Martin Renvoize for noting this.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 59 Marcel de Rooy 2023-03-07 12:18:04 UTC
So we need libparallel-forkmanager-perl
Comment 60 Tomás Cohen Arazi 2023-03-07 13:56:50 UTC
(In reply to Martin Renvoize from comment #57)
> Should we have moved the Parallel::ForkManager dependency from 'recommends'
> to 'requires' in this patchset?  Prior to now, it looks like it was only
> used in tests.

Oh, good catch.
Comment 61 Tomás Cohen Arazi 2023-03-07 13:58:16 UTC
Follow-up pushed to master. Please use the additional_work_needed
Comment 62 Jacob O'Mara 2023-03-07 18:27:51 UTC
Nice work, thanks everyone!

Pushed to 22.11.x for the next release.
Comment 63 Lucas Gass 2023-03-10 17:18:47 UTC
Backported to 22.05.x for upcoming 22.05.11
Comment 64 Arthur Suzuki 2023-03-29 14:33:36 UTC
I tried to fix conflicts when trying to apply to 21.11 but I don't feel super confident for testing + this looks more like an enhancement than a bug to me.
Won't backport unless there is a backport provided.
Comment 65 Jonathan Druart 2024-01-26 13:31:54 UTC
 use C4::Context;
 use Koha::Logger;
 use Koha::BackgroundJobs;
+use C4::Context;

Dup C4::Context.