Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

store.DeleteImage: correctly account for mapped layers #1062

Merged
merged 1 commit into from
Nov 19, 2021

Conversation

nalind
Copy link
Member

@nalind nalind commented Nov 17, 2021

When walking the list of layers in an image to decide which of them to remove as part of removing that image, we check that each candidate layer is a) not being used as the top layer for any other images, and b) that the only layer that lists a candidate layer as its parent layer is either the top layer of the image we're removing, or the previous layer that we decided should be removed.

When the image has multiple top layers because some of them are alternate top layers which use different ID mappings, that second check didn't account for those mapped layers as being layers that shouldn't prevent us from removing a candidate layer.

Fixes containers/podman#12316.

@rhatdan
Copy link
Member

rhatdan commented Nov 17, 2021

LGTM
@giuseppe @flouthoc PTAL

@flouthoc
Copy link
Collaborator

@nalind Plz correct me if i am wrong.

Actual logic change is in only hasOtherRefs which was before accounting for layers as a positive reference although they were already marked as to be removed reason was we were only keeping recently used layer for check i.e in lastRemoved.

But now its changed to map layersToRemoveMap keeping and validating against all the layers which we removed so hasOtherRef doesn't returns with a false positive.

Change LGTM

@flouthoc
Copy link
Collaborator

Not related to this PR: This needs a bit of re-phrasing/re-wording comment here is a bit confusing to read at first https://github.com/containers/storage/blob/main/store.go#L242 but could me only me.

@giuseppe
Copy link
Member

LGTM, as @flouthoc pointed out, could we move the change in cmd/containers-storage/main.go to a separate commit?

@nalind
Copy link
Member Author

nalind commented Nov 19, 2021

Dropped the bit from main.go.

When walking the list of layers in an image to decide which of them to
remove as part of removing that image, we check that each candidate
layer is a) not being used as the top layer for any other images, and b)
that the only layer that lists a candidate layer as its parent layer is
either the top layer of the image we're removing, or the previous layer
that we decided should be removed.

When the image has multiple top layers because some of them are
alternate top layers which use different ID mappings, that second check
didn't account for those mapped layers as being layers that shouldn't
prevent us from removing a candidate layer.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
@rhatdan rhatdan merged commit b9cea70 into containers:main Nov 19, 2021
@rhatdan
Copy link
Member

rhatdan commented Nov 19, 2021

Going to force merge, to ignore devicemapper flake.

@nalind nalind deleted the mapped-layers-deletion branch November 19, 2021 16:30
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.

Dangling layers left behind after deleting image that was started with --uidmap
4 participants