Integrate the temporary directory into layer deletion

This enables deferred deletion of physical files outside of global locks, improving performance and reducing lock contention.

Signed-off-by: Jan Rodák <hony.com@seznam.cz>
This commit is contained in:
Jan Rodák 2025-06-11 13:09:30 +02:00
parent 413d9f16ec
commit f9aef81079
No known key found for this signature in database
GPG Key ID: D458A9B20435C2BF
11 changed files with 333 additions and 45 deletions

View File

@ -36,6 +36,7 @@ import (
"time" "time"
graphdriver "github.com/containers/storage/drivers" graphdriver "github.com/containers/storage/drivers"
"github.com/containers/storage/internal/tempdir"
"github.com/containers/storage/pkg/archive" "github.com/containers/storage/pkg/archive"
"github.com/containers/storage/pkg/chrootarchive" "github.com/containers/storage/pkg/chrootarchive"
"github.com/containers/storage/pkg/directory" "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) { func (a *Driver) Dedup(req graphdriver.DedupArgs) (graphdriver.DedupResult, error) {
return graphdriver.DedupResult{}, nil 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{}
}

View File

@ -30,6 +30,7 @@ import (
"unsafe" "unsafe"
graphdriver "github.com/containers/storage/drivers" graphdriver "github.com/containers/storage/drivers"
"github.com/containers/storage/internal/tempdir"
"github.com/containers/storage/pkg/directory" "github.com/containers/storage/pkg/directory"
"github.com/containers/storage/pkg/fileutils" "github.com/containers/storage/pkg/fileutils"
"github.com/containers/storage/pkg/idtools" "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) { func (d *Driver) Dedup(req graphdriver.DedupArgs) (graphdriver.DedupResult, error) {
return graphdriver.DedupResult{}, nil 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{}
}

View File

