Merge pull request #8884 from robuye/debian-update-rcd

update syntax of `update-rc.d` commands in enable & disable actions

2 files changed, 229 insertions, 52 deletions

diff --git a/lib/chef/provider/service/debian.rb b/lib/chef/provider/service/debian.rb
index 19e8823fb6..46ac38fa6c 100644
--- a/lib/chef/provider/service/debian.rb
+++ b/lib/chef/provider/service/debian.rb
@@ -149,44 +149,102 @@ class Chef
         end
 
         def enable_service
-          if new_resource.priority.is_a? Integer
-            shell_out!("/usr/sbin/update-rc.d -f #{new_resource.service_name} remove")
-            shell_out!("/usr/sbin/update-rc.d #{new_resource.service_name} defaults #{new_resource.priority} #{100 - new_resource.priority}")
-          elsif new_resource.priority.is_a? Hash
-            # we call the same command regardless of we're enabling or disabling
-            # users passing a Hash are responsible for setting their own start priorities
+          # We call the same command regardless if we're enabling or disabling
+          # Users passing a Hash are responsible for setting their own stop priorities
+          if new_resource.priority.is_a? Hash
             set_priority
-          else # No priority, go with update-rc.d defaults
-            shell_out!("/usr/sbin/update-rc.d -f #{new_resource.service_name} remove")
-            shell_out!("/usr/sbin/update-rc.d #{new_resource.service_name} defaults")
+            return
           end
+
+          start_priority = new_resource.priority.is_a?(Integer) ? new_resource.priority : 20
+          # Stop processes in reverse order of start using '100 - start_priority'.
+          stop_priority = 100 - start_priority
+
+          shell_out!("/usr/sbin/update-rc.d -f #{new_resource.service_name} remove")
+          shell_out!("/usr/sbin/update-rc.d #{new_resource.service_name} defaults #{start_priority} #{stop_priority}")
         end
 
         def disable_service
-          if new_resource.priority.is_a? Integer
-            # Stop processes in reverse order of start using '100 - start_priority'
-            shell_out!("/usr/sbin/update-rc.d -f #{new_resource.service_name} remove")
-            shell_out!("/usr/sbin/update-rc.d -f #{new_resource.service_name} stop #{100 - new_resource.priority} 2 3 4 5 .")
-          elsif new_resource.priority.is_a? Hash
-            # we call the same command regardless of we're enabling or disabling
-            # users passing a Hash are responsible for setting their own stop priorities
+          if new_resource.priority.is_a? Hash
+            # We call the same command regardless if we're enabling or disabling
+            # Users passing a Hash are responsible for setting their own stop priorities
             set_priority
+            return
+          end
+
+          shell_out!("/usr/sbin/update-rc.d -f #{new_resource.service_name} remove")
+
+          # Use legacy syntax if update-rc.d supports it for backward compatibility.
+          if use_legacy_update_rc_d?
+            # If no priority was given assume 20 (update-rc.d default).
+            start_priority = new_resource.priority.is_a?(Integer) ? new_resource.priority : 20
+            # Stop processes in reverse order of start using '100 - start_priority'.
+            stop_priority = 100 - start_priority
+
+            shell_out!("/usr/sbin/update-rc.d -f #{new_resource.service_name} stop #{stop_priority} 2 3 4 5 .")
           else
-            # no priority, using '100 - 20 (update-rc.d default)' to stop in reverse order of start
-            shell_out!("/usr/sbin/update-rc.d -f #{new_resource.service_name} remove")
-            shell_out!("/usr/sbin/update-rc.d -f #{new_resource.service_name} stop 80 2 3 4 5 .")
+            shell_out!("/usr/sbin/update-rc.d #{new_resource.service_name} defaults")
+            shell_out!("/usr/sbin/update-rc.d #{new_resource.service_name} disable")
           end
         end
 
         def set_priority
