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.

Sign up for GitHub

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

improve number of method calls to k8s api server for endpoints catalog watcher #1393

Merged
merged 5 commits into from
Aug 2, 2023
Merged

improve number of method calls to k8s api server for endpoints catalog watcher #1393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
283fb9a
fix
wind57 Jul 28, 2023
97afe75
add a tst
wind57 Jul 28, 2023
deae49a
Merge branch '3.0.x' into simplify-catalog-calls
wind57 Aug 1, 2023
fc116fe
checkstyle
wind57 Aug 1, 2023
ef4660b
trigger
wind57 Aug 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .../org/springframework/cloud/kubernetes/fabric8/discovery/Fabric8EndpointsCatalogWatch.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@

import org.springframework.cloud.kubernetes.commons.discovery.EndpointNameAndNamespace;

import static org.springframework.cloud.kubernetes.fabric8.discovery.Fabric8KubernetesDiscoveryClientUtils.ALWAYS_TRUE;
import static org.springframework.cloud.kubernetes.fabric8.discovery.Fabric8KubernetesDiscoveryClientUtils.endpoints;

/**
Expand All @@ -41,7 +42,7 @@ final class Fabric8EndpointsCatalogWatch
@Override
public List<EndpointNameAndNamespace> apply(Fabric8CatalogWatchContext context) {
List<Endpoints> endpoints = endpoints(context.properties(), context.kubernetesClient(),
context.namespaceProvider(), "catalog-watcher", null, x -> true);
context.namespaceProvider(), "catalog-watcher", null, ALWAYS_TRUE);

/**
* <pre>
Expand Down
4 changes: 3 additions & 1 deletion ...ngframework/cloud/kubernetes/fabric8/discovery/Fabric8KubernetesDiscoveryClientUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ final class Fabric8KubernetesDiscoveryClientUtils {
private static final LogAccessor LOG = new LogAccessor(
LogFactory.getLog(Fabric8KubernetesDiscoveryClientUtils.class));

static final Predicate<Service> ALWAYS_TRUE = x -> true;

private Fabric8KubernetesDiscoveryClientUtils() {

}
Expand Down Expand Up @@ -225,7 +227,7 @@ else if (!properties.namespaces().isEmpty()) {
static List<Endpoints> withFilter(List<Endpoints> endpoints, KubernetesDiscoveryProperties properties,
KubernetesClient client, Predicate<Service> filter) {

if (properties.filter() == null || properties.filter().isBlank()) {
if (properties.filter() == null || properties.filter().isBlank() || filter == ALWAYS_TRUE) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this one is interesting and got my attention purely by accident, while working on a different thing.

Both discovery and catalog watch need to search for endpoints, as such they both delegate to the same method. At the same time, because of an issue we had, when searching for endpoints, we need to filter them based on a Predicate. That predicate is a service predicate, it sounds weird - but there was a real defect for this. An example will make this may be easier to understand:

  • in discovery implementation, we search for endpoints
  • users have specified a service SPeL expression (predicate)
  • as such, after we find the needed endpoints, we then go on to find the services for these endpoints (they have to match by name), but only take those that match the predicate.
  • with these service names, we then filter the endpoints and that is the result.

In plain english, it's like we filter endpoints based on a service predicate.

On the other hand, we also need to look for endpoints in the catalog implementation. The difference is that catalog does not support a predicate.

Both catalog and discovery delegate to the same method when looking for endpoints.

And now suppose the case:

  • users have enabled some SPeL Expression
  • they also enabled catalog watcher

Suppose catalog watcher calls this method, and since it does not care about service filtering it passes x -> true as the predicate into it. This is how it looks like:

List<Endpoints> endpoints = endpoints(context.properties(), context.kubernetesClient(),
				context.namespaceProvider(), "catalog-watcher", null, x -> true);

This if statement:

if (properties.filter() == null || properties.filter().isBlank()

is false (because users have enabled a SPeL Expression), so catalog watcher will go on and start searching for services and to try and filter them out by the predicate, again, since this is what the common method does.

But it does not need to do that, since it does not support filtering anyway (we used to pass x -> true always).

So with this fix, we essentially trim down a few method calls to the API server, when not needed.

LOG.debug(() -> "filter not present");
return endpoints;
}
Expand Down
30 changes: 27 additions & 3 deletions .../cloud/kubernetes/fabric8/discovery/Fabric8KubernetesDiscoveryClientUtilsFilterTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@

import org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscoveryProperties;

import static org.springframework.cloud.kubernetes.fabric8.discovery.Fabric8KubernetesDiscoveryClientUtils.ALWAYS_TRUE;

/**
* @author wind57
*/
Expand All @@ -53,7 +55,7 @@ void afterEach() {
@Test
void withFilterEmptyInput() {
List<Endpoints> result = Fabric8KubernetesDiscoveryClientUtils.withFilter(List.of(), PROPERTIES, client,
x -> true);
ALWAYS_TRUE);
Assertions.assertEquals(result.size(), 0);
}

Expand Down Expand Up @@ -87,7 +89,7 @@ void withFilterOneEndpointsMatchInService() {
Endpoints endpoints = createEndpoints("a", "namespace-a");
createService("a", "namespace-a");
List<Endpoints> result = Fabric8KubernetesDiscoveryClientUtils.withFilter(List.of(endpoints), PROPERTIES,
client, x -> true);
client, ALWAYS_TRUE);
Assertions.assertEquals(result.size(), 1);
}

Expand All @@ -112,6 +114,28 @@ void withFilterTwoEndpointsOneMatchInService() {
Assertions.assertEquals(result.get(0).getMetadata().getNamespace(), "namespace-a");
}

/**
* <pre>
* - Endpoints with name : "a" and namespace "namespace-a", present
* - Endpoints with name : "b" and namespace "namespace-b", present
* - Service with name "a" and namespace "namespace-a" present
* - Predicate that we use is "ALWAYS_TRUE", so no service filter is applied
*
* As such, there is a match, single endpoints as result.
* This test is the same as above with the difference in the predicate.
* It simulates Fabric8EndpointsCatalogWatch::apply
* </pre>
*/
@Test
void withFilterTwoEndpointsOneMatchInServiceAlwaysTruePredicate() {
Endpoints endpointsA = createEndpoints("a", "namespace-a");
Endpoints endpointsB = createEndpoints("b", "namespace-b");
createService("a", "namespace-a");
List<Endpoints> result = Fabric8KubernetesDiscoveryClientUtils.withFilter(List.of(endpointsA, endpointsB),
PROPERTIES, client, ALWAYS_TRUE);
Assertions.assertEquals(result.size(), 2);
}

/**
* <pre>
* - Endpoints with name : "a" and namespace "namespace-a", present
Expand All @@ -132,7 +156,7 @@ void withFilterTwoEndpointsAndThreeServices() {
createService("c", "namespace-c");

List<Endpoints> result = Fabric8KubernetesDiscoveryClientUtils.withFilter(List.of(endpointsA, endpointsB),
PROPERTIES, client, x -> true);
PROPERTIES, client, ALWAYS_TRUE);
Assertions.assertEquals(result.size(), 2);
result = result.stream().sorted(Comparator.comparing(x -> x.getMetadata().getName())).toList();
Assertions.assertEquals(result.get(0).getMetadata().getName(), "a");
Expand Down