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

Configurable number of executors #81 #174

Closed
wants to merge 1 commit into from
Closed

Configurable number of executors #81 #174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 12 additions & 3 deletions docker-plugin/src/main/java/com/nirima/jenkins/plugins/docker/DockerTemplate.java
View file
Open in desktop
Expand Up @@ -37,6 +37,7 @@
import hudson.model.labels.LabelAtom;
import hudson.plugins.sshslaves.SSHLauncher;
import hudson.security.ACL;
import hudson.slaves.CloudRetentionStrategy;
import hudson.slaves.ComputerLauncher;
import hudson.slaves.NodeProperty;
import hudson.slaves.RetentionStrategy;
Expand All @@ -50,6 +51,8 @@ public class DockerTemplate extends DockerTemplateBase implements Describable<Do


public final String labelString;

public final int numExecutors;

// SSH settings
/**
Expand Down Expand Up @@ -102,6 +105,7 @@ public class DockerTemplate extends DockerTemplateBase implements Describable<Do

@DataBoundConstructor
public DockerTemplate(String image, String labelString,
int numExecutors,
String remoteFs,
String remoteFsMapping,
String credentialsId, String idleTerminationMinutes,
Expand All @@ -127,6 +131,7 @@ public DockerTemplate(String image, String labelString,


this.labelString = Util.fixNull(labelString);
this.numExecutors = numExecutors;
this.credentialsId = credentialsId;
this.idleTerminationMinutes = idleTerminationMinutes;
this.sshLaunchTimeoutMinutes = sshLaunchTimeoutMinutes;
Expand Down Expand Up @@ -227,10 +232,14 @@ public DockerSlave provision(StreamTaskListener listener) throws IOException, De

logger.println("Launching " + image );

int numExecutors = 1;
Node.Mode mode = Node.Mode.NORMAL;

RetentionStrategy retentionStrategy = new OnceRetentionStrategy(idleTerminationMinutes());
RetentionStrategy retentionStrategy = null;
if (numExecutors == 1) {
retentionStrategy = new OnceRetentionStrategy(idleTerminationMinutes());
} else {
retentionStrategy = new CloudRetentionStrategy(idleTerminationMinutes());
}
Copy link
Member

Choose a reason for hiding this comment

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

Is it make any sense? What will happen if OnceRetentionStrategy will be used with more executors?

Copy link
Member

Choose a reason for hiding this comment

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

Sent this question to maillist hope to get some comments https://groups.google.com/d/msg/jenkinsci-dev/6nGESEvJ_S0/M-jLIrHJS6AJ

Copy link
Author

Choose a reason for hiding this comment

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

I'm not 100% sure if this answers your question, but hopefully this will make sense. I have not tested with OnceRetentionStrategy and multiple executors. From the source and docs, it looked like it very much expected to be running on a slave with one executor. As far as I could tell from the source, as soon as any task was completed, it would try and terminate the slave. What I didn't want to happen was the following scenario:

  • DockerTemplate configured with two executors and OnceRetentionStrategy
  • Jenkins queues two jobs, QuickJob and LongJob
  • Jenkins spins up a new slave with that template and assigns QuickJob to one executor and LongJob to the other on that slave.
  • QuickJob completes quickly and triggers a terminate long before LongJob completes.

I wasn't sure what would happen at this point, whether or not it would let LongJob finish up before tearing down or if it would just get killed. If it's the latter, then we certainly can't allow Once with multiple executors, if it's the former, then it'd be fine (but it doesn't seem significantly different than running as Cloud with a low idle termination time).

As for the case where we're running a ContinuableExecutable and a Cloud strategy, I don't have that in my system and am not familiar enough with the paradigm to speak to it, so it is untested. (Do such tasks assume/require that they be built on the same node?)


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

Expand Down Expand Up @@ -273,7 +282,7 @@ public InspectContainerResponse provisionNew() throws DockerException {
}

public int getNumExecutors() {
return 1;
return numExecutors;
}

@Override
Expand Down
6 changes: 4 additions & 2 deletions ...gin/src/main/java/com/nirima/jenkins/plugins/docker/builder/DockerBuilderNewTemplate.java
View file
Open in desktop
Expand Up @@ -38,6 +38,7 @@ public class DockerBuilderNewTemplate extends Builder implements Serializable {

public final String image;
public final String labelString;
public final int numExecutors;
public final String remoteFsMapping;
public final String remoteFs;
public final String credentialsId;
Expand All @@ -63,7 +64,7 @@ public class DockerBuilderNewTemplate extends Builder implements Serializable {
public final String hostname;

@DataBoundConstructor
public DockerBuilderNewTemplate(String image, String labelString, String remoteFs, String remoteFsMapping,
public DockerBuilderNewTemplate(String image, String labelString, int numExecutors, String remoteFs, String remoteFsMapping,
String credentialsId, String idleTerminationMinutes,
String sshLaunchTimeoutMinutes,
String jvmOptions, String javaPath,
Expand All @@ -82,6 +83,7 @@ public DockerBuilderNewTemplate(String image, String labelString, String remoteF

this.image = image;
this.labelString = labelString;
this.numExecutors = numExecutors;
this.remoteFs = remoteFs;
this.remoteFsMapping = remoteFsMapping;
this.credentialsId = credentialsId;
Expand Down Expand Up @@ -140,7 +142,7 @@ 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, remoteFsMapping,
DockerTemplate t = new DockerTemplate(image, labelString, numExecutors, remoteFs, remoteFsMapping,
credentialsId, idleTerminationMinutes,
sshLaunchTimeoutMinutes,
jvmOptions, javaPath,
Expand Down
4 changes: 4 additions & 0 deletions ...plugin/src/main/resources/com/nirima/jenkins/plugins/docker/DockerTemplate/template.jelly
View file
Open in desktop
Expand Up @@ -11,6 +11,10 @@
<f:textbox/>
</f:entry>

<f:entry title="${%Num Executors}" field="numExecutors">
<f:number name="numExecutors" clazz="positive-number" min="1" step="1"/>
</f:entry>

<f:entry title="${%Credentials}" field="credentialsId">
<c:select/>
</f:entry>
Expand Down
2 changes: 2 additions & 0 deletions docker-plugin/src/test/java/com/nirima/jenkins/plugins/docker/DockerTemplateTest.java
View file
Open in desktop
Expand Up @@ -9,6 +9,7 @@ public class DockerTemplateTest {

String image = "image";
String labelString;
int numExecutors = 1;
String remoteFs = "remoteFs";
String remoteFsMapping = "remoteFsMapping";
String credentialsId = "credentialsId";
Expand Down Expand Up @@ -36,6 +37,7 @@ public class DockerTemplateTest {

private DockerTemplate getDockerTemplateInstanceWithDNSHost(String dnsString) {
DockerTemplate instance = new DockerTemplate(image, labelString,
numExecutors,
remoteFs,
remoteFsMapping,
credentialsId, idleTerminationMinutes,
Expand Down