-
Notifications
You must be signed in to change notification settings - Fork 4.5k
xdsclient: fixed a bug to not report connectivity error from new stream recreation if a response was received before #8373
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8373 +/- ##
==========================================
- Coverage 82.22% 82.15% -0.08%
==========================================
Files 419 419
Lines 42047 42066 +19
==========================================
- Hits 34574 34560 -14
- Misses 6003 6035 +32
- Partials 1470 1471 +1
🚀 New features to boost your workflow:
|
868f0f0
to
295dc3a
Compare
…ng new stream creation if msg was received before
295dc3a
to
b54468a
Compare
Looks like its failing some fallack tests. Will check them. Unassigning the reviewers until then. |
02e583d
to
86fbe6e
Compare
86fbe6e
to
562b1a0
Compare
Fixed by resetting after first attempt of recreation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please make the following fixes in the test logic and re-verify on forge?
- Validate that we get a second update metric emitted.
- Use a short context and verify that we don't receive a server failure metric.
grpc-go/xds/internal/xdsclient/metrics_test.go
Lines 320 to 329 in ec4810c
mdWant = stats.MetricsData{ | |
Handle: xdsClientServerFailureMetric.Descriptor(), | |
IntIncr: 1, | |
LabelKeys: []string{"grpc.target", "grpc.xds.server"}, | |
LabelVals: []string{"Test/ServerFailureMetrics_AfterResponseRecv", mgmtServer.Address}, | |
} | |
// Server failure should still have no recording point. | |
if err := tmr.WaitForInt64Count(ctx, mdWant); err == nil { | |
t.Fatal("tmr.WaitForInt64Count(ctx, mdWant) succeeded when expected to timeout.") | |
} |
I don't think we can deterministically verify that with the current setup of test metrics recorder as only one metric is kept at a time. We already have tests that verify the behavior of re-requesting resources in case of stream restarts. I have removed the restart operation in metrics test so that we don't have possibility of re-request and resource update metrics.
Done |
Fixed by resetting messageReceived to false after first attempt of recreation because subsequent attempts should backoff as usual. |
Made a comment on the bug. In my opinion, the test should be changed instead of the ADS stream implementation. |
Fixes: #8351
As per gRFC 57 we do not consider it an error if the ADS stream was closed after having received a response on the stream. This should hold true even if we get an error during first attempt of recreation of the stream.
Successful forge test
RELEASE NOTES: