From ef01c5c140b200f7bb883fe0e205ee3ab496be46 Mon Sep 17 00:00:00 2001 From: yuqian8 Date: Tue, 28 Jul 2026 20:52:43 +0800 Subject: [PATCH 1/7] feat(sandbox): add Firecracker disk I/O rate limiting (bandwidth + IOPS) Wire Firecracker's per-drive TokenBucket rate limiter into the sandbox lifecycle. Configurable via [machine.disk_rate_limit] with bandwidth (bytes/sec), IOPS caps, burst allowances, and refill interval. Applied at both fresh-boot (add_drive) and snapshot-resume (PATCH /drives) paths so all sandboxes are governed regardless of launch mode. --- config/default.toml | 8 +++++ src/cfg.rs | 24 +++++++++++++++ src/sandbox/firecracker/config.rs | 3 ++ src/sandbox/firecracker/instance.rs | 21 ++++++++++++- src/sandbox/firecracker/sandbox.rs | 46 +++++++++++++++++++++++++++++ 5 files changed, 101 insertions(+), 1 deletion(-) diff --git a/config/default.toml b/config/default.toml index 3dab300b..a4e1475a 100644 --- a/config/default.toml +++ b/config/default.toml @@ -110,6 +110,14 @@ veth_cidr = "10.12.0.0/16" mem_size_mib = 1024 vcpu_count = 2 +[machine.disk_rate_limit] +# enabled = true +# bandwidth_bytes_per_sec = 104857600 # 100 MB/s +# bandwidth_burst_bytes = 10485760 # 10 MB burst +# iops = 3000 +# iops_burst = 500 +# refill_time_ms = 1000 + [envd] version = "0.5.15" init_timeout_secs = 60 diff --git a/src/cfg.rs b/src/cfg.rs index baf32196..7d2f2ea3 100644 --- a/src/cfg.rs +++ b/src/cfg.rs @@ -252,6 +252,30 @@ pub struct MachineConfig { pub vcpu_count: u32, #[config(default = 1024u32)] pub mem_size_mib: u32, + #[config(nested)] + pub disk_rate_limit: DiskRateLimitConfig, +} + +#[derive(Debug, Config, Clone)] +pub struct DiskRateLimitConfig { + /// Enable per-sandbox disk I/O rate limiting via Firecracker's virtio-blk rate limiter. + #[config(default = false)] + pub enabled: bool, + /// Sustained disk bandwidth limit in bytes per second (0 = unlimited). + #[config(default = 0u64)] + pub bandwidth_bytes_per_sec: u64, + /// One-time burst allowance in bytes above the sustained bandwidth. + #[config(default = 0u64)] + pub bandwidth_burst_bytes: u64, + /// Sustained IOPS limit (0 = unlimited). + #[config(default = 0u64)] + pub iops: u64, + /// One-time burst allowance in operations above the sustained IOPS. + #[config(default = 0u64)] + pub iops_burst: u64, + /// Token bucket refill period in milliseconds. + #[config(default = 1000u64)] + pub refill_time_ms: u64, } #[derive(Debug, Config, Clone)] diff --git a/src/sandbox/firecracker/config.rs b/src/sandbox/firecracker/config.rs index a18ac70e..c6450856 100644 --- a/src/sandbox/firecracker/config.rs +++ b/src/sandbox/firecracker/config.rs @@ -326,6 +326,7 @@ pub struct FirecrackerSandboxConfig { pub boot_args: Option, pub vcpu_count: u32, pub mem_size_mib: u32, + pub disk_rate_limit: crate::cfg::DiskRateLimitConfig, } impl FirecrackerSandboxConfig { @@ -352,6 +353,7 @@ impl FirecrackerSandboxConfig { boot_args: None, vcpu_count: app_config.machine.vcpu_count, mem_size_mib: app_config.machine.mem_size_mib, + disk_rate_limit: app_config.machine.disk_rate_limit, } } @@ -385,6 +387,7 @@ impl FirecrackerSandboxConfig { .or_else(|| Some(DEFAULT_BOOT_ARGS.to_string())), vcpu_count: config.machine.vcpu_count, mem_size_mib: config.machine.mem_size_mib, + disk_rate_limit: config.machine.disk_rate_limit.clone(), }) } diff --git a/src/sandbox/firecracker/instance.rs b/src/sandbox/firecracker/instance.rs index 88d8cec7..637768b7 100644 --- a/src/sandbox/firecracker/instance.rs +++ b/src/sandbox/firecracker/instance.rs @@ -8,7 +8,9 @@ use anyhow::{bail, Context, Result}; use firecracker_client::models::drive::IoEngine; use firecracker_client::models::instance_action_info::ActionType; use firecracker_client::models::vm::State as VmState; -use firecracker_client::models::{mmds_config::Version as MmdsVersion, MmdsConfig}; +use firecracker_client::models::{ + mmds_config::Version as MmdsVersion, MmdsConfig, PartialDrive, +}; use firecracker_client::models::{ Balloon, BootSource, DirtyMemoryRanges, Drive, InstanceActionInfo, Logger, MachineConfiguration, MemoryBackend, NetworkInterface, NetworkOverride, RateLimiter, @@ -320,12 +322,14 @@ impl FirecrackerInstance { read_only: bool, direct: bool, io_engine: IoEngine, + rate_limiter: Option>, ) -> Result<()> { let mut drive = Drive::new(drive_id.to_string(), is_root_device); drive.path_on_host = Some(path_on_host.to_string_lossy().into_owned()); drive.is_read_only = Some(read_only); drive.direct = Some(direct); drive.io_engine = Some(io_engine); + drive.rate_limiter = rate_limiter; let path = format!("/drives/{}", drive_id); self.client .request_no_content(Method::PUT, &path, Some(&drive)) @@ -333,6 +337,21 @@ impl FirecrackerInstance { .with_context(|| format!("Failed to add/update drive {}", drive_id)) } + /// Updates the rate limiter on a running VM's drive via PATCH. + pub async fn patch_drive_rate_limiter( + &self, + drive_id: &str, + rate_limiter: Box, + ) -> Result<()> { + let mut partial = PartialDrive::new(drive_id.to_string()); + partial.rate_limiter = Some(rate_limiter); + let path = format!("/drives/{}", drive_id); + self.client + .request_no_content(Method::PATCH, &path, Some(&partial)) + .await + .with_context(|| format!("Failed to patch rate limiter on drive {}", drive_id)) + } + /// Adds a network interface. /// Pre-boot only. pub async fn add_network_interface( diff --git a/src/sandbox/firecracker/sandbox.rs b/src/sandbox/firecracker/sandbox.rs index 5adcaef0..2670f1f0 100644 --- a/src/sandbox/firecracker/sandbox.rs +++ b/src/sandbox/firecracker/sandbox.rs @@ -56,6 +56,39 @@ const VM_STATE_FILE_NAME: &str = "vm_state.bin"; const ROOTFS_DRIVE_PATH: &str = "rootfs.ext4"; const USER_ROOTFS_DRIVE_PATH: &str = "user-rootfs"; +fn build_disk_rate_limiter( + cfg: &crate::cfg::DiskRateLimitConfig, +) -> Option> { + if !cfg.enabled { + return None; + } + let mut rl = firecracker_client::models::RateLimiter::new(); + if cfg.bandwidth_bytes_per_sec > 0 { + let mut bw = firecracker_client::models::TokenBucket::new( + cfg.refill_time_ms as i64, + cfg.bandwidth_bytes_per_sec as i64, + ); + if cfg.bandwidth_burst_bytes > 0 { + bw.one_time_burst = Some(cfg.bandwidth_burst_bytes as i64); + } + rl.bandwidth = Some(Box::new(bw)); + } + if cfg.iops > 0 { + let mut ops = firecracker_client::models::TokenBucket::new( + cfg.refill_time_ms as i64, + cfg.iops as i64, + ); + if cfg.iops_burst > 0 { + ops.one_time_burst = Some(cfg.iops_burst as i64); + } + rl.ops = Some(Box::new(ops)); + } + if rl.bandwidth.is_none() && rl.ops.is_none() { + return None; + } + Some(Box::new(rl)) +} + pub(super) fn managed_snapshot_base() -> PathBuf { ConfigManager::global_config() .firecracker @@ -1470,6 +1503,15 @@ impl FirecrackerSandbox { self.fc_instance.set_mmds(&mmds_metadata).await?; self.fc_instance.resume().await?; + // Apply disk rate limiter to user rootfs drive after resume. + let disk_rl_cfg = &ConfigManager::global_config().machine.disk_rate_limit; + if let Some(rl) = build_disk_rate_limiter(disk_rl_cfg) { + self.fc_instance + .patch_drive_rate_limiter(USER_ROOTFS_DRIVE_ID, rl) + .await + .context("apply disk rate limiter after resume")?; + } + debug!("sandbox restored from snapshot config"); Ok(()) } @@ -1572,6 +1614,7 @@ impl FirecrackerSandbox { true, false, IoEngine::Sync, + None, ) .await .with_context(|| { @@ -1582,6 +1625,7 @@ impl FirecrackerSandbox { })?; // Drive 1 (/dev/vdb): user image, writable. + let disk_rl = build_disk_rate_limiter(&config.disk_rate_limit); self.fc_instance .add_drive( USER_ROOTFS_DRIVE_ID, @@ -1590,6 +1634,7 @@ impl FirecrackerSandbox { false, true, IoEngine::Async, + disk_rl, ) .await?; @@ -1631,6 +1676,7 @@ impl FirecrackerSandbox { drive.read_only, true, IoEngine::Async, + None, ) .await .with_context(|| format!("Failed to add extra drive {}", drive.drive_id))?; From ab2ab1f52b5b4c5c2806344fd9d3bad6fa3a73e7 Mon Sep 17 00:00:00 2001 From: yuqian8 Date: Tue, 4 Aug 2026 14:08:00 +0800 Subject: [PATCH 2/7] fix(sandbox): correct disk rate limiter refill and resume reconciliation Address review feedback on the Firecracker disk I/O rate limiter: - Drop the configurable refill_time_ms and pin the token-bucket refill period to 1000 ms, so the configured *_per_sec values are the actual sustained per-second rates. - Replace lossy `as i64` casts with checked conversions that surface an error instead of silently truncating oversized byte/IOPS values. - Unify the fresh-boot and resume paths behind a single post-boot PATCH helper, and remove the now-unused rate_limiter argument from add_drive. - Fix resume reconciliation: a restored snapshot inherits its previous limiter, and an empty RateLimiter PATCH is a no-op because Firecracker treats an absent bucket as "leave unchanged". When the current config disables limiting, overwrite both buckets with an effectively-unlimited bucket so the inherited throttle is actually cleared. Add unit tests covering bucket construction, the disabled/zero cases, checked-conversion overflow, and the unlimited-limiter override. --- config/default.toml | 1 - src/cfg.rs | 3 - src/sandbox/firecracker/instance.rs | 2 - src/sandbox/firecracker/sandbox.rs | 174 +++++++++++++++++++++++----- 4 files changed, 148 insertions(+), 32 deletions(-) diff --git a/config/default.toml b/config/default.toml index a4e1475a..8c8dbdf3 100644 --- a/config/default.toml +++ b/config/default.toml @@ -116,7 +116,6 @@ vcpu_count = 2 # bandwidth_burst_bytes = 10485760 # 10 MB burst # iops = 3000 # iops_burst = 500 -# refill_time_ms = 1000 [envd] version = "0.5.15" diff --git a/src/cfg.rs b/src/cfg.rs index 7d2f2ea3..0e3d7e38 100644 --- a/src/cfg.rs +++ b/src/cfg.rs @@ -273,9 +273,6 @@ pub struct DiskRateLimitConfig { /// One-time burst allowance in operations above the sustained IOPS. #[config(default = 0u64)] pub iops_burst: u64, - /// Token bucket refill period in milliseconds. - #[config(default = 1000u64)] - pub refill_time_ms: u64, } #[derive(Debug, Config, Clone)] diff --git a/src/sandbox/firecracker/instance.rs b/src/sandbox/firecracker/instance.rs index 637768b7..cb639ae7 100644 --- a/src/sandbox/firecracker/instance.rs +++ b/src/sandbox/firecracker/instance.rs @@ -322,14 +322,12 @@ impl FirecrackerInstance { read_only: bool, direct: bool, io_engine: IoEngine, - rate_limiter: Option>, ) -> Result<()> { let mut drive = Drive::new(drive_id.to_string(), is_root_device); drive.path_on_host = Some(path_on_host.to_string_lossy().into_owned()); drive.is_read_only = Some(read_only); drive.direct = Some(direct); drive.io_engine = Some(io_engine); - drive.rate_limiter = rate_limiter; let path = format!("/drives/{}", drive_id); self.client .request_no_content(Method::PUT, &path, Some(&drive)) diff --git a/src/sandbox/firecracker/sandbox.rs b/src/sandbox/firecracker/sandbox.rs index 2670f1f0..946d6aac 100644 --- a/src/sandbox/firecracker/sandbox.rs +++ b/src/sandbox/firecracker/sandbox.rs @@ -56,37 +56,68 @@ const VM_STATE_FILE_NAME: &str = "vm_state.bin"; const ROOTFS_DRIVE_PATH: &str = "rootfs.ext4"; const USER_ROOTFS_DRIVE_PATH: &str = "user-rootfs"; +/// Firecracker's `TokenBucket::size` is the number of tokens replenished every +/// `refill_time`, not a per-second rate. Pinning the refill period to 1000 ms +/// makes the configured `*_per_sec` values equal the sustained per-second rate. +const RATE_LIMIT_REFILL_TIME_MS: i64 = 1000; + fn build_disk_rate_limiter( cfg: &crate::cfg::DiskRateLimitConfig, -) -> Option> { +) -> Result>> { if !cfg.enabled { - return None; + return Ok(None); } let mut rl = firecracker_client::models::RateLimiter::new(); if cfg.bandwidth_bytes_per_sec > 0 { - let mut bw = firecracker_client::models::TokenBucket::new( - cfg.refill_time_ms as i64, - cfg.bandwidth_bytes_per_sec as i64, - ); + let size = i64::try_from(cfg.bandwidth_bytes_per_sec) + .context("disk bandwidth_bytes_per_sec exceeds Firecracker's i64 range")?; + let mut bw = firecracker_client::models::TokenBucket::new(RATE_LIMIT_REFILL_TIME_MS, size); if cfg.bandwidth_burst_bytes > 0 { - bw.one_time_burst = Some(cfg.bandwidth_burst_bytes as i64); + bw.one_time_burst = Some( + i64::try_from(cfg.bandwidth_burst_bytes) + .context("disk bandwidth_burst_bytes exceeds Firecracker's i64 range")?, + ); } rl.bandwidth = Some(Box::new(bw)); } if cfg.iops > 0 { - let mut ops = firecracker_client::models::TokenBucket::new( - cfg.refill_time_ms as i64, - cfg.iops as i64, - ); + let size = i64::try_from(cfg.iops).context("disk iops exceeds Firecracker's i64 range")?; + let mut ops = firecracker_client::models::TokenBucket::new(RATE_LIMIT_REFILL_TIME_MS, size); if cfg.iops_burst > 0 { - ops.one_time_burst = Some(cfg.iops_burst as i64); + ops.one_time_burst = Some( + i64::try_from(cfg.iops_burst) + .context("disk iops_burst exceeds Firecracker's i64 range")?, + ); } rl.ops = Some(Box::new(ops)); } if rl.bandwidth.is_none() && rl.ops.is_none() { - return None; + return Ok(None); } - Some(Box::new(rl)) + Ok(Some(Box::new(rl))) +} + +/// Build an effectively-unlimited rate limiter used to strip a limiter that a +/// resumed snapshot inherited when the current config disables limiting. +/// +/// Firecracker's `PATCH /drives` maps an absent token bucket to +/// `BucketUpdate::None` (leave unchanged), so PATCHing an empty `RateLimiter` +/// is a silent no-op and cannot remove an inherited limiter. Overwriting both +/// buckets with a huge size and a 1 ms refill is the only way to undo it: the +/// resulting rate dwarfs any real disk, so throttling no longer bites. +fn unlimited_rate_limiter() -> Box { + const UNLIMITED_BUCKET_SIZE: i64 = 1 << 40; // 1 TiB replenished every 1 ms + const UNLIMITED_REFILL_TIME_MS: i64 = 1; + let mut rl = firecracker_client::models::RateLimiter::new(); + rl.bandwidth = Some(Box::new(firecracker_client::models::TokenBucket::new( + UNLIMITED_REFILL_TIME_MS, + UNLIMITED_BUCKET_SIZE, + ))); + rl.ops = Some(Box::new(firecracker_client::models::TokenBucket::new( + UNLIMITED_REFILL_TIME_MS, + UNLIMITED_BUCKET_SIZE, + ))); + Box::new(rl) } pub(super) fn managed_snapshot_base() -> PathBuf { @@ -1290,6 +1321,7 @@ impl FirecrackerSandbox { self.configure_microvm(&config, boot_args.as_deref(), &extra_drive_attachments) .await?; self.fc_instance.start().await?; + self.apply_disk_rate_limiter(false).await?; debug!("fresh sandbox started"); Ok(()) } @@ -1503,14 +1535,10 @@ impl FirecrackerSandbox { self.fc_instance.set_mmds(&mmds_metadata).await?; self.fc_instance.resume().await?; - // Apply disk rate limiter to user rootfs drive after resume. - let disk_rl_cfg = &ConfigManager::global_config().machine.disk_rate_limit; - if let Some(rl) = build_disk_rate_limiter(disk_rl_cfg) { - self.fc_instance - .patch_drive_rate_limiter(USER_ROOTFS_DRIVE_ID, rl) - .await - .context("apply disk rate limiter after resume")?; - } + // A restored snapshot may carry a previously configured limiter, so + // reconcile against the node's current config, clearing any inherited + // limiter when disk rate limiting is now disabled. + self.apply_disk_rate_limiter(true).await?; debug!("sandbox restored from snapshot config"); Ok(()) @@ -1614,7 +1642,6 @@ impl FirecrackerSandbox { true, false, IoEngine::Sync, - None, ) .await .with_context(|| { @@ -1625,7 +1652,6 @@ impl FirecrackerSandbox { })?; // Drive 1 (/dev/vdb): user image, writable. - let disk_rl = build_disk_rate_limiter(&config.disk_rate_limit); self.fc_instance .add_drive( USER_ROOTFS_DRIVE_ID, @@ -1634,7 +1660,6 @@ impl FirecrackerSandbox { false, true, IoEngine::Async, - disk_rl, ) .await?; @@ -1676,7 +1701,6 @@ impl FirecrackerSandbox { drive.read_only, true, IoEngine::Async, - None, ) .await .with_context(|| format!("Failed to add extra drive {}", drive.drive_id))?; @@ -1685,6 +1709,27 @@ impl FirecrackerSandbox { Ok(()) } + /// Applies the node's configured disk rate limiter to the user rootfs drive + /// via a post-boot PATCH, unifying the fresh-boot and resume paths. + /// + /// When limiting is disabled, `clear_inherited` controls behavior: resume + /// passes `true` to overwrite any limiter a restored snapshot inherited with + /// an effectively-unlimited one (Firecracker cannot remove a limiter via + /// PATCH; see `unlimited_rate_limiter`), while a fresh boot passes `false` + /// and skips the PATCH entirely since its device model starts clean. + async fn apply_disk_rate_limiter(&self, clear_inherited: bool) -> Result<()> { + let cfg = &ConfigManager::global_config().machine.disk_rate_limit; + let rl = match build_disk_rate_limiter(cfg)? { + Some(rl) => rl, + None if clear_inherited => unlimited_rate_limiter(), + None => return Ok(()), + }; + self.fc_instance + .patch_drive_rate_limiter(USER_ROOTFS_DRIVE_ID, rl) + .await + .context("apply disk rate limiter to user rootfs drive") + } + async fn prepare_snapshot_backing_drives(&mut self, extra_drives: &[ExtraDrive]) -> Result<()> { if extra_drives.is_empty() { self.extra_drive_runtimes.clear(); @@ -1837,6 +1882,83 @@ mod tests { config } + fn rate_limit_cfg() -> crate::cfg::DiskRateLimitConfig { + crate::cfg::DiskRateLimitConfig { + enabled: true, + bandwidth_bytes_per_sec: 0, + bandwidth_burst_bytes: 0, + iops: 0, + iops_burst: 0, + } + } + + #[test] + fn rate_limiter_disabled_returns_none() { + let mut cfg = rate_limit_cfg(); + cfg.enabled = false; + cfg.bandwidth_bytes_per_sec = 104_857_600; + assert!(build_disk_rate_limiter(&cfg).unwrap().is_none()); + } + + #[test] + fn rate_limiter_enabled_but_all_zero_returns_none() { + assert!(build_disk_rate_limiter(&rate_limit_cfg()) + .unwrap() + .is_none()); + } + + #[test] + fn rate_limiter_bandwidth_size_equals_per_second_rate() { + let mut cfg = rate_limit_cfg(); + cfg.bandwidth_bytes_per_sec = 104_857_600; // 100 MB/s + cfg.bandwidth_burst_bytes = 10_485_760; + let rl = build_disk_rate_limiter(&cfg) + .unwrap() + .expect("limiter present"); + let bw = rl.bandwidth.expect("bandwidth bucket"); + // With refill pinned to 1000 ms, bucket size == sustained bytes/sec. + assert_eq!(bw.refill_time, RATE_LIMIT_REFILL_TIME_MS); + assert_eq!(bw.size, 104_857_600); + assert_eq!(bw.one_time_burst, Some(10_485_760)); + assert!(rl.ops.is_none()); + } + + #[test] + fn rate_limiter_iops_bucket_populated() { + let mut cfg = rate_limit_cfg(); + cfg.iops = 3000; + cfg.iops_burst = 500; + let rl = build_disk_rate_limiter(&cfg) + .unwrap() + .expect("limiter present"); + let ops = rl.ops.expect("ops bucket"); + assert_eq!(ops.refill_time, RATE_LIMIT_REFILL_TIME_MS); + assert_eq!(ops.size, 3000); + assert_eq!(ops.one_time_burst, Some(500)); + assert!(rl.bandwidth.is_none()); + } + + #[test] + fn rate_limiter_rejects_values_beyond_i64_range() { + let mut cfg = rate_limit_cfg(); + cfg.bandwidth_bytes_per_sec = u64::MAX; + assert!(build_disk_rate_limiter(&cfg).is_err()); + } + + #[test] + fn unlimited_rate_limiter_overwrites_both_buckets() { + // Firecracker treats an absent bucket in a PATCH as "leave unchanged", + // so clearing an inherited limiter requires overwriting both buckets + // with an effectively-unlimited one rather than an empty RateLimiter. + let rl = unlimited_rate_limiter(); + let bw = rl.bandwidth.expect("bandwidth bucket present"); + let ops = rl.ops.expect("ops bucket present"); + assert_eq!(bw.refill_time, 1); + assert_eq!(ops.refill_time, 1); + assert_eq!(bw.size, 1 << 40); + assert_eq!(ops.size, 1 << 40); + } + #[test] fn paused_state_image_cache_paths_use_snapshot_artifact_config() { let mut common = fresh_config().common; From 16f17220cbef92bfc69e93359a1ee560c4f34542 Mon Sep 17 00:00:00 2001 From: yuqian8 Date: Wed, 5 Aug 2026 15:18:43 +0800 Subject: [PATCH 3/7] fix(sandbox): apply fresh sandbox's own disk rate limit config apply_disk_rate_limiter previously always read the global config, leaving the disk_rate_limit captured on FirecrackerSandboxConfig unused. A fresh sandbox built from an explicit AppConfig could thus be throttled with a different global setting than the one used to construct it. Pass the config source in explicitly: fresh boot uses the sandbox's own launch config, while snapshot resume intentionally reconciles against the node's current global config. --- src/sandbox/firecracker/sandbox.rs | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/src/sandbox/firecracker/sandbox.rs b/src/sandbox/firecracker/sandbox.rs index 946d6aac..119aa566 100644 --- a/src/sandbox/firecracker/sandbox.rs +++ b/src/sandbox/firecracker/sandbox.rs @@ -1321,7 +1321,8 @@ impl FirecrackerSandbox { self.configure_microvm(&config, boot_args.as_deref(), &extra_drive_attachments) .await?; self.fc_instance.start().await?; - self.apply_disk_rate_limiter(false).await?; + self.apply_disk_rate_limiter(&config.disk_rate_limit, false) + .await?; debug!("fresh sandbox started"); Ok(()) } @@ -1538,7 +1539,11 @@ impl FirecrackerSandbox { // A restored snapshot may carry a previously configured limiter, so // reconcile against the node's current config, clearing any inherited // limiter when disk rate limiting is now disabled. - self.apply_disk_rate_limiter(true).await?; + self.apply_disk_rate_limiter( + &ConfigManager::global_config().machine.disk_rate_limit, + true, + ) + .await?; debug!("sandbox restored from snapshot config"); Ok(()) @@ -1709,16 +1714,22 @@ impl FirecrackerSandbox { Ok(()) } - /// Applies the node's configured disk rate limiter to the user rootfs drive - /// via a post-boot PATCH, unifying the fresh-boot and resume paths. + /// Applies the given disk rate limiter config to the user rootfs drive via a + /// post-boot PATCH, unifying the fresh-boot and resume paths. Fresh boot passes + /// the sandbox's own launch config so the configuration used to construct the + /// sandbox is the one applied; resume intentionally passes the node's current + /// global config to reconcile a restored snapshot against present settings. /// /// When limiting is disabled, `clear_inherited` controls behavior: resume /// passes `true` to overwrite any limiter a restored snapshot inherited with /// an effectively-unlimited one (Firecracker cannot remove a limiter via /// PATCH; see `unlimited_rate_limiter`), while a fresh boot passes `false` /// and skips the PATCH entirely since its device model starts clean. - async fn apply_disk_rate_limiter(&self, clear_inherited: bool) -> Result<()> { - let cfg = &ConfigManager::global_config().machine.disk_rate_limit; + async fn apply_disk_rate_limiter( + &self, + cfg: &crate::cfg::DiskRateLimitConfig, + clear_inherited: bool, + ) -> Result<()> { let rl = match build_disk_rate_limiter(cfg)? { Some(rl) => rl, None if clear_inherited => unlimited_rate_limiter(), From d394ede25470b03f2b8e30d06f3156c168fa0c0d Mon Sep 17 00:00:00 2001 From: yuqian8 Date: Wed, 5 Aug 2026 21:35:58 +0800 Subject: [PATCH 4/7] fix(sandbox): apply disk rate limiter before I/O and reconcile per dimension Address review feedback on the disk rate limiter: - cfg: reject a burst configured without a nonzero sustained rate (bandwidth_burst_bytes/iops_burst require bandwidth_bytes_per_sec/iops), which would otherwise be silently ineffective. - fresh boot: attach the limiter as pre-boot drive config (PUT /drives) instead of a post-start PATCH, closing the window where the guest could issue unthrottled I/O before the limiter was applied. Re-adds the rate_limiter parameter to Instance::add_drive. - resume: move the reconcile PATCH to before resume() (while the VM is loaded-but-paused) so the limiter is in force the instant the guest runs. - resume reconcile is now per-dimension: both buckets are always sent (configured bucket if set, otherwise an effectively-unlimited bucket). Firecracker treats an omitted bucket in PATCH as "leave unchanged", so an inherited dimension the current config leaves unset must be overwritten rather than omitted, or a snapshot-inherited limit would survive resume. --- src/cfg.rs | 57 +++++++ src/sandbox/firecracker/instance.rs | 7 +- src/sandbox/firecracker/sandbox.rs | 239 +++++++++++++++++----------- 3 files changed, 213 insertions(+), 90 deletions(-) diff --git a/src/cfg.rs b/src/cfg.rs index 0e3d7e38..c4003aef 100644 --- a/src/cfg.rs +++ b/src/cfg.rs @@ -799,6 +799,29 @@ impl AppConfig { } self.validate_memory_snapshot_background_download()?; self.validate_overlaybd_global_config_paths()?; + self.validate_disk_rate_limit()?; + Ok(()) + } + + /// Reject internally inconsistent disk rate limit configs so operator + /// mistakes fail at load time. A one-time burst is meaningless without a + /// nonzero sustained limit: `build_disk_rate_limiter` only creates a token + /// bucket when the sustained value is > 0, so a burst paired with a zero + /// sustained limit would be silently ignored. + fn validate_disk_rate_limit(&self) -> Result<()> { + let cfg = &self.machine.disk_rate_limit; + if cfg.bandwidth_burst_bytes > 0 && cfg.bandwidth_bytes_per_sec == 0 { + bail!( + "machine.disk_rate_limit: bandwidth_burst_bytes is set but \ + bandwidth_bytes_per_sec is 0; a burst requires a nonzero sustained limit" + ); + } + if cfg.iops_burst > 0 && cfg.iops == 0 { + bail!( + "machine.disk_rate_limit: iops_burst is set but iops is 0; \ + a burst requires a nonzero sustained limit" + ); + } Ok(()) } @@ -1131,6 +1154,40 @@ mod tests { ); } + #[test] + fn validate_rejects_disk_burst_without_sustained() { + let mut config = AppConfig::default(); + config.machine.disk_rate_limit.bandwidth_bytes_per_sec = 0; + config.machine.disk_rate_limit.bandwidth_burst_bytes = 1024; + let err = config.validate().unwrap_err(); + assert!( + err.to_string().contains("bandwidth_burst_bytes is set but"), + "unexpected error: {err}" + ); + + let mut config = AppConfig::default(); + config.machine.disk_rate_limit.iops = 0; + config.machine.disk_rate_limit.iops_burst = 500; + let err = config.validate().unwrap_err(); + assert!( + err.to_string().contains("iops_burst is set but"), + "unexpected error: {err}" + ); + } + + #[test] + fn validate_accepts_consistent_disk_rate_limit() { + let mut config = AppConfig::default(); + config.machine.disk_rate_limit.enabled = true; + config.machine.disk_rate_limit.bandwidth_bytes_per_sec = 104_857_600; + config.machine.disk_rate_limit.bandwidth_burst_bytes = 10_485_760; + config.machine.disk_rate_limit.iops = 3000; + config.machine.disk_rate_limit.iops_burst = 500; + config + .validate() + .expect("consistent disk rate limit config passes"); + } + #[test] fn validate_rejects_shared_overlaybd_global_config_path() { let mut config = AppConfig::default(); diff --git a/src/sandbox/firecracker/instance.rs b/src/sandbox/firecracker/instance.rs index cb639ae7..e8bf0033 100644 --- a/src/sandbox/firecracker/instance.rs +++ b/src/sandbox/firecracker/instance.rs @@ -313,7 +313,10 @@ impl FirecrackerInstance { /// /// `is_root_device` must be `true` for the boot rootfs drive and `false` /// for any non-root extra drive. This API is only used during the - /// pre-boot configuration phase. + /// pre-boot configuration phase. `rate_limiter` attaches a pre-boot limiter + /// so throttling is in force from the guest's first I/O (a post-start PATCH + /// would leave a brief unthrottled window). + #[allow(clippy::too_many_arguments)] pub async fn add_drive( &self, drive_id: &str, @@ -322,12 +325,14 @@ impl FirecrackerInstance { read_only: bool, direct: bool, io_engine: IoEngine, + rate_limiter: Option>, ) -> Result<()> { let mut drive = Drive::new(drive_id.to_string(), is_root_device); drive.path_on_host = Some(path_on_host.to_string_lossy().into_owned()); drive.is_read_only = Some(read_only); drive.direct = Some(direct); drive.io_engine = Some(io_engine); + drive.rate_limiter = rate_limiter; let path = format!("/drives/{}", drive_id); self.client .request_no_content(Method::PUT, &path, Some(&drive)) diff --git a/src/sandbox/firecracker/sandbox.rs b/src/sandbox/firecracker/sandbox.rs index 119aa566..4ae31747 100644 --- a/src/sandbox/firecracker/sandbox.rs +++ b/src/sandbox/firecracker/sandbox.rs @@ -61,63 +61,94 @@ const USER_ROOTFS_DRIVE_PATH: &str = "user-rootfs"; /// makes the configured `*_per_sec` values equal the sustained per-second rate. const RATE_LIMIT_REFILL_TIME_MS: i64 = 1000; +fn bandwidth_bucket( + cfg: &crate::cfg::DiskRateLimitConfig, +) -> Result>> { + if cfg.bandwidth_bytes_per_sec == 0 { + return Ok(None); + } + let size = i64::try_from(cfg.bandwidth_bytes_per_sec) + .context("disk bandwidth_bytes_per_sec exceeds Firecracker's i64 range")?; + let mut bw = firecracker_client::models::TokenBucket::new(RATE_LIMIT_REFILL_TIME_MS, size); + if cfg.bandwidth_burst_bytes > 0 { + bw.one_time_burst = Some( + i64::try_from(cfg.bandwidth_burst_bytes) + .context("disk bandwidth_burst_bytes exceeds Firecracker's i64 range")?, + ); + } + Ok(Some(Box::new(bw))) +} + +fn ops_bucket( + cfg: &crate::cfg::DiskRateLimitConfig, +) -> Result>> { + if cfg.iops == 0 { + return Ok(None); + } + let size = i64::try_from(cfg.iops).context("disk iops exceeds Firecracker's i64 range")?; + let mut ops = firecracker_client::models::TokenBucket::new(RATE_LIMIT_REFILL_TIME_MS, size); + if cfg.iops_burst > 0 { + ops.one_time_burst = Some( + i64::try_from(cfg.iops_burst) + .context("disk iops_burst exceeds Firecracker's i64 range")?, + ); + } + Ok(Some(Box::new(ops))) +} + +/// Build the limiter attached to the user rootfs drive at fresh boot (pre-boot +/// `PUT /drives`). Returns `None` when limiting is disabled or no dimension is +/// configured, in which case the drive is added with no limiter. fn build_disk_rate_limiter( cfg: &crate::cfg::DiskRateLimitConfig, ) -> Result>> { if !cfg.enabled { return Ok(None); } - let mut rl = firecracker_client::models::RateLimiter::new(); - if cfg.bandwidth_bytes_per_sec > 0 { - let size = i64::try_from(cfg.bandwidth_bytes_per_sec) - .context("disk bandwidth_bytes_per_sec exceeds Firecracker's i64 range")?; - let mut bw = firecracker_client::models::TokenBucket::new(RATE_LIMIT_REFILL_TIME_MS, size); - if cfg.bandwidth_burst_bytes > 0 { - bw.one_time_burst = Some( - i64::try_from(cfg.bandwidth_burst_bytes) - .context("disk bandwidth_burst_bytes exceeds Firecracker's i64 range")?, - ); - } - rl.bandwidth = Some(Box::new(bw)); - } - if cfg.iops > 0 { - let size = i64::try_from(cfg.iops).context("disk iops exceeds Firecracker's i64 range")?; - let mut ops = firecracker_client::models::TokenBucket::new(RATE_LIMIT_REFILL_TIME_MS, size); - if cfg.iops_burst > 0 { - ops.one_time_burst = Some( - i64::try_from(cfg.iops_burst) - .context("disk iops_burst exceeds Firecracker's i64 range")?, - ); - } - rl.ops = Some(Box::new(ops)); - } - if rl.bandwidth.is_none() && rl.ops.is_none() { + let bandwidth = bandwidth_bucket(cfg)?; + let ops = ops_bucket(cfg)?; + if bandwidth.is_none() && ops.is_none() { return Ok(None); } + let mut rl = firecracker_client::models::RateLimiter::new(); + rl.bandwidth = bandwidth; + rl.ops = ops; Ok(Some(Box::new(rl))) } -/// Build an effectively-unlimited rate limiter used to strip a limiter that a -/// resumed snapshot inherited when the current config disables limiting. +/// A token bucket so large it never throttles: 1 TiB replenished every 1 ms. /// -/// Firecracker's `PATCH /drives` maps an absent token bucket to -/// `BucketUpdate::None` (leave unchanged), so PATCHing an empty `RateLimiter` -/// is a silent no-op and cannot remove an inherited limiter. Overwriting both -/// buckets with a huge size and a 1 ms refill is the only way to undo it: the -/// resulting rate dwarfs any real disk, so throttling no longer bites. -fn unlimited_rate_limiter() -> Box { - const UNLIMITED_BUCKET_SIZE: i64 = 1 << 40; // 1 TiB replenished every 1 ms +/// Used to overwrite a snapshot-inherited limiter dimension the current config +/// leaves unset. Firecracker's `PATCH /drives` maps an *absent* token bucket to +/// `BucketUpdate::None` (leave unchanged), so an inherited limit cannot be +/// removed by omission — it must be overwritten with an effectively-unlimited +/// bucket, whose rate dwarfs any real disk so throttling no longer bites. +fn unlimited_bucket() -> Box { + const UNLIMITED_BUCKET_SIZE: i64 = 1 << 40; // 1 TiB const UNLIMITED_REFILL_TIME_MS: i64 = 1; - let mut rl = firecracker_client::models::RateLimiter::new(); - rl.bandwidth = Some(Box::new(firecracker_client::models::TokenBucket::new( - UNLIMITED_REFILL_TIME_MS, - UNLIMITED_BUCKET_SIZE, - ))); - rl.ops = Some(Box::new(firecracker_client::models::TokenBucket::new( + Box::new(firecracker_client::models::TokenBucket::new( UNLIMITED_REFILL_TIME_MS, UNLIMITED_BUCKET_SIZE, - ))); - Box::new(rl) + )) +} + +/// Build the limiter to PATCH on resume, reconciling a snapshot-inherited +/// limiter against the node's current config. BOTH buckets are always present: +/// a configured dimension uses its own bucket, an unset dimension is overwritten +/// with an unlimited bucket so any inherited limit on that dimension is cleared +/// (an omitted bucket would instead be left unchanged; see [`unlimited_bucket`]). +fn reconcile_disk_rate_limiter( + cfg: &crate::cfg::DiskRateLimitConfig, +) -> Result> { + let (bandwidth, ops) = if cfg.enabled { + (bandwidth_bucket(cfg)?, ops_bucket(cfg)?) + } else { + (None, None) + }; + let mut rl = firecracker_client::models::RateLimiter::new(); + rl.bandwidth = Some(bandwidth.unwrap_or_else(unlimited_bucket)); + rl.ops = Some(ops.unwrap_or_else(unlimited_bucket)); + Ok(Box::new(rl)) } pub(super) fn managed_snapshot_base() -> PathBuf { @@ -1321,8 +1352,6 @@ impl FirecrackerSandbox { self.configure_microvm(&config, boot_args.as_deref(), &extra_drive_attachments) .await?; self.fc_instance.start().await?; - self.apply_disk_rate_limiter(&config.disk_rate_limit, false) - .await?; debug!("fresh sandbox started"); Ok(()) } @@ -1534,16 +1563,22 @@ impl FirecrackerSandbox { let mmds_metadata = self.mmds_metadata(&config.common); self.fc_instance.set_mmds(&mmds_metadata).await?; - self.fc_instance.resume().await?; - // A restored snapshot may carry a previously configured limiter, so - // reconcile against the node's current config, clearing any inherited - // limiter when disk rate limiting is now disabled. - self.apply_disk_rate_limiter( + // A restored snapshot inherits whatever limiter was active when it was + // paused, so reconcile against the node's current config while the VM is + // still loaded-but-paused — before resume() lets the guest issue I/O. + // Both buckets are always overwritten (configured or unlimited) so an + // inherited dimension the current config leaves unset is cleared rather + // than left unchanged. + let reconciled = reconcile_disk_rate_limiter( &ConfigManager::global_config().machine.disk_rate_limit, - true, - ) - .await?; + )?; + self.fc_instance + .patch_drive_rate_limiter(USER_ROOTFS_DRIVE_ID, reconciled) + .await + .context("reconcile disk rate limiter on snapshot resume")?; + + self.fc_instance.resume().await?; debug!("sandbox restored from snapshot config"); Ok(()) @@ -1647,6 +1682,7 @@ impl FirecrackerSandbox { true, false, IoEngine::Sync, + None, ) .await .with_context(|| { @@ -1656,7 +1692,9 @@ impl FirecrackerSandbox { ) })?; - // Drive 1 (/dev/vdb): user image, writable. + // Drive 1 (/dev/vdb): user image, writable. The disk rate limiter is + // applied here as pre-boot drive config (rather than a post-start PATCH) + // so throttling is in force the instant the guest starts issuing I/O. self.fc_instance .add_drive( USER_ROOTFS_DRIVE_ID, @@ -1665,6 +1703,7 @@ impl FirecrackerSandbox { false, true, IoEngine::Async, + build_disk_rate_limiter(&config.disk_rate_limit)?, ) .await?; @@ -1706,6 +1745,7 @@ impl FirecrackerSandbox { drive.read_only, true, IoEngine::Async, + None, ) .await .with_context(|| format!("Failed to add extra drive {}", drive.drive_id))?; @@ -1714,33 +1754,6 @@ impl FirecrackerSandbox { Ok(()) } - /// Applies the given disk rate limiter config to the user rootfs drive via a - /// post-boot PATCH, unifying the fresh-boot and resume paths. Fresh boot passes - /// the sandbox's own launch config so the configuration used to construct the - /// sandbox is the one applied; resume intentionally passes the node's current - /// global config to reconcile a restored snapshot against present settings. - /// - /// When limiting is disabled, `clear_inherited` controls behavior: resume - /// passes `true` to overwrite any limiter a restored snapshot inherited with - /// an effectively-unlimited one (Firecracker cannot remove a limiter via - /// PATCH; see `unlimited_rate_limiter`), while a fresh boot passes `false` - /// and skips the PATCH entirely since its device model starts clean. - async fn apply_disk_rate_limiter( - &self, - cfg: &crate::cfg::DiskRateLimitConfig, - clear_inherited: bool, - ) -> Result<()> { - let rl = match build_disk_rate_limiter(cfg)? { - Some(rl) => rl, - None if clear_inherited => unlimited_rate_limiter(), - None => return Ok(()), - }; - self.fc_instance - .patch_drive_rate_limiter(USER_ROOTFS_DRIVE_ID, rl) - .await - .context("apply disk rate limiter to user rootfs drive") - } - async fn prepare_snapshot_backing_drives(&mut self, extra_drives: &[ExtraDrive]) -> Result<()> { if extra_drives.is_empty() { self.extra_drive_runtimes.clear(); @@ -1957,17 +1970,65 @@ mod tests { } #[test] - fn unlimited_rate_limiter_overwrites_both_buckets() { - // Firecracker treats an absent bucket in a PATCH as "leave unchanged", - // so clearing an inherited limiter requires overwriting both buckets - // with an effectively-unlimited one rather than an empty RateLimiter. - let rl = unlimited_rate_limiter(); + fn reconcile_disabled_makes_both_buckets_unlimited() { + // Firecracker treats an absent bucket in a PATCH as "leave unchanged", so + // clearing an inherited limiter requires overwriting BOTH buckets with an + // effectively-unlimited one rather than sending an empty RateLimiter. + let mut cfg = rate_limit_cfg(); + cfg.enabled = false; + cfg.bandwidth_bytes_per_sec = 100 << 20; + cfg.iops = 3000; + let rl = reconcile_disk_rate_limiter(&cfg).unwrap(); let bw = rl.bandwidth.expect("bandwidth bucket present"); let ops = rl.ops.expect("ops bucket present"); - assert_eq!(bw.refill_time, 1); - assert_eq!(ops.refill_time, 1); - assert_eq!(bw.size, 1 << 40); - assert_eq!(ops.size, 1 << 40); + assert_eq!((bw.refill_time, bw.size), (1, 1 << 40)); + assert_eq!((ops.refill_time, ops.size), (1, 1 << 40)); + } + + #[test] + fn reconcile_bandwidth_only_clears_inherited_iops() { + // Enabled with bandwidth but no iops: bandwidth gets its configured + // bucket, while the unset iops dimension is overwritten with an unlimited + // bucket so a snapshot-inherited IOPS limit does not survive the resume. + let mut cfg = rate_limit_cfg(); + cfg.enabled = true; + cfg.bandwidth_bytes_per_sec = 100 << 20; + cfg.iops = 0; + let rl = reconcile_disk_rate_limiter(&cfg).unwrap(); + let bw = rl.bandwidth.expect("bandwidth bucket present"); + let ops = rl.ops.expect("ops bucket present"); + assert_eq!(bw.refill_time, RATE_LIMIT_REFILL_TIME_MS); + assert_eq!(bw.size, 100 << 20); + assert_eq!((ops.refill_time, ops.size), (1, 1 << 40)); + } + + #[test] + fn reconcile_iops_only_clears_inherited_bandwidth() { + let mut cfg = rate_limit_cfg(); + cfg.enabled = true; + cfg.bandwidth_bytes_per_sec = 0; + cfg.iops = 3000; + let rl = reconcile_disk_rate_limiter(&cfg).unwrap(); + let bw = rl.bandwidth.expect("bandwidth bucket present"); + let ops = rl.ops.expect("ops bucket present"); + assert_eq!((bw.refill_time, bw.size), (1, 1 << 40)); + assert_eq!(ops.refill_time, RATE_LIMIT_REFILL_TIME_MS); + assert_eq!(ops.size, 3000); + } + + #[test] + fn reconcile_both_dimensions_use_configured_buckets() { + let mut cfg = rate_limit_cfg(); + cfg.enabled = true; + cfg.bandwidth_bytes_per_sec = 100 << 20; + cfg.iops = 3000; + let rl = reconcile_disk_rate_limiter(&cfg).unwrap(); + let bw = rl.bandwidth.expect("bandwidth bucket present"); + let ops = rl.ops.expect("ops bucket present"); + assert_eq!(bw.refill_time, RATE_LIMIT_REFILL_TIME_MS); + assert_eq!(bw.size, 100 << 20); + assert_eq!(ops.refill_time, RATE_LIMIT_REFILL_TIME_MS); + assert_eq!(ops.size, 3000); } #[test] From 26994b8e07e8e2ffde1dc6c34034ad975f65be85 Mon Sep 17 00:00:00 2001 From: yuqian8 Date: Thu, 6 Aug 2026 14:50:54 +0800 Subject: [PATCH 5/7] fix(sandbox): refine disk rate limit validation and disable semantics - cfg: skip disk_rate_limit validation entirely when disabled - cfg: reject rate limit values exceeding i64::MAX with field-named errors - sandbox: use size-0 token bucket to disable a limiter dimension instead of an unlimited large-value bucket (exact Firecracker disable semantics) - config: carry disk_rate_limit in FirecrackerCommonConfig so fresh boot and snapshot resume read the same config and it persists across pause/resume, dropping the resume-path dependency on the global config --- src/cfg.rs | 64 +++++++++++++++++++++++++++--- src/sandbox/firecracker/config.rs | 12 ++++-- src/sandbox/firecracker/sandbox.rs | 54 ++++++++++++------------- 3 files changed, 92 insertions(+), 38 deletions(-) diff --git a/src/cfg.rs b/src/cfg.rs index c4003aef..54916d5e 100644 --- a/src/cfg.rs +++ b/src/cfg.rs @@ -256,7 +256,7 @@ pub struct MachineConfig { pub disk_rate_limit: DiskRateLimitConfig, } -#[derive(Debug, Config, Clone)] +#[derive(Debug, Config, Clone, Default, serde::Serialize, serde::Deserialize)] pub struct DiskRateLimitConfig { /// Enable per-sandbox disk I/O rate limiting via Firecracker's virtio-blk rate limiter. #[config(default = false)] @@ -803,13 +803,22 @@ impl AppConfig { Ok(()) } - /// Reject internally inconsistent disk rate limit configs so operator - /// mistakes fail at load time. A one-time burst is meaningless without a - /// nonzero sustained limit: `build_disk_rate_limiter` only creates a token - /// bucket when the sustained value is > 0, so a burst paired with a zero - /// sustained limit would be silently ignored. + /// Reject internally inconsistent or out-of-range disk rate limit configs so + /// operator mistakes fail at load time. Disabled sections are skipped: both + /// the fresh-boot and snapshot-resume paths ignore all configured values when + /// disabled, so dormant/pre-staged values must not block startup. + /// + /// When enabled: a one-time burst is meaningless without a nonzero sustained + /// limit (`build_disk_rate_limiter` only creates a bucket when the sustained + /// value is > 0, so a burst paired with a zero sustained limit is silently + /// ignored), and every value must fit Firecracker's signed `i64` token-bucket + /// fields (the consumer converts with `i64::try_from`, so an out-of-range + /// value would otherwise only fail later at sandbox start). fn validate_disk_rate_limit(&self) -> Result<()> { let cfg = &self.machine.disk_rate_limit; + if !cfg.enabled { + return Ok(()); + } if cfg.bandwidth_burst_bytes > 0 && cfg.bandwidth_bytes_per_sec == 0 { bail!( "machine.disk_rate_limit: bandwidth_burst_bytes is set but \ @@ -822,6 +831,20 @@ impl AppConfig { a burst requires a nonzero sustained limit" ); } + for (name, value) in [ + ("bandwidth_bytes_per_sec", cfg.bandwidth_bytes_per_sec), + ("bandwidth_burst_bytes", cfg.bandwidth_burst_bytes), + ("iops", cfg.iops), + ("iops_burst", cfg.iops_burst), + ] { + if value > i64::MAX as u64 { + bail!( + "machine.disk_rate_limit.{name} ({value}) exceeds the maximum \ + supported value {}", + i64::MAX + ); + } + } Ok(()) } @@ -1157,6 +1180,7 @@ mod tests { #[test] fn validate_rejects_disk_burst_without_sustained() { let mut config = AppConfig::default(); + config.machine.disk_rate_limit.enabled = true; config.machine.disk_rate_limit.bandwidth_bytes_per_sec = 0; config.machine.disk_rate_limit.bandwidth_burst_bytes = 1024; let err = config.validate().unwrap_err(); @@ -1166,6 +1190,7 @@ mod tests { ); let mut config = AppConfig::default(); + config.machine.disk_rate_limit.enabled = true; config.machine.disk_rate_limit.iops = 0; config.machine.disk_rate_limit.iops_burst = 500; let err = config.validate().unwrap_err(); @@ -1175,6 +1200,33 @@ mod tests { ); } + #[test] + fn validate_skips_disabled_disk_rate_limit() { + // A disabled section is ignored at runtime, so even internally + // inconsistent or out-of-range values must not block startup. + let mut config = AppConfig::default(); + config.machine.disk_rate_limit.enabled = false; + config.machine.disk_rate_limit.bandwidth_bytes_per_sec = 0; + config.machine.disk_rate_limit.bandwidth_burst_bytes = 1024; + config.machine.disk_rate_limit.iops = u64::MAX; + config + .validate() + .expect("disabled disk rate limit config is not validated"); + } + + #[test] + fn validate_rejects_disk_rate_limit_above_i64_max() { + let mut config = AppConfig::default(); + config.machine.disk_rate_limit.enabled = true; + config.machine.disk_rate_limit.bandwidth_bytes_per_sec = i64::MAX as u64 + 1; + let err = config.validate().unwrap_err(); + assert!( + err.to_string() + .contains("machine.disk_rate_limit.bandwidth_bytes_per_sec"), + "unexpected error: {err}" + ); + } + #[test] fn validate_accepts_consistent_disk_rate_limit() { let mut config = AppConfig::default(); diff --git a/src/sandbox/firecracker/config.rs b/src/sandbox/firecracker/config.rs index c6450856..0236446b 100644 --- a/src/sandbox/firecracker/config.rs +++ b/src/sandbox/firecracker/config.rs @@ -150,6 +150,12 @@ pub struct FirecrackerCommonConfig { /// start-fresh / start-resume hooks. Persisted with snapshot configs. #[serde(default, skip_serializing_if = "Option::is_none")] pub custom_extension_params: Option, + /// Effective disk I/O rate limit for the user rootfs drive. Carried in the + /// common config so fresh boot and snapshot resume apply the same config + /// and it persists across pause/resume, instead of each path reaching for + /// the process-global config. + #[serde(default)] + pub disk_rate_limit: crate::cfg::DiskRateLimitConfig, } impl FirecrackerCommonConfig { @@ -181,6 +187,7 @@ impl FirecrackerCommonConfig { cpu_config_json: None, network_policy: None, custom_extension_params: None, + disk_rate_limit: crate::cfg::DiskRateLimitConfig::default(), } } @@ -191,6 +198,7 @@ impl FirecrackerCommonConfig { let mut common = Self::new(firecracker_binary, tools_drive_version, runtime_policy); common.envd_version = config.envd.version.clone(); + common.disk_rate_limit = config.machine.disk_rate_limit.clone(); common.rootfs_allow_shrink = config.ublk.overlaybd.allow_shrink; common.control_plane_port = config.tools.control_plane_port; common.firecracker_work_base_dir = config.firecracker.work_dir.clone(); @@ -326,7 +334,6 @@ pub struct FirecrackerSandboxConfig { pub boot_args: Option, pub vcpu_count: u32, pub mem_size_mib: u32, - pub disk_rate_limit: crate::cfg::DiskRateLimitConfig, } impl FirecrackerSandboxConfig { @@ -347,13 +354,13 @@ impl FirecrackerSandboxConfig { read_only: false, runtime_upper_mode: UpperMode::LogStructured, }); + common.disk_rate_limit = app_config.machine.disk_rate_limit; Self { common, kernel_image, boot_args: None, vcpu_count: app_config.machine.vcpu_count, mem_size_mib: app_config.machine.mem_size_mib, - disk_rate_limit: app_config.machine.disk_rate_limit, } } @@ -387,7 +394,6 @@ impl FirecrackerSandboxConfig { .or_else(|| Some(DEFAULT_BOOT_ARGS.to_string())), vcpu_count: config.machine.vcpu_count, mem_size_mib: config.machine.mem_size_mib, - disk_rate_limit: config.machine.disk_rate_limit.clone(), }) } diff --git a/src/sandbox/firecracker/sandbox.rs b/src/sandbox/firecracker/sandbox.rs index 4ae31747..a3ca8eca 100644 --- a/src/sandbox/firecracker/sandbox.rs +++ b/src/sandbox/firecracker/sandbox.rs @@ -116,27 +116,25 @@ fn build_disk_rate_limiter( Ok(Some(Box::new(rl))) } -/// A token bucket so large it never throttles: 1 TiB replenished every 1 ms. +/// A token bucket Firecracker interprets as "disable this dimension". /// -/// Used to overwrite a snapshot-inherited limiter dimension the current config -/// leaves unset. Firecracker's `PATCH /drives` maps an *absent* token bucket to -/// `BucketUpdate::None` (leave unchanged), so an inherited limit cannot be -/// removed by omission — it must be overwritten with an effectively-unlimited -/// bucket, whose rate dwarfs any real disk so throttling no longer bites. -fn unlimited_bucket() -> Box { - const UNLIMITED_BUCKET_SIZE: i64 = 1 << 40; // 1 TiB - const UNLIMITED_REFILL_TIME_MS: i64 = 1; - Box::new(firecracker_client::models::TokenBucket::new( - UNLIMITED_REFILL_TIME_MS, - UNLIMITED_BUCKET_SIZE, - )) +/// Firecracker's `PATCH /drives` maps an *absent* token bucket to +/// `BucketUpdate::None` (leave unchanged), so a snapshot-inherited limit cannot +/// be removed by omission. A bucket with `size == 0` is instead treated as an +/// explicit disable, giving exact remove semantics without depending on the +/// runtime accepting an unusually large limiter. +fn disabled_bucket() -> Box { + // size = 0 is the disable signal; refill_time is irrelevant then but kept + // nonzero so the bucket is still structurally valid. + Box::new(firecracker_client::models::TokenBucket::new(1, 0)) } /// Build the limiter to PATCH on resume, reconciling a snapshot-inherited /// limiter against the node's current config. BOTH buckets are always present: /// a configured dimension uses its own bucket, an unset dimension is overwritten -/// with an unlimited bucket so any inherited limit on that dimension is cleared -/// (an omitted bucket would instead be left unchanged; see [`unlimited_bucket`]). +/// with a disabled (`size == 0`) bucket so any inherited limit on that dimension +/// is cleared (an omitted bucket would instead be left unchanged; see +/// [`disabled_bucket`]). fn reconcile_disk_rate_limiter( cfg: &crate::cfg::DiskRateLimitConfig, ) -> Result> { @@ -146,8 +144,8 @@ fn reconcile_disk_rate_limiter( (None, None) }; let mut rl = firecracker_client::models::RateLimiter::new(); - rl.bandwidth = Some(bandwidth.unwrap_or_else(unlimited_bucket)); - rl.ops = Some(ops.unwrap_or_else(unlimited_bucket)); + rl.bandwidth = Some(bandwidth.unwrap_or_else(disabled_bucket)); + rl.ops = Some(ops.unwrap_or_else(disabled_bucket)); Ok(Box::new(rl)) } @@ -1570,9 +1568,7 @@ impl FirecrackerSandbox { // Both buckets are always overwritten (configured or unlimited) so an // inherited dimension the current config leaves unset is cleared rather // than left unchanged. - let reconciled = reconcile_disk_rate_limiter( - &ConfigManager::global_config().machine.disk_rate_limit, - )?; + let reconciled = reconcile_disk_rate_limiter(&config.common.disk_rate_limit)?; self.fc_instance .patch_drive_rate_limiter(USER_ROOTFS_DRIVE_ID, reconciled) .await @@ -1703,7 +1699,7 @@ impl FirecrackerSandbox { false, true, IoEngine::Async, - build_disk_rate_limiter(&config.disk_rate_limit)?, + build_disk_rate_limiter(&config.common.disk_rate_limit)?, ) .await?; @@ -1970,10 +1966,10 @@ mod tests { } #[test] - fn reconcile_disabled_makes_both_buckets_unlimited() { + fn reconcile_disabled_makes_both_buckets_disabled() { // Firecracker treats an absent bucket in a PATCH as "leave unchanged", so - // clearing an inherited limiter requires overwriting BOTH buckets with an - // effectively-unlimited one rather than sending an empty RateLimiter. + // clearing an inherited limiter requires overwriting BOTH buckets with a + // disabled (size == 0) bucket rather than sending an empty RateLimiter. let mut cfg = rate_limit_cfg(); cfg.enabled = false; cfg.bandwidth_bytes_per_sec = 100 << 20; @@ -1981,14 +1977,14 @@ mod tests { let rl = reconcile_disk_rate_limiter(&cfg).unwrap(); let bw = rl.bandwidth.expect("bandwidth bucket present"); let ops = rl.ops.expect("ops bucket present"); - assert_eq!((bw.refill_time, bw.size), (1, 1 << 40)); - assert_eq!((ops.refill_time, ops.size), (1, 1 << 40)); + assert_eq!(bw.size, 0); + assert_eq!(ops.size, 0); } #[test] fn reconcile_bandwidth_only_clears_inherited_iops() { // Enabled with bandwidth but no iops: bandwidth gets its configured - // bucket, while the unset iops dimension is overwritten with an unlimited + // bucket, while the unset iops dimension is overwritten with a disabled // bucket so a snapshot-inherited IOPS limit does not survive the resume. let mut cfg = rate_limit_cfg(); cfg.enabled = true; @@ -1999,7 +1995,7 @@ mod tests { let ops = rl.ops.expect("ops bucket present"); assert_eq!(bw.refill_time, RATE_LIMIT_REFILL_TIME_MS); assert_eq!(bw.size, 100 << 20); - assert_eq!((ops.refill_time, ops.size), (1, 1 << 40)); + assert_eq!(ops.size, 0); } #[test] @@ -2011,7 +2007,7 @@ mod tests { let rl = reconcile_disk_rate_limiter(&cfg).unwrap(); let bw = rl.bandwidth.expect("bandwidth bucket present"); let ops = rl.ops.expect("ops bucket present"); - assert_eq!((bw.refill_time, bw.size), (1, 1 << 40)); + assert_eq!(bw.size, 0); assert_eq!(ops.refill_time, RATE_LIMIT_REFILL_TIME_MS); assert_eq!(ops.size, 3000); } From 3535c8bffe51a4e5e0013ce4d461a52882384ac8 Mon Sep 17 00:00:00 2001 From: yuqian8 Date: Thu, 6 Aug 2026 17:54:22 +0800 Subject: [PATCH 6/7] fix(sandbox): use all-zero token bucket as disable sentinel and fix fmt - disabled_bucket now sends {size: 0, refill_time: 0}; a mixed bucket (size 0, refill_time 1) is not Firecracker's disable sentinel and can be rejected as invalid, failing snapshot resume for an unset dimension - fix rustfmt violation in instance.rs import grouping --- src/sandbox/firecracker/instance.rs | 4 +--- src/sandbox/firecracker/sandbox.rs | 11 +++++------ 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/sandbox/firecracker/instance.rs b/src/sandbox/firecracker/instance.rs index e8bf0033..227a5782 100644 --- a/src/sandbox/firecracker/instance.rs +++ b/src/sandbox/firecracker/instance.rs @@ -8,9 +8,7 @@ use anyhow::{bail, Context, Result}; use firecracker_client::models::drive::IoEngine; use firecracker_client::models::instance_action_info::ActionType; use firecracker_client::models::vm::State as VmState; -use firecracker_client::models::{ - mmds_config::Version as MmdsVersion, MmdsConfig, PartialDrive, -}; +use firecracker_client::models::{mmds_config::Version as MmdsVersion, MmdsConfig, PartialDrive}; use firecracker_client::models::{ Balloon, BootSource, DirtyMemoryRanges, Drive, InstanceActionInfo, Logger, MachineConfiguration, MemoryBackend, NetworkInterface, NetworkOverride, RateLimiter, diff --git a/src/sandbox/firecracker/sandbox.rs b/src/sandbox/firecracker/sandbox.rs index a3ca8eca..88858271 100644 --- a/src/sandbox/firecracker/sandbox.rs +++ b/src/sandbox/firecracker/sandbox.rs @@ -120,13 +120,12 @@ fn build_disk_rate_limiter( /// /// Firecracker's `PATCH /drives` maps an *absent* token bucket to /// `BucketUpdate::None` (leave unchanged), so a snapshot-inherited limit cannot -/// be removed by omission. A bucket with `size == 0` is instead treated as an -/// explicit disable, giving exact remove semantics without depending on the -/// runtime accepting an unusually large limiter. +/// be removed by omission. The explicit disable sentinel is a bucket with both +/// `size == 0` and `refill_time == 0`; a mixed bucket (e.g. `size == 0`, +/// `refill_time == 1`) is not the sentinel and can be rejected as an invalid +/// token bucket, failing the resume PATCH. Send both fields as zero. fn disabled_bucket() -> Box { - // size = 0 is the disable signal; refill_time is irrelevant then but kept - // nonzero so the bucket is still structurally valid. - Box::new(firecracker_client::models::TokenBucket::new(1, 0)) + Box::new(firecracker_client::models::TokenBucket::new(0, 0)) } /// Build the limiter to PATCH on resume, reconciling a snapshot-inherited From de45f98e66bac49344d302fbb0d38cd0f5906a19 Mon Sep 17 00:00:00 2001 From: yuqian8 Date: Thu, 6 Aug 2026 20:15:31 +0800 Subject: [PATCH 7/7] docs(cfg): clarify one_time_burst is a one-time start-of-VM allowance Document bandwidth/IOPS burst fields as Firecracker one_time_burst: a separate allowance granted once at VM start, consumed before the sustained bucket and not replenished, so it absorbs the initial I/O spike rather than raising the steady-state rate. --- config/default.toml | 6 +++--- src/cfg.rs | 9 +++++++-- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/config/default.toml b/config/default.toml index 8c8dbdf3..ecc97b5e 100644 --- a/config/default.toml +++ b/config/default.toml @@ -112,10 +112,10 @@ vcpu_count = 2 [machine.disk_rate_limit] # enabled = true -# bandwidth_bytes_per_sec = 104857600 # 100 MB/s -# bandwidth_burst_bytes = 10485760 # 10 MB burst +# bandwidth_bytes_per_sec = 104857600 # 100 MB/s sustained +# bandwidth_burst_bytes = 10485760 # 10 MB one-time burst at VM start (not replenished) # iops = 3000 -# iops_burst = 500 +# iops_burst = 500 # one-time IOPS burst at VM start (not replenished) [envd] version = "0.5.15" diff --git a/src/cfg.rs b/src/cfg.rs index 54916d5e..fcc8cc2b 100644 --- a/src/cfg.rs +++ b/src/cfg.rs @@ -264,13 +264,18 @@ pub struct DiskRateLimitConfig { /// Sustained disk bandwidth limit in bytes per second (0 = unlimited). #[config(default = 0u64)] pub bandwidth_bytes_per_sec: u64, - /// One-time burst allowance in bytes above the sustained bandwidth. + /// One-time bandwidth burst in bytes, granted once when the VM starts (maps + /// to Firecracker's `one_time_burst`). It is a separate allowance consumed + /// before the sustained bucket and is not replenished after use, so it only + /// absorbs the initial I/O spike; it does not raise the steady-state rate. #[config(default = 0u64)] pub bandwidth_burst_bytes: u64, /// Sustained IOPS limit (0 = unlimited). #[config(default = 0u64)] pub iops: u64, - /// One-time burst allowance in operations above the sustained IOPS. + /// One-time IOPS burst, granted once when the VM starts (maps to + /// Firecracker's `one_time_burst`). Consumed before the sustained bucket and + /// not replenished, so it only absorbs the initial spike, not steady state. #[config(default = 0u64)] pub iops_burst: u64, }