Fix randomly failures when deploying to OpenShift/K8s on JVM/Native #31503

Sgitario · 2023-03-01T10:19:42Z

Finally, I've found out the root cause of this issue. There were several places where a new KubernetesClient was being created:

Some places use Clients.fromConfig.
Other places use KubernetesClientUtils.createConfig.
And other places use KubernetesClientBuildItem.getClient

And the problem is that every time we invoke any of these methods, Fabric8 KubernetesClient tries to locate one HttpClient.Factory and it seems that the logic to get one HttpClient.Factory sometimes gets the VertxHttpClientFactory implementation over the expected one QuarkusHttpClientFactory.

The previous solution about keeping the Disable Http Dns system property worked fine because this property was used either by VertxHttpClientFactory or QuarkusHttpClientFactory.

However, I've updated the pull request with a better solution and more efficient one as it avoids finding a HttpClient.Factory via the ServiceLoader logic in Fabric8 Kubernetes Client, but instead it directly provides the expected QuarkusHttpClientFactory implementation always.

Also, the warning message that always appeared, it's now gone:

[WARNING] [io.netty.resolver.dns.DefaultDnsServerAddressStreamProvider] Default DNS servers: [/[2001:4860:4860:0:0:0:0:8888]:53, /[2001:4860:4860:0:0:0:0:8844]:53] (Google Public DNS as a fallback)
[WARNING] There are multiple httpclient implementation in the classpath, choosing the first non-default implementation. You should exclude dependencies that aren't needed or use an explicit association of the HttpClient.Factory.

Fix #31476

Sgitario · 2023-03-01T10:21:04Z

@yrodiere with these changes, it seems to be consistently working for me. Could you try it yourself too just to be sure?

geoand · 2023-03-01T10:25:32Z

Can you please explain this change. I don't understand it

Sgitario · 2023-03-01T10:29:51Z

Can you please explain this change. I don't understand it

This is based on my findings here: #31476 (comment)

yrodiere

I can't easily test with a Quarkus snapshot as I've only ever deployed from CI, but @gsmet is giving a try with another app that he usually deploys from his laptop, so let's wait for that.

As to the solution, I'm not sure I understand the content, but I added a comment about the form :)

...netes/vanilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/KnativeDeployer.java

...tes/vanilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/OpenshiftDeployer.java

geoand · 2023-03-01T10:31:33Z

Can you please explain this change. I don't understand it

This is based on my findings here: #31476 (comment)

I'd like a more complete explanation please. Both to help me understand why this change fixes the problem and to have an easily accessible record of why this change was made.

Sgitario · 2023-03-01T12:17:38Z

Can you please explain this change. I don't understand it

This is based on my findings here: #31476 (comment)

I'd like a more complete explanation please. Both to help me understand why this change fixes the problem and to have an easily accessible record of why this change was made.

My guess was that the system property was cleared up and a new vert.x instance was created with the wrong value. Anyway, @gsmet is getting a different error after trying these changes. So, I'm changing this pull request to draft and keep working on it.

geoand · 2023-03-01T12:33:39Z

So, I'm changing this pull request to draft and keep working on it.

👍🏼.

Let me know if you need me to look into anything

Sgitario · 2023-03-02T05:45:16Z

@geoand I've just updated the pull request, more information about the changes is in the description.
@yrodiere I've tried these changes using the GitHub lottery repo and it seems to be consistently working fine either in JVM and Native.

Sgitario · 2023-03-02T05:57:28Z

@gsmet I've also tried to deploy the (quarkus-github-bot)[https://github.com/quarkusio/quarkus-github-bot] repo into OpenShift and it's working fine too (no warnings messages are printed).

@gsmet @yrodiere I would like to try these changes yourself too just to be surer that all the issues are gone.

Sgitario · 2023-03-02T07:48:54Z

there is a regression with these changes that cause an out-of-memory error. I can reproduce it locally, investigating.

geoand · 2023-03-02T07:54:15Z

That almost certainly means that there are multiple Vert.x instances that have not been closed

Sgitario · 2023-03-02T08:56:50Z

It should be fixed with the latest changes.

geoand · 2023-03-02T09:01:20Z

...nternal/src/main/java/io/quarkus/kubernetes/client/deployment/KubernetesClientBuildStep.java

