From c400cc7ead181ca7ff42264b18ef4c27f9f0902e Mon Sep 17 00:00:00 2001 From: Nalin Dahyabhai Date: Mon, 8 May 2023 13:47:28 -0400 Subject: [PATCH] libpod/Container.rootFsSize(): use recorded image sizes In rootFsSize(), instead of calculating the size of the diff for every layer of the container's base image, ask the storage library for the sum of the values it recorded when it first wrote those layers. In a similar fashion, teach rwSize() to use the library's ContainerSize() method instead of trying to roll its own. Replace calls to pkg/util.SizeOfPath() with calls to github.com/containers/storage/pkg/directory.Size(), which does the same thing. Signed-off-by: Nalin Dahyabhai --- libpod/container_internal.go | 52 +++++++++------------------------- libpod/volume.go | 5 ++-- pkg/domain/infra/abi/system.go | 13 +++++---- pkg/util/utils.go | 17 +++-------- test/system/320-system-df.bats | 13 ++++++--- 5 files changed, 37 insertions(+), 63 deletions(-) diff --git a/libpod/container_internal.go b/libpod/container_internal.go index 9be8b9397e..f64117c6a9 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -55,10 +55,11 @@ const ( preCheckpointDir = "pre-checkpoint" ) -// rootFsSize gets the size of the container's root filesystem -// A container FS is split into two parts. The first is the top layer, a -// mutable layer, and the rest is the RootFS: the set of immutable layers -// that make up the image on which the container is based. +// rootFsSize gets the size of the container, which can be divided notionally +// into two parts. The first is the part of its size that can be directly +// attributed to its base image, if it has one. The second is the set of +// changes that the container has had made relative to that base image. Both +// parts include some ancillary data, and we count that, too. func (c *Container) rootFsSize() (int64, error) { if c.config.Rootfs != "" { return 0, nil @@ -72,58 +73,33 @@ func (c *Container) rootFsSize() (int64, error) { return 0, err } - // Ignore the size of the top layer. The top layer is a mutable RW layer - // and is not considered a part of the rootfs - rwLayer, err := c.runtime.store.Layer(container.LayerID) - if err != nil { - return 0, err - } - layer, err := c.runtime.store.Layer(rwLayer.Parent) - if err != nil { - return 0, err - } - size := int64(0) - for layer.Parent != "" { - layerSize, err := c.runtime.store.DiffSize(layer.Parent, layer.ID) - if err != nil { - return size, fmt.Errorf("getting diffsize of layer %q and its parent %q: %w", layer.ID, layer.Parent, err) - } - size += layerSize - layer, err = c.runtime.store.Layer(layer.Parent) + if container.ImageID != "" { + size, err = c.runtime.store.ImageSize(container.ImageID) if err != nil { return 0, err } } - // Get the size of the last layer. Has to be outside of the loop - // because the parent of the last layer is "", and lstore.Get("") - // will return an error. - layerSize, err := c.runtime.store.DiffSize(layer.Parent, layer.ID) + + layerSize, err := c.runtime.store.ContainerSize(c.ID()) + return size + layerSize, err } -// rwSize gets the size of the mutable top layer of the container. +// rwSize gets the combined size of the writeable layer and any ancillary data +// for a given container. func (c *Container) rwSize() (int64, error) { if c.config.Rootfs != "" { size, err := util.SizeOfPath(c.config.Rootfs) return int64(size), err } - container, err := c.runtime.store.Container(c.ID()) + layerSize, err := c.runtime.store.ContainerSize(c.ID()) if err != nil { return 0, err } - // The top layer of a container is - // the only readable/writeable layer, all others are immutable. - rwLayer, err := c.runtime.store.Layer(container.LayerID) - if err != nil { - return 0, err - } - - // Get the size of the top layer by calculating the size of the diff - // between the layer and its parent. - return c.runtime.store.DiffSize(rwLayer.Parent, rwLayer.ID) + return layerSize, nil } // bundlePath returns the path to the container's root filesystem - where the OCI spec will be diff --git a/libpod/volume.go b/libpod/volume.go index b2cd5c0438..9f08031548 100644 --- a/libpod/volume.go +++ b/libpod/volume.go @@ -6,7 +6,7 @@ import ( "github.com/containers/podman/v4/libpod/define" "github.com/containers/podman/v4/libpod/lock" "github.com/containers/podman/v4/libpod/plugin" - "github.com/containers/podman/v4/pkg/util" + "github.com/containers/storage/pkg/directory" ) // Volume is a libpod named volume. @@ -109,7 +109,8 @@ func (v *Volume) Name() string { // Returns the size on disk of volume func (v *Volume) Size() (uint64, error) { - return util.SizeOfPath(v.config.MountPoint) + size, err := directory.Size(v.config.MountPoint) + return uint64(size), err } // Driver retrieves the volume's driver. diff --git a/pkg/domain/infra/abi/system.go b/pkg/domain/infra/abi/system.go index 4a58b87e80..09cae3b3f3 100644 --- a/pkg/domain/infra/abi/system.go +++ b/pkg/domain/infra/abi/system.go @@ -18,6 +18,7 @@ import ( "github.com/containers/podman/v4/pkg/util" "github.com/containers/podman/v4/utils" "github.com/containers/storage" + "github.com/containers/storage/pkg/directory" "github.com/containers/storage/pkg/unshare" "github.com/sirupsen/logrus" "github.com/spf13/pflag" @@ -280,7 +281,7 @@ func (ic *ContainerEngine) SystemDf(ctx context.Context, options entities.System dfImages = append(dfImages, &report) } - // Get Containers and iterate them + // Get containers and iterate over them cons, err := ic.Libpod.GetAllContainers() if err != nil { return nil, err @@ -322,7 +323,7 @@ func (ic *ContainerEngine) SystemDf(ctx context.Context, options entities.System dfContainers = append(dfContainers, &report) } - // Get volumes and iterate them + // Get volumes and iterate over them vols, err := ic.Libpod.GetAllVolumes() if err != nil { return nil, err @@ -330,7 +331,7 @@ func (ic *ContainerEngine) SystemDf(ctx context.Context, options entities.System dfVolumes := make([]*entities.SystemDfVolumeReport, 0, len(vols)) for _, v := range vols { - var reclaimableSize uint64 + var reclaimableSize int64 mountPoint, err := v.MountPoint() if err != nil { return nil, err @@ -341,7 +342,7 @@ func (ic *ContainerEngine) SystemDf(ctx context.Context, options entities.System // TODO: fix this. continue } - volSize, err := util.SizeOfPath(mountPoint) + volSize, err := directory.Size(mountPoint) if err != nil { return nil, err } @@ -355,8 +356,8 @@ func (ic *ContainerEngine) SystemDf(ctx context.Context, options entities.System report := entities.SystemDfVolumeReport{ VolumeName: v.Name(), Links: len(inUse), - Size: int64(volSize), - ReclaimableSize: int64(reclaimableSize), + Size: volSize, + ReclaimableSize: reclaimableSize, } dfVolumes = append(dfVolumes, &report) } diff --git a/pkg/util/utils.go b/pkg/util/utils.go index 4aab28964c..16a91fbaf2 100644 --- a/pkg/util/utils.go +++ b/pkg/util/utils.go @@ -3,7 +3,6 @@ package util import ( "errors" "fmt" - "io/fs" "math" "os" "os/user" @@ -26,6 +25,7 @@ import ( "github.com/containers/podman/v4/pkg/namespaces" "github.com/containers/podman/v4/pkg/rootless" "github.com/containers/podman/v4/pkg/signal" + "github.com/containers/storage/pkg/directory" "github.com/containers/storage/pkg/idtools" stypes "github.com/containers/storage/types" securejoin "github.com/cyphar/filepath-securejoin" @@ -618,19 +618,10 @@ func LookupUser(name string) (*user.User, error) { // SizeOfPath determines the file usage of a given path. it was called volumeSize in v1 // and now is made to be generic and take a path instead of a libpod volume +// Deprecated: use github.com/containers/storage/pkg/directory.Size() instead. func SizeOfPath(path string) (uint64, error) { - var size uint64 - err := filepath.WalkDir(path, func(path string, d fs.DirEntry, err error) error { - if err == nil && !d.IsDir() { - info, err := d.Info() - if err != nil { - return err - } - size += uint64(info.Size()) - } - return err - }) - return size, err + size, err := directory.Size(path) + return uint64(size), err } // EncryptConfig translates encryptionKeys into a EncriptionsConfig structure diff --git a/test/system/320-system-df.bats b/test/system/320-system-df.bats index 52d8df0fd7..1084d25fcd 100644 --- a/test/system/320-system-df.bats +++ b/test/system/320-system-df.bats @@ -55,18 +55,23 @@ function teardown() { Type | Images | Containers | Local Volumes Total | 1 | 2 | 0 Active | 1 | 1 | 0 -RawSize | ~12...... | 0 | 0 -RawReclaimable | 0 | 0 | 0 +RawSize | ~12...... | !0 | 0 +RawReclaimable | 0 | !0 | 0 +Reclaimable | ~\(0%\) | ~\(50%\) | ~\(0%\) TotalCount | 1 | 2 | 0 -Size | ~12.*MB | 0B | 0B +Size | ~12.*MB | !0B | 0B ' while read -a fields; do for i in 0 1 2;do expect="${fields[$((i+1))]}" actual=$(jq -r ".[$i].${fields[0]}" <<<"$results") - # Do exact-match check, unless the expect term starts with ~ + # Do exact-match check, unless the expect term starts with ~ or ! op='=' + if [[ "$expect" =~ ^\! ]]; then + op='!=' + expect=${expect##\!} + fi if [[ "$expect" =~ ^~ ]]; then op='=~' expect=${expect##\~}