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.

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

fix: Refactor error propagation #4180

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@
- Simplified and clarified the removal policy of deprecated API elements
to follow semantic versioning 2.0.0. For more information, please refer to
[this GitHub discussion](https://github.com/firecracker-microvm/firecracker/discussions/4135).
- [#4180](https://github.com/firecracker-microvm/firecracker/pull/4180):
Refactored error propagation to avoid logging and printing an error on
exits with a zero exit code. Now, on successful exit
"Firecracker exited successfully" is logged.

### Deprecated

Expand Down
13 changes: 9 additions & 4 deletions src/firecracker/src/api_server_adapter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@
vm_resources: VmResources,
vmm: Arc<Mutex<Vmm>>,
event_manager: &mut EventManager,
) -> FcExitCode {
) -> Result<(), FcExitCode> {

Check warning on line 55 in src/firecracker/src/api_server_adapter.rs

View check run for this annotation

Codecov / codecov/patch

src/firecracker/src/api_server_adapter.rs#L55

Added line #L55 was not covered by tests
let api_adapter = Arc::new(Mutex::new(Self {
api_event_fd,
from_api,
Expand All @@ -64,10 +64,14 @@
event_manager
.run()
.expect("EventManager events driver fatal error");
if let Some(exit_code) = vmm.lock().unwrap().shutdown_exit_code() {
return exit_code;

match vmm.lock().unwrap().shutdown_exit_code() {
Some(FcExitCode::Ok) => break,
Some(exit_code) => return Err(exit_code),
None => continue,

Check warning on line 71 in src/firecracker/src/api_server_adapter.rs

View check run for this annotation

Codecov / codecov/patch

src/firecracker/src/api_server_adapter.rs#L67-L71

Added lines #L67 - L71 were not covered by tests
}
}
Ok(())

Check warning on line 74 in src/firecracker/src/api_server_adapter.rs

View check run for this annotation

Codecov / codecov/patch

src/firecracker/src/api_server_adapter.rs#L74

Added line #L74 was not covered by tests
}

fn handle_request(&mut self, req_action: VmmAction) {
Expand Down Expand Up @@ -245,7 +249,8 @@
api_thread.join().expect("Api thread should join");

match result {
Ok(exit_code) => Err(ApiServerError::MicroVMStoppedWithoutError(exit_code)),
Ok(Ok(())) => Ok(()),
Ok(Err(exit_code)) => Err(ApiServerError::MicroVMStoppedWithoutError(exit_code)),

Check warning on line 253 in src/firecracker/src/api_server_adapter.rs

View check run for this annotation

Codecov / codecov/patch

src/firecracker/src/api_server_adapter.rs#L252-L253

Added lines #L252 - L253 were not covered by tests
roypat marked this conversation as resolved.
Show resolved Hide resolved
Err(exit_error) => Err(exit_error),
}
}
8 changes: 6 additions & 2 deletions src/firecracker/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@
eprintln!("Error: {err:?}");
ExitCode::from(err)
} else {
info!("Firecracker exited successfully");

Check warning on line 101 in src/firecracker/src/main.rs

View check run for this annotation

Codecov / codecov/patch

src/firecracker/src/main.rs#L101

Added line #L101 was not covered by tests
ExitCode::SUCCESS
}
}
Expand Down Expand Up @@ -631,8 +632,11 @@
.run()
.expect("Failed to start the event manager");

if let Some(exit_code) = vmm.lock().unwrap().shutdown_exit_code() {
return Err(RunWithoutApiError::Shutdown(exit_code));
match vmm.lock().unwrap().shutdown_exit_code() {
Some(FcExitCode::Ok) => break,
Some(exit_code) => return Err(RunWithoutApiError::Shutdown(exit_code)),
None => continue,

Check warning on line 638 in src/firecracker/src/main.rs

View check run for this annotation

Codecov / codecov/patch

src/firecracker/src/main.rs#L635-L638

Added lines #L635 - L638 were not covered by tests
}
}
Ok(())

Check warning on line 641 in src/firecracker/src/main.rs

View check run for this annotation

Codecov / codecov/patch

src/firecracker/src/main.rs#L641

Added line #L641 was not covered by tests
}
4 changes: 1 addition & 3 deletions tests/integration_tests/functional/test_cmd_line_start.py
Original file line number Diff line number Diff line change
Expand Up @@ -170,9 +170,7 @@ def test_config_start_no_api_exit(uvm_plain, vm_config_file):
time.sleep(3) # Wait for shutdown

# Check error log
test_microvm.check_log_message(
"RunWithoutApiError error: MicroVMStopped without an error: Ok"
)
test_microvm.check_log_message("Firecracker exited successfully")


@pytest.mark.parametrize(
Expand Down