Use native Puppet instead of the retries Gem in the CLI provider, replacing try_sleep parameter by exponential backoff #904
Conversation
| @@ -478,15 +467,15 @@ | |||
|
|
|||
| context 'network failure' do | |||
| context 'without ssh_private_key' do | |||
| CLI_NET_ERRORS.each do |error| | |||
| CLI_AUTH_ERRORS.each do |error| | |||
| it 'does not retry cli on AuthError exception' do | |||
There was a problem hiding this comment.
In this test I went with this description of what it was supposed to do rather than the implementation.
|
I like it, but does it create any breaking changes? |
|
Maybe. From a user perspective the max total sleep time is ignored and that's a change. Perhaps we should go further and actually remove the parameter from the types. That would be a breaking change, but more honest. |
ekohl
force-pushed
the
remove-retries-gem
branch
2 times, most recently
from
31e388d
to
154cd91
Compare
|
Dear @ekohl, thanks for the PR! This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase? You can find my sourcecode at voxpupuli/vox-pupuli-tasks |
|
Dear @ekohl, thanks for the PR! This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase? You can find my sourcecode at voxpupuli/vox-pupuli-tasks |
ekohl
force-pushed
the
remove-retries-gem
branch
2 times, most recently
from
b815827
to
f7929bd
Compare
lib/puppet/x/jenkins/provider/cli.rb
Outdated
| Puppet.debug("#{sname} caught #{exception.class.to_s.match(%r{::([^:]+)$})[1]}; retry attempt #{attempt_number}") | ||
| if attempt < cli_tries | ||
| attempt += 1 | ||
| sleep(cli_try_sleep) |
There was a problem hiding this comment.
we could even try some exponential back-off here!
| @@ -27,7 +27,6 @@ def self.inherited(subclass) | |||
| initvars | |||
|
|
|||
| commands java: 'java' | |||
| confine feature: :retries | |||
There was a problem hiding this comment.
why is that feature now gone?
does the below function not provide that feature?
There was a problem hiding this comment.
Previously the feature was provided by the gem. Now it uses functionality built into Puppet itself.
There was a problem hiding this comment.
you mean Ruby?
retry is a Ruby keyword (that i only learned about yesterday)
ekohl
force-pushed
the
remove-retries-gem
branch
2 times, most recently
from
19d6187
to
76ee2bc
Compare
ekohl
force-pushed
the
remove-retries-gem
branch
from
d7e6c5a
to
a504ae2
Compare
ekohl
force-pushed
the
remove-retries-gem
branch
2 times, most recently
from
12a2a49
to
da4283e
Compare
|
It looks like the tests are failing because something doesn't pass. With the exponential backoff I think it takes too long. Probably need to tune the 30 retries a bit. |
I think 10 retries is a good number. 30 retries is about 3,4 years which is a bit excessive. Edit: looks like the default is already 10. Going to tune it down to 9 and see if that's better. |
ekohl
force-pushed
the
remove-retries-gem
branch
from
da4283e
to
4d859cd
Compare
|
It still times out where Ubuntu was previously green. I created #986 so we don't need pending anymore. |
ekohl
force-pushed
the
remove-retries-gem
branch
from
4d859cd
to
f7d7b2c
Compare
ekohl
force-pushed
the
remove-retries-gem
branch
from
f7d7b2c
to
619371b
Compare
evgeni
force-pushed
the
remove-retries-gem
branch
3 times, most recently
from
154451a
to
ebd0035
Compare
|
No need to fix tests if you fix the changed default value back to the default the spec tests expect. |
This removes the retries gem in favor of the built in RetryAction. This does swap the parameter cli_try_sleep for the built in exponential backoff. It also raises a RetriesExceeded exception instead of the original exception. Since those aren't caught explicitly, that's not really a huge difference. RetryAction logs at the info level which exception has been caught.
evgeni
force-pushed
the
remove-retries-gem
branch
from
ebd0035
to
ec80583
Compare
evgeni
changed the title
Retries is only used by the CLI providers. Puppet itself also provides retry functionality. This avoids the need to ensure a gem is installed in the right environment, which may not even work if it's a disconnected environment.
This does ignore a parameter for the maximum time to sleep. I'm looking for feedback on how to best deal with this.