(PUP-1353) Start Posix based services with a 0 priority

Puppet's base service provider starts services as a child process of the
Puppet process. On Posix systems the child process inherits the priority
or nice value of the parent process. The result is that services which
are started as child processes of Puppet have their priority set to
Puppet's priority. This behavior is undesirable as services expect to
be started with a 0 or normal priority. This commit ensures services are
started with a normal priority by changing the priority of the child
process when starting a service.

Example of old & new behavior on Debian Wheezy:

  ; git checkout master
  Switched to branch 'master'
  Your branch is up-to-date with 'origin/master'.

  ; sudo /etc/init.d/ntp stop
  Stopping NTP server: ntpd.

  ; nice -n 13 sudo bundle exec puppet apply -e 'service { "ntp": ensure => "running" }'
  Notice: Compiled catalog for foo in environment production in 0.12 seconds
  Notice: /Stage[main]/Main/Service[ntp]/ensure: ensure changed 'stopped' to 'running'
  Notice: Applied catalog in 0.05 seconds

  ; ps -C ntpd -o pid,tid,class,rtprio,ni,pri,psr,pcpu,stat,wchan:14,comm
    PID   TID CLS RTPRIO  NI PRI PSR %CPU STAT WCHAN          COMMAND
  30269 30269 TS       -  13   6   6  0.0 SNs  -              ntpd

  ; git checkout service_priority
  Switched to branch 'service_priority'
  Your branch is up-to-date with 'origin/service_priority'.

  ; sudo /etc/init.d/ntp stop
  Stopping NTP server: ntpd.

  ; nice -n 13 sudo bundle exec puppet apply -e 'service { "ntp": ensure => "running" }'
  Notice: Compiled catalog for foo in environment production in 0.11 seconds
  Notice: /Stage[main]/Main/Service[ntp]/ensure: ensure changed 'stopped' to 'running'
  Notice: Applied catalog in 0.04 seconds

  ; ps -C ntpd -o pid,tid,class,rtprio,ni,pri,psr,pcpu,stat,wchan:14,comm
    PID   TID CLS RTPRIO  NI PRI PSR %CPU STAT WCHAN          COMMAND
  30536 30536 TS       -   0  19  10  0.0 Ss   -              ntpd

Author: lollipopman

lib/puppet/provider/service/service.rb

@@ -21,8 +21,23 @@ def restartcmd
 
   # A simple wrapper so execution failures are a bit more informative.
   def texecute(type, command, fof = true, squelch = false, combine = true)
+    # Set the process priority to 0 (or normal in Windows) so that services
+    # which are started as children of puppet will start with normal priority,
+    # rather than the priority of the puppet process itself.
+    if Facter.value(:osfamily) == 'windows'
+      priority = Process::NORMAL_PRIORITY_CLASS
+    else
+      priority = 0
+    end
     begin
-      execute(command, :failonfail => fof, :override_locale => false, :squelch => squelch, :combine => combine)
+      opts = {
+        :combine => combine,
+        :failonfail => fof,
+        :override_locale => false,
+        :priority => priority,
+        :squelch => squelch,
+      }
+      execute(command, opts)
     rescue Puppet::ExecutionFailure => detail
       @resource.fail Puppet::Error, "Could not #{type} #{@resource.ref}: #{detail}", detail
     end

lib/puppet/util/execution.rb

@@ -1,5 +1,6 @@
 require 'timeout'
 require 'puppet/file_system/uniquefile'
+require 'puppet/util/limits'
 
 module Puppet
   require 'rbconfig'
@@ -133,6 +134,7 @@ def self.execfail(command, exception)
   #   Passing in a value of false for this option will allow the command to be executed using the user/system locale.
   # @option options [Hash<{String => String}>] :custom_environment ({}) a hash of key/value pairs to set as environment variables for the duration
   #   of the command.
+  # @option options [Integer] :priority (nil) The priority of child process, aka the nice value in Posix operatingsystems.
   # @return [Puppet::Util::Execution::ProcessOutput] output as specified by options
   # @raise [Puppet::ExecutionFailure] if the executed chiled process did not exit with status == 0 and `failonfail` is
   #   `true`.
@@ -158,6 +160,7 @@ def self.execute(command, options = NoOptionsSpecified)
         :custom_environment => {},
         :sensitive => false,
         :suppress_window => false,
+        :priority => nil,
     }
 
     options = default_options.merge(options)
@@ -336,6 +339,11 @@ def self.execute_posix(command, options, stdin, stdout, stderr)
       # turning "/tmp/foo;\r\n /bin/echo" into ["/tmp/foo;\r\n", " /bin/echo"]
       command = [command].flatten
       Process.setsid
+      if options[:priority]
+        if Puppet.features.root?
+          setpriority(options[:priority])
+        end
+      end
       begin
         # We need to chdir to our cwd before changing privileges as there's a
         # chance that the user may not have permissions to access the cwd, which

spec/unit/provider/service/bsd_spec.rb

@@ -94,13 +94,13 @@
 
     it "should use the supplied start command if specified" do
       provider = provider_class.new(Puppet::Type.type(:service).new(:name => 'sshd', :start => '/bin/foo'))
-      expect(provider).to receive(:execute).with(['/bin/foo'], :failonfail => true, :override_locale => false, :squelch => false, :combine => true)
+      expect(provider).to receive(:execute).with(['/bin/foo'], :combine => true, :failonfail => true, :override_locale => false, :priority => 0, :squelch => false)
       provider.start
     end
 
     it "should start the serviced directly otherwise" do
       provider = provider_class.new(Puppet::Type.type(:service).new(:name => 'sshd'))
-      expect(provider).to receive(:execute).with(['/etc/rc.d/sshd', :onestart], :failonfail => true, :override_locale => false, :squelch => false, :combine => true)
+      expect(provider).to receive(:execute).with(['/etc/rc.d/sshd', :onestart], :combine => true, :failonfail => true, :override_locale => false, :priority => 0, :squelch => false)
       expect(provider).to receive(:search).with('sshd').and_return('/etc/rc.d/sshd')
       provider.start
     end
@@ -113,13 +113,13 @@
 
     it "should use the supplied stop command if specified" do
       provider = provider_class.new(Puppet::Type.type(:service).new(:name => 'sshd', :stop => '/bin/foo'))
-      expect(provider).to receive(:execute).with(['/bin/foo'], :failonfail => true, :override_locale => false, :squelch => false, :combine => true)
+      expect(provider).to receive(:execute).with(['/bin/foo'], :combine => true, :failonfail => true, :override_locale => false, :priority => 0, :squelch => false)
       provider.stop
     end
 
     it "should stop the serviced directly otherwise" do
       provider = provider_class.new(Puppet::Type.type(:service).new(:name => 'sshd'))
-      expect(provider).to receive(:execute).with(['/etc/rc.d/sshd', :onestop], :failonfail => true, :override_locale => false, :squelch => false, :combine => true)
+      expect(provider).to receive(:execute).with(['/etc/rc.d/sshd', :onestop], :combine => true, :failonfail => true, :override_locale => false, :priority => 0, :squelch => false)
       expect(provider).to receive(:search).with('sshd').and_return('/etc/rc.d/sshd')
       provider.stop
     end

spec/unit/provider/service/gentoo_spec.rb

@@ -79,13 +79,13 @@
   describe "#start" do
     it "should use the supplied start command if specified" do
       provider = provider_class.new(Puppet::Type.type(:service).new(:name => 'sshd', :start => '/bin/foo'))
-      expect(provider).to receive(:execute).with(['/bin/foo'], :failonfail => true, :override_locale => false, :squelch => false, :combine => true)
+      expect(provider).to receive(:execute).with(['/bin/foo'], :combine => true, :failonfail => true, :override_locale => false, :priority => 0, :squelch => false)
       provider.start
     end
 
     it "should start the service with <initscript> start otherwise" do
       provider = provider_class.new(Puppet::Type.type(:service).new(:name => 'sshd'))
-      expect(provider).to receive(:execute).with(['/etc/init.d/sshd',:start], :failonfail => true, :override_locale => false, :squelch => false, :combine => true)
+      expect(provider).to receive(:execute).with(['/etc/init.d/sshd',:start], :combine => true, :failonfail => true, :override_locale => false, :priority => 0, :squelch => false)
       expect(provider).to receive(:search).with('sshd').and_return('/etc/init.d/sshd')
       provider.start
     end
@@ -94,13 +94,13 @@
   describe "#stop" do
     it "should use the supplied stop command if specified" do
       provider = provider_class.new(Puppet::Type.type(:service).new(:name => 'sshd', :stop => '/bin/foo'))
-      expect(provider).to receive(:execute).with(['/bin/foo'], :failonfail => true, :override_locale => false, :squelch => false, :combine => true)
+      expect(provider).to receive(:execute).with(['/bin/foo'], :combine => true, :failonfail => true, :override_locale => false, :priority => 0, :squelch => false)
       provider.stop
     end
 
     it "should stop the service with <initscript> stop otherwise" do
       provider = provider_class.new(Puppet::Type.type(:service).new(:name => 'sshd'))
-      expect(provider).to receive(:execute).with(['/etc/init.d/sshd',:stop], :failonfail => true, :override_locale => false, :squelch => false, :combine => true)
+      expect(provider).to receive(:execute).with(['/etc/init.d/sshd',:stop], :combine => true, :failonfail => true, :override_locale => false, :priority => 0, :squelch => false)
       expect(provider).to receive(:search).with('sshd').and_return('/etc/init.d/sshd')
       provider.stop
     end
@@ -160,24 +160,24 @@
     describe "when a special status command is specified" do
       it "should use the status command from the resource" do
         provider = provider_class.new(Puppet::Type.type(:service).new(:name => 'sshd', :status => '/bin/foo'))
-        expect(provider).not_to receive(:execute).with(['/etc/init.d/sshd',:status], :failonfail => false, :override_locale => false, :squelch => false, :combine => true)
-        expect(provider).to receive(:execute).with(['/bin/foo'], :failonfail => false, :override_locale => false, :squelch => false, :combine => true)
+        expect(provider).not_to receive(:execute).with(['/etc/init.d/sshd',:status], :combine => true, :failonfail => false, :override_locale => false, :priority => 0, :squelch => false)
+        expect(provider).to receive(:execute).with(['/bin/foo'], :combine => true, :failonfail => false, :override_locale => false, :priority => 0, :squelch => false)
         allow($CHILD_STATUS).to receive(:exitstatus).and_return(0)
         provider.status
       end
 
       it "should return :stopped when the status command returns with a non-zero exitcode" do
         provider = provider_class.new(Puppet::Type.type(:service).new(:name => 'sshd', :status => '/bin/foo'))
-        expect(provider).not_to receive(:execute).with(['/etc/init.d/sshd',:status], :failonfail => false, :override_locale => false, :squelch => false, :combine => true)
-        expect(provider).to receive(:execute).with(['/bin/foo'], :failonfail => false, :override_locale => false, :squelch => false, :combine => true)
+        expect(provider).not_to receive(:execute).with(['/etc/init.d/sshd',:status], :combine => true, :failonfail => false, :override_locale => false, :priority => 0, :squelch => false)
+        expect(provider).to receive(:execute).with(['/bin/foo'], :combine => true, :failonfail => false, :override_locale => false, :priority => 0, :squelch => false)
         allow($CHILD_STATUS).to receive(:exitstatus).and_return(3)
         expect(provider.status).to eq(:stopped)
       end
 
       it "should return :running when the status command returns with a zero exitcode" do
         provider = provider_class.new(Puppet::Type.type(:service).new(:name => 'sshd', :status => '/bin/foo'))
-        expect(provider).not_to receive(:execute).with(['/etc/init.d/sshd',:status], :failonfail => false, :override_locale => false, :squelch => false, :combine => true)
-        expect(provider).to receive(:execute).with(['/bin/foo'], :failonfail => false, :override_locale => false, :squelch => false, :combine => true)
+        expect(provider).not_to receive(:execute).with(['/etc/init.d/sshd',:status], :combine => true, :failonfail => false, :override_locale => false, :priority => 0, :squelch => false)
+        expect(provider).to receive(:execute).with(['/bin/foo'], :combine => true, :failonfail => false, :override_locale => false, :priority => 0, :squelch => false)
         allow($CHILD_STATUS).to receive(:exitstatus).and_return(0)
         expect(provider.status).to eq(:running)
       end
@@ -186,14 +186,14 @@
     describe "when hasstatus is false" do
       it "should return running if a pid can be found" do
         provider = provider_class.new(Puppet::Type.type(:service).new(:name => 'sshd', :hasstatus => false))
-        expect(provider).not_to receive(:execute).with(['/etc/init.d/sshd',:status], :failonfail => false, :override_locale => false, :squelch => false, :combine => true)
+        expect(provider).not_to receive(:execute).with(['/etc/init.d/sshd',:status], :combine => true, :failonfail => false, :override_locale => false, :priority => 0, :squelch => false)
         expect(provider).to receive(:getpid).and_return(1000)
         expect(provider.status).to eq(:running)
       end
 
       it "should return stopped if no pid can be found" do
         provider = provider_class.new(Puppet::Type.type(:service).new(:name => 'sshd', :hasstatus => false))
-        expect(provider).not_to receive(:execute).with(['/etc/init.d/sshd',:status], :failonfail => false, :override_locale => false, :squelch => false, :combine => true)
+        expect(provider).not_to receive(:execute).with(['/etc/init.d/sshd',:status], :combine => true, :failonfail => false, :override_locale => false, :priority => 0, :squelch => false)
         expect(provider).to receive(:getpid).and_return(nil)
         expect(provider.status).to eq(:stopped)
       end
@@ -203,15 +203,15 @@
       it "should return running if <initscript> status exits with a zero exitcode" do
         provider = provider_class.new(Puppet::Type.type(:service).new(:name => 'sshd', :hasstatus => true))
         expect(provider).to receive(:search).with('sshd').and_return('/etc/init.d/sshd')
-        expect(provider).to receive(:execute).with(['/etc/init.d/sshd',:status], :failonfail => false, :override_locale => false, :squelch => false, :combine => true)
+        expect(provider).to receive(:execute).with(['/etc/init.d/sshd',:status], :combine => true, :failonfail => false, :override_locale => false, :priority => 0, :squelch => false)
         allow($CHILD_STATUS).to receive(:exitstatus).and_return(0)
         expect(provider.status).to eq(:running)
       end
 
       it "should return stopped if <initscript> status exits with a non-zero exitcode" do
         provider = provider_class.new(Puppet::Type.type(:service).new(:name => 'sshd', :hasstatus => true))
         expect(provider).to receive(:search).with('sshd').and_return('/etc/init.d/sshd')
-        expect(provider).to receive(:execute).with(['/etc/init.d/sshd',:status], :failonfail => false, :override_locale => false, :squelch => false, :combine => true)
+        expect(provider).to receive(:execute).with(['/etc/init.d/sshd',:status], :combine => true, :failonfail => false, :override_locale => false, :priority => 0, :squelch => false)
         allow($CHILD_STATUS).to receive(:exitstatus).and_return(3)
         expect(provider.status).to eq(:stopped)
       end
@@ -221,24 +221,24 @@
   describe "#restart" do
     it "should use the supplied restart command if specified" do
       provider = provider_class.new(Puppet::Type.type(:service).new(:name => 'sshd', :restart => '/bin/foo'))
-      expect(provider).not_to receive(:execute).with(['/etc/init.d/sshd',:restart], :failonfail => true, :override_locale => false, :squelch => false, :combine => true)
-      expect(provider).to receive(:execute).with(['/bin/foo'], :failonfail => true, :override_locale => false, :squelch => false, :combine => true)
+      expect(provider).not_to receive(:execute).with(['/etc/init.d/sshd',:restart], :combine => true, :failonfail => true, :override_locale => false, :priority => 0, :squelch => false)
+      expect(provider).to receive(:execute).with(['/bin/foo'], :combine => true, :failonfail => true, :override_locale => false, :priority => 0, :squelch => false)
       provider.restart
     end
 
     it "should restart the service with <initscript> restart if hasrestart is true" do
       provider = provider_class.new(Puppet::Type.type(:service).new(:name => 'sshd', :hasrestart => true))
       expect(provider).to receive(:search).with('sshd').and_return('/etc/init.d/sshd')
-      expect(provider).to receive(:execute).with(['/etc/init.d/sshd',:restart], :failonfail => true, :override_locale => false, :squelch => false, :combine => true)
+      expect(provider).to receive(:execute).with(['/etc/init.d/sshd',:restart], :combine => true, :failonfail => true, :override_locale => false, :priority => 0, :squelch => false)
       provider.restart
     end
 
     it "should restart the service with <initscript> stop/start if hasrestart is false" do
       provider = provider_class.new(Puppet::Type.type(:service).new(:name => 'sshd', :hasrestart => false))
       expect(provider).to receive(:search).with('sshd').and_return('/etc/init.d/sshd')
-      expect(provider).not_to receive(:execute).with(['/etc/init.d/sshd',:restart], :failonfail => true, :override_locale => false, :squelch => false, :combine => true)
-      expect(provider).to receive(:execute).with(['/etc/init.d/sshd',:stop], :failonfail => true, :override_locale => false, :squelch => false, :combine => true)
-      expect(provider).to receive(:execute).with(['/etc/init.d/sshd',:start], :failonfail => true, :override_locale => false, :squelch => false, :combine => true)
+      expect(provider).not_to receive(:execute).with(['/etc/init.d/sshd',:restart], :combine => true, :failonfail => true, :override_locale => false, :priority => 0, :squelch => false)
+      expect(provider).to receive(:execute).with(['/etc/init.d/sshd',:stop], :combine => true, :failonfail => true, :override_locale => false, :priority => 0, :squelch => false)
+      expect(provider).to receive(:execute).with(['/etc/init.d/sshd',:start], :combine => true, :failonfail => true, :override_locale => false, :priority => 0, :squelch => false)
       provider.restart
     end
   end

spec/unit/provider/service/init_spec.rb

@@ -242,7 +242,7 @@
     describe "when starting a service on Solaris" do
       it "should use ctrun" do
         allow(Facter).to receive(:value).with(:osfamily).and_return('Solaris')
-        expect(provider).to receive(:execute).with('/usr/bin/ctrun -l child /service/path/myservice start', {:failonfail => true, :override_locale => false, :squelch => false, :combine => true}).and_return("")
+        expect(provider).to receive(:execute).with('/usr/bin/ctrun -l child /service/path/myservice start', {:combine => true, :failonfail => true, :override_locale => false, :priority => 0, :squelch => false}).and_return("")
         allow($CHILD_STATUS).to receive(:exitstatus).and_return(0)
         provider.start
       end
@@ -251,7 +251,7 @@
     describe "when starting a service on RedHat" do
       it "should not use ctrun" do
         allow(Facter).to receive(:value).with(:osfamily).and_return('RedHat')
-        expect(provider).to receive(:execute).with(['/service/path/myservice', :start], {:failonfail => true, :override_locale => false, :squelch => false, :combine => true}).and_return("")
+        expect(provider).to receive(:execute).with(['/service/path/myservice', :start], {:combine => true, :failonfail => true, :override_locale => false, :priority => 0, :squelch => false}).and_return("")
         allow($CHILD_STATUS).to receive(:exitstatus).and_return(0)
         provider.start
       end