feat: support stargz snapshotter for devbox with lvm powerd#29
feat: support stargz snapshotter for devbox with lvm powerd#29lingdie wants to merge 2 commits intolabring:v1.7from
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for treating the stargz snapshotter as participating in the devbox “writable-layer” flow (labels/pinning/snapshot updates), and centralizes the snapshotter eligibility checks so the logic can be reused across CRI server paths.
Changes:
- Introduce helpers to detect snapshotters that should participate in the devbox writable-layer flow (including
stargz). - Propagate devbox-related labels/opts in container create paths (server + sbserver) and extend pin-label logic in image pull.
- Add/extend Linux unit tests covering devbox/stargz snapshot label behavior.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/cri/server/image_pull.go | Pins repoTag images when using a devbox-writable snapshotter (now includes stargz). |
| pkg/cri/server/helpers.go | Clarifies that DevboxSnapshotter refers to the “classic” devbox snapshotter. |
| pkg/cri/server/events.go | Uses the new helper to decide whether to update devbox snapshot labels on exit. |
| pkg/cri/server/devbox_snapshotter.go | Adds isDevboxWritableSnapshotter helper (devbox + stargz). |
| pkg/cri/server/container_stop.go | Extends devbox snapshot update to stargz via the helper. |
| pkg/cri/server/container_create_other.go | Adds non-Linux stub for devboxSnapshotterOpts. |
| pkg/cri/server/container_create_linux.go | Preserves sandbox annotations as snapshot labels for devbox flow; minor label key cleanup. |
| pkg/cri/server/container_create.go | Uses a cached runtime snapshotter name and applies devbox snapshot opts when eligible. |
| pkg/cri/server/container_create_linux_test.go | Adds tests for devboxSnapshotterOpts. |
| pkg/cri/sbserver/devbox_snapshotter.go | Adds helper to determine when sbserver needs devbox labels (stargz). |
| pkg/cri/sbserver/container_create_other.go | Updates sbserver snapshotter opts signature to accept sandbox config. |
| pkg/cri/sbserver/container_create_linux.go | Adds label propagation for stargz via LabelsFromAnnotations. |
| pkg/cri/sbserver/container_create.go | Passes sandbox config to snapshotter opts and uses cached runtime snapshotter. |
| pkg/cri/sbserver/container_create_linux_test.go | Adds tests ensuring only relevant devbox annotations become labels for stargz. |
| pkg/cri/internal/devboxsnapshotter/labels.go | Introduces constants + helper to extract devbox labels from annotations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Preserve the original devbox annotations here and let | ||
| // containerd.WithNewSnapshot translate them for snapshotters that consume | ||
| // containerd.io/snapshot/devbox-* labels. | ||
| labels := make(map[string]string) | ||
| maps.Copy(labels, config.Annotations) | ||
| return snapshots.WithLabels(labels), nil |
There was a problem hiding this comment.
devboxSnapshotterOpts currently copies all sandbox annotations into snapshot labels (maps.Copy(labels, config.Annotations)). Only devbox.sealos.io/* keys are meant for the devbox writable-layer flow (and containerd.WithNewSnapshot only translates those), so this can unintentionally propagate unrelated annotations into snapshot metadata. Filter to just the devbox annotations (e.g., via devboxsnapshotter.LabelsFromAnnotations or by prefix) before returning snapshots.WithLabels.
| name: "stargz keeps original annotations", | ||
| snapshotter: devboxsnapshotter.StargzSnapshotter, | ||
| annotations: map[string]string{ | ||
| devboxsnapshotter.SealosDevboxContentIDAnnotation: "workspace-1", | ||
| devboxsnapshotter.SealosDevboxStorageLimitAnnotation: "20Gi", | ||
| }, |
There was a problem hiding this comment.
TestDevboxSnapshotterOpts only asserts a round-trip when the input annotations map contains only devbox keys, so it won’t catch accidental propagation of unrelated sandbox annotations into snapshot labels. Add a non-devbox annotation (e.g. "other.annotation") to one case and assert it is not present in info.Labels to cover the intended filtering behavior.
| @@ -448,7 +448,7 @@ func handleContainerExit(ctx context.Context, e *eventtypes.TaskExit, cntr conta | |||
| return status, err | |||
| } | |||
| fmt.Println("Container snapshotter:", container.Snapshotter, "ID:", cntr.Container.ID()) | |||
There was a problem hiding this comment.
fmt.Println in the event handler writes directly to stdout on every container exit, which can create noisy logs and bypass the configured logger. Replace this with structured logging (e.g. logrus.WithFields(...).Debug(...) / log.G(ctx)), or remove it if it was only for debugging.
| fmt.Println("Container snapshotter:", container.Snapshotter, "ID:", cntr.Container.ID()) | |
| logrus.WithFields(logrus.Fields{ | |
| "snapshotter": container.Snapshotter, | |
| "containerID": cntr.Container.ID(), | |
| }).Debug("Container snapshotter information on exit") |
| // Grab any platform specific snapshotter opts. | ||
| sOpts := snapshotterOpts(c.config.ContainerdConfig.Snapshotter, config) | ||
| runtimeSnapshotter := c.runtimeSnapshotter(ctx, ociRuntime) | ||
| sOpts := snapshotterOpts(runtimeSnapshotter, config, sandboxConfig) | ||
|
|
There was a problem hiding this comment.
snapshotterOpts is now called with sandboxConfig, but the Windows implementation (pkg/cri/sbserver/container_create_windows.go) still has the old 2-arg signature. This will break Windows builds (compile-time mismatch). Update the Windows snapshotterOpts signature to accept sandboxConfig *runtime.PodSandboxConfig (unused) and adjust any callers accordingly.
No description provided.