-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 6.6.2 to 6.7.1 #33767
Conversation
Thanks for your pull request! The title of your pull request does not follow our editorial rules. Could you have a look?
|
|
This comment has been minimized.
This comment has been minimized.
The native test failure looks very suspicious |
I'll look into it |
Note that we've been running native tests with snapshots of 6.7 without issue so the problem might come from the mock server. To be honest, I'm not even sure what the failing tests are testing 😅 since they seem to only check that the mock server is returning the answer it is set to return in the setup method, they don't appear to use the client at all. So, at least without further digging, these tests seem useless to me for the purpose of testing the client. |
That's a good hint (the fact that the native tests have been run with the SNAPSHOTs). However, this is the detailed log message (irrelevant parts omitted) of the test failure:
I'm mostly sure it has to do with the more elaborate changed we did to to serialization in this release fabric8io/kubernetes-client#4662 However, Chris' comment makes me believe that maybe is an issue while registering the SPI modules 🤷 |
So the deserializer is now lazy initialized at runtime when used: Also the deserializer is no longer static but an instance (first step to allow having multiple serialization options for the same application): fabric8io/kubernetes-client@6d7db80 Probably this is the cause of the issue. |
Not sure if the new Lines 205 to 209 in 187ca4b
|
The native binary built for the integration test fails as soon as it tries to deserialize something (with the same error -
|
In general, anything that uses the Java ServiceLoader, needs to be explicitly registered for it to work in native mode. |
Yes, that was taken care of some time ago. |
So providing a static KubernetesSerialization instance does the trick and allows the class loaders ro detect the SPI provided KubernetesResource classes. We should probably come up with a better and more elegant solution to deal with the class loaders. However, I think that this will require additional work both in the client and in Quarkus. Also, the current implementation has a small workaround to be able to register the KubernetesResources when the static variable is initialized by invoking I created some changes in the Client (fabric8io/kubernetes-client#5200) that when released will allow for something nicer. However, if everyone agrees I think we can proceed with the current changes and create a follow up issue to implement KubernetesSerialization class loading from Quarkus' KubernetesClientUtils after we release the next Kubernetes Client version (I would take care of fixing that one) ---> not urgent. /cc @shawkins |
This comment has been minimized.
This comment has been minimized.
import io.quarkus.runtime.TlsConfig; | ||
|
||
public class KubernetesClientUtils { | ||
|
||
private static final String PREFIX = "quarkus.kubernetes-client."; | ||
private static final KubernetesSerialization SERIALIZATION = new KubernetesSerialization(new ObjectMapper(), true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we use the Quarkus ObjectMapper here, in case it has been customised by users?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an important point.
I'd first ask how were users customizing the Fabric8's serialization purposes ObjectMapper
before.
If mutating Serialization.jsonMapper()
is the answer to the previous question and this was an extended practice, then we should definitely provide a way to customize this mapper. With the current changes, this will no longer be possible.
The next point would be to know if we want to share a same ObjectMapper
used application-wise with the client or have an exclusive one for it. Some mapper configurations might not be ideal for the client or even break it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, it should not use the user customizable mapper, because the client itself knows everything about the model serialization/deserialization and should not be affected by the user's configuration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@geoand this isn't quite true, for example, if people want to use Kotlin or other libs in conjunction to the Fabric8 client, they would need to customise the mapper to make it work correctly with the client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that's the case, I think it would make more sense to have a way to configure this mapper specifically
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I'm aware, without being a kotlin expert, you cannot use the fabric8 client from kotlin without customizing the fabric8 mapper. Similarly, as far as I'm aware, you cannot have two different mappers take part into the serialisation of one single object. So I would very much like to hear your actual solutions to both of these issues apart from saying you shouldn't customise the mapper because that doesn't really get our users anywhere, and by users, I mean Quarkus users of the Fabric8 client, not JOSDK users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a bean and qualifier to be able to inject the KubernetesClient-specific ObjectMapper for further modifications such as the Kotlin use case. (see @KubernetesClientObjectMapper
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it make more sense to implement an approach similar to what I did in QOSDK, i.e. using the customizer approach, instead of this approach which would require people to create a whole new ObjectMapper
(at least, if I understand things properly)? That seems like exactly the kind of thing we would rather avoid because that would require people to know how the original mapper is configured so that they don't break the client's functionality. We would rather want people to add their configuration on top of what is already configured instead of doing it from scratch.
Also, with this approach, shouldn't the producing method of KubernetesClientObjectMapperProducer
be marked as DefaultBean
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just deleted the previous reply (when I wrote it, I was confused and mixed two of Chris' comments)
The ObjectMapper
that we pass into the KubernetesSerialization
will get customized by the Fabric8 Client (at a later point).
With the latest changes, users are able to pass their own ObjectMapper
or a new ObjectMapper
will be created for them. There's no reason why users should do this besides enabling the Kotlin stuff. I think it's now clearer that this Mapper has no customization whatsoever until it has been injected into the KubernetesSerializationProducer
and a new KubernetesSerialization
is created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As explained above customisation could occur for a variety of reasons. I still think that users should customise the instance rather than provide their own. We might not use any customisation at this point, but should that change, users would need to use the same customisation and add theirs on top so I really believe customizing the existing instance is a better choice than injecting a new one. This would also allows us to control at the customisation point that the customisations are indeed compatible with the needs of the client.
This would also fit with what Quarkus is doing with the ObjectMapperCustomizer
feature.
Signed-off-by: Marc Nuri <[email protected]>
e7ad0b7
to
f43669e
Compare
import io.quarkus.runtime.TlsConfig; | ||
|
||
public class KubernetesClientUtils { | ||
|
||
private static final String PREFIX = "quarkus.kubernetes-client."; | ||
private static final ObjectMapper KUBERNETES_CLIENT_MAPPER = new ObjectMapper(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I very much want to avoid this, but I would need to time to look into alternatives
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in order for me to understand: before this change, was the k8s client in Quarkus using it's own mapper or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this more, we probably can't avoid is done here. What we do want to avoid however is using lambdas in runtime code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in order for me to understand: before this change, was the k8s client in Quarkus using it's own mapper or not?
The Kubernetes Client was using a static serialization utils class. Everything was static.
We're doing some changes to make the serialization part instantiatable, so you can have multiple clients with multiple serialization configurations.
Seems like the move to instances is not playing nice with SPI, native, and these processors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Kubernetes Client was using a static serialization utils class. Everything was static.
Good to know. Just to make sure there is no misunderstanding: The old code was instantiating an ObjectMapper
, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha thanks. In that case, I am pretty sure that this general approach augmented with build time discovery of KubernetesResource
will be fine.
I'll have a look at the latter tomorrow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this, actually. If we make it possible for users to inject a mapper, shouldn't that mapper be used here instead of a static version that won't be configured the same way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not application code, but build time code. The mapper is not really the problem, but the registration of the KubernetesResource classes is, since doing this at runtime is not working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is in the runtime module, therefore accessible to users and I do know users that use this class to create instances of the client so I don't think that this is strictly build time code, even if we'd like to think it is…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the current state, there are no notable differences with what was done before bumping. The logic that was run upstream is now located here. In fact, just running the scanKubernetesResources()
method from the static block should suffice to fix the native issue.
// since those classes won't be available and won't be registered in the KubernetesSerialization instance | ||
private static final Set<Class<? extends KubernetesResource>> KUBERNETES_RESOURCE_CLASES = scanKubernetesResources(); | ||
private static final KubernetesSerialization SERIALIZATION = new KubernetesSerialization(KUBERNETES_CLIENT_MAPPER, false); | ||
static { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably add all initialization in this block, otherwise the code is hard to follow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: shouldn't this use the injectable version instead of a static version?
It does seem that the TCCL is unable to find the SPI provided classes that were recorded during build time at the static init phase. |
b27efeb
to
21982eb
Compare
So it seems that I did actually mischose the package and module location. Moved some things around and everything seems to be working locally now. |
This comment has been minimized.
This comment has been minimized.
OK, so for the next set of failures
So, when preparing the list of classes, we should filter those that are provided by the deployment extension (only needed for deployment purposes) from those that are production classes (needed by the application at runtime). Not sure how to do this though. Any ideas? |
I would suggest the following: Instead of using @Recorder
public class KubernetesSerializationRecorder {
public RuntimeValue<Class<? extends KubernetesResource>[]> initKubernetesResources(
Class<? extends KubernetesResource>[] resources) {
return new RuntimeValue<>(resources);
}
} pass in an array of |
That's what I was more or less thinking of, but it felt a little bit dirty. Knowing that you share the same approach I'm more comfortable with it 😅. |
It's pretty much what Quarkus does under the covers anyway, but I don't remember which ClassLoader it uses, so we can try and be explicit |
It depends on the phase. For the For the |
I was referring to the CL used under the covers when "recording" classes. |
Unfortunately, the workaround is not working since the recorded bytecode is played as it is and the exception is still thrown for the second invocation. |
Sorry, I am not following this. |
21982eb
to
a8b005f
Compare
Now I see what you meant earlier today.
Yes, there is a way and we should indeed use it in order to reduce the recorded bytecode size.
|
Signed-off-by: Marc Nuri <[email protected]>
a8b005f
to
fea1aee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot!
Fabric8 team removed[1][2] constructors, which we were using to create Openshift client Switched to other methods. This change was brought to Quarkus with this update[3] [1] fabric8io/kubernetes-client@6d7db80#diff-92a1266d167b46838301a64f043de2895376ebf7ec810e7769d641d2fed6f511L227-L246 [2] fabric8io/kubernetes-client#4662 [3] quarkusio/quarkus#33767
Kubernetes Client 6.7.0 was just released: https://github.com/fabric8io/kubernetes-client/releases/tag/v6.7.0Kubernetes Client 6.7.1 was just released: https://github.com/fabric8io/kubernetes-client/releases/tag/v6.7.1
Just speeding up the dependabot process to see if CI reports any issue.
/cc @metacosm