From f9aef8107905ca49573cc1f70542ff8b50df1717 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Rod=C3=A1k?= Date: Wed, 11 Jun 2025 13:09:30 +0200 Subject: [PATCH] Integrate the temporary directory into layer deletion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This enables deferred deletion of physical files outside of global locks, improving performance and reducing lock contention. Signed-off-by: Jan Rodák --- drivers/aufs/aufs.go | 12 +++++ drivers/btrfs/btrfs.go | 12 +++++ drivers/driver.go | 11 +++++ drivers/overlay/overlay.go | 60 +++++++++++++++++++---- drivers/vfs/driver.go | 42 +++++++++++++++- drivers/windows/windows.go | 12 +++++ drivers/zfs/zfs.go | 12 +++++ layers.go | 98 +++++++++++++++++++++++++++++--------- store.go | 57 +++++++++++++++++----- store_test.go | 60 +++++++++++++++++++++++ userns.go | 2 +- 11 files changed, 333 insertions(+), 45 deletions(-) diff --git a/drivers/aufs/aufs.go b/drivers/aufs/aufs.go index 964ba8c91..5925c9da3 100644 --- a/drivers/aufs/aufs.go +++ b/drivers/aufs/aufs.go @@ -36,6 +36,7 @@ import ( "time" graphdriver "github.com/containers/storage/drivers" + "github.com/containers/storage/internal/tempdir" "github.com/containers/storage/pkg/archive" "github.com/containers/storage/pkg/chrootarchive" "github.com/containers/storage/pkg/directory" @@ -781,3 +782,14 @@ func (a *Driver) SupportsShifting(uidmap, gidmap []idtools.IDMap) bool { func (a *Driver) Dedup(req graphdriver.DedupArgs) (graphdriver.DedupResult, error) { return graphdriver.DedupResult{}, nil } + +// DeferredRemove is not implemented. +// It calls Remove directly. +func (a *Driver) DeferredRemove(id string) (tempdir.CleanupTempDirFunc, error) { + return nil, a.Remove(id) +} + +// GetTempDirRootDirs is not implemented. +func (a *Driver) GetTempDirRootDirs() []string { + return []string{} +} diff --git a/drivers/btrfs/btrfs.go b/drivers/btrfs/btrfs.go index 4a80339f4..95bbe8cc7 100644 --- a/drivers/btrfs/btrfs.go +++ b/drivers/btrfs/btrfs.go @@ -30,6 +30,7 @@ import ( "unsafe" graphdriver "github.com/containers/storage/drivers" + "github.com/containers/storage/internal/tempdir" "github.com/containers/storage/pkg/directory" "github.com/containers/storage/pkg/fileutils" "github.com/containers/storage/pkg/idtools" @@ -678,3 +679,14 @@ func (d *Driver) AdditionalImageStores() []string { func (d *Driver) Dedup(req graphdriver.DedupArgs) (graphdriver.DedupResult, error) { return graphdriver.DedupResult{}, nil } + +// DeferredRemove is not implemented. +// It calls Remove directly. +func (d *Driver) DeferredRemove(id string) (tempdir.CleanupTempDirFunc, error) { + return nil, d.Remove(id) +} + +// GetTempDirRootDirs is not implemented. +func (d *Driver) GetTempDirRootDirs() []string { + return []string{} +} diff --git a/drivers/driver.go b/drivers/driver.go index 24d7b66b0..553778081 100644 --- a/drivers/driver.go +++ b/drivers/driver.go @@ -9,6 +9,7 @@ import ( "strings" "github.com/containers/storage/internal/dedup" + "github.com/containers/storage/internal/tempdir" "github.com/containers/storage/pkg/archive" "github.com/containers/storage/pkg/directory" "github.com/containers/storage/pkg/fileutils" @@ -123,7 +124,17 @@ type ProtoDriver interface { // and parent, with contents identical to the specified template layer. CreateFromTemplate(id, template string, templateIDMappings *idtools.IDMappings, parent string, parentIDMappings *idtools.IDMappings, opts *CreateOpts, readWrite bool) error // Remove attempts to remove the filesystem layer with this id. + // This is soft-deprecated and should not get any new callers; use DeferredRemove. Remove(id string) error + // DeferredRemove is used to remove the filesystem layer with this id. + // This removal happen immediately (the layer is no longer usable), + // but physically deleting the files may be deferred. + // Caller MUST call returned Cleanup function EVEN IF the function returns an error. + DeferredRemove(id string) (tempdir.CleanupTempDirFunc, error) + // GetTempDirRootDirs returns the root directories for temporary directories. + // Multiple directories may be returned when drivers support different filesystems + // for layers (e.g., overlay with imageStore vs home directory). + GetTempDirRootDirs() []string // Get returns the mountpoint for the layered filesystem referred // to by this id. You can optionally specify a mountLabel or "". // Optionally it gets the mappings used to create the layer. diff --git a/drivers/overlay/overlay.go b/drivers/overlay/overlay.go index a01d6b369..2bc115afd 100644 --- a/drivers/overlay/overlay.go +++ b/drivers/overlay/overlay.go @@ -24,6 +24,7 @@ import ( "github.com/containers/storage/drivers/quota" "github.com/containers/storage/internal/dedup" "github.com/containers/storage/internal/staging_lockfile" + "github.com/containers/storage/internal/tempdir" "github.com/containers/storage/pkg/archive" "github.com/containers/storage/pkg/chrootarchive" "github.com/containers/storage/pkg/directory" @@ -80,10 +81,11 @@ const ( // that mounts do not fail due to length. const ( - linkDir = "l" - stagingDir = "staging" - lowerFile = "lower" - maxDepth = 500 + linkDir = "l" + stagingDir = "staging" + tempDirName = "tempdirs" + lowerFile = "lower" + maxDepth = 500 stagingLockFile = "staging.lock" @@ -1305,17 +1307,22 @@ func (d *Driver) optsAppendMappings(opts string, uidMaps, gidMaps []idtools.IDMa // Remove cleans the directories that are created for this id. func (d *Driver) Remove(id string) error { + return d.removeCommon(id, system.EnsureRemoveAll) +} + +func (d *Driver) removeCommon(id string, cleanup func(string) error) error { dir := d.dir(id) lid, err := os.ReadFile(path.Join(dir, "link")) if err == nil { - if err := os.RemoveAll(path.Join(d.home, linkDir, string(lid))); err != nil { + linkPath := path.Join(d.home, linkDir, string(lid)) + if err := cleanup(linkPath); err != nil { logrus.Debugf("Failed to remove link: %v", err) } } d.releaseAdditionalLayerByID(id) - if err := system.EnsureRemoveAll(dir); err != nil && !os.IsNotExist(err) { + if err := cleanup(dir); err != nil && !os.IsNotExist(err) { return err } if d.quotaCtl != nil { @@ -1327,6 +1334,41 @@ func (d *Driver) Remove(id string) error { return nil } +func (d *Driver) GetTempDirRootDirs() []string { + tempDirs := []string{filepath.Join(d.home, tempDirName)} + // Include imageStore temp directory if it's configured + // Writable layers can only be in d.home or d.imageStore, not in additional image stores + if d.imageStore != "" { + tempDirs = append(tempDirs, filepath.Join(d.imageStore, d.name, tempDirName)) + } + return tempDirs +} + +// Determine the correct temp directory root based on where the layer actually exists. +func (d *Driver) getTempDirRoot(id string) string { + layerDir := d.dir(id) + if d.imageStore != "" { + expectedLayerDir := path.Join(d.imageStore, d.name, id) + if layerDir == expectedLayerDir { + return filepath.Join(d.imageStore, d.name, tempDirName) + } + } + return filepath.Join(d.home, tempDirName) +} + +func (d *Driver) DeferredRemove(id string) (tempdir.CleanupTempDirFunc, error) { + tempDirRoot := d.getTempDirRoot(id) + t, err := tempdir.NewTempDir(tempDirRoot) + if err != nil { + return nil, err + } + + if err := d.removeCommon(id, t.StageDeletion); err != nil { + return t.Cleanup, fmt.Errorf("failed to add to stage directory: %w", err) + } + return t.Cleanup, nil +} + // recreateSymlinks goes through the driver's home directory and checks if the diff directory // under each layer has a symlink created for it under the linkDir. If the symlink does not // exist, it creates them @@ -1353,8 +1395,8 @@ func (d *Driver) recreateSymlinks() error { // Check that for each layer, there's a link in "l" with the name in // the layer's "link" file that points to the layer's "diff" directory. for _, dir := range dirs { - // Skip over the linkDir and anything that is not a directory - if dir.Name() == linkDir || !dir.IsDir() { + // Skip over the linkDir, stagingDir, tempDirName and anything that is not a directory + if dir.Name() == linkDir || dir.Name() == stagingDir || dir.Name() == tempDirName || !dir.IsDir() { continue } // Read the "link" file under each layer to get the name of the symlink @@ -2022,7 +2064,7 @@ func (d *Driver) ListLayers() ([]string, error) { for _, entry := range entries { id := entry.Name() switch id { - case linkDir, stagingDir, quota.BackingFsBlockDeviceLink, mountProgramFlagFile: + case linkDir, stagingDir, tempDirName, quota.BackingFsBlockDeviceLink, mountProgramFlagFile: // expected, but not a layer. skip it continue default: diff --git a/drivers/vfs/driver.go b/drivers/vfs/driver.go index 87ff885ec..7ad5186ae 100644 --- a/drivers/vfs/driver.go +++ b/drivers/vfs/driver.go @@ -11,6 +11,7 @@ import ( graphdriver "github.com/containers/storage/drivers" "github.com/containers/storage/internal/dedup" + "github.com/containers/storage/internal/tempdir" "github.com/containers/storage/pkg/archive" "github.com/containers/storage/pkg/directory" "github.com/containers/storage/pkg/fileutils" @@ -22,7 +23,10 @@ import ( "github.com/vbatts/tar-split/tar/storage" ) -const defaultPerms = os.FileMode(0o555) +const ( + defaultPerms = os.FileMode(0o555) + tempDirName = "tempdirs" +) func init() { graphdriver.MustRegister("vfs", Init) @@ -244,6 +248,42 @@ func (d *Driver) Remove(id string) error { return system.EnsureRemoveAll(d.dir(id)) } +func (d *Driver) GetTempDirRootDirs() []string { + tempDirs := []string{filepath.Join(d.home, tempDirName)} + // Include imageStore temp directory if it's configured + // Writable layers can only be in d.home or d.imageStore, not in additionalHomes (which are read-only) + if d.imageStore != "" { + tempDirs = append(tempDirs, filepath.Join(d.imageStore, d.String(), tempDirName)) + } + return tempDirs +} + +// Determine the correct temp directory root based on where the layer actually exists. +func (d *Driver) getTempDirRoot(id string) string { + layerDir := d.dir(id) + if d.imageStore != "" { + expectedLayerDir := filepath.Join(d.imageStore, d.String(), "dir", filepath.Base(id)) + if layerDir == expectedLayerDir { + return filepath.Join(d.imageStore, d.String(), tempDirName) + } + } + return filepath.Join(d.home, tempDirName) +} + +func (d *Driver) DeferredRemove(id string) (tempdir.CleanupTempDirFunc, error) { + tempDirRoot := d.getTempDirRoot(id) + t, err := tempdir.NewTempDir(tempDirRoot) + if err != nil { + return nil, err + } + + layerDir := d.dir(id) + if err := t.StageDeletion(layerDir); err != nil { + return t.Cleanup, err + } + return t.Cleanup, nil +} + // Get returns the directory for the given id. func (d *Driver) Get(id string, options graphdriver.MountOpts) (_ string, retErr error) { dir := d.dir(id) diff --git a/drivers/windows/windows.go b/drivers/windows/windows.go index 6a5c9bcd1..13bd75cfb 100644 --- a/drivers/windows/windows.go +++ b/drivers/windows/windows.go @@ -24,6 +24,7 @@ import ( "github.com/Microsoft/go-winio/backuptar" "github.com/Microsoft/hcsshim" graphdriver "github.com/containers/storage/drivers" + "github.com/containers/storage/internal/tempdir" "github.com/containers/storage/pkg/archive" "github.com/containers/storage/pkg/directory" "github.com/containers/storage/pkg/fileutils" @@ -1014,3 +1015,14 @@ func parseStorageOpt(storageOpt map[string]string) (*storageOptions, error) { } return &options, nil } + +// DeferredRemove is not implemented. +// It calls Remove directly. +func (d *Driver) DeferredRemove(id string) (tempdir.CleanupTempDirFunc, error) { + return nil, d.Remove(id) +} + +// GetTempDirRootDirs is not implemented. +func (d *Driver) GetTempDirRootDirs() []string { + return []string{} +} diff --git a/drivers/zfs/zfs.go b/drivers/zfs/zfs.go index f53b0e1b6..6be430b3e 100644 --- a/drivers/zfs/zfs.go +++ b/drivers/zfs/zfs.go @@ -13,6 +13,7 @@ import ( "time" graphdriver "github.com/containers/storage/drivers" + "github.com/containers/storage/internal/tempdir" "github.com/containers/storage/pkg/directory" "github.com/containers/storage/pkg/idtools" "github.com/containers/storage/pkg/mount" @@ -406,6 +407,12 @@ func (d *Driver) Remove(id string) error { return nil } +// DeferredRemove is not implemented. +// It calls Remove directly. +func (d *Driver) DeferredRemove(id string) (tempdir.CleanupTempDirFunc, error) { + return nil, d.Remove(id) +} + // Get returns the mountpoint for the given id after creating the target directories if necessary. func (d *Driver) Get(id string, options graphdriver.MountOpts) (_ string, retErr error) { mountpoint := d.mountPath(id) @@ -516,3 +523,8 @@ func (d *Driver) AdditionalImageStores() []string { func (d *Driver) Dedup(req graphdriver.DedupArgs) (graphdriver.DedupResult, error) { return graphdriver.DedupResult{}, nil } + +// GetTempDirRootDirs is not implemented. +func (d *Driver) GetTempDirRootDirs() []string { + return []string{} +} diff --git a/layers.go b/layers.go index a84706e4c..4d9e0b882 100644 --- a/layers.go +++ b/layers.go @@ -18,6 +18,7 @@ import ( "time" drivers "github.com/containers/storage/drivers" + "github.com/containers/storage/internal/tempdir" "github.com/containers/storage/pkg/archive" "github.com/containers/storage/pkg/idtools" "github.com/containers/storage/pkg/ioutils" @@ -38,6 +39,8 @@ import ( const ( tarSplitSuffix = ".tar-split.gz" + // tempDirPath is the subdirectory name used for storing temporary directories during layer deletion + tempDirPath = "tmp" incompleteFlag = "incomplete" // maxLayerStoreCleanupIterations is the number of times we try to clean up inconsistent layer store state // in readers (which, for implementation reasons, gives other writers the opportunity to create more inconsistent state) @@ -290,8 +293,14 @@ type rwLayerStore interface { // updateNames modifies names associated with a layer based on (op, names). updateNames(id string, names []string, op updateNameOperation) error - // Delete deletes a layer with the specified name or ID. - Delete(id string) error + // deleteWhileHoldingLock deletes a layer with the specified name or ID. + deleteWhileHoldingLock(id string) error + + // deferredDelete deletes a layer with the specified name or ID. + // This removal happen immediately (the layer is no longer usable), + // but physically deleting the files may be deferred. + // Caller MUST call all returned cleanup functions outside of the locks. + deferredDelete(id string) ([]tempdir.CleanupTempDirFunc, error) // Wipe deletes all layers. Wipe() error @@ -794,6 +803,17 @@ func (r *layerStore) load(lockedForWriting bool) (bool, error) { layers := []*Layer{} ids := make(map[string]*Layer) + if r.lockfile.IsReadWrite() { + if err := tempdir.RecoverStaleDirs(filepath.Join(r.layerdir, tempDirPath)); err != nil { + return false, err + } + for _, driverTempDirPath := range r.driver.GetTempDirRootDirs() { + if err := tempdir.RecoverStaleDirs(driverTempDirPath); err != nil { + return false, err + } + } + } + for locationIndex := range numLayerLocationIndex { location := layerLocationFromIndex(locationIndex) rpath := r.jsonPath[locationIndex] @@ -935,7 +955,12 @@ func (r *layerStore) load(lockedForWriting bool) (bool, error) { // Now actually delete the layers for _, layer := range layersToDelete { logrus.Warnf("Found incomplete layer %q, deleting it", layer.ID) - err := r.deleteInternal(layer.ID) + cleanFunctions, err := r.internalDelete(layer.ID) + defer func() { + if err := tempdir.CleanupTemporaryDirectories(cleanFunctions...); err != nil { + logrus.Errorf("Error cleaning up temporary directories: %v", err) + } + }() if err != nil { // Don't return the error immediately, because deleteInternal does not saveLayers(); // Even if deleting one incomplete layer fails, call saveLayers() so that other possible successfully @@ -1334,7 +1359,7 @@ func (r *layerStore) PutAdditionalLayer(id string, parentLayer *Layer, names []s r.bytocsum[layer.TOCDigest] = append(r.bytocsum[layer.TOCDigest], layer.ID) } if err := r.saveFor(layer); err != nil { - if e := r.Delete(layer.ID); e != nil { + if e := r.deleteWhileHoldingLock(layer.ID); e != nil { logrus.Errorf("While recovering from a failure to save layers, error deleting layer %#v: %v", id, e) } return nil, err @@ -1469,7 +1494,7 @@ func (r *layerStore) create(id string, parentLayer *Layer, names []string, mount if cleanupFailureContext == "" { cleanupFailureContext = "unknown: cleanupFailureContext not set at the failure site" } - if e := r.Delete(id); e != nil { + if e := r.deleteWhileHoldingLock(id); e != nil { logrus.Errorf("While recovering from a failure (%s), error deleting layer %#v: %v", cleanupFailureContext, id, e) } } @@ -1920,13 +1945,15 @@ func layerHasIncompleteFlag(layer *Layer) bool { } // Requires startWriting. -func (r *layerStore) deleteInternal(id string) error { +// Caller MUST run all returned cleanup functions after this, EVEN IF the function returns an error. +// Ideally outside of the startWriting. +func (r *layerStore) internalDelete(id string) ([]tempdir.CleanupTempDirFunc, error) { if !r.lockfile.IsReadWrite() { - return fmt.Errorf("not allowed to delete layers at %q: %w", r.layerdir, ErrStoreIsReadOnly) + return nil, fmt.Errorf("not allowed to delete layers at %q: %w", r.layerdir, ErrStoreIsReadOnly) } layer, ok := r.lookup(id) if !ok { - return ErrLayerUnknown + return nil, ErrLayerUnknown } // Ensure that if we are interrupted, the layer will be cleaned up. if !layerHasIncompleteFlag(layer) { @@ -1935,16 +1962,30 @@ func (r *layerStore) deleteInternal(id string) error { } layer.Flags[incompleteFlag] = true if err := r.saveFor(layer); err != nil { - return err + return nil, err } } // We never unset incompleteFlag; below, we remove the entire object from r.layers. - id = layer.ID - if err := r.driver.Remove(id); err != nil && !errors.Is(err, os.ErrNotExist) { - return err + tempDirectory, err := tempdir.NewTempDir(filepath.Join(r.layerdir, tempDirPath)) + cleanFunctions := []tempdir.CleanupTempDirFunc{} + cleanFunctions = append(cleanFunctions, tempDirectory.Cleanup) + if err != nil { + return nil, err + } + id = layer.ID + cleanFunc, err := r.driver.DeferredRemove(id) + cleanFunctions = append(cleanFunctions, cleanFunc) + if err != nil && !errors.Is(err, os.ErrNotExist) { + return cleanFunctions, err + } + + cleanFunctions = append(cleanFunctions, tempDirectory.Cleanup) + if err := tempDirectory.StageDeletion(r.tspath(id)); err != nil && !errors.Is(err, os.ErrNotExist) { + return cleanFunctions, err + } + if err := tempDirectory.StageDeletion(r.datadir(id)); err != nil && !errors.Is(err, os.ErrNotExist) { + return cleanFunctions, err } - os.Remove(r.tspath(id)) - os.RemoveAll(r.datadir(id)) delete(r.byid, id) for _, name := range layer.Names { delete(r.byname, name) @@ -1968,7 +2009,7 @@ func (r *layerStore) deleteInternal(id string) error { }) { selinux.ReleaseLabel(mountLabel) } - return nil + return cleanFunctions, nil } // Requires startWriting. @@ -1988,10 +2029,20 @@ func (r *layerStore) deleteInDigestMap(id string) { } // Requires startWriting. -func (r *layerStore) Delete(id string) error { +// This is soft-deprecated and should not have any new callers; use deferredDelete instead. +func (r *layerStore) deleteWhileHoldingLock(id string) error { + cleanupFunctions, deferErr := r.deferredDelete(id) + cleanupErr := tempdir.CleanupTemporaryDirectories(cleanupFunctions...) + return errors.Join(deferErr, cleanupErr) +} + +// Requires startWriting. +// Caller MUST run all returned cleanup functions after this, EVEN IF the function returns an error. +// Ideally outside of the startWriting. +func (r *layerStore) deferredDelete(id string) ([]tempdir.CleanupTempDirFunc, error) { layer, ok := r.lookup(id) if !ok { - return ErrLayerUnknown + return nil, ErrLayerUnknown } id = layer.ID // The layer may already have been explicitly unmounted, but if not, we @@ -2003,13 +2054,14 @@ func (r *layerStore) Delete(id string) error { break } if err != nil { - return err + return nil, err } } - if err := r.deleteInternal(id); err != nil { - return err + cleanFunctions, err := r.internalDelete(id) + if err != nil { + return cleanFunctions, err } - return r.saveFor(layer) + return cleanFunctions, r.saveFor(layer) } // Requires startReading or startWriting. @@ -2039,7 +2091,7 @@ func (r *layerStore) Wipe() error { return r.byid[ids[i]].Created.After(r.byid[ids[j]].Created) }) for _, id := range ids { - if err := r.Delete(id); err != nil { + if err := r.deleteWhileHoldingLock(id); err != nil { return err } } @@ -2571,7 +2623,7 @@ func (r *layerStore) applyDiffFromStagingDirectory(id string, diffOutput *driver } for k, v := range diffOutput.BigData { if err := r.SetBigData(id, k, bytes.NewReader(v)); err != nil { - if err2 := r.Delete(id); err2 != nil { + if err2 := r.deleteWhileHoldingLock(id); err2 != nil { logrus.Errorf("While recovering from a failure to set big data, error deleting layer %#v: %v", id, err2) } return err diff --git a/store.go b/store.go index 073a766f8..10f4d56a8 100644 --- a/store.go +++ b/store.go @@ -22,6 +22,7 @@ import ( drivers "github.com/containers/storage/drivers" "github.com/containers/storage/internal/dedup" + "github.com/containers/storage/internal/tempdir" "github.com/containers/storage/pkg/archive" "github.com/containers/storage/pkg/directory" "github.com/containers/storage/pkg/idtools" @@ -1758,7 +1759,7 @@ func (s *store) imageTopLayerForMapping(image *Image, ristore roImageStore, rlst } // By construction, createMappedLayer can only be true if ristore == s.imageStore. if err = s.imageStore.addMappedTopLayer(image.ID, mappedLayer.ID); err != nil { - if err2 := rlstore.Delete(mappedLayer.ID); err2 != nil { + if err2 := rlstore.deleteWhileHoldingLock(mappedLayer.ID); err2 != nil { err = fmt.Errorf("deleting layer %q: %v: %w", mappedLayer.ID, err2, err) } return nil, fmt.Errorf("registering ID-mapped layer with image %q: %w", image.ID, err) @@ -1943,7 +1944,7 @@ func (s *store) CreateContainer(id string, names []string, image, layer, metadat } container, err := s.containerStore.create(id, names, imageID, layer, &options) if err != nil || container == nil { - if err2 := rlstore.Delete(layer); err2 != nil { + if err2 := rlstore.deleteWhileHoldingLock(layer); err2 != nil { if err == nil { err = fmt.Errorf("deleting layer %#v: %w", layer, err2) } else { @@ -2540,7 +2541,13 @@ func (s *store) Lookup(name string) (string, error) { return "", ErrLayerUnknown } -func (s *store) DeleteLayer(id string) error { +func (s *store) DeleteLayer(id string) (retErr error) { + cleanupFunctions := []tempdir.CleanupTempDirFunc{} + defer func() { + if cleanupErr := tempdir.CleanupTemporaryDirectories(cleanupFunctions...); cleanupErr != nil { + retErr = errors.Join(cleanupErr, retErr) + } + }() return s.writeToAllStores(func(rlstore rwLayerStore) error { if rlstore.Exists(id) { if l, err := rlstore.Get(id); err != nil { @@ -2574,7 +2581,9 @@ func (s *store) DeleteLayer(id string) error { return fmt.Errorf("layer %v used by container %v: %w", id, container.ID, ErrLayerUsedByContainer) } } - if err := rlstore.Delete(id); err != nil { + cf, err := rlstore.deferredDelete(id) + cleanupFunctions = append(cleanupFunctions, cf...) + if err != nil { return fmt.Errorf("delete layer %v: %w", id, err) } @@ -2591,8 +2600,14 @@ func (s *store) DeleteLayer(id string) error { }) } -func (s *store) DeleteImage(id string, commit bool) (layers []string, err error) { +func (s *store) DeleteImage(id string, commit bool) (layers []string, retErr error) { layersToRemove := []string{} + cleanupFunctions := []tempdir.CleanupTempDirFunc{} + defer func() { + if cleanupErr := tempdir.CleanupTemporaryDirectories(cleanupFunctions...); cleanupErr != nil { + retErr = errors.Join(cleanupErr, retErr) + } + }() if err := s.writeToAllStores(func(rlstore rwLayerStore) error { // Delete image from all available imagestores configured to be used. imageFound := false @@ -2698,7 +2713,9 @@ func (s *store) DeleteImage(id string, commit bool) (layers []string, err error) } if commit { for _, layer := range layersToRemove { - if err = rlstore.Delete(layer); err != nil { + cf, err := rlstore.deferredDelete(layer) + cleanupFunctions = append(cleanupFunctions, cf...) + if err != nil { return err } } @@ -2710,7 +2727,13 @@ func (s *store) DeleteImage(id string, commit bool) (layers []string, err error) return layersToRemove, nil } -func (s *store) DeleteContainer(id string) error { +func (s *store) DeleteContainer(id string) (retErr error) { + cleanupFunctions := []tempdir.CleanupTempDirFunc{} + defer func() { + if cleanupErr := tempdir.CleanupTemporaryDirectories(cleanupFunctions...); cleanupErr != nil { + retErr = errors.Join(cleanupErr, retErr) + } + }() return s.writeToAllStores(func(rlstore rwLayerStore) error { if !s.containerStore.Exists(id) { return ErrNotAContainer @@ -2726,7 +2749,9 @@ func (s *store) DeleteContainer(id string) error { // the container record that refers to it, effectively losing // track of it if rlstore.Exists(container.LayerID) { - if err := rlstore.Delete(container.LayerID); err != nil { + cf, err := rlstore.deferredDelete(container.LayerID) + cleanupFunctions = append(cleanupFunctions, cf...) + if err != nil { return err } } @@ -2752,12 +2777,20 @@ func (s *store) DeleteContainer(id string) error { }) } -func (s *store) Delete(id string) error { +func (s *store) Delete(id string) (retErr error) { + cleanupFunctions := []tempdir.CleanupTempDirFunc{} + defer func() { + if cleanupErr := tempdir.CleanupTemporaryDirectories(cleanupFunctions...); cleanupErr != nil { + retErr = errors.Join(cleanupErr, retErr) + } + }() return s.writeToAllStores(func(rlstore rwLayerStore) error { if s.containerStore.Exists(id) { if container, err := s.containerStore.Get(id); err == nil { if rlstore.Exists(container.LayerID) { - if err = rlstore.Delete(container.LayerID); err != nil { + cf, err := rlstore.deferredDelete(container.LayerID) + cleanupFunctions = append(cleanupFunctions, cf...) + if err != nil { return err } if err = s.containerStore.Delete(id); err != nil { @@ -2781,7 +2814,9 @@ func (s *store) Delete(id string) error { return s.imageStore.Delete(id) } if rlstore.Exists(id) { - return rlstore.Delete(id) + cf, err := rlstore.deferredDelete(id) + cleanupFunctions = append(cleanupFunctions, cf...) + return err } return ErrLayerUnknown }) diff --git a/store_test.go b/store_test.go index d8a1ad238..df1342e53 100644 --- a/store_test.go +++ b/store_test.go @@ -569,3 +569,63 @@ func TestStoreMultiList(t *testing.T) { store.Free() } + +func TestStoreDelete(t *testing.T) { + reexec.Init() + + store := newTestStore(t, StoreOptions{}) + + options := MultiListOptions{ + Layers: true, + Images: true, + Containers: true, + } + + expectedResult, err := store.MultiList(options) + require.NoError(t, err) + + _, err = store.CreateLayer("LayerNoUsed", "", []string{"not-used"}, "", false, nil) + require.NoError(t, err) + + _, err = store.CreateLayer("Layer", "", []string{"l1"}, "", false, nil) + require.NoError(t, err) + + _, err = store.CreateImage("Image1", []string{"i1"}, "Layer", "", nil) + require.NoError(t, err) + + _, err = store.CreateImage("Image", []string{"i"}, "Layer", "", nil) + require.NoError(t, err) + + _, err = store.CreateContainer("Container", []string{"c"}, "Image", "", "", nil) + require.NoError(t, err) + + _, err = store.CreateContainer("Container1", []string{"c1"}, "Image1", "", "", nil) + require.NoError(t, err) + + err = store.DeleteContainer("Container") + require.NoError(t, err) + + _, err = store.DeleteImage("Image", true) + require.NoError(t, err) + + err = store.DeleteContainer("Container1") + require.NoError(t, err) + + _, err = store.DeleteImage("Image1", true) + require.NoError(t, err) + + err = store.DeleteLayer("LayerNoUsed") + require.NoError(t, err) + + listResults, err := store.MultiList(options) + require.NoError(t, err) + + require.Equal(t, expectedResult.Layers, listResults.Layers) + require.Equal(t, expectedResult.Containers, listResults.Containers) + require.Equal(t, expectedResult.Images, listResults.Images) + + _, err = store.Shutdown(true) + require.Nil(t, err) + + store.Free() +} diff --git a/userns.go b/userns.go index 9cfd6ea34..117a732ce 100644 --- a/userns.go +++ b/userns.go @@ -202,7 +202,7 @@ outer: return 0, err } defer func() { - if err2 := rlstore.Delete(clayer.ID); err2 != nil { + if err2 := rlstore.deleteWhileHoldingLock(clayer.ID); err2 != nil { if retErr == nil { retErr = fmt.Errorf("deleting temporary layer %#v: %w", clayer.ID, err2) } else {