-
Notifications
You must be signed in to change notification settings - Fork 219
Copy FC memory directly #1600
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Copy FC memory directly #1600
Conversation
|
@codex review |
|
@cursor review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request implements a major refactoring of Firecracker memory handling during snapshot creation, eliminating the temporary memfile approach in favor of direct memory copying via process_vm_readv syscall. The changes support an upgrade to Firecracker v1.12.2.
Key Changes:
- Replace temporary memfile-based snapshot creation with direct memory export via
process_vm_readv - Update Firecracker API to v1.12.2 with new memory-related endpoints (
/memoryand/memory/mappings) - Refactor memory backend interface from
Disable()toDirty()to better reflect the new workflow - Remove temporary memfile management infrastructure
Reviewed changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
packages/shared/pkg/storage/temporary_memfile.go |
Removed - temporary memfile infrastructure no longer needed |
packages/shared/pkg/fc/firecracker.yml |
Updated Firecracker API version from 1.7.0-dev to 1.12.2 |
packages/shared/pkg/fc/models/snapshot_create_params.go |
Made MemFilePath optional (no longer required) |
packages/shared/pkg/fc/models/snapshot_load_params.go |
Added NetworkOverrides field for network interface restoration |
packages/shared/pkg/fc/models/network_override.go |
New model for network device overrides during snapshot load |
packages/shared/pkg/fc/models/memory_response.go |
New model for memory info (resident/empty pages) |
packages/shared/pkg/fc/models/memory_mappings_response.go |
New model for memory region mappings |
packages/shared/pkg/fc/models/guest_memory_region_mapping.go |
New model for individual memory region details |
packages/shared/pkg/fc/models/cpu_config.go |
Changed from string type to structured object with CPU configuration fields |
packages/shared/pkg/fc/models/full_vm_configuration.go |
Added CPUConfig and Entropy device support |
packages/shared/pkg/fc/client/operations/operations_client.go |
Added GetMemory and GetMemoryMappings client methods |
packages/shared/pkg/fc/client/operations/get_memory_*.go |
New client implementation for memory API endpoints |
packages/orchestrator/internal/sandbox/uffd/userfaultfd/userfaultfd.go |
Refactored parameter order in handleMissing and removed Unregister method |
packages/orchestrator/internal/sandbox/uffd/userfaultfd/fd.go |
Renamed uffdFd to Fd, removed unregister method, adjusted parameter order in copy |
packages/orchestrator/internal/sandbox/uffd/userfaultfd/fd_helpers_test.go |
New test helpers extracted from production code |
packages/orchestrator/internal/sandbox/uffd/memory/region.go |
Added helper methods for region offset calculations and iteration |
packages/orchestrator/internal/sandbox/uffd/memory/mapping.go |
Added reverse mapping functions and FC model integration |
packages/orchestrator/internal/sandbox/uffd/memory/mapping_*_test.go |
Comprehensive test coverage for new mapping functionality |
packages/orchestrator/internal/sandbox/uffd/uffd.go |
Changed interface from Disable to Dirty method |
packages/orchestrator/internal/sandbox/uffd/noop.go |
Updated to match new Dirty interface |
packages/orchestrator/internal/sandbox/uffd/memory_backend.go |
Changed interface from Disable to Dirty |
packages/orchestrator/internal/sandbox/sandbox.go |
Integrated direct memory export, removed temporary memfile handling |
packages/orchestrator/internal/sandbox/fc/process.go |
Removed memfilePath parameter from CreateSnapshot |
packages/orchestrator/internal/sandbox/fc/memory.go |
New implementation for direct memory export via process_vm_readv |
packages/orchestrator/internal/sandbox/fc/client.go |
Added memory mapping and info API client methods |
packages/orchestrator/internal/sandbox/diffcreator.go |
Removed MemoryDiffCreator class |
packages/orchestrator/internal/sandbox/build/diff.go |
Added GenerateDiffCachePath helper function |
packages/orchestrator/internal/sandbox/build/local_diff.go |
Refactored to use Cache directly, added NewLocalDiffFromCache |
packages/orchestrator/internal/sandbox/build/storage_diff.go |
Updated to use centralized cache path generation |
packages/orchestrator/internal/sandbox/block/tracker.go |
Changed BitSet() to return internal reference without cloning (concurrency implications) |
packages/orchestrator/internal/sandbox/block/range.go |
New range abstraction for memory/block operations |
packages/orchestrator/internal/sandbox/block/cache.go |
Added helper methods for address access and metadata |
packages/orchestrator/go.mod |
Moved tklauser/go-sysconf from indirect to direct dependency |
packages/api/internal/cfg/model.go |
Updated default Firecracker version to v1.12.2_g1133bd6cd |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Note
Exports VM memory by reading Firecracker process memory using memory mappings, replacing tmp memfile snapshots and refactoring UFFD/diff pipeline with new API endpoints.
block.Cacheviaprocess_vm_readvusing newuffd/memory.Mappingandblock.Rangeutilities; addIOV_MAXhandling and extensive tests.fc.Process.CreateSnapshotno longer takesmemfilePath;Sandbox.Pausebuilds memfile diff from FC process memory;MemoryBackend.DiffMetadataadded;NoopMemoryderives dirty/empty via FC.DiffMetadata; adjust fd helpers and copy API; remove unregister usage.GET /memoryandGET /memory/mappingswith generated models (MemoryResponse,MemoryMappingsResponse,GuestMemoryRegionMapping) and client methods;fc.ProcessexposesMemoryInfo/ExportMemory.block.Cachesupports zero-size caches and exporting diffs when unmapped; addNewCacheFromProcessMemory, address helpers.GenerateDiffCachePath;localDiffwraps an existingblock.Cache; storage diff uses generator.vmlinux-6.1.158, FC tov1.12.1_717921c; update default FC versions; addgithub.1485827954.workers.dev/tklauser/go-sysconf.Written by Cursor Bugbot for commit 518e4ee. This will update automatically on new commits. Configure here.