ringhash: implement gRFC A76 #8159

atollena · 2025-03-11T09:27:29Z

Fixes #8147, #7990.

Add the ability to set the request hash key, to extract the hash from a header. This allows using ring hash without xDS.
Add the ability to specify the location of endpoints on the ring, and a default implementation based on EDS metadata that matches Envoy behavior.

See https://github.com/grpc/proposal/blob/master/A76-ring-hash-improvements.md for details.

RELEASE NOTES:

ringhash: implement gRFC A76.

codecov · 2025-03-11T09:35:52Z

Codecov Report

Attention: Patch coverage is 97.45223% with 4 lines in your changes missing coverage. Please review.

Project coverage is 81.97%. Comparing base (a51009d) to head (8816ae5).
Report is 13 commits behind head on master.

Files with missing lines	Patch %	Lines
internal/testutils/xds/e2e/clientresources.go	75.00%	1 Missing and 1 partial ⚠️
xds/internal/balancer/ringhash/picker.go	96.42%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8159      +/-   ##
==========================================
+ Coverage   81.91%   81.97%   +0.06%     
==========================================
  Files         410      412       +2     
  Lines       40216    40498     +282     
==========================================
+ Hits        32942    33198     +256     
- Misses       5894     5912      +18     
- Partials     1380     1388       +8

Files with missing lines	Coverage Δ
internal/envconfig/envconfig.go	`100.00% <ø> (ø)`
internal/metadata/metadata.go	`82.08% <100.00%> (+1.44%)`	⬆️
internal/testutils/envconfig.go	`100.00% <100.00%> (ø)`
resolver/ringhash/attr.go	`100.00% <100.00%> (ø)`
...internal/balancer/clusterresolver/configbuilder.go	`91.22% <100.00%> (+0.05%)`	⬆️
xds/internal/balancer/ringhash/config.go	`92.10% <100.00%> (+3.21%)`	⬆️
xds/internal/balancer/ringhash/ring.go	`100.00% <100.00%> (ø)`
xds/internal/balancer/ringhash/ringhash.go	`94.22% <100.00%> (+0.35%)`	⬆️
xds/internal/balancer/ringhash/util.go	`100.00% <100.00%> (ø)`
xds/internal/resolver/serviceconfig.go	`85.71% <100.00%> (ø)`
... and 3 more

... and 37 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- Add the ability to set the request hash header. This allows using ring hash without xDS. - Add the ability to specify the location of endpoints on the ring, and a default implementation based on EDS metadata that matches Envoy behavior. See https://github.com/grpc/proposal/blob/master/A76-ring-hash-improvements.md for details. Release Notes: - ringhash: implement gRFC A76.

danielzhaotongliu

Perhaps @easwars or @dfawley should also take a pass at the review if they have time.

internal/envconfig/envconfig.go

internal/testutils/xds/e2e/clientresources.go

xds/internal/balancer/ringhash/config.go

danielzhaotongliu · 2025-03-17T21:56:29Z

xds/internal/balancer/ringhash/config_test.go

@@ -94,6 +98,27 @@ func (s) TestParseConfig(t *testing.T) {
 			want:    nil,
 			wantErr: true,
 		},
