From d647557dde67c1d82e8bccd671b98c0e9e4e6d36 Mon Sep 17 00:00:00 2001 From: Benjamin Naecker Date: Thu, 28 Sep 2023 21:26:42 +0000 Subject: [PATCH] Fix path checks for archived log files - Fixes #4160. - Checks for archived log files were wrong. This used the SMF FMRI, rather than the derived log filename, which translates slashes into dashes. This separates checks for Oxide-managed FMRIs and the log files for those. - Use the _log file_ check when looking for archived files. - Add tests for both checks and the method for finding archived log files for a service. --- Cargo.lock | 1 + illumos-utils/src/running_zone.rs | 55 +++++++++++++++++++++---- sled-agent/Cargo.toml | 1 + sled-agent/src/zone_bundle.rs | 67 +++++++++++++++++++++++++++++-- 4 files changed, 113 insertions(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e5130b6b33..44142472e8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5254,6 +5254,7 @@ dependencies = [ "static_assertions", "subprocess", "tar", + "tempfile", "thiserror", "tofino", "tokio", diff --git a/illumos-utils/src/running_zone.rs b/illumos-utils/src/running_zone.rs index 4d3481b6c3..734f22bd30 100644 --- a/illumos-utils/src/running_zone.rs +++ b/illumos-utils/src/running_zone.rs @@ -987,7 +987,7 @@ impl RunningZone { let output = self.run_cmd(&["svcs", "-H", "-o", "fmri"])?; Ok(output .lines() - .filter(|line| is_oxide_smf_log_file(line)) + .filter(|line| is_oxide_smf_service(line)) .map(|line| line.trim().to_string()) .collect()) } @@ -1267,10 +1267,51 @@ impl InstalledZone { } } -/// Return true if the named file appears to be a log file for an Oxide SMF -/// service. -pub fn is_oxide_smf_log_file(name: impl AsRef) -> bool { - const SMF_SERVICE_PREFIXES: [&str; 2] = ["/oxide", "/system/illumos"]; - let name = name.as_ref(); - SMF_SERVICE_PREFIXES.iter().any(|needle| name.contains(needle)) +/// Return true if the service with the given FMRI appears to be an +/// Oxide-managed service. +pub fn is_oxide_smf_service(fmri: impl AsRef) -> bool { + const SMF_SERVICE_PREFIXES: [&str; 2] = + ["svc:/oxide/", "svc:/system/illumos/"]; + let fmri = fmri.as_ref(); + SMF_SERVICE_PREFIXES.iter().any(|prefix| fmri.starts_with(prefix)) +} + +/// Return true if the provided file name appears to be a valid log file for an +/// Oxide-managed SMF service. +/// +/// Note that this operates on the _file name_. Any leading path components will +/// cause this check to return `false`. +pub fn is_oxide_smf_log_file(filename: impl AsRef) -> bool { + // Log files are named by the SMF services, with the `/` in the FMRI + // translated to a `-`. + const PREFIXES: [&str; 2] = ["oxide-", "system-illumos-"]; + let filename = filename.as_ref(); + PREFIXES + .iter() + .any(|prefix| filename.starts_with(prefix) && filename.contains(".log")) +} + +#[cfg(test)] +mod tests { + use super::is_oxide_smf_log_file; + use super::is_oxide_smf_service; + + #[test] + fn test_is_oxide_smf_service() { + assert!(is_oxide_smf_service("svc:/oxide/blah:default")); + assert!(is_oxide_smf_service("svc:/system/illumos/blah:default")); + assert!(!is_oxide_smf_service("svc:/system/blah:default")); + assert!(!is_oxide_smf_service("svc:/not/oxide/blah:default")); + } + + #[test] + fn test_is_oxide_smf_log_file() { + assert!(is_oxide_smf_log_file("oxide-blah:default.log")); + assert!(is_oxide_smf_log_file("oxide-blah:default.log.0")); + assert!(is_oxide_smf_log_file("oxide-blah:default.log.1111")); + assert!(is_oxide_smf_log_file("system-illumos-blah:default.log")); + assert!(is_oxide_smf_log_file("system-illumos-blah:default.log.0")); + assert!(!is_oxide_smf_log_file("not-oxide-blah:default.log")); + assert!(!is_oxide_smf_log_file("not-system-illumos-blah:default.log")); + } } diff --git a/sled-agent/Cargo.toml b/sled-agent/Cargo.toml index f172136726..840ffe243f 100644 --- a/sled-agent/Cargo.toml +++ b/sled-agent/Cargo.toml @@ -94,6 +94,7 @@ serial_test.workspace = true subprocess.workspace = true slog-async.workspace = true slog-term.workspace = true +tempfile.workspace = true illumos-utils = { workspace = true, features = ["testing"] } diff --git a/sled-agent/src/zone_bundle.rs b/sled-agent/src/zone_bundle.rs index 2eeb8ebe7d..4c2d6a4113 100644 --- a/sled-agent/src/zone_bundle.rs +++ b/sled-agent/src/zone_bundle.rs @@ -899,9 +899,9 @@ async fn find_archived_log_files( continue; }; let fname = path.file_name().unwrap(); - if is_oxide_smf_log_file(fname) - && fname.contains(svc_name) - { + let is_oxide = is_oxide_smf_log_file(fname); + let contains = fname.contains(svc_name); + if is_oxide && contains { debug!( log, "found archived log file"; @@ -910,6 +910,14 @@ async fn find_archived_log_files( "path" => ?path, ); files.push(path); + } else { + debug!( + log, + "skipping non-matching log file"; + "filename" => fname, + "is_oxide_smf_log_file" => is_oxide, + "contains_svc_name" => contains, + ); } } Err(e) => { @@ -1764,6 +1772,7 @@ mod tests { #[cfg(all(target_os = "illumos", test))] mod illumos_tests { + use super::find_archived_log_files; use super::zfs_quota; use super::CleanupContext; use super::CleanupPeriod; @@ -1852,12 +1861,17 @@ mod illumos_tests { } } - async fn setup_fake_cleanup_task() -> anyhow::Result { + fn test_logger() -> Logger { let dec = slog_term::PlainSyncDecorator::new(slog_term::TestStdoutWriter); let drain = slog_term::FullFormat::new(dec).build().fuse(); let log = Logger::root(drain, slog::o!("component" => "fake-cleanup-task")); + log + } + + async fn setup_fake_cleanup_task() -> anyhow::Result { + let log = test_logger(); let context = CleanupContext::default(); let resource_wrapper = ResourceWrapper::new().await; let bundler = @@ -2279,4 +2293,49 @@ mod illumos_tests { let bytes = tokio::fs::metadata(&path).await?.len(); Ok(ZoneBundleInfo { metadata, path, bytes }) } + + #[tokio::test] + async fn test_find_archived_log_files() { + let log = test_logger(); + let tmpdir = tempfile::tempdir().expect("Failed to make tempdir"); + + let mut should_match = [ + "oxide-foo:default.log", + "oxide-foo:default.log.1000", + "system-illumos-foo:default.log", + "system-illumos-foo:default.log.100", + ]; + let should_not_match = [ + "oxide-foo:default", + "not-oxide-foo:default.log.1000", + "system-illumos-foo", + "not-system-illumos-foo:default.log.100", + ]; + for name in should_match.iter().chain(should_not_match.iter()) { + let path = tmpdir.path().join(name); + tokio::fs::File::create(path) + .await + .expect("failed to create dummy file"); + } + + let path = + Utf8PathBuf::try_from(tmpdir.path().as_os_str().to_str().unwrap()) + .unwrap(); + let mut files = find_archived_log_files( + &log, + "zone-name", // unused here, for logging only + "foo", + &[path], + ) + .await; + + // Sort everything to compare correctly. + should_match.sort(); + files.sort(); + assert_eq!(files.len(), should_match.len()); + assert!(files + .iter() + .zip(should_match.iter()) + .all(|(file, name)| { file.file_name().unwrap() == *name })); + } }