Reconfigure signal trapping.

Exit gracefully when sent TERM

Fix spec.

3 files changed, 133 insertions, 60 deletions

diff --git a/lib/chef/application.rb b/lib/chef/application.rb
index c007867f2b..8a0c82d39d 100644
--- a/lib/chef/application.rb
+++ b/lib/chef/application.rb
@@ -62,6 +62,10 @@ class Chef::Application
       Chef::Application.fatal!("SIGINT received, stopping", 2)
     end
 
+    trap("TERM") do
+      Chef::Application.fatal!("SIGTERM received, stopping", 3)
+    end
+
     unless Chef::Platform.windows?
       trap("QUIT") do
         Chef::Log.info("SIGQUIT received, call stack:\n  " + caller.join("\n  "))
@@ -206,44 +210,66 @@ class Chef::Application
       )
       @chef_client_json = nil
 
-      if Chef::Config[:client_fork]
+      if can_fork?
         fork_chef_client # allowed to run client in forked process
       else
-        @chef_client.run
+        # Unforked interval runs are disabled, so this runs chef-client
+        # once and then exits. If TERM signal is received, will "ignore"
+        # the signal to finish converge, then exit with exitstatus 3.
+        run_with_graceful_exit_option
       end
       @chef_client = nil
     end
   end
 
   private
-  def fork_chef_client
+  def can_fork?
     # win32-process gem exposes some form of :fork for Process
     # class. So we are seperately ensuring that the platform we're
     # running on is not windows before forking.
-    if(Process.respond_to?(:fork) && !Chef::Platform.windows?)
-      Chef::Log.info "Forking chef instance to converge..."
-      pid = fork do
-        [:INT, :TERM].each {|s| trap(s, "EXIT") }
-        client_solo = Chef::Config[:solo] ? "chef-solo" : "chef-client"
-        $0 = "#{client_solo} worker: ppid=#{Process.ppid};start=#{Time.new.strftime("%R:%S")};"
-        begin
-          Chef::Log.debug "Forked instance now converging"
-          @chef_client.run
-        rescue Exception => e
-          Chef::Log.error(e.to_s)
-          exit 1
-        else
-          exit 0
-        end
+    Chef::Config[:client_fork] && Process.respond_to?(:fork) && !Chef::Platform.windows?
+  end
+
+  # Run chef-client once and then exit. If TERM signal is received, ignores the
+  # signal to finish the converge and exists.
+  def run_with_graceful_exit_option
+    # Override the TERM signal.
+    trap('TERM') do
+      Chef::Log.debug("SIGTERM received during converge," +
+        " finishing converge to exit normally (send SIGINT to terminate immediately)")
+    end
+
+    @chef_client.run
+    true
+  end
+
+  def fork_chef_client
+    Chef::Log.info "Forking chef instance to converge..."
+    pid = fork do
+      # Want to allow forked processes to finish converging when
+      # TERM singal is received (exit gracefully)
+      trap('TERM') do
+        Chef::Log.debug("SIGTERM received during converge," +
+          " finishing converge to exit normally (send SIGINT to terminate immediately)")
+      end
+
+      client_solo = Chef::Config[:solo] ? "chef-solo" : "chef-client"
+      $0 = "#{client_solo} worker: ppid=#{Process.ppid};start=#{Time.new.strftime("%R:%S")};"
+      begin
+        Chef::Log.debug "Forked instance now converging"
+        @chef_client.run
+      rescue Exception => e
+        Chef::Log.error(e.to_s)
+        exit 1
+      else
+        exit 0
       end
-      Chef::Log.debug "Fork successful. Waiting for new chef pid: #{pid}"
-      result = Process.waitpid2(pid)
-      handle_child_exit(result)
-      Chef::Log.debug "Forked instance successfully reaped (pid: #{pid})"
-      true
-    else
-      @chef_client.run
     end
+    Chef::Log.debug "Fork successful. Waiting for new chef pid: #{pid}"
+    result = Process.waitpid2(pid)
+    handle_child_exit(result)
+    Chef::Log.debug "Forked instance successfully reaped (pid: #{pid})"
+    true
   end
 
   def handle_child_exit(pid_and_status)
