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.

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

Enable Podman and Docker Windows quarkus-container-image-docker testing #31490

Merged
merged 6 commits into from
Mar 14, 2023

Conversation

Karm
Copy link
Member

@Karm Karm commented Feb 28, 2023

This PR addresses problems I experienced trying to run quarkus-container-image-docker driven tests on Windows with Podman for Windows and with Docker Desktop for Windows.

I used the changes to Quarkus 2.16 branch to integrate Quarkus PrimeFaces test run that uses a builder image to compile a Linux native executable and then builds a runtime image and runs it for tests. It enables people who work with Windows to test the native workflow despite the fact we do not support Windows MSVC toolchain with e.g. AWT dependency.

Notes

  • quarkus.docker.executable-name and quarkus.native.container-runtime, the former actually comes from
    the quarkus-container-image-docker and the latter from the core. It is used instead of the hardcoded "docker"

  • Caused by: java.nio.file.FileSystemException: target\quarkus.log: The process cannot access the file because it is being used by another process
    is now wrapped in a try block with a logged warning.

  • Logs from the container: The former logic did not work for me on Windows, the log was left in the container
    and there was nothing to pipe to a file. Was it formerly used with mounting the volume? I added -i so as the container
    pours the log to stdout and capture that. WDYT @geoand 🤔?

  • Added quarkus-container-image-docker based test to the AWT extension test to try these changes.

Testing

I used the AWT integration test like this to test Podman and Docker on Linux and Windows:

mvnw clean verify -f integration-tests/pom.xml -pl awt -Ddocker -Dnative
  -Dnative.surefire.skip
  -Dquarkus.native.container-build=true
  -Dquarkus.container-image.build=true
  -Dquarkus.native.builder-image=quay.io/quarkus/ubi-quarkus-mandrel-builder-image:22.3-java17
  -Dquarkus.native.container-runtime=podman
  -Dquarkus.docker.executable-name=podman

(with docker in docker variants)

Logs for Quarkus main:

System Podman Docker
CentOS 8 4.0.2 ✔️ logs 20.10.21 ✔️ logs
Windows 10 4.1.0 ✔️ logs 20.10.22 ✔️ logs

@Karm Karm requested review from zakkak and geoand February 28, 2023 23:32
@Karm Karm self-assigned this Feb 28, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Feb 28, 2023

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

  • title should preferably start with an uppercase character (if it makes sense!)

This message is automatically generated by a bot.

@quarkus-bot
Copy link

quarkus-bot bot commented Feb 28, 2023

/cc @galderz (awt), @zakkak (awt)

@quarkus-bot

This comment has been minimized.

@Karm
Copy link
Member Author

Karm commented Mar 1, 2023

I am on the results, gonna refactor it.

@geoand geoand changed the title [main] Enables Podman and Docker Windows quarkus-container-image-docker testing Enable Podman and Docker Windows quarkus-container-image-docker testing Mar 1, 2023
@geoand
Copy link
Contributor

geoand commented Mar 1, 2023

Logs from the container: The former logic did not work for me on Windows, the log was left in the container
and there was nothing to pipe to a file. Was it formerly used with mounting the volume? I added -i so as the container
pours the log to stdout and capture that. WDYT @geoand thinking?

I am actually very sceptical of this. I can't remember why I had not ended up doing this, but the only way I will accept this change is if you can guarantee me that it's been thoroughly tested

@geoand
Copy link
Contributor

geoand commented Mar 1, 2023

The rest of the changes seem perfectly reasonable

