Make relevant filesystem controller errors "user errors" and categorize them #2070
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The mechanics we use to configure whether an unhandled StorageRecoverableError should produce a crash report is not very flexible. Because StorageRecoverableError is basically a type alias, inheriting from it is essentially the same as inheriting from the type it was pointing to when the inheritance was declared. Therefore, if we want different categories of StorageRecoverableError we need to rebuild them after changing what type StorageRecoverableError points to. Instead, RecoverableError now has a class attribute "produce_crash_report" which controls whether a crash report will be created when such exception is unhandled in a request handler. To configure whether we want to produce crash reports, we just need to update the class attribute. Once can also inherit from RecoverableError and set a class attribute to a different value in the subtype. This is what we do to make storage recoverable errors not produce crash reports. Note than RecoverableError instances can "override" the "produce_crash_report" setting by creating an instance attribute of the same name. Signed-off-by: Olivier Gayot <[email protected]>
Signed-off-by: Olivier Gayot <[email protected]>
In addition to returning a HTTP response of 422 (when configured to do
so), recoverable errors will now include two information in the HTTP
headers:
* x-error-code: a unique identifier of the error category, e.g.,
"storage-invalid-usage"
* x-error-title: a description of the error category, subject to
translation and encoded as a non-indented JSON string
This is basically one step towards implementing RFC 9457 (Problem
Details for HTTP APIs) ; which we may or may not decide to implement in
the future.
Signed-off-by: Olivier Gayot <[email protected]>
Chris-Peterson444
approved these changes
Sep 6, 2024
Comment on lines
+33
to
+35
| # If this exception is uncaught in a request handler, what should we do. | ||
| # By default we produce a crash report, but one can change set it to False. | ||
| produce_crash_report = True |
There was a problem hiding this comment.
This also controls whether the client actually sees (or should treat) this as a recoverable error or not right? Also, having RecoverableError produce crash reports by default feels a little weird without the immediate context of the --no-report-stroage-user-errors flag changing this globally. Could we add some text to the comment about this behavior?
| @@ -551,7 +552,7 @@ async def middleware(self, request, handler): | |||
| resp.headers["x-error-msg"], | |||
| exc_info=exc, | |||
| ) | |||
| if not isinstance(exc, NonReportableException): | |||
| if not isinstance(exc, RecoverableError) or exc.produce_crash_report: | |||
There was a problem hiding this comment.
The way this is written, if we ever throw a NonReportableException in a request handler then we'll get an apport report but we won't halt the installer, which is probably fine because we want to catch those and convert those to RecoverableError anyways?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR comes with 3 main changes: