(#7127) Stop puppet if a prerun command fails

Before this change there were several problems with pre and post run
commands, logging, and sending of reports.

1. If a prerun command failed, puppet would attempt to apply the
catalog. Now puppet will not apply the catalog, but it will run the
postrun command and send the report (as it did before).

2. If a postrun command failed, puppet would not send the report. Sending the
report is now in an outer ensure block from the postrun command, so
postrun failures won't prevent the report from being sent.

3. Errors, e.g. Puppet.err, occuring during the prepare step, which
which includes plugin/fact download and prerun commands were not
appended to the report. Now the report log destination is registered as
early as possible, and unregistered as late as possible to ensure
Configurer errors that occur in the run method are included in the report.

4. The transaction was closing the Configurer's report destination out
from underneath it. As a result, postrun errors were not included in the
report.

Paired-with: Nick Lewis <nick@puppetlabs.com>
Reviewed-by: Jacob Helwig <jacob@puppetlabs.com>

5 files changed, 85 insertions, 75 deletions

diff --git a/lib/puppet/application/apply.rb b/lib/puppet/application/apply.rb
index 717935640..f2bbcb99b 100644
--- a/lib/puppet/application/apply.rb
+++ b/lib/puppet/application/apply.rb
@@ -130,7 +130,13 @@ class Puppet::Application::Apply < Puppet::Application
       configurer = Puppet::Configurer.new
       report = configurer.run(:skip_plugin_download => true, :catalog => catalog)
 
-      exit( options[:detailed_exitcodes] ? report.exit_status : 0 )
+      if not report
+        exit(1)
+      elsif options[:detailed_exitcodes] then
+        exit(report.exit_status)
+      else
+        exit(0)
+      end
     rescue => detail
       puts detail.backtrace if Puppet[:trace]
       $stderr.puts detail.message
diff --git a/lib/puppet/configurer.rb b/lib/puppet/configurer.rb
index 596d2dc53..7f39a3853 100644
--- a/lib/puppet/configurer.rb
+++ b/lib/puppet/configurer.rb
@@ -5,8 +5,6 @@ require 'puppet/network/http_pool'
 require 'puppet/util'
 
 class Puppet::Configurer
-  class CommandHookError < RuntimeError; end
-
   require 'puppet/configurer/fact_handler'
   require 'puppet/configurer/plugin_handler'
 
@@ -79,8 +77,6 @@ class Puppet::Configurer
     download_plugins unless options[:skip_plugin_download]
 
     download_fact_plugins unless options[:skip_plugin_download]
-
-    execute_prerun_command
   end
 
   # Get the remote catalog, yo.  Returns nil if no catalog can be found.
@@ -109,67 +105,73 @@ class Puppet::Configurer
     catalog
   end
 
-  # The code that actually runs the catalog.
-  # This just passes any options on to the catalog,
-  # which accepts :tags and :ignoreschedules.
-  def run(options = {})
-    begin
-      prepare(options)
-    rescue SystemExit,NoMemoryError
-      raise
-    rescue Exception => detail
-      puts detail.backtrace if Puppet[:trace]
-      Puppet.err "Failed to prepare catalog: #{detail}"
+  # Retrieve (optionally) and apply a catalog. If a catalog is passed in
+  # the options, then apply that one, otherwise retrieve it.
+  def retrieve_and_apply_catalog(options, fact_options)
+    unless catalog = (options.delete(:catalog) || retrieve_catalog(fact_options))
+      Puppet.err "Could not retrieve catalog; skipping run"
+      return
     end
 
-    if Puppet::Resource::Catalog.indirection.terminus_class == :rest
-      # This is a bit complicated.  We need the serialized and escaped facts,
-      # and we need to know which format they're encoded in.  Thus, we
-      # get a hash with both of these pieces of information.
-      fact_options = facts_for_uploading
+    report = options[:report]
+    report.configuration_version = catalog.version
+
+    benchmark(:notice, "Finished catalog run") do
+      catalog.apply(options)
     end
 
+    report.finalize_report
+    report
+  end
+
+  # The code that actually runs the catalog.
+  # This just passes any options on to the catalog,
+  # which accepts :tags and :ignoreschedules.
+  def run(options = {})
     options[:report] ||= Puppet::Transaction::Report.new("apply")
     report = options[:report]
-    Puppet::Util::Log.newdestination(report)
 
-    if catalog = options[:catalog]
-      options.delete(:catalog)
-    elsif ! catalog = retrieve_catalog(fact_options)
-      Puppet.err "Could not retrieve catalog; skipping run"
-      return
-    end
+    Puppet::Util::Log.newdestination(report)
+    begin
+      prepare(options)
 
-    report.configuration_version = catalog.version
+      if Puppet::Resource::Catalog.indirection.terminus_class == :rest
+        # This is a bit complicated.  We need the serialized and escaped facts,
+        # and we need to know which format they're encoded in.  Thus, we
+        # get a hash with both of these pieces of information.
+        fact_options = facts_for_uploading
+      end
 
-    transaction = nil
+      # set report host name now that we have the fact
+      report.host = Puppet[:node_name_value]
 
-    begin
-      benchmark(:notice, "Finished catalog run") do
-        transaction = catalog.apply(options)
+      begin
+        execute_prerun_command or return nil
+        retrieve_and_apply_catalog(options, fact_options)
+      rescue SystemExit,NoMemoryError
+        raise
+      rescue => detail
+        puts detail.backtrace if Puppet[:trace]
+        Puppet.err "Failed to apply catalog: #{detail}"
+        return nil
+      ensure
+        execute_postrun_command or return nil
       end
-      report
-    rescue => detail
-      puts detail.backtrace if Puppet[:trace]
-      Puppet.err "Failed to apply catalog: #{detail}"
-      return
+    ensure
+      # Make sure we forget the retained module_directories of any autoload
+      # we might have used.
+      Thread.current[:env_module_directories] = nil
+
+      # Now close all of our existing http connections, since there's no
+      # reason to leave them lying open.
+      Puppet::Network::HttpPool.clear_http_instances
     end
   ensure
-    # Make sure we forget the retained module_directories of any autoload
-    # we might have used.
-    Thread.current[:env_module_directories] = nil
-
-    # Now close all of our existing http connections, since there's no
-    # reason to leave them lying open.
-    Puppet::Network::HttpPool.clear_http_instances
-    execute_postrun_command
-
     Puppet::Util::Log.close(report)
-    send_report(report, transaction)
+    send_report(report)
   end
 
-  def send_report(report, trans)
-    report.finalize_report if trans
+  def send_report(report)
     puts report.summary if Puppet[:summarize]
     save_last_run_summary(report)
     report.save if Puppet[:report]
@@ -207,12 +209,15 @@ class Puppet::Configurer
   end
 
   def execute_from_setting(setting)
-    return if (command = Puppet[setting]) == ""
+    return true if (command = Puppet[setting]) == ""
 
     begin
       Puppet::Util.execute([command])
+      true
     rescue => detail
-      raise CommandHookError, "Could not run command from #{setting}: #{detail}"
+      puts detail.backtrace if Puppet[:trace]
+      Puppet.err "Could not run command from #{setting}: #{detail}"
+      false
     end
   end
 
diff --git a/lib/puppet/resource/catalog.rb b/lib/puppet/resource/catalog.rb
index 8d4918bbf..0a63ff172 100644
--- a/lib/puppet/resource/catalog.rb
+++ b/lib/puppet/resource/catalog.rb
@@ -132,7 +132,9 @@ class Puppet::Resource::Catalog < Puppet::SimpleGraph
     expire
 
     Puppet::Util::Storage.load if host_config?
+
     transaction = Puppet::Transaction.new(self, options[:report])
+    register_report = options[:report].nil?
 
     transaction.tags = options[:tags] if options[:tags]
     transaction.ignoreschedules = true if options[:ignoreschedules]
@@ -140,7 +142,12 @@ class Puppet::Resource::Catalog < Puppet::SimpleGraph
     transaction.add_times :config_retrieval => self.retrieval_duration || 0
 
     begin
-      transaction.evaluate
+      Puppet::Util::Log.newdestination(transaction.report) if register_report
+      begin
+        transaction.evaluate
+      ensure
+        Puppet::Util::Log.close(transaction.report) if register_report
+      end
     rescue Puppet::Error => detail
       puts detail.backtrace if Puppet[:trace]
       Puppet.err "Could not apply complete catalog: #{detail}"
diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb
index 16f201ec5..d6d50d410 100644
--- a/lib/puppet/transaction.rb
+++ b/lib/puppet/transaction.rb
@@ -123,31 +123,23 @@ class Puppet::Transaction
   # collects all of the changes, executes them, and responds to any
   # necessary events.
   def evaluate
-    # Start logging.
-    Puppet::Util::Log.newdestination(@report)
-
     prepare
 
     Puppet.info "Applying configuration version '#{catalog.version}'" if catalog.version
 
-    begin
-      @sorted_resources.each do |resource|
-        next if stop_processing?
-        if resource.is_a?(Puppet::Type::Component)
-          Puppet.warning "Somehow left a component in the relationship graph"
-          next
-        end
-        ret = nil
-        seconds = thinmark do
-          ret = eval_resource(resource)
-        end
-
-        resource.info "Evaluated in %0.2f seconds" % seconds if Puppet[:evaltrace] and @catalog.host_config?
-        ret
+    @sorted_resources.each do |resource|
+      next if stop_processing?
+      if resource.is_a?(Puppet::Type::Component)
+        Puppet.warning "Somehow left a component in the relationship graph"
+        next
+      end
+      ret = nil
+      seconds = thinmark do
+        ret = eval_resource(resource)
       end
-    ensure
-      # And then close the transaction log.
-      Puppet::Util::Log.close(@report)
+
+      resource.info "valuated in %0.2f seconds" % seconds if Puppet[:evaltrace] and @catalog.host_config?
+      ret
     end
 
     Puppet.debug "Finishing transaction #{object_id}"
diff --git a/lib/puppet/transaction/report.rb b/lib/puppet/transaction/report.rb
index 841c56968..77b9da833 100644
--- a/lib/puppet/transaction/report.rb
+++ b/lib/puppet/transaction/report.rb
@@ -10,8 +10,8 @@ class Puppet::Transaction::Report
 
   indirects :report, :terminus_class => :processor
 
-  attr_accessor :configuration_version
-  attr_reader :resource_statuses, :logs, :metrics, :host, :time, :kind, :status
+  attr_accessor :configuration_version, :host
+  attr_reader :resource_statuses, :logs, :metrics, :time, :kind, :status
 
   # This is necessary since Marshall doesn't know how to
   # dump hash with default proc (see below @records)