From e412eff33f79a73b2515c85b73723469655f55f1 Mon Sep 17 00:00:00 2001 From: Ashley Cui Date: Thu, 25 Apr 2024 09:32:57 -0400 Subject: [PATCH] Clean machine pull cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- pkg/machine/e2e/machine_pull_test.go | 2 +- pkg/machine/ocipull/ociartifact.go | 65 +++++++++++++++++++++++---- pkg/machine/shim/diskpull/diskpull.go | 2 +- pkg/machine/stdpull/url.go | 14 +++++- pkg/machine/wsl/stubber.go | 32 ++++++++++--- 5 files changed, 96 insertions(+), 19 deletions(-) diff --git a/pkg/machine/e2e/machine_pull_test.go b/pkg/machine/e2e/machine_pull_test.go index 8a37e178c5..ca81ecf7ff 100644 --- a/pkg/machine/e2e/machine_pull_test.go +++ b/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 } diff --git a/pkg/machine/ocipull/ociartifact.go b/pkg/machine/ocipull/ociartifact.go index 6c48be1e89..58c4f87eb8 100644 --- a/pkg/machine/ocipull/ociartifact.go +++ b/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) { diff --git a/pkg/machine/shim/diskpull/diskpull.go b/pkg/machine/shim/diskpull/diskpull.go index 49e9ff09e1..83526d3f5b 100644 --- a/pkg/machine/shim/diskpull/diskpull.go +++ b/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) } diff --git a/pkg/machine/stdpull/url.go b/pkg/machine/stdpull/url.go index d9f86aba0c..c724d54ef8 100644 --- a/pkg/machine/stdpull/url.go +++ b/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()) } diff --git a/pkg/machine/wsl/stubber.go b/pkg/machine/wsl/stubber.go index c0dd592b4f..5197893c9e 100644 --- a/pkg/machine/wsl/stubber.go +++ b/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