Merge pull request #904 from ekohl/remove-retries-gem

Use native Puppet instead of the retries Gem in the CLI provider, replacing try_sleep parameter by exponential backoff

.sync.yml

@@ -1,7 +1,3 @@
 ---
-Gemfile:
-  optional:
-    ':test':
-      - gem: 'retries'
 .rubocop.yml:
   unmanaged: true

Gemfile

@@ -8,7 +8,6 @@ group :test do
   gem 'coveralls',                  :require => false
   gem 'simplecov-console',          :require => false
   gem 'puppet_metadata', '~> 3.5',  :require => false
-  gem 'retries',                    :require => false
 end
 
 group :development do

NATIVE_TYPES_AND_PROVIDERS.md

@@ -121,8 +121,6 @@ class { '::jenkins':
 include ::jenkins::cli_helper
 ```
 
-The ruby gem `retries` is presently required by all providers.
-
 ### `puppetserver`
 
 There is a known issue with `puppetserver` being unable to load code from
@@ -141,13 +139,6 @@ jruby-puppet: {
 
 See [SERVER-973](https://tickets.puppetlabs.com/browse/SERVER-973)
 
-Additionally, the `retries` gem is required.  This may be installed on the master by running:
-
-```
-/opt/puppetlabs/bin/puppetserver gem install retries
-```
-
-
 Types
 --
 

lib/puppet/x/jenkins/config.rb

@@ -18,7 +18,6 @@ class UnknownConfig < ArgumentError; end
     ssh_private_key: nil,
     puppet_helper: '/usr/share/java/puppet_helper.groovy',
     cli_tries: 30,
-    cli_try_sleep: 2,
     cli_username: nil,
     cli_password: nil,
     cli_password_file: '/tmp/jenkins_credentials_for_puppet',

lib/puppet/x/jenkins/provider/cli.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require 'puppet/provider'
+require 'puppet/util/retry_action'
 require 'facter'
 
 require 'json'
@@ -31,7 +32,6 @@ def self.inherited(subclass) # rubocop:todo Lint/MissingSuper
   initvars
 
   commands java: 'java'
-  confine feature: :retries
 
   # subclasses should inherit this value once it has been determined that
   # jenkins requires authorization, it shortens the run time be elemating the
@@ -170,7 +170,6 @@ def self.execute_with_retry(command, options = {}, cli_pre_cmd = [])
     url                      = config[:url]
     ssh_private_key          = config[:ssh_private_key]
     cli_tries                = config[:cli_tries]
-    cli_try_sleep            = config[:cli_try_sleep]
     cli_username             = config[:cli_username]
     cli_password             = config[:cli_password]
     cli_password_file        = config[:cli_password_file]
@@ -202,16 +201,7 @@ def self.execute_with_retry(command, options = {}, cli_pre_cmd = [])
     # retry on "unknown" execution errors but don't catch AuthErrors.  If an
     # AuthError has bubbled up to this level it means either an ssh_private_key
     # is required and we don't have one or that one we have was rejected.
-    handler = proc do |exception, attempt_number, total_delay|
-      Puppet.debug("#{sname} caught #{exception.class.to_s.match(%r{::([^:]+)$})[1]}; retry attempt #{attempt_number}; #{total_delay.round(3)} seconds have passed")
-    end
-    with_retries(
-      max_tries: cli_tries,
-      base_sleep_seconds: 1,
-      max_sleep_seconds: cli_try_sleep,
-      rescue: [UnknownError, NetError],
-      handler: handler
-    ) do
+    Puppet::Util::RetryAction.retry_action(retries: cli_tries, retry_exceptions: [UnknownError, NetError]) do
       result = execute_with_auth(cli_cmd, auth_cmd, options)
       Puppet.debug("#{sname} command stdout:\n#{result}") unless result == ''
       return result
@@ -262,7 +252,8 @@ def self.execute_exceptionify(cmd, options)
       'SEVERE: I/O error in channel CLI connection',
       'java.net.SocketException: Connection reset',
       'java.net.ConnectException: Connection refused',
-      'java.io.IOException: Failed to connect'
+      'java.io.IOException: Failed to connect',
+      'java.io.IOException: Server returned HTTP response code: 503'
     ]
 
     if options.key?(:tmpfile_as_param)

manifests/cli/config.pp

@@ -17,28 +17,10 @@
   Boolean $cli_password_file_exists               = false,
   Optional[String] $ssh_private_key_content       = undef,
 ) {
-  if str2bool($facts['is_pe']) {
-    $gem_provider = 'pe_gem'
-    # lint:ignore:legacy_facts
-  } elsif $facts['rubysitedir'] and ('/opt/puppetlabs/puppet/lib/ruby' in $facts['rubysitedir']) {
-    # lint:endignore
-    # AIO puppet
-    $gem_provider = 'puppet_gem'
-  } else {
-    $gem_provider = 'gem'
-  }
-
   # lint:ignore:legacy_facts
   $is_root = $facts['id'] == 'root'
   # lint:endignore
 
-  # required by PuppetX::Jenkins::Provider::Clihelper base
-  if ! defined(Package['retries']) {
-    package { 'retries':
-      provider => $gem_provider,
-    }
-  }
-
   if $ssh_private_key and $ssh_private_key_content {
     file { $ssh_private_key:
       ensure  => 'file',

spec/classes/cli/config_spec.rb

@@ -126,26 +126,6 @@
           end
         end
       end
-
-      describe 'package gem provider' do
-        context 'is_pe fact' do
-          context 'true' do
-            let :facts do
-              super().merge(is_pe: true)
-            end
-
-            it { is_expected.to contain_package('retries').with(provider: 'pe_gem') }
-          end
-
-          context 'false' do
-            let :facts do
-              super().merge(is_pe: false)
-            end
-
-            it { is_expected.to contain_package('retries').with(provider: 'gem') }
-          end
-        end
-      end
     end
   end
 end

spec/unit/puppet/x/jenkins/config_spec.rb

@@ -10,8 +10,7 @@
     url: 'http://localhost:8080',
     ssh_private_key: nil,
     puppet_helper: '/usr/share/java/puppet_helper.groovy',
-    cli_tries: 30,
-    cli_try_sleep: 2
+    cli_tries: 30
   }.freeze
 
   shared_context 'facts' do
@@ -21,7 +20,6 @@
       Facter.add(:jenkins_ssh_private_key) { setcode { 'fact.id_rsa' } }
       Facter.add(:jenkins_puppet_helper) { setcode { 'fact.groovy' } }
       Facter.add(:jenkins_cli_tries) { setcode { 22 } }
-      Facter.add(:jenkins_cli_try_sleep) { setcode { 33 } }
     end
   end
 
@@ -126,8 +124,7 @@
               url: 'http://localhost:111',
               ssh_private_key: 'cat.id_rsa',
               puppet_helper: 'cat.groovy',
-              cli_tries: 222,
-              cli_try_sleep: 333
+              cli_tries: 222
             )
 
             catalog.add_resource jenkins

spec/unit/puppet/x/jenkins/provider/cli_spec.rb

@@ -3,9 +3,6 @@
 require 'spec_helper'
 require 'unit/puppet/x/spec_jenkins_providers'
 
-# we need to make sure retries is always loaded or random test ordering can
-# cause failures when a side effect hasn't yet caused the lib to be loaded
-require 'retries'
 require 'puppet/x/jenkins/provider/cli'
 
 describe Puppet::X::Jenkins::Provider::Cli do
@@ -38,7 +35,6 @@
       Facter.add(:jenkins_puppet_helper) { setcode { 'fact.groovy' } }
       Facter.add(:jenkins_cli_username) { setcode { 'myuser' } }
       Facter.add(:jenkins_cli_tries) { setcode { 22 } }
-      Facter.add(:jenkins_cli_try_sleep) { setcode { 33 } }
     end
   end
 
@@ -316,13 +312,6 @@
   end
 
   describe '::cli' do
-    before do
-      # disable with_retries sleeping to [vastly] speed up testing
-      #
-      # we are relying the side effects of ::suitable? from a previous example
-      Retries.sleep_enabled = false
-    end
-
     shared_examples 'uses default values' do
       it 'uses default values' do
         expect(described_class.superclass).to receive(:execute).with(
@@ -400,8 +389,7 @@
             url: 'http://localhost:111',
             ssh_private_key: 'cat.id_rsa',
             cli_username: 'myuser',
-            cli_tries: 222,
-            cli_try_sleep: 333
+            cli_tries: 222
           )
 
           catalog.add_resource jenkins
@@ -423,10 +411,8 @@
       context 'without ssh_private_key' do
         CLI_AUTH_ERRORS.each do |error|
           it 'does not retry cli on AuthError exception' do
-            expect(described_class.superclass).to receive(:execute).with(
-              'java -jar /usr/share/java/jenkins-cli.jar -s http://localhost:8080 -logger WARNING foo',
-              failonfail: true, combine: true
-            ).and_raise(AuthError, error)
+            expect(described_class).to receive(:execute_with_auth).once.and_raise(AuthError, error)
+            expect(Puppet::Util::RetryAction).not_to receive(:sleep)
 
             expect { described_class.cli('foo') }.
               to raise_error(AuthError)
@@ -490,14 +476,12 @@
     context 'network failure' do
       context 'without ssh_private_key' do
         CLI_NET_ERRORS.each do |error|
-          it 'does not retry cli on AuthError exception' do
-            expect(described_class.superclass).to receive(:execute).with(
-              'java -jar /usr/share/java/jenkins-cli.jar -s http://localhost:8080 -logger WARNING foo',
-              failonfail: true, combine: true
-            ).exactly(30).times.and_raise(NetError, error)
+          it 'retries cli on NetError exception' do
+            expect(described_class).to receive(:execute_with_auth).exactly(31).times.and_raise(NetError, error)
+            expect(Puppet::Util::RetryAction).to receive(:sleep).exactly(30).times
 
             expect { described_class.cli('foo') }.
-              to raise_error(NetError)
+              to raise_error(Puppet::Util::RetryAction::RetryException::RetriesExceeded)
           end
         end
       end
@@ -514,10 +498,7 @@
           )
           catalog.add_resource jenkins
 
-          expect(described_class.superclass).to receive(:execute).with(
-            'java -jar /usr/share/java/jenkins-cli.jar -s http://localhost:8080 -logger WARNING foo',
-            failonfail: true, combine: true
-          ).exactly(30).times.and_raise(UnknownError, 'foo')
+          expect(Puppet::Util::RetryAction).to receive(:retry_action).with(retries: 30, retry_exceptions: [UnknownError, NetError]).and_raise(UnknownError, 'foo')
 
           expect { described_class.cli('foo', catalog: catalog) }.
             to raise_error(UnknownError, 'foo')
@@ -530,10 +511,7 @@
           )
           catalog.add_resource jenkins
 
-          expect(described_class.superclass).to receive(:execute).with(
-            'java -jar /usr/share/java/jenkins-cli.jar -s http://localhost:8080 -logger WARNING foo',
-            failonfail: true, combine: true
-          ).twice.and_raise(UnknownError, 'foo')
+          expect(Puppet::Util::RetryAction).to receive(:retry_action).with(retries: 2, retry_exceptions: [UnknownError, NetError]).and_raise(UnknownError, 'foo')
 
           expect { described_class.cli('foo', catalog: catalog) }.
             to raise_error(UnknownError, 'foo')
@@ -547,10 +525,7 @@
           )
           catalog.add_resource jenkins
 
-          expect(described_class.superclass).to receive(:execute).with(
-            'java -jar /usr/share/java/jenkins-cli.jar -s http://localhost:8080 -logger WARNING foo',
-            failonfail: true, combine: true
-          ).exactly(3).times.and_raise(UnknownError, 'foo')
+          expect(Puppet::Util::RetryAction).to receive(:retry_action).with(retries: 3, retry_exceptions: [UnknownError, NetError]).and_raise(UnknownError, 'foo')
 
           expect { described_class.cli('foo', catalog: catalog) }.
             to raise_error(UnknownError, 'foo')
@@ -565,69 +540,12 @@
           )
           catalog.add_resource jenkins
 
-          expect(described_class.superclass).to receive(:execute).with(
-            'java -jar /usr/share/java/jenkins-cli.jar -s http://localhost:8080 -logger WARNING foo',
-            failonfail: true, combine: true
-          ).twice.and_raise(UnknownError, 'foo')
+          expect(Puppet::Util::RetryAction).to receive(:retry_action).with(retries: 2, retry_exceptions: [UnknownError, NetError]).and_raise(UnknownError, 'foo')
 
           expect { described_class.cli('foo', catalog: catalog) }.
             to raise_error(UnknownError, 'foo')
         end
       end
-
-      context 'waiting up to n seconds' do
-        # this isn't behavioral testing because we don't want to either wait
-        # for the wallclock delay timeout or attempt to accurate time examples
-        it 'by default' do
-          jenkins = Puppet::Type.type(:component).new(
-            name: 'jenkins::cli::config'
-          )
-          catalog.add_resource jenkins
-
-          expect(described_class).to receive(:with_retries).with(hash_including(max_sleep_seconds: 2))
-
-          described_class.cli('foo', catalog: catalog)
-        end
-
-        it 'from catalog value' do
-          jenkins = Puppet::Type.type(:component).new(
-            name: 'jenkins::cli::config',
-            cli_try_sleep: 3
-          )
-          catalog.add_resource jenkins
-
-          expect(described_class).to receive(:with_retries).with(hash_including(max_sleep_seconds: 3))
-
-          described_class.cli('foo', catalog: catalog)
-        end
-
-        it 'from fact' do
-          Facter.add(:jenkins_cli_try_sleep) { setcode { 4 } }
-
-          jenkins = Puppet::Type.type(:component).new(
-            name: 'jenkins::cli::config'
-          )
-          catalog.add_resource jenkins
-
-          expect(described_class).to receive(:with_retries).with(hash_including(max_sleep_seconds: 4))
-
-          described_class.cli('foo', catalog: catalog)
-        end
-
-        it 'from catalog overriding fact' do
-          Facter.add(:jenkins_cli_try_sleep) { setcode { 4 } }
-
-          jenkins = Puppet::Type.type(:component).new(
-            name: 'jenkins::cli::config',
-            cli_try_sleep: 3
-          )
-          catalog.add_resource jenkins
-
-          expect(described_class).to receive(:with_retries).with(hash_including(max_sleep_seconds: 3))
-
-          described_class.cli('foo', catalog: catalog)
-        end
-      end
     end
 
     context 'options with :stdinjson' do

spec/unit/puppet/x/spec_jenkins_providers.rb

@@ -8,19 +8,6 @@
       described_class.confine_collection.instance_variable_get(:@confines)
     end
 
-    it 'has no matched confines' do
-      expect(described_class.confine_collection.summary).to eq({})
-    end
-
-    context 'feature :retries' do
-      it do
-        expect(confines).to include(
-          be_a(Puppet::Confine::Feature).
-          and(have_attributes(values: [:retries]))
-        )
-      end
-    end
-
     context 'commands :java' do
       it do
         expect(confines).to include(