-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
xds: ensure node ID is populated in errors from the server #8140
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8140 +/- ##
==========================================
- Coverage 82.34% 82.22% -0.13%
==========================================
Files 389 392 +3
Lines 39103 39106 +3
==========================================
- Hits 32200 32154 -46
- Misses 5579 5619 +40
- Partials 1324 1333 +9
🚀 New features to boost your workflow:
|
xds/server.go
Outdated
} | ||
} | ||
return nil | ||
} | ||
|
||
func annotateErrorWithNodeID(nodeID string, err error) error { | ||
return fmt.Errorf("[xDS node id: %v]: %w", nodeID, err) |
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.
These were status errors and now they will be wrapped status errors.
Is this desirable/OK?
Also, WDYT about something like:
func (rc *UsableRouteConfiguration) statusErrWithNodeID(c codes.Code, msg string, ...any) error
to avoid the need to pass NodeID explicitly, like you did with listenerWrapper
?
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.
Is this desirable/OK?
I guess if the caller deals with wrapped errors properly, it should be fine. And if they use our status
package functions to read the code/message out of it, they should be fine too. But I guess the approach you suggested avoids that and is nicer than what I had. So, switched to that. Thanks.
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.
The main part of the changes LGTM.
I was thinking this would impact the generic xdsclient, so assigned to @purnesh42H, but actually it looks like it probably won't since it's all in the resources. But can you still please review the e2e test changes @purnesh42H?
Addresses #7931
This PR ensures that the following scenarios in the xDS-enabled server result in errors that contain the xDS node id:
The PR also includes a whole bunch of test cleanup. But essentially, the only important things changing are the following:
Apart from that, it moves tests around quite a bit so that most server tests live in the top level
xds
directory where the server code lives. Also moving these tests out of thetest/xds
package causes a overall reduction in test run time.RELEASE NOTES: none