Skip to content

Commit

Permalink
Add timeout for a slave that gets provisioned but then has no work
Browse files Browse the repository at this point in the history 
Resolves effect of jenkinsci#54

Also fix DockerTemplateTest

This was broken before by jenkinsci#46, but also needed to be updated for my changes
  • Loading branch information
Thomas Suckow committed Jul 21, 2014
1 parent d441aa9 commit 7650a8a
Show file tree
Hide file tree
Showing 5 changed files with 66 additions and 24 deletions.
61 changes: 43 additions & 18 deletions src/main/java/com/nirima/jenkins/plugins/docker/DockerRetentionStrategy.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,26 @@ public class DockerRetentionStrategy extends RetentionStrategy<DockerComputer>

private AtomicBoolean currentlyChecking;

/** Number of minutes of idleness before an instance should be terminated.
A value of zero indicates that the instance should never be automatically terminated */
public final int idleTerminationMinutes;

@DataBoundConstructor
public DockerRetentionStrategy() {
public DockerRetentionStrategy(String idleTerminationMinutes) {
currentlyChecking = new AtomicBoolean(false);

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

this.idleTerminationMinutes = value;
}
}

@Override
Expand All @@ -38,26 +55,34 @@ public long check(DockerComputer c) {
boolean shouldTerminate = false;

try {
synchronized (this) {
if (c.isIdle() && c.isOnline() && !disabled && c.haveWeRunAnyJobs())
shouldTerminate = true;
if( !c.isAcceptingTasks() )
shouldTerminate = true;

if (c.isIdle() && !disabled) {
if( idleTerminationMinutes > 0) {
final long idleMilliseconds = System.currentTimeMillis() - c.getIdleStartMilliseconds();
if (idleMilliseconds > TimeUnit2.MINUTES.toMillis(idleTerminationMinutes)) {
shouldTerminate = true;
}
}

//TODO: Verify all of this should be in a synchronized block
synchronized (this) {
if (c.isOnline() && c.haveWeRunAnyJobs())
shouldTerminate = true;
if( !c.isAcceptingTasks() )
shouldTerminate = true;
}
}

// TODO: really think about the right strategy here

if( shouldTerminate ) {
final long idleMilliseconds = System.currentTimeMillis() - c.getIdleStartMilliseconds();
if (idleMilliseconds > 0) {
LOGGER.info("Idle timeout: " + c.getName());
LOGGER.log(Level.INFO, "Terminating " + c);
try {
c.getNode().retentionTerminate();
} catch (IOException e) {
e.printStackTrace();
} catch (InterruptedException e) {
e.printStackTrace();
}
LOGGER.info("Idle timeout: " + c.getName());
LOGGER.log(Level.INFO, "Terminating " + c);
try {
c.getNode().retentionTerminate();
} catch (IOException e) {
e.printStackTrace();
} catch (InterruptedException e) {
e.printStackTrace();
}
}
}
Expand Down
11 changes: 9 additions & 2 deletions src/main/java/com/nirima/jenkins/plugins/docker/DockerTemplate.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ public class DockerTemplate implements Describable<DockerTemplate> {
*/
public final String dockerCommand;

/**
* Minutes before terminating an idle slave
*/
public final String idleTerminationMinutes;

/**
* Field jvmOptions.
*/
Expand Down Expand Up @@ -89,7 +94,8 @@ public class DockerTemplate implements Describable<DockerTemplate> {
@DataBoundConstructor
public DockerTemplate(String image, String labelString,
String remoteFs,
String credentialsId, String jvmOptions, String javaPath,
String credentialsId, String idleTerminationMinutes,
String jvmOptions, String javaPath,
String prefixStartSlaveCmd, String suffixStartSlaveCmd,
String instanceCapStr, String dnsString,
String dockerCommand,
Expand All @@ -101,6 +107,7 @@ public DockerTemplate(String image, String labelString,
this.image = image;
this.labelString = Util.fixNull(labelString);
this.credentialsId = credentialsId;
this.idleTerminationMinutes = idleTerminationMinutes;
this.jvmOptions = jvmOptions;
this.javaPath = javaPath;
this.prefixStartSlaveCmd = prefixStartSlaveCmd;
Expand Down Expand Up @@ -189,7 +196,7 @@ public DockerSlave provision(StreamTaskListener listener) throws IOException, De
Node.Mode mode = Node.Mode.EXCLUSIVE;


RetentionStrategy retentionStrategy = new DockerRetentionStrategy();
RetentionStrategy retentionStrategy = new DockerRetentionStrategy(idleTerminationMinutes);

List<? extends NodeProperty<?>> nodeProperties = new ArrayList();

Expand Down
11 changes: 8 additions & 3 deletions src/main/java/com/nirima/jenkins/plugins/docker/builder/DockerBuilderNewTemplate.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ public class DockerBuilderNewTemplate extends Builder implements Serializable {
public final String labelString;
public final String remoteFs;
public final String credentialsId;
public final String idleTerminationMinutes;
public final String jvmOptions;
public final String javaPath;
public final String prefixStartSlaveCmd;
Expand All @@ -54,7 +55,8 @@ public class DockerBuilderNewTemplate extends Builder implements Serializable {

@DataBoundConstructor
public DockerBuilderNewTemplate(String image, String labelString, String remoteFs,
String credentialsId, String jvmOptions, String javaPath,
String credentialsId, String idleTerminationMinutes,
String jvmOptions, String javaPath,
String prefixStartSlaveCmd, String suffixStartSlaveCmd,
String instanceCapStr, String dnsString,
String dockerCommand,
Expand All @@ -66,6 +68,7 @@ public DockerBuilderNewTemplate(String image, String labelString, String remoteF
this.labelString = labelString;
this.remoteFs = remoteFs;
this.credentialsId = credentialsId;
this.idleTerminationMinutes = idleTerminationMinutes;
this.jvmOptions = jvmOptions;
this.javaPath = javaPath;
this.prefixStartSlaveCmd = prefixStartSlaveCmd;
Expand Down Expand Up @@ -112,8 +115,10 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListen
for (Cloud c : Jenkins.getInstance().clouds) {
if (c instanceof DockerCloud && ((DockerCloud) c).getTemplate(image) == null) {
LOGGER.log(Level.INFO, "Adding new template « "+image+" » to cloud " + ((DockerCloud) c).name);
DockerTemplate t = new DockerTemplate(image, labelString, remoteFs, credentialsId,
jvmOptions, javaPath, prefixStartSlaveCmd,
DockerTemplate t = new DockerTemplate(image, labelString, remoteFs,
credentialsId, idleTerminationMinutes,
jvmOptions, javaPath,
prefixStartSlaveCmd,
suffixStartSlaveCmd, instanceCapStr,
dnsString, dockerCommand,
volumesString, volumesFrom, hostname, privileged);
Expand Down
4 changes: 4 additions & 0 deletions src/main/resources/com/nirima/jenkins/plugins/docker/DockerTemplate/template.jelly
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@

<f:advanced>

<f:entry title="${%Idle termination time}" field="idleTerminationMinutes">
<f:textbox default="5" />
</f:entry>

<f:entry title="${%JavaPath}" field="javaPath">
<f:textbox/>
</f:entry>
Expand Down
3 changes: 2 additions & 1 deletion src/test/java/com/nirima/jenkins/plugins/docker/DockerTemplateTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,12 @@

import org.junit.Test;
import static org.junit.Assert.*;
import hudson.model.Node;

public class DockerTemplateTest {

private DockerTemplate getDockerTemplateInstanceWithDNSHost(String dnsString) {
DockerTemplate instance = new DockerTemplate("image", null, "remoteFs", "credentialsId", " jvmOptions", " javaPath", "prefixStartSlaveCmd", " suffixStartSlaveCmd", "", dnsString, "dockerCommand", "volumes", "hostname", false);
DockerTemplate instance = new DockerTemplate("image", null, "remoteFs", "credentialsId", "idleTerminationMinutes", " jvmOptions", " javaPath", "prefixStartSlaveCmd", " suffixStartSlaveCmd", "", dnsString, "dockerCommand", "volumes", "volumesFrom", "hostname", false);
return instance;
}

Expand Down

0 comments on commit 7650a8a

Please sign in to comment.