Added minimum uphours feature #377

snallami · 2019-07-06T00:51:40Z

Added minimum uphours feature so that agent stays up for specified minimum hours before considering for termination. Once agent stayed for minimum number of hours other rules (like idle termination time) will get evaluated.

Why this feature is important?

Main use cases.

Avoid agent spin up time. Windows agents in AWS takes somewhere around 10 to 20 minutes to spin up and without minimum uptime feature we see that builds are waiting longer for agent to be available. Linus agents spin up time is faster though compared to windows.
Warm up ephemeral agents with caches(maven or npm or any other) so that actual developer builds can be faster and with the help of minimum uphours we can retain agent for 8 to 10 hours which helps in faster builds during development hours or working hours.
Reserve capacity upfront for pre planned work.
No need to depend on static agent for cache warmup.

And this is optional feature

res0nance

Could you also write a test?

res0nance · 2019-07-07T16:25:03Z

src/main/java/hudson/plugins/ec2/EC2OndemandSlave.java

@@ -32,29 +32,29 @@
    private static final Logger LOGGER = Logger.getLogger(EC2OndemandSlave.class.getName());

    @Deprecated
-    public EC2OndemandSlave(String instanceId, String description, String remoteFS, int numExecutors, String labelString, Mode mode, String initScript, String tmpDir, String remoteAdmin, String jvmopts, boolean stopOnTerminate, String idleTerminationMinutes, String publicDNS, String privateDNS, List<EC2Tag> tags, String cloudName, int launchTimeout, AMITypeData amiType)
+    public EC2OndemandSlave(String instanceId, String description, String remoteFS, int numExecutors, String labelString, Mode mode, String initScript, String tmpDir, String remoteAdmin, String jvmopts, boolean stopOnTerminate, String idleTerminationMinutes, String publicDNS, String privateDNS, List<EC2Tag> tags, String cloudName, int launchTimeout, AMITypeData amiType, String minUpHours)


Should not add the new option to deprecated constructors

let me see if i can bypass them

res0nance · 2019-07-07T16:28:15Z

src/main/java/hudson/plugins/ec2/SlaveTemplate.java

+
+    public String getMinUpHours() {
+ 		return minUpHours;
+ 	}


Mind the indentation

hmmm , not sure why my eclipse is messing up formatting only for this line. Let me use text editor in next commit

res0nance · 2019-07-07T16:30:06Z

src/main/java/hudson/plugins/ec2/EC2SpotSlave.java

@@ -33,23 +33,23 @@
    private final String spotInstanceRequestId;

    @Deprecated
-    public EC2SpotSlave(String name, String spotInstanceRequestId, String description, String remoteFS, int numExecutors, Mode mode, String initScript, String tmpDir, String labelString, String remoteAdmin, String jvmopts, String idleTerminationMinutes, List<EC2Tag> tags, String cloudName, int launchTimeout, AMITypeData amiType)
+    public EC2SpotSlave(String name, String spotInstanceRequestId, String description, String remoteFS, int numExecutors, Mode mode, String initScript, String tmpDir, String labelString, String remoteAdmin, String jvmopts, String idleTerminationMinutes, List<EC2Tag> tags, String cloudName, int launchTimeout, AMITypeData amiType, String minUpHours)


Same here on deprecated constructors

res0nance · 2019-07-07T16:31:49Z

src/main/java/hudson/plugins/ec2/SlaveTemplate.java

