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.

Sign up for GitHub

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 signal handling into the MiqServer object #15206

Merged
merged 3 commits into from
May 23, 2017
Merged

Move signal handling into the MiqServer object #15206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a4504ac
Reraise SignalExceptions raised in MiqServer#monitor
carbonin May 23, 2017
37b0761
Move signal handling into the MiqServer object
carbonin May 19, 2017
9944176
Move signal handling specs from evm_server_spec.rb to miq_server_spec.rb
carbonin May 23, 2017
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
9 changes: 8 additions & 1 deletion app/models/miq_server.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ def monitor
Benchmark.realtime_block(:log_active_servers) { log_active_servers } if threshold_exceeded?(:server_log_frequency, now)
Benchmark.realtime_block(:worker_monitor) { monitor_workers } if threshold_exceeded?(:worker_monitor_frequency, now)
Benchmark.realtime_block(:worker_dequeue) { populate_queue_messages } if threshold_exceeded?(:worker_dequeue_frequency, now)
rescue SystemExit
rescue SystemExit, SignalException
# TODO: We're rescuing Exception below. WHY? :bomb:
# A SystemExit would be caught below, so we need to explicitly rescue/raise.
raise
Expand All @@ -369,6 +369,13 @@ def monitor_loop
_log.info "Server Monitoring Complete - Timings: #{timings.inspect}" unless timings[:total_time] < server_log_timings_threshold
sleep monitor_poll
end
rescue Interrupt => e
_log.info("Received #{e.message} signal, killing server")
self.class.kill
exit 1
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 got squashed from @Fryguy

I think it's really strange for a regular method on a model to exit 1, however that being said, this is no normal method. We really need to rip out these "MiqServer" classes into standalone classes.

Copy link
Member Author

@carbonin carbonin May 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@Fryguy I agree, the alternative was to create a separate method (like #kill_and_exit or something) and just call it from here.

I thought that was a bit overkill, but I have no real opinion.

Copy link
Member

Choose a reason for hiding this comment

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

Why not just raise instead of exit 1?

Copy link
Member

Choose a reason for hiding this comment

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

Although, to be fair, we were exiting before so I guess that's unchanged behavior.

rescue SignalException => e
Copy link
Member

Choose a reason for hiding this comment

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

Note, we're now handling other soft signals I believe. I don't recall why we were only handling term, usr1, and usr2 as soft signals before. Maybe it doesn't matter?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was going with "it doesn't matter". Plus I think this is objectively better behavior. The alternative would be to exit the server process with the exception (it was being re-raised previously) and have the workers go down with DRb connection errors.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, let's see what happens and make it more complicated if we need to

_log.info("Received #{e.message} signal, shutting down server")
shutdown_and_exit
end

def stop(sync = false)
Expand Down
43 changes: 0 additions & 43 deletions lib/workers/evm_server.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
require 'pid_file'

class EvmServer
SOFT_INTERRUPT_SIGNALS = ["SIGTERM", "SIGUSR1", "SIGUSR2"]

OPTIONS_PARSER_SETTINGS = [
[:mode, 'EVM Server Mode', String],
]
Expand All @@ -18,42 +16,6 @@ def initialize(cfg = {})
$log ||= Rails.logger
end

def process_hard_signal(s)
exit_code = 1
message = "Interrupt signal (#{s}) received."
begin
safe_log(message, exit_code)
MiqServer.kill
ensure
do_exit(message, exit_code)
end
end

def process_soft_signal(s)
MiqServer.stop
ensure
do_exit("Interrupt signal (#{s}) received.", 0)
end

def do_exit(message = nil, exit_code = 0)
safe_log("#{message} Server exiting.", exit_code)
exit exit_code
end

def safe_log(message = nil, exit_code = 0)
meth = (exit_code == 0) ? :info : :error

prefix = "MIQ(EvmServer) "
pid = "PID [#{Process.pid}] " rescue ""
logmsg = "#{prefix}#{pid}#{message}"

begin
$log.send(meth, logmsg)
rescue
puts "#{meth.to_s.upcase}: #{logmsg}" rescue nil
end
end

def start
if pid = MiqServer.running?
$log.warn("EVM is already running (PID=#{pid})")
Expand All @@ -63,11 +25,6 @@ def start
PidFile.create(MiqServer.pidfile)
set_process_title
MiqServer.start
rescue Interrupt => e
process_hard_signal(e.message)
rescue SignalException => e
raise unless SOFT_INTERRUPT_SIGNALS.include?(e.message)
process_soft_signal(e.message)
end

##
Expand Down
31 changes: 0 additions & 31 deletions spec/lib/workers/evm_server_spec.rb
View file
Open in desktop

This file was deleted.

17 changes: 17 additions & 0 deletions spec/models/miq_server_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,23 @@
@guid, @miq_server, @zone = EvmSpecHelper.create_guid_miq_server_zone
end

describe "#monitor_loop" do
it "calls shutdown_and_exit if SIGTERM is raised" do
expect(@miq_server).to receive(:monitor).and_raise(SignalException, "SIGTERM")
expect(@miq_server).to receive(:shutdown_and_exit)

@miq_server.monitor_loop
end

it "kills the server and exits if SIGINT is raised" do
expect(@miq_server).to receive(:monitor).and_raise(Interrupt)
expect(MiqServer).to receive(:kill)
expect(@miq_server).to receive(:exit).with(1)

@miq_server.monitor_loop
end
end

it "should have proper guid" do
expect(@miq_server.guid).to eq(@guid)
end
Expand Down