Add --cleanup-level switch for choice of fast exit

While cleanly exiting threads and running all destructors in Rust
is good for peace of mind and Valgrind, it is slower than just
calling exit() and letting the OS clean up.

In order to prevent the new graceful exit mechanics from slowing
down typical runs, this adds a new switch `--cleanup-level`.

The default is `--cleanup-level 1`, which will exit at basically
the same time that Firecracker would quit previously.  It uses
libc::exit() to do so (since that's what it used before), but
there may be reasons to change to Rust's std::process:exit(), if
the code that runs is important.

The new clean exit is done by `--cleanup-level 2`.  This will join
all threads and exit at the topmost main() level, so that all the
Firecracker code is off the stack.

An added mode which doesn't run any cleanup at all is given for
`--cleanup-level 0`.  This may or may not be useful, but it would
be the fastest way to quit.

Because a strange URL had been added to the API thread for
internal use in clean shutdown, this hides that facility unless
cleanup-level 2 is being used.

CHANGELOG.md

@@ -9,6 +9,9 @@
 - Added option to configure block device flush.
 - Added `--new-pid-ns` flag to the Jailer in order to spawn the Firecracker
   process in a new PID namespace.
+- Added `--cleanup-level` parameter to Firecracker so you can ask to gracefully
+  shut down threads and release all resources, or exit abruptly.  (Higher
+  cleanup levels make shutdown slower, but help with tools like Valgrind.)
 
 ### Fixed
 

src/api_server/src/parsed_request.rs

@@ -24,6 +24,7 @@ use micro_http::{Body, Method, Request, Response, StatusCode, Version};
 
 use logger::{error, info};
 use vmm::rpc_interface::{VmmAction, VmmActionError};
+use vmm::{CleanupLevel, CLEANUP_LEVEL};
 
 pub(crate) enum ParsedRequest {
     GetInstanceInfo,
@@ -58,7 +59,18 @@ impl ParsedRequest {
 
         match (request.method(), path, request.body.as_ref()) {
             (Method::Get, "", None) => parse_get_instance_info(),
-            (Method::Get, "shutdown-internal", None) => Ok(ParsedRequest::ShutdownInternal),
+            (Method::Get, "shutdown-internal", None) => {
+                //
+                // This isn't a user-facing API, and was added solely to facilitate clean shutdowns.
+                // Calling it manually will cause problems, so only enable it if the command line option
+                // facilitating "Valgrind-level-cleanliness" is being used.
+                //
+                let cleanup_level = *CLEANUP_LEVEL.read().unwrap();
+                match cleanup_level {
+                    CleanupLevel::Valgrind => Ok(ParsedRequest::ShutdownInternal),
+                    _ => Err(Error::InvalidPathMethod("shutdown-internal".to_string(), Method::Get))
+                }
+            },
             (Method::Get, "balloon", None) => parse_get_balloon(path_tokens.get(1)),
             (Method::Get, "machine-config", None) => parse_get_machine_config(),
             (Method::Get, "mmds", None) => parse_get_mmds(),

src/firecracker/src/api_server_adapter.rs

@@ -27,6 +27,7 @@ use vmm::{
     rpc_interface::{PrebootApiController, RuntimeApiController, VmmAction},
     vmm_config::instance_info::InstanceInfo,
     Vmm,
+    CleanupLevel, CLEANUP_LEVEL
 };
 
 struct ApiServerAdapter {
@@ -251,16 +252,19 @@ pub(crate) fn run_with_api(
         &mut event_manager,
     );
 
+    // Note: By default, this point won't ever be reached...because the system can just
+    // call exit() and have the OS terminate all the threads.  It's faster.  But when the
+    // `--cleanup-level 2` is specified, this code runs.
+    //
+    assert!(*CLEANUP_LEVEL.read().unwrap() == CleanupLevel::Valgrind);
+
     // We want to tell the API thread to shut down for a clean exit.  But this is after
     // the Vmm.stop() has been called, so it's a moment of internal finalization (as
     // opposed to be something the client might call to shut the Vm down).  Since it's
     // an internal signal implementing it with an HTTP request is probably not the ideal
     // way to do it...but having another way would involve waiting on the socket or some
     // other signal.  This leverages the existing wait.
     //
-    // !!! Since the code is only needed for a "clean" shutdown mode, a non-clean mode
-    // could not respond to the request, making this effectively a debug-only feature.
-    //
     let mut sock = UnixStream::connect(bind_path).unwrap();
     assert!(sock.write_all(b"GET /shutdown-internal HTTP/1.1\r\n\r\n").is_ok());
 

src/firecracker/src/main.rs

@@ -16,6 +16,7 @@ use seccomp::{BpfProgram, SeccompLevel};
 use utils::arg_parser::{ArgParser, Argument};
 use utils::terminal::Terminal;
 use utils::validators::validate_instance_id;
+use vmm::{CleanupLevel, CLEANUP_LEVEL};
 use vmm::default_syscalls::get_seccomp_filter;
 use vmm::resources::VmResources;
 use vmm::signal_handler::{mask_handled_signals, SignalManager};
@@ -142,6 +143,15 @@ fn main_exitable() -> ExitCode {
             Argument::new("version")
                 .takes_value(false)
                 .help("Print the binary version number and a list of supported snapshot data format versions.")
+        )
+        .arg(
+            Argument::new("cleanup-level")
+                .takes_value(true)
+                .default_value("1")
+                .help(
+                    "How much cleanup on exit (0: abruptly call sys::exit | 1: minimal shutdown functions (default) \"
+                     | 2: wait for all threads to exit and all destructors to run (for use with Valgrind)."
+                ),
         );
 
     let arguments = match arg_parser.parse_from_cmdline() {
@@ -216,6 +226,15 @@ fn main_exitable() -> ExitCode {
         panic!("Could not create seccomp filter: {}", err);
     });
 
+    // The lazy_static! CLEANUP_LEVEL is only written here, but can be read from anywhere.
+    let cleanup_level_str = arguments.single_value("cleanup-level").unwrap();
+    {
+        let mut cleanup_level = CLEANUP_LEVEL.write().unwrap();
+        *cleanup_level = CleanupLevel::from_string(&cleanup_level_str).unwrap_or_else(|err| {
+            panic!("Invalid value for cleanup-level: {}", err);
+        });
+    }
+
     let vmm_config_json = arguments
         .single_value("config-file")
         .map(fs::read_to_string)

src/vmm/src/lib.rs

@@ -40,6 +40,9 @@ use std::sync::mpsc::{RecvTimeoutError, TryRecvError};
 use std::sync::Mutex;
 use std::time::Duration;
 
+use lazy_static::lazy_static;  // for CLEANUP_LEVEL arg to be globally visible
+use std::sync::RwLock;
+
 #[cfg(target_arch = "x86_64")]
 use crate::device_manager::legacy::PortIODeviceManager;
 use crate::device_manager::mmio::MMIODeviceManager;
@@ -93,6 +96,62 @@ pub const FC_EXIT_CODE_BAD_CONFIGURATION: u8 = 152;
 /// Command line arguments parsing error.
 pub const FC_EXIT_CODE_ARG_PARSING: u8 = 153;
 
+#[derive(Debug)]
+/// Possible errors that could be encountered while processing a cleanup level value
+pub enum CleanupLevelError {
+    /// Failed to parse to `u8`.
+    Parse(std::num::ParseIntError),
+    /// Cleanup level is an `u8` value, other than 0, 1 or 2.
+    Level(u8),
+}
+
+impl Display for CleanupLevelError {
+    fn fmt(&self, f: &mut Formatter) -> std::fmt::Result {
+        match *self {
+            CleanupLevelError::Parse(ref err) => {
+                write!(f, "Could not parse to 'u8': {}", err)
+            },
+            CleanupLevelError::Level(arg) => write!(
+                f,
+                "'{}' isn't a valid value for 'cleanup-level'. Must be 0, 1 or 2.",
+                arg
+            ),
+        }
+    }
+}
+
+/// Possible values for cleanup level.
+#[repr(u8)]
+#[derive(Clone, Copy, Debug, PartialEq)]
+pub enum CleanupLevel {
+    /// Abruptly call std::exit, so even successful shutdown is like a crash (fastest)
+    Abrupt = 0,
+    /// Normal shutdown, but don't worry about freeing memory or joining threads (let OS do it)
+    Default = 1,
+    /// Make sure all threads join and exit code bubbles up to main, for sanitizer accounting.
+    Valgrind = 2,
+}
+
+impl CleanupLevel {
+    /// Converts from a cleanup level value of type String to the corresponding CleanupLevel variant
+    /// or returns an error if the parsing failed.
+    pub fn from_string(cleanup_value: &str) -> std::result::Result<Self, CleanupLevelError> {
+        match cleanup_value.parse::<u8>() {
+            Ok(0) => Ok(CleanupLevel::Abrupt),
+            Ok(1) => Ok(CleanupLevel::Default),
+            Ok(2) => Ok(CleanupLevel::Valgrind),
+            Ok(level) => Err(CleanupLevelError::Level(level)),
+            Err(err) => Err(CleanupLevelError::Parse(err)),
+        }
+    }
+}
+
+lazy_static! {
+    /// Static instance for conveying the command-line `--cleanup-level` setting to the VMM.
+    pub static ref CLEANUP_LEVEL: RwLock<CleanupLevel> = RwLock::new(CleanupLevel::Default);
+}
+
+
 /// Errors associated with the VMM internal logic. These errors cannot be generated by direct user
 /// input, but can result from bad configuration of the host (for example if Firecracker doesn't
 /// have permissions to open the KVM fd).
@@ -353,12 +412,23 @@ impl Vmm {
             .map_err(Error::I8042Error)
     }
 
-    /// Waits for all vCPUs to exit.  Does not terminate the Firecracker process.
-    /// (See notes in main() about why ExitCode is bubbled up for clean shutdown.)
-    pub fn stop(&mut self) {
+    /// This stops the VMM.  Based on the setting of `--cleanup-level`, it may also exit the
+    /// Firecracker process entirely.  It does so by default (cleanup-level of 1), but for
+    /// sanity checking and Valgrind use it's important to offer a higher level of cleanliness
+    /// which can gracefully exit all threads and bubble the exit_code up to main().
+    ///
+    pub fn stop(&mut self, exit_code: ExitCode) -> ExitCode {
         info!("Vmm is stopping.");
 
-        self.exit_vcpus().unwrap();  // exit all not-already-exited VCPUs, join their threads
+        let cleanup_level = *CLEANUP_LEVEL.read().unwrap();
+
+        if cleanup_level == CleanupLevel::Abrupt {
+            //
+            // Most severe form of exiting (also the fastest), does not run any Rust shutdown
+            // that happens with `std::process::exit`.  Similar to crashing.
+            //
+            unsafe { libc::exit(i32::from(exit_code)); }
+        }
 
         if let Some(observer) = self.events_observer.as_mut() {
             if let Err(e) = observer.on_vmm_stop() {
@@ -370,6 +440,23 @@ impl Vmm {
         if let Err(e) = METRICS.write() {
             error!("Failed to write metrics while stopping: {}", e);
         }
+
+        if cleanup_level == CleanupLevel::Default {
+            //
+            // This runs as much shutdown code as Firecracker had before the inclusion of the
+            // ability to do CleanupLevel::Valgrind.
+            //
+            // !!! This preserves the usage of libc::exit() vs. std::process::exit(), just to
+            // keep it the same.  But was that initial choice intentional?
+            //
+            unsafe { libc::exit(i32::from(exit_code)); }
+        }
+
+        assert!(cleanup_level == CleanupLevel::Valgrind);  // bubble up exit_code to main()
+
+        self.exit_vcpus().unwrap();  // exit all not-already-exited VCPUs, join their threads
+
+        return exit_code
     }
 
     /// Saves the state of a paused Microvm.
@@ -777,6 +864,12 @@ impl Subscriber for Vmm {
                 }
             }
 
+            // If the exit_code can't be found on any vcpu, it means that the exit signal
+            // has been issued by the i8042 controller in which case we exit with
+            // FC_EXIT_CODE_OK.
+            //
+            let exit_code = opt_exit_code.unwrap_or(FC_EXIT_CODE_OK);
+
             // !!! The caller of this routine is receiving the exit code to bubble back up
             // to the main() function to return cleanly.  However, it does not have clean
             // access to the Vmm to shut it down (here we have it, since it is `self`).  It
@@ -787,13 +880,7 @@ impl Subscriber for Vmm {
             // that will actually work with an exit code (all other Subscriber trait
             // implementers must use process())
             //
-            self.stop();
-
-            // If the exit_code can't be found on any vcpu, it means that the exit signal
-            // has been issued by the i8042 controller in which case we exit with
-            // FC_EXIT_CODE_OK.
-            //
-            Some(opt_exit_code.unwrap_or(FC_EXIT_CODE_OK))
+            Some(self.stop(exit_code))  // may exit abruptly, depending on CLEANUP_LEVEL
         } else {
             error!("Spurious EventManager event for handler: Vmm");
             None