From 0ca27b6f30ccd327505bd3a44bd319fb3eba956b Mon Sep 17 00:00:00 2001 From: Lamont Granquist Date: Mon, 4 Apr 2016 10:45:47 -0700 Subject: Make notifications recursive. This is similar to poise's approach but has a few differences. Similarly to poise, the base behavior of notifications and find() and lookup() on the resource collection is changed to be 'recursive' and to search in outer contexts for resources and will return them by default. There are find_local() and lookup_local() methods added to allow for bypassing the recursion and making sure to throw exceptions if the current run_context does not have any matching resources. The CHEF-3694 resource cloning code has been modified to call the lookup_local() API and not to be recursive because we believe that nobody in their right mind would want that behavior (and resource cloning should eventually be removed). So the behavior of resource cloning should remain unchanged. The behavior of delayed notifications to resources outside of the current run_context is slightly different than what Poise has been implementing. The delayed notification will run in the run_context of the resource that is being notified. I think Poise tends to bubble up to the nextmost wrapping resource context (as opposed to Poise's subcontext_block or notifying_block contexts). This code I think is conceptually simpler to reason about, and I think it gets the use case right where if you're notifying a service resource in the outermost run_context from within multiple wrapping resources that it correctly bubbles out to the outermost run context and will notify with all the other delayed notifications at the end of the chef client run. Another useful feature of the delayed notification behavior is that if we do implement notifying_block or subcontext_block that each block can get its own delayed notification run and any resources that are inside of that block can run in the delayed notification phase of that block (while still being able to notify resources outside of the block and having those delayed notifications run in the receiving resources run_context). This will let us implement an often-requested feature for having "notifications delayed to the end of a block/recipe" instead of having to do all notifications absolutely immediately or delayed to the end of the chef run. This code also cleans up the object model a little bit. All of the state about notification collection is now hanging off of the run_context -- the delayed_actions have been moved from the Chef::Runner to the Chef::RunContext. Hanging it off of the Chef::Runner would have been very difficult to 'target' from other run_context's without adding a pointer back from the RunContext to the Runner and that feels like the wrong object model. The RunContext is now responsible for all of its notification state, while the Runner is responsible for wiring up the notifications across different run_contexts. Note that it will not be possible to send a notification to a run_context which has already been converged. That seems to make sense to me and the search API on the resource collection does not support returning resources from run_contexts that are children, only parents (and we don't actually hold onto pointers to child run_contexts and they may be garbage collected). --- lib/chef/resource_builder.rb | 2 +- lib/chef/resource_collection.rb | 50 ++++- lib/chef/run_context.rb | 26 ++- lib/chef/runner.rb | 40 ++-- spec/integration/recipes/notifies_spec.rb | 334 ++++++++++++++++++++++++++++++ spec/unit/resource_collection_spec.rb | 70 +++++++ 6 files changed, 497 insertions(+), 25 deletions(-) create mode 100644 spec/integration/recipes/notifies_spec.rb 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 # @@ -220,6 +230,18 @@ class Chef end 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. # @@ -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", < 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", < 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", < 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", < 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", < 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", < 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", < 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", < 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) -- cgit v1.2.1