Repository navigation
Uncomment EndpointSliceAccess function - #4760
Conversation
Signed-off-by: Matei David <matei.david.35@gmail.com>
There was a problem hiding this comment.
In your example 3 you install linkerd with target/cli/darwin/linkerd install. I don't see the endpoint slices flag there so I would expect Linkerd to use Endpoints.
| // | ||
| // return errors.New("no EndpointSlice resources exist in the cluster") | ||
| //} | ||
| func checkEndpointSlicesExist(k8sClient kubernetes.Interface) error { |
There was a problem hiding this comment.
This prevents EndpointSlices mode from being enabled if there are no EndpointSlices resources in the cluster.
Essentially, we're using the existence of an EndpointSlice as a proxy for if the cluster has enabled the EndpointSlices controller. But I wonder if this is actually always correct. Is it possible for EndpointSlices controller to be enabled on the cluster, but for no EndpointSlices resources to exist? Typically there will be services in the kube-system namespace that will have EndpointsSlices, but can we always rely on this? What if all of those services were deleted? Are there kubernetes distributions that don't create any services by default?
I'm on the fence about this but I'm leaning towards removing this check. The downside to removing it is that it opens the door for users to shoot themselves in the foot by enabling EndpointSlices mode in Linkerd for clusters that have the EndpointSlices resource type but don't run the EndpointSlices controller. The upside is that we don't have to worry about install failing if a cluster somehow has no EndpointSlices resources.
There was a problem hiding this comment.
This is a very good point @adleong and I'm happy it's been brought up. I was going to argue for the opposite actually; I think we should only check for the existence of an EndpointSlice in the cluster rather than only check if the resource exists, or rather than checking for both. I'll describe my thoughts below, let me know how you feel about it and what you think.
First, I agree that using the existence of an EndpointSlice as a proxy for the controller's enablement involves a high degree of uncertainty -- we have no way of knowing for sure whether kube-system namespace will have any EndpointSlices, or whether the services even exists to begin with. One recurring EndpointSlice however that should exist when the controller is turned on, is the slice for thekubernetes service. This is just an assumption on my side, but even if all of the kube-system services are deleted or non-existent, the kubernetes service should still be created?
I find it that it's more likely for people to shoot themselves in the foot or experience undefined behaviour, especially when they run k8s v1.18.0+ if we check whether the resource exists in the cluster. Again, this is an assumption, but since slices are enabled by default, if the slice controller is turned off it means it has been turned off intentionally. Moreover, in this scenario if for whatever reason the flag has been turned on without checking for existence of at least an EndpointSlice, the destination service will just crash (since the resource will have been created in the cluster but the controller is off). I'm not sure how likely this scenario is, but this is what has been in the back of my mind when working on this.
To have a tl;dr:
The upside is that we don't have to worry about install failing if a cluster somehow has no EndpointSlices resources
I find it more likely that there will exist a resource in the cluster especially in the newer versions of k8s and we should treat the controller as the source of the truth rather than the resource itself.
On the other hand, I see your point. I think if we remove this check we mitigate some of the risk that stems from the uncertainty you have described. In my mind, removing this check gives us a more predictive feel for how the EndpointSlices should work -- if the cluster supports the resource and you have enabled the flag, then that's what the destination service will use. I think this makes the assumption that users will be more responsible in knowing what the feature does and in adopting the feature. However, we can't always guarantee that if the resource exists, the slices are created -- I see more room for failure here.
If I had to take my chances between a user mistake and a cluster bootstrapping and/or service failure though, I'd go for the user making a mistake; it is more likely that the operator will miss something (self-evident from me pasting the wrong logs in this PR!) but I have seen an increasing amount of people rooting for the ability to do whatever they want.
I hope this point of view makes sense in the context you brought up, I might just be scratching the surface here but I thought I'd weigh in. Another option is to keep both in, of course. Excuse my inexperienced point of view if my worries are unfounded! :)
I'd like to also get your input on this @zaharidichev since you have reviewed the previous PR about adding EndpointSlices to the destination service.
There was a problem hiding this comment.
Thanks for the thoughtful and articulate response, @Matei207. I think you're right. It does seem reasonably safe to assume that the kubernetes service should always exist and have an endpoints (or EndpointSlices) object. So I'm on board with making this the source of truth.
You also mention potentially removing the check to see if the resource type exists. My question is: what happens on clusters that do not have the EndpointSlices resource type? If the EndpointSlices list simply fails with an error that we can handle and interpret to mean that EndpointSlices should be disabled, that's great. If this causes a panic or a hang, then I don't think we can remove the check.
There was a problem hiding this comment.
My question is: what happens on clusters that do not have the EndpointSlices resource type
Hmm, hate to disappoint after receiving compliments for my previous answer but I haven't given this much thought. I'm not 100% sure, but in the case that the resource does not exist, the error message would just be a NotFound. I think under the hood client-go makes REST calls to the apiserver. By this logic, trying to list the resource would give a 404.
I imagine the error would look similar to these logs (taken from a support question slack):
go@v0.17.4/tools/cache/reflector.go:105: Failed to list *v1beta1.EndpointSlice: the server could not find the requested resource
N.B in the logs above the k8s version was v1.14.0.
I'm not sure if the same would be thrown when directly trying to list the slices, that was an error propagated by an informer so the behaviour might be different. At the same time, the only reason I'd advocate for removing the check is to avoid making two calls; not sure how expensive the calls client-go makes are, but if they're not excessively detrimental to the performance I'm leaning more towards the side of safety. It also looks idiomatic to me and easier to figure out what's going on if we keep both in.
|
@adleong sorry about that, I think I made a typo when I copied the logs from my notes. I'll add some pictures with timestamps to show you the output of the commands just to be sure: Will edit the body of the PR so it's not misleading. |
There was a problem hiding this comment.
That all makes sense! Thanks for the extra detail!
What
EndpointSliceAcessmethod and cleans up left over todos in the destination service.EndpointSlices(EndpointSlice support (#4501) #4663 service topologies: add Kubernetes/API EndpointSlice support #4696 service topologies: add install flag for EndpointSlices #4740); they should now be functional (albeit prone to bugs) and ready to use.Tests
I tested a total of three broad scenarios:
EndpointSlices:EndpointSlicesupport, but not Linkerd installed withEndpointSlicesupport:EndpointSlicesupport but without support in Linkerd, the right resource is watchedEndpointSlicesupport, with Linkerd installed withEndpointSlicesupport:My conclusion is that at first glance everything seems to be working well enough to have them enabled for experimental use. The same messages are being sent as for the Endpoints resource, the flag does a well enough job of preventing installation when the resource does not exist and a well enough job of allowing the installation to proceed when the access criteria is met.
Signed-off-by: Matei David matei.david.35@gmail.com