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

[JENKINS-39748] provide Image.exec() to avoid disabling ENTRYPOINT in the image #78

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[JENKINS-39748] provide Image.exec() to avoid disabling ENTRYPOINT in the image #78

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
20 changes: 16 additions & 4 deletions src/main/java/org/jenkinsci/plugins/docker/workflow/WithContainerStep.java
View file
Open in desktop
Expand Up @@ -71,16 +71,17 @@
import org.kohsuke.stapler.DataBoundSetter;

public class WithContainerStep extends AbstractStepImpl {

private static final Logger LOGGER = Logger.getLogger(WithContainerStep.class.getName());
private final @Nonnull String image;
private String entrypoint;
private String args;
private String toolName;

@DataBoundConstructor public WithContainerStep(@Nonnull String image) {
this.image = image;
}

public String getImage() {
return image;
}
Expand All @@ -90,6 +91,15 @@ public void setArgs(String args) {
this.args = Util.fixEmpty(args);
}

@DataBoundSetter
public void setEntrypoint(String entrypoint) {
this.entrypoint = entrypoint;
}

public String getEntrypoint() {
return entrypoint;
}

public String getArgs() {
return args;
}
Expand Down Expand Up @@ -171,8 +181,10 @@ public static class Execution extends AbstractStepExecutionImpl {
volumes.put(ws, ws);
volumes.put(tmp, tmp);
}

container = dockerClient.run(env, step.image, step.args, ws, volumes, volumesFromContainers, envReduced, dockerClient.whoAmI(), /* expected to hang until killed */ "cat");
if (step.entrypoint == null) {
step.entrypoint = "cat";
Copy link
Member

Choose a reason for hiding this comment

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

See the developer guide: better to initialize the field with the "cat" value, have the setter call Util.fixEmpty, and have Image.exec specify entrypoint: ''. Also be sure to adjust WithContainerStep/config.jelly to specify

<f:entry field="entrypoint" title="…">
    <f:textbox default="cat"/> <!-- or ${descriptor.defaultEntrypoint}, if defined -->
<f:entry>

and add help-entrypoint.html.

But anyway better would probably be to change this to a boolean field, since

  • there is no use case for doing anything other than leaving the image along, or overriding its entrypoint to go to sleep
  • there is a good reason to consider replacing cat with something like sleep infinity (except with a more POSIX-compliant argument)

The special techniques for step default values are the same for boolean parameters, but if you pick the semantics so that the default is in fact false, it is somewhat easier to write:

<f:checkbox/>

rather than

<f:checkbox default="true"/>

and a simpler form in the Java class.

}
container = dockerClient.run(env, step.image, step.args, ws, volumes, volumesFromContainers, envReduced, dockerClient.whoAmI(), /* expected to hang until killed */ step.entrypoint);
DockerFingerprints.addRunFacet(dockerClient.getContainerRecord(env, container), run);
ImageAction.add(step.image, run);
getContext().newBodyInvoker().
Expand Down
34 changes: 19 additions & 15 deletions src/main/java/org/jenkinsci/plugins/docker/workflow/client/DockerClient.java
View file
Open in desktop
Expand Up @@ -53,7 +53,7 @@

/**
* Simple docker client for Pipeline.
*
*
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid making extraneous diff hunks, particular whitespace changes. You should be able to configure popular editors to not touch lines of code unless and until you edit them.

* @author <a href="mailto:tom.fennelly@gmail.com">tom.fennelly@gmail.com</a>
*/
public class DockerClient {
Expand All @@ -62,10 +62,10 @@ public class DockerClient {

// e.g. 2015-04-09T13:40:21.981801679Z
public static final String DOCKER_DATE_TIME_FORMAT = "yyyy-MM-dd'T'HH:mm:ss";

/**
* Known cgroup formats
*
*
* 4:cpuset:/system.slice/docker-3dd988081e7149463c043b5d9c57d7309e079c5e9290f91feba1cc45a04d6a5b.scope
* 2:cpu:/docker/3dd988081e7149463c043b5d9c57d7309e079c5e9290f91feba1cc45a04d6a5b
*/
Expand Down Expand Up @@ -102,7 +102,7 @@ public String run(@Nonnull EnvVars launchEnv, @Nonnull String image, @CheckForNu
if (args != null) {
argb.addTokenized(args);
}

if (workdir != null) {
argb.add("-w", workdir);
}
Expand All @@ -116,7 +116,11 @@ public String run(@Nonnull EnvVars launchEnv, @Nonnull String image, @CheckForNu
argb.add("-e");
argb.addMasked(variable.getKey()+"="+variable.getValue());
}
argb.add("--entrypoint").add(entrypoint).add(image);
if (!entrypoint.equals("--")) {
Copy link
Member

Choose a reason for hiding this comment

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

Better to make entrypoint be @CheckForNull.

argb.add("--entrypoint", entrypoint);
}

argb.add(image);

LaunchResult result = launch(launchEnv, false, null, argb);
if (result.getStatus() == 0) {
Expand All @@ -128,10 +132,10 @@ public String run(@Nonnull EnvVars launchEnv, @Nonnull String image, @CheckForNu

/**
* Stop a container.
*
* <p>
*
* <p>
* Also removes ({@link #rm(EnvVars, String)}) the container.
*
*
* @param launchEnv Docker client launch environment.
* @param containerId The container ID.
*/
Expand All @@ -145,7 +149,7 @@ public void stop(@Nonnull EnvVars launchEnv, @Nonnull String containerId) throws

/**
* Remove a container.
*
*
* @param launchEnv Docker client launch environment.
* @param containerId The container ID.
*/
Expand All @@ -172,7 +176,7 @@ public void rm(@Nonnull EnvVars launchEnv, @Nonnull String containerId) throws I
return null;
}
}

/**
* Inspect a docker image/container.
* @param launchEnv Docker client launch environment.
Expand All @@ -183,15 +187,15 @@ public void rm(@Nonnull EnvVars launchEnv, @Nonnull String containerId) throws I
* @throws InterruptedException Interrupted
* @since 1.1
*/
public @Nonnull String inspectRequiredField(@Nonnull EnvVars launchEnv, @Nonnull String objectId,
public @Nonnull String inspectRequiredField(@Nonnull EnvVars launchEnv, @Nonnull String objectId,
@Nonnull String fieldPath) throws IOException, InterruptedException {
final String fieldValue = inspect(launchEnv, objectId, fieldPath);
if (fieldValue == null) {
throw new IOException("Cannot retrieve " + fieldPath + " from 'docker inspect" + objectId + "'");
}
return fieldValue;
}

private @CheckForNull Date getCreatedDate(@Nonnull EnvVars launchEnv, @Nonnull String objectId) throws IOException, InterruptedException {
String createdString = inspect(launchEnv, objectId, "json .Created");
if (createdString == null) {
Expand Down Expand Up @@ -220,7 +224,7 @@ public void rm(@Nonnull EnvVars launchEnv, @Nonnull String containerId) throws I
return null;
}
}

private static final Pattern pattern = Pattern.compile("^(\\D+)(\\d+)\\.(\\d+)\\.(\\d+)(.*)");
/**
* Parse a Docker version string (e.g. "Docker version 1.5.0, build a8a31ef").
Expand All @@ -237,7 +241,7 @@ protected static VersionNumber parseVersionNumber(@Nonnull String versionString)
return new VersionNumber(String.format("%s.%s.%s", major, minor, maint));
} else {
return null;
}
}
}

private LaunchResult launch(@Nonnull EnvVars launchEnv, boolean quiet, @Nonnull String... args) throws IOException, InterruptedException {
Expand Down Expand Up @@ -317,7 +321,7 @@ public ContainerRecord getContainerRecord(@Nonnull EnvVars launchEnv, String con

// TODO get tags and add for ContainerRecord
return new ContainerRecord(host, containerId, image, containerName,
(created != null ? created.getTime() : 0L),
(created != null ? created.getTime() : 0L),
Collections.<String,String>emptyMap());
}

Expand Down
11 changes: 11 additions & 0 deletions src/main/resources/org/jenkinsci/plugins/docker/workflow/Docker.groovy
View file
Open in desktop
Expand Up @@ -125,6 +125,17 @@ class Docker implements Serializable {
}
}

public <V> V exec(String args = '', Closure<V> body) {
docker.node {
if (docker.script.sh(script: "docker inspect -f . ${id}", returnStatus: true) != 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Going to clash with #84…

pull()
}
docker.script.withDockerContainer(image: id, args: args, toolName: docker.script.env.DOCKER_TOOL_NAME, /* don't override entrypoint */ entrypoint: "--") {
body()
}
}
}

public void pull() {
docker.node {
docker.script.sh "docker pull ${imageName()}"
Expand Down
6 changes: 6 additions & 0 deletions src/main/resources/org/jenkinsci/plugins/docker/workflow/DockerDSL/help.jelly
View file
Open in desktop
Expand Up @@ -78,6 +78,12 @@
These commands run in the same working directory (normally a slave workspace), which means that the Docker server must be on localhost.
</p>
</dd>
<dt><code>Image.exec[(args)] {…}</code></dt>
<dd>
<p>
Like <code>inside</code> this starts a container for the duration of the body with all external commands run in the container. Unlike <code>inside</code> this does not overwrite ENTRYPOINT.
</p>
</dd>
<dt><code>Image.tag([tagname])</code></dt>
<dd>
<p>
Expand Down
50 changes: 50 additions & 0 deletions src/test/java/org/jenkinsci/plugins/docker/workflow/DockerDSLTest.java
View file
Open in desktop
Expand Up @@ -423,4 +423,54 @@ private static void grep(File dir, String text, String prefix, Set<String> match
}
});
}

@Issue("JENKINS-37987")
@Test public void image_exec() {
story.addStep(new Statement() {
@Override public void evaluate() throws Throwable {
assumeDocker();
WorkflowJob p = story.j.jenkins.createProject(WorkflowJob.class, "build");
p.setDefinition(new CpsFlowDefinition(
"node {\n" +
" writeFile file: 'Dockerfile', text: '# This is a test.\\n\\nFROM alpine\\nRUN chmod u+s /bin/busybox \\nENTRYPOINT [\\\"/bin/ping\\\" ] \\nCMD [\\\"localhost\\\"] \\n'\n" +
" def built = docker.build 'alpine-entrypoint-test'\n" +
" echo \"built ${built.id}\"\n" +
"}", true));
WorkflowRun b = story.j.assertBuildStatusSuccess(p.scheduleBuild2(0));
}
});
story.addStep(new Statement() {
Copy link
Member

Choose a reason for hiding this comment

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

Gratuitous Jenkins restarts.

@Override public void evaluate() throws Throwable {
assumeDocker();
WorkflowJob p = story.j.jenkins.createProject(WorkflowJob.class, "prj");
p.setDefinition(new CpsFlowDefinition(
"def r = docker.image('alpine-entrypoint-test').exec {\n" +
" semaphore 'wait'\n" +
" sh 'cat /etc/hosts'\n" +
" 42\n" +
"}; echo \"the answer is ${r}\"", true));
WorkflowRun b = p.scheduleBuild2(0).waitForStart();
SemaphoreStep.waitForStart("wait/1", b);
}
});
story.addStep(new Statement() {
@Override public void evaluate() throws Throwable {
SemaphoreStep.success("wait/1", null);
WorkflowJob p = story.j.jenkins.getItemByFullName("prj", WorkflowJob.class);
WorkflowRun b = p.getLastBuild();
story.j.assertLogContains("ip6-localhost", story.j.assertBuildStatusSuccess(story.j.waitForCompletion(b)));
story.j.assertLogContains("the answer is 42", b);
DockerClient client = new DockerClient(new Launcher.LocalLauncher(StreamTaskListener.NULL), null, null);
String aetIID = client.inspect(new EnvVars(), "alpine-entrypoint-test", ".Id");
Fingerprint f = DockerFingerprints.of(aetIID);
assertNotNull(f);
DockerRunFingerprintFacet facet = f.getFacet(DockerRunFingerprintFacet.class);
assertNotNull(facet);
assertEquals(1, facet.records.size());
assertNotNull(facet.records.get(0).getContainerName());
assertEquals(Fingerprint.RangeSet.fromString("1", false), facet.getRangeSet(p));
assertEquals(Collections.singleton("alpine-entrypoint-test"), DockerImageExtractor.getDockerImagesUsedByJobFromAll(p));
}
});
}
}