newly created resources should not be started

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.

Since we are honouring :start / :stop actions to determine the
target-role value, if target-role is specified via meta, it will just be
overridden anyway.  So we also deprecate direct use of target-role meta
parameter in recipes.

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

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)

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
-      # 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)
+    context "creation from scratch" do
+      include_context "stopped resource"
 
-      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)
-      expect(@resource).to be_updated
-    end
+        provider.run_action :create
 
-    it "should barf if crm fails to create the primitive" do
-      stub_shellout("", ["crm configure failed", "oh noes", 3])
+        expect(@chef_run).to run_execute(stopped_fixture.configure_command)
+        expect(@resource).to be_updated
+      end
 
-      expect { provider.run_action :create }.to \
-        raise_error(RuntimeError, "Failed to create #{fixture}")
+      it "should barf if crm fails to create the primitive" do
+        stub_shellout("", ["crm configure failed", "oh noes", 3])
 
-      expect(@chef_run).to run_execute(fixture.configure_command)
-      expect(@resource).not_to be_updated
-    end
+        expect { provider.run_action :create }.to \
+          raise_error(RuntimeError, "Failed to create #{fixture}")
 
-    # 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(@chef_run).to run_execute(stopped_fixture.configure_command)
+        expect(@resource).not_to be_updated
+      end
 
-      expect { provider.run_action :create }.to \
-        raise_error(RuntimeError, "Failed to create #{fixture}")
+      # 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(@chef_run).to run_execute(fixture.configure_command)
-      expect(@resource).not_to be_updated
+        expect { provider.run_action :create }.to \
+          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