[Bug] Firecracker cannot shut down successfully without API #4176
Assignees
Comments
mkroening
changed the title
Minimal verifiable exampleUpdated git clone https://github.com/firecracker-microvm/firecracker
cd firecracker
git switch firecracker-v1.5
cargo build --release
echo "{
\"boot-source\": {
\"kernel_image_path\": \"rusty-loader-x86_64-fc\",
\"initrd_path\": \"hermit-rs-template\",
\"boot_args\": \"\"
},
\"drives\": [],
\"machine-config\": {
\"vcpu_count\": 1,
\"mem_size_mib\": 256,
\"smt\": false
}
}" > firecracker-config.json
echo "" > firecracker.log
wget https://github.com/hermit-os/loader/releases/download/v0.4.4/rusty-loader-x86_64-fc
wget https://github.com/firecracker-microvm/firecracker/files/12915940/hermit-rs-template.zip
unzip hermit-rs-template.zip
sudo ./build/cargo_target/release/firecracker \
--no-api \
--config-file firecracker-config.json \
--log-path firecracker.log \
--level Info --show-level \
--show-log-origin
echo $?@mkroening Can you please provide links for |
|
Thanks for looking into this! :)
|
|
Fixed with impl From<MainError> for ExitCode {
fn from(value: MainError) -> Self {
let exit_code = match value {
MainError::ParseArguments(_) => FcExitCode::ArgParsing,
MainError::InvalidLogLevel(_) => FcExitCode::BadConfiguration,
MainError::RunWithApi(ApiServerError::MicroVMStoppedWithoutError(code)) => code,
MainError::RunWithApi(ApiServerError::MicroVMStoppedWithError(code)) => code,
+ MainError::RunWithoutApiError(RunWithoutApiError::Shutdown(code)) => code,
_ => FcExitCode::GenericError,
};
ExitCode::from(exit_code as u8)
}
}e.g. |
This was referenced Oct 16, 2023
|
Thanks a lot! The status code looks good, but it still prints |
|
Yeah this also applies to other cases. I think changing this further can be another discussion, I agree it is non-ideal. For now we do not recommend pattern matching on logs, they are meant for humans and often change between releases. What do you think about a solution like: fn main() -> ExitCode {
let result = main_exec();
if let Err(err) = result {
let exit_code = ExitCode::from(err);
if exit_code != ExitCode:SUCCESS {
error!("{err}");
eprintln!("Error: {err:?}");
}
exit_code
} else {
ExitCode::SUCCESS
}
}This is still quite awkward, it feels more like a work-around than fixing the problem. |
|
Although it is true we do not suggest that users should check log messages, I agree that printing something like "Error: whatever" can be quite confusing. Is this difficult to fix? |
How about fixing // Run the EventManager that drives everything in the microVM.
loop {
event_manager
.run()
.expect("Failed to start the event manager");
if let Some(exit_code) = vmm.lock().unwrap().shutdown_exit_code() {
return match exit_code {
ExitCode:SUCCESS => Ok(()),
exit_code => Err(RunWithoutApiError::Shutdown(exit_code))
};
}
} |
9 tasks
|
@mkroening I've posted a draft PR that refactors the error propagation similar to your suggestion #4180. |
|
Thanks! :) |
|
@mkroening PRs have been merged fixing this on |
|
Awesome, thanks a lot! When do you expect 1.5.1 to be released? For now, we downgraded to 1.4.1. :) |
Hi @mkroening, we expect to start working on preparing the 1.5.1 release in two weeks |
9 tasks
|
Hi @mkroening! We have released now 1.5.1 (and recently 1.6.0). Could you validate that these releases solve the problem? |
|
Both of those releases work wonderfully (see hermit-os/kernel#1000). Thanks a lot! :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the bug
When running Firecracker without API, it prints
Error: RunWithoutApiError(Shutdown(Ok))and returns with a non-zero exit code, although the kernel shut down cleanly, which is also indicated by the error message.To Reproduce
Launch any kernel without API. This has been tested with Hermit. It should work the same with any other kernel.
Create
firecracker-config.json:Launch Firecracker:
Compare output and logs from 1.4.1 with 1.5.0:
1.4.1 exit code: 0
1.5.0 exit code: 1
1.4.1 output:
1.5.0 output:
1.4.1 log:
1.5.0 log:
Expected behaviour
Return a zero exit code.
Additional context
This is a regression from 1.4.1 to 1.5.0 from
820c5acfrom #3994.This code is the culprit, as it can never return
Ok(()):A fix should be simple.
I can open a PR if you like. :)
Checks
The text was updated successfully, but these errors were encountered: