Expand data_collector resource list to include all resourcesadamleff/IPO-196/add-unprocessed-resources-to-data-collector

Historically when a Chef run fails, the event handlers only report
on resources that have been processed during the run. This change
allows the Data Collector to report those resources in the resource
collection that have not yet been processed so users can better
ascertain how much of their run was completed, and specifically
what resources were not processed as a result of the Chef failure.

Instead of building up a list of resources as they are processed,
this change instead creates a resource report for each resource+action
in the resource collection and then modifies each of those reports
once the resource is processed.

7 files changed, 153 insertions, 53 deletions

diff --git a/lib/chef/client.rb b/lib/chef/client.rb
index c857da1b93..3d78cae2ca 100644
--- a/lib/chef/client.rb
+++ b/lib/chef/client.rb
@@ -278,6 +278,7 @@ class Chef
         do_windows_admin_check
 
         run_context = setup_run_context
+        events.run_context_setup(run_context)
 
         if Chef::Config[:audit_mode] != :audit_only
           converge_error = converge_and_save(run_context)
diff --git a/lib/chef/data_collector.rb b/lib/chef/data_collector.rb
index c8dd354f60..60846b488d 100644
--- a/lib/chef/data_collector.rb
+++ b/lib/chef/data_collector.rb
@@ -51,12 +51,12 @@ class Chef
     # and exports its data through a webhook-like mechanism to a configured
     # endpoint.
     class Reporter < EventDispatch::Base
-      attr_reader :completed_resources, :status, :exception, :error_descriptions,
+      attr_reader :all_resource_reports, :status, :exception, :error_descriptions,
                   :expanded_run_list, :run_status, :http,
                   :current_resource_report, :enabled
 
       def initialize
-        @completed_resources     = []
+        @all_resource_reports    = []
         @current_resource_loaded = nil
         @error_descriptions      = {}
         @expanded_run_list       = {}
@@ -64,6 +64,19 @@ class Chef
         @enabled                 = true
       end
 
+      # see EventDispatch::Base#run_context_setup
+      # Upon receipt, we will create a resource report for every
+      # resource in the resource collection. This is needed in
+      # order to send ALL resources to the Data Collector server
+      # regardless of whether or not they were processed.
+      def run_context_setup(run_context)
+        run_context.resource_collection.all_resources.each do |resource|
+          Array(resource.action).each do |action|
+            create_resource_report(resource, action)
+          end
+        end
+      end
+
       # see EventDispatch::Base#run_started
       # Upon receipt, we will send our run start message to the
       # configured DataCollector endpoint. Depending on whether
@@ -100,13 +113,9 @@ class Chef
       # resource, and we only care about tracking top-level resources.
       def resource_current_state_loaded(new_resource, action, current_resource)
         return if nested_resource?(new_resource)
-        update_current_resource_report(
-          Chef::DataCollector::ResourceReport.new(
-            new_resource,
-            action,
-            current_resource
-          )
-        )
+        resource_report = find_or_create_resource_report(new_resource, action)
+        resource_report.current_resource = current_resource
+        update_current_resource_report(resource_report)
       end
 
       # see EventDispatch::Base#resource_up_to_date
@@ -116,19 +125,15 @@ class Chef
       end
 
       # see EventDispatch::Base#resource_skipped
-      # If this is a top-level resource, we create a ResourceReport instance
-      # (because a skipped resource does not trigger the
+      # If this is a top-level resource, we find the correct ResourceReport
+      # instance (because a skipped resource does not trigger the
       # resource_current_state_loaded event), and flag it as skipped.
       def resource_skipped(new_resource, action, conditional)
         return if nested_resource?(new_resource)
 
-        update_current_resource_report(
-          Chef::DataCollector::ResourceReport.new(
-            new_resource,
-            action
-          )
-        )
-        current_resource_report.skipped(conditional)
+        resource_report = find_or_create_resource_report(new_resource, action)
+        resource_report.skipped(conditional)
+        update_current_resource_report(resource_report)
       end
 
       # see EventDispatch::Base#resource_updated
@@ -154,13 +159,11 @@ class Chef
       end
 
       # see EventDispatch::Base#resource_completed
-      # Mark the ResourceReport instance as finished (for timing details)
-      # and add it to the list of resources encountered during this run.
+      # Mark the ResourceReport instance as finished (for timing details).
       # This marks the end of this resource during this run.
       def resource_completed(new_resource)
         if current_resource_report && !nested_resource?(new_resource)
           current_resource_report.finish
-          add_completed_resource(current_resource_report)
           update_current_resource_report(nil)
         end
       end
@@ -271,7 +274,7 @@ class Chef
           Chef::DataCollector::Messages.run_end_message(
             run_status: run_status,
             expanded_run_list: expanded_run_list,
-            completed_resources: completed_resources,
+            resources: all_resource_reports,
             status: opts[:status],
             error_descriptions: error_descriptions
           ).to_json
@@ -321,6 +324,32 @@ class Chef
         @error_descriptions = discription_hash
       end
 
+      def create_resource_report(resource, action)
+        report = Chef::DataCollector::ResourceReport.new(
+          resource,
+          action
+        )
+
+        @all_resource_reports << report
+
+        report
+      end
+
+      def find_or_create_resource_report(resource, action)
+        report = all_resource_reports.find do |r|
+          r.new_resource.class == resource.class &&
+            r.new_resource.name == resource.name &&
+            r.new_resource.source_line == resource.source_line &&
+            r.new_resource.action.include?(action)
+        end
+
+        if report.nil?
+          report = create_resource_report(resource, action)
+        end
+
+        report
+      end
+
       # If we are getting messages about a resource while we are in the middle of
       # another resource's update, we assume that the nested resource is just the
       # implementation of a provider, and we want to hide it from the reporting
diff --git a/lib/chef/data_collector/messages.rb b/lib/chef/data_collector/messages.rb
index e23262c9e4..15b451a6b0 100644
--- a/lib/chef/data_collector/messages.rb
+++ b/lib/chef/data_collector/messages.rb
@@ -70,15 +70,15 @@ class Chef
           "message_type"           => "run_converge",
           "node_name"              => run_status.node.name,
           "organization_name"      => organization,
-          "resources"              => reporter_data[:completed_resources].map(&:for_json),
+          "resources"              => reporter_data[:resources].map(&:for_json),
           "run_id"                 => run_status.run_id,
           "run_list"               => run_status.node.run_list.for_json,
           "start_time"             => run_status.start_time.utc.iso8601,
           "end_time"               => run_status.end_time.utc.iso8601,
           "source"                 => collector_source,
           "status"                 => reporter_data[:status],
