[FIXED JENKINS-30564] #46

fbelzunc · 2015-09-21T12:16:18Z

timeoutMinutesElasticDefault should be used until numberOfBuilds is not reached out.

https://issues.jenkins-ci.org/browse/JENKINS-30564

@reviewbybees

…ot reached out

ghost · 2015-09-21T12:19:41Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

daniel-beck · 2015-09-21T12:36:47Z

🐛 PR does not update existing relevant help texts to point out this special behavior.

Besides that implementation issue, this change does not have a firm rationale. It assumes a user configures elastic build timeout before the job even reached a stable behavior and execution time, which makes no sense.

OTOH users with aggressive deletion rules and/or frequently failing builds may be impacted by this change although they'd like the existing behavior: If you only keep 20 builds around due to e.g. storage limitations, but would like the elastic timeout to consider all of those that were successful, this change will break your existing timeout logic you rely on.

fbelzunc · 2015-09-21T12:50:20Z

PR does not update existing relevant help texts to point out this special behavior.

To do this is not a problem. It can be easily implemented.

Besides that implementation issue, this change does not have a firm rationale. It assumes a user configures elastic build timeout before the job even reached a stable behaviour and execution time, which makes no sense.

Is not there a similar assumption done with the standard behaviour when there is not already a successful build?

OTOH users with aggressive deletion rules and/or frequently failing builds may be impacted by this change although they'd like the existing behavior: If you only keep 20 builds around due to e.g. storage limitations, but would like the elastic timeout to consider all of those that were successful, this change will break your existing timeout logic you rely on.

For me this is the work timeoutMinutesElasticDefault will be doing. You should set-up the right value here to avoid any issue.

I understand it might have an impact, but I think this change is what an user which start with this plugin will expect. IMHO the current behaviour is not really expected unless you dig into the code.

daniel-beck · 2015-09-21T13:11:13Z

@fbelzunc To clarify, the 🐛 is about the incomplete implementation only. The second and third paragraph stand alone as comment. Although I really would prefer to see an actual rationale for this change. So far you only assert that the current behavior is wrong somehow.

ikedam · 2015-09-22T02:24:37Z

I don't think this is a good way to resolve the problem pointed in JENKINS-30564.
I prefer to add a checkbox "use this as the shortest timeout" to "Timeout minutes".

I think JENKINS-30564 is caused not for too few builds, but rather for that the project configuration is updated and the expected build duration gets longer. This can happen even when there're sufficient number of builds, and this request doesn't resolve those cases.

Though I don't think ElasticTimeOutStrategy is applicable to those cases anyway, allowing users configure fail-safe timeout duration is a better approach.

ikedam · 2015-10-07T23:48:28Z

How's work going?

fbelzunc · 2015-10-23T08:57:53Z

@ikedam I like your approach. I have just started to work on it.

fbelzunc · 2015-10-23T10:41:12Z

@ikedam @daniel-beck Would you mind to take a look at this?

ikedam · 2015-10-23T13:17:16Z

src/main/java/hudson/plugins/build_timeout/impl/ElasticTimeOutStrategy.java

@@ -22,6 +22,8 @@

    private final String numberOfBuilds;

+    private boolean failSafeTimeoutDuration;


fbelzunc · 2015-10-26T08:55:37Z

@ikedam What do you think now?

ikedam · 2015-10-26T09:55:24Z

src/main/java/hudson/plugins/build_timeout/impl/ElasticTimeOutStrategy.java

@@ -68,9 +81,17 @@ public long getTimeOut(AbstractBuild<?, ?> build, BuildListener listener)
            throws InterruptedException, MacroEvaluationException, IOException {
        double elasticTimeout = getElasticTimeout(Integer.parseInt(expandAll(build,listener,getTimeoutPercentage())), build, listener);
        if (elasticTimeout == 0) {
-            return Math.max(MINIMUM_TIMEOUT_MILLISECONDS, Integer.parseInt(expandAll(build, listener, getTimeoutMinutesElasticDefault())) * MINUTES);
+            if (isFailSafeTimeoutDuration()) {
+                return Math.max(Integer.parseInt(getTimeoutMinutesElasticDefault()) * 60L * 1000L, Integer.parseInt(expandAll(build, listener, getTimeoutMinutesElasticDefault())) * MINUTES);


This results timeoutMinutesElasticDefault anyway.
I don't think we need this if switch.

I agree with this.

fbelzunc · 2015-10-26T11:29:17Z

@ikedam I think I did all in the way you said. Would you mind to review it again?

ikedam · 2015-10-26T12:07:07Z

Looks good.
I want unit tests befere merging.
Do you plan to add them?
If not, I can do that instead.

fbelzunc · 2015-10-26T13:10:00Z

Let me give a try.

fbelzunc · 2015-10-27T08:44:22Z

@ikedam Tests added.

Do you think they are ok?

ikedam · 2015-10-27T09:54:54Z

I also want following test cases:

A case failSafe... is set to true and the default value is not used (the calcurated timeout is enough long).
A case the default value is configured with variables.

The latter case may be a little hard to implement as variables are not used in existing tests.
I suppose you can do that by adding ParametersAction to the build. See also StringParameterValue.

ikedam · 2016-01-17T04:14:39Z

I added some tests and created #49.
Would you review that?

fbelzunc · 2016-01-19T07:01:01Z

Yes, I just did it.

Felix Belzunce Arcos added 2 commits September 21, 2015 14:12

timeoutMinutesElasticDefault should be used until numberOfBuilds is n…

ea2bc00

…ot reached out

Refractor code

1555384

Refractor code

650d8a5

Add failSafeTimeoutDuration checkbox

2152cae

ikedam reviewed Oct 23, 2015
View reviewed changes

Add some minor changes

a07b3f2

ikedam reviewed Oct 26, 2015
View reviewed changes

Add some minor changes

cdcc95c

Add tests for the FailSafeTimeoutDuration feature

b972c30

ikedam mentioned this pull request Jan 17, 2016

[JENKINS-30564] Failsafe timeout duration for ElasticTimeoutStrategy #49

Merged

fbelzunc closed this Jan 19, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIXED JENKINS-30564] #46

[FIXED JENKINS-30564] #46

fbelzunc commented Sep 21, 2015

ghost commented Sep 21, 2015

daniel-beck commented Sep 21, 2015

fbelzunc commented Sep 21, 2015

daniel-beck commented Sep 21, 2015

ikedam commented Sep 22, 2015

ikedam commented Oct 7, 2015

fbelzunc commented Oct 23, 2015

fbelzunc commented Oct 23, 2015

ikedam Oct 23, 2015

fbelzunc commented Oct 26, 2015

ikedam Oct 26, 2015

fbelzunc Oct 26, 2015

fbelzunc commented Oct 26, 2015

ikedam commented Oct 26, 2015

fbelzunc commented Oct 26, 2015

fbelzunc commented Oct 27, 2015

ikedam commented Oct 27, 2015

ikedam commented Jan 17, 2016

fbelzunc commented Jan 19, 2016

		@@ -22,6 +22,8 @@

		private final String numberOfBuilds;

		private boolean failSafeTimeoutDuration;

[FIXED JENKINS-30564] #46

[FIXED JENKINS-30564] #46

Conversation

fbelzunc commented Sep 21, 2015

ghost commented Sep 21, 2015

daniel-beck commented Sep 21, 2015

fbelzunc commented Sep 21, 2015

daniel-beck commented Sep 21, 2015

ikedam commented Sep 22, 2015

ikedam commented Oct 7, 2015

fbelzunc commented Oct 23, 2015

fbelzunc commented Oct 23, 2015

ikedam Oct 23, 2015

Choose a reason for hiding this comment

fbelzunc commented Oct 26, 2015

ikedam Oct 26, 2015

Choose a reason for hiding this comment

fbelzunc Oct 26, 2015

Choose a reason for hiding this comment

fbelzunc commented Oct 26, 2015

ikedam commented Oct 26, 2015

fbelzunc commented Oct 26, 2015

fbelzunc commented Oct 27, 2015

ikedam commented Oct 27, 2015

ikedam commented Jan 17, 2016

fbelzunc commented Jan 19, 2016