Conversation
|
Packit jobs failed. @podman-container-tools/packit-jobs please check. |
|
@Honny1 this pr is the fix for the issue from podman ... i tagged it in the pr description... |
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
(I didn’t read this at all carefully.)
I think this approach is fairly elegant, but the cost (e.g. if zstd:chunked layers exist) is excessive.
Maybe it would work to, at the time of image removal, walk the top layers of that image until one finds a layer with >1 reference, and count sizes at that time. I have no idea what the complexity of such an approach would be.
Either way, we don’t (and currently can’t) hold storage locks between the individual removal calls, so the returned size might be inaccurate with concurrent image additions / removals. I don’t think that’s easily fixable.
ded8717 to
88d568d
Compare
|
updated the changes 👍🏼 |
| // proportional to the data that is actually released, and it accounts a layer | ||
| // shared by several removed images to the removal that releases it. | ||
| func (i *Image) freedSize() (int64, error) { | ||
| layers, err := i.runtime.store.DeleteImage(i.ID(), false) |
There was a problem hiding this comment.
The overall approach looks good, but the calculation can be inaccurate since images can be removed concurrently. I think DeleteImage could also report the freed size. Measuring the size of the staging TempDir before cleanup would give the exact bytes released. However, that would report the combined size of all removed layers rather than per-layer sizes.
Since DeleteImage is a public API, I'm not sure how much of its signature can change. Adding a DeleteImageWithSize variant might be an option.
There was a problem hiding this comment.
Agreed ... Staging happens under the lock, so once paths are renamed the measurement is exact. Combined-per-call is fine too. libimage calls DeleteImage once per image, which is the granularity FreedSize needs.
One catch that i wanted to say ... there are two staging dirs internalDelete stages only metadata (datadir, tspath)... the contents go through driver.DeferredRemove, which creates its own TempDir (vfs/driver.go:301, overlay/overlay.go:1448). Measuring only the first reports a few KB. The size needs accumulating in tempdir and summing across both cleanup functions
|
@Honny1 two things i wanted to clarify before i start to resolve the comments
|
|
@Honny1 Sorry to tag again sir but can you clarify the 2 doubts i commented ... as after that i can then proceed to solution else i have to assume mine assumption ... |
I would use the same metric.
I think yes. |
Signed-off-by: ROKUMATE <rohitkumawat0110@gmail.com>
88d568d to
f55970b
Compare
|
@Honny1 hey, i have updated the pr can you please review and check if its good to go ? thank you |
3 participants
Fixes: podman-container-tools/podman#27592.
report.Sizecarried the image's full size:i.Size()isstore.ImageSize(), which counts every layer including shared ones. podman sums those reports, so a layer shared by N pruned images was counted N times — users saw "reclaimed 346GB" on a 128GB disk.This reports the physical size the removal actually frees. A layer counts only if every image referencing it is being removed, and each freed layer is credited to exactly one report.
sum(report.Size)then equals the real disk delta, so podman'sPruneReportsSize()needs no change.Computing this needs the layer topology from before the removal, since the surviving images decide what is actually freed, so
RemoveImagessnapshots it and fills in the sizes once allimages have been processed instead of per image.Also fixed by the same change: the old size was computed before the untag logic ran, so an image that was only untagged still contributed its full size to "reclaimed".