Cleanup confusion arising from mixing of unix exit codes and Rust Result types
#4261
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
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>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #4261 +/- ##
==========================================
+ Coverage 81.66% 81.69% +0.03%
==========================================
Files 240 240
Lines 29303 29291 -12
==========================================
- Hits 23931 23930 -1
+ Misses 5372 5361 -11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
roypat
force-pushed
the
no-exitcode-in-result
branch
from
82901eb
to
d5a0ed5
Compare
roypat
force-pushed
the
no-exitcode-in-result
branch
from
d5a0ed5
to
0ce5344
Compare
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>
roypat
force-pushed
the
no-exitcode-in-result
branch
from
0ce5344
to
33ff7b0
Compare
roypat
added
the
Status: Awaiting review
JonathanWoollett-Light
previously approved these changes
Nov 23, 2023
wearyzen
reviewed
Nov 24, 2023
wearyzen
reviewed
Nov 24, 2023
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
no-exitcode-in-result
branch
from
6a7b7e3
to
31323f9
Compare
wearyzen
approved these changes
Nov 24, 2023
ShadowCurse
approved these changes
Nov 24, 2023
9 tasks
roypat
added a commit
to roypat/firecracker
that referenced
this pull request
Nov 24, 2023
It is technically a bug fix Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
wearyzen
pushed a commit
to roypat/firecracker
that referenced
this pull request
Nov 27, 2023
It is technically a bug fix Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
roypat
added a commit
that referenced
this pull request
Nov 27, 2023
It is technically a bug fix Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
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.
An exit code is a kind of
Result: Either everything isOk()if the exit code = 0, or someErr()happened if exit code != 0. Therefore, nestingFcExitCodeinside theErrvariant of a RustResultyields a construct ofResult<_, Result<_, _>>. This is no well-defined (as it is unclear whatErr(Ok(...))is supposed to mean). This was made worse by the existence ofApiServerError::MicroVMStoppedWithoutError, which turnedApiServerErrorinto a kind ofResultitself. SinceMicroVmStoppedWithoutErrorhas a field of typeFcExitCode, we ended up with a construct ofResult<_, Result<Result<_, _>, _>, or, in terms of user visible error messages: "RunWithApiError error: MicroVMStopped without an error: GenericError". The confusion around this lead to bugs such as #4176.This PR attempts to clear up this confusion by removing
ApiServer::Error::MicroVMSteppedWithoutError, and eliminating the use ofFcExitCodeas anErrtype wherever possible. We are only left with a single instance of this (ApiServerError::MicroVmStoppedWithError), which cannot be resolved without significantly refractoring (and eliminating the use of exit codes from the emulation code).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.