Skip to content

fix(image): avoid dangling-ref name collision on rmi -f for running images - #5127

Open
pujitha24 wants to merge 1 commit into
containerd:mainfrom
pujitha24:auto/issue-4109
Open

fix(image): avoid dangling-ref name collision on rmi -f for running images#5127
pujitha24 wants to merge 1 commit into
containerd:mainfrom
pujitha24:auto/issue-4109

Conversation

@pujitha24

Copy link
Copy Markdown
Contributor

Motivation:
nerdctl rmi -f on an image that is still used by a running container
keeps a dangling ref alive (renaming the image before deleting its
original name) so containerd does not garbage-collect the layers.
That dangling ref was always named with the literal string ":". Since
containerd's image store requires unique names, force-removing a
second running image's image in the same run made the second Create
fail with "image ":": already exists", aborting the whole command.

Approach:
Name the dangling ref after its content digest (":") instead
of the fixed ":" literal, so each kept-alive ref gets a distinct name.
The one remaining consumer that special-cased the exact ":" name
(pkg/imgutil filtering, used by --filter reference=... to skip
dangling images without erroring on their unparsable name) is updated
to match on the ":" prefix instead. Also tolerate AlreadyExists on the
Create call: if two different tags happen to share the same digest and
are both force-removed as running images in one invocation, the second
Create legitimately no-ops (the digest is already pinned).

Validation:
This is a name-collision defect in pkg/cmd/image/remove.go, reproducible
against containerd's real images.Store semantics without a live daemon.
Added pkg/cmd/image/remove_test.go, whose
TestDanglingImageNameIsUniquePerDigest exercises a fake images.Store
that enforces the same create-time uniqueness constraint containerd
does, and asserts the new per-digest naming avoids the collision.
I confirmed this test fails against the old behavior (temporarily
reverting the helper to return the literal ":") and passes against the
fix.

Ran:

  • go build ./... (and GOOS=linux, GOOS=windows) - all pass
  • go vet ./pkg/cmd/image/... ./pkg/imgutil/... - clean
  • gofmt -l on the changed files - no output
  • go test ./pkg/... - all pass, including the new test
  • go test ./pkg/cmd/image/... -run TestDanglingImageNameIsUniquePerDigest -v - PASS

This sandbox is macOS without containerd/runc installed, so the repo's
documented integration suite (go test -p 1 ./cmd/nerdctl/... against a
real containerd store, per docs/testing/README.md) could not be run;
confirmed those tests fail uniformly here (binary not found) regardless
of this change. The added unit test is a targeted regression test for
the digest-collision defect, not a full CLI-level reproduction.

Report: #4109
Signed-off-by: Pujitha Paladugu 10557236+pujitha24@users.noreply.github.com

Fixes #4109

… images

Motivation:
`nerdctl rmi -f` on an image that is still used by a running container
keeps a dangling ref alive (renaming the image before deleting its
original name) so containerd does not garbage-collect the layers.
That dangling ref was always named with the literal string ":". Since
containerd's image store requires unique names, force-removing a
second running image's image in the same run made the second `Create`
fail with "image \":\": already exists", aborting the whole command.

Approach:
Name the dangling ref after its content digest (":<digest>") instead
of the fixed ":" literal, so each kept-alive ref gets a distinct name.
The one remaining consumer that special-cased the exact ":" name
(pkg/imgutil filtering, used by `--filter reference=...` to skip
dangling images without erroring on their unparsable name) is updated
to match on the ":" prefix instead. Also tolerate AlreadyExists on the
Create call: if two different tags happen to share the same digest and
are both force-removed as running images in one invocation, the second
Create legitimately no-ops (the digest is already pinned).

Validation:
This is a name-collision defect in pkg/cmd/image/remove.go, reproducible
against containerd's real images.Store semantics without a live daemon.
Added pkg/cmd/image/remove_test.go, whose
TestDanglingImageNameIsUniquePerDigest exercises a fake images.Store
that enforces the same create-time uniqueness constraint containerd
does, and asserts the new per-digest naming avoids the collision.
I confirmed this test fails against the old behavior (temporarily
reverting the helper to return the literal ":") and passes against the
fix.

Ran:
- go build ./... (and GOOS=linux, GOOS=windows) - all pass
- go vet ./pkg/cmd/image/... ./pkg/imgutil/... - clean
- gofmt -l on the changed files - no output
- go test ./pkg/... - all pass, including the new test
- go test ./pkg/cmd/image/... -run TestDanglingImageNameIsUniquePerDigest -v - PASS

This sandbox is macOS without containerd/runc installed, so the repo's
documented integration suite (go test -p 1 ./cmd/nerdctl/... against a
real containerd store, per docs/testing/README.md) could not be run;
confirmed those tests fail uniformly here (binary not found) regardless
of this change. The added unit test is a targeted regression test for
the digest-collision defect, not a full CLI-level reproduction.

Report: containerd#4109
Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Removing multiple running images is broken

1 participant