diff --git a/lib/chef/application/client.rb b/lib/chef/application/client.rb
index aa394cb911..098804739e 100644
--- a/lib/chef/application/client.rb
+++ b/lib/chef/application/client.rb
@@ -239,7 +239,6 @@ class Chef::Application::Client < Chef::Application
   end
 
   IMMEDIATE_RUN_SIGNAL = "1".freeze
-  GRACEFUL_EXIT_SIGNAL = "2".freeze
 
   attr_reader :chef_client_json
 
@@ -298,8 +297,9 @@ class Chef::Application::Client < Chef::Application
     Chef::Daemon.change_privilege
   end
 
-  # Run the chef client, optionally daemonizing or looping at intervals.
-  def run_application
+  def setup_signal_handlers
+    super
+
     unless Chef::Platform.windows?
       SELF_PIPE.replace IO.pipe
 
@@ -307,14 +307,6 @@ class Chef::Application::Client < Chef::Application
         Chef::Log.info("SIGUSR1 received, waking up")
         SELF_PIPE[1].putc(IMMEDIATE_RUN_SIGNAL) # wakeup master process from select
       end
-
-      # see CHEF-5172
-      if Chef::Config[:daemonize] || Chef::Config[:interval]
-        trap("TERM") do
-          Chef::Log.info("SIGTERM received, exiting gracefully")
-          SELF_PIPE[1].putc(GRACEFUL_EXIT_SIGNAL)
-        end
-      end
     end
   end
 
@@ -327,7 +319,7 @@ class Chef::Application::Client < Chef::Application
     if !Chef::Config[:client_fork] || Chef::Config[:once]
       begin
         # run immediately without interval sleep, or splay
-        run_chef_client(Chef::Config[:specific_recipes])
+        run_chef_client(Chef::Config[:specific_recipes] || [])
       rescue SystemExit
         raise
       rescue Exception => e
@@ -344,18 +336,17 @@ class Chef::Application::Client < Chef::Application
       Chef::Daemon.daemonize("chef-client")
     end
 
-    signal = nil
-
     loop do
       begin
-        Chef::Application.exit!("Exiting", 0) if signal == GRACEFUL_EXIT_SIGNAL
-
-        if signal != IMMEDIATE_RUN_SIGNAL
-          signal = interval_sleep(time_to_sleep)
+        @signal = test_signal
+        if @signal != IMMEDIATE_RUN_SIGNAL
+          sleep_sec = time_to_sleep
+          Chef::Log.debug("Sleeping for #{sleep_sec} seconds")
+          interval_sleep(sleep_sec)
         end
 
-        signal = nil
-        run_chef_client(Chef::Config[:specific_recipes])
+        @signal = nil
+        run_chef_client(Chef::Config[:specific_recipes] || [])
 
         Chef::Application.exit!("Exiting", 0) if !Chef::Config[:interval]
       rescue SystemExit => e
@@ -372,6 +363,10 @@ class Chef::Application::Client < Chef::Application
     end
   end
 
+  def test_signal
+    client_sleep(0)
+  end
+
   def time_to_sleep
     duration = 0
     duration += rand(Chef::Config[:splay]) if Chef::Config[:splay]
@@ -380,7 +375,6 @@ class Chef::Application::Client < Chef::Application
   end
 
   def interval_sleep(sec)
-    Chef::Log.debug("Sleeping for #{sec} seconds")
     unless SELF_PIPE.empty?
       client_sleep(sec)
     else
@@ -391,7 +385,7 @@ class Chef::Application::Client < Chef::Application
 
   def client_sleep(sec)
     IO.select([ SELF_PIPE[0] ], nil, nil, sec) or return
-    SELF_PIPE[0].getc.chr
+    @signal = SELF_PIPE[0].getc.chr
   end
 
   def unforked_interval_error_message
