Add lib/workers/bin/run_single_worker.rb script #15132

NickLaMuro · 2017-05-17T19:37:12Z

Allows running of workers via a single script.

Currently skips removing db records, so stale MiqWorker records will still be left in the DB. Also, doesn't load the "MVP" number of classes, which will be done in a separate effort, but allows for any worker known to by MIQ to be spawned.

Requires disabling heartbeating, unless a MiqServer::WorkerManagement process is running (disabling heartbeating is done in #15123 ).

Links

Steps for Testing/QA

~~If #15123 is not merged yet, run the following in this branch:~~ UPDATE: #15123 is now merged into this branch, so the following is now not needed.

$ git apply <(curl -L https://github.com/ManageIQ/manageiq/pull/15123.patch)

From there you can try out the following:

$ bundle exec ruby lib/workers/bin/run_single_worker.rb
$ bundle exec ruby lib/workers/bin/run_single_worker.rb -h
$ bundle exec ruby lib/workers/bin/run_single_worker.rb -l
$ bundle exec ruby lib/workers/bin/run_single_worker.rb MiqUiWorker
$ bundle exec ruby lib/workers/bin/run_single_worker.rb [PICK_CLASS_FROM_DASH_L_LIST_FROM_ABOVE]

And confirm that the last to start the worker as expected.

Actually, this now doesn't require bundle exec to function, and you can clearly see the difference now by running it with and without using the -h or -l flags. Even passing in something like FakeWorker as an argument will be caught much quicker thanks to the last two commits.

kbrock · 2017-05-17T19:59:14Z

lib/workers/bin/run_single_worker.rb

+opt_parser = OptionParser.new do |opts|
+  opts.banner = "usage: #{File.basename $0, '.rb'} MIQ_WORKER_CLASS_NAME"
+
+  opts.on("-l", "--[no]-list", "Toggle available worker class names") do


... do |val| options[:list] = val end

Yup, you are right. Good catch.

kbrock

This looks great

kbrock · 2017-05-17T20:00:09Z

/cc @jrafanie Think you were doing similar things

chessbyte · 2017-05-19T20:09:29Z

lib/workers/bin/run_single_worker.rb

+
+worker = ARGV[0].constantize.create_worker_record
+at_exit {
+  puts "\nGracefullying stoping worker... Deleting server record..."


typo Gracefullying

Woops... thanks.

NickLaMuro · 2017-05-19T22:36:23Z

For those following along at home, there used to be a commit with the following:

From 234370a20f3b805897475bdfe5810082e3fcd9b0 Mon Sep 17 00:00:00 2001
From: Nick LaMuro <nicklamuro@gmail.com>
Date: Fri, 19 May 2017 14:07:38 -0500
Subject: [PATCH] Avoid needing bundle exec for run_single_worker.rb

`bundle exec ...` takes about 1.3 seconds (on my machine) in the
manageiq application, and if we don't even get that far in the script to
load the queries, it is not even necessary (optparse is part of standard
lib), this makes printing help and the list of possible workers almost
instant, instead of waiting on dependency resolution.
---
 lib/workers/bin/run_single_worker.rb | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/workers/bin/run_single_worker.rb b/lib/workers/bin/run_single_worker.rb
index 047c7b06cfb..28cb90f04e4 100644
--- a/lib/workers/bin/run_single_worker.rb
+++ b/lib/workers/bin/run_single_worker.rb
@@ -38,6 +38,8 @@ module MiqServer; module WorkerManagement; module Monitor;
 # Skip heartbeating with single worker
 ENV["DISABLE_MIQ_WORKER_HEARTBEAT"] ||= '1'
 
+require "rubygems"
+require "bundler/setup"
 require File.expand_path("../../../config/environment", __dir__)
 
 worker_list  = MiqServer::WorkerManagement::Monitor::ClassNames::MONITOR_CLASS_NAMES

This turned out to be completely useless and redundant, as config/environment.rb already does this... so I have removed it from the history. Leaving this comment here for reference of that commit (and my stupidity).

miq-bot · 2017-05-22T16:52:43Z

This pull request is not mergeable. Please rebase and repush.

kbrock · 2017-05-31T21:16:30Z

This looks promising. Any status update?

NickLaMuro · 2017-06-01T03:51:20Z

@kbrock ~~Dependent on #15020, which is finally getting a review.~~ Scratch that, I lied, guess this isn't that involved yet. Yell at @jrafanie to review.

gtanzillo

These changes look good to me 👍 . We should be able to use this as our entrynpoint for running in a container. In addition to disabling heartbeating, we'll need to come up with a new mechanism for messaging between the server monitor and workers for thing like sync'ing configuration changes.

kbrock · 2017-06-07T13:26:36Z

Think the last rubocop is valid.

Maybe just merge those 2 exceptions?

jrafanie · 2017-06-07T15:11:34Z

app/models/miq_server/worker_management/monitor/class_names.rb

-    MiqCockpitWsWorker
-  ).freeze
+  MONITOR_CLASS_NAMES               = MIQ_WORKER_TYPES
+  MONITOR_CLASS_NAMES_IN_KILL_ORDER = MIQ_WORKER_TYPES_IN_KILL_ORDER


Looks good. The only feedback I have is it is a bit odd to reference a global constant. Can we put them in a small module that "thin ruby" processes can require? Not for now, just wondering if you had reasons for making using a global constant.

NickLaMuro · 2017-06-07T15:19:46Z

Think the last rubocop is valid.

Maybe just merge those 2 exceptions?

@kbrock Yup, doing that, and also noticed a bug that was caused from some recent rebasing because of that, so thanks (worker variable assignment was removed in the last commit)!

The only feedback I have is it is a bit odd to reference a global constant. Can we put them in a small module that "thin ruby" processes can require? Not for now, just wondering if you had reasons for making using a global constant.

@jrafanie I don't have any problems changing this now, but really don't have a module name I am thinking of using for this (#namingIsHardMmmkay). Any suggestions?

As far as why I went with the global constant, since I didn't have one to consider, I figured a unique global constant was good enough, but again, not sold on the idea. Also didn't want to potentially conflict with existing modules, so there was that as well.

jrafanie · 2017-06-07T15:40:07Z

@jrafanie I don't have any problems changing this now, but really don't have a module name I am thinking of using for this (#namingIsHardMmmkay). Any suggestions?

As far as why I went with the global constant, since I didn't have one to consider, I figured a unique global constant was good enough, but again, not sold on the idea. Also didn't want to potentially conflict with existing modules, so there was that as well.

@NickLaMuro I'm fine with putting a TODO to put those constants in a module when we figure out where they belong... also a comment that we're using a global constant provided by another file.

jrafanie · 2017-06-07T17:28:53Z

lib/workers/bin/run_single_worker.rb

+    if s.is_a?(Interrupt)
+      puts "\nInterrupted... Deleting server record..."
+    else
+      puts "\n#{s} caught... Deleting server record..."


Can we just do this else for Interrupts too? It's nearly identical.

@jrafanie I actually removed this code. I will have an additional commit in a few explaining the rational, but basically signal handling is now handled because I am using .start_worker instead of #do_work_loop.

Allows running of workers via a single script. Currently skips removing db records, so stale MiqWorker records will still be left in the DB. Also, doesn't load the "MVP" number of classes, which will be done in a separate effort, but allows for any worker known to by MIQ to be spawned. Requires disabling heartbeating, unless a MiqServer::WorkerManagement process is running.

Prior to this, worker records would pile up in the database because they weren't being deleted on exit. With `evm:start`, this would kill any existing records, but not any that might have been aborted or didn't start properly. Killing when the script is complete does automatic cleanup for the record that the script created, and doesn't potentially kill workers that might still be running (say with MiqServer.kill_all_workers). In previous iterations prior to a rebase, we were handling signals using a more verbose `begin`/`rescue` block, similar to this: begin worker_class::Runner.start_worker(:guid => worker.guid) rescue Interrupt, SignalException => s if s.is_a?(Interrupt) puts "\nInterrupted... Deleting server record..." else puts "\n#{s} caught... Deleting server record..." end ensure worker.delete end This existed in this form also because `start_worker` wasn't used in a previous iteration, and a lower level interface of `do_work_loop` was being called in it's place. The `start_worker` method actually has some rescuing in place around it for signals (where the latter doesn't), and handles MiqEvents as well, so using that is preferred. It doesn't, however, handle deleting the record, so we do it ourselves in the ensure block to make sure the record is removed before exiting.

MiqServer::WorkerManagement::Monitor::ClassNames is a module nested inside of the MiqServer's namespace, making it nearly impossible to load it on it's own without including the rest of the classes and subclasses that are included by it. Moving the constants into a global constant then allows the MiqServer::WorkerManagement::Monitor::ClassNames model to reference the constants defined there, and allows it to be used by plain ruby without the overhead of rails/activesupport/etc.

With the addition of the lib/worker/miq_worker_types.rb file, we can now load the class names hire up in the stack without needing bundler, and also make sure the majority of the error handling happens before we do the heavy lifting of loading the application code. Also refactored the opt_parser.abort to use the worker_class variable and pushes that farther up the script.

Currently, we either set the DISABLE_MIQ_WORKER_HEARTBEAT var if it isn't set, or use what is already there. The problem with that is if a user wants to heartbeat and use the run_single_worker script, that currently isn't possible. This adds a flag to the script to either set the ENV var to nil if the user passes in the `--heartbeat` flag, or set it to '1' (current functionality). This functionality from this commit favors the existing ENV variable over the flag, which might be counter-intuitive.

Allows to user to try loading everything up until the point of creating the worker record in the DB and running the worker loop. Useful if the user wishes to test everything can be loaded properly (this is mostly for me for future PRs... #HonestNVL)

NickLaMuro · 2017-06-07T17:47:25Z

@jrafanie Updated commit message to explain the rationale for your most recent review concern: 34b2d71

NickLaMuro · 2017-06-07T17:48:42Z

Also, when doing the QA plan, if you use something like the MiqUiWorker, everything will work as expected (needed to add worker_class.before_fork to make that work by the way).

miq-bot · 2017-06-07T17:51:54Z

Some comments on commits NickLaMuro/manageiq@5dbdefb~...9a6f825

lib/workers/bin/run_single_worker.rb

⚠️ - 27 - Detected puts. Remove all debugging statements.
⚠️ - 37 - Detected puts. Remove all debugging statements.
⚠️ - 43 - Detected puts. Remove all debugging statements.

miq-bot · 2017-06-07T17:52:41Z

Checked commits NickLaMuro/manageiq@5dbdefb~...9a6f825 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. 🍰

kbrock reviewed May 17, 2017

View reviewed changes

NickLaMuro force-pushed the run_single_worker_script branch from 73ee3b9 to 8726d46 Compare May 17, 2017 20:05

chessbyte assigned jrafanie May 17, 2017

NickLaMuro force-pushed the run_single_worker_script branch 3 times, most recently from 1db33a7 to e00633e Compare May 19, 2017 19:38

chessbyte reviewed May 19, 2017

View reviewed changes

NickLaMuro force-pushed the run_single_worker_script branch 2 times, most recently from 137ae6a to 21cb2a8 Compare May 19, 2017 22:34

NickLaMuro mentioned this pull request May 20, 2017

[WIP] Slim Worker Runner #15174

Closed

10 tasks

miq-bot added the unmergeable label May 22, 2017

NickLaMuro force-pushed the run_single_worker_script branch from 21cb2a8 to d0f6564 Compare May 23, 2017 20:25

miq-bot removed the unmergeable label May 23, 2017

NickLaMuro mentioned this pull request Jun 2, 2017

Modularize patches/database_configuration.rb #14953

Merged

gtanzillo approved these changes Jun 4, 2017

View reviewed changes

NickLaMuro force-pushed the run_single_worker_script branch from d0f6564 to 88e6e58 Compare June 6, 2017 16:15

jrafanie reviewed Jun 7, 2017

View reviewed changes

NickLaMuro force-pushed the run_single_worker_script branch 2 times, most recently from 1c37e67 to 29d47b4 Compare June 7, 2017 17:13

jrafanie reviewed Jun 7, 2017

View reviewed changes

NickLaMuro added 5 commits June 7, 2017 12:44

Add --dry-run flag

9a6f825

Allows to user to try loading everything up until the point of creating the worker record in the DB and running the worker loop. Useful if the user wishes to test everything can be loaded properly (this is mostly for me for future PRs... #HonestNVL)

NickLaMuro force-pushed the run_single_worker_script branch from 29d47b4 to 9a6f825 Compare June 7, 2017 17:46

jrafanie merged commit be8613f into ManageIQ:master Jun 8, 2017

jrafanie added core/workers performance labels Jun 8, 2017

jrafanie added this to the Sprint 63 Ending Jun 19, 2017 milestone Jun 8, 2017

chessbyte added the developer label Jun 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lib/workers/bin/run_single_worker.rb script #15132

Add lib/workers/bin/run_single_worker.rb script #15132

NickLaMuro commented May 17, 2017 •

edited

Loading

kbrock May 17, 2017

NickLaMuro May 17, 2017

kbrock left a comment

kbrock commented May 17, 2017

chessbyte May 19, 2017

NickLaMuro May 19, 2017

NickLaMuro commented May 19, 2017

miq-bot commented May 22, 2017

kbrock commented May 31, 2017

NickLaMuro commented Jun 1, 2017 •

edited

Loading

gtanzillo left a comment

kbrock commented Jun 7, 2017

jrafanie Jun 7, 2017

NickLaMuro commented Jun 7, 2017

jrafanie commented Jun 7, 2017

jrafanie Jun 7, 2017

NickLaMuro Jun 7, 2017

jrafanie Jun 7, 2017

NickLaMuro commented Jun 7, 2017 •

edited by chessbyte

Loading

NickLaMuro commented Jun 7, 2017

miq-bot commented Jun 7, 2017

miq-bot commented Jun 7, 2017

Add lib/workers/bin/run_single_worker.rb script #15132

Add lib/workers/bin/run_single_worker.rb script #15132

Conversation

NickLaMuro commented May 17, 2017 • edited Loading

Links

Steps for Testing/QA

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock left a comment

Choose a reason for hiding this comment

kbrock commented May 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickLaMuro commented May 19, 2017

miq-bot commented May 22, 2017

kbrock commented May 31, 2017

NickLaMuro commented Jun 1, 2017 • edited Loading

gtanzillo left a comment

Choose a reason for hiding this comment

kbrock commented Jun 7, 2017

Choose a reason for hiding this comment

NickLaMuro commented Jun 7, 2017

jrafanie commented Jun 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickLaMuro commented Jun 7, 2017 • edited by chessbyte Loading

NickLaMuro commented Jun 7, 2017

miq-bot commented Jun 7, 2017

miq-bot commented Jun 7, 2017

NickLaMuro commented May 17, 2017 •

edited

Loading

NickLaMuro commented Jun 1, 2017 •

edited

Loading

NickLaMuro commented Jun 7, 2017 •

edited by chessbyte

Loading