Clean machine pull cache

Cache cleanups only happen if there is a cache miss, and we need to pull a new image

For quay.io/podman/machine-os, we remove all old images from the cache dir. This means we will delete any file that exists in the cache dir; this should be safe to do since the machine pull code should be the only thing touching this cache dir. OCI machine images will always have a different manifest, and won’t be updated with the same manifest, so if the version moves on, there isn’t a reason to keep the old version in the cache, it really doesn’t change.

For Fedora (WSL), we use the cache, so we go through the cache dir and remove any old cached images, on a cache miss. We also switch to using ~/.local/share/containers/podman/machine/wsl/cache as the cache dir rather than ~/.local/share/containers/podman/machine/wsl. Both these behaviors existed in v4.9, but are now added back into 5.x.

For generic files pulled from a URL or a non-default OCI image, we shouldn’t actually cache, so we delete the pulled file immediately after creating a machine image. This restores the behavior from v4.9.

For generic files from a local path, the original file will never be cleaned up

Unsure how to test, so:
[NO NEW TESTS NEEDED]

Signed-off-by: Ashley Cui <acui@redhat.com>

pkg/machine/e2e/machine_pull_test.go

@@ -26,7 +26,7 @@ func pullOCITestDisk(finalDir string, vmType define.VMType) error {
 	if err != nil {
 		return err
 	}
-	err = ociArtPull.GetNoCompress()
+	_, err = ociArtPull.GetNoCompress()
 	if err != nil {
 		return err
 	}

pkg/machine/ocipull/ociartifact.go

@@ -32,6 +32,7 @@ const (
 )
 
 type OCIArtifactDisk struct {
+	cache                    bool
 	cachedCompressedDiskPath *define.VMFile
 	name                     string
 	ctx                      context.Context
@@ -91,12 +92,15 @@ func NewOCIArtifactPull(ctx context.Context, dirs *define.MachineDirs, endpoint
 		os:       machineOS,
 	}
 
+	cache := false
 	if endpoint == "" {
 		endpoint = fmt.Sprintf("docker://%s/%s/%s:%s", artifactRegistry, artifactRepo, artifactImageName, artifactVersion.majorMinor())
+		cache = true
 	}
 
 	ociDisk := OCIArtifactDisk{
 		ctx:              ctx,
+		cache:            cache,
 		dirs:             dirs,
 		diskArtifactOpts: &diskOpts,
 		finalPath:        finalPath.GetPath(),
@@ -114,36 +118,46 @@ func (o *OCIArtifactDisk) OriginalFileName() (string, string) {
 }
 
 func (o *OCIArtifactDisk) Get() error {
-	if err := o.get(); err != nil {
+	cleanCache, err := o.get()
+	if err != nil {
 		return err
 	}
+	if cleanCache != nil {
+		defer cleanCache()
+	}
 	return o.decompress()
 }
 
-func (o *OCIArtifactDisk) GetNoCompress() error {
+func (o *OCIArtifactDisk) GetNoCompress() (func(), error) {
 	return o.get()
 }
 
-func (o *OCIArtifactDisk) get() error {
+func (o *OCIArtifactDisk) get() (func(), error) {
+	cleanCache := func() {}
+
 	destRef, artifactDigest, err := o.getDestArtifact()
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	// Note: the artifactDigest here is the hash of the most recent disk image available
 	cachedImagePath, err := o.dirs.ImageCacheDir.AppendToNewVMFile(fmt.Sprintf("%s.%s", artifactDigest.Encoded(), o.vmType.ImageFormat().KindWithCompression()), nil)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	// check if we have the latest and greatest disk image
 	if _, err = os.Stat(cachedImagePath.GetPath()); err != nil {
 		if !errors.Is(err, os.ErrNotExist) {
-			return fmt.Errorf("unable to access cached image path %q: %q", cachedImagePath.GetPath(), err)
+			return nil, fmt.Errorf("unable to access cached image path %q: %q", cachedImagePath.GetPath(), err)
 		}
+
+		// On cache misses, we clean out the cache
+		cleanCache = o.cleanCache(cachedImagePath.GetPath())
+
 		// pull the image down to our local filesystem
 		if err := o.pull(destRef, artifactDigest); err != nil {
-			return fmt.Errorf("failed to pull %s: %w", destRef.DockerReference(), err)
+			return nil, fmt.Errorf("failed to pull %s: %w", destRef.DockerReference(), err)
 		}
 		// grab the artifact disk out of the cache and lay
 		// it into our local cache in the format of
@@ -153,13 +167,46 @@ func (o *OCIArtifactDisk) get() error {
 		//
 		// i.e. 91d1e51...d28974.qcow2.xz
 		if err := o.unpack(artifactDigest); err != nil {
-			return err
+			return nil, err
 		}
 	} else {
 		logrus.Debugf("cached image exists and is latest: %s", cachedImagePath.GetPath())
 		o.cachedCompressedDiskPath = cachedImagePath
 	}
-	return nil
+	return cleanCache, nil
+}
+
+func (o *OCIArtifactDisk) cleanCache(cachedImagePath string) func() {
+	// cache miss while using an image that we cache, ie the default image
+	// clean out all old files fron the cache dir
+	if o.cache {
+		files, err := os.ReadDir(o.dirs.ImageCacheDir.GetPath())
+		if err != nil {
+			logrus.Warn("failed to clean machine image cache: ", err)
+			return nil
+		}
+
+		return func() {
+			for _, file := range files {
+				path := filepath.Join(o.dirs.ImageCacheDir.GetPath(), file.Name())
+				logrus.Debugf("cleaning cached file: %s", path)
+				err := utils.GuardedRemoveAll(path)
+				if err != nil && !errors.Is(err, os.ErrNotExist) {
+					logrus.Warn("failed to clean machine image cache: ", err)
+				}
+			}
+		}
+	} else {
+		// using an image that we don't cache, ie not the default image
+		// delete image after use and don't cache
+		return func() {
+			logrus.Debugf("cleaning cache: %s", o.dirs.ImageCacheDir.GetPath())
+			err := os.Remove(cachedImagePath)
+			if err != nil && !errors.Is(err, os.ErrNotExist) {
+				logrus.Warn("failed to clean pulled machine image: ", err)
+			}
+		}
+	}
 }
 
 func (o *OCIArtifactDisk) getDestArtifact() (types.ImageReference, digest.Digest, error) {

pkg/machine/shim/diskpull/diskpull.go

@@ -20,7 +20,7 @@ func GetDisk(userInputPath string, dirs *define.MachineDirs, imagePath *define.V
 	} else {
 		if strings.HasPrefix(userInputPath, "http") {
 			// TODO probably should use tempdir instead of datadir
-			mydisk, err = stdpull.NewDiskFromURL(userInputPath, imagePath, dirs.DataDir, nil)
+			mydisk, err = stdpull.NewDiskFromURL(userInputPath, imagePath, dirs.DataDir, nil, false)
 		} else {
 			mydisk, err = stdpull.NewStdDiskPull(userInputPath, imagePath)
 		}

pkg/machine/stdpull/url.go

@@ -22,9 +22,10 @@ type DiskFromURL struct {
 	u            *url2.URL
 	finalPath    *define.VMFile
 	tempLocation *define.VMFile
+	cache        bool
 }
 
-func NewDiskFromURL(inputPath string, finalPath *define.VMFile, tempDir *define.VMFile, optionalTempFileName *string) (*DiskFromURL, error) {
+func NewDiskFromURL(inputPath string, finalPath *define.VMFile, tempDir *define.VMFile, optionalTempFileName *string, cache bool) (*DiskFromURL, error) {
 	var (
 		err error
 	)
@@ -57,6 +58,7 @@ func NewDiskFromURL(inputPath string, finalPath *define.VMFile, tempDir *define.
 		u:            u,
 		finalPath:    finalPath,
 		tempLocation: tempLocation,
+		cache:        cache,
 	}, nil
 }
 
@@ -65,6 +67,16 @@ func (d *DiskFromURL) Get() error {
 	if err := d.pull(); err != nil {
 		return err
 	}
+	if !d.cache {
+		defer func() {
+			if err := utils.GuardedRemoveAll(d.tempLocation.GetPath()); err != nil {
+				if !errors.Is(err, os.ErrNotExist) {
+					logrus.Warn("failed to clean machine image cache: ", err)
+				}
+			}
+		}()
+	}
+
 	logrus.Debugf("decompressing (if needed) %s to %s", d.tempLocation.GetPath(), d.finalPath.GetPath())
 	return compression.Decompress(d.tempLocation, d.finalPath.GetPath())
 }

pkg/machine/wsl/stubber.go

@@ -15,6 +15,7 @@ import (
 	"github.com/containers/podman/v5/pkg/machine/shim/diskpull"
 	"github.com/containers/podman/v5/pkg/machine/stdpull"
 	"github.com/containers/podman/v5/pkg/machine/wsl/wutil"
+	"github.com/containers/podman/v5/utils"
 
 	gvproxy "github.com/containers/gvisor-tap-vsock/pkg/types"
 	"github.com/containers/podman/v5/pkg/machine"
@@ -303,8 +304,7 @@ func (w WSLStubber) GetDisk(userInputPath string, dirs *define.MachineDirs, mc *
 	// i.e.v39.0.31-rootfs.tar.xz
 	versionedBase := fmt.Sprintf("%s-%s", downloadVersion, filepath.Base(downloadURL.Path))
 
-	// TODO we need a mechanism for "flushing" old cache files
-	cachedFile, err := dirs.DataDir.AppendToNewVMFile(versionedBase, nil)
+	cachedFile, err := dirs.ImageCacheDir.AppendToNewVMFile(versionedBase, nil)
 	if err != nil {
 		return err
 	}
@@ -313,12 +313,30 @@ func (w WSLStubber) GetDisk(userInputPath string, dirs *define.MachineDirs, mc *
 	if _, err = os.Stat(cachedFile.GetPath()); err == nil {
 		logrus.Debugf("%q already exists locally", cachedFile.GetPath())
 		myDisk, err = stdpull.NewStdDiskPull(cachedFile.GetPath(), mc.ImagePath)
+		if err != nil {
+			return err
+		}
 	} else {
-		// no cached file
-		myDisk, err = stdpull.NewDiskFromURL(downloadURL.String(), mc.ImagePath, dirs.DataDir, &versionedBase)
-	}
-	if err != nil {
-		return err
+		files, err := os.ReadDir(dirs.ImageCacheDir.GetPath())
+		if err != nil {
+			logrus.Warn("failed to clean machine image cache: ", err)
+		} else {
+			defer func() {
+				for _, file := range files {
+					path := filepath.Join(dirs.ImageCacheDir.GetPath(), file.Name())
+					logrus.Debugf("cleaning cached image: %s", path)
+					err := utils.GuardedRemoveAll(path)
+					if err != nil && !errors.Is(err, os.ErrNotExist) {
+						logrus.Warn("failed to clean machine image cache: ", err)
+					}
+				}
+			}()
+		}
+
+		myDisk, err = stdpull.NewDiskFromURL(downloadURL.String(), mc.ImagePath, dirs.ImageCacheDir, &versionedBase, true)
+		if err != nil {
+			return err
+		}
 	}
 	// up until now, nothing has really happened
 	// pull if needed and decompress to image location