From 66bdbd5839faa3cc0073fc6f02cb579095aa0aed Mon Sep 17 00:00:00 2001 From: Paco Huelsz Prince Date: Sat, 14 Mar 2026 14:19:32 -0700 Subject: [PATCH 01/12] Multiple http streaming improvements, bug fixes, and download speed monitoring. --- .../src/io_utils/http/download_monitor.rs | 190 ++++++++++++++++++ crates/trident/src/io_utils/http/file.rs | 101 +++++++++- crates/trident/src/io_utils/http/mod.rs | 2 + crates/trident/src/io_utils/http/subfile.rs | 6 +- crates/trident/src/io_utils/image_streamer.rs | 5 +- 5 files changed, 289 insertions(+), 15 deletions(-) create mode 100644 crates/trident/src/io_utils/http/download_monitor.rs diff --git a/crates/trident/src/io_utils/http/download_monitor.rs b/crates/trident/src/io_utils/http/download_monitor.rs new file mode 100644 index 000000000..ea5aa35d3 --- /dev/null +++ b/crates/trident/src/io_utils/http/download_monitor.rs @@ -0,0 +1,190 @@ +use std::{ + io::{Read, Result as IoResult}, + time::{Duration, Instant}, +}; + +use log::debug; +use trident_api::primitives::bytes::ByteCount; + +const RING_BUFFER_SIZE: usize = 10; + +/// A `Read` wrapper that monitors download speed using a moving average over +/// the last [`RING_BUFFER_SIZE`] reads. When the speed falls below a +/// configurable threshold, it emits debug-level log messages at a configurable +/// minimum cadence. +pub struct HttpDownloadMonitor { + inner: R, + /// Expected size of the complete file being read (for log context). + size: u64, + /// Ring buffer of (bytes_read, elapsed) samples. + samples: [(u64, Duration); RING_BUFFER_SIZE], + /// Next write position in the ring buffer. + sample_idx: usize, + /// Number of samples recorded so far (capped at RING_BUFFER_SIZE). + sample_count: usize, + /// Speed threshold in megabits per second below which to start reporting. + threshold_mbps: f64, + /// Minimum time between consecutive log messages. + report_cadence: Duration, + /// When the last slow-speed message was emitted. + last_report: Instant, + /// Total bytes read through the monitor (for log context). + total_bytes: u64, +} + +impl HttpDownloadMonitor { + /// Creates a new download monitor wrapping `inner`. + /// + /// * `threshold_mbps` — speed in Mbps below which debug messages are + /// emitted. + /// * `report_cadence` — minimum interval between consecutive log messages. + pub fn new(inner: R, size: u64, threshold_mbps: f64, report_cadence: Duration) -> Self { + Self { + inner, + size, + samples: [(0, Duration::ZERO); RING_BUFFER_SIZE], + sample_idx: 0, + sample_count: 0, + threshold_mbps, + report_cadence, + last_report: Instant::now(), + total_bytes: 0, + } + } + + /// Computes the moving-average speed in Mbps from the ring buffer. + fn moving_average_mbps(&self) -> Option { + self.moving_average_bytes_per_sec() + .map(|bps| bps * 8.0 / 1_000_000.0) + } + + /// Computes the moving-average speed in bytes per second. + fn moving_average_bytes_per_sec(&self) -> Option { + if self.sample_count == 0 { + return None; + } + + let (total_bytes, total_dur) = self.samples[..self.sample_count] + .iter() + .fold((0u64, Duration::ZERO), |(b, d), (sb, sd)| (b + sb, d + *sd)); + + let secs = total_dur.as_secs_f64(); + if secs <= 0.0 { + return None; + } + + Some(total_bytes as f64 / secs) + } + + fn record_sample(&mut self, bytes: u64, elapsed: Duration) { + self.samples[self.sample_idx] = (bytes, elapsed); + self.sample_idx = (self.sample_idx + 1) % RING_BUFFER_SIZE; + if self.sample_count < RING_BUFFER_SIZE { + self.sample_count += 1; + } + } +} + +impl Read for HttpDownloadMonitor { + fn read(&mut self, buf: &mut [u8]) -> IoResult { + let start = Instant::now(); + let n = self.inner.read(buf)?; + let elapsed = start.elapsed(); + + if n > 0 { + self.total_bytes += n as u64; + self.record_sample(n as u64, elapsed); + + if let Some(mbps) = self.moving_average_mbps() { + if mbps <= self.threshold_mbps && self.last_report.elapsed() >= self.report_cadence + { + let pct = if self.size > 0 { + self.total_bytes as f64 / self.size as f64 * 100.0 + } else { + 0.0 + }; + + let eta = if self.size > self.total_bytes { + self.moving_average_bytes_per_sec() + .filter(|&bps| bps > 0.0) + .map(|bps| { + let remaining = (self.size - self.total_bytes) as f64; + format_duration(Duration::from_secs_f64(remaining / bps)) + }) + .unwrap_or_else(|| "unknown".to_string()) + } else { + "done".to_string() + }; + + debug!( + "Slow download: {:.2} Mbps, {:.1}% complete ({}/{}), ETA: {}", + mbps, + pct, + ByteCount::from(self.total_bytes).to_human_readable_approx(), + ByteCount::from(self.size).to_human_readable_approx(), + eta, + ); + self.last_report = Instant::now(); + } + } + } + + Ok(n) + } +} + +/// Formats a duration as a human-readable string (e.g., "2h 15m", "3m 42s", "17s"). +fn format_duration(d: Duration) -> String { + let total_secs = d.as_secs(); + let hours = total_secs / 3600; + let mins = (total_secs % 3600) / 60; + let secs = total_secs % 60; + + if hours > 0 { + format!("{hours}h {mins:02}m") + } else if mins > 0 { + format!("{mins}m {secs:02}s") + } else { + format!("{secs}s") + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::io::Cursor; + + #[test] + fn test_monitor_passes_through_data() { + let data = b"hello world"; + let len = data.len() as u64; + let mut monitor = HttpDownloadMonitor::new( + Cursor::new(data.as_slice()), + len, + 10.0, + Duration::from_secs(1), + ); + + let mut buf = vec![0u8; 32]; + let n = monitor.read(&mut buf).unwrap(); + assert_eq!(n, data.len()); + assert_eq!(&buf[..n], data); + } + + #[test] + fn test_ring_buffer_wraps() { + let data = vec![0u8; 1024]; + let len = data.len() as u64; + let mut monitor = + HttpDownloadMonitor::new(Cursor::new(data), len, 10.0, Duration::from_secs(1)); + + let mut buf = vec![0u8; 64]; + // Read more times than the ring buffer size. + for _ in 0..RING_BUFFER_SIZE + 5 { + let _ = monitor.read(&mut buf); + } + + assert_eq!(monitor.sample_count, RING_BUFFER_SIZE); + assert_eq!(monitor.sample_idx, 5); // wrapped around + } +} diff --git a/crates/trident/src/io_utils/http/file.rs b/crates/trident/src/io_utils/http/file.rs index f8a71ccc7..b15ba687e 100644 --- a/crates/trident/src/io_utils/http/file.rs +++ b/crates/trident/src/io_utils/http/file.rs @@ -10,7 +10,7 @@ use anyhow::{ensure, Context, Error}; use log::{debug, trace, warn}; use oci_client::{secrets::RegistryAuth, Client as OciClient, Reference, RegistryOperation}; use reqwest::{ - blocking::Client, + blocking::{Client, ClientBuilder}, header::{ACCEPT_RANGES, AUTHORIZATION}, }; use tokio::runtime::Runtime; @@ -19,7 +19,22 @@ use url::Url; #[cfg(feature = "dangerous-options")] use docker_credential::{self, DockerCredential}; -use crate::io_utils::http::subfile::HttpSubFile; +use super::{subfile::HttpSubFile, HttpDownloadMonitor}; + +/// The maximum timeout for a single HTTP request to establish a connection. +/// This is not a timeout for the entire file read operation, but rather a +/// timeout for each individual HTTP request. The `HttpFile` implementation will +/// retry requests that fail due to transient errors, up to the overall timeout +/// specified when creating the `HttpFile`. +const MAX_PER_REQUEST_TIMEOUT_SECONDS: u64 = 10; + +/// Threshold speed in Mbps below which to emit debug log messages about slow +/// HTTP download speed. This is used by the `HttpDownloadMonitor` wrapper +/// returned by `HttpFile`. +const SLOW_DOWNLOAD_THRESHOLD_MBPS: f64 = 10.0; + +/// Minimum interval between consecutive slow download log messages. +const SLOW_DOWNLOAD_REPORT_CADENCE: Duration = Duration::from_secs(5); #[cfg(feature = "dangerous-options")] const DOCKER_CONFIG_FILE_PATH: &str = ".docker/config.json"; @@ -36,7 +51,8 @@ const DOCKER_CONFIG_FILE_PATH: &str = ".docker/config.json"; pub struct HttpFile { url: Url, position: u64, - pub(super) size: u64, + /// The total size of the file in bytes. + pub size: u64, client: Client, timeout: Duration, token: Option, @@ -81,8 +97,21 @@ impl HttpFile { ) -> IoResult { debug!("Opening HTTP file '{}'", url); - // Create a new client for this file. - let client = Client::new(); + // Create a new client for this file with a + // `MAX_PER_REQUEST_TIMEOUT_SECONDS` second connect timeout. We + // intentionally do not set a total request timeout here because body + // reads for large range requests can take much longer than the + // connection timeout, and reqwest's `.timeout()` applies to the entire + // transfer including body streaming. + // + // The `MAX_PER_REQUEST_TIMEOUT_SECONDS` connect timeout is per request, + // We always do requests in a retry loop that respects the overall timeout + // given to us. + let client = ClientBuilder::new() + .connect_timeout(Duration::from_secs(MAX_PER_REQUEST_TIMEOUT_SECONDS)) + .build() + .map_err(|e| IoError::other(format!("Failed to create HTTP client: {e}")))?; + let request_sender = || { let mut request = client.head(url.as_str()); if let Some(token) = &token { @@ -241,8 +270,7 @@ impl HttpFile { }) } - /// Returns an HTTPSubFile object covering a specific section of the file. - pub(crate) fn section_reader(&self, section_offset: u64, size: u64) -> HttpSubFile { + fn section_reader_inner(&self, section_offset: u64, size: u64) -> HttpSubFile { let end = section_offset + size - 1; trace!( "Reading HTTP file '{}' from {} to {} (inclusive) [{} bytes]", @@ -267,11 +295,31 @@ impl HttpFile { subfile } + /// Returns an HTTPSubFile object covering a specific section of the file. + pub(crate) fn section_reader( + &self, + section_offset: u64, + size: u64, + ) -> HttpDownloadMonitor { + HttpDownloadMonitor::new( + self.section_reader_inner(section_offset, size), + size, + SLOW_DOWNLOAD_THRESHOLD_MBPS, + SLOW_DOWNLOAD_REPORT_CADENCE, + ) + } + /// Returns an HTTPSubFile object covering the complete file. - pub(crate) fn complete_reader(&self) -> HttpSubFile { + pub fn complete_reader(&self) -> HttpDownloadMonitor { trace!("Reading complete HTTP file '{}'", self.url); // Create a section reader optimized to read the complete file. - self.section_reader(0, self.size).with_end_is_parent_eof() + HttpDownloadMonitor::new( + self.section_reader_inner(0, self.size) + .with_end_is_parent_eof(), + self.size, + SLOW_DOWNLOAD_THRESHOLD_MBPS, + SLOW_DOWNLOAD_REPORT_CADENCE, + ) } } @@ -327,21 +375,54 @@ impl Seek for HttpFile { } impl Read for HttpFile { + /// Implementation of `read()` from the `Read` trait for the HTTP file + /// reader provided for broad compatibility. Where possible, it is + /// recommended to use specialized methods, such as `section_reader()` or + /// `complete_reader()`, as they will make more efficient use of HTTP range + /// requests and avoid unnecessary requests. Each call to `read` will result + /// in a new HTTP request for the requested range of bytes, so using this + /// method for large reads may be inefficient. fn read(&mut self, buf: &mut [u8]) -> IoResult { - let mut subfile = self.section_reader(self.position, buf.len() as u64); + if self.position >= self.size { + return Ok(0); + } + + let size_to_read = std::cmp::min(buf.len() as u64, self.size - self.position) as usize; + let mut subfile = self.section_reader(self.position, size_to_read as u64); let res = subfile.read(buf)?; self.position += res as u64; Ok(res) } + /// Implementation of `read_exact()` from the `Read` trait for the HTTP file + /// reader. Each call to `read_exact` will result in a new HTTP request for + /// the requested range of bytes, so using this method for large reads may + /// be inefficient. This method will return an error if there are not enough + /// bytes remaining in the file to fill the buffer, even if the end of the + /// file has not been reached yet. fn read_exact(&mut self, buf: &mut [u8]) -> IoResult<()> { + if buf.len() as u64 > self.size - self.position { + return Err(IoError::new( + IoErrorKind::UnexpectedEof, + "Not enough bytes remaining in the file to fill the buffer", + )); + } + let mut subfile = self.section_reader(self.position, buf.len() as u64); subfile.read_exact(buf)?; self.position += buf.len() as u64; Ok(()) } + /// Implementation of `read_to_end()` from the `Read` trait for the HTTP + /// file reader. This method will read until the end of the file is reached. + /// In best case scenarios, only one HTTP request will be made. Internal + /// retries may result in additional requests. fn read_to_end(&mut self, buf: &mut Vec) -> IoResult { + if self.position >= self.size { + return Ok(0); + } + let mut subfile = self.section_reader(self.position, self.size - self.position); let res = subfile.read_to_end(buf)?; self.position += res as u64; diff --git a/crates/trident/src/io_utils/http/mod.rs b/crates/trident/src/io_utils/http/mod.rs index e861bbcfa..a14c8a5f8 100644 --- a/crates/trident/src/io_utils/http/mod.rs +++ b/crates/trident/src/io_utils/http/mod.rs @@ -10,10 +10,12 @@ use reqwest::{ StatusCode, }; +mod download_monitor; pub mod file; mod range; mod subfile; +pub use download_monitor::HttpDownloadMonitor; pub use file::HttpFile; use range::HttpRangeRequest; diff --git a/crates/trident/src/io_utils/http/subfile.rs b/crates/trident/src/io_utils/http/subfile.rs index 04b408292..f089a175d 100644 --- a/crates/trident/src/io_utils/http/subfile.rs +++ b/crates/trident/src/io_utils/http/subfile.rs @@ -154,7 +154,7 @@ impl HttpSubFile { /// Returns the length of the subfile in bytes. pub fn size(&self) -> u64 { // Add 1 because the range is inclusive. - self.end - self.start + 1 + self.end.saturating_sub(self.start) + 1 } /// Returns whether we have reached the end of the subfile. @@ -236,7 +236,7 @@ impl HttpSubFile { let response = super::retriable_request_sender( || { - let mut req = self.client.get(self.url.clone()).timeout(self.timeout); + let mut req = self.client.get(self.url.clone()); if let Some(range) = range.to_header_value_option() { req = req.header(RANGE, range); @@ -287,7 +287,7 @@ impl Read for HttpSubFile { Ok(n) => n, Err(e) => { warn!( - "Error reading from HTTP subfile at position {}: {e}", + "Error reading from HTTP subfile at position {}: {e:?}", self.position, ); diff --git a/crates/trident/src/io_utils/image_streamer.rs b/crates/trident/src/io_utils/image_streamer.rs index e8c3097f2..595fd9763 100644 --- a/crates/trident/src/io_utils/image_streamer.rs +++ b/crates/trident/src/io_utils/image_streamer.rs @@ -97,7 +97,7 @@ where .context("Failed to sync")?; debug!( - "Copied {} [{}] to '{}'{} in {:.2} seconds", + "Copied {} [{}] to '{}'{} in {:.2} seconds: {:.2} Mbps", ByteCount::from(bytes_copied).to_human_readable_approx(), bytes_copied, destination_path.display(), @@ -107,7 +107,8 @@ where format!(" ('{}')", real_path.display()), _ => "".into(), }, - t.elapsed().as_secs_f32() + t.elapsed().as_secs_f32(), + (bytes_copied as f64 * 8.0) / (t.elapsed().as_secs_f64() * 1_000_000.0) ); Ok(()) From 7aba152fe8d8763830470234be18088d828bb644 Mon Sep 17 00:00:00 2001 From: Paco Huelsz Prince Date: Sat, 14 Mar 2026 14:40:00 -0700 Subject: [PATCH 02/12] revert unneeded changes --- crates/trident/src/io_utils/http/file.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/crates/trident/src/io_utils/http/file.rs b/crates/trident/src/io_utils/http/file.rs index b15ba687e..101878244 100644 --- a/crates/trident/src/io_utils/http/file.rs +++ b/crates/trident/src/io_utils/http/file.rs @@ -51,8 +51,7 @@ const DOCKER_CONFIG_FILE_PATH: &str = ".docker/config.json"; pub struct HttpFile { url: Url, position: u64, - /// The total size of the file in bytes. - pub size: u64, + pub(super) size: u64, client: Client, timeout: Duration, token: Option, @@ -310,7 +309,7 @@ impl HttpFile { } /// Returns an HTTPSubFile object covering the complete file. - pub fn complete_reader(&self) -> HttpDownloadMonitor { + pub(crate) fn complete_reader(&self) -> HttpDownloadMonitor { trace!("Reading complete HTTP file '{}'", self.url); // Create a section reader optimized to read the complete file. HttpDownloadMonitor::new( From 5a58e163cf66870298982abb5e201cc7e53577c9 Mon Sep 17 00:00:00 2001 From: Paco Huelsz Prince Date: Mon, 16 Mar 2026 16:32:41 -0700 Subject: [PATCH 03/12] MBps in print --- crates/trident/src/io_utils/image_streamer.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/trident/src/io_utils/image_streamer.rs b/crates/trident/src/io_utils/image_streamer.rs index 595fd9763..a6d0d1a91 100644 --- a/crates/trident/src/io_utils/image_streamer.rs +++ b/crates/trident/src/io_utils/image_streamer.rs @@ -97,7 +97,7 @@ where .context("Failed to sync")?; debug!( - "Copied {} [{}] to '{}'{} in {:.2} seconds: {:.2} Mbps", + "Copied {} [{}] to '{}'{} in {:.2} seconds: {:.2} MB/s", ByteCount::from(bytes_copied).to_human_readable_approx(), bytes_copied, destination_path.display(), @@ -108,7 +108,7 @@ where _ => "".into(), }, t.elapsed().as_secs_f32(), - (bytes_copied as f64 * 8.0) / (t.elapsed().as_secs_f64() * 1_000_000.0) + (bytes_copied as f64) / (t.elapsed().as_secs_f64() * 1_000_000.0) ); Ok(()) From e258ccdbfec57f540a72fb0bd22d3967cf0b8312 Mon Sep 17 00:00:00 2001 From: Paco Huelsz Prince Date: Mon, 16 Mar 2026 16:39:16 -0700 Subject: [PATCH 04/12] Copilot comments --- crates/trident/src/io_utils/http/file.rs | 12 +++++++++++- crates/trident/src/io_utils/http/subfile.rs | 9 +++++++-- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/crates/trident/src/io_utils/http/file.rs b/crates/trident/src/io_utils/http/file.rs index 101878244..1915f3f0f 100644 --- a/crates/trident/src/io_utils/http/file.rs +++ b/crates/trident/src/io_utils/http/file.rs @@ -270,6 +270,12 @@ impl HttpFile { } fn section_reader_inner(&self, section_offset: u64, size: u64) -> HttpSubFile { + if size == 0 { + // Create an empty subfile if the requested size is 0, to avoid + // making unnecessary HTTP requests + return HttpSubFile::new_with_client(self.url.clone(), 0, 0, self.client.clone()); + } + let end = section_offset + size - 1; trace!( "Reading HTTP file '{}' from {} to {} (inclusive) [{} bytes]", @@ -382,7 +388,7 @@ impl Read for HttpFile { /// in a new HTTP request for the requested range of bytes, so using this /// method for large reads may be inefficient. fn read(&mut self, buf: &mut [u8]) -> IoResult { - if self.position >= self.size { + if self.position >= self.size || buf.is_empty() { return Ok(0); } @@ -400,6 +406,10 @@ impl Read for HttpFile { /// bytes remaining in the file to fill the buffer, even if the end of the /// file has not been reached yet. fn read_exact(&mut self, buf: &mut [u8]) -> IoResult<()> { + if buf.is_empty() { + return Ok(()); + } + if buf.len() as u64 > self.size - self.position { return Err(IoError::new( IoErrorKind::UnexpectedEof, diff --git a/crates/trident/src/io_utils/http/subfile.rs b/crates/trident/src/io_utils/http/subfile.rs index f089a175d..d885bfbca 100644 --- a/crates/trident/src/io_utils/http/subfile.rs +++ b/crates/trident/src/io_utils/http/subfile.rs @@ -153,8 +153,13 @@ impl HttpSubFile { /// Returns the length of the subfile in bytes. pub fn size(&self) -> u64 { - // Add 1 because the range is inclusive. - self.end.saturating_sub(self.start) + 1 + if self.start > self.end { + // Invalid range, treat as empty subfile. + 0 + } else { + // Add 1 because the range is inclusive. + self.end - self.start + 1 + } } /// Returns whether we have reached the end of the subfile. From 32d687a93f3e7caee95f03a1d3748bc27c9169e0 Mon Sep 17 00:00:00 2001 From: Paco Huelsz Prince Date: Mon, 23 Mar 2026 13:42:50 -0700 Subject: [PATCH 05/12] Avoid Inf result from float zero division. --- crates/trident/src/io_utils/image_streamer.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/crates/trident/src/io_utils/image_streamer.rs b/crates/trident/src/io_utils/image_streamer.rs index a6d0d1a91..022e71861 100644 --- a/crates/trident/src/io_utils/image_streamer.rs +++ b/crates/trident/src/io_utils/image_streamer.rs @@ -96,6 +96,15 @@ where .sync_all() .context("Failed to sync")?; + let elapsed_safe = { + let elapsed = t.elapsed().as_secs_f64(); + if elapsed == 0.0 { + 0.001 // Avoid division by zero in case of very fast writes + } else { + elapsed + } + }; + debug!( "Copied {} [{}] to '{}'{} in {:.2} seconds: {:.2} MB/s", ByteCount::from(bytes_copied).to_human_readable_approx(), @@ -108,7 +117,7 @@ where _ => "".into(), }, t.elapsed().as_secs_f32(), - (bytes_copied as f64) / (t.elapsed().as_secs_f64() * 1_000_000.0) + (bytes_copied as f64) / (elapsed_safe * 1_000_000.0) ); Ok(()) From cb51daab36250dee6c4a71e3c4378810539b6462 Mon Sep 17 00:00:00 2001 From: Paco Huelsz Prince Date: Mon, 23 Mar 2026 14:54:52 -0700 Subject: [PATCH 06/12] Swap layers to expose monitor. --- crates/trident/src/engine/storage/image.rs | 19 +++- crates/trident/src/io_utils/http/file.rs | 34 ++------ crates/trident/src/io_utils/http/mod.rs | 2 - crates/trident/src/io_utils/mod.rs | 1 + .../download_monitor.rs => read_monitor.rs} | 86 ++++++++++--------- crates/trident/src/osimage/cosi/mod.rs | 9 +- 6 files changed, 71 insertions(+), 80 deletions(-) rename crates/trident/src/io_utils/{http/download_monitor.rs => read_monitor.rs} (69%) diff --git a/crates/trident/src/engine/storage/image.rs b/crates/trident/src/engine/storage/image.rs index f9c74e2d8..a2c89cffd 100644 --- a/crates/trident/src/engine/storage/image.rs +++ b/crates/trident/src/engine/storage/image.rs @@ -3,6 +3,7 @@ use std::{ io::Read, ops::ControlFlow, path::{Path, PathBuf}, + time::Duration, }; use anyhow::{bail, ensure, Context, Error}; @@ -21,7 +22,9 @@ use trident_api::{ use crate::{ engine::{context::filesystem::FileSystemDataImage, EngineContext}, - io_utils::{hashing_reader::HashingReader384, image_streamer}, + io_utils::{ + hashing_reader::HashingReader384, image_streamer, read_monitor::ReadMonitor, + }, osimage::{OsImageFile, OsImagePartition}, }; @@ -163,7 +166,15 @@ pub(super) fn deploy_images(ctx: &EngineContext) -> Result<(), TridentError> { "Initializing '{id}': writing image for filesystem from '{}'", os_img.source() ); - if let Err(e) = deploy_os_image_file(ctx, &id, image_file, resize, reader) { + + let monitored_reader = ReadMonitor::new( + reader, + image_file.compressed_size, + 100.0, + Duration::from_secs(5), + ); + + if let Err(e) = deploy_os_image_file(ctx, &id, image_file, resize, monitored_reader) { return ControlFlow::Break(Err(e).structured(ServicingError::DeployImages)); } @@ -290,12 +301,12 @@ enum FileSystemResize { } /// Deploys an individual OS image file from an OS image. -fn deploy_os_image_file( +fn deploy_os_image_file( ctx: &EngineContext, id: &BlockDeviceId, image_file: &OsImageFile, fs_resize: FileSystemResize, - reader: Box, + reader: R, ) -> Result<(), Error> { let block_device_path = ctx .get_block_device_path(id) diff --git a/crates/trident/src/io_utils/http/file.rs b/crates/trident/src/io_utils/http/file.rs index 1915f3f0f..170283276 100644 --- a/crates/trident/src/io_utils/http/file.rs +++ b/crates/trident/src/io_utils/http/file.rs @@ -19,7 +19,7 @@ use url::Url; #[cfg(feature = "dangerous-options")] use docker_credential::{self, DockerCredential}; -use super::{subfile::HttpSubFile, HttpDownloadMonitor}; +use super::subfile::HttpSubFile; /// The maximum timeout for a single HTTP request to establish a connection. /// This is not a timeout for the entire file read operation, but rather a @@ -28,14 +28,6 @@ use super::{subfile::HttpSubFile, HttpDownloadMonitor}; /// specified when creating the `HttpFile`. const MAX_PER_REQUEST_TIMEOUT_SECONDS: u64 = 10; -/// Threshold speed in Mbps below which to emit debug log messages about slow -/// HTTP download speed. This is used by the `HttpDownloadMonitor` wrapper -/// returned by `HttpFile`. -const SLOW_DOWNLOAD_THRESHOLD_MBPS: f64 = 10.0; - -/// Minimum interval between consecutive slow download log messages. -const SLOW_DOWNLOAD_REPORT_CADENCE: Duration = Duration::from_secs(5); - #[cfg(feature = "dangerous-options")] const DOCKER_CONFIG_FILE_PATH: &str = ".docker/config.json"; @@ -301,30 +293,16 @@ impl HttpFile { } /// Returns an HTTPSubFile object covering a specific section of the file. - pub(crate) fn section_reader( - &self, - section_offset: u64, - size: u64, - ) -> HttpDownloadMonitor { - HttpDownloadMonitor::new( - self.section_reader_inner(section_offset, size), - size, - SLOW_DOWNLOAD_THRESHOLD_MBPS, - SLOW_DOWNLOAD_REPORT_CADENCE, - ) + pub(crate) fn section_reader(&self, section_offset: u64, size: u64) -> HttpSubFile { + self.section_reader_inner(section_offset, size) } /// Returns an HTTPSubFile object covering the complete file. - pub(crate) fn complete_reader(&self) -> HttpDownloadMonitor { + pub(crate) fn complete_reader(&self) -> HttpSubFile { trace!("Reading complete HTTP file '{}'", self.url); // Create a section reader optimized to read the complete file. - HttpDownloadMonitor::new( - self.section_reader_inner(0, self.size) - .with_end_is_parent_eof(), - self.size, - SLOW_DOWNLOAD_THRESHOLD_MBPS, - SLOW_DOWNLOAD_REPORT_CADENCE, - ) + self.section_reader_inner(0, self.size) + .with_end_is_parent_eof() } } diff --git a/crates/trident/src/io_utils/http/mod.rs b/crates/trident/src/io_utils/http/mod.rs index a14c8a5f8..e861bbcfa 100644 --- a/crates/trident/src/io_utils/http/mod.rs +++ b/crates/trident/src/io_utils/http/mod.rs @@ -10,12 +10,10 @@ use reqwest::{ StatusCode, }; -mod download_monitor; pub mod file; mod range; mod subfile; -pub use download_monitor::HttpDownloadMonitor; pub use file::HttpFile; use range::HttpRangeRequest; diff --git a/crates/trident/src/io_utils/mod.rs b/crates/trident/src/io_utils/mod.rs index 73a1d08d6..6b5135186 100644 --- a/crates/trident/src/io_utils/mod.rs +++ b/crates/trident/src/io_utils/mod.rs @@ -2,3 +2,4 @@ pub mod file_reader; pub mod hashing_reader; mod http; pub mod image_streamer; +pub mod read_monitor; diff --git a/crates/trident/src/io_utils/http/download_monitor.rs b/crates/trident/src/io_utils/read_monitor.rs similarity index 69% rename from crates/trident/src/io_utils/http/download_monitor.rs rename to crates/trident/src/io_utils/read_monitor.rs index ea5aa35d3..7ac8d6793 100644 --- a/crates/trident/src/io_utils/http/download_monitor.rs +++ b/crates/trident/src/io_utils/read_monitor.rs @@ -12,7 +12,7 @@ const RING_BUFFER_SIZE: usize = 10; /// the last [`RING_BUFFER_SIZE`] reads. When the speed falls below a /// configurable threshold, it emits debug-level log messages at a configurable /// minimum cadence. -pub struct HttpDownloadMonitor { +pub struct ReadMonitor { inner: R, /// Expected size of the complete file being read (for log context). size: u64, @@ -32,7 +32,7 @@ pub struct HttpDownloadMonitor { total_bytes: u64, } -impl HttpDownloadMonitor { +impl ReadMonitor { /// Creates a new download monitor wrapping `inner`. /// /// * `threshold_mbps` — speed in Mbps below which debug messages are @@ -85,47 +85,52 @@ impl HttpDownloadMonitor { } } -impl Read for HttpDownloadMonitor { +impl Read for ReadMonitor { fn read(&mut self, buf: &mut [u8]) -> IoResult { let start = Instant::now(); let n = self.inner.read(buf)?; let elapsed = start.elapsed(); - if n > 0 { - self.total_bytes += n as u64; - self.record_sample(n as u64, elapsed); - - if let Some(mbps) = self.moving_average_mbps() { - if mbps <= self.threshold_mbps && self.last_report.elapsed() >= self.report_cadence - { - let pct = if self.size > 0 { - self.total_bytes as f64 / self.size as f64 * 100.0 - } else { - 0.0 - }; - - let eta = if self.size > self.total_bytes { - self.moving_average_bytes_per_sec() - .filter(|&bps| bps > 0.0) - .map(|bps| { - let remaining = (self.size - self.total_bytes) as f64; - format_duration(Duration::from_secs_f64(remaining / bps)) - }) - .unwrap_or_else(|| "unknown".to_string()) - } else { - "done".to_string() - }; - - debug!( - "Slow download: {:.2} Mbps, {:.1}% complete ({}/{}), ETA: {}", - mbps, - pct, - ByteCount::from(self.total_bytes).to_human_readable_approx(), - ByteCount::from(self.size).to_human_readable_approx(), - eta, - ); - self.last_report = Instant::now(); - } + // Return early if there is no threshold configured, to avoid the + // overhead of recording samples and computing averages. + // + // Also return early if n == 0, which naturally happens at EOF. + if self.threshold_mbps <= 0.0 || n == 0 { + return Ok(n); + } + + self.total_bytes += n as u64; + self.record_sample(n as u64, elapsed); + + if let Some(mbps) = self.moving_average_mbps() { + if mbps <= self.threshold_mbps && self.last_report.elapsed() >= self.report_cadence { + let pct = if self.size > 0 { + self.total_bytes as f64 / self.size as f64 * 100.0 + } else { + 0.0 + }; + + let eta = if self.size > self.total_bytes { + self.moving_average_bytes_per_sec() + .filter(|&bps| bps > 0.0) + .map(|bps| { + let remaining = (self.size - self.total_bytes) as f64; + format_duration(Duration::from_secs_f64(remaining / bps)) + }) + .unwrap_or_else(|| "unknown".to_string()) + } else { + "done".to_string() + }; + + debug!( + "Slow download: {:.2} Mbps, {:.1}% complete ({}/{}), ETA: {}", + mbps, + pct, + ByteCount::from(self.total_bytes).to_human_readable_approx(), + ByteCount::from(self.size).to_human_readable_approx(), + eta, + ); + self.last_report = Instant::now(); } } @@ -158,7 +163,7 @@ mod tests { fn test_monitor_passes_through_data() { let data = b"hello world"; let len = data.len() as u64; - let mut monitor = HttpDownloadMonitor::new( + let mut monitor = ReadMonitor::new( Cursor::new(data.as_slice()), len, 10.0, @@ -175,8 +180,7 @@ mod tests { fn test_ring_buffer_wraps() { let data = vec![0u8; 1024]; let len = data.len() as u64; - let mut monitor = - HttpDownloadMonitor::new(Cursor::new(data), len, 10.0, Duration::from_secs(1)); + let mut monitor = ReadMonitor::new(Cursor::new(data), len, 10.0, Duration::from_secs(1)); let mut buf = vec![0u8; 64]; // Read more times than the ring buffer size. diff --git a/crates/trident/src/osimage/cosi/mod.rs b/crates/trident/src/osimage/cosi/mod.rs index 85bc28c13..e831451d3 100644 --- a/crates/trident/src/osimage/cosi/mod.rs +++ b/crates/trident/src/osimage/cosi/mod.rs @@ -235,11 +235,10 @@ impl Cosi { { let (path, entry) = entry.structured(InternalError::Internal("read COSI archive"))?; - let reader = Box::new( - self.reader - .section_reader(entry.offset, entry.size) - .structured(InternalError::Internal("read COSI archive"))?, - ); + let reader = self + .reader + .section_reader(entry.offset, entry.size) + .structured(InternalError::Internal("read COSI archive"))?; match f(&path, reader) { ControlFlow::Continue(()) => {} ControlFlow::Break(b) => return b, From f7af9904f9879d08f2a95c3e28c338fa4a58d170 Mon Sep 17 00:00:00 2001 From: Paco Huelsz Prince Date: Mon, 23 Mar 2026 16:49:34 -0700 Subject: [PATCH 07/12] Internal params for monitor --- crates/trident/src/engine/context/mod.rs | 34 ++++++++++++++++++++-- crates/trident/src/engine/storage/image.rs | 14 +++++---- crates/trident/src/lib.rs | 9 ++++++ crates/trident/src/subsystems/esp.rs | 15 +++++++++- crates/trident_api/src/constants.rs | 11 +++++++ 5 files changed, 74 insertions(+), 9 deletions(-) diff --git a/crates/trident/src/engine/context/mod.rs b/crates/trident/src/engine/context/mod.rs index 1cda35cda..d934b49fd 100644 --- a/crates/trident/src/engine/context/mod.rs +++ b/crates/trident/src/engine/context/mod.rs @@ -1,6 +1,7 @@ use std::{ collections::{BTreeMap, HashMap}, path::{Path, PathBuf}, + time::Duration, }; use anyhow::{bail, Context, Error}; @@ -9,14 +10,23 @@ use log::{debug, trace}; use trident_api::{ config::{HostConfiguration, Partition, VerityDevice}, - constants::ROOT_MOUNT_POINT_PATH, + constants::{ + internal_params::{ + STREAM_SLOW_SPEED_REPORTING_INTERVAL_SECONDS, + STREAM_SLOW_SPEED_REPORTING_THRESHOLD_MBPS, + }, + ROOT_MOUNT_POINT_PATH, + }, error::{InternalError, ReportError, TridentError}, status::{AbVolumeSelection, ServicingType}, storage_graph::graph::StorageGraph, BlockDeviceId, }; -use crate::osimage::OsImage; +use crate::{ + osimage::OsImage, STREAM_SLOW_SPEED_REPORTING_INTERVAL_SECONDS_DEFAULT, + STREAM_SLOW_SPEED_REPORTING_THRESHOLD_MBPS_DEFAULT, +}; #[allow(dead_code)] pub mod filesystem; @@ -313,6 +323,26 @@ impl EngineContext { .zstd_decompression_parameters() .and_then(|p| p.max_window_log) } + + /// Returns the threshold and interval for reporting slow streaming speed. + pub(crate) fn read_monitor_params(&self) -> Result<(f64, Duration), TridentError> { + Ok(( + self.spec + .internal_params + .get::(STREAM_SLOW_SPEED_REPORTING_THRESHOLD_MBPS) + .transpose() + .map_err(TridentError::new)? + .unwrap_or(STREAM_SLOW_SPEED_REPORTING_THRESHOLD_MBPS_DEFAULT), + Duration::from_secs( + self.spec + .internal_params + .get_u64(STREAM_SLOW_SPEED_REPORTING_INTERVAL_SECONDS) + .transpose() + .map_err(TridentError::new)? + .unwrap_or(STREAM_SLOW_SPEED_REPORTING_INTERVAL_SECONDS_DEFAULT), + ), + )) + } } #[cfg(test)] diff --git a/crates/trident/src/engine/storage/image.rs b/crates/trident/src/engine/storage/image.rs index a2c89cffd..3f84818b6 100644 --- a/crates/trident/src/engine/storage/image.rs +++ b/crates/trident/src/engine/storage/image.rs @@ -3,7 +3,6 @@ use std::{ io::Read, ops::ControlFlow, path::{Path, PathBuf}, - time::Duration, }; use anyhow::{bail, ensure, Context, Error}; @@ -22,9 +21,7 @@ use trident_api::{ use crate::{ engine::{context::filesystem::FileSystemDataImage, EngineContext}, - io_utils::{ - hashing_reader::HashingReader384, image_streamer, read_monitor::ReadMonitor, - }, + io_utils::{hashing_reader::HashingReader384, image_streamer, read_monitor::ReadMonitor}, osimage::{OsImageFile, OsImagePartition}, }; @@ -153,6 +150,11 @@ pub(super) fn deploy_images(ctx: &EngineContext) -> Result<(), TridentError> { ); } + // Get the threshold and interval for reporting slow streaming speed from + // the context, to be used in the ReadMonitor while streaming images to the + // block devices. + let (threshold_reporting, reporting_interval) = ctx.read_monitor_params()?; + os_img.read_images(|path, reader| -> ControlFlow> { let Some((id, image_file, resize)) = combined_images.remove(path) else { debug!( @@ -170,8 +172,8 @@ pub(super) fn deploy_images(ctx: &EngineContext) -> Result<(), TridentError> { let monitored_reader = ReadMonitor::new( reader, image_file.compressed_size, - 100.0, - Duration::from_secs(5), + threshold_reporting, + reporting_interval, ); if let Err(e) = deploy_os_image_file(ctx, &id, image_file, resize, monitored_reader) { diff --git a/crates/trident/src/lib.rs b/crates/trident/src/lib.rs index 96b3b0147..1ac2aef0a 100644 --- a/crates/trident/src/lib.rs +++ b/crates/trident/src/lib.rs @@ -104,6 +104,15 @@ const SAFETY_OVERRIDE_CHECK_PATH: &str = "/override-trident-safety-check"; /// Temporary location of the datastore for multiboot install scenarios. pub const TEMPORARY_DATASTORE_PATH: &str = "/tmp/trident-datastore.sqlite"; +/// Threshold in mega bits per second for reporting slow streaming speeds. If +/// the streaming speed is below this threshold, it will be reported via in the +/// logs. +pub const STREAM_SLOW_SPEED_REPORTING_THRESHOLD_MBPS_DEFAULT: f64 = 15.0; + +/// Interval in seconds for reporting slow streaming speeds. If the streaming +/// speed is below the threshold, it will be reported every this many seconds. +pub const STREAM_SLOW_SPEED_REPORTING_INTERVAL_SECONDS_DEFAULT: u64 = 10; + #[must_use] #[derive(Clone, Copy, PartialEq, Eq, Debug)] pub enum ExitKind { diff --git a/crates/trident/src/subsystems/esp.rs b/crates/trident/src/subsystems/esp.rs index e483b392e..f8bb5e0e5 100644 --- a/crates/trident/src/subsystems/esp.rs +++ b/crates/trident/src/subsystems/esp.rs @@ -35,6 +35,7 @@ use crate::{ io_utils::{ hashing_reader::{HashingReader, HashingReader384}, image_streamer, + read_monitor::ReadMonitor, }, }; @@ -117,18 +118,30 @@ fn deploy_esp(ctx: &EngineContext, mount_point: &Path) -> Result<(), TridentErro // have to store a potentially large ESP image in memory. let esp_extraction_dir = path::join_relative(mount_point, ESP_EXTRACTION_DIRECTORY); + // Get the threshold and interval for reporting slow streaming speed from + // the context, to be used in the ReadMonitor while streaming images to the + // block devices. + let (threshold_reporting, reporting_interval) = ctx.read_monitor_params()?; + let mut found_esp = false; os_image.read_images(|path, stream| { if path != esp_img.image_file.path { return ControlFlow::Continue(()); } + let monitored_reader = ReadMonitor::new( + stream, + esp_img.image_file.compressed_size, + threshold_reporting, + reporting_interval, + ); + found_esp = true; let (temp_file, computed_sha384) = match load_raw_image( ctx, &esp_extraction_dir, os_image.source(), - HashingReader384::new(stream), + HashingReader384::new(monitored_reader), ) { Ok(r) => r, Err(e) => { diff --git a/crates/trident_api/src/constants.rs b/crates/trident_api/src/constants.rs index 21b30e762..5faec16fd 100644 --- a/crates/trident_api/src/constants.rs +++ b/crates/trident_api/src/constants.rs @@ -268,4 +268,15 @@ pub mod internal_params { /// Enable Raw COSI mode where the storage configuration is sourced from the /// COSI metadata. pub const RAW_COSI_STORAGE: &str = "rawCosi"; + + /// Threshold in mega bits per second for reporting slow streaming speeds. If + /// the streaming speed is below this threshold, it will be reported via in the + /// logs. + pub const STREAM_SLOW_SPEED_REPORTING_THRESHOLD_MBPS: &str = + "streamSlowSpeedReportingThresholdMbps"; + + /// Interval in seconds for reporting slow streaming speeds. If the streaming + /// speed is below the threshold, it will be reported every this many seconds. + pub const STREAM_SLOW_SPEED_REPORTING_INTERVAL_SECONDS: &str = + "streamSlowSpeedReportingIntervalSeconds"; } From 0c44618c53a771b247016498d717ca4c38df9305 Mon Sep 17 00:00:00 2001 From: Paco Huelsz Date: Mon, 23 Mar 2026 17:02:11 -0700 Subject: [PATCH 08/12] Update crates/trident/src/io_utils/http/file.rs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- crates/trident/src/io_utils/http/file.rs | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/crates/trident/src/io_utils/http/file.rs b/crates/trident/src/io_utils/http/file.rs index 170283276..f29fe37d7 100644 --- a/crates/trident/src/io_utils/http/file.rs +++ b/crates/trident/src/io_utils/http/file.rs @@ -88,18 +88,19 @@ impl HttpFile { ) -> IoResult { debug!("Opening HTTP file '{}'", url); - // Create a new client for this file with a - // `MAX_PER_REQUEST_TIMEOUT_SECONDS` second connect timeout. We - // intentionally do not set a total request timeout here because body - // reads for large range requests can take much longer than the - // connection timeout, and reqwest's `.timeout()` applies to the entire - // transfer including body streaming. + // Create a new client for this file with a per-request connect timeout + // that is clamped to at most `MAX_PER_REQUEST_TIMEOUT_SECONDS` and the + // overall `timeout` passed in. We intentionally do not set a total + // request timeout here because body reads for large range requests can + // take much longer than the connection timeout, and reqwest's + // `.timeout()` applies to the entire transfer including body streaming. // - // The `MAX_PER_REQUEST_TIMEOUT_SECONDS` connect timeout is per request, - // We always do requests in a retry loop that respects the overall timeout - // given to us. + // The clamped connect timeout is per request. We always do requests in + // a retry loop that respects the overall timeout given to us. + let connect_timeout = + Duration::from_secs(MAX_PER_REQUEST_TIMEOUT_SECONDS).min(timeout); let client = ClientBuilder::new() - .connect_timeout(Duration::from_secs(MAX_PER_REQUEST_TIMEOUT_SECONDS)) + .connect_timeout(connect_timeout) .build() .map_err(|e| IoError::other(format!("Failed to create HTTP client: {e}")))?; From 61702b1ef731861fe07445c5459c41c6d4f25cbf Mon Sep 17 00:00:00 2001 From: Paco Huelsz Date: Mon, 23 Mar 2026 17:04:43 -0700 Subject: [PATCH 09/12] Update crates/trident_api/src/constants.rs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- crates/trident_api/src/constants.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/trident_api/src/constants.rs b/crates/trident_api/src/constants.rs index 5faec16fd..3d6acf098 100644 --- a/crates/trident_api/src/constants.rs +++ b/crates/trident_api/src/constants.rs @@ -269,14 +269,15 @@ pub mod internal_params { /// COSI metadata. pub const RAW_COSI_STORAGE: &str = "rawCosi"; - /// Threshold in mega bits per second for reporting slow streaming speeds. If - /// the streaming speed is below this threshold, it will be reported via in the + /// Threshold in megabits per second for reporting slow streaming speeds. If + /// the streaming speed is below this threshold, it will be reported in the /// logs. pub const STREAM_SLOW_SPEED_REPORTING_THRESHOLD_MBPS: &str = "streamSlowSpeedReportingThresholdMbps"; /// Interval in seconds for reporting slow streaming speeds. If the streaming - /// speed is below the threshold, it will be reported every this many seconds. + /// speed is below the threshold, it will be reported at most once every this + /// many seconds. pub const STREAM_SLOW_SPEED_REPORTING_INTERVAL_SECONDS: &str = "streamSlowSpeedReportingIntervalSeconds"; } From f7cb205694d3dc313e49702fa04cdf1f3280c1b6 Mon Sep 17 00:00:00 2001 From: Paco Huelsz Prince Date: Mon, 23 Mar 2026 17:12:42 -0700 Subject: [PATCH 10/12] fmt --- crates/trident/src/io_utils/http/file.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/trident/src/io_utils/http/file.rs b/crates/trident/src/io_utils/http/file.rs index f29fe37d7..6f00c6d73 100644 --- a/crates/trident/src/io_utils/http/file.rs +++ b/crates/trident/src/io_utils/http/file.rs @@ -97,8 +97,7 @@ impl HttpFile { // // The clamped connect timeout is per request. We always do requests in // a retry loop that respects the overall timeout given to us. - let connect_timeout = - Duration::from_secs(MAX_PER_REQUEST_TIMEOUT_SECONDS).min(timeout); + let connect_timeout = Duration::from_secs(MAX_PER_REQUEST_TIMEOUT_SECONDS).min(timeout); let client = ClientBuilder::new() .connect_timeout(connect_timeout) .build() From 4e6367ab2cd8f713d632990fd3d3777936f30428 Mon Sep 17 00:00:00 2001 From: Paco Huelsz Prince Date: Mon, 23 Mar 2026 17:16:53 -0700 Subject: [PATCH 11/12] report cadence == 0 disables monitor --- crates/trident/src/io_utils/read_monitor.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/trident/src/io_utils/read_monitor.rs b/crates/trident/src/io_utils/read_monitor.rs index 7ac8d6793..59481ecc7 100644 --- a/crates/trident/src/io_utils/read_monitor.rs +++ b/crates/trident/src/io_utils/read_monitor.rs @@ -91,11 +91,11 @@ impl Read for ReadMonitor { let n = self.inner.read(buf)?; let elapsed = start.elapsed(); - // Return early if there is no threshold configured, to avoid the - // overhead of recording samples and computing averages. - // - // Also return early if n == 0, which naturally happens at EOF. - if self.threshold_mbps <= 0.0 || n == 0 { + // Return early: + // - if there is no threshold configured, which disables the monitor, or + // - report cadence is 0 or negative, which disables the monitor, or + // - if n == 0, which naturally happens at EOF. + if self.threshold_mbps <= 0.0 || self.report_cadence <= Duration::ZERO || n == 0 { return Ok(n); } From 020f69dfa51c19ec3153161d79ec798f6c7af1b8 Mon Sep 17 00:00:00 2001 From: Paco Huelsz Prince Date: Mon, 23 Mar 2026 17:32:56 -0700 Subject: [PATCH 12/12] Comments and nits --- crates/trident/src/io_utils/http/file.rs | 9 ++-- crates/trident/src/io_utils/http/subfile.rs | 55 +++++++++++++++++++-- crates/trident/src/lib.rs | 2 +- 3 files changed, 59 insertions(+), 7 deletions(-) diff --git a/crates/trident/src/io_utils/http/file.rs b/crates/trident/src/io_utils/http/file.rs index 6f00c6d73..1e0a6f1e6 100644 --- a/crates/trident/src/io_utils/http/file.rs +++ b/crates/trident/src/io_utils/http/file.rs @@ -263,9 +263,12 @@ impl HttpFile { fn section_reader_inner(&self, section_offset: u64, size: u64) -> HttpSubFile { if size == 0 { - // Create an empty subfile if the requested size is 0, to avoid - // making unnecessary HTTP requests - return HttpSubFile::new_with_client(self.url.clone(), 0, 0, self.client.clone()); + // When size is 0, create an empty subfile reader. This avoids + // making an HTTP request with an invalid range header (e.g. "Range: + // bytes=100-99") and also allows us to return an empty reader even + // if the server does not support range requests, as long as we + // never actually try to read from it. + return HttpSubFile::new_empty_with_client(self.url.clone(), self.client.clone()); } let end = section_offset + size - 1; diff --git a/crates/trident/src/io_utils/http/subfile.rs b/crates/trident/src/io_utils/http/subfile.rs index d885bfbca..dff6b5538 100644 --- a/crates/trident/src/io_utils/http/subfile.rs +++ b/crates/trident/src/io_utils/http/subfile.rs @@ -78,6 +78,10 @@ use super::HttpRangeRequest; /// occurs, without the caller even needing to be aware of it. Note that the /// `read()` call never fails in this example, even though multiple requests /// were needed and a network error occurred during the process. +/// +/// When `start > end`, the subfile is considered empty and all reads will +/// return EOF. This allows creating empty subfiles without needing to make an +/// HTTP request, which can be useful for certain edge cases and optimizations. pub struct HttpSubFile { /// The URL of the HTTP resource. url: Url, @@ -108,8 +112,8 @@ pub struct HttpSubFile { } impl HttpSubFile { - /// Creates a new HttpSubFile that reads the byte range [start, end] from the - /// given URL. The range is inclusive like the HTTP Range header, and is + /// Creates a new HttpSubFile that reads the byte range [start, end] from + /// the given URL. The range is inclusive like the HTTP Range header, and is /// expected to have been validated beforehand. #[allow(dead_code)] // Used in tests pub fn new(url: Url, start: u64, end: u64) -> Self { @@ -131,6 +135,22 @@ impl HttpSubFile { } } + /// Creates a new empty HttpSubFile with the given URL and HTTP client. The + /// returned HttpSubFile will always return `Ok(0)` when read. + pub(super) fn new_empty_with_client(url: Url, client: Client) -> Self { + HttpSubFile { + url, + start: 1, + end: 0, + client, + position: 0, + reader: None, + authorization: None, + timeout: Duration::from_secs(30), + end_is_parent_eof: false, + } + } + /// Sets the authorization header value to use for requests. pub(super) fn with_authorization(mut self, authorization: impl Into) -> Self { self.authorization = Some(authorization.into()); @@ -154,7 +174,8 @@ impl HttpSubFile { /// Returns the length of the subfile in bytes. pub fn size(&self) -> u64 { if self.start > self.end { - // Invalid range, treat as empty subfile. + // Invalid range, this means the subfile is empty and all reads + // should return EOF. 0 } else { // Add 1 because the range is inclusive. @@ -784,6 +805,34 @@ mod tests { mock.assert(); } + #[test] + fn test_empty_subfile_when_start_greater_than_end() { + let url = Url::parse("http://localhost:1/should-not-be-contacted").unwrap(); + let mut subfile = HttpSubFile::new(url, 10, 5); + + assert_eq!(subfile.size(), 0); + + let mut buf = vec![0_u8; 16]; + let bytes_read = subfile.read(&mut buf).unwrap(); + assert_eq!(bytes_read, 0); + + // A second read should also return EOF. + let bytes_read = subfile.read(&mut buf).unwrap(); + assert_eq!(bytes_read, 0); + } + + #[test] + fn test_new_empty_with_client_returns_eof() { + let url = Url::parse("http://localhost:1/should-not-be-contacted").unwrap(); + let mut subfile = HttpSubFile::new_empty_with_client(url, Client::new()); + + assert_eq!(subfile.size(), 0); + + let mut buf = vec![0_u8; 16]; + let bytes_read = subfile.read(&mut buf).unwrap(); + assert_eq!(bytes_read, 0); + } + #[test] fn test_timeout_is_respected() { let mut subfile = HttpSubFile::new( diff --git a/crates/trident/src/lib.rs b/crates/trident/src/lib.rs index 1ac2aef0a..999959936 100644 --- a/crates/trident/src/lib.rs +++ b/crates/trident/src/lib.rs @@ -104,7 +104,7 @@ const SAFETY_OVERRIDE_CHECK_PATH: &str = "/override-trident-safety-check"; /// Temporary location of the datastore for multiboot install scenarios. pub const TEMPORARY_DATASTORE_PATH: &str = "/tmp/trident-datastore.sqlite"; -/// Threshold in mega bits per second for reporting slow streaming speeds. If +/// Threshold in megabits per second for reporting slow streaming speeds. If /// the streaming speed is below this threshold, it will be reported via in the /// logs. pub const STREAM_SLOW_SPEED_REPORTING_THRESHOLD_MBPS_DEFAULT: f64 = 15.0;