diff --git a/lib/puppet/provider/openstack.rb b/lib/puppet/provider/openstack.rb index f7101993..5e40c090 100644 --- a/lib/puppet/provider/openstack.rb +++ b/lib/puppet/provider/openstack.rb @@ -1,5 +1,6 @@ require 'csv' require 'puppet' +require 'timeout' class Puppet::Error::OpenstackAuthInputError < Puppet::Error end @@ -10,7 +11,49 @@ end class Puppet::Provider::Openstack < Puppet::Provider initvars # so commands will work - commands :openstack => 'openstack' + commands :openstack_command => 'openstack' + + # this actions are not idempotent and retries can cause + # duplications or endless loops + def self.no_retry_actions + %w(create remove delete) + end + + # timeout the openstack command + # after this number of seconds + # retry the command until the request_timeout + def self.command_timeout + 20 + end + + # timeout the entire request with error + # after this number of seconds + def self.request_timeout + 60 + end + + # sleep for this number of seconds + # between command retries + def self.retry_sleep + 3 + end + + # run the openstack command + # with command_timeout + def self.openstack(*args) + begin + Timeout.timeout(command_timeout) do + openstack_command *args + end + rescue Timeout::Error + raise Puppet::ExecutionFailure, "Command: 'openstack #{args.inspect}' has been running for more then #{command_timeout} seconds!" + end + end + + # get the current timestamp + def self.current_time + Time.now.to_i + end # Returns an array of hashes, where the keys are the downcased CSV headers # with underscores instead of spaces @@ -18,14 +61,15 @@ class Puppet::Provider::Openstack < Puppet::Provider env = credentials ? credentials.to_env : {} Puppet::Util.withenv(env) do rv = nil - timeout = 10 - end_time = Time.now.to_i + timeout + end_time = current_time + request_timeout loop do begin - if(action == 'list') + if action == 'list' + # shell output is: + # ID,Name,Description,Enabled response = openstack(service, action, '--quiet', '--format', 'csv', properties) response = parse_csv(response) - keys = response.delete_at(0) # ID,Name,Description,Enabled + keys = response.delete_at(0) rv = response.collect do |line| hash = {} keys.each_index do |index| @@ -34,12 +78,15 @@ class Puppet::Provider::Openstack < Puppet::Provider end hash end - elsif(action == 'show' || action == 'create') + elsif action == 'show' or action == 'create' rv = {} - # shell output is name="value"\nid="value2"\ndescription="value3" etc. + # shell output is: + # name="value1" + # id="value2" + # description="value3" openstack(service, action, '--format', 'shell', properties).split("\n").each do |line| # key is everything before the first "=" - key, val = line.split("=", 2) + key, val = line.split('=', 2) next unless val # Ignore warnings # value is everything after the first "=", with leading and trailing double quotes stripped val = val.gsub(/\A"|"\Z/, '') @@ -49,24 +96,13 @@ class Puppet::Provider::Openstack < Puppet::Provider rv = openstack(service, action, properties) end break - rescue Puppet::ExecutionFailure => e - if e.message =~ /HTTP 40[13]/ - raise(Puppet::Error::OpenstackUnauthorizedError, 'Could not authenticate.') - elsif e.message =~ /Unable to establish connection/ - current_time = Time.now.to_i - if current_time > end_time - break - else - wait = end_time - current_time - Puppet::debug("Non-fatal error: \"#{e.message}\"; retrying for #{wait} more seconds.") - if wait > timeout - 2 # Only notice the first time - notice("#{service} service is unavailable. Will retry for up to #{wait} seconds.") - end - end - sleep(2) - else - raise e - end + rescue Puppet::ExecutionFailure => exception + raise Puppet::Error::OpenstackUnauthorizedError, 'Could not authenticate' if exception.message =~ /HTTP 40[13]/ + raise exception if current_time > end_time + debug "Non-fatal error: '#{exception.message}'. Retrying for #{end_time - current_time} more seconds" + raise exception if no_retry_actions.include? action + sleep retry_sleep + retry end end return rv diff --git a/spec/unit/provider/openstack_spec.rb b/spec/unit/provider/openstack_spec.rb index 35a1d4bb..5f2fd9f8 100644 --- a/spec/unit/provider/openstack_spec.rb +++ b/spec/unit/provider/openstack_spec.rb @@ -4,10 +4,10 @@ require 'puppet/provider/openstack' describe Puppet::Provider::Openstack do before(:each) do - ENV['OS_USERNAME'] = nil - ENV['OS_PASSWORD'] = nil + ENV['OS_USERNAME'] = nil + ENV['OS_PASSWORD'] = nil ENV['OS_PROJECT_NAME'] = nil - ENV['OS_AUTH_URL'] = nil + ENV['OS_AUTH_URL'] = nil end let(:type) do @@ -17,10 +17,37 @@ describe Puppet::Provider::Openstack do end end + let(:credentials) do + credentials = mock('credentials') + credentials.stubs(:to_env).returns({ + 'OS_USERNAME' => 'user', + 'OS_PASSWORD' => 'password', + 'OS_PROJECT_NAME' => 'project', + 'OS_AUTH_URL' => 'http://url', + }) + credentials + end + + let(:list_data) do + <<-eos +"ID","Name","Description","Enabled" +"1cb05cfed7c24279be884ba4f6520262","test","Test tenant",True + eos + end + + let(:show_data) do + <<-eos +description="Test tenant" +enabled="True" +id="1cb05cfed7c24279be884ba4f6520262" +name="test" + eos + end + describe '#request' do let(:resource_attrs) do { - :name => 'stubresource', + :name => 'stubresource', } end @@ -28,30 +55,72 @@ describe Puppet::Provider::Openstack do Puppet::Provider::Openstack.new(type.new(resource_attrs)) end - it 'makes a successful request' do - provider.class.stubs(:openstack) - .with('project', 'list', '--quiet', '--format', 'csv', ['--long']) - .returns('"ID","Name","Description","Enabled" -"1cb05cfed7c24279be884ba4f6520262","test","Test tenant",True -') + it 'makes a successful list request' do + provider.class.expects(:openstack) + .with('project', 'list', '--quiet', '--format', 'csv', ['--long']) + .returns list_data response = Puppet::Provider::Openstack.request('project', 'list', ['--long']) - expect(response.first[:description]).to eq("Test tenant") + expect(response.first[:description]).to eq 'Test tenant' + end + + it 'makes a successful show request' do + provider.class.expects(:openstack) + .with('project', 'show', '--format', 'shell', ['1cb05cfed7c24279be884ba4f6520262']) + .returns show_data + response = Puppet::Provider::Openstack.request('project', 'show', ['1cb05cfed7c24279be884ba4f6520262']) + expect(response[:description]).to eq 'Test tenant' + end + + it 'makes a successful set request' do + provider.class.expects(:openstack) + .with('project', 'set', ['--name', 'new name', '1cb05cfed7c24279be884ba4f6520262']) + .returns '' + response = Puppet::Provider::Openstack.request('project', 'set', ['--name', 'new name', '1cb05cfed7c24279be884ba4f6520262']) + expect(response).to eq '' + end + + it 'uses provided credentials' do + Puppet::Util.expects(:withenv).with(credentials.to_env) + Puppet::Provider::Openstack.request('project', 'list', ['--long'], credentials) end context 'on connection errors' do - it 'retries' do - ENV['OS_USERNAME'] = 'test' - ENV['OS_PASSWORD'] = 'abc123' - ENV['OS_PROJECT_NAME'] = 'test' - ENV['OS_AUTH_URL'] = 'http://127.0.0.1:5000' + it 'retries the failed command' do provider.class.stubs(:openstack) - .with('project', 'list', '--quiet', '--format', 'csv', ['--long']) - .raises(Puppet::ExecutionFailure, 'Unable to establish connection') - .then - .returns('') - provider.class.expects(:sleep).with(2).returns(nil) - Puppet::Provider::Openstack.request('project', 'list', ['--long']) + .with('project', 'list', '--quiet', '--format', 'csv', ['--long']) + .raises(Puppet::ExecutionFailure, 'Unable to establish connection') + .then + .returns list_data + provider.class.expects(:sleep).with(3).returns(nil) + response = Puppet::Provider::Openstack.request('project', 'list', ['--long']) + expect(response.first[:description]).to eq 'Test tenant' end + + it 'fails after the timeout' do + provider.class.expects(:openstack) + .with('project', 'list', '--quiet', '--format', 'csv', ['--long']) + .raises(Puppet::ExecutionFailure, 'Unable to establish connection') + .times(3) + provider.class.stubs(:sleep) + provider.class.stubs(:current_time) + .returns(0, 10, 10, 20, 20, 100, 100) + expect do + Puppet::Provider::Openstack.request('project', 'list', ['--long']) + end.to raise_error Puppet::ExecutionFailure, /Unable to establish connection/ + end + + it 'does not retry non-idempotent commands' do + provider.class.expects(:openstack) + .with('project', 'create', '--format', 'shell', ['--quiet']) + .raises(Puppet::ExecutionFailure, 'Unable to establish connection') + .then + .returns list_data + provider.class.expects(:sleep).never + expect do + Puppet::Provider::Openstack.request('project', 'create', ['--quiet']) + end.to raise_error Puppet::ExecutionFailure, /Unable to establish connection/ + end + end context 'catch unauthorized errors' do @@ -85,7 +154,7 @@ describe Puppet::Provider::Openstack do csv = Puppet::Provider::Openstack.parse_csv(text) it 'should ignore non-CSV text at the beginning of the input' do expect(csv).to be_kind_of(Array) - expect(csv[0]).to match_array(['field', 'test', '1', '2', '3']) + expect(csv[0]).to match_array(%w(field test 1 2 3)) expect(csv.size).to eq(1) end end @@ -95,7 +164,7 @@ describe Puppet::Provider::Openstack do csv = Puppet::Provider::Openstack.parse_csv(text) it 'ignore the carriage returns' do expect(csv).to be_kind_of(Array) - expect(csv[0]).to match_array(['field', 'test', '1', '2', '3']) + expect(csv[0]).to match_array(%w(field test 1 2 3)) expect(csv.size).to eq(1) end end