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

KubernetesReactiveDiscoveryClient cache configuration issues #1641

Open
habelson opened this issue Apr 17, 2024 · 7 comments
Open

KubernetesReactiveDiscoveryClient cache configuration issues #1641

habelson opened this issue Apr 17, 2024 · 7 comments

Comments

@habelson
Copy link

habelson commented Apr 17, 2024

Describe the bug
Spring Cloud 2023.0.1 (and prior)

We have an ecosystem of spring boot microservices, some are reactive and some are blocking. Some have their own need for caching and some don't. We have run into an issue with the presence of the @Cacheable annotation in KubernetesReactiveDiscoveryClient in reactive microservices that have @EnableCaching for their own purposes.

This has surfaced as several issues:

  1. DefaultKubernetesServiceInstance is not Serializable, preventing some caching implementations from functioning correctly by default
  2. JSON Serialization of DefaultKubernetesServiceInstance fails by default because the properties discovered by the getters don't match whats available in the constructor. This requires configuring the ObjectMapper to ignore unknown properties.
  3. With spring.cloud.kubernetes.loadbalancer.mode=POD the cached value of the discovered services is the POD address which changes on redeploy, and there is nothing setup to invalidate the cache.
  4. The blocking implementation of KubernetesDiscoveryClient does not have the same use of @Cacheable and therefore has different behavior from its reactive counterpart

While I'm not convinced there is necessarily a bug here, I am concerned on the lack of documentation. What sort of cache configuration is expected to be in place (backend library, TTL, etc) for the reactive discovery client to behave as intended? Additionally, was the caching behavior in the reactive version of the class intended to be different from the blocking version?

@ryanjbaxter
Copy link
Contributor

Just trying to understand exactly what you are trying to achieve with caching…

are you trying to cache the services returned by the discovery client so it doesn’t need to make a request to the api server/discovery server?

@wind57
Copy link
Contributor

wind57 commented Apr 18, 2024

these are some interesting issues, imo, but they all need to be broken down to being more specific and fix one at a time. I can definitely give it a try. I will only present my idea for one of them, and if agreed, will gradually move to the next ones.

The first issue I see is that we should provide cacheable and non-cacheable discovery clients (both in fabric8 and native client), that could be switched between each other by a setting, for example: spring.cloud.kubernetes.discovery.cache.enabled (true by default).

So, if in your app:

  • you have added @EnableCaching, we will cache results from DiscoveryClient (because spring.cloud.kubernetes.discovery.cache.enabled=true and we will be using a caching discovery client).
  • you have added @EnableCaching and set spring.cloud.kubernetes.discovery.cache.enabled=false we will not cache results from DiscoveryClient (because spring.cloud.kubernetes.discovery.cache.enabled=false and we will be using a non-caching discovery client).

There are some things to discuss in such an approach.

Currently, reactive native client already has @Cacheable (but not the blocking one), so the approach above would work for it . Fabric8 on the other hand does not (neither for blocking or reactive). This means that potentially users using fabric8 dependency, will have to switch that setting spring.cloud.kubernetes.discovery.cache.enabled to false, if they want the same behavior they had until now.
The reverse is also true, if we start with spring.cloud.kubernetes.discovery.cache.enabled=false, we will break things for native client, but not for fabric8. I find this "worse" then starting with spring.cloud.kubernetes.discovery.cache.enabled=true.

If we go with such an approach, we should evict entries. We do have KubernetesCatalogWatch that does exactly that: it watches for changes and we can somehow hook eviction into that.

If this sounds reasonable, I can create a separate issue, add all the details in there and fix it; then move to the next one in this thread and so on. Just let me know your thoughts Ryan. Thank you

@ryanjbaxter
Copy link
Contributor

I don't think there is a good solution that doesn't cause someone some amount of pain, its unfortunate that one of the clients already has @Cacheable. For the 2024 release train I would say that we just make it possible for a user to cache the services if they want. That might mean creating their own bean with the @Cacheable annotation if they are using one of the clients that doesn't have the annotation. Then in 2025 we can correct this and break functionality.

@wind57
Copy link
Contributor

wind57 commented Apr 18, 2024

yeah, agreed. can we leave this one open until those breaking are allowed then?

@ryanjbaxter
Copy link
Contributor

Yes, but also I would like to explore if we need to make any changes/fixes to the code base today to all things to be cached by using custom bean definitions

@wind57
Copy link
Contributor

wind57 commented Apr 24, 2024

you mean like a org.springframework.cloud.kubernetes.fabric8.discovery.KubernetesDiscoveryClient but one that would cache the results?

So that for example users could opt-in into such a functionality? Did I understand you correctly?

@ryanjbaxter
Copy link
Contributor

No what I am concerned about is that there is something in the way we have implemented the discovery client that would prevent users from providing their own caching implementation

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

No branches or pull requests

4 participants