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

Make spawn pass worker options again #16199

Merged
merged 6 commits into from
Oct 25, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions Procfile.example
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,9 @@
# These workers subscribe to a queue named based on the particular EMS they connect to
# The "<id>" tag in these lines should be replaced by the id of the manager instance these workers apply to

# vmware_refresh_<id>: env QUEUE=ems_<id> ruby lib/workers/bin/run_single_worker.rb ManageIQ::Providers::Vmware::InfraManager::RefreshWorker
# vmware_event_<id>: env QUEUE=ems_<id> ruby lib/workers/bin/run_single_worker.rb ManageIQ::Providers::Vmware::InfraManager::EventCatcher
# vmware_refresh_core_<id>: env QUEUE=ems_<id> ruby lib/workers/bin/run_single_worker.rb MiqEmsRefreshCoreWorker
# vmware_refresh_<id>: ruby lib/workers/bin/run_single_worker.rb --ems-id <id> ManageIQ::Providers::Vmware::InfraManager::RefreshWorker
# vmware_event_<id>: ruby lib/workers/bin/run_single_worker.rb --ems-id <id> ManageIQ::Providers::Vmware::InfraManager::EventCatcher
# vmware_refresh_core_<id>: ruby lib/workers/bin/run_single_worker.rb --ems-id <id> MiqEmsRefreshCoreWorker

# Logs
#
Expand Down
21 changes: 17 additions & 4 deletions app/models/miq_worker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -359,9 +359,18 @@ def start_runner_via_fork
pid
end

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)
Copy link
Member Author

Choose a reason for hiding this comment

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

This one is for @carbonin 🎁

raise ArgumentError, "No guid provided" unless guid

require 'awesome_spawn'
cmd = "#{Gem.ruby} #{runner_script}"
cmd = "nice #{nice_increment} #{cmd}" if ENV["APPLIANCE"]

options = {:guid => guid, :heartbeat => nil}
if ems_id
options[:ems_id] = ems_id.kind_of?(Array) ? ems_id.join(",") : ems_id
end
"#{AwesomeSpawn::CommandLineBuilder.new.build(cmd, options)} #{name}"
end

def self.runner_script
Expand All @@ -370,8 +379,12 @@ def self.runner_script
script
end

def command_line
self.class.build_command_line(*worker_options.values_at(:guid, :ems_id))
end

def start_runner_via_spawn
pid = Kernel.spawn(self.class.build_command_line(guid), [:out, :err] => [Rails.root.join("log", "evm.log"), "a"])
pid = Kernel.spawn(command_line, [:out, :err] => [Rails.root.join("log", "evm.log"), "a"])
Process.detach(pid)
pid
end
Expand Down
2 changes: 0 additions & 2 deletions app/models/miq_worker/runner.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,6 @@ def self.interrupt_signals

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

Choose a reason for hiding this comment

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

👍


$log ||= Rails.logger

@server = MiqServer.my_server(true)
Expand Down
1 change: 0 additions & 1 deletion app/models/mixins/miq_web_server_worker_mixin.rb
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,6 @@ def rails_application

def start
delete_pid_file
ENV['MIQ_GUID'] = guid
super
end

Expand Down
2 changes: 1 addition & 1 deletion lib/vmdb/initializer.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
module Vmdb
module Initializer
def self.init
_log.info("- Program Name: #{$PROGRAM_NAME}, PID: #{Process.pid}, ENV['MIQ_GUID']: #{ENV['MIQ_GUID']}, ENV['EVMSERVER']: #{ENV['EVMSERVER']}")
_log.info("- Program Name: #{$PROGRAM_NAME}, PID: #{Process.pid}, ENV['EVMSERVER']: #{ENV['EVMSERVER']}")

# UiWorker called in Development Mode
# * command line(rails server)
Expand Down
10 changes: 7 additions & 3 deletions lib/workers/bin/run_single_worker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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|
Copy link
Member

Choose a reason for hiding this comment

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

This could probably be explained better, and formatted better.

options[:ems_id] = val
end

opts.on("-h", "--help", "Displays this help") do
puts opts
exit
Expand Down Expand Up @@ -60,9 +64,9 @@
create_options = {:pid => Process.pid}
runner_options = {}

if ENV["QUEUE"]
create_options[:queue_name] = ENV["QUEUE"]
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
Copy link
Member

@juliancheal juliancheal Oct 25, 2017

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


worker = if options[:guid]
Expand Down
56 changes: 30 additions & 26 deletions spec/models/miq_worker_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,32 +29,6 @@ def all_workers
expect(result.command_line).to eq "renice -n 5 -p 123"
end

context ".build_command_line" do
before do
allow(MiqGenericWorker).to receive(:nice_increment).and_return("+10")
end

it "with ENV['APPLIANCE']" do
begin
old_env = ENV.delete('DATABASE_URL')
ENV['APPLIANCE'] = 'true'
w = FactoryGirl.build(:miq_generic_worker)
cmd = w.class.build_command_line(123)
expect(cmd).to start_with("nice +10")
expect(cmd).to end_with("MiqGenericWorker")
ensure
# ENV['x'] = nil deletes the key because ENV accepts only string values
ENV['APPLIANCE'] = old_env
end
end

it "without ENV['APPLIANCE']" do
w = FactoryGirl.build(:miq_generic_worker)
cmd = w.class.build_command_line(123)
expect(cmd).to_not start_with("nice +10")
end
end

context ".has_required_role?" do
def check_has_required_role(worker_role_names, expected_result)
allow(described_class).to receive(:required_roles).and_return(worker_role_names)
Expand Down Expand Up @@ -371,6 +345,36 @@ def check_has_required_role(worker_role_names, expected_result)
expect(@worker.worker_options).to eq(:guid => @worker.guid)
end

context "#command_line" do
it "without guid in worker_options" do
allow(@worker).to receive(:worker_options).and_return({})
expect { @worker.command_line }.to raise_error(ArgumentError)
end

it "without ENV['APPLIANCE']" do
allow(@worker).to receive(:worker_options).and_return(:ems_id => 1234, :guid => @worker.guid)
expect(@worker.command_line).to_not include("nice")
end

it "with ENV['APPLIANCE']" do
begin
allow(MiqWorker).to receive(:nice_increment).and_return("+10")
allow(@worker).to receive(:worker_options).and_return(:ems_id => 1234, :guid => @worker.guid)
old_env = ENV.delete('APPLIANCE')
ENV['APPLIANCE'] = 'true'
cmd = @worker.command_line
expect(cmd).to start_with("nice +10")
expect(cmd).to include("--ems-id 1234")
expect(cmd).to include("--guid #{@worker.guid}")
expect(cmd).to include("--heartbeat")
expect(cmd).to end_with("MiqWorker")
ensure
# ENV['x'] = nil deletes the key because ENV accepts only string values
ENV['APPLIANCE'] = old_env
end
end
end

describe "#stopping_for_too_long?" do
subject { @worker.stopping_for_too_long? }

Expand Down