xds: suppress the duplicate errors at the client side - #9185
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9185 +/- ##
==========================================
+ Coverage 83.09% 83.10% +0.01%
==========================================
Files 422 422
Lines 34826 35044 +218
==========================================
+ Hits 28938 29125 +187
- Misses 4393 4411 +18
- Partials 1495 1508 +13
🚀 New features to boost your workflow:
|
easwars
added
Type: Bug
Area: xDS
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces duplicate error suppression for ADS resource updates in the xDS client, skipping watcher notifications when duplicate errors are received, and adds a corresponding test to verify this behavior. The reviewer identified a critical issue in authority.go where a nil pointer dereference panic could occur if md.ErrState is nil but uErr.Err is non-nil, and provided a code suggestion to safely initialize errState.
|
LGTM, adding @easwars for a second review |
| // TestADS_NACKError_DuplicateSuppression_ConcatenatedErrorChange verifies that | ||
| // when multiple invalid resources cause a concatenated error stored in metadata, | ||
| // and a subsequent update carries a duplicate error for only a subset of resources | ||
| // (changing the concatenated error string), the client still suppresses the | ||
| // duplicate error notification to the watcher. |
There was a problem hiding this comment.
It feels weird to me that we store the combined error (from all resource errors and top-level errors) as part of every resource that has an error. This doesn't seem right to me. I need to think about this a little more.
There was a problem hiding this comment.
Yes, that's odd. I think storing a resource error only in state should be good enough, but maybe there is a reason, e.g. debugging/logging?
…all error (#9283) Follow-up from this discussion: #9185 (comment) RELEASE NOTES: none
4 participants
Fixes #8994
This change suppresses duplicate errors on the xds client side by skipping calling watchers for the duplicates. This also prevents generating new child names for the same broken EDS resource (EDS resource has no localities) that is being re-send by the control plane.
RELEASE NOTES: