Puppet

I’ve been investigating this and have discussed it with Luke. We agreed that having some global state in a common namespace is necessary, because the desired behavior will necessitate process/context-agnostic things (like the transactions) to check for a stop/restart request. Therefore, here’s what I think we should do at this point: * introduce a global state attribute (e.g. Puppet::Application.run_state) * introduce some helper predicates based on this, e.g.: – Puppet::Application.stop_requested? (returns result of Puppet::Application.run_state == :stop) – Puppet::Application.restart_requested? (returns result of Puppet::Application.run_state == :restart) – Puppet::Application.interrupted? (perhaps returns result of {:stop => 1, :restart => 1}.has_key?(Puppet::Application.run_state) ) * perhaps introduce similar helper setters: – Puppet::Application.stop! (would perform Puppet::Application.run_state = :stop) – Puppet::Application.restart! (guess what this does!) * modify Puppet::Daemon’s start/stop methods to call these Puppet::Application thingees, and to not call agent.stop * modify Puppet::Agent to remove its stop method, and to base its stopping attribute on Puppet::Application.stop_requested? * just verify that the agent won’t do a run if the stop/restart flag is set. * adjust the transaction process to consult Puppet::Application.stop_requested? between resources and return early if set.

We don’t need Puppet::Agent#stop, because: * the only agent ever used is in puppetd * the agent simply needs to not run again when a stop is requested, and otherwise the agent’s client needs to bail out early based on the stop/restart request values. * the only client ever used with the agent is Puppet::Configurer, which does not have a stop method, and thus no client is ever stopped by stopping an Agent.

Consequently we can throw out all unit tests related to Puppet::Agent#stop, and replace them with tests that verify: * stopping? is based on Puppet::Application state * run won’t actually do anything if Puppet::Application is in an interruption

And we introduce unit tests to validate that: * the signal handlers for Puppet::Daemon do the right thing with the new Puppet::Application state * the transaction process will return early if you Puppet::Application.stop! * the transaction will complete but the agent will then stop running upon Puppet::Application.restart!

All I’m not sure about at present is how to handle the delayed restart behavior within this, but that’s largely because I’ve run out of time to sort it of. I’ll look at that tomorrow.

• Ethan

This is perhaps a bit loosely related, but currently puppetmasterd doesn’t handle a SIGHUP well.

Jul 26 19:09:20 inocybe puppetmasterd[30458]: Caught HUP; calling restart
Jul 26 19:09:20 inocybe puppetmasterd[30458]: Restarting with '/usr/sbin/puppetmasterd '

It logs that it received the HUP, but fails to reload or restart. It just dies instead. If it’s quite difficult to get puppetmasterd to properly reload/restart on HUP, perhaps it would be easy enough to at least catch the signal and log that HUP is not supported?

I think for addressing the current restart stuff on the agent, all we really need to do is have Puppet::Agent#needing_restart? consult the Puppet::Application stuff appropriately. We probably can drop the “configure_delayed_restart” and “restart” methods from Puppet::Agent; it doesn’t look like anything actually calls restart, and “configure_delayed_restart” isn’t necessary if we base the “needing_restart?” method off the global stuff.

• Ethan

Luke, do you think for 0.25.0 it might be worth not attempting to restart on SIGHUP, since that just ends up killing puppetmasterd? If coding that is too invasive, would a patch to the puppet.conf man page section on Signals which notes that puppetmasterd does not restart on HUP be acceptable? (I can do the latter, while the former is probably not something I should attempt. ;)

Eventually (i.e. 0.26.0) I think HUP should re-read the configuration, if that’s possible. For 0.25.0, ensuring that HUP doesn’t just kill puppetmasterd would be great. In an ideal world, this would be done in the code, but if that’s too disruptive and/or time-consuming this close to a release, I can just update the wording of the documentation so that anyone reading it won’t try to send a HUP to puppetmasterd. But given how many people read documentation, this is less than ideal. Then again, maybe not many people stumble onto the section on signals in puppet.conf(8) and try to update the init script to send a HUP. :)

I don’t think I’m competent to provide a decent code patch for this. If no one pops up with a code patch, I’ll plan to whip up a documentation patch for puppet.conf(8) before 0.25.0.

Luke Kanies wrote:

What should the behaviour be on HUP? Should we just ignore it, and force people to kill and restart?

For 0.25.0, this would be the best thing it seems. Perhaps logging something like: ‘SIGHUP is currently unsupported, please restart puppetmasterd’ ? That way, it will at least be clear that puppet got the signal, which might cause less admins to scratch their heads or file bugs.

Todd Zullinger wrote:

Luke Kanies wrote:

What should the behaviour be on HUP? Should we just ignore it, and force people to kill and restart?

For 0.25.0, this would be the best thing it seems. Perhaps logging something like: ‘SIGHUP is currently unsupported, please restart puppetmasterd’ ? That way, it will at least be clear that puppet got the signal, which might cause less admins to scratch their heads or file bugs.

That seems perfectly reasonable – just have ‘restart’ do this instead of reexec'ing if we’re the server.

I’m not sure what the status of this is at present; I did a bunch of work reorganizing the process state management for this ticket, but it hasn’t been committed. I couldn’t quite get it stable by the 0.25 rc1 and we held off.

Anyway, my work is here: http://github.com/ethanrowe/puppet/tree/tickets/master/2239

The coordination of stopping/restarting is clearer than what preceded it, but the exact behavior of the signal handlers is not quite what has been most recently discussed in this issue.

If this work seems to be a decent approach for 0.26, I’m happy to tweak the signal handler methods and submit it for review. Just let me know what you think.

Thanks. – Ethan

Ethan Rowe wrote:

I’m not sure what the status of this is at present; I did a bunch of work reorganizing the process state management for this ticket, but it hasn’t been committed. I couldn’t quite get it stable by the 0.25 rc1 and we held off.

Anyway, my work is here: http://github.com/ethanrowe/puppet/tree/tickets/master/2239

The coordination of stopping/restarting is clearer than what preceded it, but the exact behavior of the signal handlers is not quite what has been most recently discussed in this issue.

If this work seems to be a decent approach for 0.26, I’m happy to tweak the signal handler methods and submit it for review. Just let me know what you think.

It should definitely be in 0.26, but let’s hold off on submitting it for review until 0.25 actually makes it out.

Note that this was discussed in puppet-dev: http://groups.google.com/group/puppet-dev/browse_thread/thread/91a467e366afb7f2

The new signal behavior will be:

All of the following signals should do a restart; there is no ‘reload’ functionality via signals (even though that’s a normal usage of HUP).

INT, TERM – finishes the current resource, then restarts HUP – finishes the current transaction (i.e., will go through all the resources in the transaction), then restarts KILL – kills the process (note that we can’t catch that signal anyway, so there’s nothing we can do to code its handling)

If this isn’t correct, let me know.

Todd?