Merge pull request #25506 from Luap99/disk-usage

Fix system df negative reclaimable size bug

docs/source/markdown/podman-system-df.1.md

@@ -7,7 +7,11 @@ podman\-system\-df - Show podman disk usage
 **podman system df** [*options*]
 
 ## DESCRIPTION
-Show podman disk usage
+Show podman disk usage for images, containers and volumes.
+
+Note: The RECLAIMABLE size that is reported for images can be incorrect. It might
+report that it can reclaim more than a prune would actually free. This will happen
+if you are using different images that share some layers.
 
 ## OPTIONS
 #### **--format**=*format*

go.mod

@@ -14,7 +14,7 @@ require (
 	github.com/checkpoint-restore/go-criu/v7 v7.2.0
 	github.com/containernetworking/plugins v1.5.1
 	github.com/containers/buildah v1.39.2
-	github.com/containers/common v0.62.2-0.20250306142925-6e82793fd29d
+	github.com/containers/common v0.62.2-0.20250311121556-b27979403716
 	github.com/containers/conmon v2.0.20+incompatible
 	github.com/containers/gvisor-tap-vsock v0.8.4
 	github.com/containers/image/v5 v5.34.2-0.20250306154130-12497efe55ac

go.sum

@@ -78,8 +78,8 @@ github.com/containernetworking/plugins v1.5.1 h1:T5ji+LPYjjgW0QM+KyrigZbLsZ8jaX+
 github.com/containernetworking/plugins v1.5.1/go.mod h1:MIQfgMayGuHYs0XdNudf31cLLAC+i242hNm6KuDGqCM=
 github.com/containers/buildah v1.39.2 h1:YaFMNnuTr7wKYKQDHkm7yyP9HhWVrNB4DA+DjYUS9k4=
 github.com/containers/buildah v1.39.2/go.mod h1:Vb4sDbEq06qQqk29mcGw/1qit8dyukpfL4hwNQ5t+z8=
-github.com/containers/common v0.62.2-0.20250306142925-6e82793fd29d h1:FTsiNAhuriMBXf6x5e9pFoc4W2mJxsnI0HUOBpPGB94=
-github.com/containers/common v0.62.2-0.20250306142925-6e82793fd29d/go.mod h1:Dta+lCx83XAeGHtWwTPz+UwpWMiC0nMZQu4LWm1mTEg=
+github.com/containers/common v0.62.2-0.20250311121556-b27979403716 h1:4kwkokczKDAeYELVxxjV5Mpys/sA5TZreq6+cTpzX/M=
+github.com/containers/common v0.62.2-0.20250311121556-b27979403716/go.mod h1:Dta+lCx83XAeGHtWwTPz+UwpWMiC0nMZQu4LWm1mTEg=
 github.com/containers/conmon v2.0.20+incompatible h1:YbCVSFSCqFjjVwHTPINGdMX1F6JXHGTUje2ZYobNrkg=
 github.com/containers/conmon v2.0.20+incompatible/go.mod h1:hgwZ2mtuDrppv78a/cOBNiCm6O0UMWGx1mu7P00nu5I=
 github.com/containers/gvisor-tap-vsock v0.8.4 h1:z7MqcldnXYGaU6uTaKVl7RFxTmbhNsd2UL0CyM3fdBs=

test/system/030-run.bats

@@ -1398,6 +1398,10 @@ EOF
 
     grep -E -q "^containers:" /etc/subuid || skip "no IDs allocated for user 'containers'"
 
+    if [[ "$(podman_storage_driver)" == "vfs" ]]; then
+        skip "FIXME #25572: image mount with uidmapping and vfs not consistent and can fail"
+    fi
+
     # the TMPDIR must be accessible by different users as the following tests use different mappings
     chmod 755 $PODMAN_TMPDIR
 

test/system/320-system-df.bats

@@ -144,11 +144,13 @@ Size           |   ~${size}.*MB |        !0B |            0B
     run_podman system df --format '{{.Reclaimable}}'
     is "${lines[0]}" ".* (100%)" "100 percent of image data is reclaimable because $IMAGE has unique size of 0"
 
-    # Make sure the unique size is now really 0.  We cannot use --format for
-    # that unfortunately but we can exploit the fact that $IMAGE is used by
-    # two containers.
+    # Note unique size is basically never 0, that is because we count certain image metadata that is always added.
+    # The unique size is not 100% stable either as the generated metadata seems to differ a few bytes each run,
+    # as such we just match any number and just check that MB/kB seems to line up.
+    #   regex for:       SHARED SIZE      |         UNIQUE SIZE       |   CONTAINERS
     run_podman system df -v
-    is "$output" ".*0B\\s\\+2.*"
+    assert "$output" =~ '[0-9]+.[0-9]+MB\s+[0-9]+.[0-9]+kB\s+2' "Shared and Unique Size 2"
+    assert "$output" =~ "[0-9]+.[0-9]+MB\s+[0-9]+.[0-9]+kB\s+0" "Shared and Unique Size 0"
 
     run_podman rm $c1 $c2
 
@@ -159,4 +161,39 @@ Size           |   ~${size}.*MB |        !0B |            0B
     run_podman volume rm -a
 }
 
