Re-implement immediate reboot using throw/catch, so all the end-of-run

tasks (e.g. report handlers) still get to run.

4 files changed, 34 insertions, 13 deletions

diff --git a/lib/chef/client.rb b/lib/chef/client.rb
index dbd26b26c0..457a67a98a 100644
--- a/lib/chef/client.rb
+++ b/lib/chef/client.rb
@@ -428,7 +428,9 @@ class Chef
 
         run_context = setup_run_context
 
-        converge(run_context)
+        catch (:interrupt_run_and_reboot) do
+          converge(run_context)
+        end
 
         save_updated_node
 
diff --git a/lib/chef/platform/rebooter.rb b/lib/chef/platform/rebooter.rb
index 08224b4d77..b46f0e394c 100644
--- a/lib/chef/platform/rebooter.rb
+++ b/lib/chef/platform/rebooter.rb
@@ -31,6 +31,7 @@ class Chef
           reboot_info = node.run_context.reboot_info
 
           cmd = if Chef::Platform.windows?
+            # should this do /f as well? do we then need a minimum delay to let apps quit?
             "shutdown /r /t #{reboot_info[:delay_mins]} /c \"#{reboot_info[:reason]}\""
           else
             # probably Linux-only.
@@ -41,6 +42,7 @@ class Chef
           shell_out!(cmd)
         end
 
+        # this is a wrapper function so Chef::Client only needs a single line of code.
         def reboot_if_needed!(node)
           if node.run_context.reboot_requested?
             reboot!(node)
diff --git a/lib/chef/provider/reboot.rb b/lib/chef/provider/reboot.rb
index a8abcb3b57..f66b42c0e8 100644
--- a/lib/chef/provider/reboot.rb
+++ b/lib/chef/provider/reboot.rb
@@ -54,7 +54,7 @@ class Chef
         converge_by("rebooting the system immediately") do
           Chef::Log.warn "Rebooting system immediately, requested by '#{@new_resource.name}'"
           request_reboot
-          Chef::Platform::Rebooter.reboot!(node)
+          throw :interrupt_run_and_reboot
         end
       end
 
diff --git a/spec/functional/resource/reboot_spec.rb b/spec/functional/resource/reboot_spec.rb
index 78feae6839..8e1ec80e8e 100644
--- a/spec/functional/resource/reboot_spec.rb
+++ b/spec/functional/resource/reboot_spec.rb
@@ -42,9 +42,22 @@ describe Chef::Resource::Reboot do
     create_resource
   end
 
-  # the currently defined behavior for multiple calls to this resource is "last one wins."
+  shared_context 'testing run context modification' do
+    def test_reboot_action(resource)
+      reboot_info = resource.run_context.reboot_info
+      expect(reboot_info.keys.sort).to eq([:delay_mins, :reason, :requested_by, :timestamp])
+      expect(reboot_info[:delay_mins]).to eq(expected[:delay_mins])
+      expect(reboot_info[:reason]).to eq(expected[:reason])
+      expect(reboot_info[:requested_by]).to eq(expected[:requested_by])
 
+      expect(resource.run_context.reboot_requested?).to be_true
+    end
+  end
+
+  # the currently defined behavior for multiple calls to this resource is "last one wins."
   describe 'the request_reboot_on_successful_run action' do
+    include_context 'testing run context modification'
+
     before do
       resource.run_action(:request_reboot_on_successful_run)
     end
@@ -54,20 +67,24 @@ describe Chef::Resource::Reboot do
     end
 
     it 'should have modified the run context correctly' do
-      reboot_info = resource.run_context.reboot_info
-      expect(reboot_info.keys.sort).to eq([:delay_mins, :reason, :requested_by, :timestamp])
-      expect(reboot_info[:delay_mins]).to eq(expected[:delay_mins])
-      expect(reboot_info[:reason]).to eq(expected[:reason])
-      expect(reboot_info[:requested_by]).to eq(expected[:requested_by])
-
-      expect(resource.run_context.reboot_requested?).to be_true
+      test_reboot_action(resource)
     end
   end
 
   describe 'the reboot_interrupt_run action' do
-    it 'should have attempted to reboot the server' do
-      expect(Chef::Platform::Rebooter).to receive(:reboot!).once
-      resource.run_action(:reboot_interrupt_run)
+    include_context 'testing run context modification'
+
+    after do
+      resource.run_context.cancel_reboot
+    end
+
+    it 'should have modified the run context correctly' do
+      # this doesn't actually test the flow of Chef::Client#do_run, unfortunately.
+      expect {
+        resource.run_action(:reboot_interrupt_run)
+      }.to throw_symbol(:interrupt_run_and_reboot)
+
+      test_reboot_action(resource)
     end
   end