Bug 33898 - background_jobs_worker.pl may leave defunct children processes for extended periods of time
Summary: background_jobs_worker.pl may leave defunct children processes for extended p...
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 minor (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 35092 35920
  Show dependency treegraph
 
Reported: 2023-06-05 14:31 UTC by Kyle M Hall
Modified: 2024-03-19 21:32 UTC (History)
7 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:
24.05.00,23.11.04,23.05.10


Attachments
Bug 33898: background_jobs_worker.pl may leave defunct children processes for extended periods of time (1.59 KB, patch)
2023-06-05 14:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33898: background_jobs_worker.pl may leave defunct children processes for extended periods of time (1.77 KB, patch)
2023-06-05 14:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33898: background_jobs_worker.pl may leave defunct children processes for extended periods of time (1.77 KB, patch)
2023-06-05 14:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33898: background_jobs_worker.pl may leave defunct children processes for extended periods of time (1.84 KB, patch)
2023-07-07 16:33 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 33898: background_jobs_worker.pl may leave defunct children processes for extended periods of time (1.96 KB, patch)
2024-03-08 06:03 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 33898: Alternative approach with receive frame timeout (1.37 KB, patch)
2024-03-08 07:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33898: Alternative approach with receive frame timeout (1.63 KB, patch)
2024-03-08 07:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33898: (follow-up) Apply same solution to es_indexer_daemon (1.24 KB, patch)
2024-03-08 07:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33898: Alternative approach with receive frame timeout (1.64 KB, patch)
2024-03-08 11:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33898: Implement reaping for database polling (729 bytes, patch)
2024-03-08 11:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33898: Alternative approach with receive frame timeout (1.70 KB, patch)
2024-03-11 19:28 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 33898: Implement reaping for database polling (785 bytes, patch)
2024-03-11 19:28 UTC, Victor Grousset/tuxayo
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-06-05 14:31:17 UTC
It appears that the background jobs worker can leave defunct processes for periods of time. Though it is mostly harmless, it would be nice if that did not happen. Children are reaped automatically when start or wait_all_children are called. We only call start when a new job is found, and wait_all_children after exiting our while loop. The solution is to simply call reap_all_children after we sleep. This is a non-blocking call that will clean up those defunct processes.
Comment 1 Kyle M Hall 2023-06-05 14:39:27 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2023-06-05 14:49:14 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2023-06-05 14:54:37 UTC
Created attachment 151997 [details] [review]
Bug 33898: background_jobs_worker.pl may leave defunct children processes for extended periods of time

It appears that the background jobs worker can leave defunct processes for periods of time. Though it is mostly harmless, it would be nice if that did not happen. Children are reaped automatically when start or wait_all_children are called. We only call start when a new job is found, and wait_all_children after exiting our while loop. The solution is to simply call reap_all_children after we sleep. This is a non-blocking call that will clean up those defunct processes.

Test Plan:
1) Disable Rabbit
2) Set background_jobs_worker/max_processes to something like 5
3) Restart all the things!
4) Run a bunch of elastic index updates
5) Verify you have defunct processes
6) Apply this patch
7) Run more elastic index updates
8) Defunct processes should disappear every 10 seconds or so!

If you do not see defunct processes, the test plan is to simply verify
everything continues to work as expected.
Comment 4 Emily Lamancusa 2023-07-07 16:33:02 UTC
Created attachment 153194 [details] [review]
Bug 33898: background_jobs_worker.pl may leave defunct children processes for extended periods of time

It appears that the background jobs worker can leave defunct processes for periods of time. Though it is mostly harmless, it would be nice if that did not happen. Children are reaped automatically when start or wait_all_children are called. We only call start when a new job is found, and wait_all_children after exiting our while loop. The solution is to simply call reap_all_children after we sleep. This is a non-blocking call that will clean up those defunct processes.

Test Plan:
1) Disable Rabbit
2) Set background_jobs_worker/max_processes to something like 5
3) Restart all the things!
4) Run a bunch of elastic index updates
5) Verify you have defunct processes
6) Apply this patch
7) Run more elastic index updates
8) Defunct processes should disappear every 10 seconds or so!