+# https://github.com/containers/podman/issues/24452
+@test "podman system df - Reclaimable is not negative" {
+    local c1="c1-$(safename)"
+    local c2="c2-$(safename)"
+    for t in "$c1" "$c2"; do
+        dir="${PODMAN_TMPDIR}${t}"
+        mkdir "$dir"
+        cat <<EOF >"$dir/Dockerfile"
+FROM $IMAGE
+RUN echo "${t}" >${t}.txt
+CMD ["sleep", "inf"]
+EOF
+
+    run_podman build --tag "${t}:latest" "$dir"
+    run_podman run -d --name $t "${t}:latest"
+    done
+
+    run_podman system df --format '{{.Reclaimable}}'
+    # Size might not be exactly static so match a range.
+    # Also note if you wondering why we claim 100% can be freed even though containers
+    # are using the images this value is simply broken.
+    # It always considers shared sizes as something that can be freed.
+    assert "${lines[0]}" =~ '1[0-9].[0-9]+MB \(100%\)' "Reclaimable size before prune"
+
+    # Prune the images to get rid of $IMAGE which is the shared parent
+    run_podman image prune -af
+
+    run_podman system df --format '{{.Reclaimable}}'
+    # Note this used to return something negative per #24452
+    assert "${lines[0]}" =~ '1[0-9].[0-9]+MB \(100%\)' "Reclaimable size after prune"
+
+    run_podman rm -f -t0 $c1 $c2
+    run_podman rmi  $c1 $c2
+}
+
 # vim: filetype=sh

test/system/400-unprivileged-access.bats

@@ -136,6 +136,9 @@ EOF
         fi
     done
 
+    # If the image does not exists, the pull output will make the test below fail
+    _prefetch $IMAGE
+
     # Run 'stat' on all the files, plus /dev/null. Get path, file type,
     # number of links, major, and minor (see below for why). Do it all
     # in one go, to avoid multiple podman-runs

vendor/github.com/containers/common/libimage/disk_usage.go

@@ -5,6 +5,9 @@ package libimage
 import (
 	"context"
 	"time"
+
+	"github.com/containers/storage"
+	"github.com/sirupsen/logrus"
 )
 
 // ImageDiskUsage reports the total size of an image.  That is the size
@@ -36,49 +39,49 @@ func (r *Runtime) DiskUsage(ctx context.Context) ([]ImageDiskUsage, int64, error
 		return nil, -1, err
 	}
 
-	layerTree, err := r.newLayerTreeFromData(images, layers)
-	if err != nil {
-		return nil, -1, err
+	var totalSize int64
+	layerMap := make(map[string]*storage.Layer)
+	for _, layer := range layers {
+		layerMap[layer.ID] = &layer
+		if layer.UncompressedSize == -1 {
+			// size is unknown, we must manually diff the layer size which
+			// can be quite slow as it might have to walk all files
+			size, err := r.store.DiffSize("", layer.ID)
+			if err != nil {
+				return nil, -1, err
+			}
+			// cache the size now
+			layer.UncompressedSize = size
+		}
+		// count the total layer size here so we know we only count each layer once
+		totalSize += layer.UncompressedSize
 	}
 
-	var totalSize int64
-	visitedImages := make(map[string]bool)
-	visistedLayers := make(map[string]bool)
+	// First walk all images to count how often each layer is used.
+	// This is done so we know if the size for an image is shared between
+	// images that use the same layer or unique.
+	layerCount := make(map[string]int)
+	for _, image := range images {
+		walkImageLayers(image, layerMap, func(layer *storage.Layer) {
+			// Increment the count for each layer visit
+			layerCount[layer.ID] += 1
+		})
+	}
 
+	// Now that we actually have all the info walk again to add the sizes.
 	var allUsages []ImageDiskUsage
 	for _, image := range images {
-		usages, err := diskUsageForImage(ctx, image, layerTree)
+		usages, err := diskUsageForImage(ctx, image, layerMap, layerCount, &totalSize)
 		if err != nil {
 			return nil, -1, err
 		}
 		allUsages = append(allUsages, usages...)
-
-		if _, ok := visitedImages[image.ID()]; ok {
-			// Do not count an image twice
-			continue
-		}
-		visitedImages[image.ID()] = true
-
-		size, err := image.Size()
-		if err != nil {
-			return nil, -1, err
-		}
-		for _, layer := range layerTree.layersOf(image) {
-			if _, ok := visistedLayers[layer.ID]; ok {
-				// Do not count a layer twice, so remove its
-				// size from the image size.
-				size -= layer.UncompressedSize
-				continue
-			}
-			visistedLayers[layer.ID] = true
-		}
-		totalSize += size
 	}
 	return allUsages, totalSize, err
 }
 
 // diskUsageForImage returns the disk-usage baseistics for the specified image.
