Skip to content

Commit 0d56d0c

Browse files
wfchandlerwfchandler
committed
Avoid getting mtime twice
Oxlog is already collecting the file mtime for us when it find the logs. Avoid `stat`ing the files a second time and pass the cached time to the zip archive.
1 parent dc4d978 commit 0d56d0c

File tree

6 files changed

+57
-78
lines changed

6 files changed

+57
-78
lines changed

‎Cargo.lock‎

Lines changed: 3 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎nexus/Cargo.toml‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ internal-dns-resolver.workspace = true
5252
internal-dns-types.workspace = true
5353
ipnetwork.workspace = true
5454
itertools.workspace = true
55+
jiff.workspace = true
5556
lldpd_client.workspace = true
5657
macaddr.workspace = true
5758
maplit.workspace = true
@@ -143,7 +144,7 @@ update-common.workspace = true
143144
update-engine.workspace = true
144145
omicron-workspace-hack.workspace = true
145146
omicron-uuid-kinds.workspace = true
146-
zip = { workspace = true, features = ["chrono"] }
147+
zip = { workspace = true, features = ["jiff-02"] }
147148

148149
[dev-dependencies]
149150
async-bb8-diesel.workspace = true

‎nexus/src/app/background/tasks/support_bundle_collector.rs‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1159,9 +1159,10 @@ fn recursively_add_directory_to_zipfile(
11591159
let file_type = entry.file_type()?;
11601160
if file_type.is_file() {
11611161
let src = entry.path();
1162-
let mtime: chrono::DateTime<chrono::Utc> =
1163-
src.metadata().and_then(|s| s.modified())?.into();
1164-
let zip_time = zip::DateTime::try_from(mtime.naive_utc())?;
1162+
1163+
let system_mtime = entry.metadata().and_then(|m| m.modified())?;
1164+
let zoned = jiff::Zoned::try_from(system_mtime)?;
1165+
let zip_time = zip::DateTime::try_from(zoned.datetime())?;
11651166

11661167
let opts = FullFileOptions::default()
11671168
.last_modified_time(zip_time)

‎sled-diagnostics/Cargo.toml‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ chrono.workspace = true
1414
fs-err = { workspace = true, features = ["tokio"] }
1515
futures.workspace = true
1616
illumos-utils.workspace = true
17+
jiff.workspace = true
1718
libc.workspace = true
1819
omicron-workspace-hack.workspace = true
1920
once_cell.workspace = true
@@ -26,7 +27,7 @@ serde.workspace = true
2627
slog.workspace = true
2728
thiserror.workspace = true
2829
tokio = { workspace = true, features = ["full"] }
29-
zip = { workspace = true, features = ["chrono","zstd"] }
30+
zip = { workspace = true, features = ["jiff-02","zstd"] }
3031

3132
[dev-dependencies]
3233
omicron-common.workspace = true

‎sled-diagnostics/src/logs.rs‎

Lines changed: 44 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ use std::{
1010
sync::LazyLock,
1111
};
1212

13-
use anyhow::Context;
1413
use camino::{Utf8Path, Utf8PathBuf};
1514
use fs_err::File;
1615
use illumos_utils::zfs::{
@@ -452,11 +451,11 @@ impl LogsHandle {
452451
service: &str,
453452
zip: &mut zip::ZipWriter<W>,
454453
log_snapshots: &mut LogSnapshots,
455-
logfile: &Utf8Path,
454+
logfile: &LogFile,
456455
logtype: LogType,
457456
) -> Result<(), LogError> {
458457
let snapshot_logfile =
459-
self.find_log_in_snapshot(log_snapshots, logfile).await?;
458+
self.find_log_in_snapshot(log_snapshots, &logfile.path).await?;
460459

461460
if logtype == LogType::Current {
462461
// Since we are processing the current log files in a zone we need
@@ -502,13 +501,24 @@ impl LogsHandle {
502501
.filter(|f| is_log_file(f.path(), filename))
503502
{
504503
let logfile = f.path();
504+
let system_mtime =
505+
f.metadata().and_then(|m| m.modified()).inspect_err(|e| {
506+
warn!(&self.log, "sled-diagnostic failed to get mtime of logfile";
507+
"error" => %e,
508+
"logfile" => %logfile,
509+
);
510+
}).ok();
511+
let mtime = system_mtime
512+
.and_then(|m| jiff::Timestamp::try_from(m).ok());
513+
505514
if logfile.is_file() {
506515
write_log_to_zip(
507516
&self.log,
508517
service,
509518
zip,
510519
LogType::Current,
511520
logfile,
521+
mtime,
512522
)?;
513523
}
514524
}
@@ -523,6 +533,7 @@ impl LogsHandle {
523533
zip,
524534
logtype,
525535
&snapshot_logfile,
536+
logfile.modified,
526537
)?;
527538
}
528539
false => {
@@ -613,7 +624,7 @@ impl LogsHandle {
613624
&service,
614625
&mut zip,
615626
&mut log_snapshots,
616-
&current.path,
627+
&current,
617628
LogType::Current,
618629
)
619630
.await?;
@@ -629,13 +640,13 @@ impl LogsHandle {
629640
.archived
630641
.into_iter()
631642
.filter(|log| log.path.as_str().contains("crypt/debug"))
632-
.map(|log| log.path)
633643
.collect();
634644

635645
// Since these logs can be spread out across multiple U.2 devices
636646
// we need to sort them by timestamp.
637647
archived.sort_by_key(|log| {
638-
log.as_str()
648+
log.path
649+
.as_str()
639650
.rsplit_once(".")
640651
.and_then(|(_, date)| date.parse::<u64>().ok())
641652
.unwrap_or(0)
@@ -704,6 +715,7 @@ fn write_log_to_zip<W: Write + Seek>(
704715
zip: &mut zip::ZipWriter<W>,
705716
logtype: LogType,
706717
snapshot_logfile: &Utf8Path,
718+
mtime: Option<jiff::Timestamp>,
707719
) -> Result<(), LogError> {
708720
let Some(log_name) = snapshot_logfile.file_name() else {
709721
warn!(
@@ -717,23 +729,18 @@ fn write_log_to_zip<W: Write + Seek>(
717729

718730
let mut src = File::open(&snapshot_logfile)?;
719731

720-
let mtime = get_log_mtime(snapshot_logfile)
721-
.inspect_err(|e| {
722-
warn!(
723-
logger,
724-
"sled-diagnostics unable to get mtime for logfile";
725-
"error" => %e,
726-
"logfile" => %snapshot_logfile,
727-
);
732+
let zip_mtime = mtime
733+
.and_then(|ts| {
734+
let zoned = ts.in_tz("UTC").ok()?;
735+
zip::DateTime::try_from(zoned.datetime()).ok()
728736
})
729-
.ok()
730-
.unwrap_or_else(zip::DateTime::default_for_write);
737+
.unwrap_or_else(zip::DateTime::default);
731738

732739
let zip_path = format!("{service}/{logtype}/{log_name}");
733740
zip.start_file_from_path(
734741
zip_path,
735742
FullFileOptions::default()
736-
.last_modified_time(mtime)
743+
.last_modified_time(zip_mtime)
737744
.compression_method(zip::CompressionMethod::Zstd)
738745
.compression_level(Some(3))
739746
// NB: From the docs
@@ -761,17 +768,6 @@ fn write_log_to_zip<W: Write + Seek>(
761768
Ok(())
762769
}
763770

764-
fn get_log_mtime(log_path: &Utf8Path) -> anyhow::Result<zip::DateTime> {
765-
let mtime = log_path
766-
.metadata()
767-
.and_then(|s| s.modified())
768-
.context("failed to stat path")?;
769-
770-
let datetime: chrono::DateTime<chrono::Utc> = mtime.into();
771-
zip::DateTime::try_from(datetime.naive_utc())
772-
.context("failed to convert file mtime to zip-compatible time")
773-
}
774-
775771
/// A log file that is found in oxlog's "extra" bucket of service logs.
776772
#[derive(Debug, PartialEq)]
777773
enum ExtraLogKind<'a> {
@@ -783,8 +779,8 @@ enum ExtraLogKind<'a> {
783779

784780
#[derive(Debug, Default, PartialEq)]
785781
struct ExtraLogs<'a> {
786-
current: Option<&'a Utf8Path>,
787-
rotated: Vec<&'a Utf8Path>,
782+
current: Option<&'a LogFile>,
783+
rotated: Vec<&'a LogFile>,
788784
}
789785

790786
fn sort_extra_logs<'a>(
@@ -806,15 +802,15 @@ fn sort_extra_logs<'a>(
806802
warn!(
807803
logger,
808804
"found multiple current log files for {name}";
809-
"old" => %old_path,
805+
"old" => %old_path.path,
810806
"new" => %log.path,
811807
);
812808
}
813-
entry.current = Some(&log.path);
809+
entry.current = Some(&log);
814810
}
815811
ExtraLogKind::Rotated { name, log } => {
816812
let entry = res.entry(name).or_default();
817-
entry.rotated.push(&log.path);
813+
entry.rotated.push(&log);
818814
}
819815
}
820816
}
@@ -864,9 +860,9 @@ fn sort_cockroach_extra_logs(logs: &[LogFile]) -> HashMap<&str, ExtraLogs<'_>> {
864860
let entry = interested.entry(prefix).or_default();
865861

866862
if file_name == format!("{prefix}.log") {
867-
entry.current = Some(log.path.as_path());
863+
entry.current = Some(log);
868864
} else {
869-
entry.rotated.push(log.path.as_path());
865+
entry.rotated.push(log);
870866
}
871867
}
872868
}
@@ -943,52 +939,30 @@ mod test {
943939
"bogus.log",
944940
"some/dir"
945941
].into_iter().map(|l| {
946-
oxlog::LogFile { path: Utf8PathBuf::from(l), size: None, modified: None }
947-
}).collect();
942+
oxlog::LogFile { path: Utf8PathBuf::from(l), size: None, modified: None }
943+
}).collect();
944+
let logs_map: HashMap<_, _> =
945+
logs.iter().map(|l| (l.path.as_str(), l)).collect();
948946

949947
let mut expected: HashMap<&str, ExtraLogs<'_>> = HashMap::new();
950948

951949
// cockroach
952950
expected.entry("cockroach").or_default().current =
953-
Some(Utf8Path::new("cockroach.log"));
954-
expected
955-
.entry("cockroach")
956-
.or_default()
957-
.rotated
958-
.push(Utf8Path::new("cockroach.oxzcockroachdba3628a56-6f85-43b5-be50-71d8f0e04877.root.2025-01-31T17_11_45Z.011435.log"));
959-
expected
960-
.entry("cockroach")
961-
.or_default()
962-
.rotated
963-
.push(Utf8Path::new("cockroach.oxzcockroachdba3628a56-6f85-43b5-be50-71d8f0e04877.root.2025-02-01T01_51_51Z.011486.log"));
951+
Some(&logs_map["cockroach.log"]);
952+
expected.entry("cockroach").or_default().rotated.push(&logs_map["cockroach.oxzcockroachdba3628a56-6f85-43b5-be50-71d8f0e04877.root.2025-01-31T17_11_45Z.011435.log"]);
953+
expected.entry("cockroach").or_default().rotated.push(&logs_map["cockroach.oxzcockroachdba3628a56-6f85-43b5-be50-71d8f0e04877.root.2025-02-01T01_51_51Z.011486.log"]);
964954

965955
// cockroach-health
966956
expected.entry("cockroach-health").or_default().current =
967-
Some(Utf8Path::new("cockroach-health.log"));
968-
expected
969-
.entry("cockroach-health")
970-
.or_default()
971-
.rotated
972-
.push(Utf8Path::new("cockroach-health.oxzcockroachdba3628a56-6f85-43b5-be50-71d8f0e04877.root.2025-01-31T21_43_26Z.011435.log"));
973-
expected
974-
.entry("cockroach-health")
975-
.or_default()
976-
.rotated
977-
.push(Utf8Path::new("cockroach-health.oxzcockroachdba3628a56-6f85-43b5-be50-71d8f0e04877.root.2025-02-01T01_51_53Z.011486.log"));
957+
Some(&logs_map["cockroach-health.log"]);
958+
expected.entry("cockroach-health").or_default().rotated.push(&logs_map["cockroach-health.oxzcockroachdba3628a56-6f85-43b5-be50-71d8f0e04877.root.2025-01-31T21_43_26Z.011435.log"]);
959+
expected.entry("cockroach-health").or_default().rotated.push(&logs_map["cockroach-health.oxzcockroachdba3628a56-6f85-43b5-be50-71d8f0e04877.root.2025-02-01T01_51_53Z.011486.log"]);
978960

979961
// cockroach-stderr
980962
expected.entry("cockroach-stderr").or_default().current =
981-
Some(Utf8Path::new("cockroach-stderr.log"));
982-
expected
983-
.entry("cockroach-stderr")
984-
.or_default()
985-
.rotated
986-
.push(Utf8Path::new("cockroach-stderr.oxzcockroachdba3628a56-6f85-43b5-be50-71d8f0e04877.root.2023-08-30T18_56_19Z.011950.log"));
987-
expected
988-
.entry("cockroach-stderr")
989-
.or_default()
990-
.rotated
991-
.push(Utf8Path::new("cockroach-stderr.oxzcockroachdba3628a56-6f85-43b5-be50-71d8f0e04877.root.2023-08-31T02_59_24Z.010479.log"));
963+
Some(&logs_map["cockroach-stderr.log"]);
964+
expected.entry("cockroach-stderr").or_default().rotated.push(&logs_map["cockroach-stderr.oxzcockroachdba3628a56-6f85-43b5-be50-71d8f0e04877.root.2023-08-30T18_56_19Z.011950.log"]);
965+
expected.entry("cockroach-stderr").or_default().rotated.push(&logs_map["cockroach-stderr.oxzcockroachdba3628a56-6f85-43b5-be50-71d8f0e04877.root.2023-08-31T02_59_24Z.010479.log"]);
992966

993967
let extra = sort_cockroach_extra_logs(logs.as_slice());
994968
assert_eq!(

‎workspace-hack/Cargo.toml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ x509-cert = { version = "0.2.5" }
150150
zerocopy-c38e5c1d305a1b54 = { package = "zerocopy", version = "0.8.27", default-features = false, features = ["derive", "simd"] }
151151
zerocopy-ca01ad9e24f5d932 = { package = "zerocopy", version = "0.7.35", features = ["derive", "simd"] }
152152
zeroize = { version = "1.8.1", features = ["std", "zeroize_derive"] }
153-
zip-164d15cefe24d7eb = { package = "zip", version = "4.2.0", default-features = false, features = ["bzip2", "chrono", "deflate", "zstd"] }
153+
zip-164d15cefe24d7eb = { package = "zip", version = "4.2.0", default-features = false, features = ["bzip2", "deflate", "jiff-02", "zstd"] }
154154
zip-3b31131e45eafb45 = { package = "zip", version = "0.6.6", default-features = false, features = ["bzip2", "deflate"] }
155155

156156
[build-dependencies]
@@ -291,7 +291,7 @@ x509-cert = { version = "0.2.5" }
291291
zerocopy-c38e5c1d305a1b54 = { package = "zerocopy", version = "0.8.27", default-features = false, features = ["derive", "simd"] }
292292
zerocopy-ca01ad9e24f5d932 = { package = "zerocopy", version = "0.7.35", features = ["derive", "simd"] }
293293
zeroize = { version = "1.8.1", features = ["std", "zeroize_derive"] }
294-
zip-164d15cefe24d7eb = { package = "zip", version = "4.2.0", default-features = false, features = ["bzip2", "chrono", "deflate", "zstd"] }
294+
zip-164d15cefe24d7eb = { package = "zip", version = "4.2.0", default-features = false, features = ["bzip2", "deflate", "jiff-02", "zstd"] }
295295
zip-3b31131e45eafb45 = { package = "zip", version = "0.6.6", default-features = false, features = ["bzip2", "deflate"] }
296296

297297
[target.x86_64-unknown-linux-gnu.dependencies]

0 commit comments

Comments
 (0)