Shutdown API server thread so it can thread::join()

The general presumption for Firecracker was that it would be exited
via an exit() call.  This meant there was no way to signal threads
to exit their loops so they could be join()'d to release their
resources.

Not being able to exit the stack normally (if one wants to) misses
a sanity check that shutting down cleanly provides.  The output of
Valgrind and other tools is also less useful.

Because the API Server thread is in a loop that does a blocking
wait on a socket, the only way to tell it to stop looping is via
information on that socket.  This uses an internal HTTP GET signal
to request that shutdown.  Rather than forget the thread handle
and the socket bind location, the main thread now sends the
shutdown request and then waits while the thread joins.

The request is for internal use only, however--because it is
supposed to happen after the Vmm has been cleaned up and its
contents destructed.

A different approach for signaling the thread for this purpose
could be used, e.g. to have the API server thread waiting on
multiple events (the socket or this internal signal).  But since
this is only needed for clean shutdown, it is probably good enough.
A faster shutdown using exit() would be a better default--this
will likely only be used for debugging and Valgrind/sanitization.

Author: AE1020

src/api_server/src/lib.rs

@@ -198,67 +198,92 @@ impl ApiServer {
         }
 
         server.start_server().expect("Cannot start HTTP server");
+
         loop {
-            match server.requests() {
-                Ok(request_vec) => {
-                    for server_request in request_vec {
-                        let request_processing_start_us =
-                            utils::time::get_time_us(utils::time::ClockType::Monotonic);
-                        server
-                            .respond(
-                                // Use `self.handle_request()` as the processing callback.
-                                server_request.process(|request| {
-                                    self.handle_request(request, request_processing_start_us)
-                                }),
-                            )
-                            .or_else(|e| {
-                                error!("API Server encountered an error on response: {}", e);
-                                Ok(())
-                            })?;
-                        let delta_us = utils::time::get_time_us(utils::time::ClockType::Monotonic)
-                            - request_processing_start_us;
-                        debug!("Total previous API call duration: {} us.", delta_us);
-                        if self.vmm_fatal_error {
-                            // Flush the remaining outgoing responses
-                            // and proceed to exit
-                            server.flush_outgoing_writes();
-                            error!(
-                                "Fatal error with exit code: {}",
-                                FC_EXIT_CODE_BAD_CONFIGURATION
-                            );
-                            unsafe {
-                                libc::_exit(i32::from(FC_EXIT_CODE_BAD_CONFIGURATION));
-                            }
-                        }
-                    }
-                }
-                Err(e) => {
+            let request_vec = match server.requests() {
+                Ok(vec) => vec,
+                Err(e) => {  // print request error, but keep server running
                     error!(
                         "API Server error on retrieving incoming request. Error: {}",
                         e
                     );
+                    continue
+                }
+            };
+
+            for server_request in request_vec {
+                let request_processing_start_us =
+                    utils::time::get_time_us(utils::time::ClockType::Monotonic);
+
+                // !!! ServerRequest::process() doesn't appear to do anything besides put a private
+                // id field into the response.  But since it takes an inner function it cannot
+                // return from this function without giving a response.  Review a better way of
+                // exiting cleanly than by reaching out of the closure to set a boolean.
+
+                let mut shutting_down = false;
+
+                server
+                    .respond(
+                        server_request.process(|request| {
+                            match self.handle_request(request, request_processing_start_us) {
+                                Some(response) => response,
+                                None => {
+                                    shutting_down = true;
+                                    Response::new(Version::Http11, StatusCode::NoContent)
+                                }
+                            }
+                        }),
+                    )
+                    .or_else(|e| {
+                        error!("API Server encountered an error on response: {}", e);
+                        Ok(())
+                    })?;
+
+                let delta_us = utils::time::get_time_us(utils::time::ClockType::Monotonic)
+                    - request_processing_start_us;
+                debug!("Total previous API call duration: {} us.", delta_us);
+
+                if shutting_down {
+                    debug!("/shutdown-internal request received, API server thread now ending itself");
+                    return Ok(());
+                }
+
+                if self.vmm_fatal_error {
+                    // Flush the remaining outgoing responses
+                    // and proceed to exit
+                    server.flush_outgoing_writes();
+                    error!(
+                        "Fatal error with exit code: {}",
+                        FC_EXIT_CODE_BAD_CONFIGURATION
+                    );
+                    unsafe {
+                        libc::_exit(i32::from(FC_EXIT_CODE_BAD_CONFIGURATION));
+                    }
                 }
             }
         }
     }
 
     /// Handles an API request received through the associated socket.
+    /// If None is given back, it means a ShutdownInternal was requested and no response
+    /// is expected (should be requested by the main thread, not by clients)
     pub fn handle_request(
         &mut self,
         request: &Request,
         request_processing_start_us: u64,
-    ) -> Response {
+    ) -> Option<Response> {
         match ParsedRequest::try_from_request(request) {
             Ok(ParsedRequest::Sync(vmm_action)) => {
-                self.serve_vmm_action_request(vmm_action, request_processing_start_us)
+                Some(self.serve_vmm_action_request(vmm_action, request_processing_start_us))
             }
-            Ok(ParsedRequest::GetInstanceInfo) => self.get_instance_info(),
-            Ok(ParsedRequest::GetMMDS) => self.get_mmds(),
-            Ok(ParsedRequest::PatchMMDS(value)) => self.patch_mmds(value),
-            Ok(ParsedRequest::PutMMDS(value)) => self.put_mmds(value),
+            Ok(ParsedRequest::GetInstanceInfo) => Some(self.get_instance_info()),
+            Ok(ParsedRequest::GetMMDS) => Some(self.get_mmds()),
+            Ok(ParsedRequest::PatchMMDS(value)) => Some(self.patch_mmds(value)),
+            Ok(ParsedRequest::PutMMDS(value)) => Some(self.put_mmds(value)),
+            Ok(ParsedRequest::ShutdownInternal) => None,
             Err(e) => {
                 error!("{}", e);
-                e.into()
+                Some(e.into())
             }
         }
     }

src/api_server/src/parsed_request.rs

@@ -31,6 +31,7 @@ pub(crate) enum ParsedRequest {
     PatchMMDS(Value),
     PutMMDS(Value),
     Sync(Box<VmmAction>),
+    ShutdownInternal,  // !!! not an API, used by shutdown to thread::join the API thread
 }
 
 impl ParsedRequest {
@@ -57,6 +58,7 @@ 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, "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

@@ -7,6 +7,9 @@ use std::{
     sync::mpsc::{channel, Receiver, Sender, TryRecvError},
     sync::{Arc, Mutex},
     thread,
+
+    os::unix::net::UnixStream,  // for main thread to send ShutdownInternal to API thread
+    io::prelude::*,  // UnixStream write_all() requires prelude
 };
 
 use api_server::{ApiRequest, ApiResponse, ApiServer};
@@ -146,9 +149,10 @@ pub(crate) fn run_with_api(
         .try_clone()
         .expect("Failed to clone API event FD");
 
+    let api_bind_path = bind_path.clone();
     let api_seccomp_filter = seccomp_filter.clone();
     // Start the separate API thread.
-    thread::Builder::new()
+    let api_thread = thread::Builder::new()
         .name("fc_api".to_owned())
         .spawn(move || {
             mask_handled_signals().expect("Unable to install signal mask on API thread.");
@@ -160,7 +164,7 @@ pub(crate) fn run_with_api(
                 from_vmm,
                 to_vmm_event_fd,
             )
-            .bind_and_run(bind_path, process_time_reporter, api_seccomp_filter)
+            .bind_and_run(api_bind_path, process_time_reporter, api_seccomp_filter)
             {
                 Ok(_) => (),
                 Err(api_server::Error::Io(inner)) => match inner.kind() {
@@ -238,12 +242,33 @@ pub(crate) fn run_with_api(
         .expect("Poisoned lock")
         .start(super::metrics::WRITE_METRICS_PERIOD_MS);
 
-    ApiServerAdapter::run_microvm(
+    let exit_code = ApiServerAdapter::run_microvm(
         api_event_fd,
         from_api,
         to_api,
         vm_resources,
         vmm,
         &mut event_manager,
-    )
+    );
+
+    // 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());
+
+    // This call to thread::join() should block until the API thread has processed the
+    // shutdown-internal and returns from its function.  If it doesn't block here, then
+    // that means it got the message; so no need to process a response.
+    //
+    api_thread.join().unwrap();
+
+    exit_code
 }