feat(sandbox): add Firecracker disk I/O rate limiting (bandwidth + IOPS) - #48
feat(sandbox): add Firecracker disk I/O rate limiting (bandwidth + IOPS)#48rogeroger-yu wants to merge 1 commit into
Conversation
|
🔍 OpenCodeReview found 3 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| # bandwidth_bytes_per_sec = 104857600 # 100 MB/s | ||
| # bandwidth_burst_bytes = 10485760 # 10 MB burst | ||
| # iops = 3000 | ||
| # iops_burst = 500 | ||
| # refill_time_ms = 1000 |
There was a problem hiding this comment.
The *_per_sec naming/documentation is inconsistent with how this configuration is converted to Firecracker token buckets: these values are passed directly as the bucket size, while refill_time_ms controls how often that many tokens are replenished. Thus bandwidth_bytes_per_sec = 104857600 is 100 MB/s only at 1000 ms; setting 500 ms yields 200 MB/s, and iops behaves similarly. Either convert per-second rates into bucket sizes based on refill_time_ms, or rename these options to bucket-size values and document the resulting rate semantics.
| /// Sustained disk bandwidth limit in bytes per second (0 = unlimited). | ||
| #[config(default = 0u64)] | ||
| pub bandwidth_bytes_per_sec: u64, |
There was a problem hiding this comment.
This value is described as a per-second rate, but the Firecracker integration passes it directly as the token-bucket size. The effective rate is size / refill_time, so any configured refill_time_ms other than 1000 changes the requested bytes/second (for example, 500 ms doubles it). Either remove/configure a fixed 1000 ms refill period, or convert the per-second value to a bucket size using checked arithmetic before constructing the limiter.
| /// Token bucket refill period in milliseconds. | ||
| #[config(default = 1000u64)] | ||
| pub refill_time_ms: u64, |
There was a problem hiding this comment.
These public values are later converted from u64 to Firecracker's i64 fields with as, but there is no configuration validation. Values above i64::MAX wrap to negative numbers, and refill_time_ms = 0 creates an invalid token bucket whenever bandwidth or IOPS limiting is configured, causing sandbox startup/restore to fail at the Firecracker API boundary. Validate enabled limiter values (refill_time_ms > 0 and every converted value <= i64::MAX) during AppConfig::validate, or use checked conversions when building the limiter.
| let mut bw = firecracker_client::models::TokenBucket::new( | ||
| cfg.refill_time_ms as i64, | ||
| cfg.bandwidth_bytes_per_sec as i64, | ||
| ); |
There was a problem hiding this comment.
TokenBucket::size is the number of tokens added per refill_time, not a per-second rate. Passing bandwidth_bytes_per_sec directly only produces the configured rate when refill_time_ms == 1000; for example, 100 MB/s with 500 ms refills becomes 200 MB/s. The IOPS bucket below has the same issue. Scale each configured per-second rate by refill_time_ms / 1000 (with checked arithmetic and a documented rounding policy), or remove the configurable refill period.
| cfg.refill_time_ms as i64, | ||
| cfg.bandwidth_bytes_per_sec as i64, |
There was a problem hiding this comment.
These values are user-configurable u64s, and as i64 silently wraps values above i64::MAX into negative API fields. The burst and IOPS conversions have the same problem. Validate all fields with i64::try_from and propagate a configuration error; this requires making build_disk_rate_limiter return Result<Option<_>>.
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.
35f51d9 to
ef01c5c
Compare
| # bandwidth_bytes_per_sec = 104857600 # 100 MB/s | ||
| # bandwidth_burst_bytes = 10485760 # 10 MB burst | ||
| # iops = 3000 | ||
| # iops_burst = 500 | ||
| # refill_time_ms = 1000 |
There was a problem hiding this comment.
These names/comments promise per-second limits, but the implementation passes bandwidth_bytes_per_sec and iops directly as Firecracker token-bucket sizes. The effective rate is bucket size per refill_time_ms, so changing refill_time_ms from 1000 changes the configured per-second rate (for example, 500 ms doubles it). Either remove/configure a fixed 1000 ms refill interval, rename the fields as per-refill bucket sizes, or scale the bucket sizes by refill_time_ms / 1000 with overflow/range validation.
| let mut bw = firecracker_client::models::TokenBucket::new( | ||
| cfg.refill_time_ms as i64, | ||
| cfg.bandwidth_bytes_per_sec as i64, | ||
| ); |
There was a problem hiding this comment.
TokenBucket::size is the number of tokens replenished per refill_time, but this passes a per-second value unchanged. Therefore any configured refill period other than 1000 ms changes the actual sustained rate (for example, 100 MB/s with 500 ms becomes 200 MB/s). Scale the bandwidth and IOPS bucket sizes by refill_time_ms / 1000, or remove the configurable refill period. Please also use checked arithmetic/conversions because these u64 as i64 casts (and the equivalent IOPS/burst casts below) wrap values above i64::MAX into invalid negative Firecracker fields instead of returning a configuration error.
Suggestion:
| let mut bw = firecracker_client::models::TokenBucket::new( | |
| cfg.refill_time_ms as i64, | |
| cfg.bandwidth_bytes_per_sec as i64, | |
| ); | |
| let refill_time = i64::try_from(cfg.refill_time_ms) | |
| .context("disk rate-limit refill_time_ms exceeds Firecracker's i64 range")?; | |
| let bandwidth_size = cfg | |
| .bandwidth_bytes_per_sec | |
| .checked_mul(cfg.refill_time_ms) | |
| .and_then(|value| value.checked_div(1000)) | |
| .and_then(|value| i64::try_from(value).ok()) | |
| .context("disk bandwidth rate limit is outside Firecracker's supported range")?; | |
| let mut bw = firecracker_client::models::TokenBucket::new(refill_time, bandwidth_size); |
| 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")?; | ||
| } |
There was a problem hiding this comment.
When rate limiting is disabled (or both limits are zero), this skips the PATCH entirely. A restored snapshot can already contain the user drive's previous rate limiter, so disabling the current configuration will leave that snapshotted limiter active. The resume path should always reconcile the drive with current configuration, including explicitly patching an empty/default RateLimiter to remove an inherited limiter (assuming Firecracker's empty limiter representation), rather than treating None as “do nothing.”
| # bandwidth_burst_bytes = 10485760 # 10 MB burst | ||
| # iops = 3000 | ||
| # iops_burst = 500 | ||
| # refill_time_ms = 1000 |
There was a problem hiding this comment.
As pointed out in the review, the current configuration exposes too much complexity to users. I'm considering dropping refill_time_ms and hardcoding it to 1000ms instead.
2 participants
Summary
[machine.disk_rate_limit]with bandwidth, IOPS, burst, and refill settingsadd_drive) and snapshot-resume (PATCH /drives) pathsCloses #46
Changes
config/default.toml: Add[machine.disk_rate_limit]section (disabled by default)src/cfg.rs: AddDiskRateLimitConfigstruct with config-rs derivesrc/sandbox/firecracker/config.rs: Plumb config intoFirecrackerSandboxConfigsrc/sandbox/firecracker/instance.rs: Addpatch_drive_rate_limiter()methodsrc/sandbox/firecracker/sandbox.rs: Build and apply rate limiter at boot and resumeTest plan
cargo build --release)cargo test