xds: use locality from the connected address for load reporting #7378

townba · 2024-07-02T00:12:12Z

Add SubConnState.connectedAddress field.
Add internal.GetConnectedAddress and internal.SetConnectedAddress accessor function vars to get and set SubConnState.connectedAddress.
Set the connected address in acBalancerWrapper.updateState when connectivity state is Ready.
Read the connected address in the subconn StateListener and call updateLocalityID() based on that address's locality.
Modify addrConn to store the full resolver.Address rather than one with BalancerAttributes removed.
Modify a test to check for the new correct behavior.

Fixes #7339

RELEASE NOTES:

TBD

balancer_wrapper.go

xds/internal/balancer/clusterimpl/clusterimpl.go

balancer/balancer.go

xds/internal/balancer/clusterimpl/clusterimpl.go

easwars

I still haven't finished reviewing the changes in clientconn.go

balancer/balancer.go

balancer_wrapper.go

xds/internal/balancer/clusterimpl/clusterimpl.go

clientconn.go

codecov · 2024-07-02T20:15:01Z

Codecov Report

Attention: Patch coverage is 94.73684% with 2 lines in your changes missing coverage. Please review.

Project coverage is 81.34%. Comparing base (c9caa9e) to head (64ae662).
Report is 9 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7378      +/-   ##
==========================================
- Coverage   81.47%   81.34%   -0.13%     
==========================================
  Files         348      348              
  Lines       26753    26757       +4     
==========================================
- Hits        21796    21765      -31     
- Misses       3769     3795      +26     
- Partials     1188     1197       +9

Files	Coverage Δ
balancer/balancer.go	`95.45% <100.00%> (+1.70%)`	⬆️
balancer_wrapper.go	`88.74% <100.00%> (+2.25%)`	⬆️
clientconn.go	`93.34% <100.00%> (+0.19%)`	⬆️
xds/internal/internal.go	`73.91% <100.00%> (+2.48%)`	⬆️
xds/internal/balancer/clusterimpl/clusterimpl.go	`86.40% <85.71%> (-1.33%)`	⬇️

... and 28 files with indirect coverage changes

townba · 2024-07-02T20:34:23Z

PTAL

easwars

Looks good. Mostly minor nits this time around.

Also, please don't mark the comments as resolved. We leave that to the reviewer who posted the original comment. That way the reviewer is not required to click "show resolved" on every comment to see if they are satisfactorily addressed. Thanks.

balancer/balancer.go

clientconn.go

easwars · 2024-07-08T18:07:59Z

clientconn.go

@@ -924,12 +918,18 @@ func (ac *addrConn) connect() error {
 	return nil
 }

