feat: support topology aware hints #9165

tombokombo · 2022-10-14T22:55:36Z

What this PR does / why we need it:

This PR brings support for topology aware hints service annotation. If annotation is present on service and all conditions are met ( see safeguards in doc ), ingress controller pod will try to use endpoint slices zone hints.
Feature is enabled with service annotation service.kubernetes.io/topology-aware-hints: "auto".
Follow up to #8890
Will work only on not "scoped" controller deployment. It needs a clusterrole to access node, which is running controller pod to extract topology labels. For scoped deployment, it just print warning about inaccessible NODE info and will work as you don't have topology labeled cluster, which means no hints.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
CVE Report (Scanner found CVE and adding report)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation only

Which issue/s this PR fixes

How Has This Been Tested?

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have added unit and/or e2e tests to cover my changes.
All new and existing tests passed.
Added Release Notes.

Does my pull request need a release note?

Any user-visible or operator-visible change qualifies for a release note. This could be a:

CLI change
API change
UI change
configuration schema change
behavioral change
change in non-functional attributes such as efficiency or availability, availability of a new platform
a warning about a deprecation
fix of a previous Known Issue
fix of a vulnerability (CVE)

No release notes are required for changes to the following:

Tests
Build infrastructure
Fixes for unreleased bugs

For more tips on writing good release notes, check out the Release Notes Handbook

Support for topology aware hints service annotation, when set to "auto" ingress pod will try to use endpointslices topology hints

k8s-ci-robot · 2022-10-14T22:55:43Z

@tombokombo: This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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/test-infra repository.

k8s-ci-robot · 2022-10-14T22:55:44Z

Hi @tombokombo. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

Signed-off-by: tombokombo <tombo@sysart.tech>

tombokombo · 2022-10-27T05:48:02Z

@ElvinEfendi @tao12345666333 could you please take a look? Thx

tao12345666333 · 2022-11-10T15:18:51Z

/assign

jensoncs · 2022-11-18T06:51:04Z

@tao12345666333 Do we have any update on the review?

Sharechat-Taran · 2022-11-21T14:16:31Z

any update?

tombokombo · 2022-11-22T16:24:14Z

@tombokombo We built using the PR but could not get it working, if possible can we connect some time so I could have more clarity?

You have to use cluster role, helm chart rbac.scope: false, check controller logs for warning Unable to get NODE information:
service needs to have annotation service.kubernetes.io/topology-aware-hints: "auto"
check endpointslices for hints

kubectl -n ${namesapce} get endpointslices  -o yaml
there must be hint with zone for every slice endpoint
...
    hints:
      forZones:
      - name: some-zone
...

How do you know that it's not working? How are you checking active endpoints for you ingress/svc?
Please provide controller logs, there are few info level messages which will be very helpful to see what's going on.

tombokombo · 2022-11-30T15:01:39Z

@tao12345666333 any update?

abhiroop93 · 2022-12-01T05:01:05Z

@tombokombo We built using the PR but could not get it working, if possible can we connect some time so I could have more clarity?

You have to use cluster role, helm chart rbac.scope: false, check controller logs for warning Unable to get NODE information:

service needs to have annotation service.kubernetes.io/topology-aware-hints: "auto"

check endpointslices for hints
kubectl -n ${namesapce} get endpointslices  -o yaml
there must be hint with zone for every slice endpoint
...
    hints:
      forZones:
      - name: some-zone
...
How do you know that it's not working? How are you checking active endpoints for you ingress/svc? Please provide controller logs, there are few info level messages which will be very helpful to see what's going on.

@tombokombo
We managed to get it working, it was an issue with GKE Dataplane V2 as it turns out (they do not support topology aware hints). Thanks!

rikatz · 2022-12-12T11:57:23Z

internal/ingress/controller/controller.go

+			}
+		}
+	}
+	return emptyZone


why defining a new var when you can just return empty? I would just do return "" and remove this var declaration

It's not defined just for one use, it appears here
https://github.com/kubernetes/ingress-nginx/pull/9165/files#diff-e2c540c6da86368f0fa693d5634db0a92e8ef119e5cc2609bd3d24baf6c0fa8aR116
I defined it for better readability, not just comparing against empty string, but against variable which name give some hint what is going on.

Now it has even more usage.

internal/ingress/controller/endpointslices.go

rikatz · 2022-12-12T12:00:01Z

internal/ingress/controller/endpointslices.go

+			}
+		}
+		if useTopologyHints {
+			klog.V(3).Infof("All endpoint slices has zone hint, using zone %q for Service %q", zoneForHints, svcKey)


you can move this log to after the for, inside the if, and get rid of this additional "if useTopologyHints" just for this logging

All other possible places are inside loops, it will spam logs depending on count of endpointslices objects.

rikatz · 2022-12-12T12:01:03Z

internal/k8s/main.go

+		TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "Node"},
+	}
+
+	node, err := kubeClient.CoreV1().Nodes().Get(context.TODO(), pod.Spec.NodeName, metav1.GetOptions{})


please add a comment here on why we need the Node information now. BTW, is this already part of the clusterrole we use in ingress? can't remember :)

Ok, I will add comment. Yes cluster role is without change and can access nodes.

rikatz

/ok-to-test
@tombokombo change seems fine, I just want to add some safeguards.

Thanks

Signed-off-by: tombokombo <tombo@sysart.tech>

brodin · 2022-12-21T12:17:00Z

So much looking forward to this @tombokombo – thanks for taking the time!

Paying a hefty preminum today on GCP with all the Inter-Zone Egress when we run multi-zonal clusters.

Signed-off-by: tombokombo <tombo@sysart.tech>

tombokombo · 2023-01-07T11:11:32Z

/retest

test/e2e/framework/deployment.go

internal/ingress/controller/controller.go

rikatz · 2023-01-08T21:55:52Z

@tombokombo left some minor comment. Looks good to me, if the helm test fails again we need to check what is going on
/approve
/hold

Signed-off-by: tombokombo <tombo@sysart.tech>

ElvinEfendi · 2023-01-10T04:25:57Z

Thanks for this feature!

Am I reading it right that an ingress-nginx pod in a zone A, will never proxy requests to endpoints outsize of zone A? Because it seems like kube-proxy can end up proxying to the endpoints outside of its zone: https://kubernetes.io/docs/concepts/services-networking/topology-aware-hints/#implementation-kube-proxy

tombokombo · 2023-01-10T11:49:36Z

@ElvinEfendi
It depends on k8s endpoint slices controller, if zone hints are provided then ingress-controller pod will build backends list only from endpoints with that zone. If there are no zone hints, ingress-controller pod fallback to normal behavior with all endpoints.

Regarding safeguard, I'm checking if all slices endpoints has hints, if no, fallback to normal.

Safeguard are mostly implemented on endpoint slice controller, not kube-proxy, see
1 Insufficient number of endpoints: ...the controller will not assign any hints....
2. Impossible to achieve balanced allocation: ...The controller does not assign hints if it can't get this "expected overload" value below an acceptable threshold for each zone....
3 One or more Nodes has insufficient information: ...the control plane does not set any topology-aware endpoint hints...

So behaviour should be same as kube-proxy has.

rikatz · 2023-01-16T02:43:50Z

/label tide/merge-method-squash
/lgtm
/approve
Let's see how this goes on v1.6 :) thanks!
/hold cancel

k8s-ci-robot · 2023-01-16T02:43:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rikatz, tombokombo

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rikatz]

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

* support topology aware hints Signed-off-by: tombokombo <tombo@sysart.tech> * add flag to enable topology and fixes Signed-off-by: tombokombo <tombo@sysart.tech> * update readme Signed-off-by: tombokombo <tombo@sysart.tech> * add e2e test Signed-off-by: tombokombo <tombo@sysart.tech> * isolate topology test Signed-off-by: tombokombo <tombo@sysart.tech> * gofmt fix Signed-off-by: tombokombo <tombo@sysart.tech> Signed-off-by: tombokombo <tombo@sysart.tech>

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 14, 2022

k8s-ci-robot added needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 14, 2022

k8s-ci-robot requested review from ElvinEfendi and tao12345666333 October 14, 2022 22:56

support topology aware hints

b7e8423

Signed-off-by: tombokombo <tombo@sysart.tech>

tombokombo force-pushed the feature/topology_zones branch from 8965346 to b7e8423 Compare October 15, 2022 02:56

