xds: fix support for circuit breakers in LOGICAL_DNS clusters #8169
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8169 +/- ##
==========================================
- Coverage 82.25% 82.08% -0.18%
==========================================
Files 393 410 +17
Lines 39143 40253 +1110
==========================================
+ Hits 32197 33040 +843
- Misses 5616 5851 +235
- Partials 1330 1362 +32
🚀 New features to boost your workflow:
|
| 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}) |
There was a problem hiding this comment.
nit: Do we need to server LRS for this test?
There was a problem hiding this comment.
Nope. This was copy/pasted from another test that probably did.
| @@ -354,3 +357,213 @@ func waitForSuccessfulLoadReport(ctx context.Context, lrsServer *fakeserver.Serv | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| func (s) TestCircuitBreaking(t *testing.T) { | |||
|
|
||
| // Configure the xDS management server with default resources. Override the | ||
| // default cluster to include an LRS server config pointing to self. | ||
| const serviceName = "my-test-xds-service" |
There was a problem hiding this comment.
nit: similar to maxRequests, serviceName can be declared at the start as well?
There was a problem hiding this comment.
Moved maxRequests here instead.
| return | ||
| } | ||
| if err == nil { | ||
| stream.CloseSend() |
There was a problem hiding this comment.
nit: may be comment saying we are closing unintended new streams?
| t.Fatalf("RPCs unexpectedly allowed beyond circuit breaking maximum") | ||
| } | ||
|
|
||
| func (s) TestCircuitBreakingLogicalDNS(t *testing.T) { |
There was a problem hiding this comment.
Add docstring for this as well explaining what the test is verifying
| 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}) |
There was a problem hiding this comment.
same. Do we need LRS for this test?
| 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. |
There was a problem hiding this comment.
default cluster to include an LRS server config pointing to self.
not LRS server. Comment should mention CircuitBreakers ?
|
Thanks, PTAL |
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: