Merge pull request #4741 from chef/lcg/notify-scopes

Notifications from LWRPS/sub-resources can trigger resources in outer run_context scopes

7 files changed, 500 insertions, 28 deletions

diff --git a/.travis.yml b/.travis.yml
index b3321813a6..e93d6ce246 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -149,11 +149,11 @@ matrix:
       - sudo cat /var/log/squid3/access.log
 
   allow_failures:
+    - rvm: 2.2
+      env: "GEMFILE_MOD=\"gem 'poise', github: 'poise/poise'\""
+      script: bundle exec rake poise_spec
     - rvm: 2.3.0
     - rvm: rbx
-    - rvm: 2.2
-      env: "GEMFILE_MOD=\"gem 'halite', github: 'poise/halite'\""
-      script: bundle exec rake halite_spec
 
 notifications:
   on_change: true
diff --git a/lib/chef/resource_builder.rb b/lib/chef/resource_builder.rb
index f3ca2e95ad..138e401d5c 100644
--- a/lib/chef/resource_builder.rb
+++ b/lib/chef/resource_builder.rb
@@ -137,7 +137,7 @@ class Chef
       @prior_resource ||=
         begin
           key = "#{type}[#{name}]"
-          run_context.resource_collection.lookup(key)
+          run_context.resource_collection.lookup_local(key)
         rescue Chef::Exceptions::ResourceNotFound
           nil
         end
diff --git a/lib/chef/resource_collection.rb b/lib/chef/resource_collection.rb
index 6c5b4289d8..1429c25d7f 100644
--- a/lib/chef/resource_collection.rb
+++ b/lib/chef/resource_collection.rb
@@ -33,9 +33,12 @@ class Chef
     extend Forwardable
 
     attr_reader :resource_set, :resource_list
-    private :resource_set, :resource_list
+    attr_accessor :run_context
 
-    def initialize
+    protected :resource_set, :resource_list
+
+    def initialize(run_context = nil)
+      @run_context = run_context
       @resource_set = ResourceSet.new
       @resource_list = ResourceList.new
     end
@@ -79,11 +82,50 @@ class Chef
 
     resource_list_methods = Enumerable.instance_methods +
       [:iterator, :all_resources, :[], :each, :execute_each_resource, :each_index, :empty?] -
-      [:find] # find needs to run on the set
-    resource_set_methods = [:lookup, :find, :resources, :keys, :validate_lookup_spec!]
+      [:find] # find overridden below
+    resource_set_methods = [:resources, :keys, :validate_lookup_spec!]
 
     def_delegators :resource_list, *resource_list_methods
     def_delegators :resource_set, *resource_set_methods
 
+    def lookup_local(key)
+      resource_set.lookup(key)
+    end
+
+    def find_local(*args)
+      resource_set.find(*args)
+    end
+
+    def lookup(key)
+      if run_context.nil?
+        lookup_local(key)
+      else
+        lookup_recursive(run_context, key)
+      end
+    end
+
+    def find(*args)
+      if run_context.nil?
+        find_local(*args)
+      else
+        find_recursive(run_context, *args)
+      end
+    end
+
+    private
+
+    def lookup_recursive(rc, key)
+      rc.resource_collection.resource_set.lookup(key)
+    rescue Chef::Exceptions::ResourceNotFound
+      raise if rc.parent_run_context.nil?
+      lookup_recursive(rc.parent_run_context, key)
+    end
+
+    def find_recursive(rc, *args)
+      rc.resource_collection.resource_set.find(*args)
+    rescue Chef::Exceptions::ResourceNotFound
+      raise if rc.parent_run_context.nil?
+      find_recursive(rc.parent_run_context, *args)
+    end
   end
 end
diff --git a/lib/chef/run_context.rb b/lib/chef/run_context.rb
index cb3338d3de..29c936a932 100644
--- a/lib/chef/run_context.rb
+++ b/lib/chef/run_context.rb
@@ -130,6 +130,14 @@ class Chef
     #
     attr_reader :delayed_notification_collection
 
+    #
+    # An Array containing the delayed (end of run) notifications triggered by
+    # resources during the converge phase of the chef run.
+    #
+    # @return [Array[Chef::Resource::Notification]] An array of notification objects
+    #
+    attr_reader :delayed_actions
+
     # Creates a new Chef::RunContext object and populates its fields. This object gets
     # used by the Chef Server to generate a fully compiled recipe list for a node.
     #
@@ -152,6 +160,7 @@ class Chef
       @loaded_attributes_hash = {}
       @reboot_info = {}
       @cookbook_compiler = nil
+      @delayed_actions = []
 
       initialize_child_state
     end
@@ -172,10 +181,11 @@ class Chef
     #
     def initialize_child_state
       @audits = {}
-      @resource_collection = Chef::ResourceCollection.new
+      @resource_collection = Chef::ResourceCollection.new(self)
       @before_notification_collection = Hash.new { |h, k| h[k] = [] }
       @immediate_notification_collection = Hash.new { |h, k| h[k] = [] }
       @delayed_notification_collection = Hash.new { |h, k| h[k] = [] }
+      @delayed_actions = []
     end
 
     #
@@ -221,6 +231,18 @@ class Chef
     end
 
     #
+    # Adds a delayed action to the +delayed_actions+.
+    #
+    def add_delayed_action(notification)
+      if delayed_actions.any? { |existing_notification| existing_notification.duplicates?(notification) }
+        Chef::Log.info( "#{notification.notifying_resource} not queuing delayed action #{notification.action} on #{notification.resource}"\
+                       " (delayed), as it's already been queued")
+      else
+        delayed_actions << notification
+      end
+    end
+
+    #
     # Get the list of before notifications sent by the given resource.
     #
     # TODO seriously, this is actually wrong.  resource.name is not unique,
@@ -640,6 +662,8 @@ ERROR_MESSAGE
         audits
         audits=
         create_child
+        add_delayed_action
+        delayed_actions
         delayed_notification_collection
         delayed_notification_collection=
         delayed_notifications
diff --git a/lib/chef/runner.rb b/lib/chef/runner.rb
index ce128203f2..cd5615bcec 100644
--- a/lib/chef/runner.rb
+++ b/lib/chef/runner.rb
@@ -30,13 +30,14 @@ class Chef
 
     attr_reader :run_context
 
-    attr_reader :delayed_actions
-
     include Chef::Mixin::ParamsValidate
 
     def initialize(run_context)
-      @run_context      = run_context
-      @delayed_actions  = []
+      @run_context = run_context
+    end
+
+    def delayed_actions
+      @run_context.delayed_actions
     end
 
     def events
@@ -48,16 +49,11 @@ class Chef
     def run_action(resource, action, notification_type = nil, notifying_resource = nil)
       # If there are any before notifications, why-run the resource
       # and notify anyone who needs notifying
-      # TODO cheffish has a bug where it passes itself instead of the run_context to us, so doesn't have before_notifications. Fix there, update dependency requirement, and remove this if statement.
-      before_notifications = run_context.before_notifications(resource) if run_context.respond_to?(:before_notifications)
-      if before_notifications && !before_notifications.empty?
-        whyrun_before = Chef::Config[:why_run]
-        begin
-          Chef::Config[:why_run] = true
+      before_notifications = run_context.before_notifications(resource) || []
+      unless before_notifications.empty?
+        forced_why_run do
           Chef::Log.info("#{resource} running why-run #{action} action to support before action")
           resource.run_action(action, notification_type, notifying_resource)
-        ensure
-          Chef::Config[:why_run] = whyrun_before
         end
 
         if resource.updated_by_last_action?
@@ -65,8 +61,8 @@ class Chef
             Chef::Log.info("#{resource} sending #{notification.action} action to #{notification.resource} (before)")
             run_action(notification.resource, notification.action, :before, resource)
           end
+          resource.updated_by_last_action(false)
         end
-
       end
 
       # Actually run the action for realsies
@@ -82,12 +78,8 @@ class Chef
         end
 
         run_context.delayed_notifications(resource).each do |notification|
-          if delayed_actions.any? { |existing_notification| existing_notification.duplicates?(notification) }
-            Chef::Log.info( "#{resource} not queuing delayed action #{notification.action} on #{notification.resource}"\
-                            " (delayed), as it's already been queued")
-          else
-            delayed_actions << notification
-          end
+          # send the notification to the run_context of the receiving resource
+          notification.resource.run_context.add_delayed_action(notification)
         end
       end
     end
@@ -137,5 +129,15 @@ class Chef
     rescue Exception => e
       e
     end
+
+    # helper to run a block of code with why_run forced to true and then restore it correctly
+    def forced_why_run
+      saved = Chef::Config[:why_run]
+      Chef::Config[:why_run] = true
+      yield
+    ensure
+      Chef::Config[:why_run] = saved
+    end
+
   end
 end
diff --git a/spec/integration/recipes/notifies_spec.rb b/spec/integration/recipes/notifies_spec.rb
new file mode 100644
index 0000000000..000f5e37bf
--- /dev/null
+++ b/spec/integration/recipes/notifies_spec.rb
@@ -0,0 +1,334 @@
+require "support/shared/integration/integration_helper"
+require "chef/mixin/shell_out"
+
+describe "notifications" do
+  include IntegrationSupport
+  include Chef::Mixin::ShellOut
+
+  let(:chef_dir) { File.expand_path("../../../../bin", __FILE__) }
+  let(:chef_client) { "ruby '#{chef_dir}/chef-client' --minimal-ohai" }
+
+  when_the_repository "notifies delayed one" do
+    before do
+      directory "cookbooks/x" do
+
+        file "resources/notifying_test.rb", <<EOM
+default_action :run
+provides :notifying_test
+resource_name :notifying_test
+
+action :run do
+  log "bar" do
+    notifies :write, 'log[foo]', :delayed
+  end
+end
+EOM
+
+        file "recipes/default.rb", <<EOM
+log "foo" do
+  action :nothing
+end
+notifying_test "whatever"
+log "baz"
+EOM
+
+      end
+    end
+
+    it "should complete with success" do
+      file "config/client.rb", <<EOM
+local_mode true
+cookbook_path "#{path_to('cookbooks')}"
+log_level :warn
+EOM
+
+      result = shell_out("#{chef_client} -c \"#{path_to('config/client.rb')}\" --no-color -F doc -o 'x::default'", :cwd => chef_dir)
+      # our delayed notification should run at the end of the parent run_context after the baz resource
+      expect(result.stdout).to match(/\* log\[bar\] action write\s+\* log\[baz\] action write\s+\* log\[foo\] action write/)
+      result.error!
+    end
+  end
+
+  when_the_repository "notifies delayed two" do
+    before do
+      directory "cookbooks/x" do
+
+        file "resources/notifying_test.rb", <<EOM
+default_action :run
+provides :notifying_test
+resource_name :notifying_test
+
+action :run do
+  log "bar" do
+    notifies :write, 'log[foo]', :delayed
+  end
+end
+EOM
+
+        file "recipes/default.rb", <<EOM
+log "foo" do
+  action :nothing
+end
+notifying_test "whatever"
+log "baz" do
+  notifies :write, 'log[foo]', :delayed
+end
+EOM
+
+      end
+    end
+
+    it "should complete with success" do
+      file "config/client.rb", <<EOM
+local_mode true
+cookbook_path "#{path_to('cookbooks')}"
+log_level :warn
+EOM
+
+      result = shell_out("#{chef_client} -c \"#{path_to('config/client.rb')}\" --no-color -F doc -o 'x::default'", :cwd => chef_dir)
+      # our delayed notification should run at the end of the parent run_context after the baz resource
+      expect(result.stdout).to match(/\* log\[bar\] action write\s+\* log\[baz\] action write\s+\* log\[foo\] action write/)
+      # and only run once
+      expect(result.stdout).not_to match(/\* log\[foo\] action write.*\* log\[foo\] action write/)
+      result.error!
+    end
+  end
+
+  when_the_repository "notifies delayed three" do
+    before do
+      directory "cookbooks/x" do
+
+        file "resources/notifying_test.rb", <<EOM
+default_action :run
+provides :notifying_test
+resource_name :notifying_test
+
+action :run do
+  log "bar" do
+    notifies :write, 'log[foo]', :delayed
+  end
+end
+EOM
+
+        file "recipes/default.rb", <<EOM
+log "foo" do
+  action :nothing
+end
+log "quux" do
+  notifies :write, 'log[foo]', :delayed
+  notifies :write, 'log[baz]', :delayed
+end
+notifying_test "whatever"
+log "baz"
+EOM
+
+      end
+    end
+
+    it "should complete with success" do
+      file "config/client.rb", <<EOM
+local_mode true
+cookbook_path "#{path_to('cookbooks')}"
+log_level :warn
+EOM
+
+      result = shell_out("#{chef_client} -c \"#{path_to('config/client.rb')}\" --no-color -F doc -o 'x::default'", :cwd => chef_dir)
+      # the delayed notification from the sub-resource is de-duplicated by the notification already in the parent run_context
+      expect(result.stdout).to match(/\* log\[quux\] action write\s+\* notifying_test\[whatever\] action run\s+\* log\[bar\] action write\s+\* log\[baz\] action write\s+\* log\[foo\] action write\s+\* log\[baz\] action write/)
+      # and only run once
+      expect(result.stdout).not_to match(/\* log\[foo\] action write.*\* log\[foo\] action write/)
+      result.error!
+    end
+  end
+
+  when_the_repository "notifies delayed four" do
+    before do
+      directory "cookbooks/x" do
+        file "recipes/default.rb", <<EOM
+log "foo" do
+  action :nothing
+end
+log "bar" do
+  notifies :write, 'log[foo]', :delayed
+end
+log "baz" do
+  notifies :write, 'log[foo]', :delayed
+end
+EOM
+
+      end
+    end
+
+    it "should complete with success" do
+      file "config/client.rb", <<EOM
+local_mode true
+cookbook_path "#{path_to('cookbooks')}"
+log_level :warn
+EOM
+
+      result = shell_out("#{chef_client} -c \"#{path_to('config/client.rb')}\" --no-color -F doc -o 'x::default'", :cwd => chef_dir)
+      # the delayed notification from the sub-resource is de-duplicated by the notification already in the parent run_context
+      expect(result.stdout).to match(/\* log\[bar\] action write\s+\* log\[baz\] action write\s+\* log\[foo\] action write/)
+      # and only run once
+      expect(result.stdout).not_to match(/\* log\[foo\] action write.*\* log\[foo\] action write/)
+      result.error!
+    end
+  end
+
+  when_the_repository "notifies immediately" do
+    before do
+      directory "cookbooks/x" do
+
+        file "resources/notifying_test.rb", <<EOM
+default_action :run
+provides :notifying_test
+resource_name :notifying_test
+
+action :run do
+  log "bar" do
+    notifies :write, 'log[foo]', :immediately
+  end
+end
+EOM
+
+        file "recipes/default.rb", <<EOM
+log "foo" do
+  action :nothing
+end
+notifying_test "whatever"
+log "baz"
+EOM
+
+      end
+    end
+
+    it "should complete with success" do
+      file "config/client.rb", <<EOM
+local_mode true
+cookbook_path "#{path_to('cookbooks')}"
+log_level :warn
+EOM
+
+      result = shell_out("#{chef_client} -c \"#{path_to('config/client.rb')}\" --no-color -F doc -o 'x::default'", :cwd => chef_dir)
+      expect(result.stdout).to match(/\* log\[bar\] action write\s+\* log\[foo\] action write\s+\* log\[baz\] action write/)
+      result.error!
+    end
+  end
+
+  when_the_repository "uses old notifies syntax" do
+    before do
+      directory "cookbooks/x" do
+
+        file "resources/notifying_test.rb", <<EOM
+default_action :run
+provides :notifying_test
+resource_name :notifying_test
+
+action :run do
+  log "bar" do
+    notifies :write, resources(log: "foo"), :immediately
+  end
+end
+EOM
+
+        file "recipes/default.rb", <<EOM
+log "foo" do
+  action :nothing
+end
+notifying_test "whatever"
+log "baz"
+EOM
+
+      end
+    end
+
+    it "should complete with success" do
+      file "config/client.rb", <<EOM
+local_mode true
+cookbook_path "#{path_to('cookbooks')}"
+log_level :warn
+EOM
+
+      result = shell_out("#{chef_client} -c \"#{path_to('config/client.rb')}\" --no-color -F doc -o 'x::default'", :cwd => chef_dir)
+      expect(result.stdout).to match(/\* log\[bar\] action write\s+\* log\[foo\] action write\s+\* log\[baz\] action write/)
+      result.error!
+    end
+  end
+
+  when_the_repository "does not have a matching resource" do
+    before do
+      directory "cookbooks/x" do
+
+        file "resources/notifying_test.rb", <<EOM
+default_action :run
+provides :notifying_test
+resource_name :notifying_test
+
+action :run do
+  log "bar" do
+    notifies :write, "log[foo]"
+  end
+end
+EOM
+
+        file "recipes/default.rb", <<EOM
+notifying_test "whatever"
+log "baz"
+EOM
+
+      end
+    end
+
+    it "should complete with success" do
+      file "config/client.rb", <<EOM
+local_mode true
+cookbook_path "#{path_to('cookbooks')}"
+log_level :warn
+EOM
+
+      result = shell_out("#{chef_client} -c \"#{path_to('config/client.rb')}\" --no-color -F doc -o 'x::default'", :cwd => chef_dir)
+      expect(result.stdout).to match(/Chef::Exceptions::ResourceNotFound/)
+      expect(result.exitstatus).not_to eql(0)
+    end
+  end
+
+  when_the_repository "encounters identical resources in parent and child resource collections" do
+    before do
+      directory "cookbooks/x" do
+
+        file "resources/cloning_test.rb", <<EOM
+default_action :run
+provides :cloning_test
+resource_name :cloning_test
+
+action :run do
+  log "bar" do
+    level :info
+  end
+end
+EOM
+
+        file "recipes/default.rb", <<EOM
+log "bar" do
+  level :warn
+end
+
+cloning_test "whatever"
+EOM
+
+      end
+    end
+
+    it "should complete with success" do
+      file "config/client.rb", <<EOM
+local_mode true
+cookbook_path "#{path_to('cookbooks')}"
+log_level :warn
+EOM
+
+      result = shell_out("#{chef_client} -c \"#{path_to('config/client.rb')}\" --no-color -F doc -o 'x::default'", :cwd => chef_dir)
+      expect(result.stdout).not_to match(/CHEF-3694/)
+      result.error!
+    end
+  end
+end
diff --git a/spec/unit/resource_collection_spec.rb b/spec/unit/resource_collection_spec.rb
index a6ad895c51..256ae090e0 100644
--- a/spec/unit/resource_collection_spec.rb
+++ b/spec/unit/resource_collection_spec.rb
@@ -283,6 +283,76 @@ describe Chef::ResourceCollection do
     end
   end
 