k8s-ci-robot assigned tao12345666333 Nov 10, 2022

rikatz reviewed Dec 12, 2022

View reviewed changes

internal/ingress/controller/endpointslices.go Show resolved Hide resolved

rikatz reviewed Dec 12, 2022

View reviewed changes

rikatz requested changes Dec 12, 2022

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 12, 2022

add flag to enable topology and fixes

0651ac7

Signed-off-by: tombokombo <tombo@sysart.tech>

k8s-ci-robot added area/docs area/helm Issues or PRs related to helm charts labels Dec 12, 2022

tombokombo requested a review from rikatz December 12, 2022 22:52

update readme

3412833

Signed-off-by: tombokombo <tombo@sysart.tech>

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 27, 2022

Merge branch 'main' into feature/topology_zones

a5ed101

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 28, 2022

Merge branch 'kubernetes:main' into feature/topology_zones

78f277f

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 6, 2023

add e2e test

1b8a726

Signed-off-by: tombokombo <tombo@sysart.tech>

tombokombo force-pushed the feature/topology_zones branch from 0cb4a44 to 1b8a726 Compare January 7, 2023 01:28

isolate topology test

b98675b

Signed-off-by: tombokombo <tombo@sysart.tech>

rikatz reviewed Jan 8, 2023

View reviewed changes

test/e2e/framework/deployment.go Show resolved Hide resolved

rikatz reviewed Jan 8, 2023

View reviewed changes

internal/ingress/controller/controller.go Show resolved Hide resolved

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 8, 2023

tombokombo added 2 commits January 9, 2023 19:35

Merge branch 'main' into feature/topology_zones

81148e7

gofmt fix

d738528

Signed-off-by: tombokombo <tombo@sysart.tech>

k8s-ci-robot added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 16, 2023

k8s-ci-robot assigned rikatz Jan 16, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 16, 2023

k8s-ci-robot merged commit 5b2a947 into kubernetes:main Jan 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support topology aware hints #9165

feat: support topology aware hints #9165

tombokombo commented Oct 14, 2022 •

edited

k8s-ci-robot commented Oct 14, 2022

k8s-ci-robot commented Oct 14, 2022

tombokombo commented Oct 27, 2022

tao12345666333 commented Nov 10, 2022

jensoncs commented Nov 18, 2022

Sharechat-Taran commented Nov 21, 2022

tombokombo commented Nov 22, 2022

tombokombo commented Nov 30, 2022

abhiroop93 commented Dec 1, 2022

rikatz Dec 12, 2022

tombokombo Dec 12, 2022

tombokombo Dec 12, 2022

rikatz Dec 12, 2022

tombokombo Dec 12, 2022

rikatz Dec 12, 2022

tombokombo Dec 12, 2022

tombokombo Dec 12, 2022

rikatz left a comment

brodin commented Dec 21, 2022

tombokombo commented Jan 7, 2023

rikatz commented Jan 8, 2023

ElvinEfendi commented Jan 10, 2023

tombokombo commented Jan 10, 2023

rikatz commented Jan 16, 2023

k8s-ci-robot commented Jan 16, 2023

feat: support topology aware hints #9165

feat: support topology aware hints #9165

Conversation

tombokombo commented Oct 14, 2022 • edited

What this PR does / why we need it:

Types of changes

Which issue/s this PR fixes

How Has This Been Tested?

Checklist:

Does my pull request need a release note?

k8s-ci-robot commented Oct 14, 2022

k8s-ci-robot commented Oct 14, 2022

tombokombo commented Oct 27, 2022

tao12345666333 commented Nov 10, 2022

jensoncs commented Nov 18, 2022

Sharechat-Taran commented Nov 21, 2022

tombokombo commented Nov 22, 2022

tombokombo commented Nov 30, 2022

abhiroop93 commented Dec 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rikatz left a comment

Choose a reason for hiding this comment

brodin commented Dec 21, 2022

tombokombo commented Jan 7, 2023

rikatz commented Jan 8, 2023

ElvinEfendi commented Jan 10, 2023

tombokombo commented Jan 10, 2023

rikatz commented Jan 16, 2023

k8s-ci-robot commented Jan 16, 2023

tombokombo commented Oct 14, 2022 •

edited