Merge pull request #5063 from ChaosCloud/master

service - upstart restart does not uses latest job config, need start/stop

2 files changed, 38 insertions, 15 deletions

diff --git a/lib/chef/provider/service/upstart.rb b/lib/chef/provider/service/upstart.rb
index 6e2a3b6473..9c0d97d376 100644
--- a/lib/chef/provider/service/upstart.rb
+++ b/lib/chef/provider/service/upstart.rb
@@ -26,6 +26,9 @@ class Chef
     class Service
       class Upstart < Chef::Provider::Service::Simple
 
+        # to maintain a local state of service across restart's internal calls
+        attr_accessor :upstart_service_running
+
         provides :service, platform_family: "debian", override: true do |node|
           Chef::Platform::ServiceHelpers.service_resource_providers.include?(:upstart)
         end
@@ -110,23 +113,23 @@ class Chef
 
             begin
               if shell_out!(@new_resource.status_command).exitstatus == 0
-                @current_resource.running true
+                @upstart_service_running = true
               end
             rescue
               @command_success = false
-              @current_resource.running false
+              @upstart_service_running = false
               nil
             end
           else
             begin
               if upstart_goal_state == "start"
-                @current_resource.running true
+                @upstart_service_running = true
               else
-                @current_resource.running false
+                @upstart_service_running = false
               end
             rescue Chef::Exceptions::Exec
               @command_success = false
-              @current_resource.running false
+              @upstart_service_running = false
               nil
             end
           end
@@ -153,13 +156,14 @@ class Chef
             @current_resource.enabled false
           end
 
+          @current_resource.running @upstart_service_running
           @current_resource
         end
 
         def start_service
           # Calling start on a service that is already started will return 1
           # Our 'goal' when we call start is to ensure the service is started
-          if @current_resource.running
+          if @upstart_service_running
             Chef::Log.debug("#{@new_resource} already running, not starting")
           else
             if @new_resource.start_command
@@ -168,12 +172,14 @@ class Chef
               shell_out_with_systems_locale!("/sbin/start #{@job}")
             end
           end
+
+          @upstart_service_running = true
         end
 
         def stop_service
           # Calling stop on a service that is already stopped will return 1
           # Our 'goal' when we call stop is to ensure the service is stopped
-          unless @current_resource.running
+          unless @upstart_service_running
             Chef::Log.debug("#{@new_resource} not running, not stopping")
           else
             if @new_resource.stop_command
@@ -182,6 +188,8 @@ class Chef
               shell_out_with_systems_locale!("/sbin/stop #{@job}")
             end
           end
+
+          @upstart_service_running = false
         end
 
         def restart_service
@@ -189,13 +197,19 @@ class Chef
             super
           # Upstart always provides restart functionality so we don't need to mimic it with stop/sleep/start.
           # Older versions of upstart would fail on restart if the service was currently stopped, check for that. LP:430883
+          # But for safe working of latest upstart job config being loaded, 'restart' can't be used as per link
+          # http://upstart.ubuntu.com/cookbook/#restart (it doesn't uses latest jon config from disk but retains old)
           else
-            if @current_resource.running
-              shell_out_with_systems_locale!("/sbin/restart #{@job}")
+            if @upstart_service_running
+              stop_service
+              sleep 1
+              start_service
             else
               start_service
             end
           end
+
+          @upstart_service_running = true
         end
 
         def reload_service
@@ -205,6 +219,8 @@ class Chef
             # upstart >= 0.6.3-4 supports reload (HUP)
             shell_out_with_systems_locale!("/sbin/reload #{@job}")
           end
+
+          @upstart_service_running = true
         end
 
         # https://bugs.launchpad.net/upstart/+bug/94065
diff --git a/spec/unit/provider/service/upstart_service_spec.rb b/spec/unit/provider/service/upstart_service_spec.rb
index fb5a418684..0245dd038c 100644
--- a/spec/unit/provider/service/upstart_service_spec.rb
+++ b/spec/unit/provider/service/upstart_service_spec.rb
@@ -278,18 +278,20 @@ describe Chef::Provider::Service::Upstart do
     end
 
     it "should call the start command if one is specified" do
+      @provider.upstart_service_running = false
       allow(@new_resource).to receive(:start_command).and_return("/sbin/rsyslog startyousillysally")
       expect(@provider).to receive(:shell_out_with_systems_locale!).with("/sbin/rsyslog startyousillysally")
       @provider.start_service()
     end
 
     it "should call '/sbin/start service_name' if no start command is specified" do
+      @provider.upstart_service_running = false
       expect(@provider).to receive(:shell_out_with_systems_locale!).with("/sbin/start #{@new_resource.service_name}").and_return(shell_out_success)
       @provider.start_service()
     end
 
     it "should not call '/sbin/start service_name' if it is already running" do
-      allow(@current_resource).to receive(:running).and_return(true)
+      @provider.upstart_service_running = true
       expect(@provider).not_to receive(:shell_out_with_systems_locale!)
       @provider.start_service()
     end
@@ -310,13 +312,16 @@ describe Chef::Provider::Service::Upstart do
       @provider.restart_service()
     end
 
-    it "should call '/sbin/restart service_name' if no restart command is specified" do
-      allow(@current_resource).to receive(:running).and_return(true)
-      expect(@provider).to receive(:shell_out_with_systems_locale!).with("/sbin/restart #{@new_resource.service_name}").and_return(shell_out_success)
+    it "should call start/sleep/stop if no restart command is specified" do
+      @provider.upstart_service_running = true
+      expect(@provider).to receive(:stop_service)
+      expect(@provider).to receive(:sleep).with(1)
+      expect(@provider).to receive(:start_service)
       @provider.restart_service()
     end
 
     it "should call '/sbin/start service_name' if restart_service is called for a stopped service" do
+      @provider.upstart_service_running = false
       allow(@current_resource).to receive(:running).and_return(false)
       expect(@provider).to receive(:shell_out_with_systems_locale!).with("/sbin/start #{@new_resource.service_name}").and_return(shell_out_success)
       @provider.restart_service()
@@ -336,22 +341,24 @@ describe Chef::Provider::Service::Upstart do
     end
 
     it "should call the stop command if one is specified" do
-      allow(@current_resource).to receive(:running).and_return(true)
+      @provider.upstart_service_running = true
       allow(@new_resource).to receive(:stop_command).and_return("/sbin/rsyslog stopyousillysally")
       expect(@provider).to receive(:shell_out_with_systems_locale!).with("/sbin/rsyslog stopyousillysally")
       @provider.stop_service()
     end
 
     it "should call '/sbin/stop service_name' if no stop command is specified" do
-      allow(@current_resource).to receive(:running).and_return(true)
+      @provider.upstart_service_running = true
       expect(@provider).to receive(:shell_out_with_systems_locale!).with("/sbin/stop #{@new_resource.service_name}").and_return(shell_out_success)
       @provider.stop_service()
     end
 
     it "should not call '/sbin/stop service_name' if it is already stopped" do
+      @provider.upstart_service_running = false
       allow(@current_resource).to receive(:running).and_return(false)
       expect(@provider).not_to receive(:shell_out_with_systems_locale!).with("/sbin/stop #{@new_resource.service_name}")
       @provider.stop_service()
+      expect(@upstart_service_running).to be_falsey
     end
   end
 end