Skip to content

Jenkins 60473 #305

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

Closed
wants to merge 2 commits into from
Closed

Jenkins 60473 #305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9971adc
Fixed Absolute path, and container not stopping issues
ThugPigeon653 Sep 17, 2023
b2a6c3c
Removed un-used variable assignment
ThugPigeon653 Sep 17, 2023
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
2 changes: 1 addition & 1 deletion src/main/java/org/jenkinsci/plugins/docker/workflow/WithContainerStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ public Execution() {
volumes.put(tmp, tmp);
}

String command = launcher.isUnix() ? "cat" : "cmd.exe";
String command = "cat";
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the motivation for this change. Can you provide more details?

Did you test with a Windows container on a Windows host? I would expect a Windows container on a Windows host to report that cat could not be found.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like this is attempting to solve an issue in one environment (a Windows agent with Docker Desktop installed and I suppose running Linux containers) while breaking the plugin in other environments (a Windows agent running Windows containers). Does not look safe as written.

Copy link
Member

Choose a reason for hiding this comment

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

Offhand it sounds like this bug might have multiple components—a need for an option to customize the sleep command (here), and some issue with cross-OS file path calculations.

container = dockerClient.run(env, step.image, step.args, ws, volumes, volumesFromContainers, envReduced, dockerClient.whoAmI(), /* expected to hang until killed */ command);
final List<String> ps = dockerClient.listProcess(env, container);
if (!ps.contains(command)) {
Expand Down
11 changes: 8 additions & 3 deletions src/main/java/org/jenkinsci/plugins/docker/workflow/client/DockerClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,10 +182,15 @@ public List<String> listProcess(@NonNull EnvVars launchEnv, @NonNull String cont
* @param containerId The container ID.
*/
public void stop(@NonNull EnvVars launchEnv, @NonNull String containerId) throws IOException, InterruptedException {
LaunchResult result = launch(launchEnv, false, "stop", "--time=1", containerId);
if (result.getStatus() != 0) {
throw new IOException(String.format("Failed to kill container '%s'.", containerId));
stop(launchEnv, containerId, 1);
}
public void stop(@NonNull EnvVars launchEnv, @NonNull String containerId, @NonNull int stopTime) throws IOException, InterruptedException {
launch(launchEnv, false, "docker", "stop", String.format("--time=%s", stopTime), containerId);
Thread.sleep((long)stopTime*1100);
Copy link
Member

Choose a reason for hiding this comment

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

Unclear what this sleep is for.

if(inspect(launchEnv, containerId, ".Name")==null){
throw new IOException(String.format("Container '%s' failed to kill.", containerId));
}

if (!SKIP_RM_ON_STOP) {
rm(launchEnv, containerId);
}
Expand Down
18 changes: 15 additions & 3 deletions src/main/java/org/jenkinsci/plugins/docker/workflow/client/WindowsDockerClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,14 @@ public WindowsDockerClient(@NonNull Launcher launcher, @CheckForNull Node node,
this.node = node;
}

private String getDockerFormattedPath(@NonNull String path){
path=path.replaceAll(":", "");
if(!path.startsWith("/")&&path.contains("/")){
path="/"+path;
}
return path;
}

@Override
public String run(@NonNull EnvVars launchEnv, @NonNull String image, @CheckForNull String args, @CheckForNull String workdir, @NonNull Map<String, String> volumes, @NonNull Collection<String> volumesFromContainers, @NonNull EnvVars containerEnv, @NonNull String user, @NonNull String... command) throws IOException, InterruptedException {
ArgumentListBuilder argb = new ArgumentListBuilder("docker", "run", "-d", "-t");
Expand All @@ -37,10 +45,10 @@ public String run(@NonNull EnvVars launchEnv, @NonNull String image, @CheckForNu
}

if (workdir != null) {
argb.add("-w", workdir);
argb.add("-w", getDockerFormattedPath(workdir));
}
for (Map.Entry<String, String> volume : volumes.entrySet()) {
argb.add("-v", volume.getKey() + ":" + volume.getValue());
argb.add("-v", getDockerFormattedPath(volume.getKey()) + ":" + getDockerFormattedPath(volume.getValue()));
}
for (String containerId : volumesFromContainers) {
argb.add("--volumes-from", containerId);
Expand Down Expand Up @@ -113,7 +121,11 @@ public String whoAmI() throws IOException, InterruptedException {
}

private LaunchResult launch(EnvVars env, boolean quiet, FilePath workDir, String... args) throws IOException, InterruptedException {
return launch(env, quiet, workDir, new ArgumentListBuilder(args));
String[] newArgs = new String[args.length];
for (int i = 0; i < args.length; i++) {
newArgs[i]=getDockerFormattedPath(args[i]);
}
return launch(env, quiet, workDir, new ArgumentListBuilder(newArgs));
}
private LaunchResult launch(EnvVars env, boolean quiet, FilePath workDir, ArgumentListBuilder argb) throws IOException, InterruptedException {
if (LOGGER.isLoggable(Level.FINE)) {
Expand Down