Skip to content
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.

Sign up for GitHub

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

Jump to bottom

xRFC TP3: Error Propagation #107

Merged
merged 21 commits into from
Dec 13, 2024
Merged

xRFC TP3: Error Propagation #107

merged 21 commits into from
Dec 13, 2024

Conversation

anicr7
Copy link
Contributor

@anicr7 anicr7 commented Oct 25, 2024 •
edited
Loading

Proposal for the xDS error propagation which allows xDS management servers to provide additional information to the clients in case of errors like permission errors or resource being missing.

cc: @markdroth, @adisuissa, @htuch

@markdroth markdroth changed the title Signed-off-by: Anirudh Ramachandra <[email protected]> xRFC TP3: Error Propagation Oct 25, 2024
@anicr7 anicr7 marked this pull request as ready for review October 25, 2024 23:18
howardjohn
howardjohn reviewed Oct 28, 2024
View reviewed changes
proposals/TP3-xds-error-propagation.md Outdated Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Outdated Show resolved Hide resolved
markdroth
markdroth reviewed Nov 1, 2024
View reviewed changes
Copy link
Contributor

@markdroth markdroth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for writing this up, @anicr7!

Please let me know if you have any questions about any of this.

proposals/TP3-xds-error-propagation.md Outdated Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Outdated Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Outdated Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Outdated Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Outdated Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Outdated Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Outdated Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Outdated Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Outdated Show resolved Hide resolved
markdroth
markdroth reviewed Nov 11, 2024
View reviewed changes
Copy link
Contributor

@markdroth markdroth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good overall! I have a few more cosmetic comments, but the two main open questions are the ones from my last review pass on which I want to get some additional input.

proposals/TP3-xds-error-propagation.md Outdated Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Outdated Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Outdated Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Outdated Show resolved Hide resolved
@anicr7 anicr7 requested a review from markdroth November 13, 2024 20:36
markdroth
markdroth reviewed Nov 16, 2024
View reviewed changes
proposals/TP3-xds-error-propagation.md Outdated Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Outdated Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Outdated Show resolved Hide resolved
@anicr7 anicr7 requested a review from markdroth November 18, 2024 18:46
markdroth
markdroth reviewed Nov 20, 2024
View reviewed changes
proposals/TP3-xds-error-propagation.md Outdated Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Outdated Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Outdated Show resolved Hide resolved
adisuissa
adisuissa reviewed Nov 21, 2024
View reviewed changes
proposals/TP3-xds-error-propagation.md Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Outdated Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Show resolved Hide resolved
markdroth
markdroth reviewed Nov 21, 2024
View reviewed changes
proposals/TP3-xds-error-propagation.md Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Outdated Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Outdated Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Outdated Show resolved Hide resolved
adisuissa
adisuissa reviewed Dec 10, 2024
View reviewed changes
Copy link

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think that having the knowledge of the state of a resource (even if it exists) may be challenging in a distributed system that has a non-strong consistency model.
However, I understand that some systems may benefit from this field, and therefore it should be allowed but optional.

I'm resolving the comments.

proposals/TP3-xds-error-propagation.md Outdated Show resolved Hide resolved
markdroth
markdroth reviewed Dec 13, 2024
View reviewed changes
Copy link
Contributor

@markdroth markdroth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good! Just a couple of minor things left.

proposals/TP3-xds-error-propagation.md Outdated Show resolved Hide resolved
proposals/TP3-xds-error-propagation.md Show resolved Hide resolved
@anicr7 anicr7 requested a review from markdroth December 13, 2024 07:22
adisuissa
adisuissa reviewed Dec 13, 2024
View reviewed changes
Copy link

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix DCO

@anicr7 anicr7 force-pushed the xds_error_propagation branch from efb4593 to 57a08b7 Compare December 13, 2024 18:05
@anicr7 anicr7 requested a review from adisuissa December 13, 2024 18:07
markdroth
markdroth reviewed Dec 13, 2024
View reviewed changes
Copy link
Contributor

@markdroth markdroth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one small nit left!

For future reference, please don't force-push to a PR once a review has started, since that makes it hard for the review to see what's changed since their last review pass. Thanks!

proposals/TP3-xds-error-propagation.md Outdated Show resolved Hide resolved
@anicr7 anicr7 requested a review from markdroth December 13, 2024 21:37
markdroth
markdroth approved these changes Dec 13, 2024
View reviewed changes
Copy link
Contributor

@markdroth markdroth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great!

Please fix CI, so that we can get this merged. Thanks!

@anicr7
Signed-off-by: Anirudh Ramachandra <[email protected]>
ff3b0e8 
Proposal for the xDS error propagation which allows xDS management
servers to provide additional information to the clients in case of
errors like permission errors or resource being missing.

Signed-off-by: Anirudh Ramachandra <[email protected]>
@anicr7
Formating changes: Trim empty whitespaces, add required identations f…
60aed56 
…or lists and nested lists

Signed-off-by: Anirudh Ramachandra <[email protected]>
@anicr7
Further formatting changes, add last updated date as well
e60bf96 
Signed-off-by: Anirudh Ramachandra <[email protected]>
@anicr7
Few grammatical changes
342385c 
Signed-off-by: Anirudh Ramachandra <[email protected]>
@anicr7
Readability improvements
8264a25 
Signed-off-by: Anirudh Ramachandra <[email protected]>
@anicr7
Remove Harvey from the approver list
fa5abe8 
Signed-off-by: Anirudh Ramachandra <[email protected]>
anicr7 added 15 commits December 13, 2024 21:41
@anicr7
Remove NACK discussion from the Open Issue list
5c189cf 
Signed-off-by: Anirudh Ramachandra <[email protected]>
@anicr7
Add reference to xRFC TP1 in the related proposals section
57f017d 
Signed-off-by: Anirudh Ramachandra <[email protected]>
@anicr7
Fix formatting with asterisks
50a67e6 
Signed-off-by: Anirudh Ramachandra <[email protected]>
@anicr7
Rewrite background/abstract
13f90b3 
Signed-off-by: Anirudh Ramachandra <[email protected]>
@anicr7
Improve the details around the protocol behavior.
a30b0f9 
Signed-off-by: Anirudh Ramachandra <[email protected]>
@anicr7
Make backward compatibility section clearer
49f5d1e 
Signed-off-by: Anirudh Ramachandra <[email protected]>
@anicr7
Address review comments including fixing links to be relative and rew…
b125d22 
…ording the wildcard resources section

Signed-off-by: Anirudh Ramachandra <[email protected]>
@anicr7
Change the xRFC to remove additional field and use status code directly
53345e4 
Signed-off-by: Anirudh Ramachandra <[email protected]>
@anicr7
Format the protocol behavior to be clearer
0d6dc42 
Signed-off-by: Anirudh Ramachandra <[email protected]>
@anicr7
Formatting changes
78a5348 
Signed-off-by: Anirudh Ramachandra <[email protected]>
@anicr7
Change must to may
41d09fd 
Signed-off-by: Anirudh Ramachandra <[email protected]>
@anicr7
Clarify that wildcard subscriptions are not supported
2f1ba89 
Signed-off-by: Anirudh Ramachandra <[email protected]>
@anicr7
Fix protocol behavior to specify that the resources shouldn't intersect
fe17408 
Signed-off-by: Anirudh Ramachandra <[email protected]>
@anicr7
Add a section about CSDS changes
6db2d54 
Signed-off-by: Anirudh Ramachandra <[email protected]>
@anicr7
Fix comment on the CSDS proto
92b6224 
Signed-off-by: Anirudh Ramachandra <[email protected]>
@anicr7 anicr7 force-pushed the xds_error_propagation branch from 918a257 to 92b6224 Compare December 13, 2024 21:41
@anicr7
Copy link
Contributor Author

anicr7 commented Dec 13, 2024

This looks great!

Please fix CI, so that we can get this merged. Thanks!

@markdroth This should now be fixed!

@markdroth markdroth merged commit 57cfbe6 into cncf:main Dec 13, 2024
4 checks passed
@anicr7 anicr7 deleted the xds_error_propagation branch December 13, 2024 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markdroth markdroth markdroth approved these changes

@howardjohn howardjohn Awaiting requested review from howardjohn

@adisuissa adisuissa Awaiting requested review from adisuissa

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@anicr7 @howardjohn @adisuissa @markdroth