@@ -175,7 +177,7 @@ public SlaveTemplate(String ami, String zone, SpotConfiguration spotConfig, Stri
            String instanceCapStr, String iamInstanceProfile, boolean deleteRootOnTermination,
            boolean useEphemeralDevices, boolean useDedicatedTenancy, String launchTimeoutStr, boolean associatePublicIp,
            String customDeviceMapping, boolean connectBySSHProcess, boolean monitoring,
-            boolean t2Unlimited, ConnectionStrategy connectionStrategy, int maxTotalUses) {
+            boolean t2Unlimited, ConnectionStrategy connectionStrategy, int maxTotalUses, String minUpHours) {


Modifying the ctor this way also breaks backwards compat with groovy scripts

Do you want me to add one more overloaded constructor?

Add another and deprecate the older one, is how it usually is handled. The older constructors should not take in new params and should provide a safe default. i.e the old behaviour.

Same here about the constructor. Same comment about the type (use int or Integer).

snallami · 2019-07-07T18:57:02Z

For some odd reason, I am not able to comment on review for minuphours=0 inline. Adding comment here.

If minuphours=0 then code evaluates other rule like idle termination time. See code @
https://gist.github.com/snallami/d917ec049e5d80e4206e2ce0cd2ef895

snallami · 2019-07-07T19:08:03Z

Thanks for review. I updated code as per review comments. Adding notes here as well.

If minuphours=0 then code evaluates other rule like idle termination time. See code @
https://gist.github.com/snallami/d917ec049e5d80e4206e2ce0cd2ef895

Need clarification

I am not clear about what are the next steps about review comment "Modifying the ctor this way also breaks backwards compat with groovy scripts".

Let me know if you want me to add an overloaded constructor or modify groovy scripts.

I will look into better way to add tests since check method always return 1

res0nance · 2019-07-08T02:25:43Z

A good start to tests is an old testcase that would otherwise timeout should not timeout any longer.

2. Reverted previous changes in test cases and also removed minuphours in deprecated constructors. 3. Corrected logic in retention strategy. 4. Added test case for minuphours. Not all cases are covered sine idlemilliseconds method is final in computer onject

snallami · 2019-07-14T03:50:37Z

Jenkinsfile

@@ -1 +0,0 @@
-buildPlugin(configurations: buildPlugin.recommendedConfigurations())


Not sure why this got deleted in my commit. let me submit a new commit

…dded file again

…that this turns our to me ok

jvz

Coming along nicely, thanks for the contribution! I've left some feedback on areas we need to fix before merging.

jvz · 2019-07-15T15:16:16Z

src/main/java/hudson/plugins/ec2/EC2RetentionStrategy.java

@@ -73,7 +76,7 @@
                    String.valueOf(STARTUP_TIME_DEFAULT_VALUE)), STARTUP_TIME_DEFAULT_VALUE);

    @DataBoundConstructor
-    public EC2RetentionStrategy(String idleTerminationMinutes) {
+    public EC2RetentionStrategy(String idleTerminationMinutes, String minUpHours) {


You need to keep the old constructors and mark them @Deprecated in order to preserve backward compatibility as this class is public.

Also, you should use an argument type of either int (for a required value) or Integer (for a nullable one). I'd suggest OptionalInt as an argument type for the latter, though I'm not sure if that actually works with @DataBoundConstructor.

jvz · 2019-07-15T15:17:33Z

src/main/java/hudson/plugins/ec2/EC2OndemandSlave.java

    }

    @DataBoundConstructor
-    public EC2OndemandSlave(String name, String instanceId, String description, String remoteFS, int numExecutors, String labelString, Mode mode, String initScript, String tmpDir, List<? extends NodeProperty<?>> nodeProperties, String remoteAdmin, String jvmopts, boolean stopOnTerminate, String idleTerminationMinutes, String publicDNS, String privateDNS, List<EC2Tag> tags, String cloudName, boolean useDedicatedTenancy, int launchTimeout, AMITypeData amiType, ConnectionStrategy connectionStrategy, int maxTotalUses)
+    public EC2OndemandSlave(String name, String instanceId, String description, String remoteFS, int numExecutors, String labelString, Mode mode, String initScript, String tmpDir, List<? extends NodeProperty<?>> nodeProperties, String remoteAdmin, String jvmopts, boolean stopOnTerminate, String idleTerminationMinutes, String publicDNS, String privateDNS, List<EC2Tag> tags, String cloudName, boolean useDedicatedTenancy, int launchTimeout, AMITypeData amiType, ConnectionStrategy connectionStrategy, int maxTotalUses, String minUpHours)


Same here; you need to create a new constructor and mark the old one @Deprecated.

I'll note that this sort of redundancy could be avoided by using a value object as the constructor argument rather than all of its attributes, but this would require a larger effort to fix at this point.

jvz · 2019-07-15T15:20:18Z

src/main/java/hudson/plugins/ec2/EC2SpotSlave.java

    }

    @DataBoundConstructor
-    public EC2SpotSlave(String name, String spotInstanceRequestId, String description, String remoteFS, int numExecutors, Mode mode, String initScript, String tmpDir, String labelString, List<? extends NodeProperty<?>> nodeProperties, String remoteAdmin, String jvmopts, String idleTerminationMinutes, List<EC2Tag> tags, String cloudName, int launchTimeout, AMITypeData amiType, ConnectionStrategy connectionStrategy, int maxTotalUses)
+    public EC2SpotSlave(String name, String spotInstanceRequestId, String description, String remoteFS, int numExecutors, Mode mode, String initScript, String tmpDir, String labelString, List<? extends NodeProperty<?>> nodeProperties, String remoteAdmin, String jvmopts, String idleTerminationMinutes, List<EC2Tag> tags, String cloudName, int launchTimeout, AMITypeData amiType, ConnectionStrategy connectionStrategy, int maxTotalUses, String minUpHours)


Same here, need another constructor.

jvz · 2019-07-15T15:20:51Z

src/main/java/hudson/plugins/ec2/SlaveTemplate.java

@@ -175,7 +177,7 @@ public SlaveTemplate(String ami, String zone, SpotConfiguration spotConfig, Stri
            String instanceCapStr, String iamInstanceProfile, boolean deleteRootOnTermination,
            boolean useEphemeralDevices, boolean useDedicatedTenancy, String launchTimeoutStr, boolean associatePublicIp,
            String customDeviceMapping, boolean connectBySSHProcess, boolean monitoring,
-            boolean t2Unlimited, ConnectionStrategy connectionStrategy, int maxTotalUses) {
+            boolean t2Unlimited, ConnectionStrategy connectionStrategy, int maxTotalUses, String minUpHours) {


Same here about the constructor. Same comment about the type (use int or Integer).

jvz · 2019-07-15T15:21:58Z

src/main/java/hudson/plugins/ec2/SlaveTemplate.java

@@ -140,6 +140,8 @@

    public int nextSubnet;

+    private String minUpHours;


This should be Integer for a nullable value, or int for a non-null value. Type conversion is handled automatically for you at the point of @DataBoundConstructor and @DataBoundSetter, so you can maintain the real type throughout your code.

jvz · 2019-07-15T15:24:03Z

src/main/resources/hudson/plugins/ec2/SlaveTemplate/config.jelly

@@ -177,6 +177,10 @@ THE SOFTWARE.
    <f:entry title="${%Maximum Total Uses}" field="maxTotalUses">
      <f:textbox default="-1"/>
    </f:entry>
+
+    	<f:entry title="Minimum uptime in hours" field="minUpHours">
+		<f:textbox default="0" />


This should use <f:number> instead. In fact, all the other textboxes in this form that are numeric should also use that.

jvz · 2019-07-15T15:24:32Z

src/main/resources/hudson/plugins/ec2/SlaveTemplate/help-minUpHours.html

@@ -0,0 +1,6 @@
+<div>
+	<p> Retains ephemeral agents for specified hours before applying "idle termination minutes" rules.


Suggested change

<p> Retains ephemeral agents for specified hours before applying "idle termination minutes" rules.

<p>Retains ephemeral agents for specified hours before applying "idle termination minutes" rules.</p>

sureshna added 2 commits July 5, 2019 17:38

Added minimum uphours feature

8010750

Corrected error found by spotbugs

fb65164

res0nance requested changes Jul 7, 2019

View reviewed changes

Updated code as per review comments

a08f527

snallami commented Jul 14, 2019

View reviewed changes

sureshna added 2 commits July 13, 2019 20:52

For some odd reason, Jenkinsfile got deleted in my previous commit; a…

f3a14c8

…dded file again

Corrected intendation from text editor instead of eclipse, lets hope …

47a5df1

…that this turns our to me ok

thoulen requested a review from jvz July 14, 2019 19:57

jvz requested changes Jul 15, 2019

View reviewed changes

res0nance added the enhancement Feature additions or enhancements label Sep 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added minimum uphours feature #377

Added minimum uphours feature #377

snallami commented Jul 6, 2019

res0nance left a comment

res0nance Jul 7, 2019

snallami Jul 7, 2019

res0nance Jul 7, 2019

snallami Jul 8, 2019

res0nance Jul 7, 2019

res0nance Jul 7, 2019

snallami Jul 7, 2019

res0nance Jul 8, 2019

jvz Jul 15, 2019

snallami commented Jul 7, 2019

snallami commented Jul 7, 2019

res0nance commented Jul 8, 2019

snallami Jul 14, 2019

jvz left a comment

jvz Jul 15, 2019

jvz Jul 15, 2019

jvz Jul 15, 2019

jvz Jul 15, 2019

jvz Jul 15, 2019

jvz Jul 15, 2019

jvz Jul 15, 2019

		@@ -1 +0,0 @@
		buildPlugin(configurations: buildPlugin.recommendedConfigurations())

		@@ -140,6 +140,8 @@

		public int nextSubnet;

		private String minUpHours;

		@@ -0,0 +1,6 @@
		<div>
		<p> Retains ephemeral agents for specified hours before applying "idle termination minutes" rules.

	<p> Retains ephemeral agents for specified hours before applying "idle termination minutes" rules.
	<p>Retains ephemeral agents for specified hours before applying "idle termination minutes" rules.</p>

Added minimum uphours feature #377

Are you sure you want to change the base?

Added minimum uphours feature #377

Conversation

snallami commented Jul 6, 2019

res0nance left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snallami commented Jul 7, 2019

snallami commented Jul 7, 2019

res0nance commented Jul 8, 2019

Choose a reason for hiding this comment

jvz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment