Make spawn pass worker options again #16199

jrafanie · 2017-10-13T15:50:18Z

When #16130 was merged, it revealed that spawn was not properly passing worker_options, specifically, the ems id needed by provider workers.

That PR was reverted and now we need to get spawn working properly again.

This PR adds the --ems-id option to run_single_worker.rb, updates the Procfile example to leverage this instead of the QUEUE environment variable, removes support for that env variable and the now unused MIQ_GUID environment variable.

There's still some bugs to fix with spawn + BUNDLER_GROUPS excluding ui-classic for all non-ui workers but those can be done as follows up while fork is still the default (report_formatter in ui repo being used by our reporting code, ick).

We also want to support passing multiple ems id values for amazon workers cc @juliancheal @agrare but that will be done also as a followup to this PR.

To test this PR as is, you'll need to enable spawn workers locally: MIQ_SPAWN_WORKERS=true bin/rake evm:start or use the Procfile via bin/rake evm:foreman:start and make sure your provider workers get the proper ems_id passed down.

agrare

👍 tested with 2 vSphere providers and a SCVMM provider, and confirmed it doesn't work for Amazon/multiple ems_ids yet

jrafanie · 2017-10-16T21:24:30Z

@agrare according to a link @juliancheal sent me here, we shouldn't need to pass multiple ems_ids to the runner initialize. It sounds like as long as you have an amazon ems with many managers, it should automatically, pickup the managers in the refresh worker.

jrafanie · 2017-10-16T21:26:19Z

@agrare Or, more importantly, if we need to have multiple ems_id's passed to the runner initialize, what is the proper queue name for that worker?

agrare · 2017-10-17T14:01:37Z

@jrafanie hm well amazon workers weren't starting up so there is something else up.
@juliancheal can you help debug this?

juliancheal · 2017-10-17T14:14:24Z

Not at work today. But can look tonight.

…

Sent from my iPhone

On 17 Oct 2017, at 15:01, Adam Grare ***@***.***> wrote: @jrafanie hm well amazon workers weren't starting up so there is something else up. @juliancheal can you help debug this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

carbonin

This looks good to me.

Just waiting on @juliancheal to give the 👍 for the amazon multi-manager stuff.

carbonin · 2017-10-23T13:24:55Z

app/models/miq_worker/runner.rb

@@ -43,8 +43,6 @@ def self.interrupt_signals

  def initialize(cfg = {})
    @cfg = cfg
-    @cfg[:guid] ||= ENV['MIQ_GUID']


carbonin · 2017-10-23T13:29:54Z

app/models/miq_worker.rb

@@ -370,8 +375,12 @@ def self.runner_script
    script
  end

+  def command_line
+    self.class.build_command_line(worker_options)


If guid and ems_id are the only things we are using worker_options for, I think we should probably make this less opaque and pass those values explicitly.

The fact that worker_options is a hash makes it tough to see if we are covering all the cases for all the worker sub classes.

This can definitely be done in a follow up though.

juliancheal · 2017-10-24T11:45:47Z

I need to see what's going on, as so far with Amazon we get as far as this

 id  |                               type                        |  pid  |               q                |  status  |        
 321 | ManageIQ::Providers::Amazon::CloudManager::RefreshWorker  | 12426 | ["ems_12", "ems_13", "ems_14"] | creating |

jrafanie · 2017-10-24T19:32:38Z

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)


This one is for @carbonin 🎁

jrafanie · 2017-10-24T19:33:32Z

@juliancheal I added a commit to address @carbonin's concern about YAOH (yet another options hash)

juliancheal · 2017-10-24T22:08:01Z

Fixed the Amazon provider issue over here #16297

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"]

miq-bot · 2017-10-25T02:13:04Z

Checked commits jrafanie/manageiq@26b5ac9~...1c75bc4 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
6 files checked, 0 offenses detected
Everything looks fine. 🏆

juliancheal · 2017-10-25T13:30:16Z

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|


This could probably be explained better, and formatted better.

juliancheal · 2017-10-25T13:32:50Z

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 }
  end


I really don't like how I did this. The problem is, create_options[:queue_name] & runner_options[:ems_id] can accept an array when there are multiple ems ids. However when there is just a singular ems id, I don't think that works as well. So I am trying to pass a single ems id, but it just makes this look a mess.

