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 usage of random port for the Management Interface #39588

Merged
merged 1 commit into from
Mar 22, 2024

Conversation

cescoffier
Copy link
Member

@cescoffier cescoffier commented Mar 20, 2024

Setting the management interface port to a random value in testing scenarios can be beneficial. This can be achieved by configuring quarkus.management.test-port to 0. However, retrieving the actual port in tests was not feasible previously.

This PR addresses this limitation by introducing the following enhancements:

  • It stores the actual management port in a system property when it differs from the configured port.
  • It enables injecting the actual management port using @TestHTTPResource(management=true,...).

This comment has been minimized.

Copy link

github-actions bot commented Mar 20, 2024

🙈 The PR is closed and the preview is expired.


@Test
public void test() {
Assertions.assertNotEquals(url.getPort(), management.getPort());
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 ;)

@@ -644,9 +665,21 @@ private static CompletableFuture<HttpServer> initializeManagementInterface(Vertx
}

actualManagementPort = ar.result().actualPort();
if (actualManagementPort != httpManagementServerOptions.getPort()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wonder, is this enough from preventing vertx to round-robin?
(Only see this managing system properties - so I'm a bit confused)

Copy link
Member Author

Choose a reason for hiding this comment

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

It fixes how it is retrieved and invoked (the system property is used to inject the URL in the test). I was not able to get the same port for both the primary and management ports. I repeated the tests many times without luck.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me try it

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm.

[stdout] 2024-03-21 10:43:54,019 INFO  [io.quarkus] (main) nessie-quarkus 0.79.1-SNAPSHOT on JVM (powered by Quarkus 999-SNAPSHOT) started in 1.830s. Listening on: http://0.0.0.0:44735. Management interface listening on http://0.0.0.0:44735.

Copy link
Contributor

@snazy snazy Mar 21, 2024

Choose a reason for hiding this comment

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

Repro:

  • Clone from this repo: git clone -o snazy -b quarkus-management-http-JAVA_HTTP_QUARKUS_DEBUG https://github.com/snazy/nessie.git (hope I got the git syntax right)
  • Build Quarkus (./mvnw -Dquickly -T1C)
  • Build and start: ./gradlew :nessie-quarkus:quarkusBuild && java -Dquarkus.http.port=0 -Dquarkus.management.port=0 -jar servers/quarkus-server/build/quarkus-app/quarkus-run.jar

(EDIT: Removed the intTest example - requires changes to the source code for the random port)

Copy link
Contributor

@snazy snazy Mar 21, 2024

Choose a reason for hiding this comment

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

Oh - the round-robin behavior is reproducible w/ curl:

curl http://127.0.0.1:39135/api/v1/config

Good:

$ curl http://127.0.0.1:39135/api/v1/config
{
  "defaultBranch" : "main",
  "maxSupportedApiVersion" : 2
}

Bad:

$ curl http://127.0.0.1:39135/api/v1/config
<html><body><h1>Resource not found</h1></body></html>

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!
It's weird that you end up with the same port. I believe it's a timing issue (like your machine is super fast and so the 2 random are generated exactly at the same time).

This comment has been minimized.

@snazy
Copy link
Contributor

snazy commented Mar 21, 2024

Looks like the "magic values" -1 & -2 are the issue here.
Opened a PR against your branch @cescoffier
With that change, it reports:

2024-03-21 11:07:11,775 INFO  [io.quarkus] (main) nessie-quarkus 0.79.1-SNAPSHOT on JVM (powered by Quarkus 999-SNAPSHOT) started in 1.829s. Listening on: http://0.0.0.0:34035. Management interface listening on http://0.0.0.0:40375.

:)

This comment has been minimized.

This comment has been minimized.

@cescoffier cescoffier force-pushed the management-random-port branch from 270e060 to 4cc7eea Compare March 21, 2024 12:57
@cescoffier
Copy link
Member Author

@snazy alright! I've integrated your commit and did a few cosmetic fixes.

This comment has been minimized.

This comment has been minimized.

/**
* When the management port is set to 0, replace it by this value to let Vert.x choose a random port
*/
public static final int RANDOM_PORT_MANAGEMENT = -3;
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we need one for managment/TLS?

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, management has only one port. Unlike the primary HTTP server which can serve both HTTP and HTTPS at the same time (using 2 ports), it's not possible with the management interface.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, okay.

Copy link
Contributor

@snazy snazy left a comment

Choose a reason for hiding this comment

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

LGTM

@cescoffier
Copy link
Member Author

super weird compilation issue - as it compiles locally (using java 17 and 21, doing clean too)

@cescoffier
Copy link
Member Author

@geoand @gsmet, did you already see that kind of thing?

@geoand
Copy link
Contributor

geoand commented Mar 21, 2024

I have not encountered it...

@cescoffier
Copy link
Member Author

ok, it's actually some kind of conflict... I don't like when the main HTTP recorder gets modified without me reviewing it.

@geoand
Copy link
Contributor

geoand commented Mar 21, 2024

Which PR made a change?

In testing scenarios, setting the management interface port to a random value can be beneficial. This can be achieved by configuring `quarkus.management.test-port` to `0`. However, previously, retrieving the actual port in tests was not feasible.

This commit addresses this limitation by introducing the following enhancements:

- It stores the actual management port in a system property when it differs from the configured port.
- It enables the injection of the actual management port using @TestHTTPResource(management=true,...).
@cescoffier cescoffier force-pushed the management-random-port branch from 4cc7eea to d0d5ef5 Compare March 21, 2024 16:22
Copy link

quarkus-bot bot commented Mar 21, 2024

Status for workflow Quarkus Documentation CI

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

✅ 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.

⚠️ There are other workflow runs running, you probably need to wait for their status before merging.

Copy link

quarkus-bot bot commented Mar 21, 2024

Status for workflow Quarkus CI

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

✅ 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.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 extensions/smallrye-reactive-messaging-kafka/deployment

io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase.sseStream - History

  • Assertion condition Expecting size of: [] to be greater than or equal to 2 but was 0 within 10 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: 
Assertion condition 
Expecting size of:
  []
to be greater than or equal to 2 but was 0 within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)

@cescoffier cescoffier merged commit 64ddfe0 into quarkusio:main Mar 22, 2024
52 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.10 - main milestone Mar 22, 2024
@quarkus-bot quarkus-bot bot added kind/bugfix kind/enhancement New feature or request labels Mar 22, 2024
@cescoffier cescoffier deleted the management-random-port branch March 26, 2024 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
3 participants