If you do not see defunct processes, the test plan is to simply verify
everything continues to work as expected.

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 5 Marcel de Rooy 2023-07-08 06:03:16 UTC
You did not see https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32558#c36 btw?

Signal handlers are not the most elegant way to address problems. Inherited by children too, altough no big deal here (not the pending signals).
P::F allows you to define a run_on_finish callback. Could that be used?
Comment 6 David Cook 2023-07-10 01:08:44 UTC
(In reply to Marcel de Rooy from comment #5)
> You did not see
> https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32558#c36 btw?
> 
> Signal handlers are not the most elegant way to address problems. Inherited
> by children too, altough no big deal here (not the pending signals).
> P::F allows you to define a run_on_finish callback. Could that be used?

I agree that signal handlers aren't the way to go.

It looks like run_on_finish is called via wait_all_children, so probably won't work in this case.

An alternative could be to add a timeout (e.g. 10 seconds) to $conn->receive_frame, and to call $pm->reap_finished_children() if it returns undef (before calling next()).

That would be pretty lightweight, since receive_frame is just calling the can_read () on the select loop.

In effect, it's doing a 10 second sleep which can be interrupted by an incoming frame. 

An incoming frame reaps via start(), or we'd reap after that timeout sleep. 

That should do the trick.
Comment 7 Kyle M Hall 2023-07-10 11:19:52 UTC
(In reply to David Cook from comment #6)
> (In reply to Marcel de Rooy from comment #5)
> > You did not see
> > https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32558#c36 btw?

I'm not sure what you are trying to indicate. Your patch was included in the bug push and does not solve this problem.

> An alternative could be to add a timeout (e.g. 10 seconds) to
> $conn->receive_frame, and to call $pm->reap_finished_children() if it
> returns undef (before calling next()).

That would only work for Koha's using Rabbit and 10 seconds is quite a long delay between calls to reap all.
Comment 8 David Cook 2023-07-10 23:35:25 UTC
(In reply to Kyle M Hall from comment #7)
> (In reply to David Cook from comment #6)
> > An alternative could be to add a timeout (e.g. 10 seconds) to
> > $conn->receive_frame, and to call $pm->reap_finished_children() if it
> > returns undef (before calling next()).
> 
> That would only work for Koha's using Rabbit and 10 seconds is quite a long
> delay between calls to reap all.

You could easily add it to the database polling block too. 

I suggested 10 seconds since that's the sleep used for the database polling block, but we could use shorter intervals for reaping. 

That said, why would 10 seconds be a long delay between calls to reap? If you have frequent jobs, then start() will be reaping previous ones. If you have infrequent jobs, then you probably won't have a lot of zombie children around, so 10 seconds seems all right to me to wait for reaping them. I suppose the problem compounds if you have many instances on a server, but I don't think they'd fill up the process table.
Comment 9 Victor Grousset/tuxayo 2023-08-06 02:23:56 UTC
> 4) Run a bunch of elastic index updates
> 5) Verify you have defunct processes

> If you do not see defunct processes, the test plan is to simply verify
> everything continues to work as expected.

Any idea what factors can help having the issue reproduced?

Also, I renamed multiple times at record, that causes ES updates with a background worker, right?
Comment 10 Victor Grousset/tuxayo 2023-08-06 02:26:20 UTC
at record => a record
Comment 11 Katrin Fischer 2023-10-03 12:39:13 UTC
Taking a closer look at the comments here, there seems to be some discussion about implementation, also question about how to reproduce the issue this is trying to fix. 

Truth is this has had comments from a lot of QA team members already and we are a little stuck.

Kyle, could you review the last few comments for a start please?
Comment 12 Asymar Riu 2024-03-07 09:49:06 UTC
Hi,

I encounter the same issue.

In my case, Koha 22.11.15 (Debian Package) on Ubuntu 22.04.1(MariaDB + RabbitMQ), the issue occurs when I do:

From Koha Staff Interface
1) Stage records for import (everything goes fine)
2) Manage staged MARC records (everything goes fine)

FromShell
3) # ps aux | grep 'Z'
find the "background_jobs" as Zombie <defunct> with a PID (e.g. 54679)

