Fix path checks for archived log files

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<str>) -> 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<str>) -> 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<str>) -> 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"));
+    }
 }

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"] }
 

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<CleanupTestContext> {
+    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<CleanupTestContext> {
+        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 }));
+    }
 }