@ -9,6 +9,7 @@ import (
"strings" "strings"
"github.com/containers/storage/internal/dedup" "github.com/containers/storage/internal/dedup"
"github.com/containers/storage/internal/tempdir"
"github.com/containers/storage/pkg/archive" "github.com/containers/storage/pkg/archive"
"github.com/containers/storage/pkg/directory" "github.com/containers/storage/pkg/directory"
"github.com/containers/storage/pkg/fileutils" "github.com/containers/storage/pkg/fileutils"
@ -123,7 +124,17 @@ type ProtoDriver interface {
// and parent, with contents identical to the specified template layer. // 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 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. // 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 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 // Get returns the mountpoint for the layered filesystem referred
// to by this id. You can optionally specify a mountLabel or "". // to by this id. You can optionally specify a mountLabel or "".
// Optionally it gets the mappings used to create the layer. // Optionally it gets the mappings used to create the layer.

View File

@ -24,6 +24,7 @@ import (
"github.com/containers/storage/drivers/quota" "github.com/containers/storage/drivers/quota"
"github.com/containers/storage/internal/dedup" "github.com/containers/storage/internal/dedup"
"github.com/containers/storage/internal/staging_lockfile" "github.com/containers/storage/internal/staging_lockfile"
"github.com/containers/storage/internal/tempdir"
"github.com/containers/storage/pkg/archive" "github.com/containers/storage/pkg/archive"
"github.com/containers/storage/pkg/chrootarchive" "github.com/containers/storage/pkg/chrootarchive"
"github.com/containers/storage/pkg/directory" "github.com/containers/storage/pkg/directory"
@ -80,10 +81,11 @@ const (
// that mounts do not fail due to length. // that mounts do not fail due to length.
const ( const (
linkDir = "l" linkDir = "l"
stagingDir = "staging" stagingDir = "staging"
lowerFile = "lower" tempDirName = "tempdirs"
maxDepth = 500 lowerFile = "lower"
maxDepth = 500
stagingLockFile = "staging.lock" 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. // Remove cleans the directories that are created for this id.
func (d *Driver) Remove(id string) error { 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) dir := d.dir(id)
lid, err := os.ReadFile(path.Join(dir, "link")) lid, err := os.ReadFile(path.Join(dir, "link"))
if err == nil { 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) logrus.Debugf("Failed to remove link: %v", err)
} }
} }
d.releaseAdditionalLayerByID(id) d.releaseAdditionalLayerByID(id)
if err := system.EnsureRemoveAll(dir); err != nil && !os.IsNotExist(err) { if err := cleanup(dir); err != nil && !os.IsNotExist(err) {
return err return err
} }
if d.quotaCtl != nil { if d.quotaCtl != nil {
@ -1327,6 +1334,41 @@ func (d *Driver) Remove(id string) error {
return nil 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 // 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 // under each layer has a symlink created for it under the linkDir. If the symlink does not
// exist, it creates them // 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 // 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. // the layer's "link" file that points to the layer's "diff" directory.
for _, dir := range dirs { for _, dir := range dirs {
// Skip over the linkDir and anything that is not a directory // Skip over the linkDir, stagingDir, tempDirName and anything that is not a directory
if dir.Name() == linkDir || !dir.IsDir() { if dir.Name() == linkDir || dir.Name() == stagingDir || dir.Name() == tempDirName || !dir.IsDir() {
continue continue
} }
// Read the "link" file under each layer to get the name of the symlink // 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 { for _, entry := range entries {
id := entry.Name() id := entry.Name()
switch id { switch id {
case linkDir, stagingDir, quota.BackingFsBlockDeviceLink, mountProgramFlagFile: case linkDir, stagingDir, tempDirName, quota.BackingFsBlockDeviceLink, mountProgramFlagFile:
// expected, but not a layer. skip it // expected, but not a layer. skip it
continue continue
default: default:

View File

@ -11,6 +11,7 @@ import (
graphdriver "github.com/containers/storage/drivers" graphdriver "github.com/containers/storage/drivers"
"github.com/containers/storage/internal/dedup" "github.com/containers/storage/internal/dedup"
"github.com/containers/storage/internal/tempdir"
"github.com/containers/storage/pkg/archive" "github.com/containers/storage/pkg/archive"
"github.com/containers/storage/pkg/directory" "github.com/containers/storage/pkg/directory"
"github.com/containers/storage/pkg/fileutils" "github.com/containers/storage/pkg/fileutils"
@ -22,7 +23,10 @@ import (
"github.com/vbatts/tar-split/tar/storage" "github.com/vbatts/tar-split/tar/storage"
) )
const defaultPerms = os.FileMode(0o555) const (
defaultPerms = os.FileMode(0o555)
tempDirName = "tempdirs"
)
func init() { func init() {
graphdriver.MustRegister("vfs", Init) graphdriver.MustRegister("vfs", Init)
@ -244,6 +248,42 @@ func (d *Driver) Remove(id string) error {
return system.EnsureRemoveAll(d.dir(id)) 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. // Get returns the directory for the given id.
func (d *Driver) Get(id string, options graphdriver.MountOpts) (_ string, retErr error) { func (d *Driver) Get(id string, options graphdriver.MountOpts) (_ string, retErr error) {
dir := d.dir(id) dir := d.dir(id)

View File

@ -24,6 +24,7 @@ import (
"github.com/Microsoft/go-winio/backuptar" "github.com/Microsoft/go-winio/backuptar"
"github.com/Microsoft/hcsshim" "github.com/Microsoft/hcsshim"
graphdriver "github.com/containers/storage/drivers" graphdriver "github.com/containers/storage/drivers"
"github.com/containers/storage/internal/tempdir"
"github.com/containers/storage/pkg/archive" "github.com/containers/storage/pkg/archive"
"github.com/containers/storage/pkg/directory" "github.com/containers/storage/pkg/directory"
"github.com/containers/storage/pkg/fileutils" "github.com/containers/storage/pkg/fileutils"
@ -1014,3 +1015,14 @@ func parseStorageOpt(storageOpt map[string]string) (*storageOptions, error) {
} }
return &options, nil 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{}
}

View File

@ -13,6 +13,7 @@ import (
"time" "time"
graphdriver "github.com/containers/storage/drivers" graphdriver "github.com/containers/storage/drivers"
"github.com/containers/storage/internal/tempdir"
"github.com/containers/storage/pkg/directory" "github.com/containers/storage/pkg/directory"
"github.com/containers/storage/pkg/idtools" "github.com/containers/storage/pkg/idtools"
"github.com/containers/storage/pkg/mount" "github.com/containers/storage/pkg/mount"
@ -406,6 +407,12 @@ func (d *Driver) Remove(id string) error {
return nil 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. // 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) { func (d *Driver) Get(id string, options graphdriver.MountOpts) (_ string, retErr error) {
mountpoint := d.mountPath(id) mountpoint := d.mountPath(id)
@ -516,3 +523,8 @@ func (d *Driver) AdditionalImageStores() []string {
func (d *Driver) Dedup(req graphdriver.DedupArgs) (graphdriver.DedupResult, error) { func (d *Driver) Dedup(req graphdriver.DedupArgs) (graphdriver.DedupResult, error) {
return graphdriver.DedupResult{}, nil return graphdriver.DedupResult{}, nil
} }
// GetTempDirRootDirs is not implemented.
func (d *Driver) GetTempDirRootDirs() []string {
return []string{}
}

View File

@ -18,6 +18,7 @@ import (
"time" "time"
drivers "github.com/containers/storage/drivers" drivers "github.com/containers/storage/drivers"
"github.com/containers/storage/internal/tempdir"
"github.com/containers/storage/pkg/archive" "github.com/containers/storage/pkg/archive"
"github.com/containers/storage/pkg/idtools" "github.com/containers/storage/pkg/idtools"
"github.com/containers/storage/pkg/ioutils" "github.com/containers/storage/pkg/ioutils"
@ -38,6 +39,8 @@ import (
const ( const (
tarSplitSuffix = ".tar-split.gz" tarSplitSuffix = ".tar-split.gz"
// tempDirPath is the subdirectory name used for storing temporary directories during layer deletion
tempDirPath = "tmp"
incompleteFlag = "incomplete" incompleteFlag = "incomplete"
// maxLayerStoreCleanupIterations is the number of times we try to clean up inconsistent layer store state // 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) // 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 modifies names associated with a layer based on (op, names).
updateNames(id string, names []string, op updateNameOperation) error updateNames(id string, names []string, op updateNameOperation) error
// Delete deletes a layer with the specified name or ID. // deleteWhileHoldingLock deletes a layer with the specified name or ID.
Delete(id string) error 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 deletes all layers.
Wipe() error Wipe() error
@ -794,6 +803,17 @@ func (r *layerStore) load(lockedForWriting bool) (bool, error) {
layers := []*Layer{} layers := []*Layer{}
ids := make(map[string]*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 { for locationIndex := range numLayerLocationIndex {
location := layerLocationFromIndex(locationIndex) location := layerLocationFromIndex(locationIndex)
rpath := r.jsonPath[locationIndex] rpath := r.jsonPath[locationIndex]
@ -935,7 +955,12 @@ func (r *layerStore) load(lockedForWriting bool) (bool, error) {
// Now actually delete the layers // Now actually delete the layers
for _, layer := range layersToDelete { for _, layer := range layersToDelete {
logrus.Warnf("Found incomplete layer %q, deleting it", layer.ID) 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 { if err != nil {
// Don't return the error immediately, because deleteInternal does not saveLayers(); // 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 // 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) r.bytocsum[layer.TOCDigest] = append(r.bytocsum[layer.TOCDigest], layer.ID)
} }
if err := r.saveFor(layer); err != nil { 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) logrus.Errorf("While recovering from a failure to save layers, error deleting layer %#v: %v", id, e)
} }
return nil, err return nil, err
@ -1469,7 +1494,7 @@ func (r *layerStore) create(id string, parentLayer *Layer, names []string, mount
if cleanupFailureContext == "" { if cleanupFailureContext == "" {
cleanupFailureContext = "unknown: cleanupFailureContext not set at the failure site" 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) 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. // 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() { 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) layer, ok := r.lookup(id)
if !ok { if !ok {
return ErrLayerUnknown return nil, ErrLayerUnknown
} }
// Ensure that if we are interrupted, the layer will be cleaned up. // Ensure that if we are interrupted, the layer will be cleaned up.
if !layerHasIncompleteFlag(layer) { if !layerHasIncompleteFlag(layer) {
@ -1935,16 +1962,30 @@ func (r *layerStore) deleteInternal(id string) error {
} }
layer.Flags[incompleteFlag] = true layer.Flags[incompleteFlag] = true
if err := r.saveFor(layer); err != nil { 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. // We never unset incompleteFlag; below, we remove the entire object from r.layers.
id = layer.ID tempDirectory, err := tempdir.NewTempDir(filepath.Join(r.layerdir, tempDirPath))
if err := r.driver.Remove(id); err != nil && !errors.Is(err, os.ErrNotExist) { cleanFunctions := []tempdir.CleanupTempDirFunc{}
return err 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) delete(r.byid, id)
for _, name := range layer.Names { for _, name := range layer.Names {
delete(r.byname, name) delete(r.byname, name)
@ -1968,7 +2009,7 @@ func (r *layerStore) deleteInternal(id string) error {
}) { }) {
selinux.ReleaseLabel(mountLabel) selinux.ReleaseLabel(mountLabel)
} }
return nil return cleanFunctions, nil
} }
// Requires startWriting. // Requires startWriting.
@ -1988,10 +2029,20 @@ func (r *layerStore) deleteInDigestMap(id string) {
} }
// Requires startWriting. // 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) layer, ok := r.lookup(id)
if !ok { if !ok {
return ErrLayerUnknown return nil, ErrLayerUnknown
} }
id = layer.ID id = layer.ID
// The layer may already have been explicitly unmounted, but if not, we // The layer may already have been explicitly unmounted, but if not, we
@ -2003,13 +2054,14 @@ func (r *layerStore) Delete(id string) error {
break break
} }
if err != nil { if err != nil {
return err return nil, err
} }
} }
if err := r.deleteInternal(id); err != nil { cleanFunctions, err := r.internalDelete(id)
return err if err != nil {
return cleanFunctions, err
} }
return r.saveFor(layer) return cleanFunctions, r.saveFor(layer)
} }
// Requires startReading or startWriting. // 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) return r.byid[ids[i]].Created.After(r.byid[ids[j]].Created)
}) })
for _, id := range ids { for _, id := range ids {
if err := r.Delete(id); err != nil { if err := r.deleteWhileHoldingLock(id); err != nil {
return err return err
} }
} }
@ -2571,7 +2623,7 @@ func (r *layerStore) applyDiffFromStagingDirectory(id string, diffOutput *driver
} }
for k, v := range diffOutput.BigData { for k, v := range diffOutput.BigData {
if err := r.SetBigData(id, k, bytes.NewReader(v)); err != nil { 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) logrus.Errorf("While recovering from a failure to set big data, error deleting layer %#v: %v", id, err2)
} }
return err return err

View File

@ -22,6 +22,7 @@ import (
drivers "github.com/containers/storage/drivers" drivers "github.com/containers/storage/drivers"
"github.com/containers/storage/internal/dedup" "github.com/containers/storage/internal/dedup"
"github.com/containers/storage/internal/tempdir"
"github.com/containers/storage/pkg/archive" "github.com/containers/storage/pkg/archive"
"github.com/containers/storage/pkg/directory" "github.com/containers/storage/pkg/directory"
"github.com/containers/storage/pkg/idtools" "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. // By construction, createMappedLayer can only be true if ristore == s.imageStore.
if err = s.imageStore.addMappedTopLayer(image.ID, mappedLayer.ID); err != nil { 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) 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) 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) container, err := s.containerStore.create(id, names, imageID, layer, &options)
if err != nil || container == nil { if err != nil || container == nil {
if err2 := rlstore.Delete(layer); err2 != nil { if err2 := rlstore.deleteWhileHoldingLock(layer); err2 != nil {
if err == nil { if err == nil {
err = fmt.Errorf("deleting layer %#v: %w", layer, err2) err = fmt.Errorf("deleting layer %#v: %w", layer, err2)
} else { } else {
@ -2540,7 +2541,13 @@ func (s *store) Lookup(name string) (string, error) {
return "", ErrLayerUnknown 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 { return s.writeToAllStores(func(rlstore rwLayerStore) error {
if rlstore.Exists(id) { if rlstore.Exists(id) {
if l, err := rlstore.Get(id); err != nil { 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) 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) 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{} 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 { if err := s.writeToAllStores(func(rlstore rwLayerStore) error {
// Delete image from all available imagestores configured to be used. // Delete image from all available imagestores configured to be used.
imageFound := false imageFound := false
@ -2698,7 +2713,9 @@ func (s *store) DeleteImage(id string, commit bool) (layers []string, err error)
} }
if commit { if commit {
for _, layer := range layersToRemove { 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 return err
} }
} }
@ -2710,7 +2727,13 @@ func (s *store) DeleteImage(id string, commit bool) (layers []string, err error)
return layersToRemove, nil 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 { return s.writeToAllStores(func(rlstore rwLayerStore) error {
if !s.containerStore.Exists(id) { if !s.containerStore.Exists(id) {
return ErrNotAContainer return ErrNotAContainer
@ -2726,7 +2749,9 @@ func (s *store) DeleteContainer(id string) error {
// the container record that refers to it, effectively losing // the container record that refers to it, effectively losing
// track of it // track of it
if rlstore.Exists(container.LayerID) { 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 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 { return s.writeToAllStores(func(rlstore rwLayerStore) error {
if s.containerStore.Exists(id) { if s.containerStore.Exists(id) {
if container, err := s.containerStore.Get(id); err == nil { if container, err := s.containerStore.Get(id); err == nil {
if rlstore.Exists(container.LayerID) { 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 return err
} }
if err = s.containerStore.Delete(id); err != nil { if err = s.containerStore.Delete(id); err != nil {
@ -2781,7 +2814,9 @@ func (s *store) Delete(id string) error {
return s.imageStore.Delete(id) return s.imageStore.Delete(id)
} }
if rlstore.Exists(id) { if rlstore.Exists(id) {
return rlstore.Delete(id) cf, err := rlstore.deferredDelete(id)
cleanupFunctions = append(cleanupFunctions, cf...)
return err
} }
return ErrLayerUnknown return ErrLayerUnknown
}) })

View File

@ -569,3 +569,63 @@ func TestStoreMultiList(t *testing.T) {
store.Free() 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()
}

View File

@ -202,7 +202,7 @@ outer:
return 0, err return 0, err
} }
defer func() { defer func() {
if err2 := rlstore.Delete(clayer.ID); err2 != nil { if err2 := rlstore.deleteWhileHoldingLock(clayer.ID); err2 != nil {
if retErr == nil { if retErr == nil {
retErr = fmt.Errorf("deleting temporary layer %#v: %w", clayer.ID, err2) retErr = fmt.Errorf("deleting temporary layer %#v: %w", clayer.ID, err2)
} else { } else {