Skip to content

Commit

Permalink
Prospective fix for jenkinsci#757 and JENKINS-59790.
Browse files Browse the repository at this point in the history 
Refactor DockerComputerConnector.beforeContainerStarted(...) and
afterContainerStarted(...) so it takes a node (that holds a containerId)
instead of a containerId.
Refactor DockerTransientNode constructors so the launcher can be
provided post-construction.
Refactor DockerTemplate.doProvision(...) so that the DockerTransientNode
is created without a launcher before we start the container and the
launcher is only set after the container is started.
Enhance DockerComputerJNLPConnector so that it adds the node to Jenkins
immediately before the container is created, ensuring that it'll be
known to Jenkins (just) before the JNLP process starts.
  • Loading branch information
@pjdarton
pjdarton committed Apr 28, 2020
1 parent 20593ea commit c117ae1
Show file tree
Hide file tree
Showing 6 changed files with 74 additions and 17 deletions.
12 changes: 6 additions & 6 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 @@ -716,12 +716,7 @@ private DockerTransientNode doProvisionNode(final DockerAPI api, final DockerCli
LOGGER.info("Started container ID {} for node {} from image: {}", containerId, nodeName, ourImage);

try {
ourConnector.beforeContainerStarted(api, effectiveRemoteFsDir, containerId);
client.startContainerCmd(containerId).exec();
ourConnector.afterContainerStarted(api, effectiveRemoteFsDir, containerId);

final ComputerLauncher nodeLauncher = ourConnector.createLauncher(api, containerId, effectiveRemoteFsDir, listener);
final DockerTransientNode node = new DockerTransientNode(nodeName, containerId, effectiveRemoteFsDir, nodeLauncher);
final DockerTransientNode node = new DockerTransientNode(nodeName, containerId, effectiveRemoteFsDir);
node.setNodeDescription("Docker Agent [" + ourImage + " on "+ api.getDockerHost().getUri() + " ID " + containerId + "]");
node.setMode(getMode());
node.setLabelString(getLabelString());
Expand All @@ -730,6 +725,11 @@ private DockerTransientNode doProvisionNode(final DockerAPI api, final DockerCli
node.setRemoveVolumes(isRemoveVolumes());
node.setStopTimeout(getStopTimeout());
node.setDockerAPI(api);
ourConnector.beforeContainerStarted(api, effectiveRemoteFsDir, node);
client.startContainerCmd(containerId).exec();
ourConnector.afterContainerStarted(api, effectiveRemoteFsDir, node);
final ComputerLauncher nodeLauncher = ourConnector.createLauncher(api, containerId, effectiveRemoteFsDir, listener);
node.setLauncher(nodeLauncher);
finallyRemoveTheContainer = false;
return node;
} finally {
Expand Down
40 changes: 37 additions & 3 deletions src/main/java/io/jenkins/docker/DockerTransientNode.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ public class DockerTransientNode extends Slave {
private static final long serialVersionUID = 1349729340506926183L;
private static final Logger LOGGER = LoggerFactory.getLogger(DockerTransientNode.class.getName());

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

private transient DockerAPI dockerAPI;
Expand All @@ -48,8 +47,42 @@ public class DockerTransientNode extends Slave {

private AtomicBoolean acceptingTasks = new AtomicBoolean(true);

public DockerTransientNode(@Nonnull String nodeName, String containerId, String workdir, ComputerLauncher launcher) throws Descriptor.FormException, IOException {
super(nodeName, workdir, launcher);
/**
* @deprecated Use {@link #DockerTransientNode(String, String, String)} then
* {@link #setLauncher(ComputerLauncher)}.
* @param nodeName Passed to
* {@link #DockerTransientNode(String, String, String)}.
* @param containerId Passed to
* {@link #DockerTransientNode(String, String, String)}.
* @param workdir Passed to
* {@link #DockerTransientNode(String, String, String)}.
* @param launcher Passed to {@link #setLauncher(ComputerLauncher)}
* @throws Descriptor.FormException See
* {@link #DockerTransientNode(String, String, String)}.
* @throws IOException See {@link #DockerTransientNode(String, String, String)}.
*/
@Deprecated
public DockerTransientNode(@Nonnull String nodeName, @Nonnull String containerId, String workdir, ComputerLauncher launcher) throws Descriptor.FormException, IOException {
this(nodeName, containerId, workdir);
setLauncher(launcher);
}

/**
* Preferred constructor. Note that, unless this is a JNLP node, callers will
* later have to call {@link #setLauncher(ComputerLauncher)}.
*
* @param nodeName Name of the node; passed to
* {@link Slave#Slave(String, String, ComputerLauncher)}.
* @param containerId Docker container id.
* @param workdir remoteFs home dir; passed to
* {@link Slave#Slave(String, String, ComputerLauncher)}.
* @throws Descriptor.FormException See
* {@link Slave#Slave(String, String, ComputerLauncher)}.
* @throws IOException See
* {@link Slave#Slave(String, String, ComputerLauncher)}.
*/
public DockerTransientNode(@Nonnull String nodeName, @Nonnull String containerId, String workdir) throws Descriptor.FormException, IOException {
super(nodeName, workdir, null);
this.containerId = containerId;
setNumExecutors(1);
setMode(Mode.EXCLUSIVE);
Expand All @@ -65,6 +98,7 @@ public void setAcceptingTasks(boolean acceptingTasks) {
this.acceptingTasks.set(acceptingTasks);
}

@Nonnull
public String getContainerId(){
return containerId;
}
Expand Down
4 changes: 3 additions & 1 deletion src/main/java/io/jenkins/docker/connector/DockerComputerAttachConnector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import hudson.remoting.Channel;
import hudson.slaves.ComputerLauncher;
import hudson.slaves.SlaveComputer;
import io.jenkins.docker.DockerTransientNode;
import io.jenkins.docker.client.DockerAPI;
import io.jenkins.docker.client.DockerMultiplexedInputStream;
import jenkins.model.Jenkins;
Expand Down Expand Up @@ -160,7 +161,8 @@ public void beforeContainerCreated(DockerAPI api, String workdir, CreateContaine
}

@Override
public void afterContainerStarted(DockerAPI api, String workdir, String containerId) throws IOException, InterruptedException {
public void afterContainerStarted(DockerAPI api, String workdir, DockerTransientNode node) throws IOException, InterruptedException {
final String containerId = node.getContainerId();
try(final DockerClient client = api.getClient()) {
injectRemotingJar(containerId, workdir, client);
}
Expand Down
19 changes: 13 additions & 6 deletions src/main/java/io/jenkins/docker/connector/DockerComputerConnector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import com.github.dockerjava.api.DockerClient;
import com.github.dockerjava.api.command.CreateContainerCmd;
import com.github.dockerjava.api.command.InspectContainerResponse;
import com.nirima.jenkins.plugins.docker.DockerSlave;
import com.thoughtworks.xstream.InitializationException;

import hudson.DescriptorExtensionList;
Expand All @@ -16,6 +15,7 @@
import hudson.slaves.ComputerLauncher;
import hudson.slaves.NodeProperty;
import hudson.util.LogTaskListener;
import io.jenkins.docker.DockerTransientNode;
import io.jenkins.docker.client.DockerAPI;
import jenkins.model.Jenkins;

Expand All @@ -31,7 +31,7 @@
import java.util.logging.Logger;

/**
* Create a {@link DockerSlave} based on a template. Container is created in detached mode so it can survive
* Create a {@link DockerTransientNode} based on a template. Container is created in detached mode so it can survive
* a jenkins restart (typically when Pipelines are used) then a launcher can re-connect. In many cases this
* means container is running a dummy command as main process, then launcher is establish with `docker exec`.
*
Expand All @@ -40,9 +40,8 @@
public abstract class DockerComputerConnector extends AbstractDescribableImpl<DockerComputerConnector> {
private static final Logger LOGGER = Logger.getLogger(DockerComputerConnector.class.getName());
private static final TaskListener LOGGER_LISTENER = new LogTaskListener(LOGGER, Level.FINER);

/** Name of the remoting jar file */
protected static final File remoting;

static {
try {
remoting = Which.jarFile(Channel.class);
Expand Down Expand Up @@ -78,13 +77,13 @@ public void beforeContainerCreated(@Nonnull DockerAPI api, @Nonnull String workd
* using {@link #injectRemotingJar(String, String, DockerClient)}
*/
@SuppressWarnings("unused")
public void beforeContainerStarted(@Nonnull DockerAPI api, @Nonnull String workdir, @Nonnull String containerId) throws IOException, InterruptedException {}
public void beforeContainerStarted(@Nonnull DockerAPI api, @Nonnull String workdir, @Nonnull DockerTransientNode node) throws IOException, InterruptedException {}

/**
* Container has started. Good place to check it's healthy before considering agent is ready to accept connexions
*/
@SuppressWarnings("unused")
public void afterContainerStarted(@Nonnull DockerAPI api, @Nonnull String workdir, @Nonnull String containerId) throws IOException, InterruptedException {}
public void afterContainerStarted(@Nonnull DockerAPI api, @Nonnull String workdir, @Nonnull DockerTransientNode node) throws IOException, InterruptedException {}


/**
Expand All @@ -100,6 +99,14 @@ protected void ensureWaiting(@Nonnull CreateContainerCmd cmd) {
}
}

/**
* Ensure that a DockerNode is known to Jenkins so that Jenkins will accept an incoming JNLP connection etc.
* @throws IOException if Jenkins is unable to persist the details.
*/
protected void ensureNodeIsKnown(DockerTransientNode node) throws IOException {
Jenkins.getInstance().addNode(node);
}

/**
* Utility method to copy remoting runtime into container on specified working directory
*/
Expand Down
12 changes: 12 additions & 0 deletions src/main/java/io/jenkins/docker/connector/DockerComputerJNLPConnector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import hudson.model.TaskListener;
import hudson.slaves.ComputerLauncher;
import hudson.slaves.JNLPLauncher;
import io.jenkins.docker.DockerTransientNode;
import io.jenkins.docker.client.DockerAPI;
import io.jenkins.docker.client.DockerEnvUtils;
import jenkins.model.Jenkins;
Expand Down Expand Up @@ -202,6 +203,17 @@ public void beforeContainerCreated(DockerAPI api, String workdir, CreateContaine
}
}

@Override
public void beforeContainerStarted(DockerAPI api, String workdir, DockerTransientNode node)
throws IOException, InterruptedException {
// For JNLP, we need to have the Jenkins Node known to Jenkins as a valid JNLP
// node before the container starts, otherwise it might get started before
// Jenkins is ready for it.
// That's why we explicitly add the node here instead of allowing the cloud
// provisioning process to add it later.
ensureNodeIsKnown(node);
}

private static EnvVars calculateVariablesForVariableSubstitution(final String nodeName, final String secret,
final String jnlpTunnel, final String jenkinsUrl) throws IOException, InterruptedException {
final EnvVars knownVariables = new EnvVars();
Expand Down
4 changes: 3 additions & 1 deletion src/main/java/io/jenkins/docker/connector/DockerComputerSSHConnector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import hudson.security.ACL;
import hudson.slaves.ComputerLauncher;
import hudson.util.ListBoxModel;
import io.jenkins.docker.DockerTransientNode;
import io.jenkins.docker.client.DockerAPI;
import jenkins.bouncycastle.api.PEMEncodable;
import jenkins.model.Jenkins;
Expand Down Expand Up @@ -252,9 +253,10 @@ public void beforeContainerCreated(DockerAPI api, String workdir, CreateContaine
}

@Override
public void beforeContainerStarted(DockerAPI api, String workdir, String containerId) throws IOException, InterruptedException {
public void beforeContainerStarted(DockerAPI api, String workdir, DockerTransientNode node) throws IOException, InterruptedException {
final String key = sshKeyStrategy.getInjectedKey();
if (key != null) {
final String containerId = node.getContainerId();
final String authorizedKeysCommand = "#!/bin/sh\n"
+ "[ \"$1\" = \"" + sshKeyStrategy.getUser() + "\" ] "
+ "&& echo '" + key + "'"
Expand Down

0 comments on commit c117ae1

Please sign in to comment.