Honor SCM checkout retry count #110
Conversation
|
@jglick Could you take a look? |
| throw new AbortException("Maximum checkout retry attempts reached, aborting"); | ||
|
|
||
| listener.getLogger().println("Retrying after 10 seconds"); | ||
| Thread.sleep(10000); |
There was a problem hiding this comment.
This does not look right to me, should probably do something like what the sleep step does instead of blocking the thread here. Though I am not sure atm how a SynchronousNonBlockingStepExecution is supposed to behave in these situations.
There was a problem hiding this comment.
Well, a background thread will sleep. Not terrible, under the assumption this code path is rarely used.
There was a problem hiding this comment.
Shall I keep this change? Or, do I need to rewrite the whole StepExecutionImpl?
There was a problem hiding this comment.
Probably good enough as is. This step used SynchronousNonBlockingStepExecution (or the equivalent) from the beginning since the SCM interface in Jenkins does not support durable execution, so unlike
sh 'git clone https://github.com/myorg/myrepo'a slow checkout cannot survive a controller restart. As of jenkinsci/workflow-basic-steps-plugin#203 this matters less anyway since you can use the new retry system with the nonresumable condition to handle that case.
| if (retryCount == 0) // all attempts failed | ||
| throw new AbortException("Maximum checkout retry attempts reached, aborting"); |
There was a problem hiding this comment.
Ideally we would throw up the original exception if this is the last attempt, rather than constructing a fresh exception.
| throw new AbortException("Maximum checkout retry attempts reached, aborting"); | ||
|
|
||
| listener.getLogger().println("Retrying after 10 seconds"); | ||
| Thread.sleep(10000); |
There was a problem hiding this comment.
Probably good enough as is. This step used SynchronousNonBlockingStepExecution (or the equivalent) from the beginning since the SCM interface in Jenkins does not support durable execution, so unlike
sh 'git clone https://github.com/myorg/myrepo'a slow checkout cannot survive a controller restart. As of jenkinsci/workflow-basic-steps-plugin#203 this matters less anyway since you can use the new retry system with the nonresumable condition to handle that case.
|
@topikachu mind rebasing this PR? I rebased here locally and tests are still passing. Also, the failure in CI was due to some dependency download issue. |
|
@rsandell @jglick would you accept this PR as is? I can rebase and send as another (or you can do as well, there's no conflict and tests are passing). This feature is of extreme importance for me and saves countless hours of rebuilding pipelines that had just failed at the very beginning at cloning due to some intermittency. I am running this hpi in my Jenkins for a week now and everything seems ok. |
|
Thanks a ton! |
|
A small note, I noticed the retry functionality is catching the It should not, otherwise when we press the Abort button for the build and the build is still cloning, the abort will be ignored, the retry will resume the clone operation, and the build will continue as if it was not aborted. |
|
@felipecrs then something is failing to check https://javadoc.jenkins.io/plugin/workflow-step-api/org/jenkinsci/plugins/workflow/steps/FlowInterruptedException.html#isActualInterruption() I think. |
| break; | ||
| } catch (InterruptedIOException e) { | ||
| throw e; | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
This block should include:
if (e instanceof FlowInterruptedException && ((FlowInterruptedException)e).isActualInterruption()) {
throw e;
}
Honor SCM checkout retry count
Inspired by jenkinsci/workflow-cps-plugin#147