xds: fix support for circuit breakers in LOGICAL_DNS clusters #8169

dfawley · 2025-03-13T18:28:24Z

Previously this setting was only plumbed to EDS, which meant it didn't apply to LOGICAL_DNS, but it should apply to both.

RELEASE NOTES:

xds: fix support for circuit breakers in LOGICAL_DNS clusters

codecov · 2025-03-13T18:33:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.07%. Comparing base (775150f) to head (3bc9d53).
Report is 7 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8169      +/-   ##
==========================================
- Coverage   82.25%   82.07%   -0.19%     
==========================================
  Files         393      410      +17     
  Lines       39143    40235    +1092     
==========================================
+ Hits        32197    33021     +824     
- Misses       5616     5851     +235     
- Partials     1330     1363      +33

Files with missing lines	Coverage Δ
xds/internal/balancer/cdsbalancer/cdsbalancer.go	`84.59% <100.00%> (+0.03%)`	⬆️
...internal/balancer/clusterresolver/configbuilder.go	`91.12% <100.00%> (+0.05%)`	⬆️

... and 43 files with indirect coverage changes

🚀 New features to boost your workflow:

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

purnesh42H · 2025-03-18T07:25:57Z

xds/internal/balancer/clusterimpl/tests/balancer_test.go

+func (s) TestCircuitBreaking(t *testing.T) {
+	const maxRequests = 3
+	// Create an xDS management server that serves ADS and LRS requests.
+	mgmtServer := e2e.StartManagementServer(t, e2e.ManagementServerOptions{SupportLoadReportingService: true})


nit: Do we need to server LRS for this test?

Nope. This was copy/pasted from another test that probably did.

You need not set SupportLoadReportingService: true if LRS is not needed

Ah sorry I thought you were just talking about the comment.

xds/internal/balancer/clusterimpl/tests/balancer_test.go

purnesh42H · 2025-03-18T09:40:16Z

xds/internal/balancer/clusterimpl/tests/balancer_test.go

+			return
+		}
+		if err == nil {
+			stream.CloseSend()


nit: may be comment saying we are closing unintended new streams?

xds/internal/balancer/clusterimpl/tests/balancer_test.go

purnesh42H · 2025-03-18T09:49:55Z

xds/internal/balancer/clusterimpl/tests/balancer_test.go

+func (s) TestCircuitBreakingLogicalDNS(t *testing.T) {
+	const maxRequests = 3
+	// Create an xDS management server that serves ADS and LRS requests.
+	mgmtServer := e2e.StartManagementServer(t, e2e.ManagementServerOptions{SupportLoadReportingService: true})


same. Do we need LRS for this test?

same. Don't set SupportLoadReportingService: true

purnesh42H · 2025-03-18T09:55:06Z

xds/internal/balancer/clusterimpl/tests/balancer_test.go

+	host, port := hostAndPortFromAddress(t, server.Address)
+
+	// Configure the xDS management server with default resources. Override the
+	// default cluster to include an LRS server config pointing to self.


default cluster to include an LRS server config pointing to self.

not LRS server. Comment should mention CircuitBreakers ?

dfawley · 2025-03-18T20:12:19Z

Thanks, PTAL

purnesh42H

LGTM. Kept some comments unresolved. PTAL

purnesh42H · 2025-03-19T14:09:53Z

xds/internal/balancer/clusterimpl/tests/balancer_test.go

+func (s) TestCircuitBreaking(t *testing.T) {
+	const maxRequests = 3
+	// Create an xDS management server that serves ADS and LRS requests.
+	mgmtServer := e2e.StartManagementServer(t, e2e.ManagementServerOptions{SupportLoadReportingService: true})


You need not set SupportLoadReportingService: true if LRS is not needed

purnesh42H · 2025-03-19T14:10:32Z

xds/internal/balancer/clusterimpl/tests/balancer_test.go

+	defer server.Stop()
+
+	// Configure the xDS management server with default resources. Override the
+	// default cluster to include an LRS server config pointing to self.


default cluster to include an LRS server config pointing to self.

need to change this as well

purnesh42H · 2025-03-19T14:11:45Z

xds/internal/balancer/clusterimpl/tests/balancer_test.go

+func (s) TestCircuitBreakingLogicalDNS(t *testing.T) {
+	const maxRequests = 3
+	// Create an xDS management server that serves ADS and LRS requests.
+	mgmtServer := e2e.StartManagementServer(t, e2e.ManagementServerOptions{SupportLoadReportingService: true})


same. Don't set SupportLoadReportingService: true

purnesh42H · 2025-03-19T14:15:22Z

Doesn't look like related to change but since its xds test, might be good to confirm https://github.com/grpc/grpc-go/actions/runs/13932590107/job/38993150684?pr=8169

dfawley · 2025-03-19T20:55:42Z

Doesn't look like related to change but since its xds test, might be good to confirm grpc/grpc-go/actions/runs/13932590107/job/38993150684?pr=8169

Filed #8185 for the flake.

)

xds: fix support for circuit breakers in LOGICAL_DNS clusters

fe9ac91

dfawley added the Type: Bug label Mar 13, 2025

dfawley added this to the 1.72 Release milestone Mar 13, 2025

dfawley requested a review from purnesh42H March 13, 2025 18:28

dfawley assigned purnesh42H Mar 13, 2025

dfawley mentioned this pull request Mar 13, 2025

xds: fix support for load reporting in LOGICAL_DNS clusters #8170

Merged

purnesh42H reviewed Mar 18, 2025

View reviewed changes

purnesh42H assigned dfawley and unassigned purnesh42H Mar 18, 2025

test comments

f527755

dfawley assigned purnesh42H and unassigned dfawley Mar 18, 2025

purnesh42H approved these changes Mar 19, 2025

View reviewed changes

purnesh42H assigned dfawley and unassigned purnesh42H Mar 19, 2025

nits

3bc9d53

dfawley merged commit ce2fded into grpc:master Mar 20, 2025
15 checks passed

dfawley deleted the logical_dns_cb branch March 20, 2025 19:55

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

xds: fix support for circuit breakers in LOGICAL_DNS clusters (grpc#8169

cfc1939

)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xds: fix support for circuit breakers in LOGICAL_DNS clusters #8169

xds: fix support for circuit breakers in LOGICAL_DNS clusters #8169

dfawley commented Mar 13, 2025 •

edited

Loading

codecov bot commented Mar 13, 2025 •

edited

Loading

purnesh42H Mar 18, 2025

dfawley Mar 18, 2025

purnesh42H Mar 19, 2025

dfawley Mar 19, 2025

purnesh42H Mar 18, 2025

dfawley Mar 18, 2025

purnesh42H Mar 18, 2025

dfawley Mar 18, 2025

purnesh42H Mar 19, 2025

dfawley Mar 19, 2025

purnesh42H Mar 18, 2025

dfawley Mar 18, 2025

dfawley commented Mar 18, 2025

purnesh42H left a comment

purnesh42H Mar 19, 2025

purnesh42H Mar 19, 2025

dfawley Mar 19, 2025

purnesh42H Mar 19, 2025

purnesh42H commented Mar 19, 2025

dfawley commented Mar 19, 2025

xds: fix support for circuit breakers in LOGICAL_DNS clusters #8169

xds: fix support for circuit breakers in LOGICAL_DNS clusters #8169

Conversation

dfawley commented Mar 13, 2025 • edited Loading

codecov bot commented Mar 13, 2025 • edited Loading

Codecov Report

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

dfawley commented Mar 18, 2025

purnesh42H 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

purnesh42H commented Mar 19, 2025

dfawley commented Mar 19, 2025

dfawley commented Mar 13, 2025 •

edited

Loading

codecov bot commented Mar 13, 2025 •

edited

Loading