[Moved to #4057] Apply SOURCE_DATE_EPOCH to the files and the whiteouts inside OCI tar layers #3560
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ import ( | |
| "fmt" | ||
| "os" | ||
| "strconv" | ||
| "time" | ||
|
|
||
| "github.com/containerd/containerd/diff" | ||
| "github.com/containerd/containerd/diff/walking" | ||
|
|
@@ -19,6 +20,7 @@ import ( | |
| imagespecidentity "github.com/opencontainers/image-spec/identity" | ||
| ocispecs "github.com/opencontainers/image-spec/specs-go/v1" | ||
| "github.com/pkg/errors" | ||
| "github.com/sirupsen/logrus" | ||
| "golang.org/x/sync/errgroup" | ||
| ) | ||
|
|
||
|
|
@@ -32,7 +34,7 @@ var ErrNoBlobs = errors.Errorf("no blobs for snapshot") | |
| // a blob is missing and createIfNeeded is true, then the blob will be created, otherwise ErrNoBlobs will | ||
| // be returned. Caller must hold a lease when calling this function. | ||
| // If forceCompression is specified but the blob of compressionType doesn't exist, this function creates it. | ||
| func (sr *immutableRef) computeBlobChain(ctx context.Context, createIfNeeded bool, comp compression.Config, s session.Group, sourceDateEpoch *time.Time) error { | ||
| if _, ok := leases.FromContext(ctx); !ok { | ||
| return errors.Errorf("missing lease requirement for computeBlobChain") | ||
| } | ||
|
|
@@ -58,36 +60,58 @@ func (sr *immutableRef) computeBlobChain(ctx context.Context, createIfNeeded boo | |
| // refs rather than every single layer present among their ancestors. | ||
| filter := sr.layerSet() | ||
|
|
||
| return computeBlobChain(ctx, sr, createIfNeeded, comp, s, filter, sourceDateEpoch) | ||
| } | ||
|
|
||
| func blobExistsWithSourceDateEpoch(sr *immutableRef, sourceDateEpoch *time.Time) bool { | ||
| if sr.getBlob() == "" { | ||
| return false | ||
| } | ||
| if sourceDateEpoch == nil { | ||
| // nil means "any epoch is ok" | ||
| return true | ||
| } | ||
| srEpoch := sr.GetSourceDateEpoch() | ||
| return srEpoch != nil && srEpoch.Equal(*sourceDateEpoch) | ||
| } | ||
|
|
||
| func computeBlobChain(ctx context.Context, sr *immutableRef, createIfNeeded bool, comp compression.Config, s session.Group, filter map[string]struct{}, sourceDateEpoch *time.Time) error { | ||
| if blob := sr.getBlob(); blob != "" { | ||
| imageRefs := sr.getImageRefs() | ||
| logrus.Debugf("blob=%q, imageRefs=%v", blob, imageRefs) | ||
| if len(imageRefs) > 0 { | ||
| // Do not check the epoch for the blobs of the base images. | ||
| // https://github.com/moby/buildkit/pull/3560#pullrequestreview-1353829200 | ||
| sourceDateEpoch = nil | ||
| } | ||
| } | ||
|
|
||
| eg, ctx := errgroup.WithContext(ctx) | ||
| switch sr.kind() { | ||
| case Merge: | ||
| for _, parent := range sr.mergeParents { | ||
| parent := parent | ||
| eg.Go(func() error { | ||
| return computeBlobChain(ctx, parent, createIfNeeded, comp, s, filter, sourceDateEpoch) | ||
| }) | ||
| } | ||
| case Diff: | ||
| if _, ok := filter[sr.ID()]; !ok && sr.diffParents.upper != nil { | ||
| // This diff is just re-using the upper blob, compute that | ||
| eg.Go(func() error { | ||
| return computeBlobChain(ctx, sr.diffParents.upper, createIfNeeded, comp, s, filter, sourceDateEpoch) | ||
| }) | ||
| } | ||
| case Layer: | ||
| eg.Go(func() error { | ||
| return computeBlobChain(ctx, sr.layerParent, createIfNeeded, comp, s, filter, sourceDateEpoch) | ||
| }) | ||
| } | ||
|
|
||
| if _, ok := filter[sr.ID()]; ok { | ||
| eg.Go(func() error { | ||
| _, err := g.Do(ctx, fmt.Sprintf("%s-%t", sr.ID(), createIfNeeded), func(ctx context.Context) (interface{}, error) { | ||
| if blobExistsWithSourceDateEpoch(sr, sourceDateEpoch) { | ||
| return nil, nil | ||
| } | ||
| if !createIfNeeded { | ||
|
|
@@ -169,7 +193,7 @@ func computeBlobChain(ctx context.Context, sr *immutableRef, createIfNeeded bool | |
| } | ||
| } | ||
| if enableOverlay { | ||
| computed, ok, err := sr.tryComputeOverlayBlob(ctx, lower, upper, mediaType, sr.ID(), compressorFunc, sourceDateEpoch) | ||
| if !ok || err != nil { | ||
|
Comment on lines
195
to
197
There was a problem hiding this comment.
There was a problem hiding this comment. Probably yes, but we can defer that, as |
||
| if !fallback { | ||
| if !ok { | ||
|
|
@@ -196,6 +220,7 @@ func computeBlobChain(ctx context.Context, sr *immutableRef, createIfNeeded bool | |
| diff.WithMediaType(mediaType), | ||
| diff.WithReference(sr.ID()), | ||
| diff.WithCompressor(compressorFunc), | ||
| diff.WithSourceDateEpoch(sourceDateEpoch), | ||
| ) | ||
| if err != nil { | ||
| bklog.G(ctx).WithError(err).Warnf("failed to compute blob by buildkit differ") | ||
|
|
@@ -207,6 +232,7 @@ func computeBlobChain(ctx context.Context, sr *immutableRef, createIfNeeded bool | |
| diff.WithMediaType(mediaType), | ||
| diff.WithReference(sr.ID()), | ||
| diff.WithCompressor(compressorFunc), | ||
| diff.WithSourceDateEpoch(sourceDateEpoch), | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
|
|
@@ -238,7 +264,7 @@ func computeBlobChain(ctx context.Context, sr *immutableRef, createIfNeeded bool | |
| return nil, errors.Errorf("unknown layer compression type") | ||
| } | ||
|
|
||
| if err := sr.setBlob(ctx, desc, sourceDateEpoch); err != nil { | ||
| return nil, err | ||
| } | ||
| return nil, nil | ||
|
|
@@ -264,7 +290,7 @@ func computeBlobChain(ctx context.Context, sr *immutableRef, createIfNeeded bool | |
|
|
||
| // setBlob associates a blob with the cache record. | ||
| // A lease must be held for the blob when calling this function | ||
| func (sr *immutableRef) setBlob(ctx context.Context, desc ocispecs.Descriptor, sourceDateEpoch *time.Time) (rerr error) { | ||
| if _, ok := leases.FromContext(ctx); !ok { | ||
| return errors.Errorf("missing lease requirement for setBlob") | ||
| } | ||
|
|
@@ -285,7 +311,7 @@ func (sr *immutableRef) setBlob(ctx context.Context, desc ocispecs.Descriptor) ( | |
| sr.mu.Lock() | ||
| defer sr.mu.Unlock() | ||
|
|
||
| if blobExistsWithSourceDateEpoch(sr, sourceDateEpoch) { | ||
| return nil | ||
| } | ||
|
|
||
|
|
@@ -305,6 +331,9 @@ func (sr *immutableRef) setBlob(ctx context.Context, desc ocispecs.Descriptor) ( | |
| sr.queueMediaType(desc.MediaType) | ||
| sr.queueBlobSize(desc.Size) | ||
| sr.appendURLs(desc.URLs) | ||
| if sourceDateEpoch != nil { | ||
| sr.queueSourceDateEpoch(*sourceDateEpoch) | ||
|
There was a problem hiding this comment. Not sure I understand how this works. Doesn't it just fix a ref to a specific epoch forever? Instead, it should track multiple variants of blobs that are all associated with a ref (similar to compressions). There was a problem hiding this comment.
Does that require putting the epoch value into an OCI descriptor? There was a problem hiding this comment. You mean for the remote build-cache or for something else? In build-cache we already have "created time" but not exactly the same(although both would be epoch value). We might need a separate field for that. If you think for the case of avoiding regenerating blobs for base image layers if they were already generated with the same epoch, then yes, I think saving epoch would be needed. Otoh, I don't think any user would want us to regenerate the layers for the base image, just to change the timestamps in it. There was a problem hiding this comment.
|
||
| } | ||
| if err := sr.commitMetadata(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that if there is no blob for epoch but there is one for another one, we will run the differ again? Is it a good idea? I think it would be better to do something similar to compressions there we convert a blob to another one.