Fix Knative/OpenShift client environment check #28172

michalvavrik · 2022-09-23T11:44:51Z

According to https://github.com/fabric8io/kubernetes-client/blob/master/doc/MIGRATION-v6.md#adapt-changes, isAdaptable method semantic has changed (and the method is deprecated) and it's causing failures in Quarkus QE Test Suite. AFAIK originally isAdaptable check wasn't strict (contains, endsWith), so you could have ApiGroups like (see below) and check would succeed fabric8io/kubernetes-client@bb6115a#diff-a23a9372cb2d1a1c4121272460b2c8e2f8dd79bf5722b8b2a36ba5d4578cef75 . Now, check fails if there is no group that strictly matches APIGroup knative.dev. That's however not necessary for our app to work, f.e. our APIGroups are:

serving.knative.dev
serving.knative.dev/v1
serving.knative.dev/v1
serving.knative.dev/v1beta1
serving.knative.dev/v1alpha1
autoscaling.internal.knative.dev
autoscaling.internal.knative.dev/v1alpha1
autoscaling.internal.knative.dev/v1alpha1
caching.internal.knative.dev
caching.internal.knative.dev/v1alpha1
caching.internal.knative.dev/v1alpha1
networking.internal.knative.dev
networking.internal.knative.dev/v1alpha1
networking.internal.knative.dev/v1alpha1
operator.knative.dev
operator.knative.dev/v1alpha1
operator.knative.dev/v1alpha1

When Quarkus is not performing strict tests for knative.dev, our app is working and check performed is same as before.

Sgitario

Many many thanks for your pull request!

Spites I understand this is broken and that there might not have a better solution, I think these changes are a bit fragile (I mean checking the coordinates of the fabric8:openshift-client artifact).

About the problem itself, I don't fully follow what exactly changed in regards to the isAdaptable method. @manusa can you help here?

Also, I'm not sure if this is possible, but adding some tests would help.

Anyway, let's wait for @manusa's feedback.

michalvavrik · 2022-09-23T12:33:17Z

@Sgitario I'll try to write a test then (I've already tried, but sure, there must be a way...). As for isAdaptable, all I know is what changed fabric8io/kubernetes-client@bb6115a#diff-d3ca8c6252f8f76bcd09c8faa8b0039c49886b929de4388ed0ca05df521f01c1L37 and it does cause troubles. And this PR is what I understood a migration guide suggest. It's very much possible I misunderstood it! Let's here from expert.

michalvavrik · 2022-09-23T12:36:31Z

My understanding is that isAdaptable is now calling OC, which is surplus to requirements when just checking if we can adapt client to KnativeClient (ex. from test I wrote, not from reproducer):

...
Caused by: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.kubernetes.deployment.KnativeDeployer#checkEnvironment threw an exception: io.fabric8.kubernetes.client.KubernetesClientException: An error has occurred.
	at io.fabric8.kubernetes.client.KubernetesClientException.launderThrowable(KubernetesClientException.java:129)
	at io.fabric8.kubernetes.client.KubernetesClientException.launderThrowable(KubernetesClientException.java:122)
	at io.fabric8.kubernetes.client.dsl.internal.OperationSupport.restCall(OperationSupport.java:779)
	at io.fabric8.kubernetes.client.BaseClient.getApiGroup(BaseClient.java:242)
	at io.fabric8.kubernetes.client.BaseClient.hasApiGroup(BaseClient.java:158)
	at io.fabric8.kubernetes.client.extension.ClientAdapter.hasApiGroup(ClientAdapter.java:73)
	at io.fabric8.knative.client.DefaultKnativeClient.isSupported(DefaultKnativeClient.java:244)
	at io.fabric8.kubernetes.client.BaseClient.isAdaptable(BaseClient.java:176)
	at io.quarkus.kubernetes.deployment.KnativeDeployer.lambda$checkEnvironment$0(KnativeDeployer.java:26)
	at java.base/java.util.Optional.ifPresent(Optional.java:178)
	at io.quarkus.kubernetes.deployment.KnativeDeployer.checkEnvironment(KnativeDeployer.java:21)

...

If we could judge from ex. thrown Knative was requested as a deployment, but the target cluster is not a Knative cluster, than isAdaptable should still be called, or hasApiGroup("knative.dev", true) etc.

michalvavrik · 2022-09-23T13:40:27Z

@Sgitario I changed PR and its description. This fix should not require tests (that if you think about it are hard to do; mocks might do). I'd suggest to wait for response to @manusa and eventually close this PR if it's wrong/someone else provides better one.

manusa · 2022-09-23T14:47:19Z

Couldn't read the code changes yet, but a quick note on the isAdaptable, and in general Adapt, behavioral changes:

Adapt Changes
Client.isAdaptable and Client.adapt will check first if the existing client is an instance of the desired type.
Client.adapt will no longer perform the isAdaptable check - that is you may freely adapt from one Client to another as long as the extension exists. If you need to make a specific check of support, please use the Client.supports method.

You can read more about them at our Migration Guide.

This means that the behavior for this method has changed completely for version 6.

The isAdaptable method is deprecated and should no longer be used.

If you need to check if the cluster supports the operations that the derived client supports (in this case I guess KnativeClient or OpenShiftClient), you should use the .isSupported() method instead. (kubernetesClient.adapt(KnativeClient.class).isSupported())

In addition, you can use the .supports(Class<R> type) with any of our generated model classes, or the .hasApiGroup(String apiGroup, boolean exact) method.

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

michalvavrik · 2022-09-26T13:57:41Z

@Sgitario I want to urge you in no way, just give me a hint what's next (or ETA) as if you need a time, I'm going to disable QE tests, thanks a lot

manusa · 2022-09-26T14:12:05Z

Just a quick note that today is a bank holiday in Barcelona

michalvavrik · 2022-09-26T14:13:11Z

Ah, thank you @manusa

Sgitario

Apart from the comments, the changes look good to me.
Many thanks @michalvavrik !

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

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

fixes: quarkusio#28171

michalvavrik · 2022-09-27T08:56:11Z

Thank you for the review.

Sgitario · 2022-09-27T08:57:49Z

Many thanks @michalvavrik . lgtm now
@geoand are you ok with merging this one?

geoand

Seems like you folks have figured it out.

Feel free to merge when CI is green

quarkus-bot · 2022-09-27T14:46:46Z

Failing Jobs - Building `621fc75`

Status	Name	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 11
✔️	JVM Tests - JDK 17
✖	JVM Tests - JDK 17 MacOS M1	⚠️ Check →	Logs	Raw logs
✔️	JVM Tests - JDK 18

Sgitario · 2022-09-27T14:48:25Z

Failures are unrelated to this PR. Merging.

gsmet · 2022-09-28T14:20:11Z

@Sgitario @geoand @michalvavrik I think this should be backported, right?

michalvavrik · 2022-09-28T14:21:31Z

This behavior comes from switch to Kubernetes client 6.1.1, thus 2.13 should not be affected.

gsmet · 2022-09-28T14:31:02Z

OK, perfect, thanks!

manusa · 2023-02-20T06:27:26Z

Note that these methods will need refactoring when bumping the client to 6.5 or at most 6.6

fabric8io/kubernetes-client#4869

geoand · 2023-02-20T07:24:12Z

Thanks for the heads up!

quarkus-bot bot added the area/kubernetes label Sep 23, 2022

michalvavrik force-pushed the feature/fix-knative-client-validation branch from 89fe0db to 75d15be Compare September 23, 2022 12:27

Sgitario reviewed Sep 23, 2022

View reviewed changes

michalvavrik force-pushed the feature/fix-knative-client-validation branch from 75d15be to 04ca353 Compare September 23, 2022 13:31

michalvavrik changed the title ~~Fix Knative/OpenShift client environment check~~ Fix Knative client environment check Sep 23, 2022

michalvavrik requested a review from Sgitario September 23, 2022 13:43

This comment has been minimized.

Sign in to view

michalvavrik force-pushed the feature/fix-knative-client-validation branch 2 times, most recently from 9db9481 to 985a52d Compare September 23, 2022 14:31

michalvavrik changed the title ~~Fix Knative client environment check~~ Fix Knative/OpenShift client environment check Sep 23, 2022

manusa reviewed Sep 23, 2022

View reviewed changes

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

manusa mentioned this pull request Sep 23, 2022

KnativeClient.isSupported doesn't check all of the applicable API Groups fabric8io/kubernetes-client#4447

Closed

manusa approved these changes Sep 23, 2022

View reviewed changes

Sgitario reviewed Sep 27, 2022

View reviewed changes

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

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

Fix Knative client environment check

621fc75

fixes: quarkusio#28171

michalvavrik force-pushed the feature/fix-knative-client-validation branch from 985a52d to 621fc75 Compare September 27, 2022 08:51

Sgitario approved these changes Sep 27, 2022

View reviewed changes

geoand approved these changes Sep 27, 2022

View reviewed changes

geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Sep 27, 2022

Sgitario merged commit 463f9a6 into quarkusio:main Sep 27, 2022

quarkus-bot bot added this to the 2.14 - main milestone Sep 27, 2022

quarkus-bot bot added kind/bugfix and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Sep 27, 2022

michalvavrik deleted the feature/fix-knative-client-validation branch September 27, 2022 14:49

manusa mentioned this pull request Feb 20, 2023

Remove SupportTestingClient interface fabric8io/kubernetes-client#4659

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Knative/OpenShift client environment check #28172

Fix Knative/OpenShift client environment check #28172

michalvavrik commented Sep 23, 2022 •

edited

Sgitario left a comment

michalvavrik commented Sep 23, 2022 •

edited

michalvavrik commented Sep 23, 2022 •

edited

michalvavrik commented Sep 23, 2022 •

edited

This comment has been minimized.

manusa commented Sep 23, 2022

michalvavrik commented Sep 26, 2022

manusa commented Sep 26, 2022

michalvavrik commented Sep 26, 2022

Sgitario left a comment

michalvavrik commented Sep 27, 2022

Sgitario commented Sep 27, 2022

geoand left a comment

quarkus-bot bot commented Sep 27, 2022

Sgitario commented Sep 27, 2022

gsmet commented Sep 28, 2022

michalvavrik commented Sep 28, 2022

gsmet commented Sep 28, 2022

manusa commented Feb 20, 2023

geoand commented Feb 20, 2023

Fix Knative/OpenShift client environment check #28172

Fix Knative/OpenShift client environment check #28172

Conversation

michalvavrik commented Sep 23, 2022 • edited

Sgitario left a comment

Choose a reason for hiding this comment

michalvavrik commented Sep 23, 2022 • edited

michalvavrik commented Sep 23, 2022 • edited

michalvavrik commented Sep 23, 2022 • edited

This comment has been minimized.

manusa commented Sep 23, 2022

michalvavrik commented Sep 26, 2022

manusa commented Sep 26, 2022

michalvavrik commented Sep 26, 2022

Sgitario left a comment

Choose a reason for hiding this comment

michalvavrik commented Sep 27, 2022

Sgitario commented Sep 27, 2022

geoand left a comment

Choose a reason for hiding this comment

quarkus-bot bot commented Sep 27, 2022

Failing Jobs - Building 621fc75

Sgitario commented Sep 27, 2022

gsmet commented Sep 28, 2022

michalvavrik commented Sep 28, 2022

gsmet commented Sep 28, 2022

manusa commented Feb 20, 2023

geoand commented Feb 20, 2023

michalvavrik commented Sep 23, 2022 •

edited

michalvavrik commented Sep 23, 2022 •

edited

michalvavrik commented Sep 23, 2022 •

edited

michalvavrik commented Sep 23, 2022 •

edited

Failing Jobs - Building `621fc75`