Merge "Add retries to the openstack command"

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 = current_time + request_timeout
-      end_time = Time.now.to_i + 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
+        rescue Puppet::ExecutionFailure => exception
-          if e.message =~ /HTTP 40[13]/
+          raise Puppet::Error::OpenstackUnauthorizedError, 'Could not authenticate' if exception.message =~ /HTTP 40[13]/
-            raise(Puppet::Error::OpenstackUnauthorizedError, 'Could not authenticate.')
+          raise exception if current_time > end_time
-          elsif e.message =~ /Unable to establish connection/
+          debug "Non-fatal error: '#{exception.message}'. Retrying for #{end_time - current_time} more seconds"
-            current_time = Time.now.to_i
+          raise exception if no_retry_actions.include? action
-            if current_time > end_time
+          sleep retry_sleep
-              break
+          retry
-            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
         end
       end
       return rv

spec/unit/provider/openstack_spec.rb

@@ -17,6 +17,33 @@ 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
       {
@@ -28,30 +55,72 @@ describe Puppet::Provider::Openstack do
       Puppet::Provider::Openstack.new(type.new(resource_attrs))
     end
 
-    it 'makes a successful request' do
+    it 'makes a successful list request' do
-      provider.class.stubs(:openstack)
+      provider.class.expects(:openstack)
           .with('project', 'list', '--quiet', '--format', 'csv', ['--long'])
-                    .returns('"ID","Name","Description","Enabled"
+          .returns list_data
-"1cb05cfed7c24279be884ba4f6520262","test","Test tenant",True
-')
       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
+      it 'retries the failed command' do
-        ENV['OS_USERNAME']     = 'test'
-        ENV['OS_PASSWORD']     = 'abc123'
-        ENV['OS_PROJECT_NAME'] = 'test'
-        ENV['OS_AUTH_URL']     = 'http://127.0.0.1:5000'
         provider.class.stubs(:openstack)
             .with('project', 'list', '--quiet', '--format', 'csv', ['--long'])
             .raises(Puppet::ExecutionFailure, 'Unable to establish connection')
             .then
-                      .returns('')
+            .returns list_data
-        provider.class.expects(:sleep).with(2).returns(nil)
+        provider.class.expects(:sleep).with(3).returns(nil)
-        Puppet::Provider::Openstack.request('project', 'list', ['--long'])
+        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