load_current_resource for systemd service more efficiently

Signed-off-by: Joshua Miller <joshmiller@fb.com>

2 files changed, 150 insertions, 33 deletions

diff --git a/lib/chef/provider/service/systemd.rb b/lib/chef/provider/service/systemd.rb
index 11c8d285bc..9b0cf3abd8 100644
--- a/lib/chef/provider/service/systemd.rb
+++ b/lib/chef/provider/service/systemd.rb
@@ -76,6 +76,30 @@ class Chef::Provider::Service::Systemd < Chef::Provider::Service::Simple
     end
   end
 
+  def systemd_service_status
+    @systemd_service_status ||= begin
+      # Collect all the status information for a service and returns it at once
+      options, args = get_systemctl_options_args
+      s = shell_out!(systemctl_path, args, "show", "-p", "UnitFileState", "-p", "ActiveState", new_resource.service_name, options)
+      # e.g. /bin/systemctl --system show  -p UnitFileState -p ActiveState sshd.service
+      # Returns something like:
+      # ActiveState=active
+      # UnitFileState=enabled
+      status = {}
+      s.stdout.each_line do |line|
+        k, v = line.strip.split("=")
+        status[k] = v
+      end
+
+      # Assert requisite keys exist
+      unless status.key?("UnitFileState") && status.key?("ActiveState")
+        raise Chef::Exceptions::Service, "'#{systemctl_path} show' not reporting status for #{new_resource.service_name}!"
+      end
+
+      status
+    end
+  end
+
   def get_systemctl_options_args
     if new_resource.user
       raise NotImplementedError, "#{new_resource} does not support the user property on a target_mode host (yet)" if Chef::Config.target_mode?
@@ -173,25 +197,25 @@ class Chef::Provider::Service::Systemd < Chef::Provider::Service::Simple
   end
 
   def is_active?
-    options, args = get_systemctl_options_args
-    shell_out(systemctl_path, args, "is-active", new_resource.service_name, "--quiet", **options).exitstatus == 0
+    # Note: "activating" is not active (as with type=notify or a oneshot)
+    systemd_service_status["ActiveState"] == "active"
   end
 
   def is_enabled?
-    options, args = get_systemctl_options_args
-    shell_out(systemctl_path, args, "is-enabled", new_resource.service_name, "--quiet", **options).exitstatus == 0
+    # See https://github.com/systemd/systemd/blob/master/src/systemctl/systemctl-is-enabled.c
+    # Note: enabled-runtime is excluded because this is volatile, and the state of enabled-runtime
+    # specifically means that the service is not enabled
+    %w{enabled static generated alias indirect}.include?(systemd_service_status["UnitFileState"])
   end
 
   def is_indirect?
-    options, args = get_systemctl_options_args
-    s = shell_out(systemctl_path, args, "is-enabled", new_resource.service_name, **options)
-    s.stdout.include?("indirect")
+    systemd_service_status["UnitFileState"] == "indirect"
   end
 
   def is_masked?
-    options, args = get_systemctl_options_args
-    s = shell_out(systemctl_path, args, "is-enabled", new_resource.service_name, **options)
-    s.exitstatus != 0 && s.stdout.include?("masked")
+    # Note: masked-runtime is excluded, because runtime is volatile, and
+    # because masked-runtime is not masked.
+    systemd_service_status["UnitFileState"] == "masked"
   end
 
   private
diff --git a/spec/unit/provider/service/systemd_service_spec.rb b/spec/unit/provider/service/systemd_service_spec.rb
index 10f59ed025..34b8094974 100644
--- a/spec/unit/provider/service/systemd_service_spec.rb
+++ b/spec/unit/provider/service/systemd_service_spec.rb
@@ -297,6 +297,55 @@ describe Chef::Provider::Service::Systemd do
         end
       end
 
+      enabled_and_active = <<-STDOUT
+        ActiveState=active
+        UnitFileState=enabled
+      STDOUT
+      disabled_and_inactive = <<-STDOUT
+        ActiveState=disabled
+        UnitFileState=inactive
+      STDOUT
+      # No unit known for this service, and inactive
+      nil_and_inactive = <<-STDOUT
+        ActiveState=inactive
+        UnitFileState=
+      STDOUT
+
+      def with_systemctl_show(systemctl_path, stdout)
+        systemctl_show = [systemctl_path, "--system", "show", "-p", "UnitFileState", "-p", "ActiveState", service_name]
+        expect(provider).to receive(:shell_out!).with(*systemctl_show, {}).and_return(double(stdout: stdout, exitstatus: 0, error?: false))
+      end
+
+      describe "systemd_service_status" do
+        before(:each) do
+          provider.current_resource = current_resource
+          current_resource.service_name(service_name)
+        end
+
+        it "should return status if '#{systemctl_path} --system show -p UnitFileState -p ActiveState service_name' returns 0 and has nil" do
+          nil_and_inactive_h = {
+            "ActiveState" => "inactive",
+            "UnitFileState" => nil,
+          }
+          with_systemctl_show(systemctl_path, nil_and_inactive)
+          expect(provider.systemd_service_status).to eql(nil_and_inactive_h)
+        end
+
+        it "should error if '#{systemctl_path} --system show -p UnitFileState -p ActiveState service_name' misses fields" do
+          partial_systemctl_stdout = <<-STDOUT
+            ActiveState=inactive
+          STDOUT
+          with_systemctl_show(systemctl_path, partial_systemctl_stdout)
+          expect { provider.systemd_service_status }.to raise_error(Chef::Exceptions::Service)
+        end
+
+        it "should error if '#{systemctl_path} --system show -p UnitFileState -p ActiveState service_name' returns non 0" do
+          systemctl_show = [systemctl_path, "--system", "show", "-p", "UnitFileState", "-p", "ActiveState", service_name]
+          allow(provider).to receive(:shell_out!).with(*systemctl_show, {}).and_return(shell_out_failure)
+          expect { provider.systemd_service_status }.to raise_error(Chef::Exceptions::Service)
+        end
+      end
+
       describe "is_active?" do
         before(:each) do
           provider.current_resource = current_resource
@@ -304,13 +353,22 @@ describe Chef::Provider::Service::Systemd do
           allow(provider).to receive(:which).with("systemctl").and_return(systemctl_path.to_s)
         end
 
-        it "should return true if '#{systemctl_path} --system is-active service_name' returns 0" do
-          expect(provider).to receive(:shell_out_compacted).with(systemctl_path, "--system", "is-active", service_name, "--quiet", timeout: 900).and_return(shell_out_success)
+        it "should return true if service is active" do
+          with_systemctl_show(systemctl_path, enabled_and_active)
           expect(provider.is_active?).to be true
         end
 
-        it "should return false if '#{systemctl_path} --system is-active service_name' returns anything except 0" do
-          expect(provider).to receive(:shell_out_compacted).with(systemctl_path, "--system", "is-active", service_name, "--quiet", timeout: 900).and_return(shell_out_failure)
+        it "should return false if service is not active" do
+          with_systemctl_show(systemctl_path, disabled_and_inactive)
+          expect(provider.is_active?).to be false
+        end
+
+        it "should return false if service is activating" do
+          enabled_and_activating = <<-STDOUT
+            ActiveState=activating
+            UnitFileState=enabled
+          STDOUT
+          with_systemctl_show(systemctl_path, enabled_and_activating)
           expect(provider.is_active?).to be false
         end
       end
@@ -322,13 +380,36 @@ describe Chef::Provider::Service::Systemd do
           allow(provider).to receive(:which).with("systemctl").and_return(systemctl_path.to_s)
         end
 
-        it "should return true if '#{systemctl_path} --system is-enabled service_name' returns 0" do
-          expect(provider).to receive(:shell_out_compacted).with(systemctl_path, "--system", "is-enabled", service_name, "--quiet", timeout: 900).and_return(shell_out_success)
+        it "should return true if service is enabled" do
+          with_systemctl_show(systemctl_path, enabled_and_active)
+          expect(provider.is_enabled?).to be true
+        end
+
+        it "should return false if service is disabled" do
+          with_systemctl_show(systemctl_path, disabled_and_inactive)
+          expect(provider.is_enabled?).to be false
+        end
+
+        it "should return false if service has no unit file" do
+          with_systemctl_show(systemctl_path, nil_and_inactive)
+          expect(provider.is_enabled?).to be false
+        end
+
+        it "should return true if service is static" do
+          static_and_active = <<-STDOUT
+            ActiveState=active
+            UnitFileState=static
+          STDOUT
+          with_systemctl_show(systemctl_path, static_and_active)
           expect(provider.is_enabled?).to be true
         end
 
-        it "should return false if '#{systemctl_path} --system is-enabled service_name' returns anything except 0" do
-          expect(provider).to receive(:shell_out_compacted).with(systemctl_path, "--system", "is-enabled", service_name, "--quiet", timeout: 900).and_return(shell_out_failure)
+        it "should return false if service is enabled-runtime" do
+          enabled_runtime_and_active = <<-STDOUT
+            ActiveState=active
+            UnitFileState=enabled-runtime
+          STDOUT
+          with_systemctl_show(systemctl_path, enabled_runtime_and_active)
           expect(provider.is_enabled?).to be false
         end
       end
