Track and kill embedded ansible monitoring thread #15612
Conversation
| @@ -177,7 +177,6 @@ def find_worker_record | |||
|
|
|||
| def starting_worker_record | |||
| find_worker_record | |||
| @worker.pid = Process.pid | |||
There was a problem hiding this comment.
This is important, as the worker model will store the self.pid= with the result of MiqWorker#start_runner,
which when changed to return the Thread identifier(an object_id), gets overwritten by this line 180.
We still need to let the worker set the pid when we call start_runner and in the case of the EmbeddedAnsible worker, nil.
We need to remove most places making assumptions about calling Process.pid for the workers.
jrafanie
force-pushed
the
track_and_kill_embedded_ansible_monitoring_thread
branch
from
37a5bc6 to
d9f7c39
Compare
Each worker can implement their own way to invoke processes so let them choose and persist their pid value, don't assume we can use Process.pid. https://bugzilla.redhat.com/show_bug.cgi?id=1469307 https://bugzilla.redhat.com/show_bug.cgi?id=1468898
When the server starts the monitor thread, store the worker class and id in the thread object so the server can then kill that thread if required later. Implement stop/kill/terminate in the same way: look for the thread containing the worker's class and id in Thread.list, exit it, and destroy the worker row. https://bugzilla.redhat.com/show_bug.cgi?id=1469307 https://bugzilla.redhat.com/show_bug.cgi?id=1468898
jrafanie
force-pushed
the
track_and_kill_embedded_ansible_monitoring_thread
branch
from
d9f7c39 to
d493381
Compare
jrafanie
changed the title
| nil # return no pid | ||
|
|
||
| t[:worker_class] = self.class.name | ||
| t[:worker_id] = id |
There was a problem hiding this comment.
Here we're setting thread instance variables in the Thread object so we can locate the thread for this worker later.
|
ok @Fryguy @gtanzillo @carbonin ready for review |
|
Checked commits jrafanie/manageiq@63816f4~...d493381 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
| end | ||
|
|
||
| alias terminate kill | ||
| alias stop kill |
There was a problem hiding this comment.
Do we want this? I think this worker still should be able to exit cleanly with stop if it is heartbeating, right?
There was a problem hiding this comment.
I think you're right but I need to do some testing.
| end | ||
| end | ||
|
|
||
| alias terminate kill |
There was a problem hiding this comment.
Right now the only other workers that implement #terminate are the UI workers. Do we need that here also?
|
Backported to Fine via #15631 |
This PR will prevent embedded ansible monitoring threads from piling up if the setup takes a very long time, leading to the bug fixed by: ManageIQ/manageiq-gems-pending#247.
Do we need this on euwe?
https://bugzilla.redhat.com/show_bug.cgi?id=1469307
https://bugzilla.redhat.com/show_bug.cgi?id=1468898