Any suggestions to make it look nicer?

Let's try to find a better way to do it. I can't think of one right now. Your changes are great for now. We can clean this up when we find a better place for this logic.

Make systemd optional on systemd enabled systems Api/web service worker needs ui-classic as it can try to read an existing UI session, which can contain serialized classes from ui-classic. Previously, we tried removing fork here: ManageIQ#16130 It was reverted here: ManageIQ#16154 Some of the followups needed to fix the original problems including passing down ems_id to per ems workers, resolved in: ManageIQ#16199 and ManageIQ#18648 At this point, things should just work.

jrafanie requested review from Fryguy, carbonin and agrare October 13, 2017 15:50

jrafanie force-pushed the make_spawn_pass_options_again branch 2 times, most recently from 300efc5 to 3bd2f4e Compare October 13, 2017 15:54

chessbyte assigned gtanzillo Oct 13, 2017

chessbyte added bug core/workers labels Oct 13, 2017

agrare approved these changes Oct 13, 2017

View reviewed changes

carbonin approved these changes Oct 23, 2017

View reviewed changes

jrafanie force-pushed the make_spawn_pass_options_again branch 2 times, most recently from 0539b65 to 54462a1 Compare October 24, 2017 19:30

jrafanie commented Oct 24, 2017

View reviewed changes

jrafanie and others added 6 commits October 24, 2017 22:09

Pass worker_options as command arguments

26b5ac9

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.

7a20aa2

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

8e013be

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

1c75bc4

jrafanie force-pushed the make_spawn_pass_options_again branch from 54462a1 to 1c75bc4 Compare October 25, 2017 02:09

jrafanie mentioned this pull request Oct 25, 2017

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

Closed

juliancheal reviewed Oct 25, 2017

View reviewed changes

gtanzillo added this to the Sprint 72 Ending Oct 30, 2017 milestone Oct 25, 2017

gtanzillo merged commit b60ef1f into ManageIQ:master Oct 25, 2017

Fryguy deleted the make_spawn_pass_options_again branch October 25, 2017 16:25

jrafanie mentioned this pull request Nov 25, 2019

Drop support for forking workers, use spawn by default #19556

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make spawn pass worker options again #16199

Make spawn pass worker options again #16199

jrafanie commented Oct 13, 2017

agrare left a comment

jrafanie commented Oct 16, 2017

jrafanie commented Oct 16, 2017

agrare commented Oct 17, 2017

juliancheal commented Oct 17, 2017 via email

carbonin left a comment

carbonin Oct 23, 2017

carbonin Oct 23, 2017

juliancheal commented Oct 24, 2017 •

edited

Loading

jrafanie Oct 24, 2017

jrafanie commented Oct 24, 2017

juliancheal commented Oct 24, 2017

miq-bot commented Oct 25, 2017

juliancheal Oct 25, 2017

juliancheal Oct 25, 2017 •

edited

Loading

jrafanie Oct 25, 2017

Make spawn pass worker options again #16199

Make spawn pass worker options again #16199

Conversation

jrafanie commented Oct 13, 2017

agrare left a comment

Choose a reason for hiding this comment

jrafanie commented Oct 16, 2017

jrafanie commented Oct 16, 2017

agrare commented Oct 17, 2017

juliancheal commented Oct 17, 2017 via email

carbonin left a comment

Choose a reason for hiding this comment

carbonin Oct 23, 2017

Choose a reason for hiding this comment

carbonin Oct 23, 2017

Choose a reason for hiding this comment

juliancheal commented Oct 24, 2017 • edited Loading

jrafanie Oct 24, 2017

Choose a reason for hiding this comment

jrafanie commented Oct 24, 2017

juliancheal commented Oct 24, 2017

miq-bot commented Oct 25, 2017

juliancheal Oct 25, 2017

Choose a reason for hiding this comment

juliancheal Oct 25, 2017 • edited Loading

Choose a reason for hiding this comment

jrafanie Oct 25, 2017

Choose a reason for hiding this comment

juliancheal commented Oct 24, 2017 •

edited

Loading

juliancheal Oct 25, 2017 •

edited

Loading