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

Migrate to EndpointSlices API #10664

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jnoordsij
Copy link
Contributor

@jnoordsij jnoordsij commented Apr 28, 2024

What does this PR do?

Switch to Kubernetes EndpointSlices API as described in #10638.

Motivation

Discovered existence of this API while looking into behavior regarding non-ready service endpoints still being served traffic. Although this change will probably make little changes in that regard, it's probably still a technical improvement that allows for more features in the future and still might slightly improve performance.

See also:

More

  • Added/updated tests
  • Added/updated documentation

Additional Notes

Important changes

  • This will require Kubernetes >= 1.21, in which the EndpointSlice API graduated to stable; see also the integration test version bump.
  • Within pkg/provider/kubernetes/k8s/event_handler.go, the change-detection method has been heavily simplified, to use Go-native reflect for comparison. This might come at a little performance overhead, but is definitely more future-proof, as I would think this check could otherwise very well go unnoticed when the API might change in the future.

Upgrade notes

  • Kubernetes RBAC should be updated to allow API calls for endpointslices (see e.g. docs/content/getting-started/quick-start-with-kubernetes.md)
  • (Ensure cluster is of version >= 1.21)

Possible improvements

  • The Kubernetes docs mention the possibility of duplicate endpoints, which should probably be accounted for in some way here.
  • Check for additionally useful tests; e.g.
    • EndpointSlice with differing states
    • Testing EndpointSlices with multiple endpoints values

Possible follow-ups

The following ideas should now be implementable; however how easy and/or useful this is, I'm not sure about:

Also deduplication of some logic (e.g. Kubernetes resource querying, label selector creation and endpoint parsing) and/or tests would probably be something worth taking up here, although I have not thought about if and how to achieve this.

@nmengin
Copy link
Contributor

nmengin commented Apr 29, 2024

Hello @jnoordsij,

Thank you for your contribution.

I can see your PR is in draft.
Is it yet in a readable state? Do you want us to start the review or do you prefer waiting for the full implementation to be done?

Thanks in advance for your feedback.

@jnoordsij
Copy link
Contributor Author

Any early comments or suggestions are welcome, but given some tests are still failing and I'm aiming to write down a more descriptive summary and list of important changes, a thorough review is probably something to only do at a later point.

I'll hope to continue working on this soon. Yet like I said, any comments, concerns or suggestions are welcome in the meantime!

@nmengin
Copy link
Contributor

nmengin commented May 2, 2024

Hello @jnoordsij,

Thank you for the answer.
We are waiting for your go before starting the review 😄

@jnoordsij jnoordsij marked this pull request as ready for review May 18, 2024 16:08
@jnoordsij
Copy link
Contributor Author

@nmengin I think this is all ready for a first review right now!

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

Successfully merging this pull request may close these issues.

None yet

4 participants