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 2 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 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 @@ -32,29 +32,29 @@ public final class EC2OndemandSlave extends EC2AbstractSlave {
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not add the new option to deprecated constructors

Copy link
Member Author

Choose a reason for hiding this comment

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

let me see if i can bypass them

throws FormException, IOException {
this(instanceId, description, remoteFS, numExecutors, labelString, mode, initScript, tmpDir, remoteAdmin, jvmopts, stopOnTerminate, idleTerminationMinutes, publicDNS, privateDNS, tags, cloudName, false, launchTimeout, amiType);
this(instanceId, description, remoteFS, numExecutors, labelString, mode, initScript, tmpDir, remoteAdmin, jvmopts, stopOnTerminate, idleTerminationMinutes, publicDNS, privateDNS, tags, cloudName, false, launchTimeout, amiType, minUpHours);
}

@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 useDedicatedTenancy, 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, boolean useDedicatedTenancy, int launchTimeout, AMITypeData amiType, String minUpHours)
throws FormException, IOException {
this(instanceId, description, remoteFS, numExecutors, labelString, mode, initScript, tmpDir, remoteAdmin, jvmopts, stopOnTerminate, idleTerminationMinutes, publicDNS, privateDNS, tags, cloudName, false, useDedicatedTenancy, launchTimeout, amiType);
this(instanceId, description, remoteFS, numExecutors, labelString, mode, initScript, tmpDir, remoteAdmin, jvmopts, stopOnTerminate, idleTerminationMinutes, publicDNS, privateDNS, tags, cloudName, false, useDedicatedTenancy, launchTimeout, amiType, minUpHours);
}

@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)
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, String minUpHours)
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, minUpHours);
}

@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.MILLISECONDS.toHours(minUpHours);
if (minUpHoursInMills > uptime) {
return 1;
}

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

Expand Down
12 changes: 6 additions & 6 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 @@ -33,23 +33,23 @@ public final class EC2SpotSlave extends EC2AbstractSlave implements EC2Readiness
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here on deprecated constructors

throws FormException, IOException {
this(name, spotInstanceRequestId, description, remoteFS, numExecutors, mode, initScript, tmpDir, labelString, remoteAdmin, jvmopts, idleTerminationMinutes, tags, cloudName, false, launchTimeout, amiType);
this(name, spotInstanceRequestId, description, remoteFS, numExecutors, mode, initScript, tmpDir, labelString, remoteAdmin, jvmopts, idleTerminationMinutes, tags, cloudName, false, launchTimeout, amiType, minUpHours);
}

@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)
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, String minUpHours)
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, minUpHours);
}

@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
Loading