-func diskUsageForImage(ctx context.Context, image *Image, tree *layerTree) ([]ImageDiskUsage, error) {
+func diskUsageForImage(ctx context.Context, image *Image, layerMap map[string]*storage.Layer, layerCount map[string]int, totalSize *int64) ([]ImageDiskUsage, error) {
 	if err := image.isCorrupted(ctx, ""); err != nil {
 		return nil, err
 	}
@@ -90,36 +93,25 @@ func diskUsageForImage(ctx context.Context, image *Image, tree *layerTree) ([]Im
 		Tag:        "<none>",
 	}
 
-	// Shared, unique and total size.
-	parent, err := tree.parent(ctx, image)
-	if err != nil {
-		return nil, err
-	}
-	childIDs, err := tree.children(ctx, image, false)
-	if err != nil {
-		return nil, err
-	}
-
-	// Optimistically set unique size to the full size of the image.
-	size, err := image.Size()
-	if err != nil {
-		return nil, err
-	}
-	base.UniqueSize = size
-
-	if len(childIDs) > 0 {
-		// If we have children, we share everything.
-		base.SharedSize = base.UniqueSize
-		base.UniqueSize = 0
-	} else if parent != nil {
-		// If we have no children but a parent, remove the parent
-		// (shared) size from the unique one.
-		size, err := parent.Size()
-		if err != nil {
-			return nil, err
+	walkImageLayers(image, layerMap, func(layer *storage.Layer) {
+		// If the layer used by more than one image it shares its size
+		if layerCount[layer.ID] > 1 {
+			base.SharedSize += layer.UncompressedSize
+		} else {
+			base.UniqueSize += layer.UncompressedSize
 		}
-		base.UniqueSize -= size
-		base.SharedSize = size
+	})
+
+	// Count the image specific big data as well.
+	// store.BigDataSize() is not used intentionally, it is slower (has to take
+	// locks) and can fail.
+	// BigDataSizes is always correctly populated on new stores since c/storage
+	// commit a7d7fe8c9a (2016). It should be safe to assume that no such old
+	// store+image exist now so we don't bother. Worst case we report a few
+	// bytes to little.
+	for _, size := range image.storageImage.BigDataSizes {
+		base.UniqueSize += size
+		*totalSize += size
 	}
 
 	base.Size = base.SharedSize + base.UniqueSize
@@ -155,3 +147,33 @@ func diskUsageForImage(ctx context.Context, image *Image, tree *layerTree) ([]Im
 
 	return results, nil
 }
+
+// walkImageLayers walks all layers in an image and calls the given function for each layer.
+func walkImageLayers(image *Image, layerMap map[string]*storage.Layer, f func(layer *storage.Layer)) {
+	visited := make(map[string]struct{})
+	// Layers are walked recursively until it has no parent which means we reached the end.
+	// We must account for the fact that an image might have several top layers when id mappings are used.
+	layers := append([]string{image.storageImage.TopLayer}, image.storageImage.MappedTopLayers...)
+	for _, layerID := range layers {
+		for layerID != "" {
+			layer := layerMap[layerID]
+			if layer == nil {
+				logrus.Errorf("Local Storage is corrupt, layer %q missing from the storage", layerID)
+				break
+			}
+			if _, ok := visited[layerID]; ok {
+				// We have seen this layer before. Break here to
+				// a) Do not count the same layer twice that was shared between
+				//    the TopLayer and MappedTopLayers layer chain.
+				// b) Prevent infinite loops, should not happen per c/storage
+				//    design but it is good to be safer.
+				break
+			}
+			visited[layerID] = struct{}{}
+
+			f(layer)
+			// Set the layer for the next iteration, parent is empty if we reach the end.
+			layerID = layer.Parent
+		}
+	}
+}

vendor/github.com/containers/common/libimage/layer_tree.go

@@ -141,19 +141,6 @@ func (r *Runtime) newLayerTreeFromData(images []*Image, layers []storage.Layer)
 	return &tree, nil
 }
 
-// layersOf returns all storage layers of the specified image.
-func (t *layerTree) layersOf(image *Image) []*storage.Layer {
-	var layers []*storage.Layer
-	node := t.node(image.TopLayer())
-	for node != nil {
-		if node.layer != nil {
-			layers = append(layers, node.layer)
-		}
-		node = node.parent
-	}
-	return layers
-}
-
 // children returns the child images of parent. Child images are images with
 // either the same top layer as parent or parent being the true parent layer.
 // Furthermore, the history of the parent and child images must match with the

vendor/modules.txt

@@ -179,7 +179,7 @@ github.com/containers/buildah/pkg/sshagent
 github.com/containers/buildah/pkg/util
 github.com/containers/buildah/pkg/volumes
 github.com/containers/buildah/util
-# github.com/containers/common v0.62.2-0.20250306142925-6e82793fd29d
+# github.com/containers/common v0.62.2-0.20250311121556-b27979403716
 ## explicit; go 1.22.8
 github.com/containers/common/internal
 github.com/containers/common/internal/attributedstring