-          "total_resource_count"   => reporter_data[:completed_resources].count,
-          "updated_resource_count" => reporter_data[:completed_resources].select { |r| r.status == "updated" }.count,
+          "total_resource_count"   => reporter_data[:resources].count,
+          "updated_resource_count" => reporter_data[:resources].select { |r| r.status == "updated" }.count,
         }
 
         message["error"] = {
diff --git a/lib/chef/data_collector/resource_report.rb b/lib/chef/data_collector/resource_report.rb
index 1793fe2c9d..f3e272b0fb 100644
--- a/lib/chef/data_collector/resource_report.rb
+++ b/lib/chef/data_collector/resource_report.rb
@@ -22,13 +22,14 @@ class Chef
   class DataCollector
     class ResourceReport
 
-      attr_reader :action, :current_resource, :elapsed_time, :new_resource, :status
-      attr_accessor :conditional, :exception
+      attr_reader :action, :elapsed_time, :new_resource, :status
+      attr_accessor :conditional, :current_resource, :exception
 
       def initialize(new_resource, action, current_resource = nil)
         @new_resource     = new_resource
         @action           = action
         @current_resource = current_resource
+        @status           = "unprocessed"
       end
 
       def skipped(conditional)
@@ -54,6 +55,10 @@ class Chef
         @elapsed_time = new_resource.elapsed_time
       end
 
+      def elapsed_time_in_milliseconds
+        elapsed_time.nil? ? nil : (elapsed_time * 1000).to_i
+      end
+
       def to_hash
         hash = {
           "type"     => new_resource.resource_name.to_sym,
@@ -61,7 +66,7 @@ class Chef
           "id"       => new_resource.identity.to_s,
           "after"    => new_resource.state_for_resource_reporter,
           "before"   => current_resource ? current_resource.state_for_resource_reporter : {},
-          "duration" => (elapsed_time * 1000).to_i.to_s,
+          "duration" => elapsed_time_in_milliseconds.to_s,
           "delta"    => new_resource.respond_to?(:diff) ? new_resource.diff : "",
           "result"   => action.to_s,
           "status"   => status,
diff --git a/lib/chef/event_dispatch/base.rb b/lib/chef/event_dispatch/base.rb
index 6c6b2fa3bb..0b386ab851 100644
--- a/lib/chef/event_dispatch/base.rb
+++ b/lib/chef/event_dispatch/base.rb
@@ -43,6 +43,12 @@ class Chef
       def run_failed(exception)
       end
 
+      # Called when the run_context has been set up.
+      # Most helpful for event handlers that wish to inspect
+      # the resource collection.
+      def run_context_setup(run_context)
+      end
+
       # Called right after ohai runs.
       def ohai_completed(node)
       end
diff --git a/spec/unit/data_collector/messages_spec.rb b/spec/unit/data_collector/messages_spec.rb
index dee86a3907..bb3b158611 100644
--- a/spec/unit/data_collector/messages_spec.rb
+++ b/spec/unit/data_collector/messages_spec.rb
@@ -67,7 +67,7 @@ describe Chef::DataCollector::Messages do
     let(:reporter_data) do
       {
         run_status: run_status,
-        completed_resources: [resource1, resource2],
+        resources: [resource1, resource2],
       }
     end
 
diff --git a/spec/unit/data_collector_spec.rb b/spec/unit/data_collector_spec.rb
index 78b0aaf97d..fb47fe3012 100644
--- a/spec/unit/data_collector_spec.rb
+++ b/spec/unit/data_collector_spec.rb
@@ -20,6 +20,7 @@
 
 require "spec_helper"
 require "chef/data_collector"
+require "chef/resource_builder"
 
 describe Chef::DataCollector do
   describe ".register_reporter?" do
@@ -155,6 +156,21 @@ describe Chef::DataCollector::Reporter do
   let(:reporter) { described_class.new }
   let(:run_status) { Chef::RunStatus.new(Chef::Node.new, Chef::EventDispatch::Dispatcher.new) }
 
+  describe '#run_context_setup' do
+    it "creates resource reports for each resource in the collection" do
+      resource1           = double("resource1", action: [:create])
+      resource2           = double("resource2", action: [:enable, :start])
+      resource_collection = double("resource_collection", all_resources: [resource1, resource2])
+      run_context         = double("run_context", resource_collection: resource_collection)
+
+      expect(reporter).to receive(:create_resource_report).with(resource1, :create)
+      expect(reporter).to receive(:create_resource_report).with(resource2, :enable)
+      expect(reporter).to receive(:create_resource_report).with(resource2, :start)
+
+      reporter.run_context_setup(run_context)
+    end
+  end
+
   describe '#run_started' do
     before do
       allow(reporter).to receive(:update_run_status)
@@ -197,6 +213,7 @@ describe Chef::DataCollector::Reporter do
     let(:new_resource)     { double("new_resource") }
     let(:action)           { double("action") }
     let(:current_resource) { double("current_resource") }
+    let(:resource_report)  { double("resource_report") }
 
     context "when resource is a nested resource" do
       it "does not update the resource report" do
@@ -207,14 +224,13 @@ describe Chef::DataCollector::Reporter do
     end
 
     context "when resource is not a nested resource" do
-      it "updates the resource report" do
+      it "finds the resource report and stores it as the current one" do
         allow(reporter).to receive(:nested_resource?).and_return(false)
-        expect(Chef::DataCollector::ResourceReport).to receive(:new).with(
-          new_resource,
-          action,
-          current_resource)
-        .and_return("resource_report")
-        expect(reporter).to receive(:update_current_resource_report).with("resource_report")
+        expect(reporter).to receive(:find_or_create_resource_report)
+          .with(new_resource, action)
+          .and_return(resource_report)
+        expect(resource_report).to receive(:current_resource=).with(current_resource)
+        expect(reporter).to receive(:update_current_resource_report).with(resource_report)
         reporter.resource_current_state_loaded(new_resource, action, current_resource)
       end
     end
@@ -256,7 +272,7 @@ describe Chef::DataCollector::Reporter do
 
     before do
       allow(reporter).to receive(:nested_resource?)
-      allow(reporter).to receive(:current_resource_report).and_return(resource_report)
+      allow(reporter).to receive(:find_or_create_resource_report).and_return(resource_report)
       allow(resource_report).to receive(:skipped)
     end
 
@@ -269,13 +285,12 @@ describe Chef::DataCollector::Reporter do
     end
 
     context "when the resource is not a nested resource" do
-      it "updates the resource report" do
+      it "finds the correct resource report and stores it as the current one" do
         allow(reporter).to receive(:nested_resource?).and_return(false)
-        expect(Chef::DataCollector::ResourceReport).to receive(:new).with(
-          new_resource,
-          action)
-        .and_return("resource_report")
-        expect(reporter).to receive(:update_current_resource_report).with("resource_report")
+        expect(reporter).to receive(:find_or_create_resource_report)
+          .with(new_resource, action)
+          .and_return(resource_report)
+        expect(reporter).to receive(:update_current_resource_report).with(resource_report)
         reporter.resource_skipped(new_resource, action, conditional)
       end
 
@@ -349,15 +364,15 @@ describe Chef::DataCollector::Reporter do
     let(:resource_report) { double("resource_report") }
 
     before do
-      allow(reporter).to receive(:add_completed_resource)
       allow(reporter).to receive(:update_current_resource_report)
+      allow(reporter).to receive(:current_resource_report)
       allow(resource_report).to receive(:finish)
     end
 
     context "when there is no current resource report" do
-      it "does not add the updated resource" do
+      it "does not touch the current resource report" do
         allow(reporter).to receive(:current_resource_report).and_return(nil)
-        expect(reporter).not_to receive(:add_completed_resource)
+        expect(reporter).not_to receive(:update_current_resource_report)
         reporter.resource_completed(new_resource)
       end
     end
@@ -368,9 +383,9 @@ describe Chef::DataCollector::Reporter do
       end
 
       context "when the resource is a nested resource" do
-        it "does not add the updated resource" do
+        it "does not mark the resource as finished" do
           allow(reporter).to receive(:nested_resource?).with(new_resource).and_return(true)
-          expect(reporter).not_to receive(:add_completed_resource)
+          expect(resource_report).not_to receive(:finish)
           reporter.resource_completed(new_resource)
         end
       end
@@ -385,11 +400,6 @@ describe Chef::DataCollector::Reporter do
           reporter.resource_completed(new_resource)
         end
 
-        it "adds the resource to the updated resource list" do
-          expect(reporter).to receive(:add_completed_resource).with(resource_report)
-          reporter.resource_completed(new_resource)
-        end
-
         it "nils out the current resource report" do
           expect(reporter).to receive(:update_current_resource_report).with(nil)
           reporter.resource_completed(new_resource)
@@ -499,4 +509,53 @@ describe Chef::DataCollector::Reporter do
       end
     end
   end
+
+  describe '#find_or_create_resource_report' do
+    let(:node) { Chef::Node.new }
+    let(:run_context) { Chef::RunContext.new(node, nil, nil) }
+
+    let(:resource1) do
+      Chef::ResourceBuilder.new(
+        type: :file,
+        name: "test1",
+        run_context: run_context)
+      .build
+    end
+
+    let(:resource2) do
+      Chef::ResourceBuilder.new(
+        type: :file,
+        name: "test2",
+        run_context: run_context)
+      .build { action :delete }
+    end
+
+    let(:resource3) do
+      Chef::ResourceBuilder.new(
+        type: :service,
+        name: "test3",
+        run_context: run_context)
+      .build
+    end
+
+    let(:report1) { Chef::DataCollector::ResourceReport.new(resource1, resource1.action.first) }
+    let(:report2) { Chef::DataCollector::ResourceReport.new(resource2, resource2.action.first) }
+    let(:report3) { Chef::DataCollector::ResourceReport.new(resource3, resource3.action.first) }
+
+    context "when a matching resource report exists" do
+      it "returns the resource report" do
+        allow(reporter).to receive(:all_resource_reports).and_return([report1, report2, report3])
+        expect(reporter).not_to receive(:create_resource_report)
+        expect(reporter.send(:find_or_create_resource_report, resource1, resource1.action.first)).to eq(report1)
+      end
+    end
+
+    context "when a matching resource report does not exist" do
+      it "creates a new resource report and returns it" do
+        allow(reporter).to receive(:all_resource_reports).and_return([report1, report2])
+        expect(reporter).to receive(:create_resource_report).with(resource3, resource3.action.first).and_return("report_for_resource3")
+        expect(reporter.send(:find_or_create_resource_report, resource3, resource3.action.first)).to eq("report_for_resource3")
+      end
+    end
+  end
 end