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

Bump kubernetes-client-bom from 5.12.2 to 6.1.1 #26107

Merged
merged 1 commit into from Sep 8, 2022

Conversation

manusa
Copy link
Contributor

@manusa manusa commented Jun 14, 2022

Kubernetes Client 6.1.1 was just released: https://github.com/fabric8io/kubernetes-client/releases/tag/v6.1.1

Major versions of the Client (transitive dependencies) must be aligned, in consequence, this PR also updates Dekorate dependency to 3.0.0.

/cc @metacosm

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 14, 2022

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

  • title should preferably start with an uppercase character (if it makes sense!)

This message is automatically generated by a bot.

@quarkus-bot quarkus-bot bot added the area/dependencies Pull requests that update a dependency file label Jun 14, 2022
@manusa manusa changed the title wip: Bump kubernetes-client-bom from 5.12.2 to 6.0.0 Bump kubernetes-client-bom from 5.12.2 to 6.0.0 (wip) Jun 14, 2022
@manusa manusa marked this pull request as ready for review June 14, 2022 07:56
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@gastaldi
Copy link
Contributor

How about marking it as a draft to prevent accidental merging? The tests should still run in your fork

@manusa manusa marked this pull request as draft June 15, 2022 17:19
@manusa
Copy link
Contributor Author

manusa commented Jun 15, 2022

I think I need to reenable most pipelines in my fork.
Anyway, there's plenty of stuff to fix 😅. I'll be checking those errors shortly.

@manusa
Copy link
Contributor Author

manusa commented Jul 11, 2022

I'm checking some of these log errors. We'll need to update the Dekorate dependency along with the Kubernetes Client with a version of Dekorate based on Kubernetes Client 6.

2022-06-14T13:06:04.4042009Z Caused by: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
2022-06-14T13:06:04.4042876Z 	[error]: Build step io.quarkus.kubernetes.deployment.KubernetesProcessor#build threw an exception: java.lang.NoSuchMethodError: 'io.fabric8.kubernetes.api.model.ServicePortFluent io.fabric8.kubernetes.api.model.ServicePortBuilder.withNewTargetPort(java.lang.Integer)'
2022-06-14T13:06:04.4043305Z 	at io.dekorate.kubernetes.decorator.AddServiceResourceDecorator.toServicePort(AddServiceResourceDecorator.java:78)
2022-06-14T13:06:04.4044324Z 	at io.dekorate.kubernetes.decorator.AddServiceResourceDecorator.lambda$visit$1(AddServiceResourceDecorator.java:69)
...
2022-06-14T13:06:04.4054609Z 	at io.quarkus.kubernetes.deployment.KubernetesProcessor.lambda$build$2(KubernetesProcessor.java:173)
...

/cc @iocanel

@manusa manusa force-pushed the deps/kubernetes-client branch 2 times, most recently from 05a1760 to 1b75a41 Compare July 11, 2022 15:01
@iocanel
Copy link
Contributor

iocanel commented Jul 11, 2022

Hopefully, withNewServicePort is now simplified to withServicePort as there is no need to create a nested builder for an Integer which is something we overzelously did in the past. Need to verify though that this is intended.

@metacosm
Copy link
Contributor

Is there any update on this?

@manusa
Copy link
Contributor Author

manusa commented Aug 31, 2022

Is there any update on this?

AFAIK there is work in progress to update Dekorate to use version 6.0.0 of the client.

@manusa manusa changed the title Bump kubernetes-client-bom from 5.12.2 to 6.0.0 (wip) Bump kubernetes-client-bom from 5.12.2 to 6.1.0 (wip) Aug 31, 2022
@gastaldi
Copy link
Contributor

gastaldi commented Aug 31, 2022

Can we already exclude the io.fabric8:kubernetes-httpclient-okhttp dependency on it and use the io.fabric8:kubernetes-httpclient-jdk instead?

Or maybe we can provide our own io.fabric8.kubernetes.client.http.HttpClient$Factory implementation using the Vert.x library in the extension?

@metacosm
Copy link
Contributor

Good point! Should probably be documented though! Which also reminds me that we need to finish the vertx client implementation…

@manusa
Copy link
Contributor Author

manusa commented Sep 1, 2022

Dependencies should ideally be as follows:

  • kubernetes-client-api: compile dependency
  • kubernetes-client/openshift-client: runtime dependency

However, I'm not sure how might this affect the Kubernetes-based extensions and the native compilation. Especially regarding the client bean/singleton initialization which is based on SPI when using it this way. I guess that it's likely that we'll end up having a compile dependency to one of the Fabric8 HttpClient implementations.

Regarding the usage of alternate clients, io.fabric8:kubernetes-httpclient-jdk could be used instead but requires JRE11+. Also note that there was a bug on the JDK until v16 that prevents some features to work as expected (I can't exactly remember which /cc @shawkins ?).

IMO we should deal with the version upgrade first and then try to refactor the client dependencies and their scopes.

Which also reminds me that we need to finish the vertx client implementation…

If I have time later this month I'll give it a try

@manusa manusa changed the title Bump kubernetes-client-bom from 5.12.2 to 6.1.0 (wip) Bump kubernetes-client-bom from 5.12.2 to 6.1.1 (wip) Sep 1, 2022
@Sgitario
Copy link
Contributor

Sgitario commented Sep 6, 2022

Dekorate 3.0.0 has been released which uses kubernetes-client 6.0.0.
I think you can now resume this pull request by updating dekorate to 3.0.0 as well.

@manusa manusa marked this pull request as ready for review September 7, 2022 12:17
@manusa manusa force-pushed the deps/kubernetes-client branch 2 times, most recently from d065350 to 2c6c472 Compare September 7, 2022 12:21
@manusa manusa changed the title Bump kubernetes-client-bom from 5.12.2 to 6.1.1 (wip) Bump kubernetes-client-bom from 5.12.2 to 6.1.1 Sep 7, 2022
metacosm added a commit to quarkiverse/quarkus-operator-sdk that referenced this pull request Sep 7, 2022
@manusa
Copy link
Contributor Author

manusa commented Sep 7, 2022

The PR should be ready now.
I'm not sure about Dekorate (/cc @Sgitario), but the major version change in the Fabric8 Kubernetes Client dependency brings some breaking changes in the codebase, and in the default behavior. They are all documented in our migration guide.

I recall that in previous occasions we added something in the Quarkus Wiki. I'm not sure how to proceed in this case.

@Sgitario
Copy link
Contributor

Sgitario commented Sep 8, 2022

The PR should be ready now. I'm not sure about Dekorate (cc/ @Sgitario), but the major version change in the Fabric8 Kubernetes Client dependency brings some breaking changes in the codebase, and in the default behavior. They are all documented in our migration guide.

I recall that in previous occasions we added something in the Quarkus Wiki. I'm not sure how to proceed in this case.

I don't think Dekorate brings any breaking changes, other than the ones related to Fabric8 Kubernetes Client 6.x. Do you know something worthy to be mentioned in the migration guide @iocanel ?

<dependency>
<groupId>io.fabric8</groupId>
<artifactId>kubernetes-httpclient-okhttp</artifactId>
<scope>provided</scope>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the dependency provided?

Copy link
Member

Choose a reason for hiding this comment

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

And why is it in the deployment module?

Maybe you're trying to make it optional? But I think we should make it runtime and non-optional and then switch to the vertx one as soon as it is ready, given that's our ultimate goal.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on this plan - let's play it safe for now

Copy link
Contributor Author

@manusa manusa Sep 8, 2022

Choose a reason for hiding this comment

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

The reasoning is:

  • kuberentes-httpclient-okhttp is a runtime dependency of the kubernetes-client module. So for the deployment module, at runtime, the jar package with the SPI will always be available. Hence the provided scope.
  • It's not an optional package because it's required at runtime.
  • I added this as provided so I could simply import the factory class in the statement:
    serviceProviderProducer.produce(new ServiceProviderBuildItem(
                  "io.fabric8.kubernetes.client.http.HttpClient$Factory", OkHttpClientFactory.class.getName()));
    The alternative would be to hardcode the name of the class, but I think this would be less maintainable.

Please, let me know how do you want me to proceed.

Copy link
Contributor

Choose a reason for hiding this comment

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

The semantics of provided in a deployment module are weird, so let's avoid them and do:

The alternative would be to hardcode the name of the class

The native tests will pick up any breakage if for whatever version the class name changes in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good.
I'll rebase and update.

Copy link
Contributor

Choose a reason for hiding this comment

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

TY

Copy link
Member

Choose a reason for hiding this comment

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

I might miss something but if kubernetes-httpclient-okhttp is a transitive dependency of the runtime module, then it should be around for the deployment module, which depends on the runtime one.

Copy link
Contributor Author

@manusa manusa Sep 8, 2022

Choose a reason for hiding this comment

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

It's a runtime transitive dependency of the runtime module :)

Anyway, it's changed now.

@iocanel
Copy link
Contributor

iocanel commented Sep 8, 2022

I don't think Dekorate brings any breaking changes, other than the ones related to Fabric8 Kubernetes Client 6.x. Do you know something worthy to be mentioned in the migration guide @iocanel ?

Dekorate 3.0.0 just adopts the 6.1.0 model and does not introduce any new change.
So, I don't expect any dekorate related functionality to break.

Copy link
Contributor

@iocanel iocanel left a comment

Choose a reason for hiding this comment

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

LGTM. From my side everything looks good.
There are some questions from @geoand and @gsmet though.

Signed-off-by: Marc Nuri <marc@marcnuri.com>
@iocanel iocanel requested a review from geoand September 8, 2022 14:13
@gastaldi gastaldi merged commit c376d27 into quarkusio:main Sep 8, 2022
@quarkus-bot quarkus-bot bot added this to the 2.13 - main milestone Sep 8, 2022
@gastaldi
Copy link
Contributor

gastaldi commented Sep 8, 2022

Oh didn't notice that some checks were still running. Anyway, I'll monitor it. Pretty sure they will pass 😀

@manusa manusa deleted the deps/kubernetes-client branch September 8, 2022 16:48
@gsmet
Copy link
Member

gsmet commented Sep 8, 2022

I added a note in the migration guide: https://github.com/quarkusio/quarkus/wiki/Migration-Guide-2.13 .

@manusa
Copy link
Contributor Author

manusa commented Sep 9, 2022

I added a note in the migration guide: https://github.com/quarkusio/quarkus/wiki/Migration-Guide-2.13 .

Thx!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants