Merge pull request #37 from aspiers/create-resource-check

handle creation errors more robustly

libraries/chef/mixin/pacemaker/standard_cib_object.rb

@@ -46,13 +46,16 @@ class Chef
           action :nothing
         end.run_action(:run)
 
-        cib_object = Pacemaker::CIBObject.from_name(new_resource.name)
+        created_cib_object = Pacemaker::CIBObject.from_name(new_resource.name)
-        if cib_object.exists?
+
-          new_resource.updated_by_last_action(true)
+        raise "Failed to create #{cib_object}" if created_cib_object.nil?
-          ::Chef::Log.info "Successfully configured #{cib_object}"
+        unless created_cib_object.exists?
-        else
+          # This case seems pretty unlikely
-          ::Chef::Log.error "Failed to configure #{cib_object}"
+          raise "Definition missing for #{created_cib_object} after creation"
         end
+
+        new_resource.updated_by_last_action(true)
+        ::Chef::Log.info "Successfully configured #{created_cib_object}"
       end
 
       def standard_delete_resource

spec/helpers/cib_object.rb

@@ -8,20 +8,64 @@ require File.expand_path('../../libraries/pacemaker/cib_object',
 module Chef::RSpec
   module Pacemaker
     module CIBObject
-      # "crm configure show" is executed by load_current_resource, and
+      # Return a Mixlib::ShellOut double which mimics successful
-      # again later on for the :create action, to see whether to create or
+      # execution of a command, returning the given string on STDOUT.
-      # modify.
+      def succeeding_shellout_double(string)
-      def shellout_double(definition)
         shellout = double(Mixlib::ShellOut)
         shellout.stub(:environment).and_return({})
         shellout.stub(:run_command)
         shellout.stub(:error!)
-        expect(shellout).to receive(:stdout).and_return(definition)
+        expect(shellout).to receive(:stdout).and_return(string)
         shellout
       end
 
-      def expect_definitions(*definitions)
+      # Return a Mixlib::ShellOut double which mimics failed
-        doubles = definitions.map { |d| shellout_double(d) }
+      # execution of a command, raising an exception when #error! is
+      # called.  We expect #error! to be called, because if it isn't,
+      # that probably indicates the code isn't robust enough.  This
+      # may need to be relaxed in the future.
+      def failing_shellout_double(stdout='', stderr='', exitstatus=1)
+        shellout = double(Mixlib::ShellOut)
+        shellout.stub(:environment).and_return({})
+        shellout.stub(:run_command)
+        shellout.stub(:stdout).and_return(stdout)
+        shellout.stub(:stderr).and_return(stderr)
+        shellout.stub(:exitstatus).and_return(exitstatus)
+        exception = Mixlib::ShellOut::ShellCommandFailed.new(
+          "Expected process to exit with 0, " +
+          "but received '#{exitstatus}'"
+        )
+        expect(shellout).to receive(:error!).and_raise(exception)
+        shellout
+      end
+
+      # This stubs Mixlib::ShellOut.new with a sequence of doubles
+      # with a corresponding sequence of behaviours.  This allows us
+      # to simulate the output of a series of shell commands being run
+      # via Mixlib::ShellOut.  Each double either mimics a successful
+      # command execution whose #stdout method returns the given
+      # string, or a failed execution with the given exit code and
+      # STDOUT/STDERR.
+      #
+      # results is an Array describing the sequence of behaviours;
+      # each element is either a string mimicking STDOUT from
+      # successful command execution, or a [stdout, stderr, exitcode]
+      # status mimicking command execution failure.
+      #
+      # For example, "crm configure show" is executed by
+      # #load_current_resource, and again later on for the :create
+      # action, to see whether to create or modify.  So the first
+      # double in the sequence would return an empty definition if we
+      # wanted to test creation of a new CIB object, or an existing
+      # definition if we wanted to test modification of an existing
+      # one.  If the test needs subsequent doubles to return different
+      # values then stdout_strings can have more than one element.
+      def stub_shellout(*results)
+        doubles = results.map { |result|
+          result.is_a?(String) ?
+              succeeding_shellout_double(result)
+            : failing_shellout_double(*result)
+        }
         Mixlib::ShellOut.stub(:new).and_return(*doubles)
       end
     end
@@ -40,7 +84,7 @@ shared_examples "a CIB object" do
   end
 
   it "should be instantiated via Pacemaker::CIBObject.from_name" do
-    expect_definitions(fixture.definition_string)
+    stub_shellout(fixture.definition_string)
     obj = Pacemaker::CIBObject.from_name(fixture.name)
     expect_to_match_fixture(obj)
   end
@@ -51,7 +95,7 @@ shared_examples "a CIB object" do
   end
 
   it "should barf if the loaded definition's type is not colocation" do
-    expect_definitions("clone foo blah blah")
+    stub_shellout("clone foo blah blah")
     expect { fixture.load_definition }.to \
       raise_error(Pacemaker::CIBObject::TypeMismatch,
                   "Expected #{object_type} type but loaded definition was type clone")
@@ -62,7 +106,7 @@ shared_examples "action on non-existent resource" do |action, cmd, expected_erro
   include Chef::RSpec::Pacemaker::CIBObject
 
   it "should not attempt to #{action.to_s} a non-existent resource" do
-    expect_definitions("")
+    stub_shellout("")
 
     if expected_error
       expect { provider.run_action action }.to \

spec/helpers/runnable_resource.rb

@@ -21,7 +21,7 @@ shared_examples "a runnable resource" do |fixture|
       :delete, "crm configure delete #{fixture.name}", nil
 
     it "should not delete a running resource" do
-      expect_definitions(fixture.definition_string)
+      stub_shellout(fixture.definition_string)
       expect_running(true)
 
       expected_error = "Cannot delete running #{fixture}"
@@ -34,7 +34,7 @@ shared_examples "a runnable resource" do |fixture|
     end
 
     it "should delete a non-running resource" do
-      expect_definitions(fixture.definition_string)
+      stub_shellout(fixture.definition_string)
       expect_running(false)
 
       provider.run_action :delete
@@ -52,7 +52,7 @@ shared_examples "a runnable resource" do |fixture|
       "Cannot start non-existent #{fixture}"
 
     it "should do nothing to a started resource" do
-      expect_definitions(fixture.definition_string)
+      stub_shellout(fixture.definition_string)
       expect_running(true)
 
       provider.run_action :start
@@ -64,7 +64,7 @@ shared_examples "a runnable resource" do |fixture|
 
     it "should start a stopped resource" do
       config = fixture.definition_string.sub("Started", "Stopped")
-      expect_definitions(config)
+      stub_shellout(config)
       expect_running(false)
 
       provider.run_action :start
@@ -82,7 +82,7 @@ shared_examples "a runnable resource" do |fixture|
       "Cannot stop non-existent #{fixture}"
 
     it "should do nothing to a stopped resource" do
-      expect_definitions(fixture.definition_string)
+      stub_shellout(fixture.definition_string)
       expect_running(false)
 
       provider.run_action :stop
@@ -93,7 +93,7 @@ shared_examples "a runnable resource" do |fixture|
     end
 
     it "should stop a started resource" do
-      expect_definitions(fixture.definition_string)
+      stub_shellout(fixture.definition_string)
       expect_running(true)
 
       provider.run_action :stop

spec/providers/colocation_spec.rb

@@ -35,7 +35,7 @@ describe "Chef::Provider::PacemakerColocation" do
     def test_modify(expected_cmds)
       yield
 
-      expect_definitions(fixture.definition_string)
+      stub_shellout(fixture.definition_string)
 
       provider.run_action :create
 

spec/providers/group_spec.rb

@@ -36,7 +36,7 @@ describe "Chef::Provider::PacemakerGroup" do
     def test_modify(expected_cmds)
       yield
 
-      expect_definitions(fixture.definition_string)
+      stub_shellout(fixture.definition_string)
 
       provider.run_action :create
 

spec/providers/primitive_spec.rb

@@ -38,7 +38,7 @@ describe "Chef::Provider::PacemakerPrimitive" do
     def test_modify(expected_cmds)
       yield
 
-      expect_definitions(fixture.definition_string)
+      stub_shellout(fixture.definition_string)
 
       provider.run_action :create
 
@@ -102,7 +102,7 @@ describe "Chef::Provider::PacemakerPrimitive" do
       # The first time, Mixlib::ShellOut needs to return an empty definition.
       # Then the resource gets created so the second time it needs to return
       # the definition used for creation.
-      expect_definitions("", fixture.definition_string)
+      stub_shellout("", fixture.definition_string)
 
       provider.run_action :create
 
@@ -110,10 +110,32 @@ describe "Chef::Provider::PacemakerPrimitive" do
       expect(@resource).to be_updated
     end
 
+    it "should barf if crm fails to create the primitive" do
+      stub_shellout("", ["crm configure failed", "oh noes", 3])
+
+      expect { provider.run_action :create }.to \
+        raise_error(RuntimeError, "Failed to create #{fixture}")
+
+      expect(@chef_run).to run_execute(fixture.crm_configure_command)
+      expect(@resource).not_to be_updated
+    end
+
+    # This scenario seems rather artificial and unlikely, but it doesn't
+    # do any harm to test it.
+    it "should barf if crm creates a primitive with empty definition" do
+      stub_shellout("", "")
+
+      expect { provider.run_action :create }.to \
+        raise_error(RuntimeError, "Failed to create #{fixture}")
+
+      expect(@chef_run).to run_execute(fixture.crm_configure_command)
+      expect(@resource).not_to be_updated
+    end
+
     it "should barf if the primitive is already defined with the wrong agent" do
       existing_agent = "ocf:openstack:something-else"
       definition = fixture.definition_string.sub(fixture.agent, existing_agent)
-      expect_definitions(definition)
+      stub_shellout(definition)
 
       expected_error = \
         "Existing #{fixture} has agent '#{existing_agent}' " \