Print messages about ports that can't change at runtime for K8s #33789

Sgitario · 2023-06-02T07:48:05Z

Related to #33307, task 3.
Fix #32882

I didn't add this logic to Knative (because knative is renaming the port to http1) and Kind/Minikube because these are for DEV purposes only.

Sgitario · 2023-06-02T07:49:04Z

@iocanel @cescoffier I'm not sure if this is what you had in mind for the task 3 of #33307.
Note that I don't think this message should use the INFO level, but DEBUG.

iocanel · 2023-06-02T08:30:50Z

...anilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/KubernetesCommonHelper.java

+                    LOG.info(String.format("The target '%s' is mapping the container to the port '%s' which value is '%d'. "
+                            + "You won't be able to change it at runtime using the property '%s'.",
+                            target, port.getName(), port.getContainerPort(), portWithConfig.getValue().propertyName));


Suggested change

LOG.info(String.format("The target '%s' is mapping the container to the port '%s' which value is '%d'. "

+ "You won't be able to change it at runtime using the property '%s'.",

target, port.getName(), port.getContainerPort(), portWithConfig.getValue().propertyName));

LOG.warn(String.format("'%s' manifests are generated with container port '%s' having value '%d'. "

+ "The above is influenced by property '%s' which may change at runtime. Such change is likely to cause problems as the application and the container configuration will get out of sync.",

target, port.getName(), port.getContainerPort(), portWithConfig.getValue().propertyName));

I think the message is now too long. Also, using a warning might cause some panic among users that as far as I know, never complained about this.

let's use info instead.

iocanel

I found the message a bit misleading, so I suggested something different.

Also, since this pattern is something used in others places too, I am wondering if we should have a utility that does the config reading and logging. It would provide a more concise user experience and possibly make our lives easier years from now when we'll have forgotten all about it.

cescoffier

I like @iocanel suggestion.

cescoffier · 2023-06-04T08:31:15Z

I agree about having a utility class with a consistent, wellformed, not misleading message.

Sgitario · 2023-06-06T05:22:44Z

I'm -1 with the Ioannis' suggestion because the message is too long (imagine having the management port plus the HTTP port enabled), and I think that using the warning log level will cause some panic to users that as far as I know, never complained about this.

Sgitario · 2023-06-06T07:45:58Z

After speaking with @iocanel , we agreed with changing the message to:
[INFO] [io.quarkus.kubernetes.deployment.KubernetesDeployer] The 'kubernetes' manifests are generated with the container port 'http' having value '8080'. The app and manifests will get out of sync if the property 'quarkus.http.port' is changed at runtime..

PR updated

Sgitario

I'm -1 to include these changes as it overcomplicates the logic for very little benefit (printing messages for users that as far as I know, they never complained about it).

Though, aside of my comments, I don't want to block this change as long as @iocanel and @cescoffier are ok with proceeding.

Sgitario · 2023-06-19T11:47:27Z

extensions/kubernetes/spi/src/main/java/io/quarkus/kubernetes/spi/Property.java