From Koha Staff Interface
4) Stage records for import (everything goes fine)
5) Manage staged MARC records (everything goes well)

FromShell
6) # ps aux | grep 'Z'
find the "background_jobs" as Zombie <defunct> with another PID (e.g. 54679)

So, the fact that "background_jobs" remains a zombie does not actually prevent other processes from operating in Koha (simply once the new process has started, that zombie is killed and a new zombie is created).

A reboot of the server kills the zombie process.

Since it seems like a problem that doesn't affect everyone, could it be something related to the Server settings?

Thanks
Comment 13 David Cook 2024-03-07 22:15:39 UTC
(In reply to Asymar Riu from comment #12)
> A reboot of the server kills the zombie process.

That's overkill. To get child zombie processes reaped, you'd just need to stop the parent process. Then the child process gets inherited by PID 1 and they'll get reaped.
 
> Since it seems like a problem that doesn't affect everyone, could it be
> something related to the Server settings?

No, it's a real issue. It's  more likely to happen to systems that use background jobs more, especially sporadically. 

This is just how parent/child processes work. Parent processes are responsible for reaping their child processes. As Kyle pointed out, currently child processes are only reaped under certain conditions. It's just about tweaking the code to reap them in a more responsible way.
Comment 14 Victor Grousset/tuxayo 2024-03-08 05:35:07 UTC
(In reply to David Cook from comment #13)
> To get child zombie processes reaped, you'd just need to stop the parent process. Then the child process gets inherited by PID 1 and they'll get reaped.

In other words @Asymar just restart the koha services (plack more specifically).
I think it's here: https://wiki.koha-community.org/wiki/Commands_provided_by_the_Debian_packages#koha-plack
Comment 15 Victor Grousset/tuxayo 2024-03-08 06:03:31 UTC
Created attachment 162941 [details] [review]
Bug 33898: background_jobs_worker.pl may leave defunct children processes for extended periods of time

It appears that the background jobs worker can leave defunct processes for periods of time. Though it is mostly harmless, it would be nice if that did not happen. Children are reaped automatically when start or wait_all_children are called. We only call start when a new job is found, and wait_all_children after exiting our while loop. The solution is to simply call reap_all_children after we sleep. This is a non-blocking call that will clean up those defunct processes.

Test Plan:
1) Disable Rabbit
2) Set background_jobs_worker/max_processes to something like 5
3) Restart all the things!
4) Run a bunch of elastic index updates
5) Verify you have defunct processes
6) Apply this patch
7) Run more elastic index updates
8) Defunct processes should disappear every 10 seconds or so!

If you do not see defunct processes, the test plan is to simply verify
everything continues to work as expected.

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
  Used Asymar's test plan from comment 12. (based on record import)
Comment 16 Victor Grousset/tuxayo 2024-03-08 06:12:35 UTC
Thanks Asymar for your test plan, it's very simple and I was able to reproduce the issue and confirm that the patch prevents it.

So with comment 12's test plan, there one zombie at the time. It's killed when the next staging/import starts. And a new zombie will stay at the end of the operation.

So that blocking point is no more. (unless the elastic index update based test plan shows something more subtle or impactful that would benefit from being tested)

Remaining is «there seems to be some discussion about implementation».
Comment 17 Marcel de Rooy 2024-03-08 07:36:54 UTC
In the above test plan Elastic is used, but note that those updates do not go via the regular worker script. There is another one (with more ore less the same code). Still to be merged :)
Comment 18 Marcel de Rooy 2024-03-08 07:38:42 UTC
Created attachment 162942 [details] [review]
Bug 33898: Alternative approach with receive frame timeout

See bug 33898 comment6.
Comment 19 Marcel de Rooy 2024-03-08 07:52:24 UTC
Created attachment 162943 [details] [review]
Bug 33898: Alternative approach with receive frame timeout

See bug 33898 comment6.

Test plan:
Based on comment12: Stage MARC import and manage. Look at the same
time to the results of ps aux|grep Z. Verify that the lines with
[background_jobs] <defunct> disappear within 10 seconds.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Marcel de Rooy 2024-03-08 07:53:53 UTC
The alternative patch addresses the issues raised earlier on the use of signals and follows the suggestions of David.

Kyle, Victor and Asymar: Would this work for you? Note that you should apply only the second patch as an alternative for the first one.
Comment 21 Marcel de Rooy 2024-03-08 07:57:44 UTC
Created attachment 162944 [details] [review]
Bug 33898: (follow-up) Apply same solution to es_indexer_daemon

Test plan:
Similar as first patch with Elastic index jobs.
Comment 22 Marcel de Rooy 2024-03-08 08:01:06 UTC
Comment on attachment 162944 [details] [review]
Bug 33898: (follow-up) Apply same solution to es_indexer_daemon

Oops this still needs a bit of attention
Comment 23 Marcel de Rooy 2024-03-08 08:01:54 UTC
(In reply to Marcel de Rooy from comment #22)
> Comment on attachment 162944 [details] [review] [review]
> Bug 33898: (follow-up) Apply same solution to es_indexer_daemon
> 
> Oops this still needs a bit of attention

Ah I see $pm is not even used in the es_indexer_daemon..
Comment 24 Kyle M Hall 2024-03-08 11:08:26 UTC
Created attachment 162953 [details] [review]
Bug 33898: Alternative approach with receive frame timeout

See bug 33898 comment6.

Test plan:
Based on comment12: Stage MARC import and manage. Look at the same
time to the results of ps aux|grep Z. Verify that the lines with
[background_jobs] <defunct> disappear within 10 seconds.

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 25 Kyle M Hall 2024-03-08 11:08:33 UTC
Created attachment 162954 [details] [review]
Bug 33898: Implement reaping for database polling

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 26 Victor Grousset/tuxayo 2024-03-11 19:28:06 UTC
Created attachment 163056 [details] [review]
Bug 33898: Alternative approach with receive frame timeout

See bug 33898 comment6.

Test plan:
Based on comment12: Stage MARC import and manage. Look at the same
time to the results of ps aux|grep Z. Verify that the lines with
[background_jobs] <defunct> disappear within 10 seconds.

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 27 Victor Grousset/tuxayo 2024-03-11 19:28:09 UTC
Created attachment 163057 [details] [review]
Bug 33898: Implement reaping for database polling

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 28 Victor Grousset/tuxayo 2024-03-11 19:32:30 UTC
(In reply to Marcel de Rooy from comment #20)
> The alternative patch addresses the issues raised earlier on the use of
> signals and follows the suggestions of David.

Thanks :)

> Kyle, Victor and Asymar: Would this work for you?

It works, so signing off.
As for the signal vs no signals I don't know the good practices to QA this. Same for how this plays with when using database polling or the message queue. (I would have missed that the 1st alternate patch missed this)
Comment 29 Kyle M Hall 2024-03-13 15:01:53 UTC
(In reply to Victor Grousset/tuxayo from comment #28)
> (In reply to Marcel de Rooy from comment #20)
> > The alternative patch addresses the issues raised earlier on the use of
> > signals and follows the suggestions of David.
> 
> Thanks :)
> 
> > Kyle, Victor and Asymar: Would this work for you?
> 
> It works, so signing off.
> As for the signal vs no signals I don't know the good practices to QA this.
> Same for how this plays with when using database polling or the message
> queue. (I would have missed that the 1st alternate patch missed this)

I think we can move this to PQA with the sign-offs we have!
Comment 30 Asymar Riu 2024-03-13 15:08:23 UTC
Hi, sorry for the delay in replying: I confirm that it works.

Thank you very much! :)
Comment 31 Katrin Fischer 2024-03-15 08:39:52 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 32 Fridolin Somers 2024-03-18 08:56:42 UTC
Pushed to 23.11.x for 23.11.04
Comment 33 Lucas Gass 2024-03-19 21:32:54 UTC
Backported to 23.05.x for upcoming 23.05.10