Move fabric8 discovery to listers#2133
Conversation
Signed-off-by: wind57 <eugen.rabii@gmail.com>
Signed-off-by: wind57 <eugen.rabii@gmail.com>
Signed-off-by: wind57 <eugen.rabii@gmail.com>
Signed-off-by: wind57 <eugen.rabii@gmail.com>
Signed-off-by: wind57 <eugen.rabii@gmail.com>
Signed-off-by: wind57 <eugen.rabii@gmail.com>
Signed-off-by: wind57 <eugen.rabii@gmail.com>
| } | ||
|
|
||
| @Bean | ||
| @ConditionalOnMissingBean |
There was a problem hiding this comment.
we used to create this one in place where needed, but then I noticed that in the k8s-client native, we create in the auto-configuration. So I decided to do it in the fabric8 case too.
Also, it simplifies testing a lot, having it as a Bean
| LogFactory.getLog(Fabric8AbstractBlockingDiscoveryClient.class)); | ||
|
|
||
| private final KubernetesDiscoveryProperties properties; | ||
| private final List<Lister<Service>> serviceListers; |
There was a problem hiding this comment.
Listers and Informers here, just like the case for the k8s-native client. Unlike the native client, we don't have to deal with SharedInfomerFactory, so a bit simpler
|
|
||
| List<Endpoints> allEndpoints = endpoints(properties, client, namespaceProvider, "fabric8-discovery", serviceId, | ||
| predicate); | ||
| List<Service> allServices = serviceListers.stream() |
There was a problem hiding this comment.
this is a 1-1 mapping with k8s-native, meaning we do things the same here, much easier to reason like this
Signed-off-by: wind57 <eugen.rabii@gmail.com>
Signed-off-by: wind57 <eugen.rabii@gmail.com>
| assertThat(BASIC_JSON_TESTER.from(healthResult)) | ||
| .extractingJsonPathArrayValue("$.components.discoveryComposite.components.discoveryClient.details.services") | ||
| .containsExactlyInAnyOrder("kubernetes", "busybox-service"); | ||
| .contains("kubernetes", "busybox-service", "service-wiremock"); |
There was a problem hiding this comment.
we changed to look in all namespaces and there are a few more services present that we do not care about ( like metrics ), so instead use contains
Signed-off-by: wind57 <eugen.rabii@gmail.com>
|
@ryanjbaxter can you trigger copilot for a review here please? |
There was a problem hiding this comment.
Pull request overview
This pull request migrates the fabric8 discovery client implementation from direct Kubernetes API calls to using informers and listers for improved performance and reduced API server load. The changes involve refactoring the discovery client to work with cached data from shared index informers rather than making real-time API calls for service discovery.
Changes:
- Introduced
Fabric8InformerAutoConfigurationto manage shared index informers and listers for Services and Endpoints - Refactored
Fabric8DiscoveryClientand related classes to use listers instead of direct Kubernetes API calls - Updated test files to properly mock informers and listers, removing old WireMock-based approaches
- Added lifecycle management (
@PostConstruct/@PreDestroy) for informer resources
Reviewed changes
Copilot reviewed 45 out of 45 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Fabric8InformerAutoConfiguration.java | New configuration class that creates and manages shared index informers and listers |
| Fabric8DiscoveryClient.java | Updated to accept listers and informers instead of making direct API calls |
| Fabric8AbstractBlockingDiscoveryClient.java | Core refactoring to use listers for service discovery with proper lifecycle management |
| Fabric8DiscoveryClientUtils.java | Removed deprecated methods, added informer creation utilities |
| TestAssertions.java | Added await logic for eventual consistency in tests |
| Multiple test files | Migrated from WireMock to KubernetesMockServer with informer-based testing |
| Fabric8AutoConfiguration.java | Added KubernetesNamespaceProvider bean |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: wind57 <eugen.rabii@gmail.com>
|
copilot has no complaints? I think it just stopped abruptly, because the review is for 45 files, but only a few have summary details. Can I ask for a re-trigger? I don't if you have the free quota or not... |
|
It says it reviewed everything but I retriggered it |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 45 out of 45 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...ringframework/cloud/kubernetes/fabric8/loadbalancer/it/mode/pod/SelectiveNamespacesTest.java
Show resolved
Hide resolved
...java/org/springframework/cloud/kubernetes/fabric8/discovery/Fabric8DiscoveryClientUtils.java
Show resolved
Hide resolved
...rg/springframework/cloud/kubernetes/fabric8/client/discovery/Fabric8DiscoveryBlockingIT.java
Show resolved
Hide resolved
...java/org/springframework/cloud/kubernetes/fabric8/discovery/Fabric8DiscoveryClientUtils.java
Show resolved
Hide resolved
...java/org/springframework/cloud/kubernetes/fabric8/discovery/Fabric8DiscoveryClientUtils.java
Show resolved
Hide resolved
Signed-off-by: wind57 <eugen.rabii@gmail.com>
|
@ryanjbaxter this is ready now, and closes this issue : #2118 I tried to be careful not to have breaking changes, but it helped a lot when we made things far less reachable last year. |
3 participants
No description provided.