xds/xdsclient: create LRSClient at time of initialisation #8483

eshitachandwani · 2025-08-01T08:14:12Z

The race is in ReportLoad function of clientImpl. The implementation was recently changed as the part of xds client migration.

The comment says that lrsclient.LRSClient should be initialized only at creation time but that was not the case. It was being initialized at the time of calling ReportLoad function.

RELEASE NOTES: N/A

codecov · 2025-08-01T08:19:13Z

Codecov Report

❌ Patch coverage is 80.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.46%. Comparing base (9186ebd) to head (90bc53c).
⚠️ Report is 6 commits behind head on master.

Files with missing lines	Patch %	Lines
xds/internal/xdsclient/clientimpl.go	80.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8483      +/-   ##
==========================================
- Coverage   82.47%   82.46%   -0.02%     
==========================================
  Files         413      413              
  Lines       40513    40537      +24     
==========================================
+ Hits        33415    33429      +14     
- Misses       5743     5748       +5     
- Partials     1355     1360       +5

Files with missing lines	Coverage Δ
xds/internal/xdsclient/clientimpl_loadreport.go	`76.92% <ø> (+4.19%)`	⬆️
xds/internal/xdsclient/clientimpl.go	`82.05% <80.00%> (-0.48%)`	⬇️

... and 18 files with indirect coverage changes

🚀 New features to boost your workflow:

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

xds/internal/xdsclient/tests/loadreport_test.go

arjan-bal · 2025-08-01T08:20:11Z

xds/internal/xdsclient/tests/loadreport_test.go

+	for i := 0; i < numGoroutines; i++ {
+		go func() {
+			store, cancelStore := client.ReportLoad(serverConfig)
+			if store != nil {


Why do we need this check? Why is the store nil?

I can return nil in case of error, but in that case the cancelStore function will be a no-op function , so we can skip this check.

Do we expect an error to be returned in the test? If not, we should error log it to fail the test. My concern is that the current check may hide failures.

Right! Got your point. It will return nil in case that the XDSClient does not have lrsclient field set. So I think it makes sense to log an error if it is not set , because we want to set it unconditionally at time of creation. Does that sound good?

I had a look at the code again and I feel that we should get rid of the nil check entirely. We set the field at object creation time, so it should be impossible for it to be nil.

Got it! Done.

xds/internal/xdsclient/clientimpl_loadreport.go

xds/internal/xdsclient/clientimpl.go

arjan-bal · 2025-08-01T09:35:42Z

xds/internal/xdsclient/tests/loadreport_test.go

+	for i := 0; i < numGoroutines; i++ {
+		go func() {
+			store, cancelStore := client.ReportLoad(serverConfig)
+			if store != nil {


I had a look at the code again and I feel that we should get rid of the nil check entirely. We set the field at object creation time, so it should be impossible for it to be nil.

purnesh42H · 2025-08-01T18:30:08Z

xds/internal/xdsclient/clientimpl.go

+		TransportBuilder: gConfig.TransportBuilder,
+	})
+	if err != nil {
+		return nil, err


Now that we have moved it out of report load, we might have think a bit more here. Should error in lrs client creation be fatal? because not everyone is going to use internal xdsclient for load reporting.

In my opinion, we should keep the same behaviour as before the xDS client migration changes. If there are certain users who need to ignore LRS client creation failures, we can create a new issue to discuss if the bahviour changes makes sense.

I think it makes sense to retain the behavior of making LRS client creation failures be fatal. But we might want to change one minor thing in lrsclient.New. Currently it fails if node ID is empty in the configuration. We recently removed that check for the xDS client creation. I'm guessing other languages might not treat this as fatal for LRS creation.

@eshitachandwani : Could you please check what the other languages do and if required remove the check for empty node ID in lrsclient.New. Thanks.

If I understand correctly , Java is checking for not null here

That check is for the whole node proto or struct. Not just the node ID field.

easwars · 2025-08-04T16:16:46Z

xds/internal/xdsclient/clientimpl.go

+		TransportBuilder: gConfig.TransportBuilder,
+	})
+	if err != nil {
+		return nil, err


I think it makes sense to retain the behavior of making LRS client creation failures be fatal. But we might want to change one minor thing in lrsclient.New. Currently it fails if node ID is empty in the configuration. We recently removed that check for the xDS client creation. I'm guessing other languages might not treat this as fatal for LRS creation.

@eshitachandwani : Could you please check what the other languages do and if required remove the check for empty node ID in lrsclient.New. Thanks.

xds/internal/xdsclient/clientimpl.go

easwars · 2025-08-04T16:20:48Z

xds/internal/xdsclient/tests/loadreport_test.go

+	for i := 0; i < numGoroutines; i++ {
+		go func() {
+			defer wg.Done()
+			_, cancelStore := client.ReportLoad(serverConfig)


Would it make sense to have a loop here as well?

I am not sure if I understand, loop for what?

A loop inside the goroutine to start reporting load and subsequently canceling it. What I'm asking for is:

for i := 0; i < numGoroutines; i++ { go func() { defer wg.Done() for j := 0; j < 100; j++ { _, cancelStore := client.ReportLoad(serverConfig) cancelStore(ctx) } }() }

eshitachandwani added 2 commits July 30, 2025 13:40

create lrs at creation time

e035e08

test race

339ac57

eshitachandwani added this to the 1.75 Release milestone Aug 1, 2025

eshitachandwani added Type: Bug Area: xDS Includes everything xDS related, including LB policies used with xDS. labels Aug 1, 2025

arjan-bal reviewed Aug 1, 2025

View reviewed changes

eshitachandwani added 2 commits August 1, 2025 13:57

add waitgroup

8f53b14

error

56d3a58

eshitachandwani requested a review from arjan-bal August 1, 2025 09:13

eshitachandwani assigned arjan-bal Aug 1, 2025

eshitachandwani requested a review from easwars August 1, 2025 09:32

eshitachandwani assigned easwars Aug 1, 2025

arjan-bal reviewed Aug 1, 2025

View reviewed changes

arjan-bal assigned eshitachandwani and unassigned arjan-bal Aug 1, 2025

arjan-bal modified the milestones: 1.75 Release, 1.74 Release Aug 1, 2025

remove nil checks

2f6d3b4

eshitachandwani changed the title ~~create lrs at creation time~~ xds/xdsclient: create LRSClient at time of initialisation Aug 1, 2025

eshitachandwani requested a review from arjan-bal August 1, 2025 15:31

eshitachandwani assigned arjan-bal and unassigned eshitachandwani Aug 1, 2025

purnesh42H reviewed Aug 1, 2025

View reviewed changes

easwars reviewed Aug 4, 2025

View reviewed changes

easwars assigned eshitachandwani and unassigned easwars and arjan-bal Aug 4, 2025

format

90bc53c

eshitachandwani requested a review from easwars August 5, 2025 12:39

eshitachandwani assigned easwars and arjan-bal and unassigned eshitachandwani Aug 5, 2025

easwars assigned eshitachandwani and unassigned easwars and arjan-bal Aug 6, 2025

xds/xdsclient: create LRSClient at time of initialisation #8483

Are you sure you want to change the base?

xds/xdsclient: create LRSClient at time of initialisation #8483

Uh oh!

Conversation

eshitachandwani commented Aug 1, 2025

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arjan-bal Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

purnesh42H Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arjan-bal Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

codecov bot commented Aug 1, 2025 •

edited

Loading

arjan-bal Aug 1, 2025 •

edited

Loading

purnesh42H Aug 1, 2025 •

edited

Loading

arjan-bal Aug 4, 2025 •

edited

Loading