games-on-whales/LXC2Docker

Fork 0

mirror of https://github.com/games-on-whales/LXC2Docker.git synced 2026-10-15 12:52:04 +00:00

[PR #148] [MERGED] fix(images/containers): real config-digest image IDs + StartedAt on every start path #148

New issue

Closed

opened 2026-10-15 12:52:22 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-15 12:52:22 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/games-on-whales/LXC2Docker/pull/148
Author: @JBailes
Created: 7/25/2026
Status: ✅ Merged
Merged: 7/25/2026
Merged by: @JBailes

Base: main ← Head: worktree-image-id-parity

📝 Commits (2)

f2785c3 fix(images): content-address pulled images with the real config digest
250c882 fix(containers): stamp StartedAt on every start path, not just the API handler

📊 Changes

5 files changed (+222 additions, -14 deletions)

View changed files

📝 internal/api/containers.go (+5 -8)
📝 internal/lxc/manager.go (+30 -0)
➕ internal/lxc/started_at_test.go (+82 -0)
➕ internal/oci/config_digest_test.go (+93 -0)
📝 internal/oci/pull.go (+12 -6)

📄 Description

Two docker-parity gaps reported from a live deploy, both confirmed against the code and fixed here.

1. Pulled images weren't content-addressed

imageDisplayID already prefers a record's ConfigDigest (the sha256 of the OCI image config — stock Docker's image ID) and only falls back to the tag-derived oci_<safe-ref> pseudo-ID. Build (build.go) and commit (portainer_ops.go) both compute one; pull never did. So docker images showed IDs like sha256:oci_ghcr_io_org_app_testing, and two different images pushed to the same tag collapsed onto one ID.

The value was already being read and thrown away: parseImageConfig parses manifest.Config.Digest to locate the config blob, then dropped it. It's now surfaced on oci.ImageConfig and recorded in pullOCI — in both the store record and the oci-meta.json sidecar, so a restored record keeps it.

This is image identity, orthogonal to refresh: a moved tag is detected by comparing the registry manifest digest (RepoDigest), which PullImage already does.

2. `StartedAt` was stamped only by the API start handler

Every other start path calls Manager.StartContainer directly — the restart-policy watcher, the startup reconcile, and POST /restart — and none stamped it. A policy-started container reported 0001-01-01T00:00:00Z from docker inspect while plainly running, and docker restart never refreshed it.

The functional tail matters more than the cosmetic one: the watcher branches on StartedAt == nil and continues before its exited check, so a container the watcher itself started stayed permanently in the "never started" branch and never got restart-policy handling on later crashes — an always container would come up once, then stay dead after its next exit.

The stamp moves into StartContainer, the choke point all start paths share — the same reasoning the existing ensurePortForwards comment there documents for an identical past gap. StoppedByUser deliberately stays in the API handler: it records user intent, so a policy convergence must not silently un-stop a hand-stopped container.

Not changed

A third report — that moved tags don't refresh on recreate — did not reproduce as a missing feature. Two layers already exist: PullImage re-checks the registry manifest digest for mutable tags and drops the stale template, and reusableContainer refuses a warm rootfs on digest drift (#88, #126, #128). Worth noting stock Docker doesn't re-resolve a locally-present tag on create/run either — that's what --pull=always is for — so "pull + pin before materialise" is correct practice on both. A stale blob served after an explicit pull would point at a deployed binary predating those commits.

Testing

go build and go vet clean. New unit tests in internal/oci/config_digest_test.go (digest captured; distinct content yields distinct IDs) and internal/lxc/started_at_test.go (stamped on first start, refreshed on restart with FinishedAt cleared, no-op on a concurrently-removed record).

