[WIP] [Don't merge] Make spawn pass options again #16297

juliancheal · 2017-10-24T21:16:42Z

This change allows spawn to be passed multiple ids (or just one id) in the format 1,2,3,4 with no spaces.

The queue_name is then generated from the ids to look like either ems_1 or ["ems_1", "ems_2", "ems_3"]

I have tested this on the command line with:

ruby lib/workers/bin/run_single_worker.rb --ems-id 17,19,18 ManageIQ::Providers::Amazon::CloudManager::RefreshWorker

And run MIQ Server using MIQ_SPAWN_WORKERS=true

Fixes a bug first discovered after ManageIQ#16130 was merged. That PR was subsequently reverted. We'll remove fork support once spawn has complete parity without bugs.

Don't break an interface for yet another option hash. Change the tests to not care about the order of some of the CLI arguments since awesome spawn builds them in hash insertion order and we don't really care of the order of some of them anyway.

Sometimes a provider handles multiple managers with a single worker. This change allows spawn to be passed multiple ids (or just one id) in the format 1,2,3,4 with no spaces. The queue_name is then generated from the ids to look like either ems_1 or ["ems_1", "ems_2", "ems_3"]

juliancheal · 2017-10-24T21:47:18Z

Lol after all that, still back where we started.

id  |                                type                      |  pid  |               q                |  status  | 
456 | ManageIQ::Providers::Amazon::CloudManager::RefreshWorker | 21508 | ["ems_17", "ems_18", "ems_19"] | creating |

juliancheal · 2017-10-24T22:03:27Z

BOOM 🔥 Working @jrafanie

id  |                                type                      |  pid  |               q                |  status | created
464 | ManageIQ::Providers::Amazon::CloudManager::RefreshWorker | 24319 | ["ems_17", "ems_18", "ems_19"] | started | 2017-10-24 22:01:46.12679

NickLaMuro

Just some thoughts, but nothing show stopping. Seems like it should work, but I didn't test...

#lazyreview

NickLaMuro · 2017-10-24T22:27:42Z

lib/workers/bin/run_single_worker.rb

-    runner_options[:ems_id] = worker_class.ems_id_from_queue_name(ENV["QUEUE"]) if worker_class.respond_to?(:ems_id_from_queue_name)
+  if options[:ems_id]
+    create_options[:queue_name] = options[:ems_id].length == 1 ? "ems_#{options[:ems_id].first}" : options[:ems_id].collect { |id| "ems_#{id}" }
+    runner_options[:ems_id]     = options[:ems_id].length == 1 ? options[:ems_id].first : options[:ems_id].collect { |id| id }


Can you get rid of your last commit and just turn this [:ems_id] line into this?

runner_options[:ems_id] = worker_class.ems_id_from_queue_name(create_options[:queue_name]) if worker_class.respond_to?(:ems_id_from_queue_name)

The reason I am suggesting this is the code in ems_id_from_queue_name should already be tested, where as this script isn't (something I would prefer, but haven't gotten around to refactoring), as well as it seems like these two lines are duplicate work.

Scratch the bit about getting rid of your last commit... that was handling allowing MiqServer spawning workers with multiple Ems ids and sending it to the script, not the other way around.

Agreed with the testing!

I really don't like any of these lines tbh.

I did think the same thing with ems_id_from_queue_name its what we want, as this really is just duplicating that, I feel to an extent.

Yeah, I'd like to understand how we can move this knowledge and write tests for it. It's fine here for now. Testing by using foreman/Procfile is not easy to test all the various combinations.

NickLaMuro · 2017-10-24T22:36:12Z

app/models/miq_worker.rb

-  def self.build_command_line(guid)
-    command_line = "#{Gem.ruby} #{runner_script} --heartbeat --guid=#{guid} #{name}"
-    ENV['APPLIANCE'] ? "nice #{nice_increment} #{command_line}" : command_line
+  def self.build_command_line(guid, ems_id = nil)


Is there a reason this is a class method? I realize this was already here, just not sure why we are going from a instance method to a class method in this case.

¯\(ツ)/¯ @Fryguy @jrafanie

@NickLaMuro Yes, but I don't believe they're good reasons. I can't recall but it's worth changing in a followup PR if it's easy. There's a few class methods that would need to also change to instance methods.

NickLaMuro · 2017-10-24T22:37:57Z

lib/workers/bin/run_single_worker.rb

@@ -42,7 +46,6 @@
  exit
 end
 opt_parser.abort(opt_parser.help) unless worker_class
-


Oh that'll be me putting in logging and when I removed it i've changed the formatting. That or Atom did it. Yeah I'll blame Atom.

miq-bot · 2017-10-24T23:27:32Z

Checked commits juliancheal/manageiq@255ea34~...e0041d4 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
6 files checked, 0 offenses detected
Everything looks fine. ⭐

jrafanie · 2017-10-25T01:54:53Z

lib/workers/bin/run_single_worker.rb

@@ -27,6 +27,10 @@
    options[:guid] = val
  end

+  opts.on("-e=ems_id", "--ems-id=ems_id,ems_id", Array, "Provide a list of ems ids (without spaces) to a provider worker. This requires, at least one argument.") do |val|


Mr. Fancypants using Array. Someone read the optparse documentation. 😎
Nice @juliancheal

I admit to that catching me off guard when I first read that as well.

@jrafanie Hehehe, thanks!

jrafanie · 2017-10-25T01:57:34Z

#lazyreview

LOL @NickLaMuro

jrafanie · 2017-10-25T02:11:27Z

@juliancheal I applied your changes to the original PR #16199... We can decide which PR we want to keep tomorrow

juliancheal · 2017-10-25T11:51:27Z

@jrafanie you're PR has all the good comments, mine is full of snark. Trololo

jrafanie and others added 4 commits October 24, 2017 15:28

Pass worker_options as command arguments

255ea34

Fixes a bug first discovered after ManageIQ#16130 was merged. That PR was subsequently reverted. We'll remove fork support once spawn has complete parity without bugs.

Remove now unused MIQ_GUID environment variable.

7edeff0

juliancheal changed the title ~~Make spawn pass options again~~ [WIP] [Don't merge] Make spawn pass options again Oct 24, 2017

miq-bot added the wip label Oct 24, 2017

We don't want to pass arrays of one item to runner_options

85d2b7d

juliancheal mentioned this pull request Oct 24, 2017

Make spawn pass worker options again #16199

Merged

NickLaMuro approved these changes Oct 24, 2017

View reviewed changes

Need to pass the properly formated string of ids to single worker runner

e0041d4

juliancheal force-pushed the make_spawn_pass_options_again branch from 5b3a375 to e0041d4 Compare October 24, 2017 23:23

jrafanie reviewed Oct 25, 2017

View reviewed changes

juliancheal closed this Oct 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] [Don't merge] Make spawn pass options again #16297

[WIP] [Don't merge] Make spawn pass options again #16297

juliancheal commented Oct 24, 2017 •

edited

Loading

juliancheal commented Oct 24, 2017 •

edited

Loading

juliancheal commented Oct 24, 2017 •

edited

Loading

NickLaMuro left a comment

NickLaMuro Oct 24, 2017

NickLaMuro Oct 24, 2017

juliancheal Oct 24, 2017

juliancheal Oct 24, 2017

juliancheal Oct 25, 2017

jrafanie Oct 25, 2017

NickLaMuro Oct 24, 2017

juliancheal Oct 24, 2017

jrafanie Oct 25, 2017

NickLaMuro Oct 24, 2017

juliancheal Oct 24, 2017

jrafanie Oct 25, 2017

miq-bot commented Oct 24, 2017

jrafanie Oct 25, 2017

NickLaMuro Oct 25, 2017

juliancheal Oct 25, 2017

jrafanie commented Oct 25, 2017

jrafanie commented Oct 25, 2017

juliancheal commented Oct 25, 2017

[WIP] [Don't merge] Make spawn pass options again #16297

[WIP] [Don't merge] Make spawn pass options again #16297

Conversation

juliancheal commented Oct 24, 2017 • edited Loading

juliancheal commented Oct 24, 2017 • edited Loading

juliancheal commented Oct 24, 2017 • edited Loading

NickLaMuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie commented Oct 25, 2017

jrafanie commented Oct 25, 2017

juliancheal commented Oct 25, 2017

juliancheal commented Oct 24, 2017 •

edited

Loading

juliancheal commented Oct 24, 2017 •

edited

Loading

juliancheal commented Oct 24, 2017 •

edited

Loading