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

Container runtime detection cached in sys prop, container-docker extension #31857

Merged
merged 5 commits into from
Mar 17, 2023
Merged

Container runtime detection cached in sys prop, container-docker extension #31857

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4c449e0
Swaps container runtime caching from file to sys prop
Karm Mar 15, 2023
f8c61cf
Remove superflous calls to podman/docker on podman only/docker only box
Karm Mar 15, 2023
781eb53
Similar to AppCDSBuildStep, IntegrationTestUtil needs to handle a sit…
Karm Mar 15, 2023
ea1728b
Container Image Docker: uses Core autodetection too
Karm Mar 15, 2023
38d3c0d
Various adjustments for the container detection
gsmet Mar 16, 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
11 changes: 4 additions & 7 deletions core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/AppCDSBuildStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
import io.quarkus.deployment.pkg.builditem.OutputTargetBuildItem;
import io.quarkus.deployment.steps.MainClassBuildStep;
import io.quarkus.runtime.LaunchMode;
import io.quarkus.runtime.util.ContainerRuntimeUtil;
import io.quarkus.runtime.util.ContainerRuntimeUtil.ContainerRuntime;
import io.quarkus.utilities.JavaBinFinder;

public class AppCDSBuildStep {
Expand All @@ -39,7 +39,6 @@ public class AppCDSBuildStep {
public static final String CLASSES_LIST_FILE_NAME = "classes.lst";
private static final String CONTAINER_IMAGE_BASE_BUILD_DIR = "/tmp/quarkus";
private static final String CONTAINER_IMAGE_APPCDS_DIR = CONTAINER_IMAGE_BASE_BUILD_DIR + "/appcds";
public static final ContainerRuntimeUtil.ContainerRuntime CONTAINER_RUNTIME = detectContainerRuntime(false);
Copy link
Member

Choose a reason for hiding this comment

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

Let's avoid doing the autodetection in static fields.


@BuildStep(onlyIf = AppCDSRequired.class)
public void requested(OutputTargetBuildItem outputTarget, BuildProducer<AppCDSRequestedBuildItem> producer)
Expand Down Expand Up @@ -204,12 +203,10 @@ private Path createClassesList(JarBuildItem jarResult,
// generate the classes file on the host
private List<String> dockerRunCommands(OutputTargetBuildItem outputTarget, String containerImage,
String containerWorkingDir) {
if (CONTAINER_RUNTIME == ContainerRuntimeUtil.ContainerRuntime.UNAVAILABLE) {
throw new IllegalStateException("No container runtime was found. "
+ "Make sure you have either Docker or Podman installed in your environment.");
}
ContainerRuntime containerRuntime = detectContainerRuntime(true);
Copy link
Member

Choose a reason for hiding this comment

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

The exception will be thrown automatically if required is true.


List<String> command = new ArrayList<>(10);
command.add(CONTAINER_RUNTIME.getExecutableName());
command.add(containerRuntime.getExecutableName());
command.add("run");
command.add("-v");
command.add(outputTarget.getOutputDirectory().toAbsolutePath().toString() + ":" + CONTAINER_IMAGE_BASE_BUILD_DIR
Expand Down
13 changes: 5 additions & 8 deletions ...nt/src/test/java/io/quarkus/deployment/pkg/steps/NativeImageBuildContainerRunnerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package io.quarkus.deployment.pkg.steps;

import static io.quarkus.deployment.pkg.steps.AppCDSBuildStep.CONTAINER_RUNTIME;
Copy link
Member

Choose a reason for hiding this comment

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

Let's not involved something that has nothing to do with this test.

import static org.assertj.core.api.Assertions.assertThat;

import java.nio.file.Path;
Expand All @@ -19,10 +18,8 @@ class NativeImageBuildContainerRunnerTest {
@DisabledIfSystemProperty(named = "avoid-containers", matches = "true")
@Test
void testBuilderImageBeingPickedUp() {
if (CONTAINER_RUNTIME == ContainerRuntimeUtil.ContainerRuntime.UNAVAILABLE) {
throw new IllegalStateException("No container runtime was found. "
+ "Make sure you have either Docker or Podman installed in your environment.");
}
ContainerRuntimeUtil.ContainerRuntime containerRuntime = ContainerRuntimeUtil.detectContainerRuntime(true);

NativeConfig nativeConfig = new NativeConfig();
nativeConfig.containerRuntime = Optional.empty();
boolean found;
Expand All @@ -31,7 +28,7 @@ void testBuilderImageBeingPickedUp() {

nativeConfig.builderImage = "graalvm";
localRunner = new NativeImageBuildLocalContainerRunner(nativeConfig, Path.of("/tmp"));
command = localRunner.buildCommand(CONTAINER_RUNTIME.getExecutableName(), Collections.emptyList(),
command = localRunner.buildCommand(containerRuntime.getExecutableName(), Collections.emptyList(),
Collections.emptyList());
found = false;
for (String part : command) {
Expand All @@ -44,7 +41,7 @@ void testBuilderImageBeingPickedUp() {

nativeConfig.builderImage = "mandrel";
localRunner = new NativeImageBuildLocalContainerRunner(nativeConfig, Path.of("/tmp"));
command = localRunner.buildCommand(CONTAINER_RUNTIME.getExecutableName(), Collections.emptyList(),
command = localRunner.buildCommand(containerRuntime.getExecutableName(), Collections.emptyList(),
Collections.emptyList());
found = false;
for (String part : command) {
Expand All @@ -57,7 +54,7 @@ void testBuilderImageBeingPickedUp() {

nativeConfig.builderImage = "RandomString";
localRunner = new NativeImageBuildLocalContainerRunner(nativeConfig, Path.of("/tmp"));
command = localRunner.buildCommand(CONTAINER_RUNTIME.getExecutableName(), Collections.emptyList(),
command = localRunner.buildCommand(containerRuntime.getExecutableName(), Collections.emptyList(),
Collections.emptyList());
found = false;
for (String part : command) {
Expand Down
54 changes: 35 additions & 19 deletions core/runtime/src/main/java/io/quarkus/runtime/util/ContainerRuntimeUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public static ContainerRuntime detectContainerRuntime() {
}

public static ContainerRuntime detectContainerRuntime(boolean required) {
final ContainerRuntime containerRuntime = loadConfig();
final ContainerRuntime containerRuntime = loadContainerRuntimeFromSystemProperty();
Copy link
Member

Choose a reason for hiding this comment

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

just a simple name change for clarity.

if (containerRuntime != null) {
return containerRuntime;
} else {
Expand All @@ -57,15 +57,15 @@ public static ContainerRuntime detectContainerRuntime(boolean required) {
dockerVersionOutput = getVersionOutputFor(ContainerRuntime.DOCKER);
dockerAvailable = dockerVersionOutput.contains("Docker version");
if (dockerAvailable) {
storeConfig(ContainerRuntime.DOCKER);
storeContainerRuntimeInSystemProperty(ContainerRuntime.DOCKER);
return ContainerRuntime.DOCKER;
}
}
if (CONTAINER_EXECUTABLE.trim().equalsIgnoreCase("podman")) {
podmanVersionOutput = getVersionOutputFor(ContainerRuntime.PODMAN);
podmanAvailable = podmanVersionOutput.startsWith("podman version");
if (podmanAvailable) {
storeConfig(ContainerRuntime.PODMAN);
storeContainerRuntimeInSystemProperty(ContainerRuntime.PODMAN);
return ContainerRuntime.PODMAN;
}
}
Expand All @@ -77,46 +77,48 @@ public static ContainerRuntime detectContainerRuntime(boolean required) {
if (dockerAvailable) {
// Check if "docker" is an alias to "podman"
if (dockerVersionOutput.startsWith("podman version")) {
storeConfig(ContainerRuntime.PODMAN);
storeContainerRuntimeInSystemProperty(ContainerRuntime.PODMAN);
return ContainerRuntime.PODMAN;
}
storeConfig(ContainerRuntime.DOCKER);
storeContainerRuntimeInSystemProperty(ContainerRuntime.DOCKER);
return ContainerRuntime.DOCKER;
}
podmanVersionOutput = getVersionOutputFor(ContainerRuntime.PODMAN);
podmanAvailable = podmanVersionOutput.startsWith("podman version");
if (podmanAvailable) {
storeConfig(ContainerRuntime.PODMAN);
storeContainerRuntimeInSystemProperty(ContainerRuntime.PODMAN);
return ContainerRuntime.PODMAN;
}

storeContainerRuntimeInSystemProperty(ContainerRuntime.UNAVAILABLE);

if (required) {
throw new IllegalStateException("No container runtime was found. "
+ "Make sure you have either Docker or Podman installed in your environment.");
} else {
storeConfig(ContainerRuntime.UNAVAILABLE);
return ContainerRuntime.UNAVAILABLE;
}

return ContainerRuntime.UNAVAILABLE;
}
}

private static ContainerRuntime loadConfig() {
private static ContainerRuntime loadContainerRuntimeFromSystemProperty() {
final String runtime = System.getProperty(CONTAINER_RUNTIME_SYS_PROP);

if (runtime == null) {
return null;
} else if (ContainerRuntime.DOCKER.name().equalsIgnoreCase(runtime)) {
return ContainerRuntime.DOCKER;
} else if (ContainerRuntime.PODMAN.name().equalsIgnoreCase(runtime)) {
return ContainerRuntime.PODMAN;
} else if (ContainerRuntime.UNAVAILABLE.name().equalsIgnoreCase(runtime)) {
return ContainerRuntime.UNAVAILABLE;
} else {
}
Copy link
Member

Choose a reason for hiding this comment

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

Moved that logic to the enum and simplified things a bit. Would appreciate a second pair of eyes to make sure I didn't break something.


ContainerRuntime containerRuntime = ContainerRuntime.valueOf(runtime);

if (containerRuntime == null) {
log.warnf("System property %s contains an unknown value %s. Ignoring it.",
CONTAINER_RUNTIME_SYS_PROP, runtime);
return null;
}

return containerRuntime;
}

private static void storeConfig(ContainerRuntime containerRuntime) {
private static void storeContainerRuntimeInSystemProperty(ContainerRuntime containerRuntime) {
System.setProperty(CONTAINER_RUNTIME_SYS_PROP, containerRuntime.name());
}

Expand Down Expand Up @@ -197,6 +199,10 @@ public enum ContainerRuntime {
private Boolean rootless;

public String getExecutableName() {
if (this == UNAVAILABLE) {
throw new IllegalStateException("Cannot get an executable name when no container runtime is available");
}

return this.name().toLowerCase();
}

Expand All @@ -213,5 +219,15 @@ public boolean isRootless() {
}
return rootless;
}

public static ContainerRuntime of(String value) {
for (ContainerRuntime containerRuntime : values()) {
if (containerRuntime.name().equalsIgnoreCase(value)) {
return containerRuntime;
}
}

return null;
}
}
}
14 changes: 8 additions & 6 deletions ...eployment/src/main/java/io/quarkus/container/image/docker/deployment/DockerProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ private String createContainerImage(ContainerImageConfig containerImageConfig, D
boolean buildSuccessful = ExecUtil.exec(out.getOutputDirectory().toFile(), reader, executableName,
dockerArgs);
if (!buildSuccessful) {
throw dockerException(dockerArgs);
throw dockerException(executableName, dockerArgs);
}

dockerConfig.buildx.platform
Expand Down Expand Up @@ -250,7 +250,8 @@ private void loginToRegistryIfNeeded(ContainerImageConfig containerImageConfig,
containerImageConfig.username.get(),
"-p" + containerImageConfig.password.get());
if (!loginSuccessful) {
throw dockerException(new String[] { "-u", containerImageConfig.username.get(), "-p", "********" });
throw dockerException(executableName,
new String[] { "-u", containerImageConfig.username.get(), "-p", "********" });
}
}
}
Expand Down Expand Up @@ -326,7 +327,7 @@ private void createAdditionalTags(String image, List<String> additionalImageTags
String[] tagArgs = { "tag", image, additionalTag };
boolean tagSuccessful = ExecUtil.exec(executableName, tagArgs);
if (!tagSuccessful) {
throw dockerException(tagArgs);
throw dockerException(executableName, tagArgs);
}
}
}
Expand All @@ -336,14 +337,15 @@ private void pushImage(String image, DockerConfig dockerConfig) {
String[] pushArgs = { "push", image };
boolean pushSuccessful = ExecUtil.exec(executableName, pushArgs);
if (!pushSuccessful) {
throw dockerException(pushArgs);
throw dockerException(executableName, pushArgs);
}
log.info("Successfully pushed docker image " + image);
}

private RuntimeException dockerException(String[] dockerArgs) {
private RuntimeException dockerException(String executableName, String[] dockerArgs) {
return new RuntimeException(
"Execution of 'docker " + String.join(" ", dockerArgs) + "' failed. See docker output for more details");
"Execution of '" + executableName + " " + String.join(" ", dockerArgs)
Copy link
Member

Choose a reason for hiding this comment

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

Given we are not always using docker, let's make the message accurate.

+ "' failed. See docker output for more details");
}

private DockerfilePaths getDockerfilePaths(DockerConfig dockerConfig, boolean forNative,
Expand Down
11 changes: 4 additions & 7 deletions test-framework/junit5/src/main/java/io/quarkus/test/junit/IntegrationTestUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ public final class IntegrationTestUtil {
public static final int DEFAULT_PORT = 8081;
public static final int DEFAULT_HTTPS_PORT = 8444;
public static final long DEFAULT_WAIT_TIME_SECONDS = 60;
public static final ContainerRuntimeUtil.ContainerRuntime CONTAINER_RUNTIME = detectContainerRuntime(false);
Copy link
Member

Choose a reason for hiding this comment

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

Let's avoid doing that statically in a widely used util.


private IntegrationTestUtil() {
}
Expand Down Expand Up @@ -337,13 +336,11 @@ public void accept(String s, String s2) {
private static void createNetworkIfNecessary(
final ArtifactLauncher.InitContext.DevServicesLaunchResult devServicesLaunchResult) {
if (devServicesLaunchResult.manageNetwork() && (devServicesLaunchResult.networkId() != null)) {
if (CONTAINER_RUNTIME == ContainerRuntimeUtil.ContainerRuntime.UNAVAILABLE) {
throw new IllegalStateException("No container runtime was found. "
+ "Make sure you have either Docker or Podman installed in your environment.");
}
ContainerRuntimeUtil.ContainerRuntime containerRuntime = detectContainerRuntime(true);

try {
int networkCreateResult = new ProcessBuilder().redirectError(DISCARD).redirectOutput(DISCARD)
.command(CONTAINER_RUNTIME.getExecutableName(), "network", "create",
.command(containerRuntime.getExecutableName(), "network", "create",
devServicesLaunchResult.networkId())
.start().waitFor();
if (networkCreateResult > 0) {
Expand All @@ -356,7 +353,7 @@ private static void createNetworkIfNecessary(
public void run() {
try {
new ProcessBuilder().redirectError(DISCARD).redirectOutput(DISCARD)
.command(CONTAINER_RUNTIME.getExecutableName(), "network", "rm",
.command(containerRuntime.getExecutableName(), "network", "rm",
devServicesLaunchResult.networkId())
.start()
.waitFor();
Expand Down