Bug 32305 - Background worker doesn't check job status when received from rabbitmq
Summary: Background worker doesn't check job status when received from rabbitmq
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Marcel de Rooy
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks: 32395 34990 35092
  Show dependency treegraph
 
Reported: 2022-11-21 11:56 UTC by Didier Gautheron
Modified: 2024-01-16 11:17 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.05,22.11.12


Attachments
Bug 32305: Only process jobs in 'new' state (933 bytes, patch)
2022-11-22 14:33 UTC, Didier Gautheron
Details | Diff | Splinter Review
[ALTERNATIVE-PATCH] Bug 32305: Don't process a job if its status is not 'new' (909 bytes, patch)
2022-11-28 08:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32305: Don't process a job if its status is not 'new' (972 bytes, patch)
2022-12-02 08:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32305: Worker should ignore wrong status or queue in MQ mode (1.97 KB, patch)
2023-10-17 15:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32305: Counterpart for es_indexer_daemon.pl (1.47 KB, patch)
2023-10-18 14:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32305: Worker should ignore wrong status or queue in MQ mode (1.99 KB, patch)
2023-10-18 14:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32305: Counterpart for es_indexer_daemon.pl (1.48 KB, patch)
2023-10-18 14:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32305: Worker should ignore wrong status or queue in MQ mode (2.06 KB, patch)
2023-10-20 08:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32305: Counterpart for es_indexer_daemon.pl (1.55 KB, patch)
2023-10-20 08:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32305: Worker should ignore wrong status or queue in MQ mode (2.19 KB, patch)
2023-10-20 09:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32305: Counterpart for es_indexer_daemon.pl (1.67 KB, patch)
2023-10-20 09:20 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 Didier Gautheron 2022-11-21 11:56:47 UTC
How to reproduce :

1) stop rabbitmq and koha-worker services.
2) start koha-worker , without rabbitmq it's polling the database.
3) start rabbitmq, koha-worker is not connecting to it.
4) submit a job. A new row is add to background_jobs table and the job is enqueued in rabbitmq.
5) the job is processed by koha-worker from the database.
6) restart koha-worker, this time connection to rabbitmq succeeded 
7) koha-worker receive job message from rabbitmq and rerun it.
Comment 1 Didier Gautheron 2022-11-22 14:33:40 UTC
Created attachment 144156 [details] [review]
Bug 32305: Only process jobs in 'new' state
Comment 2 Fridolin Somers 2022-11-23 07:05:22 UTC
I've created Bug 32330 to optimize query of background_jobs_worker.pl when pooling jobs directly from database
Comment 3 Jonathan Druart 2022-11-28 08:31:35 UTC
I am not sure this is the correct place, shouldn't it be in Koha::BackgroundJob->process?
Comment 4 Jonathan Druart 2022-11-28 08:35:26 UTC
Created attachment 144278 [details] [review]
[ALTERNATIVE-PATCH] Bug 32305: Don't process a job if its status is not 'new'
Comment 5 Jonathan Druart 2022-11-28 08:35:59 UTC
Patch for discussion, will need tests if we go with it.
Comment 6 Martin Renvoize 2022-12-02 08:22:45 UTC
I think Jonathans approach is the right way to go..

Though I must admit, I found it confusing when looking at the 'start' helper method too.. that already checks status and throws an exception if the status isn't new.

Having both 'process' and 'start' methods is a tad confusing.
Comment 7 Martin Renvoize 2022-12-02 08:24:05 UTC
Created attachment 144389 [details] [review]
Bug 32305: Don't process a job if its status is not 'new'

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2022-12-02 08:27:26 UTC
Should we force a refetch from the DB just prior to checking this status.. just in case we have a stale object?
Comment 9 Fridolin Somers 2022-12-03 03:15:35 UTC
(In reply to Martin Renvoize from comment #8)
> Should we force a refetch from the DB just prior to checking this status..
> just in case we have a stale object?

Sounds like a good idea to me
Comment 10 David Cook 2022-12-04 23:03:59 UTC
(In reply to Martin Renvoize from comment #8)
> Should we force a refetch from the DB just prior to checking this status..
> just in case we have a stale object?

I suppose this goes back to what Jonathan was saying at bug 32395...
Comment 11 Marcel de Rooy 2023-01-27 09:30:48 UTC
Please tell me what we are still discussing here? I came from 30943 which is mnore or less linked to this dev.
Comment 12 Tomás Cohen Arazi 2023-01-27 10:56:54 UTC
Let's move forward with Jonathan's approach, and please add the refetch. We then make bug 32395 depend on this one, for further cleanup.
Comment 13 Jonathan Druart 2023-01-31 10:59:18 UTC
I don't understand why we would need a fetch here, and I actually think we should not get from db.

The ->process call is from the worker, which should happen right after ->find. If the object does not have a status==new then we should explode as the developer certainly messed up with something.
Comment 14 Marcel de Rooy 2023-03-03 10:28:29 UTC
Looking here
Comment 15 Marcel de Rooy 2023-03-03 10:36:11 UTC
(In reply to Jonathan Druart from comment #5)
> Patch for discussion, will need tests if we go with it.

This speaks for itself :)
Comment 16 Marcel de Rooy 2023-03-03 10:46:55 UTC
How much sense does it make to check the status in the base object if most background jobs override it with their own?
Hmm, looks like we always start in the base object and go to derived class..

And note that derived often begins with a ->start that contains an exception already for a status <> new.

Should we add the start in the general process instead ?
Comment 17 Marcel de Rooy 2023-03-03 10:48:21 UTC
(In reply to Jonathan Druart from comment #13)
> I don't understand why we would need a fetch here, and I actually think we
> should not get from db.
> 
> The ->process call is from the worker, which should happen right after
> ->find. If the object does not have a status==new then we should explode as
> the developer certainly messed up with something.

Sounds good to me.
Comment 18 Jonathan Druart 2023-03-03 10:53:56 UTC
I won't have more time to dedicate to this, feel free to take over.
Comment 19 Marcel de Rooy 2023-10-17 14:35:12 UTC
(In reply to Jonathan Druart from comment #18)
> I won't have more time to dedicate to this, feel free to take over.

I will now in light of the pending bug 34990.
Comment 20 Marcel de Rooy 2023-10-17 14:37:35 UTC
My solution is resolve the inconsistency between MQ and DB "mode":

MQ
my $job = Koha::BackgroundJobs->find($args->{job_id});
DB
my $jobs = Koha::BackgroundJobs->search({ status => 'new', queue => \@queues });

We should do the same checks for MQ mode, so:
my $job = Koha::BackgroundJobs->search( { id => $args->{job_id}, status => 'new, queue => \@queues } );

So check status new AND check queue.
Comment 21 Marcel de Rooy 2023-10-17 15:00:04 UTC
Created attachment 157256 [details] [review]
Bug 32305: Worker should ignore wrong status or queue in MQ mode

MQ mode: The worker has seen that MQ is running and consumes messages
instead of polling db only.

Test plan:
Stop worker for long tasks.
Make sure that Rabbit MQ runs.
Stage a file. (This adds a long task.)
Goto staff view of jobs and cancel this job.
Check if job is still in MQ with rabbitmqctl list_queues.
Now start worker for long tasks.
Check if job is gone in MQ with rabbitmqctl list_queues.
And check logfile for the adjusted warning like:
  [WARN] Job 5 not found, or has wrong status/queue main:: /usr/share/koha/misc/workers/background_jobs_worker.pl (134)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Marcel de Rooy 2023-10-17 15:04:56 UTC
We should do it too for es_indexer_daemon.pl
Comment 23 Marcel de Rooy 2023-10-18 14:01:36 UTC
Created attachment 157320 [details] [review]
Bug 32305: Counterpart for es_indexer_daemon.pl

This is a kind of copy-and-paste variant of background worker.
We probably could do better than having two scripts here ;)

Test plan:
See former test plan. Apply it to ES indexing.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 24 Nick Clemens 2023-10-18 14:25:03 UTC
Created attachment 157321 [details] [review]
Bug 32305: Worker should ignore wrong status or queue in MQ mode

MQ mode: The worker has seen that MQ is running and consumes messages
instead of polling db only.

Test plan:
Stop worker for long tasks.
Make sure that Rabbit MQ runs.
Stage a file. (This adds a long task.)
Goto staff view of jobs and cancel this job.
Check if job is still in MQ with rabbitmqctl list_queues.
Now start worker for long tasks.
Check if job is gone in MQ with rabbitmqctl list_queues.
And check logfile for the adjusted warning like:
  [WARN] Job 5 not found, or has wrong status/queue main:: /usr/share/koha/misc/workers/background_jobs_worker.pl (134)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 25 Nick Clemens 2023-10-18 14:25:05 UTC
Created attachment 157322 [details] [review]
Bug 32305: Counterpart for es_indexer_daemon.pl

This is a kind of copy-and-paste variant of background worker.
We probably could do better than having two scripts here ;)

Test plan:
See former test plan. Apply it to ES indexing.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 26 Marcel de Rooy 2023-10-19 09:13:26 UTC
Please QA. We need this one to go with 34990. See also omnibus.
Comment 27 Marcel de Rooy 2023-10-19 09:19:41 UTC
Note for QA: Jonathan suggested to move the selection to a filter_by method in BackgroundJobs. I agree. We should do that not on the compact bugfix here imo, but  make that part of some refactoring as described further on the omnibus.
Comment 28 Martin Renvoize 2023-10-20 08:18:40 UTC
QAing now.
Comment 29 Martin Renvoize 2023-10-20 08:30:40 UTC
Created attachment 157496 [details] [review]
Bug 32305: Worker should ignore wrong status or queue in MQ mode

MQ mode: The worker has seen that MQ is running and consumes messages
instead of polling db only.

Test plan:
Stop worker for long tasks.
Make sure that Rabbit MQ runs.
Stage a file. (This adds a long task.)
Goto staff view of jobs and cancel this job.
Check if job is still in MQ with rabbitmqctl list_queues.
Now start worker for long tasks.
Check if job is gone in MQ with rabbitmqctl list_queues.
And check logfile for the adjusted warning like:
  [WARN] Job 5 not found, or has wrong status/queue main:: /usr/share/koha/misc/workers/background_jobs_worker.pl (134)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 30 Martin Renvoize 2023-10-20 08:30:43 UTC
Created attachment 157497 [details] [review]
Bug 32305: Counterpart for es_indexer_daemon.pl

This is a kind of copy-and-paste variant of background worker.
We probably could do better than having two scripts here ;)

Test plan:
See former test plan. Apply it to ES indexing.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 31 Martin Renvoize 2023-10-20 08:32:51 UTC
All looking good here.. yes, it would be great to move to a filter_by.. but I totally agree it's OK to do that as part of the wider refactoring work going on.

Passing QA
Comment 32 Marcel de Rooy 2023-10-20 09:20:21 UTC
Created attachment 157500 [details] [review]
Bug 32305: Worker should ignore wrong status or queue in MQ mode

MQ mode: The worker has seen that MQ is running and consumes messages
instead of polling db only.

Test plan:
Stop worker for long tasks.
Make sure that Rabbit MQ runs.
Stage a file. (This adds a long task.)
Goto staff view of jobs and cancel this job.
Check if job is still in MQ with rabbitmqctl list_queues.
Now start worker for long tasks.
Check if job is gone in MQ with rabbitmqctl list_queues.
And check logfile for the adjusted warning like:
  [WARN] Job 5 not found, or has wrong status/queue main:: /usr/share/koha/misc/workers/background_jobs_worker.pl (134)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Removed queue from query at MQ side. Discussed on IRC.
Comment 33 Marcel de Rooy 2023-10-20 09:20:24 UTC
Created attachment 157501 [details] [review]
Bug 32305: Counterpart for es_indexer_daemon.pl

This is a kind of copy-and-paste variant of background worker.
We probably could do better than having two scripts here ;)

Test plan:
See former test plan. Apply it to ES indexing.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Removed queue from query at MQ side. Discussed on IRC.
Comment 34 Marcel de Rooy 2023-10-20 09:21:48 UTC
From IRC: 
[11:00] <Joubu> if the worker is getting an ID from the MQ it's not possible that the ID is from another queue.

That should be true indeed. Being cautious..
Comment 35 Martin Renvoize 2023-10-20 09:23:16 UTC
I think we can remove the queue check.. it's superflous and could lead to confusion later once we've all forgotten this discussion.

Happy for that to be a quick follow-up please :)
Comment 36 Marcel de Rooy 2023-10-20 09:34:14 UTC
(In reply to Martin Renvoize from comment #35)
> I think we can remove the queue check.. it's superflous and could lead to
> confusion later once we've all forgotten this discussion.
> 
> Happy for that to be a quick follow-up please :)

See above. Done
Comment 37 Tomás Cohen Arazi 2023-10-20 14:03:47 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 38 Fridolin Somers 2023-10-25 21:52:27 UTC
Pushed to 23.05.x for 23.05.05
Comment 39 Matt Blenkinsop 2023-11-13 14:19:50 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x