-          args = ""
-          new_resource.priority.each do |level, o|
-            action = o[0]
-            priority = o[1]
-            args += "#{action} #{priority} #{level} . "
-          end
           shell_out!("/usr/sbin/update-rc.d -f #{new_resource.service_name} remove")
-          shell_out!("/usr/sbin/update-rc.d #{new_resource.service_name} #{args}")
+
+          # Use legacy syntax if update-rc.d supports it for backward compatibility.
+          if use_legacy_update_rc_d?
+            args = ""
+            new_resource.priority.each do |level, o|
+              action = o[0]
+              priority = o[1]
+              args += "#{action} #{priority} #{level} . "
+            end
+            shell_out!("/usr/sbin/update-rc.d #{new_resource.service_name} #{args}")
+            return
+          end
+
+          # Use modern syntax, ignoring priorities as update-rc.d does not support it.
+          #
+          # Reset priorities to default values before applying customizations. This way
+          # the final state will always be consistent, regardless if all runlevels were
+          # provided.
+          shell_out!("/usr/sbin/update-rc.d #{new_resource.service_name} defaults")
+          new_resource.priority.each do |level, (action, _priority)|
+            disable_or_enable = (action == :start ? "enable" : "disable")
+
+            shell_out!("/usr/sbin/update-rc.d #{new_resource.service_name} #{disable_or_enable} #{level}")
+          end
+        end
+
+        # Ancient Debian releases used run levels and priorities to manage dependencies ordering.
+        # Old syntax no longer works and new syntax does not support priorities. If Chef detects
+        # ancient update-rc.d it will prefer legacy syntax so priorities can be set correctly in
+        # case the host is in fact running SysVinit.
+        #
+        # Additional context: https://lists.debian.org/debian-devel/2013/05/msg01109.html
+        def use_legacy_update_rc_d?
+          @sysv_rc_version ||= shell_out!("dpkg-query -W --showformat '${Version}' sysv-rc").stdout.strip
+
+          # sysv-rc is not installed therefore we're on modern Debian and legacy syntax does not work
+          if @sysv_rc_version.empty?
+            logger.trace("sysv-rc package is not installed. update-rc.d will use modern syntax")
+            return false
+          end
+
+          # sysv-rc is installed and update-rc.d is old enough to support legacy syntax and features
+          if @sysv_rc_version.to_f < 2.88
+            logger.trace("sysv-rc #{@sysv_rc_version} detected. update-rc.d will use legacy syntax")
+            return true
+          end
+
+          # sysv-rc 2.88dsf-42 drops the legacy syntax
+          if @sysv_rc_version.to_f == 2.88 && @sysv_rc_version[8..9].to_i < 42
+            logger.trace("sysv-rc #{@sysv_rc_version} detected. update-rc.d will use legacy syntax")
+            return true
+          end
+
+          # default to modern syntax
+          false
         end
       end
     end
diff --git a/spec/unit/provider/service/debian_service_spec.rb b/spec/unit/provider/service/debian_service_spec.rb
index 5f89605b2e..a69909107c 100644
--- a/spec/unit/provider/service/debian_service_spec.rb
+++ b/spec/unit/provider/service/debian_service_spec.rb
@@ -183,21 +183,21 @@ describe Chef::Provider::Service::Debian do
   describe "enable_service" do
     let(:service_name) { @new_resource.service_name }
     context "when the service doesn't set a priority" do
-      it "calls update-rc.d 'service_name' defaults" do
+      it "assumes default priority 20 and calls update-rc.d remove => defaults 20 80" do
         expect_commands(@provider, [
           "/usr/sbin/update-rc.d -f #{service_name} remove",
-          "/usr/sbin/update-rc.d #{service_name} defaults",
+          "/usr/sbin/update-rc.d #{service_name} defaults 20 80",
         ])
         @provider.enable_service
       end
     end
 
-    context "when the service sets a simple priority" do
+    context "when the service sets a simple priority 75" do
       before do
         @new_resource.priority(75)
       end
 
-      it "calls update-rc.d 'service_name' defaults" do
+      it "calls update-rc.d remove => defaults 75 25" do
         expect_commands(@provider, [
           "/usr/sbin/update-rc.d -f #{service_name} remove",
           "/usr/sbin/update-rc.d #{service_name} defaults 75 25",
@@ -206,45 +206,164 @@ describe Chef::Provider::Service::Debian do
       end
     end
 
-    context "when the service sets complex priorities" do
+    context "when the service sets complex priorities using Hash" do
       before do
         @new_resource.priority(2 => [:start, 20], 3 => [:stop, 55])
       end
 
-      it "calls update-rc.d 'service_name' with those priorities" do
-        expect_commands(@provider, [
-          "/usr/sbin/update-rc.d -f #{service_name} remove",
-          "/usr/sbin/update-rc.d #{service_name} start 20 2 . stop 55 3 . ",
-        ])
-        @provider.enable_service
+      context "when modern update-rc.d is detected" do
+        before do
+          expect(@provider).to receive(:use_legacy_update_rc_d?).and_return(false)
+        end
+
+        it "calls update-rc.d remove => defaults => enable|disable <runlevel>" do
+          expect_commands(@provider, [
+            "/usr/sbin/update-rc.d -f #{service_name} remove",
+            "/usr/sbin/update-rc.d #{service_name} defaults",
+            "/usr/sbin/update-rc.d #{service_name} enable 2",
+            "/usr/sbin/update-rc.d #{service_name} disable 3",
+          ])
+          @provider.enable_service
+        end
+      end
+
+      context "when legacy update-rc.d is detected" do
+        before do
+          expect(@provider).to receive(:use_legacy_update_rc_d?).and_return(true)
+        end
+
+        it "calls update-rc.d remove => start|stop <priority> <levels> ." do
+          expect_commands(@provider, [
+            "/usr/sbin/update-rc.d -f #{service_name} remove",
+            "/usr/sbin/update-rc.d #{service_name} start 20 2 . stop 55 3 . ",
+          ])
+          @provider.disable_service
+        end
       end
     end
   end
 
   describe "disable_service" do
     let(:service_name) { @new_resource.service_name }
-    context "when the service doesn't set a priority" do
-      it "calls update-rc.d -f 'service_name' remove + stop with default priority" do
-        expect_commands(@provider, [
-          "/usr/sbin/update-rc.d -f #{service_name} remove",
-          "/usr/sbin/update-rc.d -f #{service_name} stop 80 2 3 4 5 .",
-        ])
-        @provider.disable_service
+
+    context "when modern update-rc.d is detected" do
+      before do
+        expect(@provider).to receive(:use_legacy_update_rc_d?).and_return(false)
+      end
+
+      context "when the service doesn't set a priority" do
+        it "calls update-rc.d remove => defaults => disable" do
+          expect_commands(@provider, [
+            "/usr/sbin/update-rc.d -f #{service_name} remove",
+            "/usr/sbin/update-rc.d #{service_name} defaults",
+            "/usr/sbin/update-rc.d #{service_name} disable",
+          ])
+          @provider.disable_service
+        end
+      end
+
+      context "when the service sets a simple priority 75" do
+        before do
+          @new_resource.priority(75)
+        end
+
+        it "ignores priority and calls update-rc.d remove => defaults => disable" do
+          expect_commands(@provider, [
+            "/usr/sbin/update-rc.d -f #{service_name} remove",
+            "/usr/sbin/update-rc.d #{service_name} defaults",
+            "/usr/sbin/update-rc.d #{service_name} disable",
+          ])
+          @provider.disable_service
+        end
+      end
+
+      context "when the service sets complex priorities using Hash" do
+        before do
+          @new_resource.priority(2 => [:start, 20], 3 => [:stop, 55])
+        end
+
+        it "ignores priority and calls update-rc.d remove => defaults => enable|disable <runlevel>" do
+          expect_commands(@provider, [
+            "/usr/sbin/update-rc.d -f #{service_name} remove",
+            "/usr/sbin/update-rc.d #{service_name} defaults",
+            "/usr/sbin/update-rc.d #{service_name} enable 2",
+            "/usr/sbin/update-rc.d #{service_name} disable 3",
+          ])
+          @provider.disable_service
+        end
       end
     end
 
-    context "when the service sets a simple priority" do
+    context "when legacy update-rc.d is detected" do
       before do
-        @new_resource.priority(75)
+        expect(@provider).to receive(:use_legacy_update_rc_d?).and_return(true)
       end
 
-      it "calls update-rc.d -f 'service_name' remove + stop with the specified priority" do
-        expect_commands(@provider, [
-          "/usr/sbin/update-rc.d -f #{service_name} remove",
-          "/usr/sbin/update-rc.d -f #{service_name} stop #{100 - @new_resource.priority} 2 3 4 5 .",
-        ])
-        @provider.disable_service
+      context "when the service doesn't set a priority" do
+        it "assumes default priority 20 and calls update-rc.d remove => stop 80 2 3 4 5 ." do
+          expect_commands(@provider, [
+            "/usr/sbin/update-rc.d -f #{service_name} remove",
+            "/usr/sbin/update-rc.d -f #{service_name} stop 80 2 3 4 5 .",
+          ])
+          @provider.disable_service
+        end
+      end
+
+      context "when the service sets a simple priority 75" do
+        before do
+          @new_resource.priority(75)
+        end
+
+        it "reverses priority and calls update-rc.d remove => stop 25 2 3 4 5 ." do
+          expect_commands(@provider, [
+            "/usr/sbin/update-rc.d -f #{service_name} remove",
+            "/usr/sbin/update-rc.d -f #{service_name} stop #{100 - @new_resource.priority} 2 3 4 5 .",
+          ])
+          @provider.disable_service
+        end
       end
+
+      context "when the service sets complex priorities using Hash" do
+        before do
+          @new_resource.priority(2 => [:start, 20], 3 => [:stop, 55])
+        end
+
+        it "calls update-rc.d remove => start|stop <priority> <levels> ." do
+          expect_commands(@provider, [
+            "/usr/sbin/update-rc.d -f #{service_name} remove",
+            "/usr/sbin/update-rc.d #{service_name} start 20 2 . stop 55 3 . ",
+          ])
+          @provider.disable_service
+        end
+      end
+    end
+  end
+
+  describe "use_legacy_update_rc_d?" do
+    let(:dpkg_query_cmd) { "dpkg-query -W --showformat '${Version}' sysv-rc" }
+
+    it "is true when svsv-rc package version < 2.88 is installed" do
+      shellout_result = double(stdout: "2.86.ds1-34\n")
+      expect(@provider).to receive(:shell_out!).with(dpkg_query_cmd).and_return(shellout_result)
+      expect(@provider.use_legacy_update_rc_d?).to eq(true)
+    end
+
+    it "is true when sysv-rc is 2.88 and patch level < ('dsf-')42" do
+      shellout_result = double(stdout: "2.88.dsf-41\n")
+      expect(@provider).to receive(:shell_out!).with(dpkg_query_cmd).and_return(shellout_result)
+      expect(@provider.use_legacy_update_rc_d?).to eq(true)
+    end
+
+    it "is false when sysv-rc package version >= 2.88 is installed" do
+      shellout_result = double(stdout: "2.88dsf-59.9\n")
+      expect(@provider).to receive(:shell_out!).with(dpkg_query_cmd).and_return(shellout_result)
+      expect(@provider.use_legacy_update_rc_d?).to eq(false)
+    end
+
+    it "is false when sysv-rc package is not installed" do
+      shellout_result = double(stdout: "\n")
+      expect(@provider).to receive(:shell_out!).with(dpkg_query_cmd).and_return(shellout_result)
+      expect(@provider.use_legacy_update_rc_d?).to eq(false)
     end
   end
 end