github-actions[bot] commented on code in PR #41167:
URL: https://github.com/apache/beam/pull/41167#discussion_r4526908518
##########
runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/environment/DockerCommand.java:
##########
@@ -88,28 +88,53 @@ private DockerCommand(String dockerExecutable, Duration
commandTimeout, Duration
public String runImage(String imageTag, List<String> dockerOpts,
List<String> args)
throws IOException, TimeoutException, InterruptedException {
checkArgument(!imageTag.isEmpty(), "Docker image tag required");
- // Pull the image from docker repo. This will be no-op if the image
already exists.
- try {
- runShortCommand(
-
ImmutableList.<String>builder().add(dockerExecutable).add("pull").add(imageTag).build(),
- pullTimeout);
- } catch (IOException | TimeoutException | InterruptedException e) {
- if (LOG.isDebugEnabled()) {
- LOG.debug("Unable to pull docker image {}", imageTag, e);
- } else {
- LOG.warn("Unable to pull docker image {}", imageTag, e);
+ if (!isImagePresentLocally(imageTag)) {
+ // Pull the image from docker repo if it does not already exist locally.
+ try {
+ runShortCommand(
+
ImmutableList.<String>builder().add(dockerExecutable).add("pull").add(imageTag).build(),
+ pullTimeout);
+ } catch (IOException | TimeoutException | InterruptedException e) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Unable to pull docker image {}", imageTag, e);
+ } else {
+ LOG.warn("Unable to pull docker image {}", imageTag, e);
+ }
}
}
- // TODO: Validate args?
- return runShortCommand(
+ List<String> runCommand =
ImmutableList.<String>builder()
.add(dockerExecutable)
.add("run")
.add("-d")
.addAll(dockerOpts)
.add(imageTag)
.addAll(args)
- .build());
+ .build();
+ try {
+ return runShortCommand(runCommand);
+ } catch (TimeoutException e) {
+ LOG.warn("Timed out starting docker container for image {}, retrying
once", imageTag, e);
+ return runShortCommand(runCommand);
+ }
+ }
+
+ private boolean isImagePresentLocally(String imageTag) {
+ try {
+ runShortCommand(
+ ImmutableList.<String>builder()
+ .add(dockerExecutable)
+ .add("image")
+ .add("inspect")
+ .add(imageTag)
+ .build());
+ return true;
Review Comment:
**🟠High**: When `isImagePresentLocally` catches `InterruptedException`,
restores the interrupt flag, and returns `false`, `runImage` enters the `if
(!isImagePresentLocally(imageTag))` branch and spawns `docker pull`. Inside
`runShortCommand`, `process.waitFor(...)` immediately consumes the thread's
interrupt status and throws `InterruptedException` (leaving the spawned `docker
pull` process running), which is then caught and swallowed on line 96 without
restoring the interrupt flag before proceeding to `docker run`.
Since `runImage` already declares `throws InterruptedException`,
`isImagePresentLocally` should declare `throws InterruptedException` and let it
propagate directly.
##########
runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/environment/DockerCommand.java:
##########
@@ -88,28 +88,53 @@ private DockerCommand(String dockerExecutable, Duration
commandTimeout, Duration
public String runImage(String imageTag, List<String> dockerOpts,
List<String> args)
throws IOException, TimeoutException, InterruptedException {
checkArgument(!imageTag.isEmpty(), "Docker image tag required");
- // Pull the image from docker repo. This will be no-op if the image
already exists.
- try {
- runShortCommand(
-
ImmutableList.<String>builder().add(dockerExecutable).add("pull").add(imageTag).build(),
- pullTimeout);
- } catch (IOException | TimeoutException | InterruptedException e) {
- if (LOG.isDebugEnabled()) {
- LOG.debug("Unable to pull docker image {}", imageTag, e);
- } else {
- LOG.warn("Unable to pull docker image {}", imageTag, e);
+ if (!isImagePresentLocally(imageTag)) {
+ // Pull the image from docker repo if it does not already exist locally.
+ try {
+ runShortCommand(
+
ImmutableList.<String>builder().add(dockerExecutable).add("pull").add(imageTag).build(),
+ pullTimeout);
+ } catch (IOException | TimeoutException | InterruptedException e) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Unable to pull docker image {}", imageTag, e);
+ } else {
+ LOG.warn("Unable to pull docker image {}", imageTag, e);
+ }
}
}
- // TODO: Validate args?
- return runShortCommand(
+ List<String> runCommand =
ImmutableList.<String>builder()
.add(dockerExecutable)
.add("run")
.add("-d")
.addAll(dockerOpts)
.add(imageTag)
.addAll(args)
- .build());
+ .build();
+ try {
+ return runShortCommand(runCommand);
+ } catch (TimeoutException e) {
+ LOG.warn("Timed out starting docker container for image {}, retrying
once", imageTag, e);
+ return runShortCommand(runCommand);
+ }
+ }
+
+ private boolean isImagePresentLocally(String imageTag) {
+ try {
+ runShortCommand(
+ ImmutableList.<String>builder()
+ .add(dockerExecutable)
+ .add("image")
Review Comment:
**🟢 Low**: `runShortCommand` reads all of stdout into a `String`. Running
`docker image inspect <imageTag>` without `--format` causes the Docker daemon
to serialize and output the full image manifest/config JSON on every container
launch only for the output to be discarded. Passing `-f` / `--format` (similar
to `isContainerRunning` below) keeps stdout minimal.
```suggestion
.add("inspect")
.add("-f")
.add("{{.Id}}")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
