Skip to content

Commit

Permalink
imagebuildah: change when we decide to create layers
Browse files Browse the repository at this point in the history 
Change our strategy for creating layers from:
 * Always create layers for ADD, COPY, and RUN
to:
 * Create layers for ADD, COPY, RUN, and WORKDIR if
   they made changes to the filesystem.  This includes
   creating the WORKDIR directory if it doesn't already
   exist.

It's often a subtle difference, but it fixes cases where a WORKDIR
instruction wasn't followed by RUN, so we didn't end up accidentally
creating it ourselves.

We're not perfect here, because our RUN creates targets for bind mounts
that `docker build` avoids, but we're closer than we were before.

Signed-off-by: Nalin Dahyabhai <[email protected]>
  • Loading branch information
@nalind
nalind committed Nov 6, 2020
1 parent 9148b3c commit 5ec97be
Show file tree
Hide file tree
Showing 3 changed files with 92 additions and 33 deletions.
3 changes: 3 additions & 0 deletions commit.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,9 @@ type CommitOptions struct {
// EmptyLayer tells the builder to omit the diff for the working
// container.
EmptyLayer bool
// EmptyLayerIfRedundant tells the builder to compute the diff for the
// working container, and to omit the layer if the diff is empty.
EmptyLayerIfRedundant bool
// OmitTimestamp forces epoch 0 as created timestamp to allow for
// deterministic, content-addressable builds.
// Deprecated use HistoryTimestamp instead.
Expand Down
19 changes: 19 additions & 0 deletions image.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ type containerImageRef struct {
exporting bool
squash bool
emptyLayer bool
emptyLayerIfRedundant bool
idMappingOptions *IDMappingOptions
parent string
blobDirectory string
Expand Down Expand Up @@ -388,6 +389,18 @@ func (i *containerImageRef) NewImageSource(ctx context.Context, sc *types.System
return nil, errors.Wrapf(err, "error compressing %s", what)
}
writer := io.MultiWriter(writeCloser, srcHasher.Hash())
// Set up to check if the layer diff contains any changes.
skipCurrentLayer := i.emptyLayerIfRedundant && layerID == i.layerID
if skipCurrentLayer {
nestedWriteCloser := ioutils.NewWriteCloserWrapper(writer, writeCloser.Close)
writeCloser = newTarFilterer(nestedWriteCloser, func(hdr *tar.Header) (bool, bool, io.Reader) {
// the layer blob would be empty if there are no items, in which case
// we'll never be called
skipCurrentLayer = false
return false, false, nil
})
writer = io.Writer(writeCloser)
}
// Use specified timestamps in the layer, if we're doing that for
// history entries.
if i.created != nil {
Expand Down Expand Up @@ -433,6 +446,11 @@ func (i *containerImageRef) NewImageSource(ctx context.Context, sc *types.System
if err = os.Rename(filepath.Join(path, "layer"), finalBlobName); err != nil {
return nil, errors.Wrapf(err, "error storing %s to file while renaming %q to %q", what, filepath.Join(path, "layer"), finalBlobName)
}
// If we're skipping this layer if its empty, bail out of adding the layer.
if skipCurrentLayer {
i.emptyLayer = true
continue
}
// Add a note in the manifest about the layer. The blobs are identified by their possibly-
// compressed blob digests.
olayerDescriptor := v1.Descriptor{
Expand Down Expand Up @@ -749,6 +767,7 @@ func (b *Builder) makeImageRef(options CommitOptions, exporting bool) (types.Ima
exporting: exporting,
squash: options.Squash,
emptyLayer: options.EmptyLayer && !options.Squash,
emptyLayerIfRedundant: options.EmptyLayerIfRedundant && !options.Squash,
idMappingOptions: &b.IDMappingOptions,
parent: parent,
blobDirectory: options.BlobDirectory,
Expand Down
103 changes: 70 additions & 33 deletions imagebuildah/stage_executor.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -563,11 +563,27 @@ func (s *StageExecutor) Delete() (err error) {
return err
}

// stepRequiresLayer indicates whether or not the step should be followed by
// committing a layer container when creating an intermediate image.
// stepRequiresLayer indicates that a step must be -- and will always be --
// accompanied by a layer when creating an intermediate image.
func (*StageExecutor) stepRequiresLayer(step *imagebuilder.Step) bool {
return false
}

// stepOptionalLayer indicates that a step may or may not have a corresponding
// layer in an intermediate image.
func (*StageExecutor) stepOptionalLayer(step *imagebuilder.Step) bool {
switch strings.ToUpper(step.Command) {
case "ADD", "COPY", "RUN", "WORKDIR":
return true
}
return false
}

// stepAffectsConfig indicates that a step alters the image's RunConfig
func (*StageExecutor) stepAffectsConfig(step *imagebuilder.Step) bool {
switch strings.ToUpper(step.Command) {
case "ADD", "COPY", "RUN":
case "ARG", "CMD", "ENTRYPOINT", "ENV", "EXPOSE", "HEALTHCHECK", "LABEL", "MAINTAINER",
"ONBUILD", "SHELL", "STOPSIGNAL", "USER", "VOLUME", "WORKDIR":
return true
}
return false
Expand Down Expand Up @@ -684,14 +700,14 @@ func (s *StageExecutor) Execute(ctx context.Context, base string) (imgID string,
// squash the contents of the base image. Whichever is
// the case, we need to commit() to create a new image.
logCommit(s.output, -1)
if imgID, ref, err = s.commit(ctx, s.getCreatedBy(nil, ""), false, s.output); err != nil {
if imgID, ref, err = s.commit(ctx, s.getCreatedBy(nil, ""), false, false, s.output); err != nil {
return "", nil, errors.Wrapf(err, "error committing base container")
}
} else if len(s.executor.labels) > 0 || len(s.executor.annotations) > 0 {
// The image would be modified by the labels passed
// via the command line, so we need to commit.
logCommit(s.output, -1)
if imgID, ref, err = s.commit(ctx, s.getCreatedBy(stage.Node, ""), true, s.output); err != nil {
if imgID, ref, err = s.commit(ctx, s.getCreatedBy(stage.Node, ""), true, false, s.output); err != nil {
return "", nil, err
}
} else {
Expand Down Expand Up @@ -803,7 +819,7 @@ func (s *StageExecutor) Execute(ctx context.Context, base string) (imgID string,
// stage.
if lastStage || imageIsUsedLater {
logCommit(s.output, i)
imgID, ref, err = s.commit(ctx, s.getCreatedBy(node, addedContentSummary), false, s.output)
imgID, ref, err = s.commit(ctx, s.getCreatedBy(node, addedContentSummary), false, false, s.output)
if err != nil {
return "", nil, errors.Wrapf(err, "error committing container for step %+v", *step)
}
Expand Down Expand Up @@ -834,7 +850,7 @@ func (s *StageExecutor) Execute(ctx context.Context, base string) (imgID string,
// has the same change that we're about to make, so far as we
// can tell.
if checkForLayers {
cacheID, err = s.intermediateImageExists(ctx, node, addedContentSummary, s.stepRequiresLayer(step))
cacheID, err = s.intermediateImageExists(ctx, node, addedContentSummary, step)
if err != nil {
return "", nil, errors.Wrap(err, "error checking if cached image exists from a previous build")
}
Expand Down Expand Up @@ -865,7 +881,7 @@ func (s *StageExecutor) Execute(ctx context.Context, base string) (imgID string,
// Check if there's already an image based on our parent that
// has the same change that we just made.
if checkForLayers {
cacheID, err = s.intermediateImageExists(ctx, node, addedContentSummary, s.stepRequiresLayer(step))
cacheID, err = s.intermediateImageExists(ctx, node, addedContentSummary, step)
if err != nil {
return "", nil, errors.Wrap(err, "error checking if cached image exists from a previous build")
}
Expand All @@ -877,7 +893,7 @@ func (s *StageExecutor) Execute(ctx context.Context, base string) (imgID string,
// last cache image will be all that we need, since we
// still don't want to restart using the image's
// configuration blob.
if !s.stepRequiresLayer(step) {
if s.stepAffectsConfig(step) {
err := ib.Run(step, s, noRunsRemaining)
if err != nil {
logrus.Debugf("%v", errors.Wrapf(err, "error building at step %+v", *step))
Expand Down Expand Up @@ -906,7 +922,7 @@ func (s *StageExecutor) Execute(ctx context.Context, base string) (imgID string,
// Create a new image, maybe with a new layer, with the
// name for this stage if it's the last instruction.
logCommit(s.output, i)
imgID, ref, err = s.commit(ctx, s.getCreatedBy(node, addedContentSummary), !s.stepRequiresLayer(step), commitName)
imgID, ref, err = s.commit(ctx, s.getCreatedBy(node, addedContentSummary), !s.stepRequiresLayer(step) && !s.stepOptionalLayer(step), s.stepOptionalLayer(step), commitName)
if err != nil {
return "", nil, errors.Wrapf(err, "error committing container for step %+v", *step)
}
Expand Down Expand Up @@ -971,7 +987,7 @@ func historyEntriesEqual(base, derived v1.History) bool {
// that we're comparing.
// Used to verify whether a cache of the intermediate image exists and whether
// to run the build again.
func (s *StageExecutor) historyAndDiffIDsMatch(baseHistory []v1.History, baseDiffIDs []digest.Digest, child *parser.Node, history []v1.History, diffIDs []digest.Digest, addedContentSummary string, buildAddsLayer bool) bool {
func (s *StageExecutor) historyAndDiffIDsMatch(baseHistory []v1.History, baseDiffIDs []digest.Digest, child *parser.Node, history []v1.History, diffIDs []digest.Digest, addedContentSummary string, buildAddsLayer, buildSometimesAddsLayer bool) bool {
// our history should be as long as the base's, plus one entry for what
// we're doing
if len(history) != len(baseHistory)+1 {
Expand All @@ -991,17 +1007,25 @@ func (s *StageExecutor) historyAndDiffIDsMatch(baseHistory []v1.History, baseDif
if len(baseDiffIDs) != expectedDiffIDs {
return false
}
if buildAddsLayer {
// we're adding a layer, so we should have exactly one more
// layer than the base image
if len(diffIDs) != expectedDiffIDs+1 {
if buildSometimesAddsLayer {
// we might add a layer, so we should have exactly the same
// number of layers as, or one more than, the base image
if len(diffIDs) != expectedDiffIDs+1 && len(diffIDs) != expectedDiffIDs {
return false
}
} else {
// we're not adding a layer, so we should have exactly the same
// layers as the base image
if len(diffIDs) != expectedDiffIDs {
return false
if buildAddsLayer {
// we only ever add a layer, so we should have exactly one more
// layer than the base image
if len(diffIDs) != expectedDiffIDs+1 {
return false
}
} else {
// we're not adding a layer, no chance, so we should have exactly
// the same number of layers as the base image
if len(diffIDs) != expectedDiffIDs {
return false
}
}
}
// compare the diffs for the layers that we should have in common
Expand Down Expand Up @@ -1101,7 +1125,9 @@ func (s *StageExecutor) tagExistingImage(ctx context.Context, cacheID, output st

// intermediateImageExists returns true if an intermediate image of currNode exists in the image store from a previous build.
// It verifies this by checking the parent of the top layer of the image and the history.
func (s *StageExecutor) intermediateImageExists(ctx context.Context, currNode *parser.Node, addedContentDigest string, buildAddsLayer bool) (string, error) {
func (s *StageExecutor) intermediateImageExists(ctx context.Context, currNode *parser.Node, addedContentDigest string, step *imagebuilder.Step) (string, error) {
buildAddsLayer := s.stepRequiresLayer(step) || s.stepOptionalLayer(step)
buildSometimesAddsLayer := !s.stepRequiresLayer(step) || s.stepOptionalLayer(step)
// Get the list of images available in the image store
images, err := s.executor.store.Images()
if err != nil {
Expand All @@ -1117,27 +1143,37 @@ func (s *StageExecutor) intermediateImageExists(ctx context.Context, currNode *p
}
for _, image := range images {
var imageTopLayer *storage.Layer
var imageParentLayerID string
var imageParentLayerID []string
if image.TopLayer != "" {
imageTopLayer, err = s.executor.store.Layer(image.TopLayer)
if err != nil {
return "", errors.Wrapf(err, "error getting top layer info")
}
// Figure out which layer from this image we should
// compare our container's base layer to.
imageParentLayerID = imageTopLayer.ID
// If we haven't added a layer here, then our base
// layer should be the same as the image's layer. If
// did add a layer, then our base layer should be the
// same as the parent of the image's layer.
if buildAddsLayer {
imageParentLayerID = imageTopLayer.Parent
// If we don't add a layer here, then our base layer should be the same as the
// image's layer. If do add a layer, then our base layer should be the same as
// the parent of the image's layer.
if buildSometimesAddsLayer {
imageParentLayerID = append(imageParentLayerID, imageTopLayer.Parent)
imageParentLayerID = append(imageParentLayerID, imageTopLayer.ID)
} else if buildAddsLayer {
imageParentLayerID = append(imageParentLayerID, imageTopLayer.Parent)
} else {
imageParentLayerID = append(imageParentLayerID, imageTopLayer.ID)
}
} else {
imageParentLayerID = append(imageParentLayerID, "")
}
// If the parent of the top layer of an image is equal to the current build image's top layer,
// it means that this image is potentially a cached intermediate image from a previous
// build.
if s.builder.TopLayer != imageParentLayerID {
layerLooksPossible := false
for _, layer := range imageParentLayerID {
if s.builder.TopLayer == layer {
layerLooksPossible = true
break
}
}
if !layerLooksPossible {
continue
}
// Next we double check that the history of this image is equivalent to the previous
Expand All @@ -1152,7 +1188,7 @@ func (s *StageExecutor) intermediateImageExists(ctx context.Context, currNode *p
continue
}
// children + currNode is the point of the Dockerfile we are currently at.
if s.historyAndDiffIDsMatch(baseHistory, baseDiffIDs, currNode, history, diffIDs, addedContentDigest, buildAddsLayer) {
if s.historyAndDiffIDsMatch(baseHistory, baseDiffIDs, currNode, history, diffIDs, addedContentDigest, buildAddsLayer, buildSometimesAddsLayer) {
return image.ID, nil
}
}
Expand All @@ -1161,7 +1197,7 @@ func (s *StageExecutor) intermediateImageExists(ctx context.Context, currNode *p

// commit writes the container's contents to an image, using a passed-in tag as
// the name if there is one, generating a unique ID-based one otherwise.
func (s *StageExecutor) commit(ctx context.Context, createdBy string, emptyLayer bool, output string) (string, reference.Canonical, error) {
func (s *StageExecutor) commit(ctx context.Context, createdBy string, emptyLayer, emptyLayerIfRedundant bool, output string) (string, reference.Canonical, error) {
ib := s.stage.Builder
var imageRef types.ImageReference
if output != "" {
Expand Down Expand Up @@ -1259,6 +1295,7 @@ func (s *StageExecutor) commit(ctx context.Context, createdBy string, emptyLayer
SystemContext: s.executor.systemContext,
Squash: s.executor.squash,
EmptyLayer: emptyLayer,
EmptyLayerIfRedundant: emptyLayerIfRedundant,
BlobDirectory: s.executor.blobDirectory,
SignBy: s.executor.signBy,
MaxRetries: s.executor.maxPullPushRetries,
Expand Down

0 comments on commit 5ec97be

Please sign in to comment.