Merge pull request #5956 from Magisus/PUP-2280_fail-transaction-if-fa…

…iled-to-refresh

(PUP-2280) Mark runs as failed when refresh events fail

lib/puppet/transaction.rb

@@ -197,7 +197,9 @@ def stop_processing?
 
   # Are there any failed resources in this transaction?
   def any_failed?
-    report.resource_statuses.values.detect { |status| status.failed? }
+    report.resource_statuses.values.detect { |status|
+      status.failed? || status.failed_to_restart?
+    }
   end
 
   # Find all of the changed resources.
@@ -245,7 +247,10 @@ def prefetch_if_necessary(resource)
   def apply(resource, ancestor = nil)
     status = resource_harness.evaluate(resource)
     add_resource_status(status)
-    event_manager.queue_events(ancestor || resource, status.events) unless status.failed?
+    ancestor ||= resource
+    if !(status.failed? || status.failed_to_restart?)
+      event_manager.queue_events(ancestor, status.events)
+    end
   rescue => detail
     resource.err _("Could not evaluate: %{detail}") % { detail: detail }
   end
@@ -297,7 +302,7 @@ def propagate_failure(resource)
     relationship_graph.direct_dependencies_of(resource).each do |dep|
       if (s = resource_status(dep))
         failed.merge(s.failed_dependencies) if s.dependency_failed?
-        if s.failed?
+        if s.failed? || s.failed_to_restart?
           failed.add(dep)
         end
       end

lib/puppet/transaction/report.rb

@@ -170,7 +170,9 @@ def add_resource_status(status)
 
   # @api private
   def compute_status(resource_metrics, change_metric)
-    if resources_failed_to_generate || (resource_metrics["failed"] || 0) > 0
+    if resources_failed_to_generate ||
+       (resource_metrics["failed"] || 0) > 0 ||
+       (resource_metrics["failed_to_restart"] || 0) > 0
       'failed'
     elsif change_metric > 0
       'changed'

spec/integration/transaction_spec.rb

@@ -233,7 +233,7 @@ def touch(path)
     expect(Puppet::FileSystem.exist?(path)).not_to be_truthy
   end
 
-  it "should not let one failed refresh result in other refreshes failing" do
+  it "one failed refresh should propagate its failure to dependent refreshes" do
     path = tmpfile("path")
     newfile = tmpfile("file")
       file = Puppet::Type.type(:file).new(
@@ -263,7 +263,7 @@ def touch(path)
 
     catalog = mk_catalog(file, exec1, exec2)
     catalog.apply
-    expect(Puppet::FileSystem.exist?(newfile)).to be_truthy
+    expect(Puppet::FileSystem.exist?(newfile)).to be_falsey
   end
 
   # Ensure when resources have been generated with eval_generate that event

spec/unit/transaction/report_spec.rb

@@ -292,6 +292,12 @@ def add_statuses(count, type = :file)
         expect(@report.status).to eq('failed')
       end
 
+      it "should mark the report as 'failed' if resources failed to restart" do
+        add_statuses(1) { |status| status.failed_to_restart = true }
+        @report.finalize_report
+        expect(@report.status).to eq('failed')
+      end
+
       it "should mark the report as 'failed' if resources_failed_to_generate" do
         @report.resources_failed_to_generate = true
         @report.finalize_report

spec/unit/transaction_spec.rb

@@ -50,7 +50,7 @@ def transaction_with_resource(resource)
     expect(transaction.report.resource_statuses[resource.to_s]).to equal(status)
   end
 
-  it "should not consider there to be failed resources if no statuses are marked failed" do
+  it "should not consider there to be failed or failed_to_restart resources if no statuses are marked failed" do
     resource = Puppet::Type.type(:notify).new :title => "foobar"
     transaction = transaction_with_resource(resource)
     transaction.evaluate
@@ -174,6 +174,13 @@ def transaction_with_resource(resource)
 
       expect(@transaction).to be_any_failed
     end
+
+    it "should report any_failed if any resources failed to restart" do
+      @transaction.evaluate
+      @transaction.report.resource_statuses[@resource.to_s].failed_to_restart = true
+
+      expect(@transaction).to be_any_failed
+    end
   end
 
   describe "#unblock" do