Files.deleteIfExists(logFile);
Files.createDirectories(logFile.getParent());
} catch (FileSystemException e) {
log.warnf("Log file %s deletion failed, could happen on Windows, we can carry on.", logFile);
Copy link
Contributor

Choose a reason for hiding this comment

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

The message can probably be limited to Windows

Copy link
Member Author

Choose a reason for hiding this comment

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

Will amend.

Copy link
Member

Choose a reason for hiding this comment

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

so on non windows we make it hard fail_

@@ -194,7 +194,7 @@ public Result get() {
try {
OutputFilter filter = new OutputFilter();
if (ExecUtil.execWithTimeout(new File("."), filter, Duration.ofMillis(DOCKER_CMD_CHECK_TIMEOUT),
"docker", "version", "--format", "'{{.Server.Version}}'")) {
binary, "version", "--format", "'{{.Server.Version}}'")) {
LOGGER.debugf("Docker daemon found. Version: %s", filter.getOutput());
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is no longer docker specific, it would probably be best to rename the class and replace all hardcoded references to docker with "container runtime" or similar.

Copy link
Member Author

Choose a reason for hiding this comment

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

@geoand @zakkak I can feel the same itch about the abundance of word "Docker" in properties, class names etc.
I just don't know what would be a better alternative for any wide renaming. Should I try "ContainerRuntime" ? Like IsContainerRuntimeWorking.java, ContainerRuntimeContainerArtifactLauncher... 😃
Not sure. I am open to suggestions...

Leaving it there is IMHO also a perfectly valid option. There are people who just alias docker=podman and use that anyway to keep all tools and scripts happy.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it's probably not something for this PR

@@ -175,7 +175,7 @@ private static class DockerBinaryStrategy implements Strategy {

private DockerBinaryStrategy(boolean silent) {
this.silent = silent;
this.binary = ConfigProvider.getConfig().getOptionalValue("quarkus.docker.executable-name", String.class)
this.binary = ConfigProvider.getConfig().getOptionalValue("quarkus.native.container-runtime", String.class)
Copy link
Contributor

@zakkak zakkak Mar 1, 2023

Choose a reason for hiding this comment

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

Hmmm, this is a bit tricky I think.

quarkus.native.container-runtime is supposed to be native-image specific. As a result users are not expected to have it set if they don't plan to build a native image.

Furthermore, when setting it a container will be used for native builds even if container-build is not set (so, we can't just ask people to set it even for non-native builds):

public boolean isContainerBuild() {
return containerBuild.orElse(containerRuntime.isPresent() || remoteContainerBuild);
}

Maybe we should come up with a better strategy in regards of figuring out the container runtime. Is there any reason to use a different container runtime for tests, for running containerized apps, and for building the native executable? I think not. So maybe we should decouple the container runtime from the native configuration. @geoand @gsmet WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, there is no good reason to have a different strategy. It might be a good time to have something common

Copy link
Member Author

Choose a reason for hiding this comment

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

Note: The thing with docker.executable-name is that it's an option introduced in an extension, not in the core, so you might get somewhat confusing:

[WARNING] [io.quarkus.config] Unrecognized configuration key "quarkus.docker.executable-name" 
was provided; it will be ignored; verify that the dependency extension for this configuration is set or
that you did not make a typo

I think that the whole "determine container runtime" should happen in the core (podman, docker, alias to one another) and both Test Framework and eventual extensions such as container-image-docker should just consume it.

The unlikely event of having to use one command to use builder image and another command to run containers could still be fine tuned by properties.

Not sure if this PR is the place where this whole thing gets refactored. I'll try.
I am less certain about the 2.16 branch, where going wild on refactoring all that matches word docker might be less desirable.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on keeping this PR as minimal as possible so that it can be backported to 2.16 and using a different one for the refactorings.

Comment on lines 41 to 42
public static final String DOCKER_EXECUTABLE = ConfigProvider.getConfig()
.getOptionalValue("quarkus.native.container-runtime", String.class).orElse("docker");
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably better to detect the runtime instead of defaulting to docker, like we do with native image building:

containerRuntime = nativeConfig.containerRuntime.orElseGet(ContainerRuntimeUtil::detectContainerRuntime);

I also think the variable should not mention docker.

Comment on lines 34 to 95

final String executable = ConfigProvider.getConfig()
.getOptionalValue("quarkus.native.container-runtime", String.class).orElse(null);
if (executable != null) {
if (executable.trim().equalsIgnoreCase("docker") && dockerAvailable) {
return ContainerRuntime.DOCKER;
} else if (executable.trim().equalsIgnoreCase("podman") && podmanAvailable) {
return ContainerRuntime.PODMAN;
} else {
log.warn("quarkus.native.container-runtime config property must be set to either podman or docker " +
"and the executable must be available. Ignoring it.");
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be revisited after reaching a conclusion for #31490 (comment)

@Karm
Copy link
Member Author

Karm commented Mar 1, 2023

Ad CI failures:

ResourceTest#testHelloEndpoint() java.util.ServiceConfigurationError:
    io.smallrye.config.SmallRyeConfigFactory:
    io.quarkus.runtime.configuration.QuarkusConfigFactory not a subtype

and

io.quarkus.vertx.http.testrunner.UnitET#unitStyleTest(UnitET.java:16) 
    UnitET#unitStyleTest() expected: <Hi> but was: <hello>
io.quarkus.vertx.http.testrunner.UnitET#unitStyleTest2(UnitET.java:10) 
    UnitET#unitStyleTest2() expected: <UNIT> but was: <unit>

These are hardly related.


Ad PostgresOpenTelemetryJdbcInstrumentationIT, that feels like it could be very well related, i.e. starting DB containers etc.
It passes on my machine though: https://karms.biz/pastebin/opentelemetry-jdbc-instrumentation-runs-fine.txt

[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 13.961 s - in 
io.quarkus.it.opentelemetry.PostgresOpenTelemetryJdbcInstrumentationIT

@Karm
Copy link
Member Author

Karm commented Mar 1, 2023

Logs from the container: The former logic did not work for me on Windows, the log was left in the container
and there was nothing to pipe to a file. Was it formerly used with mounting the volume? I added -i so as the container
pours the log to stdout and capture that. WDYT @geoand thinking?

I am actually very sceptical of this. I can't remember why I had not ended up doing this, but the only way I will accept this change is if you can guarantee me that it's been thoroughly tested

I am looking into other projects in Quarkiverse besides PrimeFaces, looking for usages of quarkus-container-image-docker.
I will gradually report what other projects are / are not fine with this change.

@Karm
Copy link
Member Author

Karm commented Mar 1, 2023

It occurred to me why it could make sense keeping the log separate, container.log vs. quarkus.log. When the container executed is a DB and not a Quarkus test application, it is weird to have the output mushed into a one file. Will try to recognize the scenario in the launcher.

@@ -37,6 +38,8 @@ 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 String DOCKER_EXECUTABLE = ConfigProvider.getConfig()
Copy link
Member Author

Choose a reason for hiding this comment

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

@geoand @zakkak So, going to ConfigProvider here is what breaks integraton-tests\maven 😞, i.e.

2023-03-06 17:34:50,615 ERROR [io.qua.test] (Test runner thread) Test ResourceTest#testHelloEndpoint() failed 
: java.lang.RuntimeException: java.util.ServiceConfigurationError: io.smallrye.config.SmallRyeConfigFactory: io.quarkus.runtime.configuration.QuarkusConfigFactory not a subtype
        at io.quarkus.test.junit.QuarkusTestExtension.throwBootFailureException(QuarkusTestExtension.java:625)
        at io.quarkus.test.junit.QuarkusTestExtension.interceptTestClassConstructor(QuarkusTestExtension.java:696)
        at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.lambda$invoke$0(InterceptingExecutableInvoker.java:93)
        at org.junit.jupiter.engine.execution.InvocationInterceptorChain$InterceptedInvocation.proceed(InvocationInterceptorChain.java:106)
        at org.junit.jupiter.api.extension.InvocationInterceptor.interceptTestClassConstructor(InvocationInterceptor.java:73)
        at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.lambda$invoke$0(InterceptingExecutableInvoker.java:93)
        at org.junit.jupiter.engine.execution.InvocationInterceptorChain$InterceptedInvocation.proceed(InvocationInterceptorChain.java:106)
        at org.junit.jupiter.engine.execution.InvocationInterceptorChain.proceed(InvocationInterceptorChain.java:64)
        at org.junit.jupiter.engine.execution.InvocationInterceptorChain.chainAndInvoke(InvocationInterceptorChain.java:45)
        at org.junit.jupiter.engine.execution.InvocationInterceptorChain.invoke(InvocationInterceptorChain.java:37)
        at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.invoke(InterceptingExecutableInvoker.java:92)
        at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.invoke(InterceptingExecutableInvoker.java:62)
        at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.invokeTestClassConstructor(ClassBasedTestDescriptor.java:363)
        at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.instantiateTestClass(ClassBasedTestDescriptor.java:310)
        at org.junit.jupiter.engine.descriptor.ClassTestDescriptor.instantiateTestClass(ClassTestDescriptor.java:79)
        at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.instantiateAndPostProcessTestInstance(ClassBasedTestDescriptor.java:286)
        at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.lambda$testInstancesProvider$4(ClassBasedTestDescriptor.java:278)
        at java.base/java.util.Optional.orElseGet(Optional.java:364)
        at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.lambda$testInstancesProvider$5(ClassBasedTestDescriptor.java:277)
        at org.junit.jupiter.engine.execution.TestInstancesProvider.getTestInstances(TestInstancesProvider.java:31)
        at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$prepare$0(TestMethodTestDescriptor.java:105)
        at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
        at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.prepare(TestMethodTestDescriptor.java:104)
        at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.prepare(TestMethodTestDescriptor.java:68)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$prepare$2(NodeTestTask.java:123)
        at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.prepare(NodeTestTask.java:123)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:90)
        at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
        at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:155)
        at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
        at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
        at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
        at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
        at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:155)
        at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
        at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
        at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
        at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.submit(SameThreadHierarchicalTestExecutorService.java:35)
        at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:57)
        at org.junit.platform.engine.support.hierarchical.HierarchicalTestEngine.execute(HierarchicalTestEngine.java:54)
        at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:147)
        at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:127)
        at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:90)
        at org.junit.platform.launcher.core.EngineExecutionOrchestrator.lambda$execute$0(EngineExecutionOrchestrator.java:55)
        at org.junit.platform.launcher.core.EngineExecutionOrchestrator.withInterceptedStreams(EngineExecutionOrchestrator.java:102)
        at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:54)
        at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:114)
        at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:95)
        at org.junit.platform.launcher.core.DefaultLauncherSession$DelegatingLauncher.execute(DefaultLauncherSession.java:91)
        at org.junit.platform.launcher.core.SessionPerRequestLauncher.execute(SessionPerRequestLauncher.java:60)
        at io.quarkus.deployment.dev.testing.JunitTestRunner$2.run(JunitTestRunner.java:227)
        at io.quarkus.deployment.dev.testing.ModuleTestRunner$2.run(ModuleTestRunner.java:92)
        at io.quarkus.deployment.dev.testing.TestSupport.runInternal(TestSupport.java:390)
        at io.quarkus.deployment.dev.testing.TestSupport$2.run(TestSupport.java:310)
        at java.base/java.lang.Thread.run(Thread.java:833)
