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

New Option for JNLP: Pass Slave Connection Arguments to Entrypoint #596

Merged
merged 1 commit into from
Jan 16, 2018
Merged

New Option for JNLP: Pass Slave Connection Arguments to Entrypoint #596

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 29 additions & 36 deletions src/main/java/com/nirima/jenkins/plugins/docker/DockerTemplate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
import com.github.dockerjava.api.command.CreateContainerCmd;
import com.github.dockerjava.api.command.PullImageCmd;
import com.github.dockerjava.api.exception.DockerClientException;
import com.github.dockerjava.api.exception.DockerException;
import com.github.dockerjava.api.exception.NotFoundException;
import com.github.dockerjava.api.model.PortBinding;
import com.github.dockerjava.api.model.PullResponseItem;
Expand All @@ -15,19 +14,13 @@
import com.nirima.jenkins.plugins.docker.strategy.DockerOnceRetentionStrategy;
import hudson.Extension;
import hudson.Util;
import hudson.model.Describable;
import hudson.model.Descriptor;
import hudson.model.DescriptorVisibilityFilter;
import hudson.model.Label;
import hudson.model.Node;
import hudson.model.TaskListener;
import hudson.model.*;
import hudson.model.labels.LabelAtom;
import hudson.slaves.ComputerLauncher;
import hudson.slaves.NodeProperty;
import hudson.slaves.NodePropertyDescriptor;
import hudson.slaves.RetentionStrategy;
import hudson.util.DescribableList;
import hudson.util.FormValidation;
import io.jenkins.docker.DockerTransientNode;
import io.jenkins.docker.client.DockerAPI;
import io.jenkins.docker.connector.DockerComputerConnector;
Expand All @@ -37,19 +30,14 @@
import org.kohsuke.accmod.restrictions.NoExternalUse;
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.DataBoundSetter;
import org.kohsuke.stapler.QueryParameter;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.Iterator;
import java.util.List;
import java.util.Set;
import java.io.Serializable;
import java.util.*;


