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

Move embedded ansible worker thread up to start_runner #14256

Merged

Conversation

jrafanie
Copy link
Member

@jrafanie jrafanie commented Mar 9, 2017

This is a followup to feedback in #14053 and was implemented with @carbonin's and @Fryguy 's extensive help:

  • move the thread creation and knowledge to start_runner, the same place miq_worker.rb uses fork
  • with thread knowledge out of the runner, we can remove our simplified do_work_loop and just use the base class's version
  • remove do_exit overriding by using the long forgotten before_exit hook (at least forgotten by me)
  • simplify do_work by overriding heartbeat to only call super if it's alive

@miq-bot
Copy link
Member

miq-bot commented Mar 9, 2017

Checked commits jrafanie/manageiq@18ffca9~...bba7fbf with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks good. 👍

@jrafanie jrafanie changed the title [WIP] Move embedded ansible worker thread up Move embedded ansible worker thread up to start_runner Mar 10, 2017
@jrafanie jrafanie removed the wip label Mar 10, 2017
@jrafanie
Copy link
Member Author

@carbonin graciously tested this branch 🙇 and it looked good for him.

@Fryguy @gtanzillo I believe this is ready for final review/merge

@@ -1204,6 +1204,7 @@
:starting_timeout: 10.minutes
:stopping_timeout: 10.minutes
:embedded_ansible_worker:
:starting_timeout: 20.minutes
Copy link
Member

@carbonin carbonin Mar 10, 2017

Choose a reason for hiding this comment

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

In my tests on a VM running in workstation with 6GB of RAM and no other workers running the initial configuration took about 7 minutes and the subsequent starts took about 1 minute.

I'm okay with this value, but we could probably make it smaller if we have a good reason to.
Thoughts?

Copy link
Member Author

@jrafanie jrafanie Mar 10, 2017

Choose a reason for hiding this comment

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

Yeah, I don't know. As long as this timeout is used for the initial setup, a task that has a variable number of things it does, I'd be concerned with having ansible workers that can never start. I'd rather be cautious here.

Copy link
Member

@Fryguy Fryguy left a comment

Choose a reason for hiding this comment

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

LGTM, but I was partially involved. @gtanzillo Please merge if you are good with this.

@gtanzillo gtanzillo added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 10, 2017
@gtanzillo gtanzillo merged commit 4d8284f into ManageIQ:master Mar 10, 2017
@jrafanie jrafanie deleted the move_embedded_ansible_worker_thread_up branch March 12, 2017 20:42
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.

5 participants