@@ -12,7 +14,9 @@ public class KubernetesClientBuildStep {
    private KubernetesClientBuildConfig buildConfig;

    @BuildStep
-    public KubernetesClientBuildItem process(TlsConfig tlsConfig) {
-        return new KubernetesClientBuildItem(createConfig(buildConfig, tlsConfig));
+    public KubernetesClientBuildItem process(TlsConfig tlsConfig, QuarkusBuildCloseablesBuildItem closeablesBuildItem) {


TIL about QuarkusBuildCloseablesBuildItem :)

gsmet · 2023-03-02T11:24:17Z

I will test it with the Quarkus Bot and the production cluster this afternoon.

gsmet · 2023-03-02T11:43:49Z

So... I don't think it's the same issue but I still have an error when pushing the Quarkus bot to the production cluster (error which apparently doesn't prevent the deployment as I have a success in the end):

[INFO] [io.quarkus.container.image.openshift.deployment.OpenshiftProcessor] Applied: ImageStream ubi-quarkus-native-binary-s2i
[INFO] [io.quarkus.container.image.openshift.deployment.OpenshiftProcessor] Applied: ImageStream quarkus-bot
[INFO] [io.quarkus.container.image.openshift.deployment.OpenshiftProcessor] Applied: BuildConfig quarkus-bot
[ERROR] Failed to upload archive file for the build: quarkus-bot
[ERROR] Please check cluster events via `oc get events` to see what could have possibly gone wrong
[WARNING] [io.quarkus.container.image.openshift.deployment.OpenshiftProcessor] An exception: 'Can't instantiate binary build, due to error reading/writing stream. Can be caused if the output stream was closed by the server.See if something's wrong in recent events in Cluster = Scheduled quarkus-bot-1-build.17489735390326d6 Successfully assigned prod-quarkus-bot/quarkus-bot-1-build to ip-10-0-142-232.us-west-2.compute.internal by ip-10-0-182-138
Created quarkus-bot-1-build.17489735b5f74d2d Created container git-clone
Started quarkus-bot-1-build.17489735b86fbe3a Started container git-clone
BuildStarted quarkus-bot-1.17489735e85618ad Build prod-quarkus-bot/quarkus-bot-1 is now running
AddedInterface quarkus-bot-1-build.17489735a33f5430 Add eth0 [10.128.3.49/23] from openshift-sdn

Talking about: [ERROR] Failed to upload archive file for the build: quarkus-bot.

Sgitario · 2023-03-02T11:46:12Z

So... I don't think it's the same issue but I still have an error when pushing the Quarkus bot to the production cluster (error which apparently doesn't prevent the deployment as I have a success in the end):

[INFO] [io.quarkus.container.image.openshift.deployment.OpenshiftProcessor] Applied: ImageStream ubi-quarkus-native-binary-s2i
[INFO] [io.quarkus.container.image.openshift.deployment.OpenshiftProcessor] Applied: ImageStream quarkus-bot
[INFO] [io.quarkus.container.image.openshift.deployment.OpenshiftProcessor] Applied: BuildConfig quarkus-bot
[ERROR] Failed to upload archive file for the build: quarkus-bot
[ERROR] Please check cluster events via `oc get events` to see what could have possibly gone wrong
[WARNING] [io.quarkus.container.image.openshift.deployment.OpenshiftProcessor] An exception: 'Can't instantiate binary build, due to error reading/writing stream. Can be caused if the output stream was closed by the server.See if something's wrong in recent events in Cluster = Scheduled quarkus-bot-1-build.17489735390326d6 Successfully assigned prod-quarkus-bot/quarkus-bot-1-build to ip-10-0-142-232.us-west-2.compute.internal by ip-10-0-182-138
Created quarkus-bot-1-build.17489735b5f74d2d Created container git-clone
Started quarkus-bot-1-build.17489735b86fbe3a Started container git-clone
BuildStarted quarkus-bot-1.17489735e85618ad Build prod-quarkus-bot/quarkus-bot-1 is now running
AddedInterface quarkus-bot-1-build.17489735a33f5430 Add eth0 [10.128.3.49/23] from openshift-sdn

Talking about: [ERROR] Failed to upload archive file for the build: quarkus-bot.

I've also seen this issue even without this change. My guess is that the client is closing the connection after some time. How long did the build take for you? I would like to address this issue as part of this pull request as well, so I'll investigate it.

gsmet · 2023-03-02T11:49:26Z

The build is a native one so it's a bit long, something like 2-3 minutes.

Sgitario · 2023-03-02T13:58:52Z

After having tried up to 10 times a Native build to be deployed into OpenShift, I could reproduce the latter issue only once, and seems to be caused because OpenShift rejects the compressed file:

error: unable to extract binary build input, must be a zip, tar, or gzipped tar, or specified as a file: exit status 1

And I think this is caused by Files.move(see here). It moves files but depending on your environment, they might not be fully ready yet and hence OpenShift can't read it. Adding the ATOMIC_MOVE flag should address this issue.

With this change, I've tried the deployment to OpenShift and I could not reproduce the issue any longer.

@gsmet can you try again?

gsmet · 2023-03-02T14:03:02Z

Sure. I’m at the doctor right now but will have a look when I’m back.

The System.out.println should probably go away though :)

Sgitario · 2023-03-03T06:33:42Z

And the problem is that every time we invoke any of these methods, Fabric8 KubernetesClient tries to locate one HttpClient.Factory and it seems that the logic to get one HttpClient.Factory sometimes gets the VertxHttpClientFactory implementation over the expected one QuarkusHttpClientFactory.

Not really, the logic I mentioned is this one (Note that there is an extra parenthesis in the third condition, but I'm not sure if this was intentional, however this was not the issue). Maybe, the root problem is how to load the factories here.

Anyway, I thought it was better to not call this logic at all to be more efficient and directly use the factory we want at build time.

This is kind of messed up, have you identified the root cause?

From the changes, I understand that the explicit HttpClient Factory is only used at build time, The runtime Bean will continue to be created by using the SPI-provided factory, right?

Yes, this is meant to be used only at build time. At runtime, the KubernetesClient instance will still be produced by https://github.com/quarkusio/quarkus/blob/main/extensions/kubernetes-client/runtime/src/main/java/io/quarkus/kubernetes/client/runtime/KubernetesClientProducer.java#L21.

There were several places where a new KubernetesClient was being created: - Some places use `Clients.fromConfig`. - Other places use `KubernetesClientUtils.createConfig`. - And other places use `KubernetesClientBuildItem.getClient` And the problem is that every time we invoke any of these methods, Fabric8 KubernetesClient tries to locate one HttpClient.Factory and it seems that the logic to get one HttpClient.Factory sometimes gets the `VertxHttpClientFactory` implementation over the expected one `QuarkusHttpClientFactory` With this solution, it avoids to find a HttpClient.Factory using the ServiceLoader logic in Fabric8 Kubernetes Client, but it provides the expected `QuarkusHttpClientFactory` implementation always. Fix quarkusio#31476

Sgitario · 2023-03-03T07:17:59Z

PR updated because I forgot to remove the file which after these changes is no longer needed: https://github.com/quarkusio/quarkus/pull/31503/files#diff-19ff5f9f5bb70cfddc9f0388d562c8b9ef15e74ead765a18f2ca00c4b99b1faa

quarkus-bot · 2023-03-03T08:46:09Z

✔️ 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.

yrodiere · 2023-03-03T09:19:27Z

Merged, thanks!

gsmet · 2023-03-03T09:40:42Z

Yes, this is meant to be used only at build time. At runtime, the KubernetesClient instance will still be produced by

@Sgitario but in this case, don't we have the same problem of having the two factories to compete while we want the Quarkus one to be used at runtime?

Sgitario · 2023-03-03T09:53:50Z

Yes, this is meant to be used only at build time. At runtime, the KubernetesClient instance will still be produced by

@Sgitario but in this case, don't we have the same problem of having the two factories to compete while we want the Quarkus one to be used at runtime?

If this was not working at runtime either, then we should amend the producer to also use our custom HttpClient.Factory. Can you clarify @manusa or @geoand ?

manusa · 2023-03-03T09:54:07Z

Note that there is an extra parenthesis in the third condition, but I'm not sure if this was intentional, however this was not the issue

🤦 OK, I think I messed up in that one. The last condition looks definitely wrong.

@Sgitario but in this case, don't we have the same problem of having the two factories to compete while we want the Quarkus one to be used at runtime?

One of the topics I discussed with @geoand while implementing the Quarkus factory was still providing the option for users to override the runtime HttpClient implementation. That's why I was specifically asking this. The Factory scoring system logic in the client impl needs to be fixed, then everything should work as expected.

manusa · 2023-03-03T09:54:54Z

Sending the fix to the Client now.

Sgitario · 2023-03-03T09:57:12Z

So, for runtime what should be the right behavior? Use the QuarkusHttpClientFactory impl by default? If used, I have to include back the service binding file.

manusa · 2023-03-03T09:58:53Z

So, for runtime what should be the right behavior? Use the QuarkusHttpClientFactory impl by default? If used, I have to include back the service binding file.

As I see it:
The QuarkusHttpClientFactory should score one point higher than the Vert.x factory provided by the client.
If a user wants to provide their own factory, it should score higher than any other factory.
If the scoring system works as expected (and there aren't classloader issues), then the highest scoring non-default factory should be selected.

Sgitario · 2023-03-03T10:00:07Z

So, for runtime what should be the right behavior? Use the QuarkusHttpClientFactory impl by default?

If so, I'm not sure if I should include back the service binding file.
But not sure it was needed it at all because we were already registering the service on this line. I would need to test this.

manusa · 2023-03-03T10:19:16Z

But not sure it was needed it at all because we were already registering the service on this line. I would need to test this.

AFAIU you need both. The file is used to register the SPI class. The second is used by Quarkus to make use of the SPI providers.

gsmet · 2023-03-03T10:26:38Z

@Sgitario and I think you probably need to exclude the service binding file coming from the Kubernetes Client Vert.x jar to make sure the Quarkus one is the only one seen at runtime.

gsmet · 2023-03-03T10:27:18Z

(by using RemovedResourceBuildItem to remove the service file from the jar)

Sgitario · 2023-03-03T11:28:39Z

I've been playing with the quarkus-kubernetes-client extension and:

The original issue io.quarkus.kubernetes.deployment.OpenshiftDeployer throws java.io.IOException: Failed to resolve <host> but evidently did reach the host #31476 does not affect the KubernetesClient instance at runtime because the Vert.x extension is already disabling the DNS verification here. (I tried to reproduce the issue replicating what the kubernetes extension was doing and I did check that this flag was being enabled by the vertx extension).
As @manusa said, as I removed the service file, the QuarkusHttpClientFactory implementation is not found at runtime, but the VertxHttpClientFactory impl, However, VertxHttpClientFactory works already fine because of the previous point.

Therefore, I think we're ok with using the VertxHttpClientFactory impl at runtime and only QuarkusHttpClientFactory at build time. So we don't need more changes. Wdyt?

Note that I'm seeing the following warning when using the Kubernetes Client:

2023-03-03 12:19:48,711 WARN  [io.ver.cor.imp.VertxImpl] (executor-thread-0) You're already on a Vert.x context, are you sure you want to create a new Vertx instance?

But this is unrelated to my changes.

manusa · 2023-03-03T11:35:24Z

Therefore, I think we're ok with using the VertxHttpClientFactory impl at runtime and only QuarkusHttpClientFactory at build time. So we don't need more changes. Wdyt?

Besides disabling the DNS verification, the purpose of the factory is to reuse the Vertx object instance at runtime which is why it was originally added.

quarkus/extensions/kubernetes-client/runtime-internal/src/main/java/io/quarkus/kubernetes/client/runtime/QuarkusHttpClientFactory.java

Line 24 in f14cb40

this.vertx = CDI.current().select(Vertx.class).get();

Which probably relates with the warning you mention

Sgitario · 2023-03-03T11:47:21Z

I see. So, if we want to let users to overwrite it via ServiceLoader and maintain QuarkusHttpClientFactory, we definitely need to exclude the VertxHttpClientFactory impl using RemovedResourceBuildItem as @gsmet .
Let me prepare a pull request with this.

gsmet · 2023-03-03T12:20:11Z

Thanks @Sgitario and @manusa !

Sgitario · 2023-03-03T13:31:09Z

Fix: #31582

Relates quarkusio#31503 (comment)

Sgitario requested a review from geoand March 1, 2023 10:19

quarkus-bot bot added the area/kubernetes label Mar 1, 2023

Sgitario requested a review from yrodiere March 1, 2023 10:20

yrodiere reviewed Mar 1, 2023

View reviewed changes

...netes/vanilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/KnativeDeployer.java Outdated Show resolved Hide resolved

...tes/vanilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/OpenshiftDeployer.java Outdated Show resolved Hide resolved

Sgitario marked this pull request as draft March 1, 2023 12:17

Sgitario force-pushed the 31476 branch from c19050a to 4cf11a7 Compare March 2, 2023 05:40

Sgitario marked this pull request as ready for review March 2, 2023 05:41

Sgitario requested a review from yrodiere March 2, 2023 05:44

geoand approved these changes Mar 2, 2023

View reviewed changes

Sgitario force-pushed the 31476 branch from 4cf11a7 to 0c3c979 Compare March 2, 2023 08:56

geoand reviewed Mar 2, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

Sgitario force-pushed the 31476 branch from 0c3c979 to 401fc01 Compare March 2, 2023 13:50

This comment has been minimized.

Sign in to view

Sgitario force-pushed the 31476 branch from 0292525 to 928266d Compare March 3, 2023 07:17

yrodiere merged commit 4f9a65e into quarkusio:main Mar 3, 2023

quarkus-bot bot added kind/bugfix and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Mar 3, 2023

quarkus-bot bot added this to the 3.0 - main milestone Mar 3, 2023

Sgitario deleted the 31476 branch March 3, 2023 09:20

manusa mentioned this pull request Mar 3, 2023

fix: typo in HttpClient.Factory scoring system logic fabric8io/kubernetes-client#4947

Merged

11 tasks

Sgitario mentioned this pull request Mar 3, 2023

Exclude VertxHttpClientFactory in favour of QuarkusHttpClientFactory #31582

Merged

Sgitario added a commit to Sgitario/quarkus that referenced this pull request Mar 3, 2023

Exclude VertxHttpClientFactory in favour of QuarkusHttpClientFactory

34264c2

Relates quarkusio#31503 (comment)

Fix randomly failures when deploying to OpenShift/K8s on JVM/Native #31503

Fix randomly failures when deploying to OpenShift/K8s on JVM/Native #31503

Conversation

Sgitario commented Mar 1, 2023 • edited Loading

Sgitario commented Mar 1, 2023

geoand commented Mar 1, 2023

Sgitario commented Mar 1, 2023

yrodiere left a comment

Choose a reason for hiding this comment

geoand commented Mar 1, 2023

Sgitario commented Mar 1, 2023

geoand commented Mar 1, 2023

Sgitario commented Mar 2, 2023

Sgitario commented Mar 2, 2023

Sgitario commented Mar 2, 2023

geoand commented Mar 2, 2023 • edited Loading

Sgitario commented Mar 2, 2023

geoand Mar 2, 2023

Choose a reason for hiding this comment

This comment has been minimized.

gsmet commented Mar 2, 2023

gsmet commented Mar 2, 2023

Sgitario commented Mar 2, 2023

gsmet commented Mar 2, 2023

Sgitario commented Mar 2, 2023

gsmet commented Mar 2, 2023

This comment has been minimized.

Sgitario commented Mar 3, 2023

Sgitario commented Mar 3, 2023

quarkus-bot bot commented Mar 3, 2023

yrodiere commented Mar 3, 2023

gsmet commented Mar 3, 2023 • edited Loading

Sgitario commented Mar 3, 2023

manusa commented Mar 3, 2023

manusa commented Mar 3, 2023

Sgitario commented Mar 3, 2023

manusa commented Mar 3, 2023

Sgitario commented Mar 3, 2023

manusa commented Mar 3, 2023

gsmet commented Mar 3, 2023

gsmet commented Mar 3, 2023

Sgitario commented Mar 3, 2023 • edited Loading

manusa commented Mar 3, 2023 • edited Loading

Sgitario commented Mar 3, 2023

gsmet commented Mar 3, 2023

Sgitario commented Mar 3, 2023

Sgitario commented Mar 1, 2023 •

edited

Loading

geoand commented Mar 2, 2023 •

edited

Loading

gsmet commented Mar 3, 2023 •

edited

Loading

Sgitario commented Mar 3, 2023 •

edited

Loading

manusa commented Mar 3, 2023 •

edited

Loading