diff --git a/spec/unit/application/client_spec.rb b/spec/unit/application/client_spec.rb
index f0b4f3bda8..a3599e2ac0 100644
--- a/spec/unit/application/client_spec.rb
+++ b/spec/unit/application/client_spec.rb
@@ -168,28 +168,78 @@ end
 
 describe Chef::Application::Client, "run_application", :unix_only do
   before(:each) do
-    Chef::Config[:daemonize] = true
-    @pipe = IO.pipe
     @app = Chef::Application::Client.new
+    @app.setup_signal_handlers
     # Default logger doesn't work correctly when logging from a trap handler.
     @app.configure_logging
-    Chef::Daemon.stub(:daemonize).and_return(true)
-    @app.stub(:run_chef_client) do
+
+    @pipe = IO.pipe
+    @client = Chef::Client.new
+    Chef::Client.stub(:new).and_return(@client)
+    @client.stub(:run) do
       @pipe[1].puts 'started'
       sleep 1
       @pipe[1].puts 'finished'
     end
   end
 
-  it "should exit gracefully when sent SIGTERM", :volatile_on_solaris do
-    pid = fork do
-      @app.run_application
+  context "when sent SIGTERM", :volatile_on_solaris do
+    context "when converging in forked process" do
+      before do
+        Chef::Config[:daemonize] = true
+        Chef::Daemon.stub(:daemonize).and_return(true)
+      end
+
+      it "should exit hard with exitstatus 3" do
+        pid = fork do
+          @app.run_application
+        end
+        Process.kill("TERM", pid)
+        _pid, result = Process.waitpid2(pid)
+        result.exitstatus.should == 3
+      end
+
+      it "should allow child to finish converging" do
+        pid = fork do
+          @app.run_application
+        end
+        @pipe[0].gets.should == "started\n"
+        Process.kill("TERM", pid)
+        Process.wait
+        sleep 1 # Make sure we give the converging child process enough time to finish
+        IO.select([@pipe[0]], nil, nil, 0).should_not be_nil
+        @pipe[0].gets.should == "finished\n"
+      end
+    end
+
+    context "when running unforked" do
+      before(:each) do
+        Chef::Config[:client_fork] = false
+        Chef::Config[:daemonize] = false
+      end
+
+      it "should exit gracefully when sent during converge" do
+        pid = fork do
+          @app.run_application
+        end
+        @pipe[0].gets.should == "started\n"
+        Process.kill("TERM", pid)
+        _pid, result = Process.waitpid2(pid)
+        result.exitstatus.should == 0
+        IO.select([@pipe[0]], nil, nil, 0).should_not be_nil
+        @pipe[0].gets.should == "finished\n"
+      end
+
+      it "should exit hard when sent before converge" do
+        pid = fork do
+          sleep 3
+          @app.run_application
+        end
+        Process.kill("TERM", pid)
+        _pid, result = Process.waitpid2(pid)
+        result.exitstatus.should == 3
+      end
     end
-    @pipe[0].gets.should == "started\n"
-    Process.kill("TERM", pid)
-    Process.wait
-    IO.select([@pipe[0]], nil, nil, 0).should_not be_nil
-    @pipe[0].gets.should == "finished\n"
   end
 
   describe "when splay is set" do
@@ -213,6 +263,9 @@ describe Chef::Application::Client, "run_application", :unix_only do
         # If everything is fine, sending USR1 to self should prevent
         # app to go into splay sleep forever.
         Process.kill("USR1", Process.pid)
+        # On Ruby < 2.1, we need to give the signal handlers a little
+        # more time, otherwise the test will fail because interleavings.
+        sleep 1
       end
 
       number_of_sleep_calls = 0
@@ -222,7 +275,7 @@ describe Chef::Application::Client, "run_application", :unix_only do
       # We have to do it this way because the main loop of
       # Chef::Application::Client swallows most exceptions, and we need to be
       # able to expose our expectation failures to the parent process in the test.
-      @app.stub(:sleep) do |arg|
+      @app.stub(:interval_sleep) do |arg|
         number_of_sleep_calls += 1
         if number_of_sleep_calls > 1
           exit 127