+  describe "multiple run_contexts" do
+    let(:node) { Chef::Node.new }
+    let(:parent_run_context) { Chef::RunContext.new(node, {}, nil) }
+    let(:parent_resource_collection) { parent_run_context.resource_collection }
+    let(:child_run_context) { parent_run_context.create_child }
+    let(:child_resource_collection) { child_run_context.resource_collection }
+
+    it "should find resources in the parent run_context with lookup" do
+      zmr = Chef::Resource::ZenMaster.new("dog")
+      parent_resource_collection << zmr
+      expect(child_resource_collection.lookup(zmr.to_s)).to eql(zmr)
+    end
+
+    it "should not find resources in the parent run_context with lookup_local" do
+      zmr = Chef::Resource::ZenMaster.new("dog")
+      parent_resource_collection << zmr
+      expect { child_resource_collection.lookup_local(zmr.to_s) }.to raise_error(Chef::Exceptions::ResourceNotFound)
+    end
+
+    it "should find resources in the child run_context with lookup_local" do
+      zmr = Chef::Resource::ZenMaster.new("dog")
+      child_resource_collection << zmr
+      expect(child_resource_collection.lookup_local(zmr.to_s)).to eql(zmr)
+    end
+
+    it "should find resources in the parent run_context with find" do
+      zmr = Chef::Resource::ZenMaster.new("dog")
+      parent_resource_collection << zmr
+      expect(child_resource_collection.find(zmr.to_s)).to eql(zmr)
+    end
+
+    it "should not find resources in the parent run_context with find_local" do
+      zmr = Chef::Resource::ZenMaster.new("dog")
+      parent_resource_collection << zmr
+      expect { child_resource_collection.find_local(zmr.to_s) }.to raise_error(Chef::Exceptions::ResourceNotFound)
+    end
+
+    it "should find resources in the child run_context with find_local" do
+      zmr = Chef::Resource::ZenMaster.new("dog")
+      child_resource_collection << zmr
+      expect(child_resource_collection.find_local(zmr.to_s)).to eql(zmr)
+    end
+
+    it "should not find resources in the child run_context in any way from the parent" do
+      zmr = Chef::Resource::ZenMaster.new("dog")
+      child_resource_collection << zmr
+      expect { parent_resource_collection.find_local(zmr.to_s) }.to raise_error(Chef::Exceptions::ResourceNotFound)
+      expect { parent_resource_collection.find(zmr.to_s) }.to raise_error(Chef::Exceptions::ResourceNotFound)
+      expect { parent_resource_collection.lookup_local(zmr.to_s) }.to raise_error(Chef::Exceptions::ResourceNotFound)
+      expect { parent_resource_collection.lookup(zmr.to_s) }.to raise_error(Chef::Exceptions::ResourceNotFound)
+    end
+
+    it "should behave correctly when there is an identically named resource in the child and parent" do
+      a = Chef::Resource::File.new("something")
+      a.content("foo")
+      parent_resource_collection << a
+      b = Chef::Resource::File.new("something")
+      b.content("bar")
+      child_resource_collection << b
+      expect(child_resource_collection.find_local("file[something]").content).to eql("bar")
+      expect(child_resource_collection.find("file[something]").content).to eql("bar")
+      expect(child_resource_collection.lookup_local("file[something]").content).to eql("bar")
+      expect(child_resource_collection.lookup("file[something]").content).to eql("bar")
+      expect(parent_resource_collection.find_local("file[something]").content).to eql("foo")
+      expect(parent_resource_collection.find("file[something]").content).to eql("foo")
+      expect(parent_resource_collection.lookup_local("file[something]").content).to eql("foo")
+      expect(parent_resource_collection.lookup("file[something]").content).to eql("foo")
+    end
+  end
+
   def check_by_names(results, *names)
     names.each do |res_name|
       expect(results.detect { |res| res.name == res_name }).not_to eql(nil)