Setup log and store dir with systemd (#8241)

Summary:
Pull Request resolved: #8241

Logging and storage directory is better setup by systemd. If the log dir is not created, let's just log errors to some temporary file instead.

Reviewed By: dschatzberg

Differential Revision: D70026615

fbshipit-source-id: 412444c8135af54edc516164a818ecd72ecefb4a

Author: lnyng

below/Cargo.toml

@@ -44,7 +44,6 @@ store = { package = "below-store", version = "0.8.1", path = "store" }
 tar = "0.4.43"
 tempfile = "3.15"
 tokio = { version = "1.41.0", features = ["full", "test-util", "tracing"] }
-uzers = "0.11.3"
 view = { package = "below-view", version = "0.8.1", path = "view" }
 
 [dev-dependencies]

below/config/src/lib.rs

@@ -56,8 +56,11 @@ pub struct BelowConfig {
 impl Default for BelowConfig {
     fn default() -> Self {
         BelowConfig {
-            log_dir: BELOW_DEFAULT_LOG.into(),
-            store_dir: BELOW_DEFAULT_STORE.into(),
+            log_dir: std::env::var_os("LOGS_DIRECTORY")
+                .map_or(BELOW_DEFAULT_LOG.into(), PathBuf::from),
+            store_dir: std::env::var_os("LOGS_DIRECTORY").map_or(BELOW_DEFAULT_STORE.into(), |d| {
+                PathBuf::from(d).join("store")
+            }),
             cgroup_root: cgroupfs::DEFAULT_CG_ROOT.into(),
             cgroup_filter_out: String::new(),
             enable_gpu_stats: false,

below/src/main.rs

@@ -19,7 +19,6 @@
 use std::cell::RefCell;
 use std::fs;
 use std::io;
-use std::os::unix::fs::PermissionsExt;
 use std::path::PathBuf;
 use std::process::exit;
 use std::rc::Rc;
@@ -54,8 +53,6 @@ use tar::Archive;
 use tar::Builder as TarBuilder;
 use tempfile::TempDir;
 use tokio::runtime::Builder as TB;
-use uzers::get_current_uid;
-use uzers::get_user_by_uid;
 
 mod exitstat;
 #[cfg(test)]
@@ -331,13 +328,6 @@ pub enum Service {
     Off,
 }
 
-// Whether or not to redirect log to stderr on fs failure
-#[derive(PartialEq)]
-pub enum RedirectLogOnFail {
-    On,
-    Off,
-}
-
 fn bump_memlock_rlimit() -> Result<()> {
     // TODO(T78976996) remove the fbcode_gate once we can exit stats is
     // enabled for opensource
@@ -355,44 +345,6 @@ fn bump_memlock_rlimit() -> Result<()> {
     Ok(())
 }
 
-fn create_log_dir(path: &PathBuf) -> Result<()> {
-    if path.exists() && !path.is_dir() {
-        bail!("{} exists and is not a directory", path.to_string_lossy());
-    }
-
-    if !path.is_dir() {
-        match fs::create_dir_all(path) {
-            Ok(()) => {}
-            Err(e) => {
-                bail!(
-                    "Failed to create dir {}: {}\nTry sudo.",
-                    path.to_string_lossy(),
-                    e
-                );
-            }
-        }
-    }
-
-    let dir = fs::File::open(path).unwrap();
-    let mut perm = dir.metadata().unwrap().permissions();
-
-    if perm.mode() & 0o777 != 0o777 {
-        perm.set_mode(0o777);
-        match dir.set_permissions(perm) {
-            Ok(()) => {}
-            Err(e) => {
-                bail!(
-                    "Failed to set permissions on {}: {}",
-                    path.to_string_lossy(),
-                    e
-                );
-            }
-        }
-    }
-
-    Ok(())
-}
-
 // Exitstat runs a bpf program that hooks into process exit events. This allows below to keep
 // track of processes whose lifetimes are shorter than polling interval.
 fn start_exitstat(
@@ -407,7 +359,7 @@ fn start_exitstat(
         .spawn(move || {
             match exit_driver.drive() {
                 Ok(_) => {}
-                Err(e) => bpf_err_send.send(e).unwrap(),
+                Err(e) => bpf_err_send.send(e).expect("Failed to send error"),
             };
         })
         .expect("Failed to spawn thread");
@@ -605,24 +557,14 @@ pub fn run<F>(
     debug: bool,
     below_config: &BelowConfig,
     _service: Service,
-    redirect: RedirectLogOnFail,
     command: F,
 ) -> i32
 where
     F: FnOnce(init::InitToken, &BelowConfig, slog::Logger, Receiver<Error>) -> Result<()>,
 {
     let (err_sender, err_receiver) = channel();
-    let mut log_dir = below_config.log_dir.clone();
-    let user = get_user_by_uid(get_current_uid()).expect("Failed to get current user for logging");
-
-    log_dir.push(format!("error_{}.log", user.name().to_string_lossy()));
-
-    if let Err(e) = create_log_dir(&below_config.log_dir) {
-        eprintln!("{:#}", e);
-        return 1;
-    }
-
-    let logger = logging::setup(init, log_dir, debug, redirect);
+    let log_path = below_config.log_dir.join("below.log");
+    let logger = logging::setup(init, log_path, debug);
     setup_log_on_panic(logger.clone());
 
     match Signals::new([signal_hook::consts::SIGINT, signal_hook::consts::SIGTERM]) {
@@ -640,7 +582,9 @@ where
                         }
                         term_now = true;
                         error!(logger, "Stop signal received: {}, exiting.", signal);
-                        err_sender.send(anyhow!(StopSignal { signal })).unwrap();
+                        err_sender
+                            .send(anyhow!(StopSignal { signal }))
+                            .expect("Failed to send stop signal");
                     }
                 })
                 .expect("Failed to spawn thread");
@@ -731,7 +675,6 @@ fn real_main(init: init::InitToken) {
                 debug,
                 below_config,
                 Service::Off,
-                RedirectLogOnFail::On,
                 |_, below_config, logger, errs| {
                     live(
                         init,
@@ -763,7 +706,6 @@ fn real_main(init: init::InitToken) {
                 debug,
                 below_config,
                 Service::On(*port),
-                RedirectLogOnFail::Off,
                 |init, below_config, logger, errs| {
                     record(
                         init,
@@ -800,7 +742,6 @@ fn real_main(init: init::InitToken) {
                 debug,
                 below_config,
                 Service::Off,
-                RedirectLogOnFail::Off,
                 |_, below_config, logger, errs| {
                     replay(
                         logger,
@@ -834,7 +775,6 @@ fn real_main(init: init::InitToken) {
                 debug,
                 below_config,
                 Service::Off,
-                RedirectLogOnFail::Off,
                 |_, below_config, logger, _errs| {
                     snapshot(
                         logger,
@@ -858,7 +798,6 @@ fn real_main(init: init::InitToken) {
                     debug,
                     below_config,
                     Service::Off,
-                    RedirectLogOnFail::Off,
                     |_, below_config, logger, _errs| dump_store(logger, time, below_config, json),
                 )
             }
@@ -884,7 +823,6 @@ fn real_main(init: init::InitToken) {
                     debug,
                     below_config,
                     Service::Off,
-                    RedirectLogOnFail::Off,
                     |_, below_config, logger, _errs| {
                         convert_store(
                             logger,
@@ -918,7 +856,6 @@ fn real_main(init: init::InitToken) {
                 debug,
                 below_config,
                 Service::Off,
-                RedirectLogOnFail::Off,
                 |_, _below_config, logger, errs| {
                     dump::run(logger, errs, store_dir, host, port, snapshot, cmd)
                 },
@@ -961,7 +898,7 @@ fn replay(
             tarball.unpack(&snapshot_dir)?;
             // Find and append the name of the original snapshot directory
             for path in fs::read_dir(&snapshot_dir)? {
-                snapshot_dir.push(path.unwrap().file_name());
+                snapshot_dir.push(path?.file_name());
             }
             new_advance_local(logger.clone(), snapshot_dir, timestamp)
         }

below/src/open_source/logging.rs

@@ -23,60 +23,37 @@ use slog::Drain;
 use crate::init::InitToken;
 use crate::logutil::CommandPaletteDrain;
 use crate::logutil::CompoundDecorator;
-use crate::RedirectLogOnFail;
 
-fn setup_log<T: 'static + std::io::Write + std::marker::Send>(
-    _init: InitToken,
-    file: T,
-    _debug: bool,
-    error: Option<std::io::Error>,
-) -> slog::Logger {
+fn setup_log(_init: InitToken, file: std::fs::File, _debug: bool) -> slog::Logger {
     let decorator = CompoundDecorator::new(file, std::io::stderr());
     let drain = slog_term::FullFormat::new(decorator).build().fuse();
     let drain = CommandPaletteDrain::new(drain).fuse();
 
-    let logger = slog::Logger::root(drain, o!());
-
-    // When we want to redirect the log, also log the open file err to stderr
-    if let Some(e) = error {
-        error!(
-            logger,
-            "Fail to open log path: {}\n.Redirecting all log to stderr.", e
-        );
-    }
-
-    logger
+    slog::Logger::root(drain, o!())
 }
 
-pub fn setup(
-    init: InitToken,
-    path: PathBuf,
-    debug: bool,
-    redirect: RedirectLogOnFail,
-) -> slog::Logger {
-    let file_maybe = OpenOptions::new().create(true).append(true).open(path);
-
-    if let Ok(file) = file_maybe.as_ref() {
-        // We don't need to worry about the permission setting here since
-        // as long as the FS is writable, user can run with sudo to reset
-        // file permission.
-        let mut perms = file
-            .metadata()
-            .expect("failed to get file metadata for logfile")
-            .permissions();
-        if perms.mode() & 0o777 != 0o666 {
-            // World readable/writable -- the devil's permissions
-            perms.set_mode(0o666);
-            file.set_permissions(perms)
-                .expect("failed to set permissions on logfile");
+pub fn setup(init: InitToken, path: PathBuf, debug: bool) -> slog::Logger {
+    let file = match OpenOptions::new().create(true).append(true).open(path) {
+        Ok(f) => f,
+        Err(_) => {
+            let temp_log_path = tempfile::Builder::new()
+                .prefix("below.log.")
+                .keep(true)
+                .tempfile()
+                .expect("Failed to create tempfile")
+                .path()
+                .to_path_buf();
+            eprintln!(
+                "Log path unavailable. Logging to {}",
+                temp_log_path.to_string_lossy()
+            );
+            OpenOptions::new()
+                .create(true)
+                .append(true)
+                .open(temp_log_path)
+                .expect("Failed to open tempfile")
         }
-    } else if redirect == RedirectLogOnFail::Off {
-        // No redirect, let it crash.
-        file_maybe.as_ref().expect("Failed to open log path");
-    }
+    };
 
-    match file_maybe {
-        Ok(f) => setup_log(init, f, debug, None),
-        Err(e) => setup_log(init, std::io::stderr(), debug, Some(e)),
-    }
+    setup_log(init, file, debug)
 }

etc/below.service

@@ -17,6 +17,7 @@ Description=below system monitor recording daemon
 After=time-sync.target
 
 [Service]
+LogsDirectory=below
 ExecStart=/usr/bin/below record --retain-for-s 604800 --compress
 # Enable backtraces in errors
 Environment=RUST_LIB_BACKTRACE=1