+		{
+			name: "request metadata key set",


there should be a test case for when there are multiple header values from the gRFC A76

If the header contains multiple values, then values are joined with a comma , character before hashing.

The behavior joining headers with a coma is for header values. Using multiple headers is not supported. I added a picker test case for this, and took the opportunity to make the picker test a bit more robust by distributing endpoints on the ring, and removing unnecessary setting of the request hash in existing tests.

Can we have a test for the case where the env var is set to false and the JSON config contains a value for the request_hash_header and we check that the parsed internal representation does not have the corresponding field set?

The behavior joining headers with a coma is for header values. Using multiple headers is not supported. I added a picker test case for this

Ack. Thanks for adding this test.

can we have a test for the case where the env var is set to false and the JSON config contains a value for the request_hash_header and we check that the parsed internal representation does not have the corresponding field set?

Added.

Nit: Since we have multiple env vars in the context of ring_hash config parsing, could we please rename the field something more specific. Maybe requestHeaderEnvVar?

xds/internal/balancer/ringhash/config.go

atollena · 2025-03-21T13:34:08Z

Thanks for the review @danielzhaotongliu . I took your feedback into consideration.

There is a failing test which I think may be flaky? (it's Test/End2End in the advancedtls module which seems unrelated).
Also the code coverage report didn't run because of a missing token, not sure if that's expected.

xds/internal/balancer/ringhash/config.go

resolver/ringhash/attr.go

arjan-bal · 2025-03-25T19:02:54Z

resolver/ringhash/attr.go

+//
+// Notice: This API is EXPERIMENTAL and may be changed or removed in a
+// later release.
+func SetEndpointHashKey(ep resolver.Endpoint, hashKey string) resolver.Endpoint {


nit: We can remove Endpoint from the function name since the function argument type conveys this information.

Nit: s/hash key attribute of addr/hash key attribute of endpoint/

resolver/ringhash/attr.go

xds/internal/balancer/ringhash/ringhash.go

xds/internal/resolver/serviceconfig.go

internal/envconfig/envconfig.go

internal/testutils/xds/e2e/clientresources.go

resolver/ringhash/attr.go

easwars · 2025-03-26T00:02:13Z

resolver/ringhash/attr.go

+//
+// Notice: This API is EXPERIMENTAL and may be changed or removed in a
+// later release.
+func SetEndpointHashKey(ep resolver.Endpoint, hashKey string) resolver.Endpoint {


xds/internal/balancer/ringhash/picker_test.go

xds/internal/balancer/ringhash/e2e/ringhash_balancer_test.go

xds/internal/balancer/ringhash/config.go

danielzhaotongliu · 2025-03-26T01:26:40Z

xds/internal/balancer/ringhash/config_test.go

@@ -94,6 +98,27 @@ func (s) TestParseConfig(t *testing.T) {
 			want:    nil,
 			wantErr: true,
 		},
+		{
+			name: "request metadata key set",


The behavior joining headers with a coma is for header values. Using multiple headers is not supported. I added a picker test case for this

Ack. Thanks for adding this test.

xds/internal/balancer/ringhash/e2e/ringhash_balancer_test.go

xds/internal/balancer/ringhash/ringhash.go

internal/testutils/xds/e2e/clientresources.go

xds/internal/balancer/ringhash/util.go

atollena · 2025-03-27T17:35:56Z

I also fixed the configuration to use camelCase instead of snake_case: requestHashHeader instead of request_hash_header, since this is the convention for service config (it's somewhat hidden in the spec since it relies on protojson conventions).

arjan-bal · 2025-04-03T08:03:10Z

There is a race in a ringhash test caught by CI: https://github.com/grpc/grpc-go/actions/runs/14220675758/job/39847655816?pr=8159

atollena · 2025-04-03T08:04:39Z

There is a race in a ringhash test caught by CI: https://github.com/grpc/grpc-go/actions/runs/14220675758/job/39847655816?pr=8159

Yep, that was just the err variable local to the test. Fixed in the latest commit.

atollena · 2025-04-03T08:32:09Z

I also opened grpc/proposal#489 to clarify some of the behaviors I discovered during the implementation. It also answers the concern from @arjan-bal regarding duplicate hash keys.

xds/internal/balancer/ringhash/e2e/ringhash_balancer_test.go

arjan-bal

LGTM!

danielzhaotongliu

LGTM!

With gRFC A76 (grpc#8159), when requestHashHeader is specified from the service config it fails the validation since MD keys with uppercase letters are normalized to lowercase. We should normalize the parsed value before validation.

atollena requested a review from arjan-bal March 11, 2025 09:27

atollena assigned arjan-bal Mar 11, 2025

atollena added P1 Type: Feature New features or improvements in behavior Area: xDS Includes everything xDS related, including LB policies used with xDS. Area: Resolvers/Balancers Includes LB policy & NR APIs, resolver/balancer/picker wrappers, LB policy impls and utilities. labels Mar 11, 2025

atollena added this to the 1.72 Release milestone Mar 11, 2025

atollena force-pushed the A76 branch from 3acd68d to 573e5e4 Compare March 11, 2025 09:30

atollena force-pushed the A76 branch from 573e5e4 to bf65b8c Compare March 11, 2025 09:47

atollena force-pushed the A76 branch from bf65b8c to 5f56c4c Compare March 11, 2025 10:22

danielzhaotongliu reviewed Mar 17, 2025

View reviewed changes

dfawley assigned atollena and unassigned arjan-bal Mar 19, 2025

dfawley added the Status: Requires Reporter Clarification label Mar 19, 2025

feedback from Daniel

db645b6

atollena removed their assignment Mar 21, 2025

atollena removed the Status: Requires Reporter Clarification label Mar 21, 2025

Merge branch 'master' into A76

2bfd696

arjan-bal self-assigned this Mar 25, 2025

arjan-bal reviewed Mar 25, 2025

View reviewed changes

arjan-bal assigned atollena and unassigned arjan-bal Mar 25, 2025

easwars reviewed Mar 26, 2025

View reviewed changes

danielzhaotongliu reviewed Mar 26, 2025

View reviewed changes

atollena added 2 commits March 27, 2025 11:04

feedback from Arjan, Easwar, Daniel

132b401

request_hash_header -> requestHashHeader

8ac641b

atollena mentioned this pull request Apr 3, 2025

A76 update: Clarify behaviors discovered during implementation grpc/proposal#489

Open

eshitachandwani modified the milestones: 1.72 Release, 1.73 Release Apr 3, 2025

arjan-bal reviewed Apr 3, 2025

View reviewed changes

xds/internal/balancer/ringhash/e2e/ringhash_balancer_test.go Outdated Show resolved Hide resolved

arjan-bal reviewed Apr 3, 2025

View reviewed changes

xds/internal/balancer/ringhash/e2e/ringhash_balancer_test.go Show resolved Hide resolved

more comments from Arjan

febd62a

atollena assigned easwars and arjan-bal and unassigned atollena Apr 4, 2025

arjan-bal approved these changes Apr 4, 2025

View reviewed changes

arjan-bal removed their assignment Apr 4, 2025

danielzhaotongliu reviewed Apr 4, 2025

View reviewed changes

missing fixes for comments from easwar

8816ae5

easwars approved these changes Apr 7, 2025

View reviewed changes

easwars assigned atollena and unassigned easwars Apr 7, 2025

arjan-bal merged commit 25c7509 into grpc:master Apr 8, 2025
14 of 15 checks passed

yousukseung mentioned this pull request Apr 11, 2025

ringhash: normalize uppercase in requestHashHeader from service config #8243

Merged

purnesh42H pushed a commit to purnesh42H/grpc-go that referenced this pull request Apr 13, 2025

ringhash: implement gRFC A76 (grpc#8159)

69f23ae

purnesh42H pushed a commit to purnesh42H/grpc-go that referenced this pull request Apr 16, 2025

ringhash: implement gRFC A76 (grpc#8159)

7c1b5af

purnesh42H mentioned this pull request Apr 16, 2025

Cherry-pick #8159 and #8243 to v1.72.x #8255

Merged

purnesh42H added a commit that referenced this pull request Apr 16, 2025

Cherry-pick #8159 and #8243 to v1.72.x (#8255)

fd6f585

arjan-bal mentioned this pull request Apr 17, 2025

ringhash: LB policy must fail RPCs if the request hash is absent in the RPC context #7990

Closed

janardhanvissa pushed a commit to janardhanvissa/grpc-go that referenced this pull request Apr 23, 2025

ringhash: implement gRFC A76 (grpc#8159)

f372962

ringhash: implement gRFC A76 #8159

ringhash: implement gRFC A76 #8159

Uh oh!

Conversation

atollena commented Mar 11, 2025 • edited by easwars Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

danielzhaotongliu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

atollena commented Mar 21, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

atollena commented Mar 27, 2025

Uh oh!

arjan-bal commented Apr 3, 2025

Uh oh!

atollena commented Apr 3, 2025

Uh oh!

atollena commented Apr 3, 2025

Uh oh!

Uh oh!

Uh oh!

arjan-bal left a comment

Choose a reason for hiding this comment

Uh oh!

danielzhaotongliu left a comment

atollena commented Mar 11, 2025 •

edited by easwars

Loading

codecov bot commented Mar 11, 2025 •

edited

Loading