Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix smartproxy worker heartbeat thread #19816

Merged
merged 3 commits into from
Feb 7, 2020

Conversation

carbonin
Copy link
Member

@carbonin carbonin commented Feb 5, 2020

The heartbeat worker thread and the main thread were competing
to define the Workers::MiqDefaults constant.

The main thread calls heartbeat from do_work_loop which is
unavoidable. So to solve the problem we just wait until the first
call to do_work to start the heartbeat worker.

This allows the main thread to resolve the constant without contention
and the heartbeat worker still gets started before we process the
first queue message

Fixes #19813

The heartbeat worker thread and the main thread were competing
to define the Workers::MiqDefaults constant.

The main thread calls heartbeat from do_work_loop which is
unavoidable. So to solve the problem we just wait until the first
call to do_work to start the heartbeat worker.

This allows the main thread to resolve the constant without contention
and the heartbeat worker still gets started before we process the
first queue message

Fixes ManageIQ#19813
@carbonin carbonin changed the title Fix smartproxy worker heartbeat thread [WIP] Fix smartproxy worker heartbeat thread Feb 5, 2020
@miq-bot miq-bot added the wip label Feb 5, 2020
Copy link
Member

@jerryk55 jerryk55 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code makes sense, and I just tested SSA end-to-end successfully.

@jerryk55
Copy link
Member

jerryk55 commented Feb 6, 2020

@miq-bot add_label smartstate

@miq-bot
Copy link
Member

miq-bot commented Feb 6, 2020

@jerryk55 Cannot apply the following label because they are not recognized: smartstate

Previously this could call join on a nil.
Now that ruby 2.5 logs exceptions raised in threads by default we
don't have to worry about joining the thread to re-raise.
Instead we can just kill the thread if it's aborting and restart
it if it is exited (cleanly or not).
@miq-bot
Copy link
Member

miq-bot commented Feb 6, 2020

Checked commits carbonin/manageiq@5a1eb86~...8399786 with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 1 offense detected

app/models/miq_smart_proxy_worker/runner.rb

@carbonin carbonin changed the title [WIP] Fix smartproxy worker heartbeat thread Fix smartproxy worker heartbeat thread Feb 6, 2020
@miq-bot miq-bot removed the wip label Feb 6, 2020

# if the thread is dead, start a new one and kill it if it is aborting
if !@tid.alive?
@tid.kill if @tid.status == "aborting"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's amazing how much easier this is to grok by splitting apart that conditional into two conditional checks.

Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jrafanie jrafanie merged commit 276cff1 into ManageIQ:master Feb 7, 2020
@jrafanie jrafanie added this to the Sprint 130 Ending Feb 17, 2020 milestone Feb 7, 2020
@carbonin carbonin deleted the fix_smartproxy_worker branch February 7, 2020 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MiqSmartProxyWorker No Longer Starts After Latest Worker Changes
5 participants