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.

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

plugin/kubernetes: Switch to v1 EndpointSlices #4568

Conversation

sgreene570
Copy link
Contributor

@sgreene570 sgreene570 commented Apr 9, 2021

Discovery.k8s.io/v1beta1 EndpointSlices are deprecated in favor of
discovery.k8s.io/v1 in Kubernetes 1.21.

https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.21.md

This commit switches all imports of "k8s.io/api/discovery/v1beta1" to
"k8s.io/api/discovery/v1" in plugin/kubernetes/*.

Signed-off-by: Stephen Greene sgreene@redhat.com


This PR is a follow up to #4567.

Discovery.k8s.io/v1beta1 EndpointSlices are deprecated in favor of
discovery.k8s.io/v1 in Kubernetes 1.21.

https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.21.md

This commit switches all imports of "k8s.io/api/discovery/v1beta1" to
"k8s.io/api/discovery/v1" in `plugin/kubernetes/*`.

Signed-off-by: Stephen Greene <sgreene@redhat.com>
@chrisohaver
Copy link
Member

The failing test is because CoreDNS watches Endpoints instead of Endpointslices, due to the Endpointslices API support check failing at ...

if _, err := kubeClient.Discovery().ServerResourcesForGroupVersion(discovery.SchemeGroupVersion.String()); err == nil {

@sgreene570
Copy link
Contributor Author

The failing test is because CoreDNS watches Endpoints instead of Endpointslices, due to the Endpointslices API support check failing at ...

if _, err := kubeClient.Discovery().ServerResourcesForGroupVersion(discovery.SchemeGroupVersion.String()); err == nil {

Mind if I ask how you were able to arrive at that conclusion? The CI output for the failing test seems minimal.

Could the problem be the fact that https://github.com/coredns/ci/blob/master/test/kubernetes/metrics_test.go is currently using discovery v1beta1?

@chrisohaver
Copy link
Member

Mind if I ask how you were able to arrive at that conclusion? The CI output for the failing test seems minimal.

Sure, I ran CoreDNS with the changes in this PR against a local instance of k8s (v1.19), and saw that the check fails because k8s 1.19 doesn't support discovery v1, which causes CoreDNS to not watch EndpointSlices (the expectation is that it will watch endpointslices). If CoreDNS doesn't watch Endpointslices, then no endpoint latency metrics will be recorded in the Endpointslice latency test.

CoreDNS releases on a single branch, so to maintain support for k8s < 1.21.0, I think we need to defer the bump to v1 until older k8s versions are no longer officially supported.

Is there a functional difference between v1beta1 and v1 that requires a bump to v1?

@sgreene570
Copy link
Contributor Author

sgreene570 commented Apr 9, 2021

CoreDNS releases on a single branch, so to maintain support for k8s < 1.21.0, I think we need to defer the bump to v1 until older k8s versions are no longer officially supported.

Is there a functional difference between v1beta1 and v1 that requires a bump to v1?

AFAIK there is no functional difference between v1 and v1beta1. v1beta1 is deprecated in 1.21, and unavailable in 1.25, however. The purpose of this PR was to avoid the following API warning messages that are displayed when running CoreDNS on kube 1.21:

W0409 15:57:51.457834       1 warnings.go:70] discovery.k8s.io/v1beta1 EndpointSlice is deprecated in v1.21+, unavailable in v1.25+; use discovery.k8s.io/v1 EndpointSlice

Would it be too much trouble to conditionally use v1beta1, or v1, dependent on the cluster's kubernetes version?

@chrisohaver
Copy link
Member

Would it be too much trouble to conditionally use v1beta1, or v1, dependent on the cluster's kubernetes version?

Playing with it a bit, it's looking quite messy to do so, although there may be a more graceful workaround I dont see yet.

@sgreene570
Copy link
Contributor Author

Would it be too much trouble to conditionally use v1beta1, or v1, dependent on the cluster's kubernetes version?

Playing with it a bit, it's looking quite messy to do so, although there may be a more graceful workaround I dont see yet.

+1

Should we look into suppressing the API warning instead (for the time being)? Anyone else have any thoughts?

@chrisohaver
Copy link
Member

FWIW, here is what I have for changes to support both versions... master...chrisohaver:epslicev1

There's probably a super simple/obvious api-versiony alternative to the above.

@sgreene570
Copy link
Contributor Author

FWIW, here is what I have for changes to support both versions... master...chrisohaver:epslicev1

There's probably a super simple/obvious api-versiony alternative to the above.

Your proposed changes look reasonable to me.

I don't think client-go has any kind of "use the newest available API version for this group, please and thank you" logic available, sadly. But I could be wrong.

@sgreene570
Copy link
Contributor Author

FWIW, here is what I have for changes to support both versions... master...chrisohaver:epslicev1

There's probably a super simple/obvious api-versiony alternative to the above.

@chrisohaver would you mind turning your WIP commit into a PR, and we can close this PR in favor of that one? 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants