feat(firecracker): implement AttachVolume, DetachVolume, ResizeInstance, and real TAP networking - #701
Merged
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 56 minutes and 44 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ 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 |
github-actions
Bot
added
enhancement
There was a problem hiding this comment.
Pull request overview
This PR closes remaining feature gaps in the Firecracker compute backend by adding cold (stop/recreate/start) support for volume attach/detach and instance resizing, and by switching network creation/deletion to real TAP devices via ip tuntap.
Changes:
- Implement cold
AttachVolume/DetachVolumeby rebuilding the Firecracker machine config and restarting the VM. - Implement cold
ResizeInstanceby updating vCPU/memory in the machine config and restarting the VM. - Implement real TAP-based
CreateNetwork/DeleteNetworkusingipcommands and deterministic MAC generation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+422
to
+425
| tapName := "fc-" + name[:8] | ||
| if len(tapName) > 14 { | ||
| tapName = tapName[:14] | ||
| } |
Comment on lines
+429
to
+432
| if err := exec.CommandContext(ctx, "ip", "tuntap", "add", "dev", tapName, "mode", "tap").Run(); err != nil { | ||
| a.logger.Warn("failed to create TAP device", "tap", tapName, "error", err) | ||
| return "", fmt.Errorf("failed to create TAP device: %w", err) | ||
| } |
| a.attachedVolumes[id] = append(a.attachedVolumes[id], volumePath) | ||
| a.mu.Unlock() | ||
|
|
||
| return "/dev/vdb", "", nil |
Comment on lines
+471
to
+483
| a.mu.Lock() | ||
| cfg, ok := a.machineConfigs[id] | ||
| m, okMachine := a.machines[id] | ||
| if !ok || !okMachine { | ||
| a.mu.Unlock() | ||
| return "", "", fmt.Errorf("instance %s not found", id) | ||
| } | ||
|
|
||
| // Stop VM gracefully | ||
| if err := m.Shutdown(ctx); err != nil { | ||
| a.mu.Unlock() | ||
| return "", "", fmt.Errorf("failed to stop VM for volume attach: %w", err) | ||
| } |
Comment on lines
+527
to
+539
| a.mu.Lock() | ||
| cfg, ok := a.machineConfigs[id] | ||
| m, okMachine := a.machines[id] | ||
| if !ok || !okMachine { | ||
| a.mu.Unlock() | ||
| return "", fmt.Errorf("instance %s not found", id) | ||
| } | ||
|
|
||
| // Stop VM gracefully | ||
| if err := m.Shutdown(ctx); err != nil { | ||
| a.mu.Unlock() | ||
| return "", fmt.Errorf("failed to stop VM for volume detach: %w", err) | ||
| } |
Comment on lines
+616
to
+620
| // Update machine config with new CPU and memory | ||
| newCfg := cfg | ||
| newCfg.MachineCfg.VcpuCount = firecracker.Int64(cpu) | ||
| newCfg.MachineCfg.MemSizeMib = firecracker.Int64(memory / 1024 / 1024) | ||
|
|
Comment on lines
+598
to
+600
| if a.cfg.MockMode { | ||
| return fmt.Errorf("resize not implemented in mock mode") | ||
| } |
Comment on lines
+602
to
+614
| a.mu.Lock() | ||
| cfg, ok := a.machineConfigs[id] | ||
| m, okMachine := a.machines[id] | ||
| if !ok || !okMachine { | ||
| a.mu.Unlock() | ||
| return fmt.Errorf("instance %s not found", id) | ||
| } | ||
|
|
||
| // Stop VM gracefully | ||
| if err := m.Shutdown(ctx); err != nil { | ||
| a.mu.Unlock() | ||
| return fmt.Errorf("failed to stop VM for resize: %w", err) | ||
| } |
Also implement real TAP device creation in CreateNetwork with MAC address. Wire generateMAC to CreateNetwork for proper TAP device configuration. Features implemented: - AttachVolume: stop VM → add drive to config → restart → track volume - DetachVolume: stop VM → remove drive from config → restart - ResizeInstance: stop VM → update CPU/memory → restart - CreateNetwork: real TAP device via ip tuntap with MAC from generateMAC - DeleteNetwork: real TAP device deletion via ip tuntap del Note: All VM-modifying operations cause brief downtime (cold restart). Requires root/CAP_NET_ADMIN for TAP device management.
The delete(a.instanceNetworks, id) line in DeleteInstance referenced a field that was never declared in the FirecrackerAdapter struct. This caused a compile error on Linux (GOOS=linux) while being invisible on macOS due to the //go:build linux build tag.
…ack, dead code - Fix TAP device name collision: use UUID instead of name truncation - Add rebuildFromConfig() helper for rollback on failed rebuilds - Apply rollback to AttachVolume, DetachVolume, ResizeInstance - Remove dead attachedVolumes map (written but never read) - Add unit tests for rebuild logic (RebuildSuccess, Rollback, NotFound)
- Format adapter.go with gofmt - Change Start mock from .Once() to .Maybe() since Start is called twice per test (once in LaunchInstanceWithOptions, once in rebuild)
Tests were asserting error messages that don't match actual code: - ResizeInstance MockMode: 'resize not supported' → 'resize not implemented in mock mode' - ResizeInstance RealMode NotFound: 'resize not supported' → 'not found' - AttachVolume RealMode NotFound: 'not implemented' → 'not found'
Test was setting newMachineFn to fail BEFORE launch, but launch also needs a working machine. Now properly: 1. Launch with working machine 2. Replace newMachineFn to fail 3. Call AttachVolume which fails on new machine creation
Use _ = to explicitly ignore error return values from cleanup exec calls in CreateNetwork error handlers (errcheck lint)
In CI, FIRECRACKER_MOCK_MODE=true so adapter returns 'not implemented' instead of 'not found'. Tests now accept either error message.
CreateNetwork/DeleteNetwork require root/CAP_NET_ADMIN privileges. Skip tests gracefully instead of failing when privileges are missing.
Extract CreateAndDeleteNetwork and DeleteNetwork_Twice into a separate TestFirecrackerBackend_E2E_Network function to reduce cyclomatic complexity of TestFirecrackerBackend_E2E below 30.
poyrazK
force-pushed
the
feature/firecracker-volume-resize-network
branch
from
June 13, 2026 13:42
1baf6d5 to
ffaaa6e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements remaining Firecracker gaps identified in PR #691:
Features Added
ip tuntap addwith MAC fromgenerateMAC()ip tuntap delTechnical Details
CAP_NET_ADMINGetInstanceIPstill returns0.0.0.0(would need instance→TAP mapping tracking)Execremains NotImplemented — Firecracker has no guest agent mechanismFiles Changed
internal/repositories/firecracker/adapter.go— main implementationTesting