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

lollipopman · 2019-05-10T20:30:04Z

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

puppetlabs-jenkins · 2019-05-10T20:30:06Z

Can one of the admins verify this patch?

puppetcla · 2019-05-11T08:00:20Z

CLA signed by all contributors.

lollipopman · 2019-05-13T16:08:38Z

updated to fix the tests, apologies for missing that with my first commit.

lollipopman · 2019-05-14T17:16:08Z

Would anyone be able to help me debug the windows build failure via AppVeyor, I don't have access to a Windows test box at present. All the tests passed but rspec exited 1 and the build failed.

lib/puppet/util/execution.rb

joshcooper · 2019-05-14T23:04:45Z

lib/puppet/provider/service/service.rb

+      priority = Process::NORMAL_PRIORITY_CLASS
+    else
+      priority = 0
+    end


Could use Puppet::Settings::PrioritySetting::PRIORITY_MAP[:normal] to get the platform-specific priority value.

That code relies on Puppet::Util::Platform.windows? which relies on !!File::ALT_SEPARATOR how would that work for the rspec tests? Since even on windows the specs for other init providers are run, e.g. systemd & openrc.

Historically we've tried to run all of the tests on every platform, as it wasn't always clear which code executed on the agent vs server. For example, if a Windows provider requires a win32-specific gem without using puppet's feature system, them it can cause compilation to fail for all nodes, not just Windows. As a result, you'll see rspec tests for the init, gentoo, etc service providers running on Windows, and they often stub:

allow(Puppet::Util::Platform).to receive(:windows?).and_return(false)

But now we generally exclude provider tests on platforms where it doesn't make sense, like

puppet/spec/unit/provider/service/openbsd_spec.rb

Lines 3 to 4 in 97a68ba

describe 'Puppet::Type::Service::Provider::Openbsd',

unless: Puppet::Util::Platform.windows? || Puppet::Util::Platform.jruby? do

To your point about PRIORITY_MAP, yeah... I think it's fine to branch based on Puppet::Util::Platform.windows? and keep it how you have it, eg

priority = Puppet::Util::Platform.windows? ? Process::NORMAL_PRIORITY_CLASS : 0

@joshcooper I tried the approach you suggested, but that caused spec failures for Appveyor on windows. For instance:

Failures: 1) Puppet::Type::Service::Provider::Gentoo #start should use the supplied start command if specified Failure/Error: execute(command, opts) Service[sshd](provider=gentoo) received :execute with unexpected arguments expected: (["/bin/foo"], {:combine=>true, :failonfail=>true, :override_locale=>false, :priority=>0, :squelch=>false}) got: (["/bin/foo"], {:combine=>true, :failonfail=>true, :override_locale=>false, :priority=>32, :squelch=>false}) Diff: @@ -2,6 +2,6 @@ {:combine=>true, :failonfail=>true, :override_locale=>false, - :priority=>0, + :priority=>32, :squelch=>false}] # ./lib/puppet/provider/service/service.rb:36:in `texecute' # ./lib/puppet/provider/service/init.rb:177:in `texecute' # ./lib/puppet/provider/service/service.rb:50:in `ucommand' # ./lib/puppet/provider/service/base.rb:93:in `start' # ./spec/unit/provider/service/gentoo_spec.rb:83:in `block (3 levels) in <top (required)>' # util/rspec_runner:44:in `run' # util/rspec_runner:59:in `<main>

This happens because priority = Puppet::Util::Platform.windows? ? Process::NORMAL_PRIORITY_CLASS : 0 returns 32 when running on windows even though we really want 0, as this is the Gentoo spec. Thoughts?

I'd skip *nix service providers (gentoo, src, openrc, init, upstart, systemd) when running on Windows by adding the following to their respective top-level describe:

describe ..., unless: Puppet::Util::Platform.windows? do

Looks like you also need to remove the Facter osfamily expectation from some of the tests.

I submitted #7534 to cleanup the specs

awesome thanks, I'll rebase this work ontop of that PR, once it is merged in

@joshcooper given that #7534 has been closed, any thoughts on the direction of this PR?

To @branan's point, I think the execution API should present a platform agnostic way to specify the priority level (eg :normal), and have the different posix vs windows execution implementations map that to the appropriate platform specific value (e.g. 0 on posix or Process::NORMAL_PRIORITY_CLASS on windows).

@joshcooper I pushed the platform specific values into the execution implementations, does that look better?

@joshcooper any chance you have time for another review?

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

jtappa · 2019-10-28T17:00:16Z

lib/puppet/util/execution.rb

@@ -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(Puppet::Settings::PrioritySetting::PRIORITY_MAP[options[:priority]])


Seems like the priority map belongs in puppet/util/limits, not the settings

@jtappa I happy to rework parts of this commit, but I was waiting for someone from puppet to thubs the general approach, prior to spending more time on it.

joshcooper · 2019-10-30T03:39:37Z

Thanks @lollipopman! I'm 👍 with the general PR, and had a few minor comments:

The PR would cause a behavior change by default, as services would be executed at normal priority instead of the priority of the puppet (parent) process. Since puppet has worked this way for a long time, someone may be relying on the current behavior. Insert favorite xkcd "you broke my workflow" quote here.
I'm thinking we should define a priority parameter on the service type and have it default to nil to mean the current behavior (run at the same priority as puppet). But allow users to specify a priority, so in lib/puppet/type/service.rb, add
```
newparam(:priority) do
  newvalues(:high, :normal, etc)
end
```
In the service provider, lookup the current priority from @resource[:priority], and only call setpriority if the value is non-nil.
I agree with @jtappa about moving the priority map from settings to Puppet::Util::Limits, and having settings and service type call that.

jtappa · 2019-11-25T17:52:39Z

Hi @lollipopman! Just checking in on this PR to make sure we keep community PRs moving along. Did you still want to work on this based on @joshcooper's and my feedback?

Thanks!

jtappa · 2019-12-02T17:56:37Z

@lollipopman we recently received a ticket where execute blows up when Puppet Server doesn't know about options or how to handle them. We need to make sure Puppet Server doesn't break if someone calls execute with a priority. This PR may need some updates related to that.

lollipopman · 2019-12-02T18:21:22Z

@jtappa thanks, I'll see if I can find some time to work on this in the next couple of weeks

jtappa · 2019-12-16T18:09:35Z

Because of the nature of our APIs, any changes we make here need to also happen in Puppet Server and right now we don't have the bandwidth between teams to coordinate the changes. We do agree this change is reasonable and something we'd like to include eventually but unfortunately right now we can't move forward with it.

joshcooper · 2020-01-17T19:13:47Z

Kicking Travis

CLAassistant · 2021-09-01T18:05:09Z

All committers have signed the CLA.

lollipopman requested review from a team May 10, 2019 20:30

lollipopman force-pushed the service_priority branch from 92c176b to 7652509 Compare May 13, 2019 16:03

joshcooper added the Community label May 13, 2019

lollipopman force-pushed the service_priority branch 2 times, most recently from e4d6767 to c5c5ac1 Compare May 14, 2019 16:20

joshcooper reviewed May 14, 2019

View reviewed changes

lib/puppet/util/execution.rb Outdated Show resolved Hide resolved

joshcooper reviewed May 14, 2019

View reviewed changes

lib/puppet/util/execution.rb Outdated Show resolved Hide resolved

joshcooper reviewed May 14, 2019

View reviewed changes

lollipopman force-pushed the service_priority branch from c5c5ac1 to 6b90e0d Compare May 15, 2019 17:47

lollipopman force-pushed the service_priority branch from 6b90e0d to 5b17ec6 Compare May 15, 2019 18:27

lollipopman added 2 commits June 7, 2019 17:52

(PUP-1353) abstract away the priority setting

0017a95

(PUP-1353) update windows exec spec

6ee23e2

lollipopman force-pushed the service_priority branch from 4db04e4 to 6ee23e2 Compare June 7, 2019 17:53

ciprianbadescu requested a review from a team October 14, 2019 07:29

jtappa reviewed Oct 28, 2019

View reviewed changes

jtappa added the blocked PRs blocked on work external to the PR itself label Dec 16, 2019

joshcooper closed this Jan 17, 2020

joshcooper reopened this Jan 17, 2020

Dorin-Pleava changed the base branch from master to 6.x December 14, 2020 12:17

Dorin-Pleava requested a review from a team December 14, 2020 12:17

joshcooper removed the community label Nov 29, 2023

	describe 'Puppet::Type::Service::Provider::Openbsd',
	unless: Puppet::Util::Platform.windows? \|\| Puppet::Util::Platform.jruby? do

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

lollipopman commented May 10, 2019

Uh oh!

puppetlabs-jenkins commented May 10, 2019

Uh oh!

puppetcla commented May 11, 2019

Uh oh!

lollipopman commented May 13, 2019

Uh oh!

lollipopman commented May 14, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshcooper commented Oct 30, 2019

Uh oh!

jtappa commented Nov 25, 2019

Uh oh!

jtappa commented Dec 2, 2019

Uh oh!

lollipopman commented Dec 2, 2019

Uh oh!

jtappa commented Dec 16, 2019

Uh oh!

joshcooper commented Jan 17, 2020

Uh oh!

CLAassistant commented Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

CLAassistant commented Sep 1, 2021 •

edited

Loading