From 091f854c55e11fe6339a847163f21a3bed81924c Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Mon, 12 Feb 2024 12:20:54 +0100 Subject: [PATCH 1/4] store: split PutLayer this is needed by the following commit. Signed-off-by: Giuseppe Scrivano --- store.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/store.go b/store.go index 4a98f54e09..ef1132ab95 100644 --- a/store.go +++ b/store.go @@ -1421,8 +1421,7 @@ func (s *store) canUseShifting(uidmap, gidmap []idtools.IDMap) bool { return true } -func (s *store) PutLayer(id, parent string, names []string, mountLabel string, writeable bool, lOptions *LayerOptions, diff io.Reader) (*Layer, int64, error) { - var parentLayer *Layer +func (s *store) putLayer(id, parent string, names []string, mountLabel string, writeable bool, lOptions *LayerOptions, diff io.Reader, slo *stagedLayerOptions) (*Layer, int64, error) { rlstore, rlstores, err := s.bothLayerStoreKinds() if err != nil { return nil, -1, err @@ -1435,6 +1434,8 @@ func (s *store) PutLayer(id, parent string, names []string, mountLabel string, w return nil, -1, err } defer s.containerStore.stopWriting() + + var parentLayer *Layer var options LayerOptions if lOptions != nil { options = *lOptions @@ -1511,6 +1512,10 @@ func (s *store) PutLayer(id, parent string, names []string, mountLabel string, w return rlstore.create(id, parentLayer, names, mountLabel, nil, &layerOptions, writeable, diff) } +func (s *store) PutLayer(id, parent string, names []string, mountLabel string, writeable bool, lOptions *LayerOptions, diff io.Reader) (*Layer, int64, error) { + return s.putLayer(id, parent, names, mountLabel, writeable, lOptions, diff, nil) +} + func (s *store) CreateLayer(id, parent string, names []string, mountLabel string, writeable bool, options *LayerOptions) (*Layer, error) { layer, _, err := s.PutLayer(id, parent, names, mountLabel, writeable, options, nil) return layer, err From c6de01cf3114f7e14c516bbba8a0b3626b5e7046 Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Mon, 12 Feb 2024 21:07:20 +0100 Subject: [PATCH 2/4] driver: simplify ApplyDiffFromStagingDirectory enforce that the stagingDirectory must have the same value as the diffOutput.Target variable. It allows to simplify the internal API. Signed-off-by: Giuseppe Scrivano --- drivers/driver.go | 4 ++-- drivers/overlay/overlay.go | 3 ++- layers.go | 8 ++++---- store.go | 6 +++++- 4 files changed, 13 insertions(+), 8 deletions(-) diff --git a/drivers/driver.go b/drivers/driver.go index 2c30df50ee..aa99fdead2 100644 --- a/drivers/driver.go +++ b/drivers/driver.go @@ -249,8 +249,8 @@ type DriverWithDiffer interface { // ApplyDiffWithDiffer applies the changes using the callback function. // If id is empty, then a staging directory is created. The staging directory is guaranteed to be usable with ApplyDiffFromStagingDirectory. ApplyDiffWithDiffer(id, parent string, options *ApplyDiffWithDifferOpts, differ Differ) (output DriverWithDifferOutput, err error) - // ApplyDiffFromStagingDirectory applies the changes using the specified staging directory. - ApplyDiffFromStagingDirectory(id, parent, stagingDirectory string, diffOutput *DriverWithDifferOutput, options *ApplyDiffWithDifferOpts) error + // ApplyDiffFromStagingDirectory applies the changes using the diffOutput target directory. + ApplyDiffFromStagingDirectory(id, parent string, diffOutput *DriverWithDifferOutput, options *ApplyDiffWithDifferOpts) error // CleanupStagingDirectory cleanups the staging directory. It can be used to cleanup the staging directory on errors CleanupStagingDirectory(stagingDirectory string) error // DifferTarget gets the location where files are stored for the layer. diff --git a/drivers/overlay/overlay.go b/drivers/overlay/overlay.go index 0492162833..d3781955d6 100644 --- a/drivers/overlay/overlay.go +++ b/drivers/overlay/overlay.go @@ -2124,7 +2124,8 @@ func (d *Driver) ApplyDiffWithDiffer(id, parent string, options *graphdriver.App } // ApplyDiffFromStagingDirectory applies the changes using the specified staging directory. -func (d *Driver) ApplyDiffFromStagingDirectory(id, parent, stagingDirectory string, diffOutput *graphdriver.DriverWithDifferOutput, options *graphdriver.ApplyDiffWithDifferOpts) error { +func (d *Driver) ApplyDiffFromStagingDirectory(id, parent string, diffOutput *graphdriver.DriverWithDifferOutput, options *graphdriver.ApplyDiffWithDifferOpts) error { + stagingDirectory := diffOutput.Target if filepath.Dir(stagingDirectory) != d.getStagingDir() { return fmt.Errorf("%q is not a staging directory", stagingDirectory) } diff --git a/layers.go b/layers.go index 76b17659d3..5543489b5e 100644 --- a/layers.go +++ b/layers.go @@ -312,8 +312,8 @@ type rwLayerStore interface { // CleanupStagingDirectory cleanups the staging directory. It can be used to cleanup the staging directory on errors CleanupStagingDirectory(stagingDirectory string) error - // ApplyDiffFromStagingDirectory uses stagingDirectory to create the diff. - ApplyDiffFromStagingDirectory(id, stagingDirectory string, diffOutput *drivers.DriverWithDifferOutput, options *drivers.ApplyDiffWithDifferOpts) error + // applyDiffFromStagingDirectory uses diffOutput.Target to create the diff. + applyDiffFromStagingDirectory(id string, diffOutput *drivers.DriverWithDifferOutput, options *drivers.ApplyDiffWithDifferOpts) error // DifferTarget gets the location where files are stored for the layer. DifferTarget(id string) (string, error) @@ -2414,7 +2414,7 @@ func (r *layerStore) DifferTarget(id string) (string, error) { } // Requires startWriting. -func (r *layerStore) ApplyDiffFromStagingDirectory(id, stagingDirectory string, diffOutput *drivers.DriverWithDifferOutput, options *drivers.ApplyDiffWithDifferOpts) error { +func (r *layerStore) applyDiffFromStagingDirectory(id string, diffOutput *drivers.DriverWithDifferOutput, options *drivers.ApplyDiffWithDifferOpts) error { ddriver, ok := r.driver.(drivers.DriverWithDiffer) if !ok { return ErrNotSupported @@ -2433,7 +2433,7 @@ func (r *layerStore) ApplyDiffFromStagingDirectory(id, stagingDirectory string, } } - err := ddriver.ApplyDiffFromStagingDirectory(layer.ID, layer.Parent, stagingDirectory, diffOutput, options) + err := ddriver.ApplyDiffFromStagingDirectory(layer.ID, layer.Parent, diffOutput, options) if err != nil { return err } diff --git a/store.go b/store.go index ef1132ab95..0944804940 100644 --- a/store.go +++ b/store.go @@ -318,6 +318,7 @@ type Store interface { ApplyDiffWithDiffer(to string, options *drivers.ApplyDiffWithDifferOpts, differ drivers.Differ) (*drivers.DriverWithDifferOutput, error) // ApplyDiffFromStagingDirectory uses stagingDirectory to create the diff. + // Deprecated: it will be removed soon. ApplyDiffFromStagingDirectory(to, stagingDirectory string, diffOutput *drivers.DriverWithDifferOutput, options *drivers.ApplyDiffWithDifferOpts) error // CleanupStagingDirectory cleanups the staging directory. It can be used to cleanup the staging directory on errors @@ -2959,11 +2960,14 @@ func (s *store) Diff(from, to string, options *DiffOptions) (io.ReadCloser, erro } func (s *store) ApplyDiffFromStagingDirectory(to, stagingDirectory string, diffOutput *drivers.DriverWithDifferOutput, options *drivers.ApplyDiffWithDifferOpts) error { + if stagingDirectory != diffOutput.Target { + return fmt.Errorf("invalid value for staging directory, it must be the same as the differ target directory") + } _, err := writeToLayerStore(s, func(rlstore rwLayerStore) (struct{}, error) { if !rlstore.Exists(to) { return struct{}{}, ErrLayerUnknown } - return struct{}{}, rlstore.ApplyDiffFromStagingDirectory(to, stagingDirectory, diffOutput, options) + return struct{}{}, rlstore.applyDiffFromStagingDirectory(to, diffOutput, options) }) return err } From 21ed482eca46b32c13e9b93d9792288f28443144 Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Mon, 12 Feb 2024 13:03:42 +0100 Subject: [PATCH 3/4] store: new API ApplyStagedLayer Add a race-condition-free alternative to using CreateLayer and ApplyDiffFromStagingDirectory, ensuring the store is locked for the entire duration while the layer is being created and populated. Signed-off-by: Giuseppe Scrivano --- layers.go | 16 ++++++++++++++-- store.go | 36 ++++++++++++++++++++++++++++++++---- userns.go | 2 +- 3 files changed, 47 insertions(+), 7 deletions(-) diff --git a/layers.go b/layers.go index 5543489b5e..e4f75dfcac 100644 --- a/layers.go +++ b/layers.go @@ -181,6 +181,13 @@ type DiffOptions struct { Compression *archive.Compression } +// stagedLayerOptions are the options passed to .create to populate a staged +// layer +type stagedLayerOptions struct { + DiffOutput *drivers.DriverWithDifferOutput + DiffOptions *drivers.ApplyDiffWithDifferOpts +} + // roLayerStore wraps a graph driver, adding the ability to refer to layers by // name, and keeping track of parent-child relationships, along with a list of // all known layers. @@ -267,7 +274,7 @@ type rwLayerStore interface { // underlying drivers do not themselves distinguish between writeable // and read-only layers. Returns the new layer structure and the size of the // diff which was applied to its parent to initialize its contents. - create(id string, parent *Layer, names []string, mountLabel string, options map[string]string, moreOptions *LayerOptions, writeable bool, diff io.Reader) (*Layer, int64, error) + create(id string, parent *Layer, names []string, mountLabel string, options map[string]string, moreOptions *LayerOptions, writeable bool, diff io.Reader, slo *stagedLayerOptions) (*Layer, int64, error) // updateNames modifies names associated with a layer based on (op, names). updateNames(id string, names []string, op updateNameOperation) error @@ -1232,7 +1239,7 @@ func (r *layerStore) PutAdditionalLayer(id string, parentLayer *Layer, names []s } // Requires startWriting. -func (r *layerStore) create(id string, parentLayer *Layer, names []string, mountLabel string, options map[string]string, moreOptions *LayerOptions, writeable bool, diff io.Reader) (layer *Layer, size int64, err error) { +func (r *layerStore) create(id string, parentLayer *Layer, names []string, mountLabel string, options map[string]string, moreOptions *LayerOptions, writeable bool, diff io.Reader, slo *stagedLayerOptions) (layer *Layer, size int64, err error) { if moreOptions == nil { moreOptions = &LayerOptions{} } @@ -1426,6 +1433,11 @@ func (r *layerStore) create(id string, parentLayer *Layer, names []string, mount cleanupFailureContext = "applying layer diff" return nil, -1, err } + } else if slo != nil { + if err := r.applyDiffFromStagingDirectory(layer.ID, slo.DiffOutput, slo.DiffOptions); err != nil { + cleanupFailureContext = "applying staged directory diff" + return nil, -1, err + } } else { // applyDiffWithOptions() would have updated r.bycompressedsum // and r.byuncompressedsum for us, but if we used a template diff --git a/store.go b/store.go index 0944804940..86604ead5a 100644 --- a/store.go +++ b/store.go @@ -71,6 +71,19 @@ type metadataStore interface { rwMetadataStore } +// ApplyStagedLayerOptions contains options to pass to ApplyStagedLayer +type ApplyStagedLayerOptions struct { + ID string // Mandatory + ParentLayer string // Optional + Names []string // Optional + MountLabel string // Optional + Writeable bool // Optional + LayerOptions *LayerOptions // Optional + + DiffOutput *drivers.DriverWithDifferOutput // Mandatory + DiffOptions *drivers.ApplyDiffWithDifferOpts // Mandatory +} + // An roBigDataStore wraps up the read-only big-data related methods of the // various types of file-based lookaside stores that we implement. type roBigDataStore interface { @@ -318,12 +331,17 @@ type Store interface { ApplyDiffWithDiffer(to string, options *drivers.ApplyDiffWithDifferOpts, differ drivers.Differ) (*drivers.DriverWithDifferOutput, error) // ApplyDiffFromStagingDirectory uses stagingDirectory to create the diff. - // Deprecated: it will be removed soon. + // Deprecated: it will be removed soon. Use ApplyStagedLayer instead. ApplyDiffFromStagingDirectory(to, stagingDirectory string, diffOutput *drivers.DriverWithDifferOutput, options *drivers.ApplyDiffWithDifferOpts) error // CleanupStagingDirectory cleanups the staging directory. It can be used to cleanup the staging directory on errors CleanupStagingDirectory(stagingDirectory string) error + // ApplyStagedLayer combines the functions of CreateLayer and ApplyDiffFromStagingDirectory, + // marking the layer for automatic removal if applying the diff fails + // for any reason. + ApplyStagedLayer(args ApplyStagedLayerOptions) (*Layer, error) + // DifferTarget gets the path to the differ target. DifferTarget(id string) (string, error) @@ -1510,7 +1528,7 @@ func (s *store) putLayer(id, parent string, names []string, mountLabel string, w GIDMap: copyIDMap(gidMap), } } - return rlstore.create(id, parentLayer, names, mountLabel, nil, &layerOptions, writeable, diff) + return rlstore.create(id, parentLayer, names, mountLabel, nil, &layerOptions, writeable, diff, slo) } func (s *store) PutLayer(id, parent string, names []string, mountLabel string, writeable bool, lOptions *LayerOptions, diff io.Reader) (*Layer, int64, error) { @@ -1724,7 +1742,7 @@ func (s *store) imageTopLayerForMapping(image *Image, ristore roImageStore, rlst } } layerOptions.TemplateLayer = layer.ID - mappedLayer, _, err := rlstore.create("", parentLayer, nil, layer.MountLabel, nil, &layerOptions, false, nil) + mappedLayer, _, err := rlstore.create("", parentLayer, nil, layer.MountLabel, nil, &layerOptions, false, nil, nil) if err != nil { return nil, fmt.Errorf("creating an ID-mapped copy of layer %q: %w", layer.ID, err) } @@ -1895,7 +1913,7 @@ func (s *store) CreateContainer(id string, names []string, image, layer, metadat options.Flags[mountLabelFlag] = mountLabel } - clayer, _, err := rlstore.create(layer, imageTopLayer, nil, mlabel, options.StorageOpt, layerOptions, true, nil) + clayer, _, err := rlstore.create(layer, imageTopLayer, nil, mlabel, options.StorageOpt, layerOptions, true, nil, nil) if err != nil { return nil, err } @@ -2972,6 +2990,16 @@ func (s *store) ApplyDiffFromStagingDirectory(to, stagingDirectory string, diffO return err } +func (s *store) ApplyStagedLayer(args ApplyStagedLayerOptions) (*Layer, error) { + slo := stagedLayerOptions{ + DiffOutput: args.DiffOutput, + DiffOptions: args.DiffOptions, + } + + layer, _, err := s.putLayer(args.ID, args.ParentLayer, args.Names, args.MountLabel, args.Writeable, args.LayerOptions, nil, &slo) + return layer, err +} + func (s *store) CleanupStagingDirectory(stagingDirectory string) error { _, err := writeToLayerStore(s, func(rlstore rwLayerStore) (struct{}, error) { return struct{}{}, rlstore.CleanupStagingDirectory(stagingDirectory) diff --git a/userns.go b/userns.go index 945b1d5ad7..57120731be 100644 --- a/userns.go +++ b/userns.go @@ -175,7 +175,7 @@ outer: // We need to create a temporary layer so we can mount it and lookup the // maximum IDs used. - clayer, _, err := rlstore.create("", topLayer, nil, "", nil, layerOptions, false, nil) + clayer, _, err := rlstore.create("", topLayer, nil, "", nil, layerOptions, false, nil, nil) if err != nil { return 0, err } From d36d6c122223355930449736dc3f9dd4598c807e Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Wed, 14 Feb 2024 10:27:52 +0100 Subject: [PATCH 4/4] store: new API CleanupStagedLayer It uses the diff output as input and callers are not expected to know about the Target directory. Signed-off-by: Giuseppe Scrivano --- store.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/store.go b/store.go index 86604ead5a..ad93979220 100644 --- a/store.go +++ b/store.go @@ -335,6 +335,7 @@ type Store interface { ApplyDiffFromStagingDirectory(to, stagingDirectory string, diffOutput *drivers.DriverWithDifferOutput, options *drivers.ApplyDiffWithDifferOpts) error // CleanupStagingDirectory cleanups the staging directory. It can be used to cleanup the staging directory on errors + // Deprecated: it will be removed soon. Use CleanupStagedLayer instead. CleanupStagingDirectory(stagingDirectory string) error // ApplyStagedLayer combines the functions of CreateLayer and ApplyDiffFromStagingDirectory, @@ -342,6 +343,9 @@ type Store interface { // for any reason. ApplyStagedLayer(args ApplyStagedLayerOptions) (*Layer, error) + // CleanupStagedLayer cleanups the staging directory. It can be used to cleanup the staging directory on errors + CleanupStagedLayer(diffOutput *drivers.DriverWithDifferOutput) error + // DifferTarget gets the path to the differ target. DifferTarget(id string) (string, error) @@ -3007,6 +3011,13 @@ func (s *store) CleanupStagingDirectory(stagingDirectory string) error { return err } +func (s *store) CleanupStagedLayer(diffOutput *drivers.DriverWithDifferOutput) error { + _, err := writeToLayerStore(s, func(rlstore rwLayerStore) (struct{}, error) { + return struct{}{}, rlstore.CleanupStagingDirectory(diffOutput.Target) + }) + return err +} + func (s *store) ApplyDiffWithDiffer(to string, options *drivers.ApplyDiffWithDifferOpts, differ drivers.Differ) (*drivers.DriverWithDifferOutput, error) { return writeToLayerStore(s, func(rlstore rwLayerStore) (*drivers.DriverWithDifferOutput, error) { if to != "" && !rlstore.Exists(to) {