From 9b23a904040cee5b1e24dcef0c8ae0602123ccb6 Mon Sep 17 00:00:00 2001 From: Bogdan Dobrelya Date: Mon, 5 Jan 2015 10:27:13 +0100 Subject: [PATCH] Make rabbitmq providers to retry the commands W/o this fix, then RabbitMQ put in a cluster and managed, for example, by Pacemaker, rabbitmqctl/rabbitmqadmin commands could fail due to cluster is not ready yet. The solution is: 1) Add a run_with_retries method to the existing Rabbitmqctl class as a retry decorator to ensure the commands will retry instead of fail then the cluster is not ready. 2) Derive the providers from the former one to make them able to retry all of the list_* commands and, as a result, to wait for RabbitMQ ready as well. 3) Update rspecs. Add missing rspec for rabbitmq_plugin Co-authors: Dmitry Ilyin , Colleen Murphy Closes-bug: #MODULES-1452 Signed-off-by: Bogdan Dobrelya --- .../rabbitmq_exchange/rabbitmqadmin.rb | 15 ++++++++--- .../rabbitmq_plugin/rabbitmqplugins.rb | 11 +++++--- .../provider/rabbitmq_policy/rabbitmqctl.rb | 4 ++- .../provider/rabbitmq_user/rabbitmqctl.rb | 13 +++++++--- .../rabbitmq_user_permissions/rabbitmqctl.rb | 7 +++-- .../provider/rabbitmq_vhost/rabbitmqctl.rb | 11 +++++--- lib/puppet/provider/rabbitmqctl.rb | 23 ++++++++++++++++ .../rabbitmq_plugin/rabbitmqctl_spec.rb | 26 +++++++++++++++++++ .../rabbitmq_user/rabbitmqctl_spec.rb | 4 +-- .../rabbitmqctl_spec.rb | 4 +-- 10 files changed, 98 insertions(+), 20 deletions(-) create mode 100644 spec/unit/puppet/provider/rabbitmq_plugin/rabbitmqctl_spec.rb diff --git a/lib/puppet/provider/rabbitmq_exchange/rabbitmqadmin.rb b/lib/puppet/provider/rabbitmq_exchange/rabbitmqadmin.rb index 15f0aed8a..f9ea3d286 100644 --- a/lib/puppet/provider/rabbitmq_exchange/rabbitmqadmin.rb +++ b/lib/puppet/provider/rabbitmq_exchange/rabbitmqadmin.rb @@ -1,5 +1,6 @@ require 'puppet' -Puppet::Type.type(:rabbitmq_exchange).provide(:rabbitmqadmin) do +require File.expand_path(File.join(File.dirname(__FILE__), '..', 'rabbitmqctl')) +Puppet::Type.type(:rabbitmq_exchange).provide(:rabbitmqadmin, :parent => Puppet::Provider::Rabbitmqctl) do if Puppet::PUPPETVERSION.to_f < 3 commands :rabbitmqctl => 'rabbitmqctl' @@ -24,7 +25,11 @@ def should_vhost def self.all_vhosts vhosts = [] - parse_command(rabbitmqctl('list_vhosts')).collect do |vhost| + parse_command( + self.run_with_retries { + rabbitmqctl('list_vhosts') + } + ).collect do |vhost| vhosts.push(vhost) end vhosts @@ -32,7 +37,11 @@ def self.all_vhosts def self.all_exchanges(vhost) exchanges = [] - parse_command(rabbitmqctl('list_exchanges', '-p', vhost, 'name', 'type')) + parse_command( + self.run_with_retries { + rabbitmqctl('list_exchanges', '-p', vhost, 'name', 'type') + } + ) end def self.parse_command(cmd_output) diff --git a/lib/puppet/provider/rabbitmq_plugin/rabbitmqplugins.rb b/lib/puppet/provider/rabbitmq_plugin/rabbitmqplugins.rb index a771a602f..7ab5420c3 100644 --- a/lib/puppet/provider/rabbitmq_plugin/rabbitmqplugins.rb +++ b/lib/puppet/provider/rabbitmq_plugin/rabbitmqplugins.rb @@ -1,4 +1,5 @@ -Puppet::Type.type(:rabbitmq_plugin).provide(:rabbitmqplugins) do +require File.expand_path(File.join(File.dirname(__FILE__), '..', 'rabbitmqctl')) +Puppet::Type.type(:rabbitmq_plugin).provide(:rabbitmqplugins, :parent => Puppet::Provider::Rabbitmqctl) do if Puppet::PUPPETVERSION.to_f < 3 if Facter.value(:osfamily) == 'RedHat' @@ -21,7 +22,9 @@ defaultfor :feature => :posix def self.instances - rabbitmqplugins('list', '-E', '-m').split(/\n/).map do |line| + self.run_with_retries { + rabbitmqplugins('list', '-E', '-m') + }.split(/\n/).map do |line| if line =~ /^(\S+)$/ new(:name => $1) else @@ -39,7 +42,9 @@ def destroy end def exists? - rabbitmqplugins('list', '-E', '-m').split(/\n/).detect do |line| + self.class.run_with_retries { + rabbitmqplugins('list', '-E', '-m') + }.split(/\n/).detect do |line| line.match(/^#{resource[:name]}$/) end end diff --git a/lib/puppet/provider/rabbitmq_policy/rabbitmqctl.rb b/lib/puppet/provider/rabbitmq_policy/rabbitmqctl.rb index 04f5a780f..1c3550dca 100644 --- a/lib/puppet/provider/rabbitmq_policy/rabbitmqctl.rb +++ b/lib/puppet/provider/rabbitmq_policy/rabbitmqctl.rb @@ -11,7 +11,9 @@ def self.policies(name, vhost) @policies = {} unless @policies unless @policies[vhost] @policies[vhost] = {} - rabbitmqctl('list_policies', '-q', '-p', vhost).split(/\n/).each do |line| + self.run_with_retries { + rabbitmqctl('list_policies', '-q', '-p', vhost) + }.split(/\n/).each do |line| # rabbitmq<3.2 does not support the applyto field # 1 2 3? 4 5 6 # / ha-all all .* {"ha-mode":"all","ha-sync-mode":"automatic"} 0 diff --git a/lib/puppet/provider/rabbitmq_user/rabbitmqctl.rb b/lib/puppet/provider/rabbitmq_user/rabbitmqctl.rb index 8a8e9f41c..cfa251581 100644 --- a/lib/puppet/provider/rabbitmq_user/rabbitmqctl.rb +++ b/lib/puppet/provider/rabbitmq_user/rabbitmqctl.rb @@ -1,6 +1,7 @@ require 'puppet' require 'set' -Puppet::Type.type(:rabbitmq_user).provide(:rabbitmqctl) do +require File.expand_path(File.join(File.dirname(__FILE__), '..', 'rabbitmqctl')) +Puppet::Type.type(:rabbitmq_user).provide(:rabbitmqctl, :parent => Puppet::Provider::Rabbitmqctl) do if Puppet::PUPPETVERSION.to_f < 3 commands :rabbitmqctl => 'rabbitmqctl' @@ -13,7 +14,9 @@ defaultfor :feature => :posix def self.instances - rabbitmqctl('-q', 'list_users').split(/\n/).collect do |line| + self.run_with_retries { + rabbitmqctl('-q', 'list_users') + }.split(/\n/).collect do |line| if line =~ /^(\S+)(\s+\[.*?\]|)$/ new(:name => $1) else @@ -55,12 +58,14 @@ def destroy end def exists? - rabbitmqctl('-q', 'list_users').split(/\n/).detect do |line| + self.class.run_with_retries { + rabbitmqctl('-q', 'list_users') + }.split(/\n/).detect do |line| line.match(/^#{Regexp.escape(resource[:name])}(\s+(\[.*?\]|\S+)|)$/) end end - + def tags get_user_tags.entries.sort end diff --git a/lib/puppet/provider/rabbitmq_user_permissions/rabbitmqctl.rb b/lib/puppet/provider/rabbitmq_user_permissions/rabbitmqctl.rb index 360850629..a0b8b5a1a 100644 --- a/lib/puppet/provider/rabbitmq_user_permissions/rabbitmqctl.rb +++ b/lib/puppet/provider/rabbitmq_user_permissions/rabbitmqctl.rb @@ -1,4 +1,5 @@ -Puppet::Type.type(:rabbitmq_user_permissions).provide(:rabbitmqctl) do +require File.expand_path(File.join(File.dirname(__FILE__), '..', 'rabbitmqctl')) +Puppet::Type.type(:rabbitmq_user_permissions).provide(:rabbitmqctl, :parent => Puppet::Provider::Rabbitmqctl) do if Puppet::PUPPETVERSION.to_f < 3 commands :rabbitmqctl => 'rabbitmqctl' @@ -15,7 +16,9 @@ def self.users(name, vhost) @users = {} unless @users unless @users[name] @users[name] = {} - rabbitmqctl('-q', 'list_user_permissions', name).split(/\n/).each do |line| + self.run_with_retries { + rabbitmqctl('-q', 'list_user_permissions', name) + }.split(/\n/).each do |line| line = self::strip_backslashes(line) if line =~ /^(\S+)\s+(\S*)\s+(\S*)\s+(\S*)$/ @users[name][$1] = diff --git a/lib/puppet/provider/rabbitmq_vhost/rabbitmqctl.rb b/lib/puppet/provider/rabbitmq_vhost/rabbitmqctl.rb index 836cff19a..fbd389d87 100644 --- a/lib/puppet/provider/rabbitmq_vhost/rabbitmqctl.rb +++ b/lib/puppet/provider/rabbitmq_vhost/rabbitmqctl.rb @@ -1,4 +1,5 @@ -Puppet::Type.type(:rabbitmq_vhost).provide(:rabbitmqctl) do +require File.expand_path(File.join(File.dirname(__FILE__), '..', 'rabbitmqctl')) +Puppet::Type.type(:rabbitmq_vhost).provide(:rabbitmqctl, :parent => Puppet::Provider::Rabbitmqctl) do if Puppet::PUPPETVERSION.to_f < 3 commands :rabbitmqctl => 'rabbitmqctl' @@ -9,7 +10,9 @@ end def self.instances - rabbitmqctl('-q', 'list_vhosts').split(/\n/).map do |line| + self.run_with_retries { + rabbitmqctl('-q', 'list_vhosts') + }.split(/\n/).map do |line| if line =~ /^(\S+)$/ new(:name => $1) else @@ -27,7 +30,9 @@ def destroy end def exists? - out = rabbitmqctl('-q', 'list_vhosts').split(/\n/).detect do |line| + out = self.class.run_with_retries { + rabbitmqctl('-q', 'list_vhosts') + }.split(/\n/).detect do |line| line.match(/^#{Regexp.escape(resource[:name])}$/) end end diff --git a/lib/puppet/provider/rabbitmqctl.rb b/lib/puppet/provider/rabbitmqctl.rb index 4afdfd168..d23664569 100644 --- a/lib/puppet/provider/rabbitmqctl.rb +++ b/lib/puppet/provider/rabbitmqctl.rb @@ -7,4 +7,27 @@ def self.rabbitmq_version version = output.match(/\{rabbit,"RabbitMQ","([\d\.]+)"\}/) version[1] if version end + + # Retry the given code block 'count' retries or until the + # command suceeeds. Use 'step' delay between retries. + # Limit each query time by 'timeout'. + # For example: + # users = self.class.run_with_retries { rabbitmqctl 'list_users' } + def self.run_with_retries(count=30, step=6, timeout=10) + count.times do |n| + begin + output = Timeout::timeout(timeout) do + yield + end + rescue Puppet::ExecutionFailure, Timeout + Puppet.debug 'Command failed, retrying' + sleep step + else + Puppet.debug 'Command succeeded' + return output + end + end + raise Puppet::Error, "Command is still failing after #{count * step} seconds expired!" + end + end diff --git a/spec/unit/puppet/provider/rabbitmq_plugin/rabbitmqctl_spec.rb b/spec/unit/puppet/provider/rabbitmq_plugin/rabbitmqctl_spec.rb new file mode 100644 index 000000000..c398b6292 --- /dev/null +++ b/spec/unit/puppet/provider/rabbitmq_plugin/rabbitmqctl_spec.rb @@ -0,0 +1,26 @@ +require 'puppet' +require 'mocha' +RSpec.configure do |config| + config.mock_with :mocha +end +provider_class = Puppet::Type.type(:rabbitmq_plugin).provider(:rabbitmqplugins) +describe provider_class do + before :each do + @resource = Puppet::Type::Rabbitmq_plugin.new( + {:name => 'foo'} + ) + @provider = provider_class.new(@resource) + end + it 'should match plugins' do + @provider.expects(:rabbitmqplugins).with('list', '-E', '-m').returns("foo\n") + @provider.exists?.should == 'foo' + end + it 'should call rabbitmqplugins to enable' do + @provider.expects(:rabbitmqplugins).with('enable', 'foo') + @provider.create + end + it 'should call rabbitmqplugins to disable' do + @provider.expects(:rabbitmqplugins).with('disable', 'foo') + @provider.destroy + end +end diff --git a/spec/unit/puppet/provider/rabbitmq_user/rabbitmqctl_spec.rb b/spec/unit/puppet/provider/rabbitmq_user/rabbitmqctl_spec.rb index a207139bf..2012ccc05 100644 --- a/spec/unit/puppet/provider/rabbitmq_user/rabbitmqctl_spec.rb +++ b/spec/unit/puppet/provider/rabbitmq_user/rabbitmqctl_spec.rb @@ -99,7 +99,7 @@ kitchen [] kitchen2 [abc, def, ghi] EOT - @provider.expects(:rabbitmqctl).with('set_user_tags', 'foo', ['bar','baz', 'administrator'].sort) + @provider.expects(:rabbitmqctl).with('set_user_tags', 'foo', ['bar','baz', 'administrator'].sort) @provider.admin=:true end it 'should be able to unset admin value' do @@ -118,7 +118,7 @@ kitchen [] kitchen2 [abc, def, ghi] EOT - @provider.expects(:rabbitmqctl).with('set_user_tags', 'foo', ['bar','baz'].sort) + @provider.expects(:rabbitmqctl).with('set_user_tags', 'foo', ['bar','baz'].sort) @provider.admin=:false end diff --git a/spec/unit/puppet/provider/rabbitmq_user_permissions/rabbitmqctl_spec.rb b/spec/unit/puppet/provider/rabbitmq_user_permissions/rabbitmqctl_spec.rb index 649fb7458..bdbc73e82 100644 --- a/spec/unit/puppet/provider/rabbitmq_user_permissions/rabbitmqctl_spec.rb +++ b/spec/unit/puppet/provider/rabbitmq_user_permissions/rabbitmqctl_spec.rb @@ -41,13 +41,13 @@ @provider.instance_variable_set(:@should_vhost, "bar") @provider.instance_variable_set(:@should_user, "foo") @provider.expects(:rabbitmqctl).with('set_permissions', '-p', 'bar', 'foo', "''", "''", "''") - @provider.create + @provider.create end it 'should destroy permissions' do @provider.instance_variable_set(:@should_vhost, "bar") @provider.instance_variable_set(:@should_user, "foo") @provider.expects(:rabbitmqctl).with('clear_permissions', '-p', 'bar', 'foo') - @provider.destroy + @provider.destroy end {:configure_permission => '1', :write_permission => '2', :read_permission => '3'}.each do |k,v| it "should be able to retrieve #{k}" do