From fdebd24117989339f53a8cea0016987a75bedb49 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Wed, 2 Apr 2014 18:27:19 +0100 Subject: [PATCH] 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. --- .../mixin/pacemaker/standard_cib_object.rb | 37 ++++++++++++ libraries/pacemaker/mixins/resource_meta.rb | 2 +- spec/fixtures/keystone_primitive.rb | 3 +- spec/fixtures/resource_group.rb | 3 +- spec/helpers/runnable_resource.rb | 22 +++++++ spec/providers/primitive_spec.rb | 60 ++++++++++--------- 6 files changed, 94 insertions(+), 33 deletions(-) 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