Jenkins 60473 by ThugPigeon653 · Pull Request #305 · jenkinsci/docker-workflow-plugin

ThugPigeon653 · 2023-09-17T03:49:08Z

Testing done

### Submitter checklist
- [x] Make sure you are opening from a **topic/feature/bugfix branch** (right side) and not your main branch!
- [x] Ensure that the pull request title represents the desired changelog entry
- [x] Please describe what you did
- [x] Link to relevant issues in GitHub or Jira
- [x] Link to relevant pull requests, esp. upstream and downstream changes
- [x] Ensure you have provided tests - that demonstrates feature works or fixes the issue

ThugPigeon653 · 2023-09-17T03:58:05Z

I fixed the absolute path error by enforcing the correct formatting for docker paths, in the WindowsDockerClient. This avoided changing any upstream path variables, so as to not interfere with other usages of the path variables.
Once that was fixed, it became clear that containers were not stopping correctly, making some unit tests fail. This was because the application was synchronously getting the 'stopped' state from the docker command to stop the container. Because it was not synchronously returning the containerID, this was failing (even though the command was correct). I rectified this by making the stop method wait for the container stop time to elapse, then inspect the container. This allowed containers to shut down as expected, and return an accurate status.
Finally WithContainerStep attempting to run 'cmd.exe' on an Ubuntu container, when the agent is Windows. Top fix this, I skipped the 'isunix' check, and set the command variable to 'cat'. The OS of the host running the agent has no bearing on which commands should be run inside the container.

MarkEWaite

I don't see a test that shows the problem. Is a test feasible, even if that test cannot be run in the ci.jenkins.io environment?

MarkEWaite · 2023-09-18T02:54:47Z

src/main/java/org/jenkinsci/plugins/docker/workflow/WithContainerStep.java

            }

-            String command = launcher.isUnix() ? "cat" : "cmd.exe";
+            String command = "cat";


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.

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.

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.

MarkEWaite · 2023-09-18T02:55:16Z

src/main/java/org/jenkinsci/plugins/docker/workflow/client/DockerClient.java

+        stop(launchEnv, containerId, 1);
+    }
+    public void stop(@NonNull EnvVars launchEnv, @NonNull String containerId, @NonNull int stopTime) throws IOException, InterruptedException {
+        LaunchResult result = launch(launchEnv, false, "docker", "stop", String.format("--time=%s", stopTime), containerId);


Needs the spotbugs warning resolved, either by checking the value of result or by not assigning to result.

jglick · 2023-09-18T11:15:08Z

https://issues.jenkins.io/browse/JENKINS-60473 FTR; if you are working on an issue, you can assign to yourself in Jira and mark it In Review.

jglick · 2023-09-18T11:18:26Z

Also please link to earlier attempts: #197, #234, #235.

jglick

This plugin has no effective test coverage on Windows so it is unclear what use cases you would be breaking by touching behavior like this. (And the patch seems to also affect behavior on Linux, in probably unwanted ways.)

My general advice with this plugin is to use it when it is convenient if it works out of the box; but at the first sign of trouble, ignore it and switch to running docker CLI commands directly, testing your proposed commands directly on an example agent until you find a script that works. Experience has proven that the range of ways people expect Docker to work in builds is too broad for a plugin to comprehensively and reliably handle, and the original idea behind this plugin was ill conceived.

jglick · 2023-09-18T11:20:25Z

src/main/java/org/jenkinsci/plugins/docker/workflow/WithContainerStep.java

            }

-            String command = launcher.isUnix() ? "cat" : "cmd.exe";
+            String command = "cat";


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.

jglick · 2023-09-18T11:21:10Z

src/main/java/org/jenkinsci/plugins/docker/workflow/client/DockerClient.java

+    }
+    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);


Unclear what this sleep is for.

ThugPigeon653 · 2023-09-18T23:24:03Z

Hi @jglick and @MarkEWaite ,
Thanks for taking the time to check this out and respond. I have taken on board your suggestions, and do agree completely with what you have said.
I will be sure to familiarise myself more deeply with the plugin, and will keep in mind what Jesse has said about correct use of Jira, as well as links to earlier attempts. I apologise if I got ahead of myself a bit with this PR.

Thanks,
Lachie

MarkEWaite · 2023-09-18T23:25:20Z

Thanks @ThugPigeon653 for your willingness to adopt the plugin and to be involved in Jenkins. Much appreciated!

jglick · 2023-09-19T10:59:19Z

Thanks!

ThugPigeon653 added 2 commits September 17, 2023 12:58

Fixed Absolute path, and container not stopping issues

9971adc

Removed un-used variable assignment

b2a6c3c

MarkEWaite reviewed Sep 18, 2023

View reviewed changes

jglick reviewed Sep 18, 2023

View reviewed changes

This was referenced Sep 18, 2023

Fix: Distroless containers do not have a PATH - sponsored by SDA SE #237

Open

Update plugin-docker-workflow.yml jenkins-infra/repository-permissions-updater#3531

Closed

ThugPigeon653 closed this Sep 18, 2023

ThugPigeon653 deleted the JENKINS-60473 branch September 18, 2023 23:24

Comments

Conversation

ThugPigeon653 commented Sep 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing done

Uh oh!

ThugPigeon653 commented Sep 17, 2023

Uh oh!

MarkEWaite left a comment

Choose a reason for hiding this comment

Uh oh!

MarkEWaite Sep 18, 2023

Choose a reason for hiding this comment

Uh oh!

jglick Sep 18, 2023

Choose a reason for hiding this comment

Uh oh!

jglick Sep 19, 2023

Choose a reason for hiding this comment

Uh oh!

MarkEWaite Sep 18, 2023

Choose a reason for hiding this comment

Uh oh!

jglick commented Sep 18, 2023

Uh oh!

jglick commented Sep 18, 2023

Uh oh!

jglick left a comment

Choose a reason for hiding this comment

Uh oh!

jglick Sep 18, 2023

Choose a reason for hiding this comment

Uh oh!

jglick Sep 18, 2023

Choose a reason for hiding this comment

Uh oh!

ThugPigeon653 commented Sep 18, 2023

Uh oh!

MarkEWaite commented Sep 18, 2023

Uh oh!

jglick commented Sep 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ThugPigeon653 commented Sep 17, 2023 •

edited

Loading