-
Notifications
You must be signed in to change notification settings - Fork 202
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
DiskUsage: return total images size #1194
Conversation
It turns out only counting the layers size is not sufficient for `podman systemd df` as it excludes the size of the manifests, configs and potentially attached meta data in the storage. Instead, sum the image sizes but distract redundant layers. That indeed gives the expected result and does not yield negative results for reclaimable space. Remove the unrelease LayersDiskUsage function again to avoid expensive recalculation of the layer tree. We are still below 1.0, and I am convinced the total image size belongs into DiskUsage. NOTE: The DiskUsage function does not have test coverage in libimage. This should be addressed at some point but in the interest of time I leverage podman's system tests. Signed-off-by: Valentin Rothberg <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flouthoc, vrothberg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@@ -775,6 +775,7 @@ func (i *Image) Unmount(force bool) error { | |||
|
|||
// Size computes the size of the image layers and associated data. | |||
func (i *Image) Size() (int64, error) { | |||
// TODO: cache the result to optimize performance of subsequent calls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementing cache for this rather looks complex at first glance, if i gauge this right.
- Store
image
size in boltdb. - Ensure to update the boltdb on every
pull
orrmi
- Ensure function is locked and waits for any parallel
pull
orrmi
to complete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each Image
object has an internal cache (see https://github.com/vrothberg/common/blob/baebe0edc671bd3f2e8e18513551df126d025186/libimage/image.go#L38). That's where I want to store it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vrothberg oh this is so cool !!
/lgtm |
It turns out only counting the layers size is not sufficient for
podman systemd df
as it excludes the size of the manifests, configs and potentially attached meta data in the storage.Instead, sum the image sizes but distract redundant layers. That indeed gives the expected result and does not yield negative results for reclaimable space.
Remove the unrelease LayersDiskUsage function again to avoid expensive recalculation of the layer tree. We are still below 1.0, and I am convinced the total image size belongs into DiskUsage.
NOTE: The DiskUsage function does not have test coverage in libimage.
This should be addressed at some point but in the interest of
time I leverage podman's system tests.
Signed-off-by: Valentin Rothberg [email protected]
@rhatdan @giuseppe @flouthoc PTAL
Spinning up a PR in Podman to fix containers/podman#16135 revealed an issue that this PR addresses.