[JENKINS-37769] Failing to stop containers cleanly, so at least stopping them quickly #65
Conversation
… stopping them quickly.
| @@ -62,7 +62,7 @@ public void test_run() throws IOException, InterruptedException { | |||
| EnvVars launchEnv = newLaunchEnv(); | |||
| String containerId = | |||
| dockerClient.run(launchEnv, "learn/tutorial", null, null, Collections.<String, String>emptyMap(), Collections.<String>emptyList(), new EnvVars(), | |||
| dockerClient.whoAmI(), "true"); | |||
| dockerClient.whoAmI(), "cat"); | |||
There was a problem hiding this comment.
This test was bogus as true was returning immediately, so the container was not even running.
There was a problem hiding this comment.
Would also depend on open stdin AFAIR
There was a problem hiding this comment.
sleep infinity might be a better choice (discussion).
|
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
| @@ -136,7 +136,7 @@ public String run(@Nonnull EnvVars launchEnv, @Nonnull String image, @CheckForNu | |||
| * @param containerId The container ID. | |||
| */ | |||
| public void stop(@Nonnull EnvVars launchEnv, @Nonnull String containerId) throws IOException, InterruptedException { | |||
| LaunchResult result = launch(launchEnv, false, "stop", containerId); | |||
| LaunchResult result = launch(launchEnv, false, "stop", "--time=1", containerId); | |||
There was a problem hiding this comment.
I would recommend using kill this will stop the container immediately instead of waiting a second and killing it then.
There was a problem hiding this comment.
So would --time=0. That is what I do not want to do. The intent is to allow the container to shut down gracefully.
The call to rm below ensures that it is stopped and removed, whatever else happens.
There was a problem hiding this comment.
But the current implementation will always run into the timeout (cat won't terminate). So as long as the containers are not able to stop gracefully it's one second which could be saved.
There was a problem hiding this comment.
Yes…for now, until I can dig into the root problem, I would rather waste the second in the interest of offering a clean shutdown.
|
🐝 AIUI |
|
@reviewbybees done |
|
This pull request has completed our internal processes and we now respectfully request the maintainers of this repository to consider our proposal contained within this pull request for merging. |
|
10 seconds is too short for real scenario... Keep hitting this. |
|
We noticed that this times out commands executed in the container, unless they return in less than a second, which makes this plugin pretty much useless in the current state: |
JENKINS-37769
@reviewbybees