From 5b8cbe0d41762efcd8b3fba12aba4a689eca8fa7 Mon Sep 17 00:00:00 2001 From: Stephan Renatus Date: Thu, 23 Mar 2017 13:53:03 +0100 Subject: Don't `rescue Exception` for retriable resources Without this, sending an interrupt signal (hitting Ctrl+C) during a chef-client run that is currently executing a resources that has `retries` set will not interrupt the run, although the interrupt handler set up in Chef::Application calls Process.exit, which raises SystemExit. If the retryable resource is shelling out, the spawned process also gets the INT signal and will stop -- however, the chef-run would go on, merely noticing that the resource has failed and theres X-1 retries left. It is imaginable that users depend on some (whacky) Ruby script in a retriable resource that raises SystemExit, and they want this to _not_ stop the chef-run. So it's up for debate if we just want to merge this or make it configurable. In general, I'd like Ctrl+C to _stop my chef-run_, but this expectation could also be misled. When adapting tests to this change, it became apparent that for some reason the runner_spec.rb test didn't behave as expected. I've thus made those tests a little stricter by replacing some allows by expects. Signed-off-by: Stephan Renatus --- lib/chef/resource.rb | 2 +- spec/unit/resource_spec.rb | 12 ++++++++++++ spec/unit/runner_spec.rb | 17 +++++++++-------- 3 files changed, 22 insertions(+), 9 deletions(-) diff --git a/lib/chef/resource.rb b/lib/chef/resource.rb index b0e3a372e8..f0d816cd89 100644 --- a/lib/chef/resource.rb +++ b/lib/chef/resource.rb @@ -589,7 +589,7 @@ class Chef begin return if should_skip?(action) provider_for_action(action).run_action - rescue Exception => e + rescue StandardError => e if ignore_failure Chef::Log.error("#{custom_exception_message(e)}; ignore_failure is set, continuing") events.resource_failed(self, action, e) diff --git a/spec/unit/resource_spec.rb b/spec/unit/resource_spec.rb index a83b22ea0f..a806c5d1d9 100644 --- a/spec/unit/resource_spec.rb +++ b/spec/unit/resource_spec.rb @@ -549,6 +549,18 @@ describe Chef::Resource do expect { retriable_resource.run_action(:purr) }.to raise_error(RuntimeError) expect(retriable_resource.retries).to eq(3) end + + it "should not rescue from non-StandardError exceptions" do + retriable_resource.retries(3) + retriable_resource.retry_delay(0) # No need to wait. + + provider = Chef::Provider::SnakeOil.new(retriable_resource, run_context) + allow(Chef::Provider::SnakeOil).to receive(:new).and_return(provider) + allow(provider).to receive(:action_purr).and_raise(LoadError) + + expect(retriable_resource).not_to receive(:sleep) + expect { retriable_resource.run_action(:purr) }.to raise_error(LoadError) + end end it "runs an action by finding its provider, loading the current resource and then running the action" do diff --git a/spec/unit/runner_spec.rb b/spec/unit/runner_spec.rb index ea721965c7..eeb471608e 100644 --- a/spec/unit/runner_spec.rb +++ b/spec/unit/runner_spec.rb @@ -122,25 +122,26 @@ describe Chef::Runner do it "should raise exceptions as thrown by a provider" do provider = Chef::Provider::SnakeOil.new(run_context.resource_collection[0], run_context) - allow(Chef::Provider::SnakeOil).to receive(:new).once.and_return(provider) - allow(provider).to receive(:action_sell).once.and_raise(ArgumentError) + expect(Chef::Provider::SnakeOil).to receive(:new).once.and_return(provider) + expect(provider).to receive(:action_sell).once.and_raise(ArgumentError) expect { runner.converge }.to raise_error(ArgumentError) end it "should not raise exceptions thrown by providers if the resource has ignore_failure set to true" do allow(run_context.resource_collection[0]).to receive(:ignore_failure).and_return(true) provider = Chef::Provider::SnakeOil.new(run_context.resource_collection[0], run_context) - allow(Chef::Provider::SnakeOil).to receive(:new).once.and_return(provider) - allow(provider).to receive(:action_sell).once.and_raise(ArgumentError) + expect(Chef::Provider::SnakeOil).to receive(:new).once.and_return(provider) + expect(provider).to receive(:action_sell).once.and_raise(ArgumentError) expect { runner.converge }.not_to raise_error end it "should retry with the specified delay if retries are specified" do - first_resource.retries 3 + num_retries = 3 + allow(run_context.resource_collection[0]).to receive(:retries).and_return(num_retries) provider = Chef::Provider::SnakeOil.new(run_context.resource_collection[0], run_context) - allow(Chef::Provider::SnakeOil).to receive(:new).once.and_return(provider) - allow(provider).to receive(:action_sell).and_raise(ArgumentError) - expect(first_resource).to receive(:sleep).with(2).exactly(3).times + expect(Chef::Provider::SnakeOil).to receive(:new).exactly(num_retries + 1).times.and_return(provider) + expect(provider).to receive(:action_sell).exactly(num_retries + 1).times.and_raise(ArgumentError) + expect(run_context.resource_collection[0]).to receive(:sleep).with(2).exactly(num_retries).times expect { runner.converge }.to raise_error(ArgumentError) end -- cgit v1.2.1 From 2d086e4b3ed047a922308582a552657e8cfe2e27 Mon Sep 17 00:00:00 2001 From: Stephan Renatus Date: Fri, 24 Mar 2017 14:45:39 +0100 Subject: add retry change to RELEASE_NOTES.md I've put it next to another change related to Chef::Resource. Signed-off-by: Stephan Renatus --- RELEASE_NOTES.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index aa8d612d81..5118ce9031 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -53,6 +53,10 @@ The remediation is removing the self-dependency `depends` line in the metadata. Retained only for the service resource (where it makes some sense) and for the mount resource. +### Removed retrying of non-StandardError exceptions for Chef::Resource + +Exceptions not decending from StandardError (e.g. LoadError, SecurityError, SystemExit) will no longer trigger a retry if they are raised during the executiong of a resources with a non-zero retries setting. + ### Removed deprecated `method_missing` access from the Chef::Node object Previously, the syntax `node.foo.bar` could be used to mean `node["foo"]["bar"]`, but this API had sharp edges where methods collided -- cgit v1.2.1