Fix #1934 - detailed-exitcodes for puppetd

This option only works when --onetime is specified, as it doesn't make
much sense to worry about exit codes in the context of a long-running
daemon.

This required a refactoring of the existing --detailed-exitcodes code,
as "puppetd" wasn't directly creating a transaction object (like
"puppet" does).

Added Report::exit_status, which did what was previously hard-coded
into the "puppet" executable.

An Agent's "run" method now returns a value (the result of the
individual client class' "run" method)

The "puppetd" agent's "run" method now returns a transaction report, as
that seems like the logical thing to return as the result of applying a
catalog.

Signed-off-by: Deepak Giridharagopal <deepak@brownman.org>

lib/puppet/agent.rb

@@ -48,14 +48,16 @@ class Puppet::Agent
             return
         end
         splay
+        result = nil
         with_client do |client|
             begin
-                sync.synchronize { lock { client.run(*args) } }
+                sync.synchronize { lock { result = client.run(*args) } }
             rescue => detail
                 puts detail.backtrace if Puppet[:trace]
                 Puppet.err "Could not run %s: %s" % [client_class, detail]
             end
         end
+        result
     end
 
     def stop

lib/puppet/application/puppet.rb

@@ -127,13 +127,12 @@ Puppet::Application.new(:puppet) do
             # And apply it
             transaction = catalog.apply
 
-            status = 0
             if not Puppet[:noop] and options[:detailed_exitcodes] then
                 transaction.generate_report
-                status |= 2 if transaction.report.metrics["changes"][:total] > 0
-                status |= 4 if transaction.report.metrics["resources"][:failed] > 0
+                exit(transaction.report.exit_status)
+            else
+                exit(0)
             end
-            exit(status)
         rescue => detail
             if Puppet[:trace]
                 puts detail.backtrace

lib/puppet/application/puppetd.rb

@@ -21,6 +21,7 @@ Puppet::Application.new(:puppetd) do
         {
             :waitforcert => 120,  # Default to checking for certs every 5 minutes
             :onetime => false,
+            :detailed_exitcodes => false,
             :verbose => false,
             :debug => false,
             :centrallogs => false,
@@ -65,6 +66,10 @@ Puppet::Application.new(:puppetd) do
         options[:waitforcert] = 0 unless @explicit_waitforcert
     end
 
+    option("--detailed-exitcodes") do |arg|
+        options[:detailed_exitcodes] = true
+    end
+
     option("--logdest DEST", "-l DEST") do |arg|
         begin
             Puppet::Util::Log.newdestination(arg)
@@ -95,19 +100,25 @@ Puppet::Application.new(:puppetd) do
         unless options[:client]
             $stderr.puts "onetime is specified but there is no client"
             exit(43)
+            return
         end
 
         @daemon.set_signal_traps
 
         begin
-            @agent.run
+            report = @agent.run
         rescue => detail
             if Puppet[:trace]
                 puts detail.backtrace
             end
             Puppet.err detail.to_s
         end
-        exit(0)
+
+        if not Puppet[:noop] and options[:detailed_exitcodes] then
+            exit(report.exit_status)
+        else
+            exit(0)
+        end
     end
 
     command(:main) do
@@ -125,6 +136,7 @@ Puppet::Application.new(:puppetd) do
         Puppet.settings.handlearg("--no-daemonize")
         options[:verbose] = true
         options[:onetime] = true
+        options[:detailed_exitcodes] = true
         options[:waitforcert] = 0
     end
 

lib/puppet/configurer.rb

@@ -144,13 +144,17 @@ class Puppet::Configurer
 
         begin
             benchmark(:notice, "Finished catalog run") do
-                catalog.apply(options)
+                transaction = catalog.apply(options)
+                transaction.generate_report
+                report = transaction.report
             end
+            report
         rescue => detail
             puts detail.backtrace if Puppet[:trace]
             Puppet.err "Failed to apply catalog: %s" % detail
+            return
         end
-
+    ensure
         # Now close all of our existing http connections, since there's no
         # reason to leave them lying open.
         Puppet::Network::HttpPool.clear_http_instances

lib/puppet/transaction/report.rb

@@ -83,5 +83,14 @@ class Puppet::Transaction::Report
         end
         return ret
     end
-end
 
+    # Based on the contents of this report's metrics, compute a single number
+    # that represents the report. The resulting number is a bitmask where
+    # individual bits represent the presence of different metrics.
+    def exit_status
+        status = 0
+        status |= 2 if @metrics["changes"][:total] > 0
+        status |= 4 if @metrics["resources"][:failed] > 0
+        return status
+    end
+end

sbin/puppetd

@@ -8,7 +8,8 @@
 #
 # = Usage
 #
-#   puppetd  [-D|--daemonize|--no-daemonize] [-d|--debug] [--disable] [--enable]
+#   puppetd  [-D|--daemonize|--no-daemonize] [-d|--debug]
+#       [--detailed-exitcodes] [--disable] [--enable]
 #       [-h|--help] [--fqdn <host name>] [-l|--logdest syslog|<file>|console]
 #       [-o|--onetime] [--serve <handler>] [-t|--test] [--noop]
 #       [-V|--version] [-v|--verbose] [-w|--waitforcert <seconds>]
@@ -71,6 +72,12 @@
 # debug::
 #   Enable full debugging.
 #
+# detailed-exitcodes::
+#   Provide transaction information via exit codes.  If this is enabled, an
+#   exit code of '2' means there were changes, and an exit code of '4' means
+#   that there were failures during the transaction. This option only makes
+#   sense in conjunction with --onetime.
+#
 # disable::
 #   Disable working on the local system.  This puts a lock file in place,
 #   causing +puppetd+ not to work on the system until the lock file is removed.

spec/unit/application/puppet.rb

@@ -283,52 +283,37 @@ describe "Puppet" do
                 @puppet.main
             end
 
-            it "should generate a report if not noop" do
-                Puppet.stubs(:[]).with(:noop).returns(false)
-                @puppet.options.stubs(:[]).with(:detailed_exitcodes).returns(true)
-                metrics = stub 'metrics', :[] => { :total => 10, :failed => 0}
-                report = stub 'report', :metrics => metrics
-                @transaction.stubs(:report).returns(report)
-
-                @transaction.expects(:generate_report)
-
-                @puppet.main
-            end
-
             describe "with detailed_exitcodes" do
-                before :each do
+                it "should exit with report's computed exit status" do
                     Puppet.stubs(:[]).with(:noop).returns(false)
                     @puppet.options.stubs(:[]).with(:detailed_exitcodes).returns(true)
-                end
-
-                it "should exit with exit code of 2 if changes" do
-                    report = stub 'report', :metrics => { "changes" => {:total => 1}, "resources" => {:failed => 0} }
-                    @transaction.stubs(:generate_report).returns(report)
+                    report = stub 'report', :exit_status => 666
                     @transaction.stubs(:report).returns(report)
-                    @puppet.expects(:exit).with(2)
+                    @puppet.expects(:exit).with(666)
 
                     @puppet.main
                 end
 
-                it "should exit with exit code of 4 if failures" do
-                    report = stub 'report', :metrics => { "changes" => {:total => 0}, "resources" => {:failed => 1} }
-                    @transaction.stubs(:generate_report).returns(report)
+                it "should always exit with 0 if option is disabled" do
+                    Puppet.stubs(:[]).with(:noop).returns(false)
+                    @puppet.options.stubs(:[]).with(:detailed_exitcodes).returns(false)
+                    report = stub 'report', :exit_status => 666
                     @transaction.stubs(:report).returns(report)
-                    @puppet.expects(:exit).with(4)
+                    @puppet.expects(:exit).with(0)
 
                     @puppet.main
                 end
 
-                it "should exit with exit code of 6 if changes and failures" do
-                    report = stub 'report', :metrics => { "changes" => {:total => 1}, "resources" => {:failed => 1} }
-                    @transaction.stubs(:generate_report).returns(report)
+                it "should always exit with 0 if --noop" do
+                    Puppet.stubs(:[]).with(:noop).returns(true)
+                    @puppet.options.stubs(:[]).with(:detailed_exitcodes).returns(true)
+                    report = stub 'report', :exit_status => 666
                     @transaction.stubs(:report).returns(report)
-                    @puppet.expects(:exit).with(6)
+                    @puppet.expects(:exit).with(0)
 
                     @puppet.main
                 end
             end
-
         end
 
         describe "the 'apply' command" do

spec/unit/application/puppetd.rb

@@ -205,6 +205,10 @@ describe "puppetd" do
                 @puppetd.options.expects(:[]=).with(:onetime,true)
                 @puppetd.setup_test
             end
+            it "should set options[:detailed_exitcodes] to true" do
+                @puppetd.options.expects(:[]=).with(:detailed_exitcodes,true)
+                @puppetd.setup_test
+            end
             it "should set waitforcert to 0" do
                 @puppetd.options.expects(:[]=).with(:waitforcert,0)
                 @puppetd.setup_test
@@ -453,6 +457,7 @@ describe "puppetd" do
 
             before :each do
                 @puppetd.options.stubs(:[]).with(:client).returns(:client)
+                @puppetd.options.stubs(:[]).with(:detailed_exitcodes).returns(false)
                 @puppetd.stubs(:exit).with(0)
                 Puppet.stubs(:newservice)
             end
@@ -484,6 +489,29 @@ describe "puppetd" do
                 @puppetd.onetime
             end
 
+            describe "and --detailed-exitcodes" do
+                before :each do
+                    @puppetd.options.stubs(:[]).with(:detailed_exitcodes).returns(true)
+                end
+
+                it "should exit with report's computed exit status" do
+                    Puppet.stubs(:[]).with(:noop).returns(false)
+                    report = stub 'report', :exit_status => 666
+                    @agent.stubs(:run).returns(report)
+                    @puppetd.expects(:exit).with(666)
+
+                    @puppetd.onetime
+                end
+
+                it "should always exit with 0 if --noop" do
+                    Puppet.stubs(:[]).with(:noop).returns(true)
+                    report = stub 'report', :exit_status => 666
+                    @agent.stubs(:run).returns(report)
+                    @puppetd.expects(:exit).with(0)
+
+                    @puppetd.onetime
+                end
+            end
         end
 
         describe "without --onetime" do

spec/unit/transaction/report.rb

@@ -38,3 +38,26 @@ describe Puppet::Transaction::Report, " when being indirect" do
         Puppet::Util::Cacher.expire
     end
 end
+
+describe Puppet::Transaction::Report, " when computing exit status" do
+    it "should compute 2 if changes present" do
+        report = Puppet::Transaction::Report.new
+        report.newmetric("changes", {:total => 1})
+        report.newmetric("resources", {:failed => 0})
+        report.exit_status.should == 2
+    end
+
+    it "should compute 4 if failures present" do
+        report = Puppet::Transaction::Report.new
+        report.newmetric("changes", {:total => 0})
+        report.newmetric("resources", {:failed => 1})
+        report.exit_status.should == 4
+    end
+
+    it "should compute 6 if both changes and present" do
+        report = Puppet::Transaction::Report.new
+        report.newmetric("changes", {:total => 1})
+        report.newmetric("resources", {:failed => 1})
+        report.exit_status.should == 6
+    end
+end