-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Compose Dev Service #46848
base: main
Are you sure you want to change the base?
Compose Dev Service #46848
Conversation
/cc @gsmet (elasticsearch), @loicmathieu (elasticsearch), @marko-bekhta (elasticsearch) |
This comment has been minimized.
This comment has been minimized.
🎊 PR Preview 21ab89f has been successfully built and deployed to https://quarkus-pr-main-46848-preview.surge.sh/version/main/guides/
|
This comment has been minimized.
This comment has been minimized.
5c8ef2f
to
2dd25b2
Compare
This comment has been minimized.
This comment has been minimized.
2dd25b2
to
3232191
Compare
This comment has been minimized.
This comment has been minimized.
@geoand last commit contains a change to enable the use of the shared network with compose services and the container-based app test runner. It also smooths already running compose app discovery scenarios and continuous-testing support. With the addition of |
This comment has been minimized.
This comment has been minimized.
...t/src/main/java/io/quarkus/deployment/dev/devservices/ComposeDevServicesBuildTimeConfig.java
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
I don't think that failing test |
...ment/src/main/java/io/quarkus/observability/deployment/ObservabilityDevServiceProcessor.java
Outdated
Show resolved
Hide resolved
|
||
1. A working *local* container environment: https://docs.docker.com/get-started/get-docker/[Docker] or https://podman.io/docs/installation[Podman] | ||
|
||
2. Compose tooling such as `docker-compose` or `podman-compose` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does it pick which to use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i.e. if i have both installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It uses the container runtime detection we've in Quarkus. It is overridable via quarkus.native.container-runtime=podman|docker
(maybe we need to remove the native from that property). I don't know if I mentioned it in the doc but podman uses docker-compose
binary when available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is overridable via quarkus.native.container-runtime=podman|docker (maybe we need to remove the native from that property)
At some point (not part of this PR) we need to unify this with quarkus.container-image.builder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will be awesome :) added a few comments/question for clarficiation.
|
||
1. A working *local* container environment: https://docs.docker.com/get-started/get-docker/[Docker] or https://podman.io/docs/installation[Podman] | ||
|
||
2. Compose tooling such as `docker-compose` or `podman-compose` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i.e. if i have both installed.
...ent/src/main/java/io/quarkus/devservices/deployment/compose/ComposeDevServicesProcessor.java
Outdated
Show resolved
Hide resolved
...ent/src/main/java/io/quarkus/devservices/deployment/compose/ComposeDevServicesProcessor.java
Outdated
Show resolved
Hide resolved
if (strategy != null) { | ||
LOG.infov("Waiting for service {0} to be ready", serviceName); | ||
try { | ||
strategy.waitUntilReady(instance); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For information, when disabling ryuk and enabling reuse of tests
quarkus.compose.devservices.ryuk-enabled=true
quarkus.compose.devservices.reuse-project-for-tests=true
An error can be thrown because the listChildContainers
also returns stopped containers and when the wait strategy is called on a stopped instance it throws an error.
For example:
io.quarkus.builder.BuildException: Build failure: Build failed due to errors
[error]: Build step io.quarkus.devservices.deployment.compose.ComposeDevServicesProcessor#config threw an exception: java.util.concurrent.CompletionException: java.lang.RuntimeException: java.lang.IllegalArgumentException: Requested port (5556) is not mapped
at java.base/java.util.concurrent.CompletableFuture.encodeThrowable(CompletableFuture.java:315)
at java.base/java.util.concurrent.CompletableFuture.completeThrowable(CompletableFuture.java:320)
at java.base/java.util.concurrent.CompletableFuture$AsyncRun.run(CompletableFuture.java:1807)
at org.jboss.threads.ContextHandler$1.runWith(ContextHandler.java:18)
at org.jboss.threads.EnhancedQueueExecutor$Task.doRunWith(EnhancedQueueExecutor.java:2675)
at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2654)
at org.jboss.threads.EnhancedQueueExecutor.runThreadBody(EnhancedQueueExecutor.java:1627)
at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1594)
at java.base/java.lang.Thread.run(Thread.java:1583)
at org.jboss.threads.JBossThread.run(JBossThread.java:499)
Caused by: java.lang.RuntimeException: java.lang.IllegalArgumentException: Requested port (5556) is not mapped
at org.rnorth.ducttape.timeouts.Timeouts.callFuture(Timeouts.java:68)
at org.rnorth.ducttape.timeouts.Timeouts.doWithTimeout(Timeouts.java:60)
at org.testcontainers.containers.wait.strategy.WaitAllStrategy.waitUntilReady(WaitAllStrategy.java:54)
at io.quarkus.devservices.deployment.compose.ComposeProject.waitUntilReady(ComposeProject.java:236)
at io.quarkus.devservices.deployment.compose.ComposeProject.lambda$waitOnThread$7(ComposeProject.java:226)
at java.base/java.util.concurrent.CompletableFuture$AsyncRun.run(CompletableFuture.java:1804)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's odd. Those config aren't that related. What are your steps to reproduce it?
If compose has an error starting services, like container created but not started etc. It leaves services in an intermediate state. I think we should capture errors on running compose up and run a compose down immediately and propagate the error so when the problem gets fixed we have the expected running state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a ran tests that started the compose services, then the containers stayed on because i disabled ryuk.
And after a while one of the container stopped and then the tests threw this error at each run until i removed the stopped container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally dev services do compose down
on shutdown. But during development, between changing the compose files and app restarts, things can get ugly and it can be in a state where compose up has run but didn't get registered for shutdown cleanup. ryuk is there to clean that up.
If a service container has stopped, the dev service is looking to validate that the container is healthy by checking mapped ports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a reproducer here, https://github.com/Malandril/reproduce-compose you can run the ./reproduce-error.sh . If the listChildContainer is patched to only list running containers, the tests works and the service is started correctly.
diff --git i/extensions/devservices/deployment/src/main/java/io/quarkus/devservices/deployment/compose/ComposeProject.java w/extensions/devservices/deployment/src/main/java/io/quarkus/devservices/deployment/compose/ComposeProject.java
index 723cb3069b5..c6f14d45c78 100644
--- i/extensions/devservices/deployment/src/main/java/io/quarkus/devservices/deployment/compose/ComposeProject.java
+++ w/extensions/devservices/deployment/src/main/java/io/quarkus/devservices/deployment/compose/ComposeProject.java
@@ -299,6 +299,7 @@ private List<Container> listChildContainers() {
return dockerClient
.listContainersCmd()
.withLabelFilter(Map.of(DOCKER_COMPOSE_PROJECT, project))
+ .withStatusFilter(List.of("running"))
.withShowAll(true)
.exec();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the reproducer!
It may be interesting to capture running
and restarting
containers at this stage.
I remember testing without showAll(true)
, and it didn't work at some point, but I don't remember why and I changed quiet a few things since.
5c34b6b
to
1d59196
Compare
}) | ||
.or(() -> ComposeLocator.locateContainer(composeProjectBuildItem, | ||
List.of(capturedDevServicesConfiguration.imageName()), LaunchMode.current()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ozangunalp so no need for additional label / tag? Image name is enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If observability service can be anything, it is better to specify the image-name directly or use the container locator label.
This will also need a rebase |
1d59196
to
722062c
Compare
Rebased and reviewed the doc. I will squash once it's good to go. |
This comment has been minimized.
This comment has been minimized.
.map(r -> { | ||
Map<String, String> cfg = new LinkedHashMap<>(); | ||
for (ContainerInfo.ContainerPort port : r.containerInfo().exposedPorts()) { | ||
cfg.putAll(dev.config(port.privatePort(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alesj could we use these properties to setup the extensions instead of relying on the internal system properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brunobat I think we can make adjustments in later iterations..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brunobat wdym? On which internal system properties
do we rely on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the grafana.endpoint
and otel-collector.url
. I wonder if we can rid of them by using the container info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense
This comment has been minimized.
This comment has been minimized.
- Fix amqp username prop - Introduced DevServicesNetworkIdBuildItem to set the dev services network id to the startup action, Regular dev services not reuse the compose default network when available
722062c
to
74bbe72
Compare
@Malandril thank you for testing this feature early. |
Status for workflow
|
Status for workflow
|
The Compose Dev Service discovers Compose files in a Quarkus project and starts services defined in the Compose project, using Docker Compose or Podman Compose.
Detailed feature docs in
compose-dev-services.adoc
.Extension dev services build steps can consume the
DevServicesComposeProjectBuildItem
and can locate whether compose has created an eligible service that they can configure the Quarkus app for, much like how dev service instances shared between projects.This change already integrates following platform dev services: Datasource Dev Services (DB2, MariaDB, MS SQL, MySQL, Oracle, Postgres), Keycloak, Elasticsearch, Infinispan, Kafka, Kubernetes, AMQP, MQTT, Pulsar, RabbitMQ, MongoDB, Observability, Redis, Apicurio Schema Registry.
For any not-yet-integrated service, it is possible to include description in the compose files to produce runtime configuration for the Quarkus application in dev/test.
The dev service hashes targeted compose files in order to detect changes in the content and restart compose.
Dev UI Devservices pane shows a brief description of the Compose Dev Services.
If a Quarkus project doesn't contain Compose files, it can still discover Compose services using a configuration, effectively sharing a single compose project between multiple projects.