Skip to content

Catch Docker check exceptions in lightweight OIDC devservice #47346

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
Merged
merged 1 commit into from
Apr 25, 2025
Merged

Catch Docker check exceptions in lightweight OIDC devservice #47346

merged 1 commit into from
Apr 25, 2025

Conversation

sberyozkin
Copy link
Member

@sberyozkin sberyozkin commented Apr 14, 2025 •
edited by geoand
Loading

I've noticed integration-tests/oidc-wiremock-logout is failing periodically on Windows, I looked at the stacktrace,

2025-04-13T22:15:17.6845464Z [INFO] --- surefire:3.5.2:test (default-test) @ quarkus-integration-test-oidc-wiremock-logout ...
2025-04-13T22:15:25.1747536Z 2025-04-13 22:15:24,935 INFO  [org.tes.DockerClientFactory] (build-49) Testcontainers version: 1.20.6
2025-04-13T22:15:26.2371962Z 2025-04-13 22:15:26,233 INFO  [org.tes.doc.DockerClientProviderStrategy] (build-49) Found Docker environment with local Npipe socket (npipe:////./pipe/docker_engine)
2025-04-13T22:15:26.2411325Z 2025-04-13 22:15:26,233 WARN  [org.tes.doc.DockerClientProviderStrategy] (build-49) windows is currently not supported

2025-04-13T22:15:26.3392629Z 2025-04-13 22:15:26,240 INFO  [org.tes.doc.DockerMachineClientProviderStrategy] (build-49) docker-machine executable was not found on PATH ...

2025-04-13T22:15:26.3401567Z 2025-04-13 22:15:26,242 ERROR [org.tes.doc.DockerClientProviderStrategy] (build-49) Could not find a valid Docker environment. Please check configuration. Attempted configurations were:
2025-04-13T22:15:26.3402733Z 	NpipeSocketClientProviderStrategy: failed with exception InvalidConfigurationException (windows containers are currently not supported)As no valid configuration was found, execution cannot continue.
2025-04-13T22:15:26.3403611Z See https://java.testcontainers.org/on_failure.html for more details.
2025-04-13T22:15:36.5670701Z java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
2025-04-13T22:15:36.5672877Z 	[error]: Build step io.quarkus.devservices.oidc.OidcDevServicesProcessor#startServer threw an exception: java.lang.IllegalThreadStateException: process has not exited
2025-04-13T22:15:36.5675023Z 	at java.base/java.lang.ProcessImpl.exitValue(ProcessImpl.java:566)
2025-04-13T22:15:36.5677248Z 	at io.quarkus.deployment.util.ContainerRuntimeUtil.getVersionOutputFor(ContainerRuntimeUtil.java:239)
2025-04-13T22:15:36.5679890Z 	at io.quarkus.deployment.util.ContainerRuntimeUtil.getContainerRuntimeEnvironment(ContainerRuntimeUtil.java:116)
2025-04-13T22:15:36.5685047Z 	at io.quarkus.deployment.util.ContainerRuntimeUtil.detectContainerRuntime(ContainerRuntimeUtil.java:66)
2025-04-13T22:15:36.5690161Z 	at io.quarkus.deployment.util.ContainerRuntimeUtil.detectContainerRuntime(ContainerRuntimeUtil.java:55)
2025-04-13T22:15:36.5692352Z 	at io.quarkus.deployment.IsDockerWorking$DockerBinaryStrategy.get(IsDockerWorking.java:23)
2025-04-13T22:15:36.5693435Z 	at io.quarkus.deployment.IsDockerWorking$DockerBinaryStrategy.get(IsDockerWorking.java:20)
2025-04-13T22:15:36.5694790Z 	at io.quarkus.deployment.IsContainerRuntimeWorking.getAsBoolean(IsContainerRuntimeWorking.java:37)
2025-04-13T22:15:36.5696365Z 	at io.quarkus.deployment.builditem.ContainerRuntimeStatusBuildItem.isContainerRuntimeAvailable(ContainerRuntimeStatusBuildItem.java:18)
2025-04-13T22:15:36.5698449Z 	at io.quarkus.devservices.oidc.OidcDevServicesProcessor.shouldNotStartServer(OidcDevServicesProcessor.java:164)

so I hope just wrapping the check in a catch block will help, the lightweight OIDC devservice only checks if the Docker is available to figure out if Keycloak dev service may have to start instead, it does not start any container itself

@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
michalvavrik
michalvavrik approved these changes Apr 14, 2025
View reviewed changes
Copy link
Member

@michalvavrik michalvavrik left a comment

Choose a reason for hiding this comment

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

I didn't know about the failure, but I have also mentioned that dockerStatusBuildItem.isContainerRuntimeAvailable() results into exception logged when I was testing DEV mode. +1 for not failing

@gsmet
Copy link
Member

gsmet commented Apr 14, 2025

Should we have a dependency between the two instead? And have a build item that states if the Keycloak Dev Services will be started or not?

@michalvavrik
Copy link
Member

Should we have a dependency between the two instead? And have a build item that states if the Keycloak Dev Services will be started or not?

yep, but as I tried to explain (I think last week on Slack? not sure when) right now when OIDC Dev Svc is tightly coupled to OIDC while Keycloak Dev SVc has very different logic for other extensions and maybe, we should share a great deal of that logic as well (e.g. is server url set, is client server url set etc. etc.). I think we shouldn't hurry with introducing SPI just because of this, I'd like to enhance the startup logic, but I don't have space now

@michalvavrik
Copy link
Member

but that would avoid the Docker check in this extension, is that what you meant? that is clever, I didn't realize. we can add spi

@sberyozkin
Copy link
Member Author

OIDC dev service is likely going to remain the only dev service that may have to be aware that Keycloak devservice may have to be started instead, is SPI really needed ?

@gsmet
Copy link
Member

gsmet commented Apr 14, 2025

So not saying we should do what I said but I'm a bit unhappy about what's going on: it's a bit annoying that we end up with logging about Docker not being available while it's a perfectly acceptable setup in this case.

@sberyozkin
Copy link
Member Author

As far as I recall, we only added this Docker check to avoid asking users to set quarkus.oidc.devservice.enabled=false, given that Dev Service for Keycloak starts by default. So the lightweight service is not started if the Keycloak devservice is expected to be able to start.
Equally, quarkus.oidc.devservice.enabled=true is not needed if we know Keycloak is not able to start given that Docker is not available

@sberyozkin
Copy link
Member Author

we end up with logging about Docker not being available while it's a perfectly acceptable setup in this case.

OK, I see what you mean

@sberyozkin
Copy link
Member Author

sberyozkin commented Apr 14, 2025 •
edited
Loading

That said, may be can shoud log: Docker is not available, enabling OIDC devservice ?

@michalvavrik
Copy link
Member

That said, may be can shoud log: Docker is not available, enabling OIDC devservice ?

@gsmet added some line like that lately

@sberyozkin
Copy link
Member Author

sberyozkin commented Apr 14, 2025 •
edited
Loading

I see it, that message is useful, but that one is about not starting because the container runtime being available, and my understanding is that @gsmet would like the users be not alarmed unnecessarily when they see a log that the docker/container not available because this is OK for this light devservice option...

@michalvavrik
Copy link
Member

michalvavrik commented Apr 14, 2025 •
edited
Loading

I see it, that message is useful, but that one is about not starting because the container runtime being available, and my understanding is that @gsmet would like the users be not alarmed unnecessarily when they see a log that the docker/container not available because this is OK for this light devservice option...

yeah, that makes definitely sense if we can't avoid the warn logging altogether

@sberyozkin sberyozkin force-pushed the catch_docker_exception_in_oidc_devservice branch from acec6cd to 821f6f4 Compare April 14, 2025 14:46
@sberyozkin
Copy link
Member Author

@michalvavrik @gsmet I've updated the log messages related to the docker check, have a look please

@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@sberyozkin
Copy link
Member Author

sberyozkin commented Apr 14, 2025 •
edited
Loading

The failure looks unrelated and to do with the legacy Keycloak image pull

@sberyozkin sberyozkin force-pushed the catch_docker_exception_in_oidc_devservice branch from 821f6f4 to ee0d217 Compare April 14, 2025 16:00
@sberyozkin
Copy link
Member Author

Just rerun it on my laptop, this test passes on this branch. Rebased and force-pushed.

@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Apr 14, 2025 •
edited by github-actions bot
Loading

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit ee0d217.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@holly-cummins holly-cummins mentioned this pull request Apr 16, 2025
Closed
This was referenced Apr 23, 2025
Merged
Merged
gsmet
gsmet approved these changes Apr 25, 2025
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Let's merge this but as discussed with @michalvavrik I would prefer if we had a proper integration between the two.

@gsmet gsmet merged commit 31ba82e into quarkusio:main Apr 25, 2025
27 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.23 - main milestone Apr 25, 2025
@sberyozkin sberyozkin deleted the catch_docker_exception_in_oidc_devservice branch April 25, 2025 16:29
@gsmet gsmet modified the milestones: 3.23 - main, 3.22.1 Apr 29, 2025
@jmartisk jmartisk modified the milestones: 3.22.1, 3.20.1 May 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gsmet gsmet gsmet approved these changes

+1 more reviewer

@michalvavrik michalvavrik michalvavrik approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/devservices area/oidc env/windows Impacts Windows machines kind/bugfix

Projects

None yet

Milestone

3.20.1

Development

Successfully merging this pull request may close these issues.

CI failure in OIDC wiremock logout on Windows

5 participants

@sberyozkin @gsmet @michalvavrik @jmartisk @quarkusbot