Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added minimum uphours feature #377

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Added minimum uphours feature #377

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8010750
Added minimum uphours feature
Jul 6, 2019
fb65164
Corrected error found by spotbugs
Jul 7, 2019
a08f527
Updated code as per review comments
Jul 7, 2019
f872639
1. Added overloaded constructor as per review suggestion.
Jul 14, 2019
f3a14c8
For some odd reason, Jenkinsfile got deleted in my previous commit; a…
Jul 14, 2019
47a5df1
Corrected intendation from text editor instead of eclipse, lets hope …
Jul 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion Jenkinsfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
buildPlugin(configurations: buildPlugin.recommendedConfigurations())
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

buildPlugin(configurations: buildPlugin.recommendedConfigurations())
12 changes: 10 additions & 2 deletions src/main/java/hudson/plugins/ec2/EC2AbstractSlave.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ public abstract class EC2AbstractSlave extends Slave {
public final String cloudName;
public AMITypeData amiType;
public int maxTotalUses;
public String minUpHours;
private String instanceType;

// Temporary stuff that is obtained live from EC2
Expand Down Expand Up @@ -135,7 +136,7 @@ public abstract class EC2AbstractSlave extends Slave {

public static final String TEST_ZONE = "testZone";

public EC2AbstractSlave(String name, String instanceId, String description, String remoteFS, int numExecutors, Mode mode, String labelString, ComputerLauncher launcher, RetentionStrategy<EC2Computer> retentionStrategy, String initScript, String tmpDir, List<? extends NodeProperty<?>> nodeProperties, String remoteAdmin, String jvmopts, boolean stopOnTerminate, String idleTerminationMinutes, List<EC2Tag> tags, String cloudName, boolean useDedicatedTenancy, int launchTimeout, AMITypeData amiType, ConnectionStrategy connectionStrategy, int maxTotalUses)
public EC2AbstractSlave(String name, String instanceId, String description, String remoteFS, int numExecutors, Mode mode, String labelString, ComputerLauncher launcher, RetentionStrategy<EC2Computer> retentionStrategy, String initScript, String tmpDir, List<? extends NodeProperty<?>> nodeProperties, String remoteAdmin, String jvmopts, boolean stopOnTerminate, String idleTerminationMinutes, List<EC2Tag> tags, String cloudName, boolean useDedicatedTenancy, int launchTimeout, AMITypeData amiType, ConnectionStrategy connectionStrategy, int maxTotalUses, String minUpHours)
throws FormException, IOException {

super(name, remoteFS, launcher);
Expand All @@ -160,10 +161,17 @@ public EC2AbstractSlave(String name, String instanceId, String description, Stri
this.launchTimeout = launchTimeout;
this.amiType = amiType;
this.maxTotalUses = maxTotalUses;
this.minUpHours = minUpHours;
readResolve();
fetchLiveInstanceData(true);
}


@Deprecated
public EC2AbstractSlave(String name, String instanceId, String description, String remoteFS, int numExecutors, Mode mode, String labelString, ComputerLauncher launcher, RetentionStrategy<EC2Computer> retentionStrategy, String initScript, String tmpDir, List<? extends NodeProperty<?>> nodeProperties, String remoteAdmin, String jvmopts, boolean stopOnTerminate, String idleTerminationMinutes, List<EC2Tag> tags, String cloudName, boolean useDedicatedTenancy, int launchTimeout, AMITypeData amiType, ConnectionStrategy connectionStrategy, int maxTotalUses)
throws FormException, IOException {
this(name, instanceId, description, remoteFS, numExecutors, mode, labelString, launcher, retentionStrategy, initScript, tmpDir, nodeProperties, remoteAdmin, jvmopts, stopOnTerminate, idleTerminationMinutes, tags, cloudName, useDedicatedTenancy, launchTimeout, amiType, connectionStrategy, maxTotalUses, "0" );
}

@Deprecated
public EC2AbstractSlave(String name, String instanceId, String description, String remoteFS, int numExecutors, Mode mode, String labelString, ComputerLauncher launcher, RetentionStrategy<EC2Computer> retentionStrategy, String initScript, String tmpDir, List<? extends NodeProperty<?>> nodeProperties, String remoteAdmin, String jvmopts, boolean stopOnTerminate, String idleTerminationMinutes, List<EC2Tag> tags, String cloudName, boolean usePrivateDnsName, boolean useDedicatedTenancy, int launchTimeout, AMITypeData amiType)
throws FormException, IOException {
Expand Down
8 changes: 4 additions & 4 deletions src/main/java/hudson/plugins/ec2/EC2OndemandSlave.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,15 @@ public EC2OndemandSlave(String instanceId, String description, String remoteFS,
@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, boolean usePrivateDnsName, boolean useDedicatedTenancy, int launchTimeout, AMITypeData amiType)
throws FormException, IOException {
this(description + " (" + instanceId + ")", instanceId, description, remoteFS, numExecutors, labelString, mode, initScript, tmpDir, Collections.emptyList(), remoteAdmin, jvmopts, stopOnTerminate, idleTerminationMinutes, publicDNS, privateDNS, tags, cloudName, useDedicatedTenancy, launchTimeout, amiType, ConnectionStrategy.backwardsCompatible(usePrivateDnsName, false, false), -1);
this(description + " (" + instanceId + ")", instanceId, description, remoteFS, numExecutors, labelString, mode, initScript, tmpDir, Collections.emptyList(), remoteAdmin, jvmopts, stopOnTerminate, idleTerminationMinutes, publicDNS, privateDNS, tags, cloudName, useDedicatedTenancy, launchTimeout, amiType, ConnectionStrategy.backwardsCompatible(usePrivateDnsName, false, false), -1, "0");
}

@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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

throws FormException, IOException {

super(name, instanceId, description, remoteFS, numExecutors, mode, labelString, amiType.isWindows() ? new EC2WindowsLauncher()
: new EC2UnixLauncher(), new EC2RetentionStrategy(idleTerminationMinutes), initScript, tmpDir, nodeProperties, remoteAdmin, jvmopts, stopOnTerminate, idleTerminationMinutes, tags, cloudName, useDedicatedTenancy, launchTimeout, amiType, connectionStrategy, maxTotalUses);
: new EC2UnixLauncher(), new EC2RetentionStrategy(idleTerminationMinutes, minUpHours), initScript, tmpDir, nodeProperties, remoteAdmin, jvmopts, stopOnTerminate, idleTerminationMinutes, tags, cloudName, useDedicatedTenancy, launchTimeout, amiType, connectionStrategy, maxTotalUses);

this.publicDNS = publicDNS;
this.privateDNS = privateDNS;
Expand All @@ -64,7 +64,7 @@ public EC2OndemandSlave(String name, String instanceId, String description, Stri
* Constructor for debugging.
*/
public EC2OndemandSlave(String instanceId) throws FormException, IOException {
this(instanceId, instanceId, "debug", "/tmp/hudson", 1, "debug", Mode.NORMAL, "", "/tmp", Collections.emptyList(), null, null, false, null, "Fake public", "Fake private", null, null, false, 0, new UnixData(null, null, null, null), ConnectionStrategy.PRIVATE_IP, -1);
this(instanceId, instanceId, "debug", "/tmp/hudson", 1, "debug", Mode.NORMAL, "", "/tmp", Collections.emptyList(), null, null, false, null, "Fake public", "Fake private", null, null, false, 0, new UnixData(null, null, null, null), ConnectionStrategy.PRIVATE_IP, -1, "0");
}

/**
Expand Down
28 changes: 25 additions & 3 deletions src/main/java/hudson/plugins/ec2/EC2RetentionStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ public class EC2RetentionStrategy extends RetentionStrategy<EC2Computer> impleme
* billing period.
*/
public final int idleTerminationMinutes;

//Retains ephemeral agents for specified hours before applying "idle termination minutes" rules.
public final int minUpHours;

private transient ReentrantLock checkLock;
private static final int STARTUP_TIME_DEFAULT_VALUE = 30;
Expand All @@ -73,7 +76,7 @@ public class EC2RetentionStrategy extends RetentionStrategy<EC2Computer> impleme
String.valueOf(STARTUP_TIME_DEFAULT_VALUE)), STARTUP_TIME_DEFAULT_VALUE);

@DataBoundConstructor
public EC2RetentionStrategy(String idleTerminationMinutes) {
public EC2RetentionStrategy(String idleTerminationMinutes, String minUpHours) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

readResolve();
if (idleTerminationMinutes == null || idleTerminationMinutes.trim().isEmpty()) {
this.idleTerminationMinutes = 0;
Expand All @@ -87,11 +90,24 @@ public EC2RetentionStrategy(String idleTerminationMinutes) {

this.idleTerminationMinutes = value;
}

if (minUpHours == null || minUpHours.trim().isEmpty()) {
this.minUpHours = 0;
} else {
int value = 0;
try {
value = Integer.parseInt(minUpHours);
} catch (NumberFormatException nfe) {
LOGGER.info("Malformed default minUpHours value: " + minUpHours);
}

this.minUpHours = value;
}
}


EC2RetentionStrategy(String idleTerminationMinutes, Clock clock, long nextCheckAfter) {
this(idleTerminationMinutes);
EC2RetentionStrategy(String idleTerminationMinutes, String minUpHours, Clock clock, long nextCheckAfter) {
this(idleTerminationMinutes, minUpHours);
this.clock = clock;
this.nextCheckAfter = nextCheckAfter;
}
Expand Down Expand Up @@ -161,6 +177,12 @@ private long internalCheck(EC2Computer computer) {
if (computer.isOffline() && uptime < TimeUnit.MINUTES.toMillis(STARTUP_TIMEOUT)) {
return 1;
}

// Check min number of hours instance must be up - if stayed up at least that minimum.
long minUpHoursInMills = TimeUnit.HOURS.toMillis(minUpHours);
if (minUpHoursInMills > uptime) {
return 1;
}

final long idleMilliseconds = this.clock.millis() - computer.getIdleStartMilliseconds();

Expand Down
6 changes: 3 additions & 3 deletions src/main/java/hudson/plugins/ec2/EC2SpotSlave.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,15 +41,15 @@ public EC2SpotSlave(String name, String spotInstanceRequestId, String descriptio
@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, boolean usePrivateDnsName, int launchTimeout, AMITypeData amiType)
throws FormException, IOException {
this(description + " (" + name + ")", spotInstanceRequestId, description, remoteFS, numExecutors, mode, initScript, tmpDir, labelString, Collections.emptyList(), remoteAdmin, jvmopts, idleTerminationMinutes, tags, cloudName, launchTimeout, amiType, ConnectionStrategy.backwardsCompatible(usePrivateDnsName, false, false), -1);
this(description + " (" + name + ")", spotInstanceRequestId, description, remoteFS, numExecutors, mode, initScript, tmpDir, labelString, Collections.emptyList(), remoteAdmin, jvmopts, idleTerminationMinutes, tags, cloudName, launchTimeout, amiType, ConnectionStrategy.backwardsCompatible(usePrivateDnsName, false, false), -1, "0");
}

@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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, need another constructor.

throws FormException, IOException {

super(name, "", description, remoteFS, numExecutors, mode, labelString, amiType.isWindows() ? new EC2WindowsLauncher() :
new EC2UnixLauncher(), new EC2RetentionStrategy(idleTerminationMinutes), initScript, tmpDir, nodeProperties, remoteAdmin, jvmopts, false, idleTerminationMinutes, tags, cloudName, false, launchTimeout, amiType, connectionStrategy, maxTotalUses);
new EC2UnixLauncher(), new EC2RetentionStrategy(idleTerminationMinutes, minUpHours), initScript, tmpDir, nodeProperties, remoteAdmin, jvmopts, false, idleTerminationMinutes, tags, cloudName, false, launchTimeout, amiType, connectionStrategy, maxTotalUses);

this.name = name;
this.spotInstanceRequestId = spotInstanceRequestId;
Expand Down
29 changes: 26 additions & 3 deletions src/main/java/hudson/plugins/ec2/SlaveTemplate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,8 @@ public class SlaveTemplate implements Describable<SlaveTemplate> {

public int nextSubnet;

private String minUpHours;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


public String currentSubnetId;

private transient/* almost final */Set<LabelAtom> labelSet;
Expand Down Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want me to add one more overloaded constructor?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if(StringUtils.isNotBlank(remoteAdmin) || StringUtils.isNotBlank(jvmopts) || StringUtils.isNotBlank(tmpDir)){
LOGGER.log(Level.FINE, "As remoteAdmin, jvmopts or tmpDir is not blank, we must ensure the user has RUN_SCRIPTS rights.");
Expand Down Expand Up @@ -234,10 +236,27 @@ public SlaveTemplate(String ami, String zone, SpotConfiguration spotConfig, Stri
this.useEphemeralDevices = useEphemeralDevices;
this.customDeviceMapping = customDeviceMapping;
this.t2Unlimited = t2Unlimited;
this.minUpHours = minUpHours;

readResolve(); // initialize
}

@Deprecated
public SlaveTemplate(String ami, String zone, SpotConfiguration spotConfig, String securityGroups, String remoteFS,
InstanceType type, boolean ebsOptimized, String labelString, Node.Mode mode, String description, String initScript,
String tmpDir, String userData, String numExecutors, String remoteAdmin, AMITypeData amiType, String jvmopts,
boolean stopOnTerminate, String subnetId, List<EC2Tag> tags, String idleTerminationMinutes,
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) {
this(ami, zone, spotConfig, securityGroups, remoteFS, type, ebsOptimized, labelString, mode, description, initScript,
tmpDir, userData, numExecutors, remoteAdmin, amiType, jvmopts, stopOnTerminate, subnetId, tags,
idleTerminationMinutes, instanceCapStr, iamInstanceProfile, deleteRootOnTermination, useEphemeralDevices,
useDedicatedTenancy, launchTimeoutStr, associatePublicIp, customDeviceMapping, connectBySSHProcess,
monitoring, t2Unlimited, connectionStrategy, -1, "0");
}

@Deprecated
public SlaveTemplate(String ami, String zone, SpotConfiguration spotConfig, String securityGroups, String remoteFS,
InstanceType type, boolean ebsOptimized, String labelString, Node.Mode mode, String description, String initScript,
Expand Down Expand Up @@ -416,6 +435,10 @@ public boolean getAssociatePublicIp() {
return associatePublicIp;
}

public String getMinUpHours() {
return minUpHours;
}

@Deprecated
@DataBoundSetter
public void setConnectUsingPublicIp(boolean connectUsingPublicIp) {
Expand Down Expand Up @@ -1102,13 +1125,13 @@ protected EC2OndemandSlave newOndemandSlave(Instance inst) throws FormException,
return new EC2OndemandSlave(getSlaveName(inst.getInstanceId()), inst.getInstanceId(), description, remoteFS, getNumExecutors(), labels, mode, initScript,
tmpDir, Collections.emptyList(), remoteAdmin, jvmopts, stopOnTerminate, idleTerminationMinutes, inst.getPublicDnsName(),
inst.getPrivateDnsName(), EC2Tag.fromAmazonTags(inst.getTags()), parent.name,
useDedicatedTenancy, getLaunchTimeout(), amiType, connectionStrategy, maxTotalUses);
useDedicatedTenancy, getLaunchTimeout(), amiType, connectionStrategy, maxTotalUses, minUpHours);
}

protected EC2SpotSlave newSpotSlave(SpotInstanceRequest sir) throws FormException, IOException {
return new EC2SpotSlave(getSlaveName(sir.getSpotInstanceRequestId()), sir.getSpotInstanceRequestId(), description, remoteFS, getNumExecutors(), mode, initScript,
tmpDir, labels, Collections.emptyList(), remoteAdmin, jvmopts, idleTerminationMinutes, EC2Tag.fromAmazonTags(sir.getTags()), parent.name,
getLaunchTimeout(), amiType, connectionStrategy, maxTotalUses);
getLaunchTimeout(), amiType, connectionStrategy, maxTotalUses, minUpHours);
}

/**
Expand Down
4 changes: 4 additions & 0 deletions src/main/resources/hudson/plugins/ec2/SlaveTemplate/config.jelly
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

</f:entry>

</f:advanced>

Expand Down
6 changes: 6 additions & 0 deletions src/main/resources/hudson/plugins/ec2/SlaveTemplate/help-minUpHours.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<div>
<p> Retains ephemeral agents for specified hours before applying "idle termination minutes" rules.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

<p>Overrides the default termination time by adding the specified number of hours before the evaluation is calculated</p>
<p>Time is expressed in hours - default being 0 which causes the system to behave as if this wasn't set</p>
<p>e.g. Setting it 7 will cause an instance to say up a minimum of 7 hours before it will be considered for shutdown</p>
</div>