Skip to content

Add Service TrafficDistribution to TopologyAwareRouting #10973

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Add Service TrafficDistribution to TopologyAwareRouting #10973

wants to merge 3 commits into from

Conversation

dergeberl
Copy link
Contributor

@dergeberl dergeberl commented Dec 4, 2024 •
edited by ialidzhikov
Loading

How to categorize this PR?

/area networking
/area high-availability
/kind enhancement

cc @ialidzhikov

What this PR does / why we need it:
With kubernetes version 1.31.x the ServiceTrafficDistribution got beta and enabled by default. ServiceTrafficDistribution PreferClose only uses zone information for routing th traffic without taking into account the allocatable CPUs see also Comparison with service.kubernetes.io/topology-mode: Auto

This PR use the ServiceTrafficDistribution instead of TopologyAwareHints with the GRM webhook, if topologyAwareRouting is enabled and kubernetes version is 1.31 or above.

The function was renamed from ReconcileTopologyAwareRoutingMetadata to ReconcileTopologyAwareRouting as this function now also changes settings in the spec.

Which issue(s) this PR fixes:
Part of #10421

Special notes for your reviewer:

Release note:

Use `ServiceTrafficDistribution` for `TopologyAwareRouting` on kubernetes version 1.31. or above.

Rename function in gardenerutils from `ReconcileTopologyAwareRoutingMetadata` to `ReconcileTopologyAwareRouting`.

@gardener-prow gardener-prow bot added area/networking Networking related area/high-availability High availability related kind/enhancement Enhancement, improvement, extension labels Dec 4, 2024
@gardener-prow gardener-prow bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 4, 2024
@ialidzhikov
Copy link
Member

/assign

@gardener-prow gardener-prow bot added cla: no Indicates the PR's author has not signed the cla-assistant.io CLA. cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. and removed cla: no Indicates the PR's author has not signed the cla-assistant.io CLA. labels Dec 4, 2024
ialidzhikov
ialidzhikov reviewed Dec 17, 2024
View reviewed changes
Copy link
Member

@ialidzhikov ialidzhikov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi @dergeberl ,

Thanks for the PR! 🚀

Sorry for the delay from my side for yet another time.

I had a look into the PR:

  • What was the concern not to disable the endpoint-slice-hints webhook starting Kubernetes 1.31? Was is it only that during the 1.30 -> 1.31 upgrade for a Seed the webhook will be disabled right away but it will take 24h until all Services reconcile? If that's the case, then I think this is something we can tolerate and live with. The topology-aware routing is a cost-optimization feature, it should be fine if for 24h the corresponding Services in the Seed are best-effort topology-aware and not 100% topology-aware. Running the endpoint-slice-hints webhook also incurs costs - mainly network costs and latency in the critical path for create/update of EndpointSlices. That's why I would suggest to disable the webhook starting K8s 1.31.
  • We would need to update the docs under docs/operations/topology_aware_routing.md.
  • We could make renaming of the ReconcileTopologyAwareRoutingMetadata more graceful by introducing the new func (for example ReconcileTopologyAwareRoutingSettings) and making the old func alias of the new one. My main motivation is to give extension folks some releases time to adapt to the new func. We always received the feedback that there are many breaking changes in the extension library and in other g/g pkgs.

Let me know if you see concerns. If you are out of capacity, I can help with addressing the review feedback.

@gardener-prow gardener-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 27, 2024
@gardener-prow Gardener Prow
Copy link
Contributor

gardener-prow bot commented Dec 27, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from ialidzhikov. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@dergeberl
Copy link
Contributor Author

Hi @ialidzhikov

Thanks for the feedback.

I adopted the docs (docs/operations/topology_aware_routing.md) and added an backwards compatible alias for func ReconcileTopologyAwareRouting.

  • What was the concern not to disable the endpoint-slice-hints webhook starting Kubernetes 1.31? Was is it only that during the 1.30 -> 1.31 upgrade for a Seed the webhook will be disabled right away but it will take 24h until all Services reconcile? If that's the case, then I think this is something we can tolerate and live with. The topology-aware routing is a cost-optimization feature, it should be fine if for 24h the corresponding Services in the Seed are best-effort topology-aware and not 100% topology-aware. Running the endpoint-slice-hints webhook also incurs costs - mainly network costs and latency in the critical path for create/update of EndpointSlices. That's why I would suggest to disable the webhook starting K8s 1.31.

I am not sure about that. I would not disable the webhook until K8s version 1.32. because the rollout like you mentioned and as I have seen it is also documented that this feature can also be used by extensions. We should make sure that all extension uses the ReconcileTopologyAwareRouting(Metadata) function. Within the documentation this function is not metioned, only which settings the Service needs (I can also add this to the docs if you want). WDYT?

@gardener-prow Gardener Prow
Copy link
Contributor

gardener-prow bot commented Dec 27, 2024

@dergeberl: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-gardener-e2e-kind-ipv6 3005acb link true /test pull-gardener-e2e-kind-ipv6
pull-gardener-e2e-kind-ha-single-zone 3005acb link true /test pull-gardener-e2e-kind-ha-single-zone
pull-gardener-e2e-kind-ha-multi-zone 3005acb link true /test pull-gardener-e2e-kind-ha-multi-zone
pull-gardener-e2e-kind-migration-ha-single-zone 3005acb link true /test pull-gardener-e2e-kind-migration-ha-single-zone
pull-gardener-e2e-kind 3005acb link true /test pull-gardener-e2e-kind

Full PR test history. Your PR dashboard. Command help for this repository.
Please help us cut down on flakes by linking this test failure to an open flake report or filing a new flake report if you can't find an existing one. Also see our testing guideline for how to avoid and hunt flakes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@ialidzhikov ialidzhikov self-requested a review January 6, 2025 09:25
@ialidzhikov
Copy link
Member

I am not sure about that. I would not disable the webhook until K8s version 1.32. because the rollout like you mentioned and as I have seen it is also documented that this feature can also be used by extensions. We should make sure that all extension uses the ReconcileTopologyAwareRouting(Metadata) function.

OK, let's disable the webhook starting K8s 1.32 to cover the migration concern and to give time extension to adapt to the new traffic distribution field. When the Seed K8s version is 1.32+, we disable the webhook.

@ialidzhikov ialidzhikov mentioned this pull request Jan 15, 2025
Merged
@ialidzhikov
Copy link
Member

Hi @dergeberl ,

To speed up the process, I opened #11178. It is based on your work in this PR. I added you as co-author.
I reflected our agreement to disable the endpoint-slice-hints webhook in Kubernetes 1.32+ there. Additionally, I applied some minor cosmetic changes.
I hope this is fine with you.

@ialidzhikov ialidzhikov mentioned this pull request Jan 16, 2025
Merged
@dergeberl
Copy link
Contributor Author

Will be done in PR #11178
/close

@gardener-prow gardener-prow bot closed this Jan 29, 2025
@gardener-prow Gardener Prow
Copy link
Contributor

gardener-prow bot commented Jan 29, 2025

@dergeberl: Closed this PR.

In response to this:

Will be done in PR #11178
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ary1992 ary1992 Awaiting requested review from ary1992

@LucaBernstein LucaBernstein Awaiting requested review from LucaBernstein

@ialidzhikov ialidzhikov Awaiting requested review from ialidzhikov

Assignees

@ialidzhikov ialidzhikov

Labels
area/high-availability High availability related area/networking Networking related cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. kind/enhancement Enhancement, improvement, extension size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dergeberl @ialidzhikov