I believe we'd essentially accepted these patches in review, right? Seems like it should be stuck into 'ready for testing' and make it into our nascent 'test' branch.
On Sep 14, 2009, at 9:09 AM, Steven Jenkins wrote: > > Now that 0.25.0 is out, I rebased Ethan's 2239 commit set onto > master and > verified that HUP still restarts the process *after* the transaction > and that > the other signals stop during the transaction. > > Could someone take a look (patch is attached and also at github at > http://github.com/stevenjenkins/puppet/tree/tickets/master/2239 -- > it's been > pushed and should show up there shortly). > > Let me know if the behavior is what is desired and/or if other > changes need to > be made. > > Thanks, > Steven Jenkins > End Point Corporation > > > > > From 063c05b6b4ee5451d972b9fb2e7d1a1a3c2c3d9d Mon Sep 17 00:00:00 2001 > From: Steven Jenkins <[email protected]> > Date: Mon, 14 Sep 2009 12:04:10 -0400 > Subject: [PATCH] FIXES 2239: changes to signal handling > > This squashes Ethan Rowe's 5 commits into a single commit > and verifies that it can be rebased onto master. > --- > lib/puppet/agent.rb | 58 +++++--------------- > lib/puppet/application.rb | 76 +++++++++++++++++++++++++++ > lib/puppet/daemon.rb | 12 ++--- > lib/puppet/transaction.rb | 7 +++ > spec/unit/agent.rb | 124 +++++++++++++++++++++++++ > +------------------ > spec/unit/application.rb | 127 ++++++++++++++++++++++++++++++++++++ > +++++++++ > spec/unit/daemon.rb | 47 +++++++++++------ > spec/unit/transaction.rb | 50 ++++++++++++++++++ > 8 files changed, 384 insertions(+), 117 deletions(-) > > diff --git a/lib/puppet/agent.rb b/lib/puppet/agent.rb > index 7d8ea7a..4240051 100644 > --- a/lib/puppet/agent.rb > +++ b/lib/puppet/agent.rb > @@ -1,5 +1,6 @@ > require 'sync' > require 'puppet/external/event-loop' > +require 'puppet/application' > > # A general class for triggering a run of another > # class. > @@ -9,12 +10,7 @@ class Puppet::Agent > > require 'puppet/agent/runner' > > - attr_reader :client_class, :client, :needing_restart, :splayed > - attr_accessor :stopping > - > - def configure_delayed_restart > - @needing_restart = true > - end > + attr_reader :client_class, :client, :splayed > > # Just so we can specify that we are "the" instance. > def initialize(client_class) > @@ -28,13 +24,7 @@ class Puppet::Agent > end > > def needing_restart? > - @needing_restart > - end > - > - def restart > - configure_delayed_restart and return if running? > - Process.kill(:HUP, $$) > - @needing_restart = false > + Puppet::Application.restart_requested? > end > > # Perform a run with our client. > @@ -43,41 +33,23 @@ class Puppet::Agent > Puppet.notice "Run of %s already in progress; skipping" > % client_class > return > end > - if stopping? > - Puppet.notice "In shutdown progress; skipping run" > - return > - end > - splay > - with_client do |client| > - begin > - sync.synchronize { lock { client.run(*args) } } > - rescue => detail > - puts detail.backtrace if Puppet[:trace] > - Puppet.err "Could not run %s: %s" % [client_class, > detail] > + block_run = Puppet::Application.controlled_run do > + splay > + with_client do |client| > + begin > + sync.synchronize { lock { client.run(*args) } } > + rescue => detail > + puts detail.backtrace if Puppet[:trace] > + Puppet.err "Could not run %s: %s" % > [client_class, detail] > + end > end > + true > end > - end > - > - def stop > - if self.stopping? > - Puppet.notice "Already in shutdown" > - return > - end > - self.stopping = true > - if client and client.respond_to?(:stop) > - begin > - client.stop > - rescue > - puts detail.backtrace if Puppet[:trace] > - Puppet.err "Could not stop %s: %s" % [client_class, > detail] > - end > - end > - ensure > - self.stopping = false > + Puppet.notice "Shutdown/restart in progress; skipping run" > unless block_run > end > > def stopping? > - stopping > + Puppet::Application.stop_requested? > end > > # Have we splayed already? > diff --git a/lib/puppet/application.rb b/lib/puppet/application.rb > index 2882c81..3c5bcb5 100644 > --- a/lib/puppet/application.rb > +++ b/lib/puppet/application.rb > @@ -5,6 +5,7 @@ require 'optparse' > # * setting up options > # * setting up logs > # * choosing what to run > +# * representing execution status > # > # === Usage > # The application is a Puppet::Application object that register > itself in the list > @@ -86,12 +87,87 @@ require 'optparse' > # to be run. > # If it doesn't exist, it defaults to execute the +main+ command if > defined. > # > +# === Execution state > +# The class attributes/methods of Puppet::Application serve as a > global place to set and query the execution > +# status of the application: stopping, restarting, etc. The > setting of the application status does not directly > +# aftect its running status; it's assumed that the various > components within the application will consult these > +# settings appropriately and affect their own processing > accordingly. Control operations (signal handlers and > +# the like) should set the status appropriately to indicate to the > overall system that it's the process of > +# stopping or restarting (or just running as usual). > +# > +# So, if something in your application needs to stop the process, > for some reason, you might consider: > +# > +# def stop_me! > +# # indicate that we're stopping > +# Puppet::Application.stop! > +# # ...do stuff... > +# end > +# > +# And, if you have some component that involves a long-running > process, you might want to consider: > +# > +# def my_long_process(giant_list_to_munge) > +# giant_list_to_munge.collect do |member| > +# # bail if we're stopping > +# return if Puppet::Application.stop_requested? > +# process_member(member) > +# end > +# end > class Puppet::Application > include Puppet::Util > > @@applications = {} > class << self > include Puppet::Util > + > + attr_accessor :run_status > + > + def clear! > + self.run_status = nil > + end > + > + def stop! > + self.run_status = :stop_requested > + end > + > + def restart! > + self.run_status = :restart_requested > + end > + > + # Indicates that Puppet::Application.restart! has been > invoked and components should > + # do what is necessary to facilitate a restart. > + def restart_requested? > + :restart_requested == run_status > + end > + > + # Indicates that Puppet::Application.stop! has been invoked > and components should do what is necessary > + # for a clean stop. > + def stop_requested? > + :stop_requested == run_status > + end > + > + # Indicates that one of stop! or start! was invoked on > Puppet::Application, and some kind of process > + # shutdown/short-circuit may be necessary. > + def interrupted? > + [:restart_requested, :stop_requested].include? run_status > + end > + > + # Indicates that Puppet::Application believes that it's in > usual running mode (no stop/restart request > + # currently active). > + def clear? > + run_status.nil? > + end > + > + # Only executes the given block if the run status of > Puppet::Application is clear (no restarts, stops, > + # etc. requested). > + # Upon block execution, checks the run status again; if a > restart has been requested during the block's > + # execution, then controlled_run will send a new HUP signal > to the current process. > + # Thus, long-running background processes can potentially > finish their work before a restart. > + def controlled_run(&block) > + return unless clear? > + result = block.call > + Process.kill(:HUP, $$) if restart_requested? > + result > + end > end > > attr_reader :options, :opt_parser > diff --git a/lib/puppet/daemon.rb b/lib/puppet/daemon.rb > index 16b1f80..d4680ab 100755 > --- a/lib/puppet/daemon.rb > +++ b/lib/puppet/daemon.rb > @@ -1,6 +1,7 @@ > require 'puppet' > require 'puppet/util/pidlock' > require 'puppet/external/event-loop' > +require 'puppet/application' > > # A module that handles operations common to all daemons. This is > included > # into the Server and Client base classes. > @@ -83,11 +84,8 @@ class Puppet::Daemon > end > > def restart > - if agent and agent.running? > - agent.configure_delayed_restart > - else > - reexec > - end > + Puppet::Application.restart! > + reexec unless agent and agent.running? > end > > def reopen_logs > @@ -107,9 +105,9 @@ class Puppet::Daemon > > # Stop everything > def stop(args = {:exit => true}) > - server.stop if server > + Puppet::Application.stop! > > - agent.stop if agent > + server.stop if server > > remove_pidfile() > > diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb > index d04856d..e4a599d 100644 > --- a/lib/puppet/transaction.rb > +++ b/lib/puppet/transaction.rb > @@ -2,6 +2,7 @@ > # and performs them > > require 'puppet' > +require 'puppet/application' > > module Puppet > class Transaction > @@ -19,6 +20,11 @@ class Transaction > > include Puppet::Util > > + # Wraps application run state check to flag need to interrupt > processing > + def stop_processing? > + Puppet::Application.stop_requested? > + end > + > # Add some additional times for reporting > def addtimes(hash) > hash.each do |name, num| > @@ -285,6 +291,7 @@ class Transaction > > begin > allevents = @sorted_resources.collect { |resource| > + next if stop_processing? > if resource.is_a?(Puppet::Type::Component) > Puppet.warning "Somehow left a component in the > relationship graph" > next > diff --git a/spec/unit/agent.rb b/spec/unit/agent.rb > index c133ca9..0a7fd57 100755 > --- a/spec/unit/agent.rb > +++ b/spec/unit/agent.rb > @@ -15,12 +15,35 @@ class AgentTestClient > end > end > > +def without_warnings > + flag = $VERBOSE > + $VERBOSE = nil > + yield > + $VERBOSE = flag > +end > + > describe Puppet::Agent do > before do > @agent = Puppet::Agent.new(AgentTestClient) > > # So we don't actually try to hit the filesystem. > @agent.stubs(:lock).yields > + > + # make Puppet::Application safe for stubbing; restore in > an :after block; silence warnings for this. > + without_warnings { Puppet::Application = > Class.new(Puppet::Application) } > + Puppet::Application.stubs(:clear?).returns(true) > + Puppet::Application.class_eval do > + class << self > + def controlled_run(&block) > + block.call > + end > + end > + end > + end > + > + after do > + # restore Puppet::Application from stub-safe subclass, and > silence warnings > + without_warnings { Puppet::Application = > Puppet::Application.superclass } > end > > it "should set its client class at initialization" do > @@ -73,9 +96,10 @@ describe Puppet::Agent do > @agent.run > end > > - it "should do nothing if it is in the process of stopping" do > - @agent.expects(:stopping?).returns true > - AgentTestClient.expects(:new).never > + it "should use Puppet::Application.controlled_run to manage > process state behavior" do > + calls = sequence('calls') > + > Puppet > ::Application.expects(:controlled_run).yields().in_sequence(calls) > + AgentTestClient.expects(:new).once.in_sequence(calls) > @agent.run > end > > @@ -170,27 +194,56 @@ describe Puppet::Agent do > end > end > > - describe "when stopping" do > - it "should do nothing if already stopping" do > - @agent.expects(:stopping?).returns true > - @agent.stop > + describe "when checking execution state" do > + describe 'with regular run status' do > + before :each do > + > Puppet::Application.stubs(:restart_requested?).returns(false) > + > Puppet::Application.stubs(:stop_requested?).returns(false) > + > Puppet::Application.stubs(:interrupted?).returns(false) > + Puppet::Application.stubs(:clear?).returns(true) > + end > + > + it 'should be false for :stopping?' do > + @agent.stopping?.should be_false > + end > + > + it 'should be false for :needing_restart?' do > + @agent.needing_restart?.should be_false > + end > end > > - it "should stop the client if one is available and it > responds to 'stop'" do > - client = AgentTestClient.new > - > - @agent.stubs(:client).returns client > - client.expects(:stop) > - @agent.stop > + describe 'with a stop requested' do > + before :each do > + Puppet::Application.stubs(:clear?).returns(false) > + > Puppet::Application.stubs(:restart_requested?).returns(false) > + > Puppet::Application.stubs(:stop_requested?).returns(true) > + > Puppet::Application.stubs(:interrupted?).returns(true) > + end > + > + it 'should be true for :stopping?' do > + @agent.stopping?.should be_true > + end > + > + it 'should be false for :needing_restart?' do > + @agent.needing_restart?.should be_false > + end > end > > - it "should mark itself as stopping while waiting for the > client to stop" do > - client = AgentTestClient.new > - > - @agent.stubs(:client).returns client > - client.expects(:stop).with { @agent.should be_stopping; > true } > - > - @agent.stop > + describe 'with a restart requested' do > + before :each do > + Puppet::Application.stubs(:clear?).returns(false) > + > Puppet::Application.stubs(:restart_requested?).returns(true) > + > Puppet::Application.stubs(:stop_requested?).returns(false) > + > Puppet::Application.stubs(:interrupted?).returns(true) > + end > + > + it 'should be false for :stopping?' do > + @agent.stopping?.should be_false > + end > + > + it 'should be true for :needing_restart?' do > + @agent.needing_restart?.should be_true > + end > end > end > > @@ -225,35 +278,4 @@ describe Puppet::Agent do > @agent.start > end > end > - > - describe "when restarting" do > - it "should configure itself for a delayed restart if > currently running" do > - @agent.expects(:running?).returns true > - > - @agent.restart > - > - @agent.should be_needing_restart > - end > - > - it "should hup itself if not running" do > - @agent.expects(:running?).returns false > - > - Process.expects(:kill).with(:HUP, $$) > - > - @agent.restart > - end > - > - it "should turn off the needing_restart switch" do > - > @agent.expects(:running?).times(2).returns(true).then.returns false > - > - Process.stubs(:kill) > - > - # First call sets up the switch > - @agent.restart > - > - # Second call should disable it > - @agent.restart > - @agent.should_not be_needing_restart > - end > - end > end > diff --git a/spec/unit/application.rb b/spec/unit/application.rb > index c087373..87a9009 100755 > --- a/spec/unit/application.rb > +++ b/spec/unit/application.rb > @@ -36,6 +36,133 @@ describe Puppet::Application do > @app.get_command.should == :main > end > > + describe 'when invoking clear!' do > + before :each do > + Puppet::Application.run_status = :stop_requested > + Puppet::Application.clear! > + end > + > + it 'should have nil run_status' do > + Puppet::Application.run_status.should be_nil > + end > + > + it 'should return false for restart_requested?' do > + Puppet::Application.restart_requested?.should be_false > + end > + > + it 'should return false for stop_requested?' do > + Puppet::Application.stop_requested?.should be_false > + end > + > + it 'should return false for interrupted?' do > + Puppet::Application.interrupted?.should be_false > + end > + > + it 'should return true for clear?' do > + Puppet::Application.clear?.should be_true > + end > + end > + > + describe 'after invoking stop!' do > + before :each do > + Puppet::Application.run_status = nil > + Puppet::Application.stop! > + end > + > + after :each do > + Puppet::Application.run_status = nil > + end > + > + it 'should have run_status of :stop_requested' do > + Puppet::Application.run_status.should == :stop_requested > + end > + > + it 'should return true for stop_requested?' do > + Puppet::Application.stop_requested?.should be_true > + end > + > + it 'should return false for restart_requested?' do > + Puppet::Application.restart_requested?.should be_false > + end > + > + it 'should return true for interrupted?' do > + Puppet::Application.interrupted?.should be_true > + end > + > + it 'should return false for clear?' do > + Puppet::Application.clear?.should be_false > + end > + end > + > + describe 'when invoking restart!' do > + before :each do > + Puppet::Application.run_status = nil > + Puppet::Application.restart! > + end > + > + after :each do > + Puppet::Application.run_status = nil > + end > + > + it 'should have run_status of :restart_requested' do > + Puppet::Application.run_status.should > == :restart_requested > + end > + > + it 'should return true for restart_requested?' do > + Puppet::Application.restart_requested?.should be_true > + end > + > + it 'should return false for stop_requested?' do > + Puppet::Application.stop_requested?.should be_false > + end > + > + it 'should return true for interrupted?' do > + Puppet::Application.interrupted?.should be_true > + end > + > + it 'should return false for clear?' do > + Puppet::Application.clear?.should be_false > + end > + end > + > + describe 'when performing a controlled_run' do > + it 'should not execute block if not :clear?' do > + Puppet::Application.run_status = :stop_requested > + target = mock 'target' > + target.expects(:some_method).never > + Puppet::Application.controlled_run do > + target.some_method > + end > + end > + > + it 'should execute block if :clear?' do > + Puppet::Application.run_status = nil > + target = mock 'target' > + target.expects(:some_method).once > + Puppet::Application.controlled_run do > + target.some_method > + end > + end > + > + it 'should signal process with HUP after block if restart > requested during block execution' do > + Puppet::Application.run_status = nil > + target = mock 'target' > + target.expects(:some_method).once > + old_handler = trap('HUP') { target.some_method } > + begin > + Puppet::Application.controlled_run do > + Puppet::Application.run_status > = :restart_requested > + end > + ensure > + trap('HUP', old_handler) > + end > + end > + > + after :each do > + Puppet::Application.run_status = nil > + end > + end > + > describe "when parsing command-line options" do > > before :each do > diff --git a/spec/unit/daemon.rb b/spec/unit/daemon.rb > index 960d79d..1bf8f56 100755 > --- a/spec/unit/daemon.rb > +++ b/spec/unit/daemon.rb > @@ -3,6 +3,13 @@ > require File.dirname(__FILE__) + '/../spec_helper' > require 'puppet/daemon' > > +def without_warnings > + flag = $VERBOSE > + $VERBOSE = nil > + yield > + $VERBOSE = flag > +end > + > describe Puppet::Daemon do > before do > @daemon = Puppet::Daemon.new > @@ -86,6 +93,14 @@ describe Puppet::Daemon do > @daemon.stubs(:remove_pidfile) > @daemon.stubs(:exit) > Puppet::Util::Log.stubs(:close_all) > + # to make the global safe to mock, set it to a subclass > of itself, > + # then restore it in an after pass > + without_warnings { Puppet::Application = > Class.new(Puppet::Application) } > + end > + > + after do > + # restore from the superclass so we lose the stub garbage > + without_warnings { Puppet::Application = > Puppet::Application.superclass } > end > > it "should stop its server if one is configured" do > @@ -96,11 +111,8 @@ describe Puppet::Daemon do > @daemon.stop > end > > - it "should stop its agent if one is configured" do > - agent = mock 'agent' > - agent.expects(:stop) > - @daemon.stubs(:agent).returns agent > - > + it 'should request a stop from Puppet::Application' do > + Puppet::Application.expects(:stop!) > @daemon.stop > end > > @@ -236,28 +248,31 @@ describe Puppet::Daemon do > end > > describe "when restarting" do > - it "should reexec itself if no agent is available" do > - @daemon.expects(:reexec) > + before do > + without_warnings { Puppet::Application = > Class.new(Puppet::Application) } > + end > > + after do > + without_warnings { Puppet::Application = > Puppet::Application.superclass } > + end > + > + it 'should set Puppet::Application.restart!' do > + Puppet::Application.expects(:restart!) > + @daemon.stubs(:reexec) > @daemon.restart > end > > - it "should reexec itself if the agent is not running" do > - agent = mock 'agent' > - agent.expects(:running?).returns false > - @daemon.stubs(:agent).returns agent > + it "should reexec itself if no agent is available" do > @daemon.expects(:reexec) > > @daemon.restart > end > > - it "should configure the agent for later restart if the > agent is running" do > + it "should reexec itself if the agent is not running" do > agent = mock 'agent' > - agent.expects(:running?).returns true > + agent.expects(:running?).returns false > @daemon.stubs(:agent).returns agent > - @daemon.expects(:reexec).never > - > - agent.expects(:configure_delayed_restart) > + @daemon.expects(:reexec) > > @daemon.restart > end > diff --git a/spec/unit/transaction.rb b/spec/unit/transaction.rb > index 7966c7a..2f04579 100755 > --- a/spec/unit/transaction.rb > +++ b/spec/unit/transaction.rb > @@ -4,6 +4,13 @@ require File.dirname(__FILE__) + '/../spec_helper' > > require 'puppet/transaction' > > +def without_warnings > + flag = $VERBOSE > + $VERBOSE = nil > + yield > + $VERBOSE = flag > +end > + > describe Puppet::Transaction do > it "should match resources by name, not title, when prefetching" > do > @catalog = Puppet::Resource::Catalog.new > @@ -80,6 +87,49 @@ describe Puppet::Transaction do > @transaction.skip?(@resource).should be_true > end > end > + > + describe 'when checking application run state' do > + before do > + without_warnings { Puppet::Application = > Class.new(Puppet::Application) } > + @catalog = Puppet::Resource::Catalog.new > + @transaction = Puppet::Transaction.new(@catalog) > + end > + > + after do > + without_warnings { Puppet::Application = > Puppet::Application.superclass } > + end > + > + it 'should return true for :stop_processing? if > Puppet::Application.stop_requested? is true' do > + Puppet::Application.stubs(:stop_requested?).returns(true) > + @transaction.stop_processing?.should be_true > + end > + > + it 'should return false for :stop_processing? if > Puppet::Application.stop_requested? is false' do > + > Puppet::Application.stubs(:stop_requested?).returns(false) > + @transaction.stop_processing?.should be_false > + end > + > + describe 'within an evaluate call' do > + before do > + @resource = stub 'resource', :ref => 'some_ref' > + @catalog.add_resource @resource > + @transaction.stubs(:prepare) > + @transaction.sorted_resources = [...@resource] > + end > + > + it 'should stop processing if :stop_processing? is > true' do > + @transaction.expects(:stop_processing?).returns(true) > + @transaction.expects(:eval_resource).never > + @transaction.evaluate > + end > + > + it 'should continue processing if :stop_processing? is > false' do > + > @transaction.expects(:stop_processing?).returns(false) > + @transaction.expects(:eval_resource).returns(nil) > + @transaction.evaluate > + end > + end > + end > end > > describe Puppet::Transaction, " when determining tags" do > -- > 1.6.1.rc3 > -- The covers of this book are too far apart. -- Ambrose Bierce --------------------------------------------------------------------- Luke Kanies | http://reductivelabs.com | http://madstop.com --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en -~----------~----~----~----~------~----~------~--~---
