diff --git a/libraries/chef/mixin/pacemaker/standard_cib_object.rb b/libraries/chef/mixin/pacemaker/standard_cib_object.rb index 9e30077..b6192e5 100644 --- a/libraries/chef/mixin/pacemaker/standard_cib_object.rb +++ b/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}" diff --git a/libraries/pacemaker/mixins/resource_meta.rb b/libraries/pacemaker/mixins/resource_meta.rb index 1c632d1..5a721ad 100644 --- a/libraries/pacemaker/mixins/resource_meta.rb +++ b/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 diff --git a/spec/fixtures/keystone_primitive.rb b/spec/fixtures/keystone_primitive.rb index fc6424d..e15139e 100644 --- a/spec/fixtures/keystone_primitive.rb +++ b/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 diff --git a/spec/fixtures/resource_group.rb b/spec/fixtures/resource_group.rb index 485ed1c..7d6d3d6 100644 --- a/spec/fixtures/resource_group.rb +++ b/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 diff --git a/spec/helpers/runnable_resource.rb b/spec/helpers/runnable_resource.rb index 5af428d..b6d8a5c 100644 --- a/spec/helpers/runnable_resource.rb +++ b/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) diff --git a/spec/providers/primitive_spec.rb b/spec/providers/primitive_spec.rb index 8b7c528..ec4f968 100644 --- a/spec/providers/primitive_spec.rb +++ b/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