Skip to content

Conversation

@arjan-bal
Copy link
Contributor

@arjan-bal arjan-bal commented Sep 25, 2025

The new pick_first, which is the default, doesn't shuffle the addresses at all for resolver updates that are missing the Endpoints field. This change fixes that. Since gRPC automatically sets the the missing Endpoints, occurrence of this bug should be uncommon in practice.

RELEASE NOTES:

  • balancer/pick_first: When configured, shuffle addresses in resolver updates that lack endpoints. Since gRPC automatically adds endpoints to resolver updates, this bug should only affect implementers of custom LB policies that use pick_first for delegation but don't forward the endpoints.
@arjan-bal arjan-bal added Type: Bug Area: Resolvers/Balancers Includes LB policy & NR APIs, resolver/balancer/picker wrappers, LB policy impls and utilities. labels Sep 25, 2025
@arjan-bal arjan-bal added this to the 1.77 Release milestone Sep 25, 2025
@codecov
Copy link

codecov bot commented Sep 25, 2025

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 82.16%. Comparing base (8ae3c07) to head (1adccbc).
⚠️ Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
balancer/pickfirst/pickfirst.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8610      +/-   ##
==========================================
+ Coverage   82.11%   82.16%   +0.04%     
==========================================
  Files         415      415              
  Lines       40692    40697       +5     
==========================================
+ Hits        33416    33439      +23     
+ Misses       5887     5881       -6     
+ Partials     1389     1377      -12     
Files with missing lines Coverage Δ
balancer/pickfirst/pickfirstleaf/pickfirstleaf.go 88.25% <100.00%> (+0.58%) ⬆️
balancer/pickfirst/pickfirst.go 34.56% <0.00%> (ø)

... and 23 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
}

pfBuilder := balancer.Get(pfbalancer.Name)
shffleConfig, err := pfBuilder.(balancer.ConfigParser).ParseConfig(json.RawMessage(`{ "shuffleAddressList": true }`))
Copy link
Contributor

Choose a reason for hiding this comment

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

Lol, here and in the next statement, prefer adding the u back into shuffle. Saving one character in the variable name is not going to be very useful :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to using complete words.

if err != nil {
t.Fatal(err)
}
activeCfg := noShffleConfig
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: remove this line since this assignment is anyways happening on line 521.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to only declaring the variable, but not initializing here.

@easwars easwars assigned arjan-bal and unassigned easwars and dfawley Sep 25, 2025
@arjan-bal arjan-bal merged commit bb71072 into grpc:master Sep 26, 2025
15 checks passed
@arjan-bal arjan-bal deleted the pf-addr-shuffle-bug branch September 26, 2025 05:38
arjan-bal added a commit to arjan-bal/grpc-go that referenced this pull request Oct 1, 2025
… endpoints (grpc#8610)

The new `pick_first`, which is the default, doesn't shuffle the
addresses at all for resolver updates that are missing the `Endpoints`
field. This change fixes that. Since [gRPC automatically sets the the
missing
`Endpoints`](https://github.com/grpc/grpc-go/blob/1059e84f885bf7ed65b3b1a4fbe914360d8ab5b1/resolver_wrapper.go#L136-L138),
occurrence of this bug should be uncommon in practice.

RELEASE NOTES:
* balancer/pick_first: When configured, shuffle addresses in resolver
updates that lack endpoints. Since gRPC automatically adds endpoints to
resolver updates, this bug should only affect implementers of custom LB
policies that use pick_first for delegation but don't forward the
endpoints.
arjan-bal added a commit that referenced this pull request Oct 3, 2025
Original PRs: #8610, #8615

RELEASE NOTES:
* balancer/pick_first:
* Fix bug that can cause balancer to get stuck in `IDLE` state on
backend address change.
* When configured, shuffle addresses in resolver updates that lack
endpoints. Since gRPC automatically adds endpoints to resolver updates,
this bug should only affect implementers of custom LB policies that use
pick_first for delegation but don't forward the endpoints.
Pranjali-2501 pushed a commit to Pranjali-2501/grpc-go that referenced this pull request Oct 6, 2025
… endpoints (grpc#8610)

The new `pick_first`, which is the default, doesn't shuffle the
addresses at all for resolver updates that are missing the `Endpoints`
field. This change fixes that. Since [gRPC automatically sets the the
missing
`Endpoints`](https://github.com/grpc/grpc-go/blob/1059e84f885bf7ed65b3b1a4fbe914360d8ab5b1/resolver_wrapper.go#L136-L138),
occurrence of this bug should be uncommon in practice.

RELEASE NOTES:
* balancer/pick_first: When configured, shuffle addresses in resolver
updates that lack endpoints. Since gRPC automatically adds endpoints to
resolver updates, this bug should only affect implementers of custom LB
policies that use pick_first for delegation but don't forward the
endpoints.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: Resolvers/Balancers Includes LB policy & NR APIs, resolver/balancer/picker wrappers, LB policy impls and utilities. Type: Bug

3 participants