+        this.runtime = runtime;
+    }
+
+    public static <T> Property<T> fromBuildTimeConfiguration(String name, Class<T> type, T defaultValue) {


Since these changes are meant to print messages for runtime properties-related only.
What I was asking was to get rid of this method (and the runtime field) and use the PortConfig class directly in the printMessages method.

Sgitario · 2023-06-19T11:48:40Z

extensions/grpc/deployment/src/main/java/io/quarkus/grpc/deployment/GrpcServerProcessor.java

-                int port = ConfigProvider.getConfig().getOptionalValue("quarkus.grpc.server.port", Integer.class)
-                        .orElse(9000);
-                return new KubernetesPortBuildItem(port, "grpc");
+                return KubernetesPortBuildItem.fromRuntimeConfiguration("https", "quarkus.grpc.server.port", 9000);


the "enabled" flag here should be based on useSeparateServer boolean, not in the presence of the property.

Sgitario · 2023-06-19T11:49:06Z

extensions/kubernetes/spi/src/main/java/io/quarkus/kubernetes/spi/KubernetesPortBuildItem.java

+        return new KubernetesPortBuildItem(port, name, enabled, Optional.of(origin));
+    }
+
+    public static KubernetesPortBuildItem fromRuntimeConfiguration(String name, String propertyName, Integer defaultValue) {


According to my previous comment, I think this method can be deleted.

Sgitario · 2023-06-19T11:51:22Z

...bernetes/vanilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/PropertyUtil.java

+            // If no kubernetes property is provided, this will be used instead.
+            String defaultOrProvided = extensionProperty.getValue().isPresent() ? "provided" : "default";
+            String stringValue = String.valueOf(extensionProperty.getValue().orElse(extensionProperty.getDefaultValue()));
+            LOG.infof("Kubernetes manifests are generated with '%s' having %s value '%s'. "


This message can be confused when using the OpenShift, Knative, etc. Kubernetes should be replaced with the target generator.

Sgitario · 2023-06-19T11:51:34Z

...bernetes/vanilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/PropertyUtil.java

+            // We have conflicting properties that need to be aligned. Maybe warn?
+            String runtimeOrBuildTime = extensionProperty.isRuntime() ? "runtime" : "buildtime";
+            LOG.debugf(
+                    "Kubernetes property '%s' has been set with value '%s' while %s property '%s' is set with '%s'. %s will be set using the former.",


Same here, Kubernetes should be replaced with the target generator.

Sgitario · 2023-06-19T11:52:25Z

...la/deployment/src/main/java/io/quarkus/kubernetes/deployment/VanillaKubernetesProcessor.java

@@ -291,8 +291,7 @@ public List<DecoratorBuildItem> createDecorators(ApplicationInfoBuildItem applic
            result.add(new DecoratorBuildItem(KUBERNETES, new RemovePortFromServiceDecorator(name, MANAGEMENT_PORT_NAME)));
        }

-        printMessageAboutPortsThatCantChange(KUBERNETES, ports, config);
-
+        printMessageAboutPortsThatCantChange(name, ports, config);


I think, name should be KUBERNETES

Sgitario · 2023-06-19T11:52:39Z

...es/vanilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/OpenshiftProcessor.java

@@ -380,8 +380,7 @@ public List<DecoratorBuildItem> createDecorators(ApplicationInfoBuildItem applic
            result.add(new DecoratorBuildItem(OPENSHIFT, new RemovePortFromServiceDecorator(name, MANAGEMENT_PORT_NAME)));
        }

-        printMessageAboutPortsThatCantChange(OPENSHIFT, ports, config);
-
+        printMessageAboutPortsThatCantChange(name, ports, config);


I think, name should be OPENSHIFT

Sgitario · 2023-06-19T11:55:15Z

extensions/kubernetes/spi/src/main/java/io/quarkus/kubernetes/spi/KubernetesPortBuildItem.java

+     * For example the presence `quarkus.http.ssl-port` also is not enought to tell us if enabled.
+     * Still, we need to communicate its value and let `quarkus-kubernetes` extension decide.
+     **/
+    private final boolean enabled;


It still confuses me to generate build items that are not enabled.
I know you explained to me that this is necessary for the case when users add quarkus.kubernetes.ports.https.tls=true, so the container port for https is resolved from here.

Sgitario · 2023-06-19T11:57:30Z

@cescoffier can you take another look to this pull request after the changes made by Ioannis please?

cescoffier · 2023-06-20T06:02:15Z

The CI issues might be related.

Related to quarkusio#33307, task 3. Fix quarkusio#32882

build items

quarkus-bot · 2023-07-25T16:53:23Z

Failing Jobs - Building `de116aa`

Status	Name	Step	Failures	Logs	Raw logs
✖	JVM Tests - JDK 11	`Build`	Failures	Logs	Raw logs
✖	JVM Tests - JDK 17	`Build`	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 19
✔️	Maven Tests - JDK 11
⌛	Maven Tests - JDK 11 Windows	`Build`	Failures	Logs	Raw logs
⌛	Native Tests - Security1	`Build`	⚠️ Check →	Logs	Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: integration-tests/grpc-hibernate

📦 integration-tests/grpc-hibernate

✖ com.example.grpc.hibernate.VertxBlockingMutinyTest.shouldAddItems - More details - Source on GitHub

io.smallrye.mutiny.CompositeException: 
Multiple exceptions caught:
	[Exception 0] io.grpc.StatusRuntimeException: INTERNAL: Half-closed without a request

⚙️ JVM Tests - JDK 17 #

- Failing: integration-tests/grpc-hibernate

📦 integration-tests/grpc-hibernate

✖ com.example.grpc.hibernate.VertxBlockingMutinyTest.shouldAddItems - More details - Source on GitHub

io.smallrye.mutiny.CompositeException: 
Multiple exceptions caught:
	[Exception 0] io.grpc.StatusRuntimeException: INTERNAL: Half-closed without a request

⚙️ Maven Tests - JDK 11 Windows #

📦 integration-tests/maven

✖ io.quarkus.maven.it.DevMojoIT.testResourcesFromClasspath line 1234 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with io.quarkus.maven.it.DevMojoIT was not fulfilled within 20 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

✖ io.quarkus.maven.it.DevMojoIT.testResourcesFromClasspath line 1234 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with io.quarkus.maven.it.DevMojoIT was not fulfilled within 20 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

iocanel

LGTM

iocanel · 2023-07-26T07:40:34Z

Don't think the failures are related.

iocanel · 2023-07-26T09:14:40Z

Don't think the failures are related.

Verified that the same tests fail in other pull requests too. So, I'll merge.

Sgitario requested review from iocanel and cescoffier June 2, 2023 07:48

quarkus-bot bot added the area/kubernetes label Jun 2, 2023

Sgitario mentioned this pull request Jun 1, 2023

Improve the logic to generate TLS-based container ports by the Quarkus Kubernetes extension #33307

Closed

4 tasks

iocanel reviewed Jun 2, 2023

View reviewed changes

iocanel requested changes Jun 2, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

cescoffier requested changes Jun 4, 2023

View reviewed changes

Sgitario force-pushed the 32882 branch from 78285bf to ce4fb43 Compare June 6, 2023 07:45

This comment has been minimized.

Sign in to view

Sgitario requested review from iocanel and cescoffier June 6, 2023 08:49

cescoffier approved these changes Jun 6, 2023

View reviewed changes

Sgitario force-pushed the 32882 branch from ce4fb43 to fed4bc4 Compare June 8, 2023 11:07

This comment has been minimized.

Sign in to view

quarkus-bot bot added area/grpc gRPC area/vertx labels Jun 19, 2023

Sgitario commented Jun 19, 2023

View reviewed changes

Sgitario requested a review from cescoffier June 19, 2023 11:57

iocanel force-pushed the 32882 branch 2 times, most recently from fc94e67 to fe95708 Compare June 19, 2023 12:57

This comment has been minimized.

Sign in to view

cescoffier approved these changes Jun 20, 2023

View reviewed changes

Print messages about ports that can't change at runtime for K8s

1da5a0b

Related to quarkusio#33307, task 3. Fix quarkusio#32882

iocanel force-pushed the 32882 branch from fe95708 to b3cbcee Compare July 12, 2023 13:06

refactor: port mgmt from kube to ext publishing the

de116aa

build items

iocanel force-pushed the 32882 branch from b3cbcee to de116aa Compare July 25, 2023 10:03

iocanel approved these changes Jul 26, 2023

View reviewed changes

iocanel merged commit c00bbdb into quarkusio:main Jul 26, 2023

quarkus-bot bot added the kind/bugfix label Jul 26, 2023

quarkus-bot bot added this to the 3.3 - main milestone Jul 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Print messages about ports that can't change at runtime for K8s #33789

Print messages about ports that can't change at runtime for K8s #33789

Sgitario commented Jun 2, 2023 •

edited

Loading

Sgitario commented Jun 2, 2023

iocanel Jun 2, 2023

Sgitario Jun 6, 2023

cescoffier Jun 6, 2023

iocanel left a comment

This comment has been minimized.

cescoffier left a comment

cescoffier commented Jun 4, 2023

Sgitario commented Jun 6, 2023

Sgitario commented Jun 6, 2023

This comment has been minimized.

This comment has been minimized.

Sgitario left a comment

Sgitario Jun 19, 2023

Sgitario Jun 19, 2023

Sgitario Jun 19, 2023

Sgitario Jun 19, 2023

Sgitario Jun 19, 2023

Sgitario Jun 19, 2023

Sgitario Jun 19, 2023

Sgitario Jun 19, 2023

Sgitario commented Jun 19, 2023

This comment has been minimized.

cescoffier commented Jun 20, 2023

quarkus-bot bot commented Jul 25, 2023

iocanel left a comment

iocanel commented Jul 26, 2023

iocanel commented Jul 26, 2023 •

edited

Loading

Print messages about ports that can't change at runtime for K8s #33789

Print messages about ports that can't change at runtime for K8s #33789

Conversation

Sgitario commented Jun 2, 2023 • edited Loading

Sgitario commented Jun 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iocanel left a comment

Choose a reason for hiding this comment

This comment has been minimized.

cescoffier left a comment

Choose a reason for hiding this comment

cescoffier commented Jun 4, 2023

Sgitario commented Jun 6, 2023

Sgitario commented Jun 6, 2023

This comment has been minimized.

This comment has been minimized.

Sgitario left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sgitario commented Jun 19, 2023

This comment has been minimized.

cescoffier commented Jun 20, 2023

quarkus-bot bot commented Jul 25, 2023

Failing Jobs - Building de116aa

Failures

⚙️ JVM Tests - JDK 11 #

📦 integration-tests/grpc-hibernate

⚙️ JVM Tests - JDK 17 #

📦 integration-tests/grpc-hibernate

⚙️ Maven Tests - JDK 11 Windows #

📦 integration-tests/maven

iocanel left a comment

Choose a reason for hiding this comment

iocanel commented Jul 26, 2023

iocanel commented Jul 26, 2023 • edited Loading

Sgitario commented Jun 2, 2023 •

edited

Loading

Failing Jobs - Building `de116aa`

iocanel commented Jul 26, 2023 •

edited

Loading