public class DockerTemplate implements Describable<DockerTemplate> {
Expand Down Expand Up @@ -412,35 +400,40 @@ public void onNext(PullResponseItem item) {

}

@Restricted(NoExternalUse.class)
public DockerTransientNode provisionNode(DockerAPI api, TaskListener listener) throws IOException, Descriptor.FormException, InterruptedException {
public class ContainerCommandCreator implements Serializable{
Copy link
Author

Choose a reason for hiding this comment

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

Hi @ndeloof, I had to add this ContainerCommandCreator, since the CreateContainerCmd is not Serializable and therefore when passing it to the anonymous DelegatingComputerLauncher(new JNLPLauncher())) through the createLauncher method, Jenkins will fail serialization.
So the trick was to do it lazily using the DockerTemplate which is serializable. So this class is a holder for the uniqueContainerName and when needed, the it creates the CreateContainerCmd and fills it will all the required data from the DockerTemplate instance.

private final String containerUniqueName;

final DockerClient client = api.getClient();
final DockerComputerConnector connector = getConnector();
pullImage(api, listener);
ContainerCommandCreator(){
// Since container can now be launched during slave launch, we need an alternative unique node name, which will be also the container name
this.containerUniqueName = Long.toHexString(System.nanoTime());
}

LOGGER.info("Trying to run container for {}", getImage());
CreateContainerCmd cmd = client.createContainerCmd(getImage());
fillContainerConfig(cmd);
public String getContainerUniqueName() {
return containerUniqueName;
}

connector.beforeContainerCreated(api, remoteFs, cmd);
public CreateContainerCmd createContainerCmd(DockerAPI api){
final DockerClient client = api.getClient();
CreateContainerCmd cmd = client.createContainerCmd(getImage());
fillContainerConfig(cmd);
cmd.withName(containerUniqueName);
return cmd;
}

String containerId = cmd.exec().getId();
}

try {
connector.beforeContainerStarted(api, remoteFs, containerId);
@Restricted(NoExternalUse.class)
public DockerTransientNode provisionNode(DockerAPI api, TaskListener listener) throws IOException, Descriptor.FormException, InterruptedException {

client.startContainerCmd(containerId).exec();
final DockerComputerConnector connector = getConnector();
pullImage(api, listener);

connector.afterContainerStarted(api, remoteFs, containerId);
} catch (DockerException e) {
// if something went wrong, cleanup aborted container
client.removeContainerCmd(containerId).withForce(true).exec();
throw e;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to move this code away and introduce DockerContainerLifecycleHandler(which makes DockerComputerConnector role unclear). Just compute containerName, create DockerTransientNode object, then enforce here CreateContainerCmd.name is set to this unique ID and pass node object to connector methods for launcher setup.

Copy link
Author

Choose a reason for hiding this comment

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

I moved this code to the DockerComputerConnector, since it must be called from the launcher itself and cannot run before the launcher is being created. In this iteration, I tried to keep the original code as it was when I could (sorry for the many refactoring - did not try to make your review hard, it was just multiple iterations).
Also removed the DockerContainerLifecycleHandler and restored the original methods inside the DockerComputerConnector)

LOGGER.info("Trying to run container for {}", getImage());
ContainerCommandCreator containerCommandCreator = new ContainerCommandCreator();
LOGGER.info("Trying to run container for image: {}, with unique name: {}", getImage(), containerCommandCreator.getContainerUniqueName());
final ComputerLauncher launcher = connector.createLauncher(api, containerCommandCreator, remoteFs, listener);

final ComputerLauncher launcher = connector.createLauncher(api, containerId, remoteFs, listener);
DockerTransientNode node = new DockerTransientNode(containerId, remoteFs, launcher);
DockerTransientNode node = new DockerTransientNode(containerCommandCreator.getContainerUniqueName(), remoteFs, launcher);
node.setNodeDescription("Docker Agent [" + getImage() + " on "+ api.getDockerHost().getUri() + "]");
node.setMode(mode);
node.setLabelString(labelString);
Expand Down
28 changes: 17 additions & 11 deletions src/main/java/io/jenkins/docker/DockerTransientNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,44 +7,50 @@
import com.nirima.jenkins.plugins.docker.strategy.DockerOnceRetentionStrategy;
import hudson.model.Computer;
import hudson.model.Descriptor;
import hudson.model.Queue;
import hudson.model.Slave;
import hudson.model.TaskListener;
import hudson.slaves.Cloud;
import hudson.slaves.ComputerLauncher;
import io.jenkins.docker.client.DockerAPI;
import jenkins.model.Jenkins;

import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
import java.io.IOException;
import java.io.Serializable;

/**
* A {@link Slave} node designed to be used only once for a build.
*
* @author <a href="mailto:[email protected]">Nicolas De Loof</a>
*/
public class DockerTransientNode extends Slave {

private final String containerId;


Copy link
Contributor

Choose a reason for hiding this comment

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

please do not remove this useful information. Just make this non-final and assign as container has been created

Copy link
Author

Choose a reason for hiding this comment

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

Restored.

//Keeping real containerId information, but using containerUniqueName as containerId
private String containerId;

private String containerUniqueName;

private DockerAPI dockerAPI;

private boolean removeVolumes;

private String cloudId;

public DockerTransientNode(@Nonnull String containerId, String remoteFS, ComputerLauncher launcher) throws Descriptor.FormException, IOException {
super("docker-" + containerId.substring(0,12), remoteFS, launcher);

public DockerTransientNode(@Nonnull String uniqueName, String workdir, ComputerLauncher launcher) throws Descriptor.FormException, IOException {
super("docker-" + uniqueName, workdir, launcher);
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't hurt to also store containerName as an attribute

Copy link
Author

Choose a reason for hiding this comment

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

Done.

this.containerUniqueName = uniqueName;
setNumExecutors(1);
setMode(Mode.EXCLUSIVE);
setRetentionStrategy(new DockerOnceRetentionStrategy(10));
}

public void setRealContainerId(String containerId){
this.containerId = containerId;
}

public String getContainerId() {
return containerId;
public String getContainerId(){
// For old sake. If the containerUniqueName was not introduced yet, use the containerId
return containerUniqueName == null ? containerId : containerUniqueName;
}

public void setDockerAPI(DockerAPI dockerAPI) {
Expand Down Expand Up @@ -92,7 +98,7 @@ public void terminate(TaskListener listener) {
}

Computer.threadPoolForRemoting.submit(() -> {

final String containerId = getContainerId();
DockerClient client = dockerAPI.getClient();

try {
Expand Down
7 changes: 5 additions & 2 deletions src/main/java/io/jenkins/docker/connector/DockerComputerAttachConnector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import com.github.dockerjava.api.command.ExecCreateCmd;
import com.github.dockerjava.api.command.ExecCreateCmdResponse;
import com.github.dockerjava.api.command.InspectContainerResponse;
import com.nirima.jenkins.plugins.docker.DockerTemplate;
import hudson.Extension;
import hudson.model.Descriptor;
import hudson.model.TaskListener;
Expand All @@ -18,7 +19,6 @@
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.DataBoundSetter;

import java.io.FilterOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
Expand Down Expand Up @@ -52,6 +52,7 @@ public void setUser(String user) {
this.user = user;
}


@Override
public void beforeContainerCreated(DockerAPI api, String workdir, CreateContainerCmd cmd) throws IOException, InterruptedException {
ensureWaiting(cmd);
Expand All @@ -64,7 +65,9 @@ public void afterContainerStarted(DockerAPI api, String workdir, String containe
}

@Override
protected ComputerLauncher createLauncher(DockerAPI api, String workdir, InspectContainerResponse inspect, TaskListener listener) throws IOException, InterruptedException {
protected ComputerLauncher createLauncher(DockerAPI api, String workdir, DockerTemplate.ContainerCommandCreator containerCommandCreator, TaskListener listener) throws IOException, InterruptedException {
CreateContainerCmd cmd = containerCommandCreator.createContainerCmd(api);
InspectContainerResponse inspect = executeContainer(api, listener, cmd, workdir);
return new DockerAttachLauncher(api, inspect.getId(), user, workdir);
}

Expand Down
38 changes: 31 additions & 7 deletions src/main/java/io/jenkins/docker/connector/DockerComputerConnector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import com.github.dockerjava.api.DockerClient;
import com.github.dockerjava.api.command.CreateContainerCmd;
import com.github.dockerjava.api.command.InspectContainerResponse;
import com.github.dockerjava.api.exception.DockerException;
import com.github.dockerjava.api.exception.NotFoundException;
import com.nirima.jenkins.plugins.docker.DockerSlave;
import com.nirima.jenkins.plugins.docker.DockerTemplate;
Expand All @@ -15,6 +16,7 @@
import hudson.slaves.ComputerLauncher;
import hudson.slaves.DelegatingComputerLauncher;
import hudson.slaves.SlaveComputer;
import io.jenkins.docker.DockerComputer;
import io.jenkins.docker.DockerTransientNode;
import io.jenkins.docker.client.DockerAPI;
import org.slf4j.Logger;
Expand All @@ -23,6 +25,7 @@
import javax.annotation.Nonnull;
import java.io.File;
import java.io.IOException;
import java.io.Serializable;

/**
* Create a {@link DockerSlave} based on a template. Container is created in detached mode so it can survive
Expand Down Expand Up @@ -89,31 +92,51 @@ protected String injectRemotingJar(String containerId, String workdir, DockerCli
return workdir + '/' + remoting.getName();
}

public final ComputerLauncher createLauncher(final DockerAPI api, @Nonnull final String containerId, String workdir, TaskListener listener) throws IOException, InterruptedException {
protected final InspectContainerResponse executeContainer(DockerAPI dockerAPI, TaskListener listener, CreateContainerCmd cmd, String workdir) throws IOException, InterruptedException{

final InspectContainerResponse inspect = api.getClient().inspectContainerCmd(containerId).exec();
beforeContainerCreated(dockerAPI, workdir, cmd);
final String containerId = cmd.exec().getId();

try {
beforeContainerStarted(dockerAPI, workdir, containerId);

dockerAPI.getClient().startContainerCmd(containerId).exec();

afterContainerStarted(dockerAPI, workdir, containerId);
} catch (DockerException e) {
// if something went wrong, cleanup aborted container
dockerAPI.getClient().removeContainerCmd(containerId).withForce(true).exec();
throw e;
}
final InspectContainerResponse inspect = dockerAPI.getClient().inspectContainerCmd(containerId).exec();
final Boolean running = inspect.getState().getRunning();
if (Boolean.FALSE.equals(running)) {
listener.error("Container {} is not running. {}", containerId, inspect.getState().getStatus());
throw new IOException("Container is not running.");
}

final ComputerLauncher launcher = createLauncher(api, workdir, inspect, listener);
return inspect;
}

public final ComputerLauncher createLauncher(final DockerAPI api, DockerTemplate.ContainerCommandCreator containerCommandCreator, String workdir, TaskListener listener) throws IOException, InterruptedException {

final ComputerLauncher launcher = createLauncher(api, workdir, containerCommandCreator, listener);
return new DelegatingComputerLauncher(launcher) {

@Override
public void launch(SlaveComputer computer, TaskListener listener) throws IOException, InterruptedException {
try {
api.getClient().inspectContainerCmd(containerId).exec();
super.launch(computer, listener);
final String containerUniqueName = ((DockerComputer)computer).getNode().getContainerId();
InspectContainerResponse response = api.getClient().inspectContainerCmd(containerUniqueName).exec();
((DockerComputer)computer).getNode().setRealContainerId(response.getId());
} catch (NotFoundException e) {
// Container has been removed
Queue.withLock( () -> {
DockerTransientNode node = (DockerTransientNode) computer.getNode();
node.terminate(listener);
});
return;
}
super.launch(computer, listener);
}
};
}
Expand All @@ -122,6 +145,7 @@ public void launch(SlaveComputer computer, TaskListener listener) throws IOExcep
* Create a Launcher to create an Agent with this container. Can assume container has been created by this
* DockerAgentConnector so adequate setup did take place.
*/
protected abstract ComputerLauncher createLauncher(DockerAPI api, String workdir, InspectContainerResponse inspect, TaskListener listener) throws IOException, InterruptedException;
protected abstract ComputerLauncher createLauncher(DockerAPI api, String workdir, DockerTemplate.ContainerCommandCreator containerCommandCreator, TaskListener listener) throws IOException, InterruptedException;


}
Loading