xdsclient: read bootstrap config before creating the first xDS client in DefaultPool #8164

purnesh42H · 2025-03-12T08:58:10Z

Fixes #8152

RELEASE NOTES:

xds: restore the behavior of reading bootstrap config before creating the first xDS client instead of package init time.

…DefaultPool

codecov · 2025-03-12T09:02:50Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.23%. Comparing base (bdba42f) to head (4d4d9ab).
Report is 6 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8164      +/-   ##
==========================================
- Coverage   82.29%   82.23%   -0.07%     
==========================================
  Files         392      393       +1     
  Lines       39106    39141      +35     
==========================================
+ Hits        32184    32186       +2     
- Misses       5596     5625      +29     
- Partials     1326     1330       +4

Files with missing lines	Coverage Δ
xds/internal/xdsclient/clientimpl.go	`80.85% <100.00%> (+0.76%)`	⬆️
xds/internal/xdsclient/pool.go	`81.08% <100.00%> (+1.66%)`	⬆️

... and 24 files with indirect coverage changes

🚀 New features to boost your workflow:

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

easwars

I'm wondering if it would be possible to add a test to ensure that the bootstrap configuration is loaded in a lazy fashion as opposed to at package init time?

purnesh42H · 2025-03-12T20:36:18Z

I'm wondering if it would be possible to add a test to ensure that the bootstrap configuration is loaded in a lazy fashion as opposed to at package init time?

i am not sure if we can set env var before package init within test and verify if config was set in DefaultPool. However, I wrote the test that verifies that bootstrap config is set only when we create new xDS client first time.

xds/internal/xdsclient/pool_test.go

easwars · 2025-03-12T22:22:53Z

xds/internal/xdsclient/pool_test.go

+		panic(fmt.Sprintf("Failed to create bootstrap configuration: %v", err))
+	}
+
+	envconfig.XDSBootstrapFileContent = string(bs)


We need to reset this env var at the end of the test.

yeah so if i unset at the end of test, the tests are failing saying bootstrap config is not set. Is it because I am setting in the init()? I can do both set and unset in the test but that wouldn't really be close to verifying the config is not being set at package init time.

Do you have any other ideas? And anyway after I move the test under xdsclient_test, we are importing xdsclient package so xdsclient package init() will execute before. Hence, I think we are not verifying the package init part?

I think the usage of envconfig.XDSBootstrapFileContent is wrong in the first place. This env var contains the file content and there can be no lazy loading of this, since the env var is read when the envconfig package is imported.

What we want instead is to use envconfig.XDSBootstrapFileName. In this case, the env var is loaded when the envconfig package is imported, but the actual file is read only when we call bootstrap.GetConfiguration.

All tests in a Go package are built into one binary and are run as a unit (either serially or in parallel based on t.Parallel flag). So, maybe we want this test to be the only test in its package. That way, we can be sure that when the test runs, no other test has run previously and created a client from the DefaultPool. Could you give that a try to see if that actually helps? Thanks.

I have moved the test to a separate pool package but same problem persist if we unset the env var at the end of test. So, i have removed the env var unset part. Is it still a problem if the test is isolated?

I didn't understand the argument about using XDSBootstrapFileContent or XDSBootstrapFileName. I think in this we want to verify that DefaultPool.config is set in lazy fashion which is when we create the first client. So, whether its coming from XDSBootstrapFileContent or XDSBootstrapFileName it shouldn't matter for this test? or you meant to refer to something else for lazy loading?

@easwars i modified the test read from XDSBootstrapFileName but moved the creation part to test itself. I think it is close to what you are suggesting? Although at the init time, XDSBootstrapFileName is loaded but unset because we are not setting it before running the test

It looks to me that what we are trying to achieve here as part of this test might not be possible. See: https://go.dev/doc/effective_go#init.

yeah that's what I was referring to in the first reply

@easwars / @purnesh42H do you want me to pull this change to manually verify the fix? Or is this no longer needed due to the addition of the test package?

yeah please pull and verify if not too much hassle. The test written has ensured the fix but would be good to confirm by retrying exactly what broke.

tested it in our e2e tests and it worked. It is a bit of a process to test this in a real env for a PR but I will also take the release and deploy it on the first day as well

thanks @davinci26. I will go ahead and merge this. We will do a patch release with the fix.

xds/internal/xdsclient/pool.go

easwars · 2025-03-17T13:59:44Z

xds/internal/xdsclient/pool/pool_test.go

+		},
+	})
+	if err != nil {
+		panic(fmt.Sprintf("Failed to create bootstrap configuration: %v", err))


t.Fatal please.

easwars · 2025-03-17T14:00:22Z

xds/internal/xdsclient/pool/pool_test.go

@@ -0,0 +1,77 @@
+/*
+ *
+ * Copyright 2020 gRPC authors.


easwars · 2025-03-17T15:05:28Z

xds/internal/xdsclient/pool_test.go

+		panic(fmt.Sprintf("Failed to create bootstrap configuration: %v", err))
+	}
+
+	envconfig.XDSBootstrapFileContent = string(bs)


It looks to me that what we are trying to achieve here as part of this test might not be possible. See: https://go.dev/doc/effective_go#init. This means that the imported package's variables (and init functions) are initialized before the current package. So, when the test starts running, the environment variable is already loaded (the file is not read though). I think we can get rid of the test in that case.

You might have to just verify this manually.

easwars · 2025-03-17T15:06:34Z

xds/internal/xdsclient/pool_test.go

+		panic(fmt.Sprintf("Failed to create bootstrap configuration: %v", err))
+	}
+
+	envconfig.XDSBootstrapFileContent = string(bs)


@davinci26 Would you be able to verify in your setup that this fix actually fixes your issue? Thanks.

easwars · 2025-03-17T16:40:38Z

@purnesh42H

I discussed this a little with @dfawley. We do feel it would be nice to have a test given that we already caused a regression once.

We do have a few options here:

Have a separate test package, and create a separate test unit in github actions, and set the environment variable from there. This might be more hassle, but if you actually try it and figure that it is not too bad, it might be a OK thing to do.
Have a separate test package:
- When the test starts, ensure that none of the env vars are set. And try creating a client from the default pool. This should fail.
- Then set envconfig. XDSBootstrapFileName and retry creating a client from the default pool. This should succeed.

Give these a try and see if they work. Thanks for your efforts on this.

purnesh42H · 2025-03-17T18:23:17Z

@purnesh42H

I discussed this a little with @dfawley. We do feel it would be nice to have a test given that we already caused a regression once.

We do have a few options here:

Have a separate test package, and create a separate test unit in github actions, and set the environment variable from there. This might be more hassle, but if you actually try it and figure that it is not too bad, it might be a OK thing to do.

Have a separate test package:

When the test starts, ensure that none of the env vars are set. And try creating a client from the default pool. This should fail.

Then set envconfig. XDSBootstrapFileName and retry creating a client from the default pool. This should succeed.

Give these a try and see if they work. Thanks for your efforts on this.

@easwars the second option is what we are doing currently in test. Except that we were only checking if BootstrapConfig was set or not when env var was not set. I have added creating new client in DefaultPool at the start of test and then check if failed or not. Should be enough?

… in DefaultPool (grpc#8164)

xdsclient: read bootstrap config before creating the first client in …

8ed4b2a

…DefaultPool

purnesh42H requested a review from easwars March 12, 2025 08:59

purnesh42H assigned easwars Mar 12, 2025

purnesh42H changed the title ~~xdsclient: read bootstrap config before creating the first client in DefaultPool~~ xdsclient: read bootstrap config before creating the first xDS client in DefaultPool Mar 12, 2025

purnesh42H added the Type: Bug label Mar 12, 2025

purnesh42H added this to the 1.72 Release milestone Mar 12, 2025

easwars reviewed Mar 12, 2025

View reviewed changes

easwars assigned purnesh42H and unassigned easwars Mar 12, 2025

purnesh42H force-pushed the fix-xds-bootstrap-read branch from cc5f6a4 to a1ec537 Compare March 12, 2025 20:32

purnesh42H force-pushed the fix-xds-bootstrap-read branch from a1ec537 to b3c5207 Compare March 12, 2025 20:58

purnesh42H requested a review from easwars March 12, 2025 20:59

purnesh42H assigned easwars and unassigned purnesh42H Mar 12, 2025

purnesh42H force-pushed the fix-xds-bootstrap-read branch 3 times, most recently from c45ccc3 to 779cc4b Compare March 12, 2025 21:34

add DefaultPool lazy load test

97ee92b

purnesh42H force-pushed the fix-xds-bootstrap-read branch from 779cc4b to 97ee92b Compare March 12, 2025 21:54

easwars reviewed Mar 12, 2025

View reviewed changes

easwars assigned purnesh42H and unassigned easwars Mar 12, 2025

purnesh42H added 3 commits March 13, 2025 10:42

easwar review 1

73fe02d

move test to own package

11f523e

use XDSBootstrapFileName

38182b7

purnesh42H assigned easwars and unassigned purnesh42H Mar 15, 2025

purnesh42H requested a review from easwars March 16, 2025 17:46

easwars approved these changes Mar 17, 2025

View reviewed changes

easwars assigned purnesh42H and unassigned easwars Mar 17, 2025

purnesh42H added 2 commits March 17, 2025 23:35

nits

77efbc0

verify creating client before

4d4d9ab

purnesh42H force-pushed the fix-xds-bootstrap-read branch from 4e5c2ce to 4d4d9ab Compare March 17, 2025 18:20

purnesh42H merged commit c27e6dc into grpc:master Mar 18, 2025
15 checks passed

purnesh42H added a commit to purnesh42H/grpc-go that referenced this pull request Mar 19, 2025

xdsclient: read bootstrap config before creating the first xDS client…

478f29c

… in DefaultPool (grpc#8164)

purnesh42H added a commit to purnesh42H/grpc-go that referenced this pull request Mar 19, 2025

xdsclient: read bootstrap config before creating the first xDS client…

ad7c2b0

… in DefaultPool (grpc#8164)

purnesh42H mentioned this pull request Mar 19, 2025

Cherry-pick #8164 #8166 to v1.71.x #8182

Merged

purnesh42H added a commit to purnesh42H/grpc-go that referenced this pull request Mar 19, 2025

xdsclient: read bootstrap config before creating the first xDS client…

18813d8

… in DefaultPool (grpc#8164)

purnesh42H added a commit that referenced this pull request Mar 20, 2025

Cherry-pick #8164 #8166 to v1.71.x (#8182)

208e03b

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

xdsclient: read bootstrap config before creating the first xDS client…

e0c6faf

… in DefaultPool (grpc#8164)

purnesh42H added a commit to purnesh42H/grpc-go that referenced this pull request Mar 28, 2025

xdsclient: read bootstrap config before creating the first xDS client…

f233d03

… in DefaultPool (grpc#8164)

arjan-bal modified the milestones: 1.72 Release, 1.73 Release, 1.71 Release Apr 17, 2025

xdsclient: read bootstrap config before creating the first xDS client in DefaultPool #8164

xdsclient: read bootstrap config before creating the first xDS client in DefaultPool #8164

Uh oh!

Conversation

purnesh42H commented Mar 12, 2025

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

easwars left a comment

Choose a reason for hiding this comment

Uh oh!

purnesh42H commented Mar 12, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

purnesh42H Mar 13, 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!

purnesh42H Mar 13, 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!

purnesh42H Mar 15, 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!

purnesh42H Mar 17, 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!

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!

easwars commented Mar 17, 2025

Uh oh!

purnesh42H commented Mar 17, 2025

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Mar 12, 2025 •

edited

Loading

purnesh42H Mar 13, 2025 •

edited

Loading

purnesh42H Mar 13, 2025 •

edited

Loading

purnesh42H Mar 15, 2025 •

edited

Loading

purnesh42H Mar 17, 2025 •

edited

Loading