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.
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
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).
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
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
(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 ;)
Created attachment 144961 [details] [review] Bug 32558: (follow-up) Set default max processes to 1
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?
(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.
(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.
(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.
(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.
(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 ;)
Created attachment 145043 [details] [review] Bug 32558: (follow-up) Allow configuration of max processes via env or koha conf
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.
(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.
Created attachment 145062 [details] [review] Bug 32558: (follow-up) Ack the stomp message before forking
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) """
(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.
(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.
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
(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.
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.
(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.
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?
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
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
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
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)
*-Step 5 should say parameter -m 3 instead of parameter -p 3 Apparently I can't type it correctly either!
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
(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!
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>
Looking here now
+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?
Not sure (yet) but should the default not be zero? Just do not fork like we currently do?
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 {
Changing status to reflect need for feedback
(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
(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.
(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.)
(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.
(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.
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.
(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'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.
(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.
Nothing changed, backed to FQA
Note: Kyle, you did not even respond to any of the comments !
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.
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 !
Opened bug 32990 to address the deadlock. Please signoff, Kyle !
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>
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>
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.
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>
Pushed to master for 23.05. Nice work everyone, thanks!
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.
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>
So we need libparallel-forkmanager-perl
(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.
Follow-up pushed to master. Please use the additional_work_needed
Nice work, thanks everyone! Pushed to 22.11.x for the next release.
Backported to 22.05.x for upcoming 22.05.11
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.
use C4::Context; use Koha::Logger; use Koha::BackgroundJobs; +use C4::Context; Dup C4::Context.