feat(lume): OCI-compliant image format with multi-layer and single-layer support#1144
Open
ddupont808 wants to merge 14 commits intomainfrom
Open
feat(lume): OCI-compliant image format with multi-layer and single-layer support#1144ddupont808 wants to merge 14 commits intomainfrom
ddupont808 wants to merge 14 commits intomainfrom
Conversation
Add --single-layer flag for kubelet-compatible single disk layer pushes (no chunking), alongside the default multi-layer chunked OCI format. Pull now uses sparse-aware decompression for single-layer images. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Convert as Convert Command
participant Controller as LumeController
participant Registry as ImageRegistry
participant TelemetryClient
Client->>Convert: execute Convert command<br/>(sourceImage, targetImage)
Convert->>TelemetryClient: log start (source, target, registry)
rect rgba(100, 200, 100, 0.5)
Note over Convert,Controller: Step 1: Pull Legacy Image
Convert->>Controller: pullImage(sourceImage → tempVM)
Controller->>Registry: retrieve legacy image
Registry-->>Controller: legacy image loaded to temp VM
end
rect rgba(100, 150, 200, 0.5)
Note over Convert,Controller: Step 2: Push as OCI
Convert->>Controller: pushImage(tempVM, targetImage,<br/>legacy: false, tags, registry, org)
Controller->>Registry: push(vmDir, imageName, tags,<br/>legacy: false, singleLayer: false)
Registry->>Registry: OCI-compliant push flow
Registry-->>Controller: push complete
Controller-->>Convert: success
end
rect rgba(200, 100, 100, 0.5)
Note over Convert,Controller: Step 3: Cleanup
Convert->>Controller: deleteVM(tempVM)
Controller->>Controller: remove temporary VM
end
Convert->>TelemetryClient: log completion
Convert-->>Client: conversion success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
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 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
Add |
![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 (1)
libs/lume/src/Server/Requests.swift (1)
145-159:⚠️ Potential issue | 🟠 MajorExpose
legacyinPushRequestto keep API parity with CLI/controller.The API contract cannot carry legacy mode, so HTTP clients cannot request compatibility pushes even though
LumeController.pushImagesupportslegacy(and CLI exposes--legacy).Proposed fix
struct PushRequest: Codable { @@ var chunkSizeMb: Int // Chunk size var singleLayer: Bool // Push as single disk layer (kubelet-compatible) + var legacy: Bool // Use legacy Lume LZ4-chunked format @@ enum CodingKeys: String, CodingKey { - case name, imageName, tags, registry, organization, storage, chunkSizeMb, singleLayer + case name, imageName, tags, registry, organization, storage, chunkSizeMb, singleLayer, legacy } @@ chunkSizeMb = try container.decodeIfPresent(Int.self, forKey: .chunkSizeMb) ?? 512 singleLayer = try container.decodeIfPresent(Bool.self, forKey: .singleLayer) ?? false + legacy = try container.decodeIfPresent(Bool.self, forKey: .legacy) ?? false } }--- a/libs/lume/src/Server/Handlers.swift +++ b/libs/lume/src/Server/Handlers.swift @@ reassemble: false, // Default API behavior is likely non-reassemble - singleLayer: request.singleLayer + singleLayer: request.singleLayer, + legacy: request.legacy )Also applies to: 171-171
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/lume/src/Server/Requests.swift` around lines 145 - 159, Add a Boolean legacy field to the PushRequest Codable struct so API clients can request legacy-mode pushes: update struct PushRequest to include var legacy: Bool (default false if omitted by decoder) and add it to the CodingKeys; ensure any other similar request struct (the second PushRequest-like definition referenced at lines ~171) is updated the same way and that LumeController.pushImage reads this new field to pass through legacy mode. Make sure the decoder will accept missing legacy by providing a default false when decoding (or mark it optional) to maintain backward compatibility.
🧹 Nitpick comments (4)
libs/lume/src/Commands/Push.swift (1)
39-43: Add a guard for incompatible--single-layer+--legacyusage.Both flags can currently be set together, which creates ambiguous push mode semantics at the CLI boundary. Fail fast before invoking the controller.
Proposed fix
`@MainActor` func run() async throws { // Record telemetry TelemetryClient.shared.record(event: TelemetryEvent.push) + guard !(singleLayer && legacy) else { + throw ValidationError("--single-layer and --legacy cannot be used together") + } + let controller = LumeController()Also applies to: 80-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/lume/src/Commands/Push.swift` around lines 39 - 43, Add a pre-flight validation in the Push command's entry (e.g., in run() or validate() for the Push command in Push.swift) that checks for the mutually incompatible flags singleLayer and legacy; if both are true, fail fast by throwing an ArgumentParser.ValidationError (or otherwise exiting with a clear error) before calling the controller or any downstream logic; update the same check in the other relevant entry point mentioned (the duplicate at lines ~80-83) so the CLI consistently rejects the --single-layer + --legacy combination.libs/lume/src/ContainerRegistry/ImageContainerRegistry.swift (3)
3884-3886: Semantically incorrect error type for decompression failure.
PullError.layerDownloadFailedis thrown when gunzip fails, but this is a decompression error, not a download error. This could make debugging harder.Consider adding a dedicated error case like
.decompressionFailed(String)toPushErrororPullError, or reuse an existing appropriate error type.🤖 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 3884 - 3886, The current guard in ImageContainerRegistry.swift that checks process.terminationStatus and throws PullError.layerDownloadFailed(...) on non‑zero exit is using the wrong error semantic for a decompression failure; add a new PullError case like .decompressionFailed(String) (or reuse an existing decompression error type) and replace the throw in the decompression/gunzip block (the guard that references process.terminationStatus and inputPath.lastPathComponent) to throw the new .decompressionFailed with a useful message (and propagate/handle this new case where PullError is matched so callers see a correct decompression error).
4144-4146: Semantically incorrect error type for missing file.
PushError.fileCreationFailedis thrown whennvram.bindoesn't exist, but this error type suggests a creation operation failed. The actual issue is that a required file is missing.Consider adding a dedicated error case like
.missingRequiredFile(String)to improve error clarity.Example fix
guard FileManager.default.fileExists(atPath: nvramPath.path) else { - throw PushError.fileCreationFailed(nvramPath.path) + throw PushError.missingRequiredFile(nvramPath.path) }(Same pattern applies at Line 4163 for
disk.img)🤖 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 4144 - 4146, The guard that checks FileManager.default.fileExists(atPath: nvramPath.path) is throwing PushError.fileCreationFailed which is semantically wrong for a missing required file; add a new PushError case such as .missingRequiredFile(String) and update the checks that validate nvramPath and the later disk.img check (the second occurrence around the disk.img path) to throw .missingRequiredFile(path) instead of .fileCreationFailed so the error communicates a missing file rather than a failed creation.
3847-3848: Unchecked file creation return value.
FileManager.default.createFilereturns aBoolindicating success. If creation fails (e.g., permissions issue), the subsequentFileHandle(forWritingTo:)will throw, but the error won't clearly indicate that file creation failed.Proposed fix
- FileManager.default.createFile(atPath: outputPath.path, contents: nil) - let outputHandle = try FileHandle(forWritingTo: outputPath) + guard FileManager.default.createFile(atPath: outputPath.path, contents: nil) else { + throw PushError.fileCreationFailed(outputPath.path) + } + let outputHandle = try FileHandle(forWritingTo: outputPath)🤖 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 3847 - 3848, The code calls FileManager.default.createFile(atPath: outputPath.path, contents: nil) and then blindly opens a FileHandle(forWritingTo: outputPath); check the Bool return from createFile and handle failure explicitly before creating outputHandle — if createFile returns false, throw or log a descriptive error (including outputPath) so failures due to permissions or creation errors are surfaced instead of relying on the FileHandle initializer to throw. Ensure the change applies around the createFile/outputHandle usage (outputPath, outputHandle, FileHandle(forWritingTo:)).
🤖 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/Commands/Convert.swift`:
- Around line 66-74: The pullImage step can leave temp VM artifacts if it fails;
wrap the controller.pullImage(...) call (the Step 1/3 block that uses tempVMName
and controller.pullImage) in a do/catch or add a defer so that the same temp-VM
cleanup routine used in the push catch block is invoked on failure—ensure you
call the identical cleanup function/method that removes tempVMName (the routine
currently used in the push catch) whenever pullImage throws so partial local
state is always removed.
In `@libs/lume/src/ContainerRegistry/GCSImageRegistry.swift`:
- Around line 116-119: The method on GCSImageRegistry that accepts parameters
reassemble:, singleLayer:, and legacy: currently ignores singleLayer and legacy;
update that method (the one with signature including reassemble: Bool,
singleLayer: Bool, legacy: Bool) to either implement the requested push modes or
explicitly reject them: if you don't support single-layer or legacy push,
validate singleLayer and legacy at the start and throw a clear, typed error
(e.g., UnsupportedPushModeError) mentioning which flag(s) were requested; if you
add support, branch on singleLayer/legacy and implement the corresponding upload
flow in GCSImageRegistry so callers' flags are honored.
In `@libs/lume/src/ContainerRegistry/ImageContainerRegistry.swift`:
- Around line 4426-4435: The code force-unwraps group.next() which can crash if
the group is unexpectedly empty; change the block that runs when enqueued >=
maxConcurrent to safely unwrap the async group result (call try await
group.next() without "!") and handle the nil case explicitly (e.g., log an error
via Logger.error and throw or continue) before using
completedIdx/completedPath/completedOffset; update the code paths in the scope
that reference enqueued, maxConcurrent, and the result of group.next() to use
the safely unwrapped values and ensure resources (FileHandle, temp file) are
still handled in the nil/error branch.
- Around line 3905-3916: The InputFilter using .zlib for decompression (in the
InputFilter(.decompress, using: .zlib) closure that reads from
sourceData/sourceReadOffset) is incorrect for gzip streams; replace this with a
gzip-capable decompressor. Options: a) switch to a gzip-aware zlib inflate by
using inflateInit2(windowBits: 16 + MAX_WBITS) on the data read from sourceData
(wrap the inflate-based decompression in the same InputFilter callback), b) use
a library that supports gzip (e.g., GzipSwift or SWCompression) to decompress
chunks read from sourceData inside the InputFilter callback, or c) manually
parse/strip the gzip header/footer from sourceData and then feed the raw DEFLATE
payload to .zlib. Update the InputFilter initialization in
ImageContainerRegistry (the closure that reads chunks from sourceData advancing
sourceReadOffset) to perform one of these gzip-capable approaches so
gzip-compressed disk chunks are correctly decompressed.
---
Outside diff comments:
In `@libs/lume/src/Server/Requests.swift`:
- Around line 145-159: Add a Boolean legacy field to the PushRequest Codable
struct so API clients can request legacy-mode pushes: update struct PushRequest
to include var legacy: Bool (default false if omitted by decoder) and add it to
the CodingKeys; ensure any other similar request struct (the second
PushRequest-like definition referenced at lines ~171) is updated the same way
and that LumeController.pushImage reads this new field to pass through legacy
mode. Make sure the decoder will accept missing legacy by providing a default
false when decoding (or mark it optional) to maintain backward compatibility.
---
Nitpick comments:
In `@libs/lume/src/Commands/Push.swift`:
- Around line 39-43: Add a pre-flight validation in the Push command's entry
(e.g., in run() or validate() for the Push command in Push.swift) that checks
for the mutually incompatible flags singleLayer and legacy; if both are true,
fail fast by throwing an ArgumentParser.ValidationError (or otherwise exiting
with a clear error) before calling the controller or any downstream logic;
update the same check in the other relevant entry point mentioned (the duplicate
at lines ~80-83) so the CLI consistently rejects the --single-layer + --legacy
combination.
In `@libs/lume/src/ContainerRegistry/ImageContainerRegistry.swift`:
- Around line 3884-3886: The current guard in ImageContainerRegistry.swift that
checks process.terminationStatus and throws PullError.layerDownloadFailed(...)
on non‑zero exit is using the wrong error semantic for a decompression failure;
add a new PullError case like .decompressionFailed(String) (or reuse an existing
decompression error type) and replace the throw in the decompression/gunzip
block (the guard that references process.terminationStatus and
inputPath.lastPathComponent) to throw the new .decompressionFailed with a useful
message (and propagate/handle this new case where PullError is matched so
callers see a correct decompression error).
- Around line 4144-4146: The guard that checks
FileManager.default.fileExists(atPath: nvramPath.path) is throwing
PushError.fileCreationFailed which is semantically wrong for a missing required
file; add a new PushError case such as .missingRequiredFile(String) and update
the checks that validate nvramPath and the later disk.img check (the second
occurrence around the disk.img path) to throw .missingRequiredFile(path) instead
of .fileCreationFailed so the error communicates a missing file rather than a
failed creation.
- Around line 3847-3848: The code calls FileManager.default.createFile(atPath:
outputPath.path, contents: nil) and then blindly opens a
FileHandle(forWritingTo: outputPath); check the Bool return from createFile and
handle failure explicitly before creating outputHandle — if createFile returns
false, throw or log a descriptive error (including outputPath) so failures due
to permissions or creation errors are surfaced instead of relying on the
FileHandle initializer to throw. Ensure the change applies around the
createFile/outputHandle usage (outputPath, outputHandle,
FileHandle(forWritingTo:)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 434efe55-0383-4535-b7b0-d23b1cc09b27
📒 Files selected for processing (10)
libs/lume/src/Commands/Convert.swiftlibs/lume/src/Commands/Push.swiftlibs/lume/src/ContainerRegistry/GCSImageRegistry.swiftlibs/lume/src/ContainerRegistry/ImageContainerRegistry.swiftlibs/lume/src/ContainerRegistry/ImageRegistry.swiftlibs/lume/src/LumeController.swiftlibs/lume/src/Server/Handlers.swiftlibs/lume/src/Server/Requests.swiftlibs/lume/src/Utils/CommandRegistry.swiftskills/gui-automation/SKILL.md
…safe unwrap - Replace Compression framework .zlib (raw DEFLATE only) with /usr/bin/gunzip for gzip-compressed disk chunks in sparse-aware decompression - Validate singleLayer/legacy flags in GCSImageRegistry and throw clear error - Wrap pullImage in Convert command with do/catch for temp VM cleanup on failure - Replace force-unwrap of group.next() with safe unwrap in chunked download Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📦 Publishable packages changed
Add |
…ploads - Only show active layers in progress display (not all waiting ones), with compact summary line showing done/waiting/speed/time - Add log level filtering to Logger (LUME_LOG_LEVEL env var, default: info) - Downgrade chatty cache, auth, blob, and VM setup logs to debug Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📦 Publishable packages changed
Add |
Contributor
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
Contributor
|
✅ Created PR with unit tests: #1145 |
- Increase blob upload timeout from 10min to 1h with dedicated URLSession (fixes timeouts on slow connections with large chunks) - Add --single-layer flag to lume convert command Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📦 Publishable packages changed
Add |
- Replace fake 50% progress with real byte-level upload tracking using URLSessionTaskDelegate.didSendBodyData callbacks - File-based upload (upload(for:fromFile:)) instead of loading into memory - Progress bars now show actual upload % for both chunked and single-layer - Add progress display to single-layer push path Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📦 Publishable packages changed
Add |
- Replace /usr/bin/gzip Process with streaming in-process zlib deflate (windowBits=MAX_WBITS+16 for gzip format) via CZlib system library - Single-pass: computes both SHA256 digests during compression (was 3 passes) - Real compression progress bars for both chunked and single-layer push - Summary line shows ETA instead of elapsed time during active work Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📦 Publishable packages changed
Add |
…gger - Downgrade pull/push/convert/cache/auth logs from INFO to DEBUG - Wire --verbose flag to Logger.setVerbose() in push, pull, convert - Add --verbose flag to pull command (was missing) - Logger.minLevel is now mutable, set via --verbose or LUME_LOG_LEVEL env Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📦 Publishable packages changed
Add |
- 4 parallel compress workers feeding 4 parallel upload workers via AsyncStream - Content-addressable dedup: duplicate chunks skip upload, hidden from progress - Fix exclusivity crash in markExists/markDone (read totalBytes before write) - Fix per-layer byte tracking (was single global counter overwritten by parallel chunks) - Smoothed ETA with hr/m/s format, labeled (compressing)/(uploading) - Speeds shown after ETA, no waiting count in summary - Hide 100% compressing layers (transitioning to upload/dedup) - Downgrade verbose push/pull/convert logs to DEBUG - Rename "disk chunk" → "disk layer" Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📦 Publishable packages changed
Add |
- Docker-style output: digest[:12] for upload/done, aggregated disk.img compress line - Done layers show "Push/Pull complete" (deduped by digest, first 10...last 10) - Compressed layers waiting for upload show "Waiting" with digest - Upload ETA uses compression ratio extrapolation for accurate total estimate - Fix AsyncStream buffer: unbounded instead of bufferingNewest (was dropping chunks) - Fix per-layer byte tracking for parallel uploads - Cap output to terminal height via TIOCGWINSZ to prevent infinite scrolling - Downgrade verbose push/pull/convert logs to DEBUG Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📦 Publishable packages changed
Add |
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
--legacyflag: Falls back to the old Lume LZ4-chunked format for backwards compatibility--single-layerflag: Kubelet-compatible single disk layer push (no chunking) for macos-vz-kubelet deploymentslume convertcommand: Migrate existing legacy Lume images to the new OCI-compliant formatTest plan
--single-layerand verify single disk layer manifest--legacyand verify old LZ4 format still workslume convertto migrate a legacy image to OCI formatsingleLayer: truein PushRequest🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
singleLayerflag to Push command for single disk layer pusheslegacyflag to Push command for legacy LZ4-chunked format optionDocumentation