Merge pull request #66 from aspiers/default-stopped-resources

prevent LWRPs starting newly-created resources by default

libraries/chef/mixin/pacemaker/standard_cib_object.rb

@@ -45,8 +45,45 @@ class Chef
         init_current_resource
       end
 
+      # In Pacemaker, target-role defaults to 'Started', but we want
+      # to allow consumers of the LWRPs the choice whether their
+      # newly created resource gets started or not, and we also want
+      # to adhere to the Principle of Least Surprise.  Therefore we
+      # stick to the intuitive semantics that
+      #
+      #   action :create
+      #
+      # creates the resource with target-role="Stopped" in order to
+      # prevent it from starting immediately, whereas
+      #
+      #   action [:create, :start]
+      #
+      # creates the resource and then starts it.
+      #
+      # Consequently we deprecate setting target-role values directly
+      # via the meta attribute.
+      def deprecate_target_role
+        if new_resource.respond_to? :meta
+          meta = new_resource.meta
+          if meta && meta['target-role']
+            ::Chef::Log.warn "#{new_resource} used deprecated target-role " +
+              "#{meta['target-role']}; use action :start / :stop instead"
+          end
+        end
+      end
+
       def standard_create_resource
+        deprecate_target_role
+
         cib_object = cib_object_class.from_chef_resource(new_resource)
+
+        # We don't want resources to automatically start on creation;
+        # only when the :create action is invoked.  However Pacemaker
+        # defaults target-role to "Started", so we need to override it.
+        if cib_object.respond_to? :meta # might be a constraint
+          cib_object.meta['target-role'] = 'Stopped'
+        end
+
         cmd = cib_object.configure_command
 
         ::Chef::Log.info "Creating new #{cib_object}"

libraries/pacemaker/mixins/resource_meta.rb

@@ -1,5 +1,5 @@
 # A mixin for Pacemaker::Resource subclasses which support meta attributes
-# (priority, target-role, is-managed)
+# (priority, target-role, is-managed, etc.)
 
 module Pacemaker
   module Mixins

libraries/pacemaker/resource.rb

@@ -17,11 +17,11 @@ module Pacemaker
     end
 
     def crm_start_command
-      "crm resource start '#{name}'"
+      "crm --force resource start '#{name}'"
     end
 
     def crm_stop_command
-      "crm resource stop '#{name}'"
+      "crm --force resource stop '#{name}'"
     end
 
     # CIB object definitions look something like:

spec/fixtures/keystone_primitive.rb

@@ -14,7 +14,6 @@ module Chef::RSpec
         [ "user",           "openstack-keystone"     ],
       ]
       KEYSTONE_PRIMITIVE.meta = [
-        [ "target-role", "Started" ],
         [ "is-managed", "true" ]
       ]
       KEYSTONE_PRIMITIVE.op = [
@@ -24,7 +23,7 @@ module Chef::RSpec
       KEYSTONE_PRIMITIVE_DEFINITION = <<'EOF'.chomp
 primitive keystone ocf:openstack:keystone \
          params os_auth_url="http://node1:5000/v2.0" os_password="adminpw" os_tenant_name="openstack" os_username="admin" user="openstack-keystone" \
-         meta is-managed="true" target-role="Started" \
+         meta is-managed="true" \
          op monitor interval="10s" timeout="60" op start interval="10s" timeout="240"
 EOF
     end

spec/fixtures/resource_group.rb

@@ -8,12 +8,11 @@ module Chef::RSpec
         ::Pacemaker::Resource::Group.new('group1')
       RESOURCE_GROUP.members = ['resource1', 'resource2']
       RESOURCE_GROUP.meta = [
-        [ "target-role", "Started" ],
         [ "is-managed", "true" ]
       ]
       RESOURCE_GROUP_DEFINITION = <<'EOF'.chomp
 group group1 resource1 resource2 \
-         meta is-managed="true" target-role="Started"
+         meta is-managed="true"
 EOF
     end
   end

spec/helpers/runnable_resource.rb

@@ -7,6 +7,15 @@ this_dir = File.dirname(__FILE__)
 require File.expand_path('provider', this_dir)
 require File.expand_path('shellout', this_dir)
 
+shared_context "stopped resource" do
+  def stopped_fixture
+    new_fixture = fixture.dup
+    new_fixture.meta = fixture.meta.dup
+    new_fixture.meta << ['target-role', 'Stopped']
+    new_fixture
+  end
+end
+
 shared_examples "a runnable resource" do |fixture|
   def expect_running(running)
     expect_any_instance_of(cib_object_class) \
@@ -18,6 +27,19 @@ shared_examples "a runnable resource" do |fixture|
 
   include Chef::RSpec::Mixlib::ShellOut
 
+  describe ":create action" do
+    include_context "stopped resource"
+
+    it "should not start a newly-created resource" do
+      stub_shellout("", fixture.definition_string)
+
+      provider.run_action :create
+
+      expect(@chef_run).to run_execute(stopped_fixture.configure_command)
+      expect(@resource).to be_updated
+    end
+  end
+
   describe ":delete action" do
     it "should not delete a running resource" do
       stub_shellout(fixture.definition_string)
@@ -47,7 +69,7 @@ shared_examples "a runnable resource" do |fixture|
   describe ":start action" do
     it_should_behave_like "action on non-existent resource", \
       :start,
-      "crm resource start #{fixture.name}", \
+      "crm --force resource start #{fixture.name}", \
       "Cannot start non-existent #{fixture}"
 
     it "should do nothing to a started resource" do
@@ -56,7 +78,7 @@ shared_examples "a runnable resource" do |fixture|
 
       provider.run_action :start
 
-      cmd = "crm resource start #{fixture.name}"
+      cmd = "crm --force resource start #{fixture.name}"
       expect(@chef_run).not_to run_execute(cmd)
       expect(@resource).not_to be_updated
     end
@@ -68,7 +90,7 @@ shared_examples "a runnable resource" do |fixture|
 
       provider.run_action :start
 
-      cmd = "crm resource start '#{fixture.name}'"
+      cmd = "crm --force resource start '#{fixture.name}'"
       expect(@chef_run).to run_execute(cmd)
       expect(@resource).to be_updated
     end
@@ -77,7 +99,7 @@ shared_examples "a runnable resource" do |fixture|
   describe ":stop action" do
     it_should_behave_like "action on non-existent resource", \
       :stop,
-      "crm resource stop #{fixture.name}", \
+      "crm --force resource stop #{fixture.name}", \
       "Cannot stop non-existent #{fixture}"
 
     it "should do nothing to a stopped resource" do
@@ -86,7 +108,7 @@ shared_examples "a runnable resource" do |fixture|
 
       provider.run_action :stop
 
-      cmd = "crm resource start #{fixture.name}"
+      cmd = "crm --force resource start #{fixture.name}"
       expect(@chef_run).not_to run_execute(cmd)
       expect(@resource).not_to be_updated
     end
@@ -97,7 +119,7 @@ shared_examples "a runnable resource" do |fixture|
 
       provider.run_action :stop
 
-      cmd = "crm resource stop '#{fixture.name}'"
+      cmd = "crm --force resource stop '#{fixture.name}'"
       expect(@chef_run).to run_execute(cmd)
       expect(@resource).to be_updated
     end

spec/providers/group_spec.rb

@@ -40,10 +40,11 @@ describe "Chef::Provider::PacemakerGroup" do
     end
 
     it "should modify the group if it has different member resources" do
-      fixture.members = %w(resource1 resource3)
+      expected = fixture.dup
-      expected_configure_cmd_args = [fixture.reconfigure_command]
+      expected.members = %w(resource1 resource3)
+      expected_configure_cmd_args = [expected.reconfigure_command]
       test_modify(expected_configure_cmd_args) do
-        @resource.members fixture.members
+        @resource.members expected.members
       end
     end
 

spec/providers/primitive_spec.rb

@@ -46,11 +46,11 @@ describe "Chef::Provider::PacemakerPrimitive" do
 
     it "should modify the primitive if it has different meta" do
       expected_configure_cmd_args = [
-        %'--set-parameter "target-role" --parameter-value "Stopped" --meta',
+        %'--set-parameter "is-managed" --parameter-value "false" --meta',
       ].map { |args| "crm_resource --resource #{fixture.name} #{args}" }
       test_modify(expected_configure_cmd_args) do
         @resource.params Hash[fixture.params]
-        @resource.meta Hash[fixture.meta].merge("target-role" => "Stopped")
+        @resource.meta Hash[fixture.meta].merge("is-managed" => "false")
       end
     end
 
@@ -58,13 +58,13 @@ describe "Chef::Provider::PacemakerPrimitive" do
       expected_configure_cmd_args = [
         %'--set-parameter "os_password" --parameter-value "newpasswd"',
         %'--delete-parameter "os_tenant_name"',
-        %'--set-parameter "target-role" --parameter-value "Stopped" --meta',
+        %'--set-parameter "is-managed" --parameter-value "false" --meta',
       ].map { |args| "crm_resource --resource #{fixture.name} #{args}" }
       test_modify(expected_configure_cmd_args) do
         new_params = Hash[fixture.params].merge("os_password" => "newpasswd")
         new_params.delete("os_tenant_name")
         @resource.params new_params
-        @resource.meta Hash[fixture.meta].merge("target-role" => "Stopped")
+        @resource.meta Hash[fixture.meta].merge("is-managed" => "false")
       end
     end
 
@@ -81,38 +81,42 @@ describe "Chef::Provider::PacemakerPrimitive" do
       end
     end
 
-    it "should create a primitive if it doesn't already exist" do
+    context "creation from scratch" do
-      # The first time, Mixlib::ShellOut needs to return an empty definition.
+      include_context "stopped resource"
-      # Then the resource gets created so the second time it needs to return
-      # the definition used for creation.
-      stub_shellout("", fixture.definition_string)
 
-      provider.run_action :create
+      it "should create a primitive if it doesn't already exist" 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.
+        stub_shellout("", fixture.definition_string)
 
-      expect(@chef_run).to run_execute(fixture.configure_command)
+        provider.run_action :create
-      expect(@resource).to be_updated
-    end
 
-    it "should barf if crm fails to create the primitive" do
+        expect(@chef_run).to run_execute(stopped_fixture.configure_command)
-      stub_shellout("", ["crm configure failed", "oh noes", 3])
+        expect(@resource).to be_updated
+      end
 
-      expect { provider.run_action :create }.to \
+      it "should barf if crm fails to create the primitive" do
-        raise_error(RuntimeError, "Failed to create #{fixture}")
+        stub_shellout("", ["crm configure failed", "oh noes", 3])
 
-      expect(@chef_run).to run_execute(fixture.configure_command)
+        expect { provider.run_action :create }.to \
-      expect(@resource).not_to be_updated
+          raise_error(RuntimeError, "Failed to create #{fixture}")
-    end
 
-    # This scenario seems rather artificial and unlikely, but it doesn't
+        expect(@chef_run).to run_execute(stopped_fixture.configure_command)
-    # do any harm to test it.
+        expect(@resource).not_to be_updated
-    it "should barf if crm creates a primitive with empty definition" do
+      end
-      stub_shellout("", "")
 
-      expect { provider.run_action :create }.to \
+      # This scenario seems rather artificial and unlikely, but it doesn't
-        raise_error(RuntimeError, "Failed to create #{fixture}")
+      # do any harm to test it.
+      it "should barf if crm creates a primitive with empty definition" do
+        stub_shellout("", "")
 
-      expect(@chef_run).to run_execute(fixture.configure_command)
+        expect { provider.run_action :create }.to \
-      expect(@resource).not_to be_updated
+          raise_error(RuntimeError, "Failed to create #{fixture}")
+
+        expect(@chef_run).to run_execute(stopped_fixture.configure_command)
+        expect(@resource).not_to be_updated
+      end
     end
 
     it "should barf if the primitive is already defined with the wrong agent" do