Skip to content

c8d/prune: Keep the last tagged image instead of creating dangling image#48076

Merged
thaJeztah merged 3 commits intomoby:masterfrom
vvoland:c8d-prune-keeplastref
Sep 12, 2024
Merged

c8d/prune: Keep the last tagged image instead of creating dangling image#48076
thaJeztah merged 3 commits intomoby:masterfrom
vvoland:c8d-prune-keeplastref

Conversation

@vvoland
Copy link
Contributor

@vvoland vvoland commented Jun 27, 2024

Don't turn images into dangling when they are used by containers created with an image specified by an ID only (e.g. docker run 82d1e9d).

Keep the last image reference with the same target when all other references would be pruned.

If the container was created with a digested and tagged reference (e.g. docker run alpine:latest@sha256:82d1e9d7ed48a7523bdebc18cf6290bdb97b82302a8a9c27d4fe885949ea94d1), the alpine:latest image won't get untagged.

This change makes the behavior consistent with the graphdriver implementation.

- What I did

- How I did it

- How to verify it
TestPruneDontDeleteUsedImage

- Description for the changelog

containerd integration: Fix `docker image prune -a` untagging images used by containers started from images referenced by a digested reference.

- A picture of a cute animal (not mandatory but encouraged)

@vvoland vvoland added area/images Image Distribution kind/bugfix PR's that fix bugs containerd-integration Issues and PRs related to containerd integration labels Jun 27, 2024
@vvoland vvoland added this to the 28.0.0 milestone Jun 27, 2024
@vvoland vvoland self-assigned this Jun 27, 2024
@vvoland vvoland force-pushed the c8d-prune-keeplastref branch from 1ac742d to 783325f Compare June 27, 2024 14:17
@vvoland

This comment was marked as resolved.

@vvoland vvoland force-pushed the c8d-prune-keeplastref branch 2 times, most recently from de6286b to 78fb7ab Compare June 27, 2024 18:19
@vvoland vvoland changed the title c8d/prune: Keep the tagged image instead of dangling image c8d/prune: Keep the last tagged image instead of creating dangling image Jun 27, 2024
@thaJeztah thaJeztah requested a review from dmcgowan June 28, 2024 21:38
@vvoland vvoland force-pushed the c8d-prune-keeplastref branch 3 times, most recently from fce4d86 to f0a608e Compare September 5, 2024 13:42
@vvoland vvoland requested a review from thaJeztah September 6, 2024 12:44
@laurazard
Copy link
Member

I was testing this on graphdrivers vs containerd, and noticed on graphdrivers, we seem to always remove the last tag alphanumerically, but with the containerd implementation it seems a bit random.

I wrote a repro here:

#!/usr/bin/env bash
set -ex
while true; do
    docker image prune -af
    docker pull busybox:latest
    repoDigest=$(docker inspect busybox:latest | jq .[0].RepoDigests[0] | tr -d '"')
    docker tag busybox:latest busybox:a
    docker tag busybox:latest busybox:b
    docker tag busybox:latest busybox:c
    docker rmi busybox:latest
    ctrId=$(docker run -d $repoDigest sleep 100)
    yes | docker image prune -a
    tags=$(docker images --filter=reference='busy*' --format json | jq .Tag | tr -d '"')
    tagsLength=${#tags[@]}
    if [[ $tagsLength -ne 1 ]]; then
        echo "this shouldn't happen! tags length != 1"
        exit 1
    fi
    if [[ $tags != c ]]; then
        exit 1
    fi
    docker kill $ctrId
    docker rm $ctrId
done

@laurazard
Copy link
Member

laurazard commented Sep 9, 2024

I don't know know whether we must keep this behavior, but if it doesn't impact performance to sort alphanumerically we should 😅.

Don't turn images into dangling when they are used by containers created
with an image specified by an ID only (e.g. `docker run 82d1e9d`).

Keep the last image reference with the same target when all other
references would be pruned.

If the container was created with a digested and tagged reference (e.g.
`docker run alpine:latest@sha256:82d1e9d7ed48a7523bdebc18cf6290bdb97b82302a8a9c27d4fe885949ea94d1`),
the `alpine:latest` image won't get untagged.

This change makes the behavior consistent with the graphdriver
implementation.

Signed-off-by: Paweł Gronowski <[email protected]>
Signed-off-by: Paweł Gronowski <[email protected]>
@vvoland vvoland force-pushed the c8d-prune-keeplastref branch 2 times, most recently from b331db1 to ec50f0d Compare September 11, 2024 12:54
@vvoland vvoland force-pushed the c8d-prune-keeplastref branch from ec50f0d to da49d28 Compare September 11, 2024 17:55
When untagging multiple images targetting the same digest, delete the
images in lexographic order to be consistent with graphdrivers.

Signed-off-by: Paweł Gronowski <[email protected]>
@vvoland vvoland force-pushed the c8d-prune-keeplastref branch from da49d28 to fefa98a Compare September 11, 2024 17:58
Copy link
Member

@laurazard laurazard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Comment on lines +113 to +115
for name := range imagesToPrune {
sorted = append(sorted, name)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably start looking at the maps package, and for these kind of things, use maps.Keys()

@thaJeztah thaJeztah merged commit a484c7c into moby:master Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/images Image Distribution containerd-integration Issues and PRs related to containerd integration kind/bugfix PR's that fix bugs process/cherry-picked status/2-code-review

Projects

Development

Successfully merging this pull request may close these issues.

containerd integration: Prune deletes images used by containers created from a digested reference

3 participants