+func equalAddress(a, b resolver.Address) bool {


Probably makes sense to add a function level comment here saying why we are not using the resolver.Address.Equal and instead defining our own. Something as simple as:

// equalAddress returns true is a and b are considered equal. // This is different from the Equal method on the resolver.Address type // which considers all fields to determine equality. Here, we only consider // fields that are meaningful to the subConn. func equalAddress(a, b resolver.Address) bool { ... }

Renaming the function might be an even nicer improvement. equalIgnoringBalAttributes?

Done. Used equalAddressIgnoringBalAttributes so I could also have equalAddressesIgnoringBalAttributes below.

internal/internal.go

xds/internal/internal.go

internal/testutils/balancer.go

xds/internal/balancer/clusterimpl/clusterimpl.go

townba · 2024-07-08T20:23:34Z

PTAL

dfawley

Looks good overall, just a few minor style things.

dfawley · 2024-07-09T22:08:45Z

balancer/balancer.go

+	connectedAddress resolver.Address
+}
+
+func (lhs SubConnState) Equal(rhs SubConnState) bool {


Will we delete this entirely when connectedAddress is removed? If so, ignore. If not, then should this accept pointers instead?

Removed entirely.

This was only added to get this single check in xds/internal/balancer/outlierdetection/balancer_test.go to pass, but that doesn't work if it uses pointers. I've removed this in favor of adding balancer.SubConnState{} to the cmp.AllowUnexported.

dfawley · 2024-07-09T22:11:35Z

clientconn.go

+// This is different from the Equal method on the resolver.Address type which
+// considers all fields to determine equality. Here, we only consider fields
+// that are meaningful to the subConn.
+func equalAddress(a, b resolver.Address) bool {


These should probably be pointers to avoid the copy.

dfawley · 2024-07-09T22:13:23Z

clientconn.go

@@ -924,12 +918,18 @@ func (ac *addrConn) connect() error {
 	return nil
 }

+func equalAddress(a, b resolver.Address) bool {


Renaming the function might be an even nicer improvement. equalIgnoringBalAttributes?

dfawley · 2024-07-09T22:15:00Z

balancer_wrapper.go

-		acbw.stateListener(balancer.SubConnState{ConnectivityState: s, ConnectionError: err})
+		scs := balancer.SubConnState{ConnectivityState: s, ConnectionError: err}
+		if s == connectivity.Ready {
+			if sca, ok := internal.SetConnectedAddress.(func(*balancer.SubConnState, resolver.Address)); ok {


Please define this as a global instead.

var setConnectedAddress = internal.SetConnectedAddress.(func(*.........))

That way we don't have a conditional here and a runtime unknown. Instead it's an init time assertion.

dfawley · 2024-07-09T22:16:04Z

xds/internal/balancer/clusterimpl/clusterimpl.go

+		b.updateSubConnState(sc, state, oldListener)
+		// Read connected address and call updateLocalityID() based on the connected
+		// address's locality. https://github.com/grpc/grpc-go/issues/7339
+		if gca, ok := internal.ConnectedAddress.(func(balancer.SubConnState) (resolver.Address, bool)); ok {


Same comment here -- remove the conditional and make this global, please.

dfawley · 2024-07-09T22:19:05Z

balancer/balancer.go

+// The second return value is set to to false if the state is not READY, and the
+// first return value is meaningless in this case.


Why is anyone even calling this if the state isn't ready? Can we just say it's only valid if READY to simplify things a bit?

dfawley · 2024-07-09T22:22:15Z

xds/internal/balancer/clusterimpl/clusterimpl.go

+			if addr, ok := gca(state); ok {
+				lID := xdsinternal.GetLocalityID(addr)
+				if !lID.Empty() {
+					scw.updateLocalityID(lID)


Go style: let's try to avoid all this nesting:

StateListener = func... { b.updateSubConnState(...) if state != Ready { return } locality := xdsinternal.GetLocalityID(getConnectedAddress(state)) if locality.Empty() { if logger.V(2) { log } return } scw.updateLocalityID(locality) }

…#7378)

xds: Use the connected address for locality (grpc#7357)

08ebd15

purnesh42H requested a review from arjan-bal July 2, 2024 03:52

purnesh42H assigned arjan-bal Jul 2, 2024

purnesh42H added P1 Type: Bug labels Jul 2, 2024

arjan-bal reviewed Jul 2, 2024

View reviewed changes

arjan-bal assigned townba and unassigned arjan-bal Jul 2, 2024

easwars changed the title ~~xds: Use the connected address for locality~~ xds: use locality from the connected address for load reporting Jul 2, 2024

easwars reviewed Jul 2, 2024

View reviewed changes

clientconn.go Outdated Show resolved Hide resolved

clientconn.go Outdated Show resolved Hide resolved

clientconn.go Outdated Show resolved Hide resolved

clientconn.go Outdated Show resolved Hide resolved

Address review comments grpc#1

32d869b

townba added 2 commits July 2, 2024 13:16

Remove unused function

5070034

Reflow comment.

8709e78

Remove setting locality in NewSubConn.

18bce36

arjan-bal mentioned this pull request Jul 4, 2024

pickfirst: Use one subConn per address #7384

Closed

arjan-bal assigned easwars Jul 4, 2024

easwars reviewed Jul 8, 2024

View reviewed changes

easwars added this to the 1.66 Release milestone Jul 8, 2024

easwars removed their assignment Jul 8, 2024

townba added 3 commits July 8, 2024 12:50

Address review comments (2/x).

69acfcf

Address review comments (3/x).

66478fb

Address review comments (4/x).

c5a4ade

easwars self-assigned this Jul 9, 2024

easwars approved these changes Jul 9, 2024

View reviewed changes

easwars assigned dfawley and unassigned townba Jul 9, 2024

dfawley reviewed Jul 9, 2024

View reviewed changes

Address review comments (5/x).

64ae662

dfawley approved these changes Jul 10, 2024

View reviewed changes

dfawley removed their assignment Jul 10, 2024

easwars approved these changes Jul 10, 2024

View reviewed changes

easwars merged commit 9c5b31d into grpc:master Jul 10, 2024
12 of 13 checks passed

townba deleted the connectedaddr branch July 10, 2024 23:24

printchard pushed a commit to printchard/grpc-go that referenced this pull request Jul 11, 2024

xds: use locality from the connected address for load reporting (grpc…

9be388f

…#7378)

printchard pushed a commit to printchard/grpc-go that referenced this pull request Jul 30, 2024

xds: use locality from the connected address for load reporting (grpc…

a6c8a32

…#7378)

printchard pushed a commit to printchard/grpc-go that referenced this pull request Jul 30, 2024

xds: use locality from the connected address for load reporting (grpc…

f360f15

…#7378)

tbg mentioned this pull request Nov 27, 2024

DEPS: upgrade grpc to v1.68.0 cockroachdb/cockroach#136278

Draft

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xds: use locality from the connected address for load reporting #7378

xds: use locality from the connected address for load reporting #7378

townba commented Jul 2, 2024 •

edited by easwars

Loading

easwars left a comment

codecov bot commented Jul 2, 2024 •

edited

Loading

townba commented Jul 2, 2024

easwars left a comment

easwars Jul 8, 2024

townba Jul 8, 2024

dfawley Jul 9, 2024

townba Jul 10, 2024 •

edited

Loading

townba commented Jul 8, 2024

dfawley left a comment

dfawley Jul 9, 2024

townba Jul 10, 2024

dfawley Jul 9, 2024

townba Jul 10, 2024

dfawley Jul 9, 2024

dfawley Jul 9, 2024

townba Jul 10, 2024

dfawley Jul 9, 2024

townba Jul 10, 2024

dfawley Jul 9, 2024

townba Jul 10, 2024

dfawley Jul 9, 2024

townba Jul 10, 2024

		// The second return value is set to to false if the state is not READY, and the
		// first return value is meaningless in this case.

xds: use locality from the connected address for load reporting #7378

xds: use locality from the connected address for load reporting #7378

Conversation

townba commented Jul 2, 2024 • edited by easwars Loading

easwars left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 2, 2024 • edited Loading

Codecov Report

townba commented Jul 2, 2024

easwars left a 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

townba Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

townba commented Jul 8, 2024

dfawley left a 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

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

townba commented Jul 2, 2024 •

edited by easwars

Loading

codecov bot commented Jul 2, 2024 •

edited

Loading

townba Jul 10, 2024 •

edited

Loading