Full suite green except TestCheckCreateDiskPressure, which fails identically on unmodified main on hosts where t.TempDir() lands on tmpfs — checkCreateDiskPressure deliberately skips tmpfs sources. Environmental, pre-existing, unrelated to this PR; CI is unaffected.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/games-on-whales/LXC2Docker/pull/148 **Author:** [@JBailes](https://github.com/JBailes) **Created:** 7/25/2026 **Status:** ✅ Merged **Merged:** 7/25/2026 **Merged by:** [@JBailes](https://github.com/JBailes) **Base:** `main` ← **Head:** `worktree-image-id-parity` --- ### 📝 Commits (2) - [`f2785c3`](https://github.com/games-on-whales/LXC2Docker/commit/f2785c3baf76cb1d4d61c3242a87522825ca50ed) fix(images): content-address pulled images with the real config digest - [`250c882`](https://github.com/games-on-whales/LXC2Docker/commit/250c882c416800cebf42ebe0bc03b2b5a2e7a3fe) fix(containers): stamp StartedAt on every start path, not just the API handler ### 📊 Changes **5 files changed** (+222 additions, -14 deletions) <details> <summary>View changed files</summary> 📝 `internal/api/containers.go` (+5 -8) 📝 `internal/lxc/manager.go` (+30 -0) ➕ `internal/lxc/started_at_test.go` (+82 -0) ➕ `internal/oci/config_digest_test.go` (+93 -0) 📝 `internal/oci/pull.go` (+12 -6) </details> ### 📄 Description Two docker-parity gaps reported from a live deploy, both confirmed against the code and fixed here. ## 1. Pulled images weren't content-addressed `imageDisplayID` already prefers a record's `ConfigDigest` (the sha256 of the OCI image config — stock Docker's image ID) and only falls back to the tag-derived `oci_<safe-ref>` pseudo-ID. Build (`build.go`) and commit (`portainer_ops.go`) both compute one; **pull never did**. So `docker images` showed IDs like `sha256:oci_ghcr_io_org_app_testing`, and two different images pushed to the same tag collapsed onto one ID. The value was already being read and thrown away: `parseImageConfig` parses `manifest.Config.Digest` to locate the config blob, then dropped it. It's now surfaced on `oci.ImageConfig` and recorded in `pullOCI` — in both the store record and the `oci-meta.json` sidecar, so a restored record keeps it. This is image *identity*, orthogonal to *refresh*: a moved tag is detected by comparing the registry **manifest** digest (`RepoDigest`), which `PullImage` already does. ## 2. `StartedAt` was stamped only by the API start handler Every other start path calls `Manager.StartContainer` directly — the restart-policy watcher, the startup reconcile, and `POST /restart` — and none stamped it. A policy-started container reported `0001-01-01T00:00:00Z` from `docker inspect` while plainly running, and `docker restart` never refreshed it. The functional tail matters more than the cosmetic one: the watcher branches on `StartedAt == nil` and `continue`s *before* its exited check, so a container the watcher itself started stayed permanently in the "never started" branch and **never got restart-policy handling on later crashes** — an `always` container would come up once, then stay dead after its next exit. The stamp moves into `StartContainer`, the choke point all start paths share — the same reasoning the existing `ensurePortForwards` comment there documents for an identical past gap. `StoppedByUser` deliberately stays in the API handler: it records *user* intent, so a policy convergence must not silently un-stop a hand-stopped container. ## Not changed A third report — that moved tags don't refresh on recreate — did **not** reproduce as a missing feature. Two layers already exist: `PullImage` re-checks the registry manifest digest for mutable tags and drops the stale template, and `reusableContainer` refuses a warm rootfs on digest drift (#88, #126, #128). Worth noting stock Docker doesn't re-resolve a locally-present tag on `create`/`run` either — that's what `--pull=always` is for — so "pull + pin before materialise" is correct practice on both. A stale blob served *after* an explicit pull would point at a deployed binary predating those commits. ## Testing `go build` and `go vet` clean. New unit tests in `internal/oci/config_digest_test.go` (digest captured; distinct content yields distinct IDs) and `internal/lxc/started_at_test.go` (stamped on first start, refreshed on restart with `FinishedAt` cleared, no-op on a concurrently-removed record). Full suite green except `TestCheckCreateDiskPressure`, which fails **identically on unmodified main** on hosts where `t.TempDir()` lands on tmpfs — `checkCreateDiskPressure` deliberately skips tmpfs sources. Environmental, pre-existing, unrelated to this PR; CI is unaffected. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-15 12:52:22 +00:00

closed this issue
added the
pull-request
label

No labels

pull-request

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

games-on-whales/LXC2Docker#148

No description provided.

Rows
Columns