fix(core): honor resources.disk_gb on sandbox create#2
Conversation
- thread DiskGB through CreateParams and the create handler so the requested writable-overlay size is no longer silently dropped - map disk_gb (GiB) to the SDK's WithOCIUpperSize (MiB) with unit conversion; only applied when > 0 so the image default still wins - extract the CreateParams -> SandboxOption mapping into a pure buildCreateOptions helper and cover it with unit tests - clarify the disk_gb description in openapi.yaml to match behavior Fixes #1
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
disk_gb end-to-end wiring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/core/service.go`:
- Around line 190-193: Validate p.DiskGB in the service path before calling
msb.WithOCIUpperSize: reject negative values explicitly and add an upper-bound
check to prevent uint32 overflow when converting GiB to MiB. Update the logic in
the disk size handling block to fail fast for invalid disk_gb inputs instead of
silently skipping them or wrapping the converted size, and keep the existing
conversion only for values safely within range.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3582705f-1e96-400c-a48a-7f31795ed985
📒 Files selected for processing (4)
internal/api/handlers.gointernal/core/service.gointernal/core/service_test.goopenapi.yaml
- reject negative disk_gb instead of silently skipping it - add an upper bound (maxDiskGB) so the GiB->MiB conversion can't overflow the uint32 WithOCIUpperSize takes - return an error from buildCreateOptions and propagate it from Create so invalid input fails fast rather than booting a misconfigured box - cover the negative, overflow, and max in-range cases with unit tests
Description
POST /v1/sandboxesacceptedresources.disk_gb(it was inopenapi.yamlandResourcesDTO), but the value was silently dropped — onlycpuandmemory_mbwere ever applied, so every sandbox got the image's default writable-overlay size. A client following the OpenAPI contract could request more scratch space and get a default-sized overlay with no error or warning.This wires
disk_gbthrough to the SDK'sWithOCIUpperSizeoption, converting the API unit (GiB) to the SDK unit (MiB). The option is only applied whendisk_gb > 0, so omitting it still falls back to the image default.Verified live against
/dev/kvm: creating withdisk_gb: 32now reportsupper_size_mib: 32768in/inspect(was4096), and omitting it keeps the4096default.Changes
internal/core/service.go— addDiskGBtoCreateParams; map it tomsb.WithOCIUpperSize(uint32(p.DiskGB)*1024)when> 0. Extract theCreateParams → []msb.SandboxOptionmapping into a pure, side-effect-freebuildCreateOptionshelper so it's unit-testable without booting a microVM.internal/api/handlers.go— passreq.Resources.DiskGBintocore.CreateParams.internal/core/service_test.go— new regression tests for the GiB→MiB conversion, the unset/default case, and the combined CPU/memory/disk mapping.openapi.yaml— clarify thedisk_gbdescription to match actual behavior (writable overlay upper size in GiB; zero = image default).Testing
go test -race ./...— passgolangci-lint run ./...— 0 issuesdisk_gb: 32→upper_size_mib: 32768; nodisk_gb→4096. MicroVM integration paths aren't exercised in CI (no/dev/kvm).Wire contract
Additive / backward-compatible. No DTO shape change —
ResourcesDTO.disk_gbalready existed in bothdto.goandopenapi.yaml; this only makes the previously-ignored field functional.dto.goandopenapi.yamlremain in lockstep.Fixes #1
Summary by CodeRabbit
New Features
Bug Fixes
0continues to use the image default.Documentation