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 --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
>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
