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
Use endpoint slices for fabric8 catalog watcher #1149
Conversation
@@ -4,6 +4,6 @@ metadata: | |||
namespace: default | |||
name: namespace-reader | |||
rules: | |||
- apiGroups: ["", "extensions", "apps"] | |||
resources: ["configmaps", "pods", "services", "endpoints", "secrets"] | |||
- apiGroups: ["", "extensions", "apps", "discovery.k8s.io"] |
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.
@ryanjbaxter before I even start documentation and explanation on my thought process in this PR, I need your supervisor advice.
Kubernetes recommends using EndpointSlices instead of Endpoints (think of it a @deprecated). Now, we can use EndpointSlices as this PR and integration tests prove. The problem is that by switching to EndpointSlices, users would need to change the roles, as this file shows (though you have already done that by now, since you need endpoints
for Endpoints anyway)
So, one hand we have the official k8s recommendation, on the other hand if we do follow it, we kind of introduce a "breaking" change, as users need to adjust to it.
Of course, we could let users opt-in into EndpointSlices and we could explain in the docs the steps needed for that, but then we would kind of go against what k8s says to do.
As a side note, there is nothing wrong in using Endpoints, the code still works... wdyt is the correct path forward here? Thank you.
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 understand the Endpoints API is going to continue to be supported and is not planned to be removed at the moment, correct?
Also it seems like Endpoint Slices is only officially supporting in k8s 1.2.1 right? If you are using a version prior to that it was still in beta, correct?
I think if we move forward with this, I would make it an opt in feature. Document how to opt in and the additional configuration that is needed to use 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.
As far as I understand the Endpoints API is going to continue to be supported and is not planned to be removed at the moment, correct?
Correct.
Also it seems like Endpoint Slices is only officially supporting in k8s 1.2.1 right? If you are using a version prior to that it was still in beta, correct?
Yeah, they had alpha support in 1.16, then beta, then stable.
I think if we move forward with this, I would make it an opt in feature. Document how to opt in and the additional configuration that is needed to use it.
Excellent! I'll update the PR accordingly.
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.
Thank you for your suggestions. I've updated the PR as suggested. Have a good trip next week!
|
||
private final KubernetesNamespaceProvider namespaceProvider; | ||
private Function<Fabric8CatalogWatchContext, List<EndpointNameAndNamespace>> stateGenerator; |
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 separate the logic now into two logical pieces: into Endpoints and EndpointSlices. Each of those (Fabric8EndpointsCatalogWatch
and Fabric8EndpointSliceV1CatalogWatch
) is actually a Function
. This function takes as input a Fabric8CatalogWatchContext
(which has members as : KubernetesClient
, KubernetesDiscoveryProperties
and KubernetesNamespaceProvider
) and returns a List<EndpointNameAndNamespace>
.
@PostConstruct | ||
void postConstruct() { | ||
|
||
if (context.properties().useEndpointSlices()) { |
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 need to make a distinction of when to use Endpoints vs EndpointSlices, because the latter might be not support on the k8s cluster. As such, we try to find out if it is supported or not and based on that return the proper stateGenerator
instance
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 think I would rather just let things fail if they enable it but its not enabled on the cluster. We don't generally try to do these types of things when someone has to explicitly enable a feature
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.
updated the PR. thank you very much for the feedback, I did not know which direction to take : exception or defaulting, so this was much appreciated
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.
No problem!
* | ||
* @author wind57 | ||
*/ | ||
record Fabric8CatalogWatchContext(KubernetesClient kubernetesClient, KubernetesDiscoveryProperties properties, |
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.
a record that acts as a Tuple for all the parameters needed in order to compute the List<EndpointNameAndNamespace>
. Please notice both Endpoints and EndpointSlices implementations will delegate to its state
method here.
This method takes as input a Stream<ObjectReference>
because both Endpoints and EndpointSlices can be narrowed to this, meaning :
Stream<ObjectReference> references = endpointSlices.stream().map(EndpointSlice::getEndpoints)
.flatMap(List::stream).map(Endpoint::getTargetRef);
and
Stream<ObjectReference> references = endpoints.stream().map(Endpoints::getSubsets).filter(Objects::nonNull)
.flatMap(List::stream).map(EndpointSubset::getAddresses).filter(Objects::nonNull).flatMap(List::stream)
.map(EndpointAddress::getTargetRef);
I hope this makes sense.
* | ||
* @author wind57 | ||
*/ | ||
final class Fabric8EndpointSliceV1CatalogWatch |
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.
implementation that uses EndpointSlices
* | ||
* @author wind57 | ||
*/ | ||
final class Fabric8EndpointsCatalogWatch |
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.
implementation that uses Endpoints
- this is what we had until now. Nothing has changed logically here
LOG.debug(() -> "discovering endpoints in all namespaces"); | ||
|
||
try (KubernetesClient client = context.kubernetesClient()) { | ||
endpointSlices = client.discovery().v1().endpointSlices().inAnyNamespace() |
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 am using client.discovery().v1()
here. There is also an option to use v1beta1
for EndpointSlices, but since its beta
, I left it out... If you disagree, please let me know.
* | ||
* @author wind57 | ||
*/ | ||
@EnableKubernetesMockClient |
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.
fabric tests for EndpointSlices
env: | ||
- name: LOGGING_LEVEL_ORG_SPRINGFRAMEWORK_CLOUD_KUBERNETES_FABRIC8_DISCOVERY | ||
value: DEBUG | ||
- name: SPRING_CLOUD_KUBERNETES_DISCOVERY_USE_ENDPOINT_SLICES |
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.
property that enables using EndpointSlices
@ryanjbaxter this is ready to be looked at too. thank you |
No description provided.