From de68119a1aa1c799dfef0a9c5fda027f58b5b838 Mon Sep 17 00:00:00 2001 From: Michele Baldessari Date: Fri, 22 Sep 2017 12:01:55 +0200 Subject: [PATCH] Fix resource defaults class Currently the resource defaults type has a few problems: 1) Calls pcs directly and so will break in case of concurrent CIB updates 2) The delete method is broken because it will not expand the name of the variable so nothing will be deleted Move to the common pcs() function which takes care of 1) and also fix 2) as well. Also add the tries/try_sleep parameters to be passed to the common pcs() function. Let's also add a test case to cover for this class. Change-Id: I1408c359ba9047f4295c9d820e055a8bef1891ee --- .../provider/pcmk_resource_default/pcs.rb | 38 ++++------- lib/puppet/type/pcmk_resource_default.rb | 65 +++++++++++++++++++ manifests/resource_defaults.pp | 28 +++++++- .../pacemaker_resource_defaults_spec.rb | 52 +++++++++++++++ 4 files changed, 155 insertions(+), 28 deletions(-) create mode 100644 spec/classes/pacemaker_resource_defaults_spec.rb diff --git a/lib/puppet/provider/pcmk_resource_default/pcs.rb b/lib/puppet/provider/pcmk_resource_default/pcs.rb index ecc1aba2..aa3b4d7e 100644 --- a/lib/puppet/provider/pcmk_resource_default/pcs.rb +++ b/lib/puppet/provider/pcmk_resource_default/pcs.rb @@ -1,3 +1,5 @@ +require_relative '../pcmk_common' + # Currently the implementation is somewhat naive (will not work great # with ensure => absent, unless the correct current value is also # specified). For more proper handling, prefetching should be @@ -13,42 +15,26 @@ Puppet::Type.type(:pcmk_resource_default).provide(:pcs) do cmd = "resource defaults #{name}='#{value}'" - pcs(cmd) + pcs('create', name, cmd, @resource[:tries], @resource[:try_sleep], + @resource[:verify_on_create], @resource[:post_success_sleep]) end def destroy name = @resource[:name] - cmd = 'resource defaults #{name}=' - pcs(cmd) + cmd = "resource defaults #{name}=" + pcs('create', name, cmd, @resource[:tries], @resource[:try_sleep], + @resource[:verify_on_create], @resource[:post_success_sleep]) end def exists? name = @resource[:name] value = @resource[:value] - cmd = 'resource defaults' - status, _ = pcs(cmd, - :grep => "^#{name}: #{value}$", - :allow_failure => true) - status == 0 - end - - def pcs(cmd, options={}) - full_cmd = "pcs #{cmd}" - - if options[:grep] - full_cmd += " | grep '#{options[:grep]}'" - end - - Puppet.debug("Running #{full_cmd}") - output = `#{full_cmd}` - status = $?.exitstatus - - if status != 0 && !options[:allow_failure] - raise Puppet::Error("#{full_cmd} returned #{status}, output: #{output}") - end - - [status, output] + cmd = "resource defaults | grep '^#{name}: #{value}\$'" + Puppet.debug("defaults exists #{cmd}") + status = pcs('show', name, cmd, @resource[:tries], @resource[:try_sleep], + @resource[:verify_on_create], @resource[:post_success_sleep]) + return status == false ? false : true end end diff --git a/lib/puppet/type/pcmk_resource_default.rb b/lib/puppet/type/pcmk_resource_default.rb index e342d80a..3d3c160c 100644 --- a/lib/puppet/type/pcmk_resource_default.rb +++ b/lib/puppet/type/pcmk_resource_default.rb @@ -13,4 +13,69 @@ Puppet::Type.newtype(:pcmk_resource_default) do desc "A default value for the option" end + newparam(:post_success_sleep) do + desc "The time to sleep after successful pcs action. The reason to set + this is to avoid immediate back-to-back 'pcs resource create' calls + when creating multiple resources. Defaults to '0'." + + munge do |value| + if value.is_a?(String) + unless value =~ /^[-\d.]+$/ + raise ArgumentError, "post_success_sleep must be a number" + end + value = Float(value) + end + raise ArgumentError, "post_success_sleep cannot be a negative number" if value < 0 + value + end + + defaultto 0 + end + + ## borrowed from exec.rb + newparam(:tries) do + desc "The number of times to attempt to create a pcs resource. + Defaults to '1'." + + munge do |value| + if value.is_a?(String) + unless value =~ /^[\d]+$/ + raise ArgumentError, "Tries must be an integer" + end + value = Integer(value) + end + raise ArgumentError, "Tries must be an integer >= 1" if value < 1 + value + end + + defaultto 1 + end + + newparam(:try_sleep) do + desc "The time to sleep in seconds between 'tries'." + + munge do |value| + if value.is_a?(String) + unless value =~ /^[-\d.]+$/ + raise ArgumentError, "try_sleep must be a number" + end + value = Float(value) + end + raise ArgumentError, "try_sleep cannot be a negative number" if value < 0 + value + end + + defaultto 0 + end + + newparam(:verify_on_create, :boolean => true, :parent => Puppet::Parameter::Boolean) do + desc "Whether to verify pcs resource creation with an additional + call to 'pcs resource show' rather than just relying on the exit + status of 'pcs resource create'. When true, $try_sleep + determines how long to wait to verify and $post_success_sleep is + ignored. Defaults to `false`." + + defaultto false + end + end diff --git a/manifests/resource_defaults.pp b/manifests/resource_defaults.pp index a1b7d1a9..55962aa0 100644 --- a/manifests/resource_defaults.pp +++ b/manifests/resource_defaults.pp @@ -7,6 +7,22 @@ # [*defaults*] # (required) Comma separated string of key=value pairs specifying defaults. # +# [*post_success_sleep*] +# (optional) How long to wait acfter successful action +# Defaults to 0 +# +# [*tries*] +# (optional) How many times to attempt to create the constraint +# Defaults to 1 +# +# [*try_sleep*] +# (optional) How long to wait between tries, in seconds +# Defaults to 0 +# +# [*verify_on_create*] +# (optional) Whether to verify creation of resource +# Defaults to false +# # [*ensure*] # (optional) Whether to create or remove the defaults # Defaults to present @@ -38,13 +54,21 @@ # class pacemaker::resource_defaults( $defaults, - $ensure = 'present', + $post_success_sleep = 0, + $tries = 1, + $try_sleep = 0, + $verify_on_create = false, + $ensure = 'present', ) { create_resources( 'pcmk_resource_default', $defaults, { - ensure => $ensure, + ensure => $ensure, + post_success_sleep => $post_success_sleep, + tries => $tries, + try_sleep => $try_sleep, + verify_on_create => $verify_on_create, } ) } diff --git a/spec/classes/pacemaker_resource_defaults_spec.rb b/spec/classes/pacemaker_resource_defaults_spec.rb new file mode 100644 index 00000000..79fcb3b4 --- /dev/null +++ b/spec/classes/pacemaker_resource_defaults_spec.rb @@ -0,0 +1,52 @@ +# +# Copyright (C) 2017 Red Hat, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. +# +require 'spec_helper' + +describe 'pacemaker::resource_defaults', type: :class do + + let(:title) { 'foo' } + + shared_examples_for 'pacemaker::resource_defaults' do + context 'with params' do + let(:params) { { + :defaults => { 'fencingtest' => { 'name' => 'requires', 'value' => 'fencing', }, }, + :tries => 10, + :try_sleep => 20, + } } + it { is_expected.to compile.with_all_deps } + + it { + is_expected.to contain_pcmk_resource_default('fencingtest').with( + :name => 'requires', + :value => 'fencing', + :tries => 10, + :try_sleep => 20, + ) + } + end + + end + + on_supported_os.each do |os, facts| + context "on #{os}" do + let(:facts) do + facts.merge({ :hostname => 'node.example.com' }) + end + + it_behaves_like 'pacemaker::resource_defaults' + end + end +end