[1.5] Cleanup confusion arising from mixing of unix exit codes and Rust Result types #4271
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
roypat
added
the
Status: Awaiting review
ShadowCurse
previously approved these changes
Nov 24, 2023
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## firecracker-v1.5 #4271 +/- ##
====================================================
+ Coverage 82.97% 83.00% +0.03%
====================================================
Files 223 223
Lines 28471 28459 -12
====================================================
- Hits 23623 23622 -1
+ Misses 4848 4837 -11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This error variant was used to encode that no error actually happened, which does not make much sense conceptually. What made this worse is that is contained a FcExitCode, which is itself just a fake Result<(), non-zero-exit code>. This means it was possible to get Firecracker to exit with status "error, but not actually error, but actually there is an error after all", or: "Firecracker exited with an error: Microvm stopped without an error: GenericError". The underlying problem here is the fact that we are using `FcExitCode` as an error variant for `Result`. Since `FcExitCode::Ok` exists, `FcExitCode` is a kind of `Result` itself, meaning we are dealing with `Result<_, Result<_, _>>` as a type, which has no well-defined interpretation. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
Using FcExitCode as an error type is undesirable, as it allows us to construct Err(FcExitCode::Ok), e.g. an object that says "error: everything's okay!". This is confusing and has caused problems in different contexts before, so replace FcExitCode with a proper error type here. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
Previously, when a VM exited, we looked for the first vcpu that reported an exit status, and then indiscriminately reported that back. However, it is possible to one vcpu to exit successfully while another exits with an error, and this could lead us to report "Firecracker Exited Successfully" even though it did not. Now, we explicitly look for the status code of all vcpus. If any of them report an error, this takes precedence over non-error status codes. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
roypat
force-pushed
the
backport-4261
branch
from
1f1e910
to
0fd90ac
Compare
wearyzen
reviewed
Nov 27, 2023
Backported commit with PR number chnaged. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
roypat
force-pushed
the
backport-4261
branch
from
0fd90ac
to
f8f446a
Compare
wearyzen
approved these changes
Nov 27, 2023
pb8o
approved these changes
Nov 27, 2023
pb8o
merged commit d785d60
into
firecracker-microvm:firecracker-v1.5
4 of 7 checks passed
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.
Backport of #4261
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following
Developer Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.PR Checklist
CHANGELOG.md.TODOs link to an issue.rust-vmm.