search

spring-cloud / spring-cloud-kubernetes

Kubernetes integration with Spring Cloud Discovery Client, Configuration, etc...
Apache License 2.0
3.47k stars 1.04k forks source link

KubernetesReactiveDiscoveryClient cache configuration issues #1641

Open habelson opened 7 months ago

habelson commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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:

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 commented 7 months ago

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 commented 7 months ago

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

ryanjbaxter commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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