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.

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

Ensure that quarkus-kubernetes does not pollute the runtime classpath #15716

Merged
merged 2 commits into from
Mar 15, 2021
Merged

Ensure that quarkus-kubernetes does not pollute the runtime classpath #15716

merged 2 commits into from
Mar 15, 2021

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Mar 15, 2021

Fixes: #15705

@geoand geoand requested a review from aloubyansky March 15, 2021 08:26
@geoand geoand mentioned this pull request Mar 15, 2021
Closed
@aloubyansky
Copy link
Member

Just to clarify, quarkus-kubernetes-client will be the only one providing io.fabrik8:kubernetes-client, the rest of the extensions depending on the quarkus-kubernetes-client-internal will not. Is this how it is intended to be?

@geoand
Copy link
Contributor Author

geoand commented Mar 15, 2021

Just to clarify, quarkus-kubernetes-client will be the only one providing io.fabrik8:kubernetes-client, the rest of the extensions depending on the quarkus-kubernetes-client-internal will not. Is this how it is intended to be?

Yes, exactly

@geoand
Copy link
Contributor Author

geoand commented Mar 15, 2021

Actually, I still need to try something

@geoand
Copy link
Contributor Author

geoand commented Mar 15, 2021

Tried what I had in mind and it works just fine

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Mar 15, 2021
@geoand
Copy link
Contributor Author

geoand commented Mar 15, 2021

Some tests are failing :(

@aloubyansky
Copy link
Member

I guess it's a compile dependency which isn't then provided at runtime. Not isolated enough.

@geoand
Copy link
Contributor Author

geoand commented Mar 15, 2021

I thought that adding

        <dependency>
            <groupId>io.fabric8</groupId>
            <artifactId>kubernetes-client</artifactId>
        </dependency>

to quarkus-kubernetes-client-deployment-internal would be enough - but it's still failing with:

Caused by: java.lang.NoClassDefFoundError: io/fabric8/kubernetes/client/KubernetesClient
        at io.quarkus.kubernetes.client.deployment.KubernetesClientBuildStep.process(KubernetesClientBuildStep.java:16)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)

and I don't see why...

@aloubyansky
Copy link
Member

Because it appears as a transitive dependency. Only direct provided dependencies (of the app or test) are supposed to be collected. If it has to be present at build time then it should just be a build-time (compile) dependency. Build time-only dependencies should be used with caution though, to make sure they are actually not required at runtime.

@geoand
Copy link
Contributor Author

geoand commented Mar 15, 2021

I am sure I am missing something from what you described, but I don't really know how to solve this case TBH...

If there is some way I can include io.fabric8:kubernetes-client in the quarkus-kubernetes-client-deployment-internal (therefore build-time) module, then everything should be OK, as the client will be present at build time when it's needed. At runtime it won't be present, unless the quarkus-kubernetes-client extensions brings it in.

@aloubyansky
Copy link
Member

You haven't actually added that dependency to the quarkus-kubernetes-client-deployment-internal. So, if i got you right, just add a direct compile dependency on io.fabric8:kubernetes-client in the quarkus-kubernetes-client-deployment-internal.

@geoand
Copy link
Contributor Author

geoand commented Mar 15, 2021 •
edited

You haven't actually added that dependency to the quarkus-kubernetes-client-deployment-internal. So, if i got you right, just add a direct compile dependency on io.fabric8:kubernetes-client in the quarkus-kubernetes-client-deployment-internal.

That is what I am trying locally and unfortunately I am seeing the same error.

@aloubyansky
Copy link
Member

I'll have a look into this a bit later today.

@geoand
Copy link
Contributor Author

geoand commented Mar 15, 2021

I pushed what I am doing locally as well.

@geoand
Copy link
Contributor Author

geoand commented Mar 15, 2021

I'll have a look into this a bit later today.

I'm looking as well to try and understand what is going on.

@aloubyansky
Copy link
Member

Is farbik8 client necessary to build the internal runtime module?

@geoand
Copy link
Contributor Author

geoand commented Mar 15, 2021

Is farbik8 client necessary to build the internal runtime module?

Yes, unfortunately it is

@geoand
Copy link
Contributor Author

geoand commented Mar 15, 2021

Looking into it a little more closely, it seems that this is caused by the fact that io.quarkus.kubernetes.client.runtime.KubernetesClientUtils is being loaded by the System ClassLoader.

@aloubyansky
Copy link
Member

Doesn't it mean that the fabrik8 client is actually required at runtime? At least from the classloading perspective?

@geoand
Copy link
Contributor Author

geoand commented Mar 15, 2021

Doesn't it mean that the fabrik8 client is actually required at runtime? At least from the classloading perspective?

From a classloader perspective, yes.

However the problem is this:

When I simple do mvn package on a project tha contains the quarkus-kubernetes, everything works as expected (io.quarkus.kubernetes.client.runtime.KubernetesClientUtils is loaded from the augmentation ClassLoader.)
The problem I mentioned above only seems to happen with the QuarkusProdModeTest, so I am really not sure what to make of it...

@geoand
Copy link
Contributor Author

geoand commented Mar 15, 2021

There is probably a horrible hack I can do to make it work, let me check

@geoand
Copy link
Contributor Author

geoand commented Mar 15, 2021

There is probably a horrible hack I can do to make it work, let me check

It didn't work.

I am really baffled by this because the only place this fails is in the QuarkusProdModeTest...

@geoand
Copy link
Contributor Author

geoand commented Mar 15, 2021 •
edited

I am really baffled by this because the only place this fails is in the QuarkusProdModeTest...

I think I know why this is happening...

@geoand
Make QuarkusProdModeTest use isolated augmentation classloader
f81baf9 
This is done to align the Quarkus prod mode test with the regular
build and is actually necessary to make the overcome classloading
issues that only with the QuarkusProdModeTest and the
kubernetes-extension
@geoand
Copy link
Contributor Author

geoand commented Mar 15, 2021

It should now be OK

@geoand
Copy link
Contributor Author

geoand commented Mar 15, 2021

I also added a test to make sure this does not happen again

@aloubyansky
Copy link
Member

Great @geoand, thanks!

@geoand
Copy link
Contributor Author

geoand commented Mar 15, 2021

👍🏼

@geoand geoand merged commit e09fe0f into quarkusio:main Mar 15, 2021
@quarkus-bot quarkus-bot bot added this to the 1.13 - main milestone Mar 15, 2021
@geoand geoand deleted the #15705 branch March 15, 2021 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aloubyansky aloubyansky aloubyansky approved these changes

Assignees
No one assigned
Labels
area/kubernetes area/testing triage/waiting-for-ci Ready to merge when CI successfully finishes
Projects
None yet
Milestone
1.13.0.CR1
Development

Successfully merging this pull request may close these issues.

quarkus-kubernetes / quarkus-openshift extension pulls in additional 10+ MB of dependencies
2 participants
@geoand @aloubyansky