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

Rely on 10 minute starting_timeout instead of a heartbeating thread #14053

Merged

Conversation

jrafanie
Copy link
Member

The server process allows "starting" workers up to 10 minutes to start
before marking them as not responding. We can use this starting_timeout
instead of heartbeating in a thread while another thread runs the
ansible setup playbook (configure).

cc @Fryguy @gtanzillo @carbonin

I manually tested this, I'm open to suggestions on how to unit test this.

The server process allows "starting" workers up to 10 minutes to start
before marking them as not responding.  We can use this starting_timeout
instead of heartbeating in a thread while another thread runs the
ansible setup playbook (configure).
@miq-bot
Copy link
Member

miq-bot commented Feb 23, 2017

Checked commit jrafanie@14ee97b with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. ⭐

@Fryguy
Copy link
Member

Fryguy commented Feb 23, 2017

I'm confused...this doesn't use the do_before_work_loop mechanism at all.

@jrafanie
Copy link
Member Author

I'm confused...this doesn't use the do_before_work_loop mechanism at all.

@Fryguy Yeah, we can't without creating a new thread. The point was to remove the heartbeating thread. We can't block the server so we'd have to create another thread to do the setup_ansible in the do_before_work_loop. This PR moves all of the logic into the do_work_loop in a single thread.

@Fryguy Fryguy self-assigned this Feb 23, 2017
Copy link
Member

@carbonin carbonin left a comment

Choose a reason for hiding this comment

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

I'm for this, we've gotten burned by this extra thread at least once already.

@jrafanie
Copy link
Member Author

jrafanie commented Mar 8, 2017

@Fryguy can you review/merge? @carbonin keeps hitting this issue when the extra thread spins up but doesn't complete.

@carbonin
Copy link
Member

carbonin commented Mar 8, 2017

I think @Fryguy is suggesting that instead of moving things into the thread, we should move the thread into EmbeddedAnsibleWorker#start_runner which would map to where we call fork for the other workers.

@carbonin carbonin assigned carbonin and unassigned Fryguy Mar 9, 2017
@carbonin carbonin added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 9, 2017
@carbonin carbonin merged commit dbc1e0b into ManageIQ:master Mar 9, 2017
@jrafanie jrafanie deleted the remove_hearbeat_thread_during_startup branch March 9, 2017 23:06
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.

4 participants