feat(lume): standardize OCI format — remove legacy types, rename aux to nvram - #1213
Merged
Conversation
Allow external tools (e.g. lumelet) to point lume at disk and NVRAM files stored outside the standard VM directory layout. When these flags are set, only config.json is required in the VM directory. This eliminates the need for file renaming/cloning when integrating lume with tools that maintain their own image cache.
…to nvram - Remove LegacyMediaType enum and annotationOrLegacy() backward-compat helper - Rename OCIMediaType.aux → .nvram (application/vnd.trycua.lume.nvram.v1) - Rename KubeletOCIConfig → LumeOCIConfig with fields hardwareModel/machineIdentifier - Rename KubeletStorageItem → LumeStorageItem - Rename createKubeletConfigData() → createOCIConfigData() - Update push/pull code: auxDigest/auxLayer → nvramDigest/nvramLayer - Replace all annotationOrLegacy() calls with direct annotation lookups BREAKING: Images pushed with old Agoda-era media types will no longer be recognized.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📦 Publishable packages changed
Add |
Contributor
📝 WalkthroughWalkthroughThe changes add support for overriding default disk and NVRAM paths when running virtual machines. Two new CLI options ( Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Command
participant Ctrl as LumeController
participant Loader as loadVM
participant VMCtx as VMDirContext
participant VM as VM Execution
CLI->>Ctrl: runVM(storage, diskPath?, nvramPath?)
alt diskPath or nvramPath overrides present
Ctrl->>Loader: loadVM(..., diskPath, nvramPath)
Loader->>VMCtx: Create with overrides<br/>(diskPathOverride, nvramPathOverride)
VMCtx->>VMCtx: diskPath = override ?? default<br/>nvramPath = override ?? default
else No overrides
Ctrl->>Loader: loadVM(...)
Loader->>VMCtx: Create with default paths
end
VMCtx->>VM: Pass resolved diskPath & nvramPath<br/>to VM execution
VM-->>VM: Start VM with custom or default paths
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
📦 Publishable packages changed
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/lume/src/ContainerRegistry/ImageContainerRegistry.swift (2)
4848-4860:⚠️ Potential issue | 🟠 MajorDon't write the compressed blob directly to
disk.img.
OCIMediaType.diskis emitted as gzip on the push path. Iforg.trycua.lume.content.uncompressed-sizeis missing, theelsebranch copies the compressed blob verbatim and reports success, leaving an unusable VM disk. This should fail withPullError.missingUncompressedSizeAnnotationinstead.Suggested fix
- let uncompSizeStr = singleDiskLayer.annotations?["org.trycua.lume.content.uncompressed-size"] - if let sizeStr = uncompSizeStr, let uncompSize = UInt64(sizeStr), uncompSize > 0 { - Logger.info("Decompressing disk image (gzip, sparse-aware)…") - FileManager.default.createFile(atPath: diskDest.path, contents: nil) - let diskHandle = try FileHandle(forWritingTo: diskDest) - try diskHandle.truncate(atOffset: uncompSize) - let _ = try gunzipChunkAndWriteSparse( - inputPath: blobDest, outputHandle: diskHandle, startOffset: 0) - try diskHandle.close() - Logger.info("Disk image decompressed (sparse)") - } else { - try FileManager.default.copyItem(at: blobDest, to: diskDest) - Logger.info("Saved disk.img (uncompressed)") - } + guard + let sizeStr = singleDiskLayer.annotations?["org.trycua.lume.content.uncompressed-size"], + let uncompSize = UInt64(sizeStr), + uncompSize > 0 + else { + throw PullError.missingUncompressedSizeAnnotation + } + Logger.info("Decompressing disk image (gzip, sparse-aware)…") + FileManager.default.createFile(atPath: diskDest.path, contents: nil) + let diskHandle = try FileHandle(forWritingTo: diskDest) + try diskHandle.truncate(atOffset: uncompSize) + let _ = try gunzipChunkAndWriteSparse( + inputPath: blobDest, outputHandle: diskHandle, startOffset: 0) + try diskHandle.close() + Logger.info("Disk image decompressed (sparse)")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/lume/src/ContainerRegistry/ImageContainerRegistry.swift` around lines 4848 - 4860, In the disk-layer handling in ImageContainerRegistry (the block that reads org.trycua.lume.content.uncompressed-size and calls gunzipChunkAndWriteSparse), do not copy the compressed blob to disk when the uncompressed-size annotation is missing; instead throw PullError.missingUncompressedSizeAnnotation; specifically, replace the else branch that calls FileManager.default.copyItem(at:to:) and logs "Saved disk.img (uncompressed)" with code that raises PullError.missingUncompressedSizeAnnotation (preserving any surrounding context/cleanup such as closing handles if needed) so compressed gzip blobs are not treated as valid VM disks.
4680-4718:⚠️ Potential issue | 🟠 MajorDon't silently drop
config.jsonon OCI decode failures.
try?turns any schema mismatch or corrupted config blob intovmConfig == nil, and the pull then completes without writingconfig.json. With the field rename in this PR, that makes format errors surface much later than their root cause.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/lume/src/ContainerRegistry/ImageContainerRegistry.swift` around lines 4680 - 4718, The decode of LumeOCIConfig currently uses try? which swallows decoding errors and leaves vmConfig nil; change it to explicitly decode inside a do-catch so decoding failures are surfaced (and logged or propagated) instead of silently dropping config.json. Locate the JSONDecoder().decode(LumeOCIConfig.self, from: data) call (using configData and the ociCfg variable) and replace the try? with a do { let ociCfg = try JSONDecoder().decode(...) ... } catch { /* log the decoding error with details and either rethrow or return a failure so the pull aborts */ } flow so VMConfig(...) is only constructed when decoding succeeds and errors include the decoding context (e.g., LumeOCIConfig, manifest digest).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/lume/src/ContainerRegistry/ImageContainerRegistry.swift`:
- Around line 4659-4673: The code currently treats the nvram layer as optional
and proceeds when manifest.layers lacks OCIMediaType.nvram; instead, in the
image pull path inside ImageContainerRegistry (the block that sets nvramBlobPath
and calls downloadLayer), detect absence of a layer with mediaType ==
OCIMediaType.nvram and fail the pull immediately by throwing or returning an
appropriate error (e.g., a descriptive PullError or throw a NSError) rather than
continuing; ensure the error mentions the missing nvram layer and reference the
manifest, nvramLayer, and nvramBlobPath variables so callers can surface the
failure early.
- Around line 4263-4265: The current code silently falls back to a synthetic
VMConfig when decoding fails, which allows OCI pushes with missing boot-critical
fields; change the behavior in the push path that uses VMConfig and
createOCIConfigData so that if JSONDecoder().decode(VMConfig.self, from:
Data(contentsOf: configPath)) returns nil or throws, the push fails with a clear
error rather than using fallbackConfig. Specifically, remove the fallbackConfig
usage around VMConfig and ensure the function (e.g., pushOCI or the caller that
builds configData) returns/throws a descriptive error referencing configPath and
VMConfig when decoding/reading config.json fails before calling
createOCIConfigData.
In `@libs/lume/src/LumeController.swift`:
- Around line 1386-1393: loadVM currently skips vmDir.initialized() when
diskPath or nvramPath overrides are provided, but it doesn't validate that the
resulting effective disk/NVRAM files actually exist; update loadVM to compute
the effective disk and nvram paths (use the provided diskPath/nvramPath if
non-nil, otherwise derive from vmDir) and perform existence checks (e.g.,
Path/FileManager existence APIs) for each required file, and if a required file
is missing throw a clear VMError (or VMError.notInitialized(vmDir.name) with a
descriptive message) before proceeding; reference loadVM, vmDir.initialized(),
vmDir, diskPath, nvramPath and VMError when implementing the checks.
- Around line 993-997: The code currently silences VM-resolution errors by using
`let actualLocationName = try? validateVMExists(normalizedName, storage:
storage)` which allows `actualLocationName` to be nil and causes
`getVMDirectory(..., storage: nil)` to resolve the wrong location; change this
to use a real throw (`let actualLocationName = try
validateVMExists(normalizedName, storage: storage)`) so resolution errors
propagate instead of being suppressed, then pass that non-optional
`actualLocationName` into `home.getVMDirectory` and assign `effectiveStorage =
actualLocationName`; if you need to handle specific errors, wrap the `try` in a
`do { ... } catch { ... }` and only recover in explicitly intended cases rather
than using `try?`.
---
Outside diff comments:
In `@libs/lume/src/ContainerRegistry/ImageContainerRegistry.swift`:
- Around line 4848-4860: In the disk-layer handling in ImageContainerRegistry
(the block that reads org.trycua.lume.content.uncompressed-size and calls
gunzipChunkAndWriteSparse), do not copy the compressed blob to disk when the
uncompressed-size annotation is missing; instead throw
PullError.missingUncompressedSizeAnnotation; specifically, replace the else
branch that calls FileManager.default.copyItem(at:to:) and logs "Saved disk.img
(uncompressed)" with code that raises
PullError.missingUncompressedSizeAnnotation (preserving any surrounding
context/cleanup such as closing handles if needed) so compressed gzip blobs are
not treated as valid VM disks.
- Around line 4680-4718: The decode of LumeOCIConfig currently uses try? which
swallows decoding errors and leaves vmConfig nil; change it to explicitly decode
inside a do-catch so decoding failures are surfaced (and logged or propagated)
instead of silently dropping config.json. Locate the
JSONDecoder().decode(LumeOCIConfig.self, from: data) call (using configData and
the ociCfg variable) and replace the try? with a do { let ociCfg = try
JSONDecoder().decode(...) ... } catch { /* log the decoding error with details
and either rethrow or return a failure so the pull aborts */ } flow so
VMConfig(...) is only constructed when decoding succeeds and errors include the
decoding context (e.g., LumeOCIConfig, manifest digest).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a3297253-fa08-4ea1-86b7-cf9e71ece3c7
📒 Files selected for processing (6)
libs/lume/src/Commands/Run.swiftlibs/lume/src/ContainerRegistry/ImageContainerRegistry.swiftlibs/lume/src/LumeController.swiftlibs/lume/src/Server/Handlers.swiftlibs/lume/src/Server/Requests.swiftlibs/lume/src/VM/VM.swift
Comment on lines
4263
to
+4265
| let vmConfig = try? JSONDecoder().decode(VMConfig.self, from: Data(contentsOf: configPath)) | ||
| let fallbackConfig = try VMConfig(os: "darwin", display: "1920x1080") | ||
| let configData = try createKubeletConfigData(vmConfig ?? fallbackConfig) | ||
| let configData = try createOCIConfigData(vmConfig ?? fallbackConfig) |
Contributor
There was a problem hiding this comment.
Require a real config.json for OCI pushes.
pushOCI now serializes boot-critical hardwareModel and machineIdentifier into the image contract. Falling back to a synthetic VMConfig publishes empty/default values, so the push succeeds but the pulled image may not boot.
Suggested fix
- let vmConfig = try? JSONDecoder().decode(VMConfig.self, from: Data(contentsOf: configPath))
- let fallbackConfig = try VMConfig(os: "darwin", display: "1920x1080")
- let configData = try createOCIConfigData(vmConfig ?? fallbackConfig)
+ guard FileManager.default.fileExists(atPath: configPath.path) else {
+ throw ValidationError("config.json is required when pushing OCI images")
+ }
+ let vmConfig = try JSONDecoder().decode(VMConfig.self, from: Data(contentsOf: configPath))
+ let configData = try createOCIConfigData(vmConfig)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let vmConfig = try? JSONDecoder().decode(VMConfig.self, from: Data(contentsOf: configPath)) | |
| let fallbackConfig = try VMConfig(os: "darwin", display: "1920x1080") | |
| let configData = try createKubeletConfigData(vmConfig ?? fallbackConfig) | |
| let configData = try createOCIConfigData(vmConfig ?? fallbackConfig) | |
| guard FileManager.default.fileExists(atPath: configPath.path) else { | |
| throw ValidationError("config.json is required when pushing OCI images") | |
| } | |
| let vmConfig = try JSONDecoder().decode(VMConfig.self, from: Data(contentsOf: configPath)) | |
| let configData = try createOCIConfigData(vmConfig) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/lume/src/ContainerRegistry/ImageContainerRegistry.swift` around lines
4263 - 4265, The current code silently falls back to a synthetic VMConfig when
decoding fails, which allows OCI pushes with missing boot-critical fields;
change the behavior in the push path that uses VMConfig and createOCIConfigData
so that if JSONDecoder().decode(VMConfig.self, from: Data(contentsOf:
configPath)) returns nil or throws, the push fails with a clear error rather
than using fallbackConfig. Specifically, remove the fallbackConfig usage around
VMConfig and ensure the function (e.g., pushOCI or the caller that builds
configData) returns/throws a descriptive error referencing configPath and
VMConfig when decoding/reading config.json fails before calling
createOCIConfigData.
Comment on lines
+4659
to
+4673
| var nvramBlobPath: URL? | ||
| if let nvramLayer = manifest.layers.first(where: { $0.mediaType == OCIMediaType.nvram }) { | ||
| let blobDest = tempDir.appendingPathComponent( | ||
| auxLayer.digest.replacingOccurrences(of: ":", with: "_")) | ||
| Logger.info("Downloading aux layer (\(auxLayer.digest.prefix(19))…)") | ||
| nvramLayer.digest.replacingOccurrences(of: ":", with: "_")) | ||
| Logger.info("Downloading nvram layer (\(nvramLayer.digest.prefix(19))…)") | ||
| try await downloadLayer( | ||
| repository: repository, | ||
| digest: auxLayer.digest, | ||
| mediaType: auxLayer.mediaType, | ||
| digest: nvramLayer.digest, | ||
| mediaType: nvramLayer.mediaType, | ||
| token: token, | ||
| to: blobDest, | ||
| maxRetries: 5, | ||
| progress: downloadProgress | ||
| ) | ||
| auxBlobPath = blobDest | ||
| nvramBlobPath = blobDest |
Contributor
There was a problem hiding this comment.
Fail the pull when the required nvram layer is absent.
After removing the legacy aux fallback, a manifest without application/vnd.trycua.lume.nvram.v1 should be rejected here. The current optional path just produces a VM directory without nvram.bin and defers the failure to runtime.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/lume/src/ContainerRegistry/ImageContainerRegistry.swift` around lines
4659 - 4673, The code currently treats the nvram layer as optional and proceeds
when manifest.layers lacks OCIMediaType.nvram; instead, in the image pull path
inside ImageContainerRegistry (the block that sets nvramBlobPath and calls
downloadLayer), detect absence of a layer with mediaType == OCIMediaType.nvram
and fail the pull immediately by throwing or returning an appropriate error
(e.g., a descriptive PullError or throw a NSError) rather than continuing;
ensure the error mentions the missing nvram layer and reference the manifest,
nvramLayer, and nvramBlobPath variables so callers can surface the failure
early.
Comment on lines
+993
to
+997
| if hasPathOverrides { | ||
| // With overrides, try to find the VM but relax initialized check | ||
| let actualLocationName = try? validateVMExists(normalizedName, storage: storage) | ||
| vmDir = try home.getVMDirectory(normalizedName, storage: actualLocationName) | ||
| effectiveStorage = actualLocationName |
Contributor
There was a problem hiding this comment.
Do not suppress VM resolution errors with try? here.
On Line 995, try? validateVMExists(...) drops actionable errors (e.g., explicit location not found), and Line 996 then resolves with storage: nil, which can change lookup behavior and target the wrong location.
Suggested fix
- if hasPathOverrides {
- // With overrides, try to find the VM but relax initialized check
- let actualLocationName = try? validateVMExists(normalizedName, storage: storage)
- vmDir = try home.getVMDirectory(normalizedName, storage: actualLocationName)
- effectiveStorage = actualLocationName
+ if hasPathOverrides {
+ // Preserve explicit storage intent and avoid silently widening lookup scope.
+ if let requestedStorage = storage {
+ vmDir = try home.getVMDirectory(normalizedName, storage: requestedStorage)
+ guard vmDir.configPath.exists() else {
+ throw VMError.notFound(normalizedName)
+ }
+ effectiveStorage = requestedStorage
+ } else {
+ let candidates = try home.getAllVMDirectories().filter {
+ $0.directory.name == normalizedName && $0.directory.configPath.exists()
+ }
+ guard let match = candidates.first else {
+ throw VMError.notFound(normalizedName)
+ }
+ vmDir = match.directory
+ effectiveStorage = match.locationName
+ }
} else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/lume/src/LumeController.swift` around lines 993 - 997, The code
currently silences VM-resolution errors by using `let actualLocationName = try?
validateVMExists(normalizedName, storage: storage)` which allows
`actualLocationName` to be nil and causes `getVMDirectory(..., storage: nil)` to
resolve the wrong location; change this to use a real throw (`let
actualLocationName = try validateVMExists(normalizedName, storage: storage)`) so
resolution errors propagate instead of being suppressed, then pass that
non-optional `actualLocationName` into `home.getVMDirectory` and assign
`effectiveStorage = actualLocationName`; if you need to handle specific errors,
wrap the `try` in a `do { ... } catch { ... }` and only recover in explicitly
intended cases rather than using `try?`.
Comment on lines
+1386
to
1393
| private func loadVM(vmDir: VMDirectory, storage: String?, diskPath: Path? = nil, nvramPath: Path? = nil) throws -> VM { | ||
| // With path overrides, only config.json is required (not full initialized check) | ||
| let hasPathOverrides = diskPath != nil || nvramPath != nil | ||
| if !hasPathOverrides { | ||
| guard vmDir.initialized() else { | ||
| throw VMError.notInitialized(vmDir.name) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Validate effective disk/NVRAM paths when overrides are enabled.
Line 1389 bypasses initialized() for any override, but there is no preflight check that the effective disk/NVRAM files exist (especially when only one override is passed). This can defer failures to runtime with unclear errors.
Suggested fix
private func loadVM(vmDir: VMDirectory, storage: String?, diskPath: Path? = nil, nvramPath: Path? = nil) throws -> VM {
// With path overrides, only config.json is required (not full initialized check)
let hasPathOverrides = diskPath != nil || nvramPath != nil
if !hasPathOverrides {
guard vmDir.initialized() else {
throw VMError.notInitialized(vmDir.name)
}
+ } else {
+ guard vmDir.configPath.exists() else {
+ throw VMError.notFound(vmDir.name)
+ }
+
+ let effectiveDiskPath = diskPath ?? vmDir.diskPath
+ let effectiveNvramPath = nvramPath ?? vmDir.nvramPath
+
+ guard effectiveDiskPath.exists() else {
+ throw ValidationError("Disk image not found: \(effectiveDiskPath.path)")
+ }
+ guard effectiveNvramPath.exists() else {
+ throw ValidationError("NVRAM image not found: \(effectiveNvramPath.path)")
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/lume/src/LumeController.swift` around lines 1386 - 1393, loadVM
currently skips vmDir.initialized() when diskPath or nvramPath overrides are
provided, but it doesn't validate that the resulting effective disk/NVRAM files
actually exist; update loadVM to compute the effective disk and nvram paths (use
the provided diskPath/nvramPath if non-nil, otherwise derive from vmDir) and
perform existence checks (e.g., Path/FileManager existence APIs) for each
required file, and if a required file is missing throw a clear VMError (or
VMError.notInitialized(vmDir.name) with a descriptive message) before
proceeding; reference loadVM, vmDir.initialized(), vmDir, diskPath, nvramPath
and VMError when implementing the checks.
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
1 participant
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
LegacyMediaTypeenum,LegacyAnnotationenum, andannotationOrLegacy()backward-compat helperOCIMediaType.aux→.nvram(application/vnd.trycua.lume.nvram.v1)KubeletOCIConfig→LumeOCIConfigwith fieldshardwareModel/machineIdentifier(washardwareModelData/machineIdData)KubeletStorageItem→LumeStorageItemauxDigest/auxLayer→nvramDigest/nvramLayerBreaking Change
Images pushed with old Agoda-era media types (
application/vnd.agoda.macosvz.*) will no longer be recognized.macos-tahoe-vanilla:latesthas already been re-pushed with the new format.Test plan
macos-tahoe-vanilla:latestwith new conventionsnvram.v1media type andnvram.bintitlehardwareModel/machineIdentifierfieldsSummary by CodeRabbit
New Features
--disk-pathand--nvram-pathCLI options to override default VM disk and NVRAM file locations.Improvements