Caused by: java.util.ServiceConfigurationError: io.smallrye.config.SmallRyeConfigFactory: io.quarkus.runtime.configuration.QuarkusConfigFactory not a subtype
        at java.base/java.util.ServiceLoader.fail(ServiceLoader.java:593)
        at java.base/java.util.ServiceLoader$LazyClassPathLookupIterator.hasNextService(ServiceLoader.java:1244)
        at java.base/java.util.ServiceLoader$LazyClassPathLookupIterator.hasNext(ServiceLoader.java:1273)
        at java.base/java.util.ServiceLoader$2.hasNext(ServiceLoader.java:1309)
        at java.base/java.util.ServiceLoader$3.hasNext(ServiceLoader.java:1393)
        at io.smallrye.config.SmallRyeConfigProviderResolver.getFactoryFor(SmallRyeConfigProviderResolver.java:100)
        at io.smallrye.config.SmallRyeConfigProviderResolver.getConfig(SmallRyeConfigProviderResolver.java:76)
        at io.smallrye.config.SmallRyeConfigProviderResolver.getConfig(SmallRyeConfigProviderResolver.java:64)
        at org.eclipse.microprofile.config.ConfigProvider.getConfig(ConfigProvider.java:85)
        at io.quarkus.test.junit.IntegrationTestUtil.<clinit>(IntegrationTestUtil.java:68)
        at io.quarkus.test.junit.QuarkusTestExtension.doJavaStart(QuarkusTestExtension.java:231)
        at io.quarkus.test.junit.QuarkusTestExtension.ensureStarted(QuarkusTestExtension.java:592)
        at io.quarkus.test.junit.QuarkusTestExtension.beforeAll(QuarkusTestExtension.java:640)
        at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.lambda$invokeBeforeAllCallbacks$12(ClassBasedTestDescriptor.java:395)
        at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
        at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.invokeBeforeAllCallbacks(ClassBasedTestDescriptor.java:395)
        at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.before(ClassBasedTestDescriptor.java:211)
        at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.before(ClassBasedTestDescriptor.java:84)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:148)
        ... 35 more

config

It doesn't break in regular integration tests, e.g. in the newly added AWT in-container test here.

I've searched for

java.util.ServiceConfigurationError: 
io.smallrye.config.SmallRyeConfigFactory: 
io.quarkus.runtime.configuration.QuarkusConfigFactory 
not a subtype

as it rang a bell for me. I have seen it before. It looks like people hit it when doing something that affects classloading.

I am not sure what to do about it ATM. It is clearer why there was a hardcoded value in the first place though 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

@geoand @zakkak So, going to ConfigProvider here is what breaks integraton-tests\maven disappointed, i.e.

2023-03-06 17:34:50,615 ERROR [io.qua.test] (Test runner thread) Test ResourceTest#testHelloEndpoint() failed 
: java.lang.RuntimeException: java.util.ServiceConfigurationError: io.smallrye.config.SmallRyeConfigFactory: io.quarkus.runtime.configuration.QuarkusConfigFactory not a subtype
        at io.quarkus.test.junit.QuarkusTestExtension.throwBootFailureException(QuarkusTestExtension.java:625)
        at io.quarkus.test.junit.QuarkusTestExtension.interceptTestClassConstructor(QuarkusTestExtension.java:696)
        at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.lambda$invoke$0(InterceptingExecutableInvoker.java:93)
        at org.junit.jupiter.engine.execution.InvocationInterceptorChain$InterceptedInvocation.proceed(InvocationInterceptorChain.java:106)
        at org.junit.jupiter.api.extension.InvocationInterceptor.interceptTestClassConstructor(InvocationInterceptor.java:73)
        at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.lambda$invoke$0(InterceptingExecutableInvoker.java:93)
        at org.junit.jupiter.engine.execution.InvocationInterceptorChain$InterceptedInvocation.proceed(InvocationInterceptorChain.java:106)
        at org.junit.jupiter.engine.execution.InvocationInterceptorChain.proceed(InvocationInterceptorChain.java:64)
        at org.junit.jupiter.engine.execution.InvocationInterceptorChain.chainAndInvoke(InvocationInterceptorChain.java:45)
        at org.junit.jupiter.engine.execution.InvocationInterceptorChain.invoke(InvocationInterceptorChain.java:37)
        at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.invoke(InterceptingExecutableInvoker.java:92)
        at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.invoke(InterceptingExecutableInvoker.java:62)
        at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.invokeTestClassConstructor(ClassBasedTestDescriptor.java:363)
        at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.instantiateTestClass(ClassBasedTestDescriptor.java:310)
        at org.junit.jupiter.engine.descriptor.ClassTestDescriptor.instantiateTestClass(ClassTestDescriptor.java:79)
        at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.instantiateAndPostProcessTestInstance(ClassBasedTestDescriptor.java:286)
        at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.lambda$testInstancesProvider$4(ClassBasedTestDescriptor.java:278)
        at java.base/java.util.Optional.orElseGet(Optional.java:364)
        at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.lambda$testInstancesProvider$5(ClassBasedTestDescriptor.java:277)
        at org.junit.jupiter.engine.execution.TestInstancesProvider.getTestInstances(TestInstancesProvider.java:31)
        at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$prepare$0(TestMethodTestDescriptor.java:105)
        at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
        at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.prepare(TestMethodTestDescriptor.java:104)
        at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.prepare(TestMethodTestDescriptor.java:68)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$prepare$2(NodeTestTask.java:123)
        at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.prepare(NodeTestTask.java:123)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:90)
        at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
        at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:155)
        at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
        at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
        at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
        at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
        at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:155)
        at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
        at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
        at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
        at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.submit(SameThreadHierarchicalTestExecutorService.java:35)
        at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:57)
        at org.junit.platform.engine.support.hierarchical.HierarchicalTestEngine.execute(HierarchicalTestEngine.java:54)
        at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:147)
        at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:127)
        at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:90)
        at org.junit.platform.launcher.core.EngineExecutionOrchestrator.lambda$execute$0(EngineExecutionOrchestrator.java:55)
        at org.junit.platform.launcher.core.EngineExecutionOrchestrator.withInterceptedStreams(EngineExecutionOrchestrator.java:102)
        at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:54)
        at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:114)
        at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:95)
        at org.junit.platform.launcher.core.DefaultLauncherSession$DelegatingLauncher.execute(DefaultLauncherSession.java:91)
        at org.junit.platform.launcher.core.SessionPerRequestLauncher.execute(SessionPerRequestLauncher.java:60)
        at io.quarkus.deployment.dev.testing.JunitTestRunner$2.run(JunitTestRunner.java:227)
        at io.quarkus.deployment.dev.testing.ModuleTestRunner$2.run(ModuleTestRunner.java:92)
        at io.quarkus.deployment.dev.testing.TestSupport.runInternal(TestSupport.java:390)
        at io.quarkus.deployment.dev.testing.TestSupport$2.run(TestSupport.java:310)
        at java.base/java.lang.Thread.run(Thread.java:833)
Caused by: java.util.ServiceConfigurationError: io.smallrye.config.SmallRyeConfigFactory: io.quarkus.runtime.configuration.QuarkusConfigFactory not a subtype
        at java.base/java.util.ServiceLoader.fail(ServiceLoader.java:593)
        at java.base/java.util.ServiceLoader$LazyClassPathLookupIterator.hasNextService(ServiceLoader.java:1244)
        at java.base/java.util.ServiceLoader$LazyClassPathLookupIterator.hasNext(ServiceLoader.java:1273)
        at java.base/java.util.ServiceLoader$2.hasNext(ServiceLoader.java:1309)
        at java.base/java.util.ServiceLoader$3.hasNext(ServiceLoader.java:1393)
        at io.smallrye.config.SmallRyeConfigProviderResolver.getFactoryFor(SmallRyeConfigProviderResolver.java:100)
        at io.smallrye.config.SmallRyeConfigProviderResolver.getConfig(SmallRyeConfigProviderResolver.java:76)
        at io.smallrye.config.SmallRyeConfigProviderResolver.getConfig(SmallRyeConfigProviderResolver.java:64)
        at org.eclipse.microprofile.config.ConfigProvider.getConfig(ConfigProvider.java:85)
        at io.quarkus.test.junit.IntegrationTestUtil.<clinit>(IntegrationTestUtil.java:68)
        at io.quarkus.test.junit.QuarkusTestExtension.doJavaStart(QuarkusTestExtension.java:231)
        at io.quarkus.test.junit.QuarkusTestExtension.ensureStarted(QuarkusTestExtension.java:592)
        at io.quarkus.test.junit.QuarkusTestExtension.beforeAll(QuarkusTestExtension.java:640)
        at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.lambda$invokeBeforeAllCallbacks$12(ClassBasedTestDescriptor.java:395)
        at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
        at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.invokeBeforeAllCallbacks(ClassBasedTestDescriptor.java:395)
        at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.before(ClassBasedTestDescriptor.java:211)
        at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.before(ClassBasedTestDescriptor.java:84)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:148)
        ... 35 more

config

It doesn't break in regular integration tests, e.g. in the newly added AWT in-container test here.

I've searched for

java.util.ServiceConfigurationError: 
io.smallrye.config.SmallRyeConfigFactory: 
io.quarkus.runtime.configuration.QuarkusConfigFactory 
not a subtype

as it rang a bell for me. I have seen it before. It looks like people hit it when doing something that affects classloading.

I am not sure what to do about it ATM. It is clearer why there was a hardcoded value in the first place though smile

I think I have it nailed. One must not try to call it from a wrong classloader. Running tests locally before I torment Q CI again...

@Karm
Copy link
Member Author

Karm commented Mar 6, 2023

@geoand Apart from the issue with the integration-tests\maven, I didn't see anything like a regression when trying out these. A huge disclaimer though: The projects are hardwired to docker in their own code. So I can see that without this patch, it doesn't even start on a Podman-only system. With this patch, it starts, builds images, all is well until the application decides it needs docker command specifically to do something.

https://github.com/quarkiverse/quarkus-mockserver.git
Linux Podman: Works partially, it's hardwired to Docker.
Windows Podman: N/A it's hardwired to Docker.

https://github.com/quarkiverse/quarkus-zeebe.git
Linux Podman:  N/A it's hardwired to Docker in Zeebe code.
Windows Podman: N/A it's hardwired to Docker in Zeebe code.
Linux Docker:   No regression.

https://github.com/quarkiverse/quarkus-cert-manager.git
Linux Docker:   No regression.
Linux Podman:   No regression.
Windows Podman: No regression.

https://github.com/quarkiverse/quarkus-helm.git
Linux Docker:   No regression.
Linux Podman:   No regression.
Windows Podman: No regression.

I tested the 2.16 port of this patch since those projects are not ready for Quarkus 3.0 yet.

@quarkus-bot

This comment has been minimized.

@geoand
Copy link
Contributor

geoand commented Mar 7, 2023

@Karm good to know!

Karm added 4 commits March 14, 2023 00:57
e.g.
Before:

```
WARN  [io.qua.run.uti.ContainerRuntimeUtil] (main) Command "docker" exited with error code 1. Rootless container runtime detection might not be reliable.
```

after:

```
WARN  [io.qua.run.uti.ContainerRuntimeUtil] (main) Command "docker info" exited with error code 1. Rootless container runtime detection might not be reliable or the container service is not running at all.
```
plus with `-Dquarkus.log.level=DEBUG` it logs:

```
DEBUG [io.qua.run.uti.ContainerRuntimeUtil] (main) Command "docker info" output: Client:
 Context:    default
 Debug Mode: false
 Plugins:
  buildx: Docker Buildx (Docker Inc.)
    Version:  v0.10.2
    Path:     /usr/libexec/docker/cli-plugins/docker-buildx
  compose: Docker Compose (Docker Inc.)
    Version:  v2.16.0
    Path:     /usr/libexec/docker/cli-plugins/docker-compose
  scan: Docker Scan (Docker Inc.)
    Version:  v0.23.0
    Path:     /usr/libexec/docker/cli-plugins/docker-scan

Server:
ERROR: Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?
```
With this change, the detected container runtime is cached per JVM run
so as it is not checked again regardless of the classloader
loading the ContainerRuntimeUtil class.
The rootless attribute of such runtime is lazily fetched once as needed.
@Karm Karm force-pushed the issue-31307-q-main branch from 908acff to 6678ea5 Compare March 13, 2023 23:57
@Karm
Copy link
Member Author

Karm commented Mar 14, 2023

I will see about this next re-run, but so far I have seen Gradle timeouts and Quarkus Scheduler arbitrary failure... Not sure how that would be related.

@geoand
Copy link
Contributor

geoand commented Mar 14, 2023

The Gradle tests are pretty unreliable lately

@quarkus-bot
Copy link

quarkus-bot bot commented Mar 14, 2023

Failing Jobs - Building 6678ea5

Status Name Step Failures Logs Raw logs
✔️ Gradle Tests - JDK 11
Gradle Tests - JDK 11 Windows Build Failures Logs Raw logs
✔️ JVM Tests - JDK 11
✔️ JVM Tests - JDK 17
JVM Tests - JDK 17 Windows Build Failures Logs Raw logs
✔️ JVM Tests - JDK 19

Full information is available in the Build summary check run.

Failures

⚙️ Gradle Tests - JDK 11 Windows #

📦 integration-tests/gradle

io.quarkus.gradle.devmode.BasicKotlinApplicationModuleDevModeTest.main line 19 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.test.devmode.util.DevModeTestUtils that uses java.util.function.Supplier, java.util.function.Supplierjava.util.concurrent.atomic.AtomicReference, java.util.concurrent.atomic.AtomicReferencejava.lang.String, java.lang.Stringboolean was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

io.quarkus.gradle.devmode.MultiSourceProjectDevModeTest.main line 22 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.test.devmode.util.DevModeTestUtils that uses java.util.function.Supplier, java.util.function.Supplierjava.util.concurrent.atomic.AtomicReference, java.util.concurrent.atomic.AtomicReferencejava.lang.String, java.lang.Stringboolean was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

⚙️ JVM Tests - JDK 17 Windows #

- Failing: extensions/grpc/deployment 
! Skipped: extensions/micrometer-registry-prometheus/deployment extensions/micrometer/deployment extensions/opentelemetry/deployment and 42 more

📦 extensions/grpc/deployment

io.quarkus.grpc.server.devmode.GrpcDevModeTest.testEchoStreamReload - More details - Source on GitHub

java.util.concurrent.RejectedExecutionException
	at org.jboss.threads.RejectingExecutor.execute(RejectingExecutor.java:38)
	at org.jboss.threads.EnhancedQueueExecutor.rejectShutdown(EnhancedQueueExecutor.java:2136)

@Karm
Copy link
Member Author

Karm commented Mar 14, 2023

@geoand @Sanne I am happy with this PR now and I'd like it to get merged.

I realize it's changing stuff, yet that is kind of the point, to change the container runtime detection handling for the better, to make it work on Windows with Podman properly, to get rid of superfluous perpetual calls to docker/podman --version and info etc.

I tested on Linux and Windows and I exhausted what I wanted to check. If you have any suggestion on what to try more or which project to test build with this change, I am all ears.

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@geoand geoand merged commit 26013ff into quarkusio:main Mar 14, 2023
@quarkus-bot quarkus-bot bot added this to the 3.0 - main milestone Mar 14, 2023
if (SystemUtils.IS_OS_MAC
&& ContainerRuntimeUtil.detectContainerRuntime() == ContainerRuntimeUtil.ContainerRuntime.PODMAN) {
final String selinuxBindOption;
if (SystemUtils.IS_OS_MAC && containerRuntime == PODMAN) {
selinuxBindOption = "";
Copy link
Member

Choose a reason for hiding this comment

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

do we know the reason why podman on mac needs different arguments here?

Copy link
Member Author

Choose a reason for hiding this comment

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

No idea. I don't have access to any mac hw, so I tested this PR only on Windows and Linux. This bit hasn't been changed though.

Copy link
Member

Choose a reason for hiding this comment

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

mac qemu doesnt support selinux labels over 9pfs. Although not sure if this leads to an actual error, if it does it could be changed to ignore

* "quarkus_container_runtime.txt")`
* as the file is deleted between JVM executions anyway.
*/
static final Path CONTAINER_RUNTIME = Path.of(System.getProperty("java.io.tmpdir"), "quarkus_container_runtime.txt");
Copy link
Member

Choose a reason for hiding this comment

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

why is it it need a file and not just a static var?

Copy link
Member

Choose a reason for hiding this comment

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

IIUC, the problem is that we are using different CLs along the build so you end up loading things several time.

That being said, I'm also concerned we are using a file.
First, it won't work well in concurrent build environments as you are using a common file for all builds. That is a bad idea. You need to use Files.createTempFile().
Second, my recollection is that using deleteOnExit() is not recommended. So you will need to figure out a better way of deleting the file at the end of the build.

Maybe we need to take a step back here? I'm not sure where this is used but could we use a build item to propagate the value? IIRC the build step will only be executed if it is consumed? Not sure it would be handy though.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I'm not comfortable releasing Alpha6 with this build concurrency issue in so I'm leaning towards reverting until we come up with a better solution.

So if you feel strongly that I shouldn't, please speak up!

FWIW, the version without the file seemed like a good transition version until we figure things out.

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 think the build item will work, because IIRC this is needed in places outside the build steps as well.

That said, using Files.createTempFile() does seem more reasonable

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could just put it into a static field of a class that we always load from the system classloader?

FTR, one such field that is always loaded from the system CL is system properties 😆

/me hides

Copy link
Contributor

Choose a reason for hiding this comment

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

Hack is definitely is... But I think it's probable acceptable.

On the other hand we could create a temp file based on each JVM run that would not be global to the whole system as the current on is

Copy link
Member Author

Choose a reason for hiding this comment

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

@gsmet

IIUC, the problem is that we are using different CLs along
the build so you end up loading things several time.

Yes, exactly. The situation had several stages:

  • the current state is really suboptimal, many calls to -v, --version, info to get rootless state etc.
  • then I got rid of perpetual docker info by having the rootlessness checked lazily only when really needed
  • then I got rid of the superfluous -v calls, getting the docker command calls to even smaller number
  • the last step is caching it in a file. The difference between this and using a static attribute is 1 superfluous docker version call

All in all, if I change the usage of a temporary file to a static variable, the PR would still be very beneficial in this aspect.

First, it won't work well in concurrent build environments
as you are using a common file for all builds.

Do you mean there would be a race with StandardOpenOption.TRUNCATE_EXISTING? If a build gets an empty file, it merely ignores it and defaults to calling docker again...

You need to use Files.createTempFile()

I don't think so. That is obviously how temp files are used, but in this context I would end up where I started: Needing to remember somewhere the path to that file. I need to know the path. Not having to remember somewhere /tmp/container_runtime_<some arbitrary generated number I don't know>.txt. So that is why I did what I did with the file name.

recollection is that using deleteOnExit() is not recommended

Could you elaborate? The only problem with deleteOnExit is that is is not guaranteed and it is platform specific, so it's a very bad idea to have it if you need to rely on the file being really gone. This is not the case. The worst that might happen (IMHO) is that the file stays there in temp, user switches from docker to podman and there would be a warning or failure depending on what user does. Perhaps we could time stamp it it mention it in the log...

Maybe we need to take a step back here?
I'm not sure where this is used but could we use a build item
to propagate the value?

I am all for taking a step back. I spent some nontrivial time on Windows and Linux, considering dev mode, docker-image extension usage, native build using builder image and test suite using runtime containers. Dumping the info into a file was the thing that looked the least complicated from my limited POV. I am happy to try other avenues.

Copy link
Member

Choose a reason for hiding this comment

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

yeah so build concurency was my concern too about the filename.

Though I'm not sure its that big a deal in practice as what will happen is a concurrent one will pick up the one running on the same system. Only time it will problematic is if those two runs are running with different default environment where one would have picked podman over docker and vice versa.

One way to avoid that issue is to use ProcessHandle.current().pid() and append/prefix the filename. Then at least it will be per process rather than global.

About deleteOnExit() - my understanding is that you just shouldn't assume it always gets deleted (i.e. ctrl+c will hard stop and deletes does not happen); but no other mechanism can do that better anyway.

Copy link
Member

Choose a reason for hiding this comment

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

@Karm for now I haven't seen any strong opinions against a system property approach? So if it works, maybe we could try that? That would alleviate all the concerns expressed so far regarding the file approach?

Maybe shoe it in with your current code to see if it works before cleaning things up.

Copy link
Member

Choose a reason for hiding this comment

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

Something like quarkus-local-container-runtime (lowercase, no quarkus. prefix) to make sure it doesn't collide with the config properties.

@Karm
Copy link
Member Author

Karm commented Mar 14, 2023

@gsmet @maxandersen

One way to avoid that issue is to use ProcessHandle.current().pid()
and append/prefix the filename. Then at least it will be per process rather than global.

I will add it in another PR and test it on windows, it seems like an obvious thing that just didn't occur to me, Thx.

@Karm
Copy link
Member Author

Karm commented Mar 14, 2023

@gsmet @Ladicek

what would be so wrong using a system property?
It's a bit of a hack, sure, but it does actually sound more
appealing than the other solutions. Would there be any obvious drawback?

When I approached that the first time, logging the zillion of various info and -v an version calls with my wrapper:

$ cat /home/karm/bin/docker
#!/bin/bash
echo "$(date) docker $@" >> ~/.docker_karm.log
/bin/docker $@

My intention was to make the setting last between different JVM runs. Then I backed away from that, realizing it would mean yet another garbage left by quarkus runs and there is already enough of that (many vertx things, many jansi files, many temp files etc.).
I briefly considered using .java directory in user's home. Then I shrieked in horror realizing that the Quarkus run is deleting and writing something in my home directory.
So the current version does the deleteOnExit thing. All in all, I don't thing there is any obvious drawback in using sys property for that same thing. I can test that... (It would obviously mean we are giving up on the idea of sharing that config state between JVM runs in any way)

@maxandersen
Copy link
Member

(It would obviously mean we are giving up on the idea of sharing that config state between JVM runs in any way)

sharing between jvm runs does NOT seem like a good thing IMO.

if we have test runs that can benefit from that its better we make that explicitly share some config.

the last step is caching it in a file. The difference between this and using a static attribute is 1 superfluous docker version call

if that's the only thing and the pid trick doesn't work then doing static attribute seems just as fine. 1 docker version call sholdn't hurt compared to otherwise fun concurrency debugging a yea from now when we all forgotten we accidentally share state between jvm runs ? :)

@Karm
Copy link
Member Author

Karm commented Mar 14, 2023

@maxandersen

sharing between jvm runs does NOT seem like a good thing IMO.

Well, depends on how you put it. sharing it between arbitrary runs, in /tmp/container-runtime.txt, bad idea. Sharing it among the various JVM executions as a part of a single ./mvn verify call, IMHO O.K. Maybe the file could be in the project root...? .container-runtime.txt

1 docker version call

That is per test suite module run AFAIK.

pid trick doesn't work

I think PID trick would work and I will do it and test it (frowning towards my Windows instance, usual suspects).
I still don't get the concurrency thing though. The file could be in a flux state depending on OS and filesystem:

  • empty
  • not there
  • locked
  • containting garbage

And if that happens, Quarkus merely falls back to calling docker version / podman version again....

EDIT: Now Karm gets it. One concurrent build is trying to use podman, another on the same system is trying to use docker.
I have Windows system that does both. It just did not occur to me to run it in parallel due to port conflicts and also system resources that just wouldn't take it...

@Sanne
Copy link
Member

Sanne commented Mar 14, 2023

Step-back idea 1: follow the example set by Testcontainers?

In Testcontainers, if there is no '~/.testcontainers.properties' file it runs some similar checks, then stores it in the ". file" in unix style - same on Windows.

It's not a perfect solution, I initially actually didn't like it as it led to a puzzling problem for me at some point - but this was trivial to fix once I became aware of this and it's clearly documented.

It does have advantages: it result in consistent behaviour, and once people become aware of it it's simple to manage and to override.

The best feature is that once this is set, the users get the benefit for any subsequent run: across tests, JVMs and projects, and consistently, with the user having clear options.

Step-back idea 2: actually delegate to Testcontainers code?

Alternatively, and perhaps nicer, is to fully leverage the code and configuration that Testcontainers already requires.

There's a docker.client.strategy property in that file, which people might need to have set to get a good experience with podman.

I haven't checked about practicalities in this case, but it certainly sounds appealing to me to delegate the problem:

  • we already expect users to use Testcontainers, so they already had the burden of making sure this is working or configured correctly.
  • don't ask our users to repeat such configuration twice?

What I mean to suggest, is that even if the ". files" strategy has the drawback of people potentially needing to be aware of the files, that ship has already sailed as we're having them use Testcontainers and so our users already need to be aware of this.

@gsmet
Copy link
Member

gsmet commented Mar 14, 2023

I think we need:

  • a fix for the behavior that was merged, and I would lean towards trying the system property idea for now: that would avoid relying on files (if it works, obviously)
  • then we can take a step back and think about the big picture. But we should do that in a second time.

@Karm
Copy link
Member Author

Karm commented Mar 14, 2023

@Sanne I like the .file situation the most, being it system wide or project wide. Not sure how ~/.testcontainers.properties address the concern about concurrent execution raised by @gsmet and @maxandersen about the current global quarkus_container_runtime.txt. I guess such possibility of having different values for different concurrent runs in

docker.client.strategy=org.testcontainers.dockerclient.UnixSocketClientProviderStrategy
testcontainers.reuse.enable=false

is just ignored.

Ad strategy using socket: It did not work for me with Podman on Windows. Not sure if it was permissions (I don't run the TS with elevated rights) or what was it.

@gsmet O.K., I'll switch to sys prop and show statistic like in #31662 (comment)
Could you also comment on the possibility of having the file in the project root (and not deleted on JVM exit)? Determined by basedir that Maven sets. I use it reliably here: Commands.java#L127. Not sure if Gradle sets anything similar...

It must feel like I am obsessed with littering files all over the place.

@gsmet
Copy link
Member

gsmet commented Mar 14, 2023

@Karm if you make it a file that survives JVM runs, you completely change the semantic of this feature:

  • currently, it's an autodetection feature. If you install Podman, it will detect it, if you go back to Docker, it will detect it.
  • if you have a file that survives JVM runs, it won't. So it somehow becomes a configuration file.

If we want to change the semantic, we definitely need to discuss the various options.

@aloubyansky
Copy link
Member

We should fix it in 2.16 too.

@gsmet gsmet modified the milestones: 3.0.0.Alpha6, 2.16.6.Final Mar 25, 2023
benkard added a commit to benkard/mulkcms2 that referenced this pull request Apr 4, 2023
This MR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [flow-bin](https://github.com/flowtype/flow-bin) ([changelog](https://github.com/facebook/flow/blob/master/Changelog.md)) | devDependencies | minor | [`^0.201.0` -> `^0.203.0`](https://renovatebot.com/diffs/npm/flow-bin/0.201.0/0.203.1) |
| [com.rometools:rome](http://rometools.com) ([source](https://github.com/rometools/rome)) | compile | minor | `2.0.0` -> `2.1.0` |
| [org.postgresql:postgresql](https://jdbc.postgresql.org) ([source](https://github.com/pgjdbc/pgjdbc)) | build | minor | `42.5.4` -> `42.6.0` |
| [com.diffplug.spotless:spotless-maven-plugin](https://github.com/diffplug/spotless) | build | minor | `2.34.0` -> `2.35.0` |
| [org.apache.maven.plugins:maven-resources-plugin](https://maven.apache.org/plugins/) | build | patch | `3.3.0` -> `3.3.1` |
| [io.quarkus:quarkus-maven-plugin](https://github.com/quarkusio/quarkus) | build | patch | `2.16.4.Final` -> `2.16.6.Final` |
| [io.quarkus:quarkus-universe-bom](https://github.com/quarkusio/quarkus-platform) | import | patch | `2.16.4.Final` -> `2.16.6.Final` |

---

### Release Notes

<details>
<summary>flowtype/flow-bin</summary>

### [`v0.203.1`](flow/flow-bin@0c16b26...5e0645d)

[Compare Source](flow/flow-bin@0c16b26...5e0645d)

### [`v0.203.0`](flow/flow-bin@861f798...0c16b26)

[Compare Source](flow/flow-bin@861f798...0c16b26)

### [`v0.202.1`](flow/flow-bin@2b48bba...861f798)

[Compare Source](flow/flow-bin@2b48bba...861f798)

### [`v0.202.0`](flow/flow-bin@86aea9c...2b48bba)

[Compare Source](flow/flow-bin@86aea9c...2b48bba)

</details>

<details>
<summary>rometools/rome</summary>

### [`v2.1.0`](https://github.com/rometools/rome/releases/tag/2.1.0)

[Compare Source](rometools/rome@2.0.0...2.1.0)

<!-- Release notes generated using configuration in .github/release.yml at 2.1.0 -->

#### What's Changed

##### ⭐ New Features

-   Downgrade Java from version 11 to 8 by [@&#8203;PatrickGotthard](https://github.com/PatrickGotthard) in rometools/rome#642
-   Add support for GraalVM native images by [@&#8203;artembilan](https://github.com/artembilan) in rometools/rome#636

##### 🔨 Dependency Upgrades

-   Bump maven-compiler-plugin from 3.10.1 to 3.11.0 by [@&#8203;dependabot](https://github.com/dependabot) in rometools/rome#635

##### 🧹 Cleanup

-   Remove unused config files by [@&#8203;PatrickGotthard](https://github.com/PatrickGotthard) in rometools/rome#632
-   Polish GitHub workflows by [@&#8203;PatrickGotthard](https://github.com/PatrickGotthard) in rometools/rome#633
-   Polish code by [@&#8203;antoniosanct](https://github.com/antoniosanct) in rometools/rome#631

##### ✔ Other Changes

-   Update configuration for automatically generated release notes by [@&#8203;PatrickGotthard](https://github.com/PatrickGotthard) in rometools/rome#634

#### New Contributors

-   [@&#8203;artembilan](https://github.com/artembilan) made their first contribution in rometools/rome#636

**Full Changelog**: rometools/rome@2.0.0...2.1.0

</details>

<details>
<summary>pgjdbc/pgjdbc</summary>

### [`v42.6.0`](https://github.com/pgjdbc/pgjdbc/blob/HEAD/CHANGELOG.md#&#8203;4260-2023-03-17-153434--0400)

##### Changed

fix: use PhantomReferences instead of `Obejct.finalize()` to track Connection leaks [MR #&#8203;2847](pgjdbc/pgjdbc#2847)

    The change replaces all uses of Object.finalize with PhantomReferences.
    The leaked resources (Connections) are tracked in a helper thread that is active as long as
    there are connections in use. By default, the thread keeps running for 30 seconds after all
    the connections are released. The timeout is set with pgjdbc.config.cleanup.thread.ttl system property.

refactor:(loom) replace the usages of synchronized with ReentrantLock [MR #&#8203;2635](pgjdbc/pgjdbc#2635)
Fixes [Issue #&#8203;1951](pgjdbc/pgjdbc#1951)

</details>

<details>
<summary>diffplug/spotless</summary>

### [`v2.35.0`](https://github.com/diffplug/spotless/blob/HEAD/CHANGES.md#&#8203;2350---2023-02-10)

##### Added

-   CleanThat Java Refactorer. ([#&#8203;1560](diffplug/spotless#1560))
-   Introduce `LazyArgLogger` to allow for lazy evaluation of log messages in slf4j logging. ([#&#8203;1565](diffplug/spotless#1565))

##### Fixed

-   Allow multiple instances of the same npm-based formatter to be used by separating their `node_modules` directories. ([#&#8203;1565](diffplug/spotless#1565))
-   `ktfmt` default style uses correct continuation indent. ([#&#8203;1562](diffplug/spotless#1562))

##### Changes

-   Bump default `ktfmt` version to latest `0.42` -> `0.43` ([#&#8203;1561](diffplug/spotless#1561))
-   Bump default `jackson` version to latest `2.14.1` -> `2.14.2` ([#&#8203;1536](diffplug/spotless#1536))

</details>

<details>
<summary>quarkusio/quarkus</summary>

### [`v2.16.6.Final`](https://github.com/quarkusio/quarkus/releases/tag/2.16.6.Final)

[Compare Source](quarkusio/quarkus@2.16.5.Final...2.16.6.Final)

##### Complete changelog

-   [#&#8203;32319](quarkusio/quarkus#32319) - \[2.16] Revert io.netty.noUnsafe change
-   [#&#8203;32302](quarkusio/quarkus#32302) - Qute - fix validation of expressions with the "cdi" namespace
-   [#&#8203;32253](quarkusio/quarkus#32253) - (2.16) Upgrade to graphql-java 19.4
-   [#&#8203;32223](quarkusio/quarkus#32223) - (2.16) Upgrade wildfly-elytron to 1.20.3.Final
-   [#&#8203;32110](quarkusio/quarkus#32110) - Prevent splitting of cookie header values when using AWS Lambda
-   [#&#8203;32107](quarkusio/quarkus#32107) - Fix Podman detection on Windows
-   [#&#8203;32106](quarkusio/quarkus#32106) - Native building with container: Podman not detected on Windows
-   [#&#8203;32093](quarkusio/quarkus#32093) - Re-use current ApplicationModel for JaCoCo reports when testing Gradle projects
-   [#&#8203;32090](quarkusio/quarkus#32090) - K8s moved its registry
-   [#&#8203;32088](quarkusio/quarkus#32088) - Remove the session cookie if ID token verification failed
-   [#&#8203;32082](quarkusio/quarkus#32082) - Add missing quote in Hibernate Reactive with Panache guide
-   [#&#8203;32079](quarkusio/quarkus#32079) - Quarkus JaCoCo extension fails to start Gradle daemon
-   [#&#8203;32058](quarkusio/quarkus#32058) - Allow use of null in REST Client request body
-   [#&#8203;32047](quarkusio/quarkus#32047) - rest client reactive throws npe on null request body
-   [#&#8203;32041](quarkusio/quarkus#32041) - K8s is moving it's images
-   [#&#8203;32037](quarkusio/quarkus#32037) - Set-Cookie Header is Split when using OIDC together with AWS Lambda
-   [#&#8203;32015](quarkusio/quarkus#32015) - Support repeatable Incomings annotation for reactive messaging
-   [#&#8203;32002](quarkusio/quarkus#32002) - Quarkus: Kafka Event Processor with 2 `@incoming` annotations throws Null Pointer SRMSG00212
-   [#&#8203;31984](quarkusio/quarkus#31984) - Only substitute OctetKeyPair\* classes when on the classpath
-   [#&#8203;31978](quarkusio/quarkus#31978) - Remove quarkus.hibernate-orm.database.generation=drop-and-create from Hibernate ORM codestart
-   [#&#8203;31930](quarkusio/quarkus#31930) - Native build fails for JWT
-   [#&#8203;31893](quarkusio/quarkus#31893) - Docker or Podman required for tests since 3.0.0.Alpha6
-   [#&#8203;31857](quarkusio/quarkus#31857) - Container runtime detection cached in sys prop, container-docker extension
-   [#&#8203;31811](quarkusio/quarkus#31811) - Check the expiry date for inactive OIDC tokens
-   [#&#8203;31717](quarkusio/quarkus#31717) - Quarkus OIDC Session Cookie not deleted in case of 401 unauthorized
-   [#&#8203;31714](quarkusio/quarkus#31714) - OIDC token refresh fails with 401, if user info is used and not available in the cache (anymore)
-   [#&#8203;31662](quarkusio/quarkus#31662) - Warning when docker is not running
-   [#&#8203;31525](quarkusio/quarkus#31525) - Bump Keycloak version to 21.0.1
-   [#&#8203;31490](quarkusio/quarkus#31490) - Enable Podman and Docker Windows quarkus-container-image-docker testing
-   [#&#8203;31307](quarkusio/quarkus#31307) - Native Build on Windows has incorrect resource slashes
-   [#&#8203;30383](quarkusio/quarkus#30383) - Create a new base classloader including parent-first test scoped dependencies when bootstrapping for CT

### [`v2.16.5.Final`](https://github.com/quarkusio/quarkus/releases/tag/2.16.5.Final)

[Compare Source](quarkusio/quarkus@2.16.4.Final...2.16.5.Final)

##### Complete changelog

-   [#&#8203;31959](quarkusio/quarkus#31959) - New home for Narayana LRA coordinator Docker images
-   [#&#8203;31931](quarkusio/quarkus#31931) - Support raw collections in RESTEasy Reactive server and client
-   [#&#8203;31922](quarkusio/quarkus#31922) - Add more lenient Liquibase ZipPathHandler to work around includeAll not working in prod mode
-   [#&#8203;31904](quarkusio/quarkus#31904) - \[2.16] Upgrade SmallRye GraphQL to 1.9.4
-   [#&#8203;31894](quarkusio/quarkus#31894) - Supply missing extension metadata for reactive keycloak client
-   [#&#8203;31891](quarkusio/quarkus#31891) - Fix truststore REST Client config when password is not set
-   [#&#8203;31867](quarkusio/quarkus#31867) - Qute type-safe fragments - fix validation for loop metadata and globals
-   [#&#8203;31866](quarkusio/quarkus#31866) -  The behavior of the `@RestHeader` annotation is different from the `@HeaderParam` annotation when the parameter is of type List
-   [#&#8203;31864](quarkusio/quarkus#31864) - Fix incorrect generic type passed to MessageBodyWriter#writeTo
-   [#&#8203;31818](quarkusio/quarkus#31818) - Jackson JAX-RS YAML Provider for Resteasy Reactive
-   [#&#8203;31804](quarkusio/quarkus#31804) - \[2.16] A test to make sure non-existing modules are ignored during workspace discovery
-   [#&#8203;31793](quarkusio/quarkus#31793) - \[2.16] Fix NPE loading workspace modules
-   [#&#8203;31770](quarkusio/quarkus#31770) - Fix native compilation when using quarkus-jdbc-oracle with elasticsearch-java
-   [#&#8203;31769](quarkusio/quarkus#31769) - Capability added for quarkus-rest-client-reactive-jackson
-   [#&#8203;31756](quarkusio/quarkus#31756) - quarkus-rest-client-reactive-jackson doesn't provide capabilities
-   [#&#8203;31728](quarkusio/quarkus#31728) - Register additional cache implementations for reflection
-   [#&#8203;31718](quarkusio/quarkus#31718) - Properly close metadata file in integration tests
-   [#&#8203;31713](quarkusio/quarkus#31713) - "Too many open files" When test native image.
-   [#&#8203;31712](quarkusio/quarkus#31712) - Make request scoped beans work properly in ReaderInterceptors
-   [#&#8203;31705](quarkusio/quarkus#31705) - Remove all dev services for kubernetes dependencies from kubernetes-client-internal
-   [#&#8203;31692](quarkusio/quarkus#31692) - RequestScoped context not active when using a ReaderInterceptor with large HTTP requests
-   [#&#8203;31688](quarkusio/quarkus#31688) - Suppress config changed warning for quarkus.test.arg-line
-   [#&#8203;31643](quarkusio/quarkus#31643) - Fix iterator issue when executing a zrange with score on a missing key
-   [#&#8203;31626](quarkusio/quarkus#31626) - quarkus.test.arg-line has become a built-time fixed property in 2.16.4
-   [#&#8203;31624](quarkusio/quarkus#31624) - native compilation : quarkus-jdbc-oracle with elasticsearch-java strange behaviour
-   [#&#8203;31617](quarkusio/quarkus#31617) - Bump Stork version 1.4.2
-   [#&#8203;31579](quarkusio/quarkus#31579) - Reinitialize sun.security.pkcs11.P11Util at runtime
-   [#&#8203;31560](quarkusio/quarkus#31560) - Prevent SSE writing from potentially causing accumulation of headers
-   [#&#8203;31559](quarkusio/quarkus#31559) - `SseUtil` unexpectedly stores headers in `Serialisers.EMPTY_MULTI_MAP`
-   [#&#8203;31551](quarkusio/quarkus#31551) - Scheduler - detect scheduled methods of the same name on a class
-   [#&#8203;31547](quarkusio/quarkus#31547) - Scheduler - it's possible to declare two scheduled methods of the same name on the same class
-   [#&#8203;31545](quarkusio/quarkus#31545) - Append System.lineSeparator() to config error messages
-   [#&#8203;31536](quarkusio/quarkus#31536) - Missing newline characters in config error message
-   [#&#8203;31532](quarkusio/quarkus#31532) - Interpret negative/zero body-limit as infinite when logging REST Client request body
-   [#&#8203;31523](quarkusio/quarkus#31523) - Request rejected by CORS for fonts in dev UI when `quarkus.http.cors=true` is set
-   [#&#8203;31496](quarkusio/quarkus#31496) - Filter out RESTEasy related warning in ProviderConfigInjectionWarningsTest
-   [#&#8203;31482](quarkusio/quarkus#31482) - Remove incorrect default value for keepAliveEnabled
-   [#&#8203;31440](quarkusio/quarkus#31440) - Several quarkus integration tests fail to compile to native with latest GraalVM master
-   [#&#8203;31384](quarkusio/quarkus#31384) - Ignore required documentation for `@ConfigMapping` default methods
-   [#&#8203;30757](quarkusio/quarkus#30757) - Allow same origin CORS requests without 3rd party origins being configured
-   [#&#8203;30744](quarkusio/quarkus#30744) - \[Quarkus Native] ClassNotFoundException: com.github.benmanes.caffeine.cache.SSSW
-   [#&#8203;30698](quarkusio/quarkus#30698) - CORS Request same origin ignored if no other origin set

</details>

<details>
<summary>quarkusio/quarkus-platform</summary>

### [`v2.16.6.Final`](quarkusio/quarkus-platform@2.16.5.Final...2.16.6.Final)

[Compare Source](quarkusio/quarkus-platform@2.16.5.Final...2.16.6.Final)

### [`v2.16.5.Final`](quarkusio/quarkus-platform@2.16.4.Final...2.16.5.Final)

[Compare Source](quarkusio/quarkus-platform@2.16.4.Final...2.16.5.Final)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox.

👻 **Immortal**: This MR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4yNC4wIiwidXBkYXRlZEluVmVyIjoiMzQuMjQuMCJ9-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warning when docker is not running Native Build on Windows has incorrect resource slashes
10 participants