@@ -340,23 +421,31 @@ describe Chef::Provider::Service::Systemd do
           allow(provider).to receive(:which).with("systemctl").and_return(systemctl_path.to_s)
         end
 
-        it "should return true if '#{systemctl_path} --system is-enabled service_name' returns 'masked' and returns anything except 0" do
-          expect(provider).to receive(:shell_out_compacted).with(systemctl_path, "--system", "is-enabled", service_name, timeout: 900).and_return(double(stdout: "masked", exitstatus: shell_out_failure))
+        it "should return true if service is masked" do
+          masked_and_inactive = <<-STDOUT
+            ActiveState=inactive
+            UnitFileState=masked
+          STDOUT
+          with_systemctl_show(systemctl_path, masked_and_inactive)
           expect(provider.is_masked?).to be true
         end
 
-        it "should return true if '#{systemctl_path} --system is-enabled service_name' outputs 'masked-runtime' and returns anything except 0" do
-          expect(provider).to receive(:shell_out_compacted).with(systemctl_path, "--system", "is-enabled", service_name, timeout: 900).and_return(double(stdout: "masked-runtime", exitstatus: shell_out_failure))
-          expect(provider.is_masked?).to be true
+        it "should return false if service is masked-runtime" do
+          masked_runtime_and_inactive = <<-STDOUT
+            ActiveState=inactive
+            UnitFileState=masked-runtime
+          STDOUT
+          with_systemctl_show(systemctl_path, masked_runtime_and_inactive)
+          expect(provider.is_masked?).to be false
         end
 
-        it "should return false if '#{systemctl_path} --system is-enabled service_name' returns 0" do
-          expect(provider).to receive(:shell_out_compacted).with(systemctl_path, "--system", "is-enabled", service_name, timeout: 900).and_return(double(stdout: "enabled", exitstatus: shell_out_success))
+        it "should return false if service is enabled" do
+          with_systemctl_show(systemctl_path, enabled_and_active)
           expect(provider.is_masked?).to be false
         end
 
-        it "should return false if '#{systemctl_path} --system is-enabled service_name' returns anything except 0 and outputs an error'" do
-          expect(provider).to receive(:shell_out_compacted).with(systemctl_path, "--system", "is-enabled", service_name, timeout: 900).and_return(double(stdout: "Failed to get unit file state for #{service_name}: No such file or directory", exitstatus: shell_out_failure))
+        it "should return false if service has no known unit file" do
+          with_systemctl_show(systemctl_path, nil_and_inactive)
           expect(provider.is_masked?).to be false
         end
       end
@@ -368,18 +457,22 @@ describe Chef::Provider::Service::Systemd do
           allow(provider).to receive(:which).with("systemctl").and_return(systemctl_path.to_s)
         end
 
-        it "should return true if '#{systemctl_path} --system is-enabled service_name' returns 'indirect'" do
-          expect(provider).to receive(:shell_out_compacted).with(systemctl_path, "--system", "is-enabled", service_name, timeout: 900).and_return(double(stdout: "indirect", exitstatus: shell_out_success))
+        it "should return true if service is indirect" do
+          indirect_and_inactive = <<-STDOUT
+            ActiveState=inactive
+            UnitFileState=indirect
+          STDOUT
+          with_systemctl_show(systemctl_path, indirect_and_inactive)
           expect(provider.is_indirect?).to be true
         end
 
-        it "should return false if '#{systemctl_path} --system is-enabled service_name' returns 0 and outputs something other than 'indirect'" do
-          expect(provider).to receive(:shell_out_compacted).with(systemctl_path, "--system", "is-enabled", service_name, timeout: 900).and_return(double(stdout: "enabled", exitstatus: shell_out_success))
+        it "should return false if service not indirect" do
+          with_systemctl_show(systemctl_path, enabled_and_active)
           expect(provider.is_indirect?).to be false
         end
 
-        it "should return false if '#{systemctl_path} --system is-enabled service_name' returns anything except 0 and outputs somethign other than 'indirect''" do
-          expect(provider).to receive(:shell_out_compacted).with(systemctl_path, "--system", "is-enabled", service_name, timeout: 900).and_return(double(stdout: "enabled", exitstatus: shell_out_failure))
+        it "should return false if service has no known unit file" do
+          with_systemctl_show(systemctl_path, nil_and_inactive)
           expect(provider.is_indirect?).to be false
         end
       end