From 9e19760d109769ebe3d74ae43f9f3deb23abf607 Mon Sep 17 00:00:00 2001 From: Joey Brown Date: Thu, 9 May 2024 09:31:46 -0500 Subject: [PATCH 01/19] add failing test to restorer Signed-off-by: Joey Brown --- phase/restorer_test.go | 132 ++++++++++++++++++++++++++--------------- 1 file changed, 85 insertions(+), 47 deletions(-) diff --git a/phase/restorer_test.go b/phase/restorer_test.go index 61066c883..9a7b27301 100644 --- a/phase/restorer_test.go +++ b/phase/restorer_test.go @@ -203,53 +203,7 @@ func testRestorer(buildpackAPI, platformAPI string) func(t *testing.T, when spec h.AssertNil(t, os.RemoveAll(layersDir)) h.AssertNil(t, os.Mkdir(layersDir, 0777)) - contents := fmt.Sprintf(`{ - "buildpacks": [ - { - "key": "buildpack.id", - "layers": { - "cache-false": { - "cache": false, - "sha": "%s" - }, - "cache-launch": { - "cache": true, - "launch": true, - "sha": "%s" - }, - "cache-only": { - "cache": true, - "data": { - "cache-only-key": "cache-only-val" - }, - "sha": "%s" - } - } - }, - { - "key": "nogroup.buildpack.id", - "layers": { - "some-layer": { - "cache": true, - "sha": "%s" - } - } - }, - { - "key": "escaped/buildpack/id", - "layers": { - "escaped-bp-layer": { - "cache": true, - "data": { - "escaped-bp-key": "escaped-bp-val" - }, - "sha": "%s" - } - } - } - ] -} -`, cacheFalseLayerSHA, cacheLaunchLayerSHA, cacheOnlyLayerSHA, noGroupLayerSHA, escapedLayerSHA) + contents := buildMetadata(cacheFalseLayerSHA, cacheLaunchLayerSHA, cacheOnlyLayerSHA, noGroupLayerSHA, escapedLayerSHA) err = os.WriteFile( filepath.Join(cacheDir, "committed", "io.buildpacks.lifecycle.cache.metadata"), @@ -348,6 +302,40 @@ func testRestorer(buildpackAPI, platformAPI string) func(t *testing.T, when spec }) }) + when("there is a cache-only layer referenced in metadata that does not exist", func() { + var nonExistentCacheLaunchLayerSHA string + + it.Before(func() { + nonExistentCacheLaunchLayerSHA = "some-made-up-sha" + contents := buildMetadata(cacheFalseLayerSHA, nonExistentCacheLaunchLayerSHA, cacheOnlyLayerSHA, noGroupLayerSHA, escapedLayerSHA) + + err := os.WriteFile( + filepath.Join(cacheDir, "committed", "io.buildpacks.lifecycle.cache.metadata"), + []byte(contents), + 0600, + ) + h.AssertNil(t, err) + + err = restorer.Restore(testCache) + h.AssertNil(t, err) + }) + + it("restores expected cache-only layer", func() { + got := h.MustReadFile(t, filepath.Join(layersDir, "buildpack.id", "cache-only", "file-from-cache-only-layer")) + want := "echo text from cache-only layer\n" + h.AssertEq(t, string(got), want) + }) + + it("keeps expected layer metadata", func() { + got := h.MustReadFile(t, filepath.Join(layersDir, "buildpack.id", "cache-only.toml")) + h.AssertEq(t, string(got), "[metadata]\n cache-only-key = \"cache-only-val\"\n") + }) + + it("skips restoring non-existent cache-launch layer", func() { + h.AssertPathDoesNotExist(t, filepath.Join(layersDir, "buildpack.id", "cache-launch")) + }) + }) + when("there is a cache=true layer not in cache", func() { it.Before(func() { var meta, sha string @@ -550,3 +538,53 @@ func TestWriteLayer(t *testing.T) { h.AssertPathDoesNotExist(t, filepath.Join(layersDir, "test-buildpack", "test-layer")) } + +func buildMetadata(cacheFalseLayerSHA string, cacheLaunchLayerSHA string, cacheOnlyLayerSHA string, noGroupLayerSHA string, escapedLayerSHA string) string { + return fmt.Sprintf(`{ + "buildpacks": [ + { + "key": "buildpack.id", + "layers": { + "cache-false": { + "cache": false, + "sha": "%s" + }, + "cache-launch": { + "cache": true, + "launch": true, + "sha": "%s" + }, + "cache-only": { + "cache": true, + "data": { + "cache-only-key": "cache-only-val" + }, + "sha": "%s" + } + } + }, + { + "key": "nogroup.buildpack.id", + "layers": { + "some-layer": { + "cache": true, + "sha": "%s" + } + } + }, + { + "key": "escaped/buildpack/id", + "layers": { + "escaped-bp-layer": { + "cache": true, + "data": { + "escaped-bp-key": "escaped-bp-val" + }, + "sha": "%s" + } + } + } + ] +} +`, cacheFalseLayerSHA, cacheLaunchLayerSHA, cacheOnlyLayerSHA, noGroupLayerSHA, escapedLayerSHA) +} From 376931a66e72043ed86571216edf9bf2d6aeefd4 Mon Sep 17 00:00:00 2001 From: Joey Brown Date: Wed, 8 May 2024 16:02:29 -0500 Subject: [PATCH 02/19] restorer and exporter working as expected Signed-off-by: Joey Brown --- cache/caching_image_test.go | 3 +- cache/common.go | 18 +++++++++++ cache/image_cache.go | 37 +++++++++++++++++++++- cache/image_cache_test.go | 4 +-- cache/image_deleter.go | 7 +++-- cache/volume_cache.go | 45 ++++++++++++++++++++++++--- cache/volume_cache_test.go | 24 ++++++++------ cmd/lifecycle/exporter.go | 7 +++-- cmd/lifecycle/main.go | 8 ++--- internal/layer/sbom_restorer_test.go | 2 +- phase/analyzer.go | 2 +- phase/analyzer_test.go | 5 ++- phase/cache.go | 11 ++++++- phase/cache_test.go | 17 +++++----- phase/connected_factory.go | 5 +-- phase/exporter.go | 2 ++ phase/restorer.go | 12 ++++++- phase/restorer_test.go | 8 ++--- phase/testmock/cache/image_deleter.go | 12 +++++++ 19 files changed, 180 insertions(+), 49 deletions(-) diff --git a/cache/caching_image_test.go b/cache/caching_image_test.go index 345acc2cc..575181847 100644 --- a/cache/caching_image_test.go +++ b/cache/caching_image_test.go @@ -10,6 +10,7 @@ import ( "github.com/buildpacks/imgutil" "github.com/buildpacks/imgutil/fakes" + "github.com/buildpacks/lifecycle/cmd" "github.com/sclevine/spec" "github.com/sclevine/spec/report" @@ -37,7 +38,7 @@ func testCachingImage(t *testing.T, when spec.G, it spec.S) { fakeImage = fakes.NewImage("some-image", "", nil) tmpDir, err = os.MkdirTemp("", "") h.AssertNil(t, err) - volumeCache, err = cache.NewVolumeCache(tmpDir) + volumeCache, err = cache.NewVolumeCache(tmpDir, cmd.DefaultLogger) h.AssertNil(t, err) subject = cache.NewCachingImage(fakeImage, volumeCache) layerPath, layerSHA, layerData = h.RandomLayer(t, tmpDir) diff --git a/cache/common.go b/cache/common.go index 8b06e362f..d9d4f3293 100644 --- a/cache/common.go +++ b/cache/common.go @@ -5,3 +5,21 @@ import ( ) var errCacheCommitted = errors.New("cache cannot be modified after commit") + +type ReadErr struct { + msg string +} + +func NewReadErr(msg string) ReadErr { + return ReadErr{msg: msg} +} + +func (e ReadErr) Error() string { + return e.msg +} + +func IsReadErr(err error) (bool, *ReadErr) { + var e ReadErr + isReadErr := errors.As(err, &e) + return isReadErr, &e +} diff --git a/cache/image_cache.go b/cache/image_cache.go index efdbdf2e9..8ac04782e 100644 --- a/cache/image_cache.go +++ b/cache/image_cache.go @@ -106,8 +106,20 @@ func (c *ImageCache) ReuseLayer(diffID string) error { return c.newImage.ReuseLayer(diffID) } +func IsLayerNotFound(err error) bool { + var e imgutil.ErrLayerNotFound + return errors.As(err, &e) +} + func (c *ImageCache) RetrieveLayer(diffID string) (io.ReadCloser, error) { - return c.origImage.GetLayer(diffID) + closer, err := c.origImage.GetLayer(diffID) + if err != nil { + if IsLayerNotFound(err) { + return nil, NewReadErr(fmt.Sprintf("Layer with SHA '%s' not found", diffID)) + } + return nil, NewReadErr(fmt.Sprintf("unknown error retrieving layer with SHA '%s'", diffID)) + } + return closer, nil } func (c *ImageCache) Commit() error { @@ -129,3 +141,26 @@ func (c *ImageCache) Commit() error { return nil } + +func (c *ImageCache) LayerExists(diffID string) (bool, error) { + layers, err := c.origImage.UnderlyingImage().Layers() + if err != nil { + return false, errors.Wrap(err, "getting image layers") + } + + for _, layer := range layers { + d, err := layer.DiffID() + if err != nil { + return false, errors.Wrap(err, "getting layer diffID") + } + + if d.String() == diffID { + return true, nil + } + } + return false, nil +} + +func (c *ImageCache) Destroy() { + c.imageDeleter.DeleteImage(c.origImage) +} diff --git a/cache/image_cache_test.go b/cache/image_cache_test.go index d3fcb2d16..67e370bf5 100644 --- a/cache/image_cache_test.go +++ b/cache/image_cache_test.go @@ -146,7 +146,7 @@ func testImageCache(t *testing.T, when spec.G, it spec.S) { when("layer does not exist", func() { it("returns an error", func() { _, err := subject.RetrieveLayer("some_nonexistent_sha") - h.AssertError(t, err, "failed to get layer with sha 'some_nonexistent_sha'") + h.AssertError(t, err, "unknown error retrieving layer with SHA 'some_nonexistent_sha'") }) }) }) @@ -236,7 +236,7 @@ func testImageCache(t *testing.T, when spec.G, it spec.S) { h.AssertNil(t, subject.AddLayerFile(testLayerTarPath, testLayerSHA)) _, err := subject.RetrieveLayer(testLayerSHA) - h.AssertError(t, err, fmt.Sprintf("failed to get layer with sha '%s'", testLayerSHA)) + h.AssertError(t, err, fmt.Sprintf("unknown error retrieving layer with SHA '%s'", testLayerSHA)) }) }) }) diff --git a/cache/image_deleter.go b/cache/image_deleter.go index 8807715d1..f6fc03acd 100644 --- a/cache/image_deleter.go +++ b/cache/image_deleter.go @@ -12,6 +12,7 @@ import ( // ImageDeleter defines the methods available to delete and compare cached images type ImageDeleter interface { DeleteOrigImageIfDifferentFromNewImage(origImage, newImage imgutil.Image) + DeleteImage(image imgutil.Image) } // ImageDeleterImpl is a component to manage cache image deletion @@ -35,13 +36,13 @@ func (c *ImageDeleterImpl) DeleteOrigImageIfDifferentFromNewImage(origImage, new } if !same { - c.deleteImage(origImage) + c.DeleteImage(origImage) } } } -// deleteImage deletes an image -func (c *ImageDeleterImpl) deleteImage(image imgutil.Image) { +// DeleteImage deletes an image +func (c *ImageDeleterImpl) DeleteImage(image imgutil.Image) { if c.deletionEnabled { if err := image.Delete(); err != nil { c.logger.Warnf("Unable to delete cache image: %v", err.Error()) diff --git a/cache/volume_cache.go b/cache/volume_cache.go index 210e1afbf..1a551f2f9 100644 --- a/cache/volume_cache.go +++ b/cache/volume_cache.go @@ -2,12 +2,14 @@ package cache import ( "encoding/json" + "fmt" "io" "os" "path/filepath" "runtime" "strings" + "github.com/buildpacks/lifecycle/log" "github.com/pkg/errors" "github.com/buildpacks/lifecycle/internal/fsutil" @@ -20,9 +22,10 @@ type VolumeCache struct { backupDir string stagingDir string committedDir string + logger log.Logger } -func NewVolumeCache(dir string) (*VolumeCache, error) { +func NewVolumeCache(dir string, logger log.Logger) (*VolumeCache, error) { if _, err := os.Stat(dir); err != nil { return nil, err } @@ -32,6 +35,7 @@ func NewVolumeCache(dir string) (*VolumeCache, error) { backupDir: filepath.Join(dir, "committed-backup"), stagingDir: filepath.Join(dir, "staging"), committedDir: filepath.Join(dir, "committed"), + logger: logger, } if err := c.setupStagingDir(); err != nil { @@ -133,7 +137,20 @@ func (c *VolumeCache) ReuseLayer(diffID string) error { if c.committed { return errCacheCommitted } - if err := os.Link(diffIDPath(c.committedDir, diffID), diffIDPath(c.stagingDir, diffID)); err != nil && !os.IsExist(err) { + committedPath := diffIDPath(c.committedDir, diffID) + stagingPath := diffIDPath(c.stagingDir, diffID) + + if _, err := os.Stat(committedPath); err != nil { + if os.IsNotExist(err) { + return NewReadErr(fmt.Sprintf("Layer with SHA '%s' not found", diffID)) + } + if os.IsPermission(err) { + return NewReadErr(fmt.Sprintf("Cannot read cache layer with SHA '%s' due to insufficient permissions", diffID)) + } + return errors.Wrapf(err, "reusing layer (%s)", diffID) + } + + if err := os.Link(committedPath, stagingPath); err != nil && !os.IsExist(err) { return errors.Wrapf(err, "reusing layer (%s)", diffID) } return nil @@ -146,7 +163,10 @@ func (c *VolumeCache) RetrieveLayer(diffID string) (io.ReadCloser, error) { } file, err := os.Open(path) if err != nil { - return nil, errors.Wrapf(err, "opening layer with SHA '%s'", diffID) + if os.IsPermission(err) { + return nil, NewReadErr(fmt.Sprintf("cannot read cache layer with SHA '%s' due to insufficient permissions", diffID)) + } + return nil, NewReadErr(fmt.Sprintf("unknown error retrieving layer with SHA '%s'", diffID)) } return file, nil } @@ -165,7 +185,7 @@ func (c *VolumeCache) RetrieveLayerFile(diffID string) (string, error) { path := diffIDPath(c.committedDir, diffID) if _, err := os.Stat(path); err != nil { if os.IsNotExist(err) { - return "", errors.Wrapf(err, "layer with SHA '%s' not found", diffID) + return "", NewReadErr(fmt.Sprintf("Layer with SHA '%s' not found", diffID)) } return "", errors.Wrapf(err, "retrieving layer with SHA '%s'", diffID) } @@ -206,3 +226,20 @@ func (c *VolumeCache) setupStagingDir() error { } return os.MkdirAll(c.stagingDir, 0777) } + +func (c *VolumeCache) LayerExists(diffID string) (bool, error) { + path := diffIDPath(c.committedDir, diffID) + if _, err := os.Stat(path); err != nil { + if os.IsNotExist(err) { + return false, nil + } + return false, errors.Wrapf(err, "retrieving layer with SHA '%s'", diffID) + } + return true, nil +} + +func (c *VolumeCache) Destroy() { + if err := os.RemoveAll(c.dir); err != nil { + c.logger.Warnf("Unable to delete cache directory: %v", err.Error()) + } +} diff --git a/cache/volume_cache_test.go b/cache/volume_cache_test.go index 3ab97ffe0..cbcea5707 100644 --- a/cache/volume_cache_test.go +++ b/cache/volume_cache_test.go @@ -7,6 +7,8 @@ import ( "path/filepath" "testing" + "github.com/buildpacks/lifecycle/cmd" + "github.com/buildpacks/lifecycle/log" "github.com/sclevine/spec" "github.com/sclevine/spec/report" @@ -28,6 +30,7 @@ func testVolumeCache(t *testing.T, when spec.G, it spec.S) { backupDir string stagingDir string committedDir string + testLogger log.Logger ) it.Before(func() { @@ -42,6 +45,7 @@ func testVolumeCache(t *testing.T, when spec.G, it spec.S) { backupDir = filepath.Join(volumeDir, "committed-backup") stagingDir = filepath.Join(volumeDir, "staging") committedDir = filepath.Join(volumeDir, "committed") + testLogger = cmd.DefaultLogger }) it.After(func() { @@ -50,7 +54,7 @@ func testVolumeCache(t *testing.T, when spec.G, it spec.S) { when("#NewVolumeCache", func() { it("returns an error when the volume path does not exist", func() { - _, err := cache.NewVolumeCache(filepath.Join(tmpDir, "does_not_exist")) + _, err := cache.NewVolumeCache(filepath.Join(tmpDir, "does_not_exist"), testLogger) if err == nil { t.Fatal("expected NewVolumeCache to fail because volume path does not exist") } @@ -66,7 +70,7 @@ func testVolumeCache(t *testing.T, when spec.G, it spec.S) { it("clears staging", func() { var err error - subject, err = cache.NewVolumeCache(volumeDir) + subject, err = cache.NewVolumeCache(volumeDir, testLogger) h.AssertNil(t, err) _, err = os.Stat(filepath.Join(stagingDir, "some-layer.tar")) @@ -80,7 +84,7 @@ func testVolumeCache(t *testing.T, when spec.G, it spec.S) { it("creates staging dir", func() { var err error - subject, err = cache.NewVolumeCache(volumeDir) + subject, err = cache.NewVolumeCache(volumeDir, testLogger) h.AssertNil(t, err) _, err = os.Stat(stagingDir) @@ -92,7 +96,7 @@ func testVolumeCache(t *testing.T, when spec.G, it spec.S) { it("creates committed dir", func() { var err error - subject, err = cache.NewVolumeCache(volumeDir) + subject, err = cache.NewVolumeCache(volumeDir, testLogger) h.AssertNil(t, err) _, err = os.Stat(committedDir) @@ -109,7 +113,7 @@ func testVolumeCache(t *testing.T, when spec.G, it spec.S) { it("clears the backup dir", func() { var err error - subject, err = cache.NewVolumeCache(volumeDir) + subject, err = cache.NewVolumeCache(volumeDir, testLogger) h.AssertNil(t, err) _, err = os.Stat(filepath.Join(backupDir, "some-layer.tar")) @@ -124,7 +128,7 @@ func testVolumeCache(t *testing.T, when spec.G, it spec.S) { it.Before(func() { var err error - subject, err = cache.NewVolumeCache(volumeDir) + subject, err = cache.NewVolumeCache(volumeDir, testLogger) h.AssertNil(t, err) }) @@ -206,7 +210,7 @@ func testVolumeCache(t *testing.T, when spec.G, it spec.S) { when("layer does not exist", func() { it("returns an error", func() { _, err := subject.RetrieveLayer("some_nonexistent_sha") - h.AssertError(t, err, "layer with SHA 'some_nonexistent_sha' not found") + h.AssertError(t, err, "Layer with SHA 'some_nonexistent_sha' not found") }) }) }) @@ -230,7 +234,7 @@ func testVolumeCache(t *testing.T, when spec.G, it spec.S) { when("layer does not exist", func() { it("returns an error", func() { _, err := subject.RetrieveLayerFile("some_nonexistent_sha") - h.AssertError(t, err, "layer with SHA 'some_nonexistent_sha' not found") + h.AssertError(t, err, "Layer with SHA 'some_nonexistent_sha' not found") }) }) }) @@ -340,7 +344,7 @@ func testVolumeCache(t *testing.T, when spec.G, it spec.S) { h.AssertNil(t, subject.AddLayerFile(tarPath, "some_sha")) _, err := subject.RetrieveLayer("some_sha") - h.AssertError(t, err, "layer with SHA 'some_sha' not found") + h.AssertError(t, err, "Layer with SHA 'some_sha' not found") }) }) @@ -415,7 +419,7 @@ func testVolumeCache(t *testing.T, when spec.G, it spec.S) { h.AssertNil(t, subject.AddLayer(layerReader, layerSha)) _, err := subject.RetrieveLayer(layerSha) - h.AssertError(t, err, fmt.Sprintf("layer with SHA '%s' not found", layerSha)) + h.AssertError(t, err, fmt.Sprintf("Layer with SHA '%s' not found", layerSha)) }) }) diff --git a/cmd/lifecycle/exporter.go b/cmd/lifecycle/exporter.go index 305186617..10d97cba2 100644 --- a/cmd/lifecycle/exporter.go +++ b/cmd/lifecycle/exporter.go @@ -12,6 +12,7 @@ import ( "github.com/buildpacks/imgutil/layout" "github.com/buildpacks/imgutil/local" "github.com/buildpacks/imgutil/remote" + "github.com/buildpacks/lifecycle/log" "github.com/docker/docker/client" "github.com/google/go-containerregistry/pkg/authn" "github.com/google/go-containerregistry/pkg/name" @@ -200,7 +201,7 @@ func (e *exportCmd) export(group buildpack.Group, cacheStore phase.Cache, analyz case e.UseLayout: appImage, runImageID, err = e.initLayoutAppImage(analyzedMD) case e.UseDaemon: - appImage, runImageID, err = e.initDaemonAppImage(analyzedMD) + appImage, runImageID, err = e.initDaemonAppImage(analyzedMD, cmd.DefaultLogger) default: appImage, runImageID, err = e.initRemoteAppImage(analyzedMD) } @@ -258,7 +259,7 @@ func (e *exportCmd) export(group buildpack.Group, cacheStore phase.Cache, analyz return nil } -func (e *exportCmd) initDaemonAppImage(analyzedMD files.Analyzed) (imgutil.Image, string, error) { +func (e *exportCmd) initDaemonAppImage(analyzedMD files.Analyzed, logger log.Logger) (imgutil.Image, string, error) { var opts = []imgutil.ImageOption{ local.FromBaseImage(e.RunImageRef), } @@ -301,7 +302,7 @@ func (e *exportCmd) initDaemonAppImage(analyzedMD files.Analyzed) (imgutil.Image } if e.LaunchCacheDir != "" { - volumeCache, err := cache.NewVolumeCache(e.LaunchCacheDir) + volumeCache, err := cache.NewVolumeCache(e.LaunchCacheDir, logger) if err != nil { return nil, "", cmd.FailErr(err, "create launch cache") } diff --git a/cmd/lifecycle/main.go b/cmd/lifecycle/main.go index 74b51a010..42fede727 100644 --- a/cmd/lifecycle/main.go +++ b/cmd/lifecycle/main.go @@ -96,14 +96,14 @@ func (ch *DefaultCacheHandler) InitCache(cacheImageRef string, cacheDir string, cacheStore phase.Cache err error ) + logger := cmd.DefaultLogger if cacheImageRef != "" { - logger := cmd.DefaultLogger cacheStore, err = cache.NewImageCacheFromName(cacheImageRef, ch.keychain, logger, cache.NewImageDeleter(cache.NewImageComparer(), logger, deletionEnabled)) if err != nil { return nil, errors.Wrap(err, "creating image cache") } } else if cacheDir != "" { - cacheStore, err = cache.NewVolumeCache(cacheDir) + cacheStore, err = cache.NewVolumeCache(cacheDir, logger) if err != nil { return nil, errors.Wrap(err, "creating volume cache") } @@ -118,14 +118,14 @@ func initCache(cacheImageTag, cacheDir string, keychain authn.Keychain, deletion cacheStore phase.Cache err error ) + logger := cmd.DefaultLogger if cacheImageTag != "" { - logger := cmd.DefaultLogger cacheStore, err = cache.NewImageCacheFromName(cacheImageTag, keychain, logger, cache.NewImageDeleter(cache.NewImageComparer(), logger, deletionEnabled)) if err != nil { return nil, cmd.FailErr(err, "create image cache") } } else if cacheDir != "" { - cacheStore, err = cache.NewVolumeCache(cacheDir) + cacheStore, err = cache.NewVolumeCache(cacheDir, logger) if err != nil { return nil, cmd.FailErr(err, "create volume cache") } diff --git a/internal/layer/sbom_restorer_test.go b/internal/layer/sbom_restorer_test.go index 544b3f296..f542f00ed 100644 --- a/internal/layer/sbom_restorer_test.go +++ b/internal/layer/sbom_restorer_test.go @@ -164,7 +164,7 @@ func testSBOMRestorer(t *testing.T, when spec.G, it spec.S) { cacheDir, err = os.MkdirTemp("", "lifecycle.cache-dir.") h.AssertNil(t, err) - testCache, err = cache.NewVolumeCache(cacheDir) + testCache, err = cache.NewVolumeCache(cacheDir, &log.Logger{Handler: &discard.Handler{}}) h.AssertNil(t, err) h.AssertNil(t, testCache.AddLayerFile(layer.TarPath, layer.Digest)) h.AssertNil(t, testCache.SetMetadata(platform.CacheMetadata{BOM: files.LayerMetadata{SHA: layer.Digest}})) diff --git a/phase/analyzer.go b/phase/analyzer.go index 0217411bf..c5d4ef6a9 100644 --- a/phase/analyzer.go +++ b/phase/analyzer.go @@ -43,7 +43,7 @@ func (f *ConnectedFactory) NewAnalyzer(inputs platform.LifecycleInputs, logger l } var err error - if analyzer.PreviousImage, err = f.getPreviousImage(inputs.PreviousImageRef, inputs.LaunchCacheDir); err != nil { + if analyzer.PreviousImage, err = f.getPreviousImage(inputs.PreviousImageRef, inputs.LaunchCacheDir, logger); err != nil { return nil, err } if analyzer.RunImage, err = f.getRunImage(inputs.RunImageRef); err != nil { diff --git a/phase/analyzer_test.go b/phase/analyzer_test.go index 71f965f0a..87f2518a9 100644 --- a/phase/analyzer_test.go +++ b/phase/analyzer_test.go @@ -332,6 +332,7 @@ func testAnalyzer(platformAPI string) func(t *testing.T, when spec.G, it spec.S) it.Before(func() { var err error + discardLogger := log.Logger{Handler: &discard.Handler{}} tmpDir, err = os.MkdirTemp("", "analyzer-tests") h.AssertNil(t, err) @@ -342,15 +343,13 @@ func testAnalyzer(platformAPI string) func(t *testing.T, when spec.G, it spec.S) cacheDir, err = os.MkdirTemp("", "some-cache-dir") h.AssertNil(t, err) - testCache, err = cache.NewVolumeCache(cacheDir) + testCache, err = cache.NewVolumeCache(cacheDir, &discardLogger) h.AssertNil(t, err) previousImage = fakes.NewImage("image-repo-name", "", local.IDIdentifier{ ImageID: "s0m3D1g3sT", }) - discardLogger := log.Logger{Handler: &discard.Handler{}} - mockCtrl = gomock.NewController(t) sbomRestorer = testmock.NewMockSBOMRestorer(mockCtrl) diff --git a/phase/cache.go b/phase/cache.go index 16685d97d..3d7bff77b 100644 --- a/phase/cache.go +++ b/phase/cache.go @@ -8,6 +8,7 @@ import ( "github.com/pkg/errors" "github.com/buildpacks/lifecycle/buildpack" + c "github.com/buildpacks/lifecycle/cache" "github.com/buildpacks/lifecycle/layers" "github.com/buildpacks/lifecycle/log" "github.com/buildpacks/lifecycle/platform" @@ -100,7 +101,15 @@ func (e *Exporter) addOrReuseCacheLayer(cache Cache, layerDir LayerDir, previous if layer.Digest == previousSHA { e.Logger.Infof("Reusing cache layer '%s'\n", layer.ID) e.Logger.Debugf("Layer '%s' SHA: %s\n", layer.ID, layer.Digest) - return layer.Digest, cache.ReuseLayer(previousSHA) + err = cache.ReuseLayer(previousSHA) + if err != nil { + isReadErr, readErr := c.IsReadErr(err) + if !isReadErr { + return "", errors.Wrapf(err, "reusing layer %s", layer.ID) + } else { + e.Logger.Warnf("%s, skipping reuse", readErr.Error()) + } + } } e.Logger.Infof("Adding cache layer '%s'\n", layer.ID) e.Logger.Debugf("Layer '%s' SHA: %s\n", layer.ID, layer.Digest) diff --git a/phase/cache_test.go b/phase/cache_test.go index 03971e7c9..bf73376a3 100644 --- a/phase/cache_test.go +++ b/phase/cache_test.go @@ -10,6 +10,7 @@ import ( "testing" "github.com/apex/log" + "github.com/apex/log/handlers/discard" "github.com/apex/log/handlers/memory" "github.com/golang/mock/gomock" "github.com/sclevine/spec" @@ -46,6 +47,10 @@ func testCache(t *testing.T, when spec.G, it spec.S) { mockCtrl = gomock.NewController(t) layerFactory = testmock.NewMockLayerFactory(mockCtrl) + logHandler = memory.New() + level, err := log.ParseLevel("info") + h.AssertNil(t, err) + tmpDir, err = os.MkdirTemp("", "lifecycle.cacher.layer") h.AssertNil(t, err) h.AssertNil(t, os.Mkdir(filepath.Join(tmpDir, "artifacts"), 0777)) @@ -53,11 +58,7 @@ func testCache(t *testing.T, when spec.G, it spec.S) { cacheDir = filepath.Join(tmpDir, "cache") h.AssertNil(t, os.Mkdir(cacheDir, 0777)) - testCache, err = cache.NewVolumeCache(cacheDir) - h.AssertNil(t, err) - - logHandler = memory.New() - level, err := log.ParseLevel("info") + testCache, err = cache.NewVolumeCache(cacheDir, &log.Logger{Handler: logHandler, Level: level}) h.AssertNil(t, err) exporter = &phase.Exporter{ @@ -300,13 +301,15 @@ func assertCacheHasLayer(t *testing.T, cache phase.Cache, id string) { } func initializeCache(t *testing.T, exporter *phase.Exporter, testCache *phase.Cache, cacheDir, layersDir, metadataTemplate string) { - previousCache, err := cache.NewVolumeCache(cacheDir) + logger := &log.Logger{Handler: &discard.Handler{}} + + previousCache, err := cache.NewVolumeCache(cacheDir, logger) h.AssertNil(t, err) err = exporter.Cache(layersDir, previousCache) h.AssertNil(t, err) - *testCache, err = cache.NewVolumeCache(cacheDir) + *testCache, err = cache.NewVolumeCache(cacheDir, logger) h.AssertNil(t, err) h.AssertNil(t, os.WriteFile( diff --git a/phase/connected_factory.go b/phase/connected_factory.go index 3514938a2..be749a718 100644 --- a/phase/connected_factory.go +++ b/phase/connected_factory.go @@ -4,6 +4,7 @@ import ( "fmt" "github.com/buildpacks/imgutil" + "github.com/buildpacks/lifecycle/log" "github.com/buildpacks/lifecycle/api" "github.com/buildpacks/lifecycle/cache" @@ -58,7 +59,7 @@ func (f *ConnectedFactory) ensureRegistryAccess(inputs platform.LifecycleInputs) return nil } -func (f *ConnectedFactory) getPreviousImage(imageRef string, launchCacheDir string) (imgutil.Image, error) { +func (f *ConnectedFactory) getPreviousImage(imageRef string, launchCacheDir string, logger log.Logger) (imgutil.Image, error) { if imageRef == "" { return nil, nil } @@ -69,7 +70,7 @@ func (f *ConnectedFactory) getPreviousImage(imageRef string, launchCacheDir stri if launchCacheDir == "" || f.imageHandler.Kind() != image.LocalKind { return previousImage, nil } - volumeCache, err := cache.NewVolumeCache(launchCacheDir) + volumeCache, err := cache.NewVolumeCache(launchCacheDir, logger) if err != nil { return nil, fmt.Errorf("creating launch cache: %w", err) } diff --git a/phase/exporter.go b/phase/exporter.go index 4808e8572..a6bce1dd9 100644 --- a/phase/exporter.go +++ b/phase/exporter.go @@ -34,7 +34,9 @@ type Cache interface { AddLayerFile(tarPath string, sha string) error ReuseLayer(sha string) error RetrieveLayer(sha string) (io.ReadCloser, error) + LayerExists(sha string) (bool, error) Commit() error + Destroy() } type Exporter struct { diff --git a/phase/restorer.go b/phase/restorer.go index 48b6da59e..b7dd26c90 100644 --- a/phase/restorer.go +++ b/phase/restorer.go @@ -3,6 +3,7 @@ package phase import ( "path/filepath" + c "github.com/buildpacks/lifecycle/cache" "github.com/pkg/errors" "golang.org/x/sync/errgroup" @@ -98,7 +99,16 @@ func (r *Restorer) Restore(cache Cache) error { } else { r.Logger.Infof("Restoring data for %q from cache", bpLayer.Identifier()) g.Go(func() error { - return r.restoreCacheLayer(cache, cachedLayer.SHA) + err = r.restoreCacheLayer(cache, cachedLayer.SHA) + if err != nil { + isReadErr, readErr := c.IsReadErr(err) + if isReadErr { + r.Logger.Warnf("%s, skipping restore", readErr.Error()) + return nil + } + return errors.Wrapf(err, "restoring layer %s", bpLayer.Identifier()) + } + return nil }) } } diff --git a/phase/restorer_test.go b/phase/restorer_test.go index 9a7b27301..ececcda28 100644 --- a/phase/restorer_test.go +++ b/phase/restorer_test.go @@ -54,6 +54,8 @@ func testRestorer(buildpackAPI, platformAPI string) func(t *testing.T, when spec it.Before(func() { var err error + logHandler = memory.New() + logger := log.Logger{Handler: logHandler, Level: log.DebugLevel} layersDir, err = os.MkdirTemp("", "lifecycle-layer-dir") h.AssertNil(t, err) @@ -61,13 +63,9 @@ func testRestorer(buildpackAPI, platformAPI string) func(t *testing.T, when spec cacheDir, err = os.MkdirTemp("", "") h.AssertNil(t, err) - testCache, err = cache.NewVolumeCache(cacheDir) + testCache, err = cache.NewVolumeCache(cacheDir, &logger) h.AssertNil(t, err) - logHandler = memory.New() - - logger := log.Logger{Handler: logHandler, Level: log.DebugLevel} - mockCtrl = gomock.NewController(t) sbomRestorer = testmock.NewMockSBOMRestorer(mockCtrl) if api.MustParse(platformAPI).AtLeast("0.8") { diff --git a/phase/testmock/cache/image_deleter.go b/phase/testmock/cache/image_deleter.go index f4a6051ec..39d6a4b5f 100644 --- a/phase/testmock/cache/image_deleter.go +++ b/phase/testmock/cache/image_deleter.go @@ -34,6 +34,18 @@ func (m *MockImageDeleter) EXPECT() *MockImageDeleterMockRecorder { return m.recorder } +// DeleteImage mocks base method. +func (m *MockImageDeleter) DeleteImage(arg0 imgutil.Image) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "DeleteImage", arg0) +} + +// DeleteImage indicates an expected call of DeleteImage. +func (mr *MockImageDeleterMockRecorder) DeleteImage(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteImage", reflect.TypeOf((*MockImageDeleter)(nil).DeleteImage), arg0) +} + // DeleteOrigImageIfDifferentFromNewImage mocks base method. func (m *MockImageDeleter) DeleteOrigImageIfDifferentFromNewImage(arg0, arg1 imgutil.Image) { m.ctrl.T.Helper() From 1c4aefa37c8f40a1cf9d5980db44e43efd2c697f Mon Sep 17 00:00:00 2001 From: Joey Brown Date: Tue, 2 Jul 2024 15:41:21 -0500 Subject: [PATCH 03/19] lint Signed-off-by: Joey Brown --- cache/caching_image_test.go | 3 ++- cache/common.go | 4 ++++ cache/image_cache.go | 4 ++++ cache/volume_cache.go | 6 +++++- cache/volume_cache_test.go | 5 +++-- cmd/lifecycle/exporter.go | 3 ++- phase/cache.go | 3 +-- phase/connected_factory.go | 1 + phase/restorer.go | 3 ++- 9 files changed, 24 insertions(+), 8 deletions(-) diff --git a/cache/caching_image_test.go b/cache/caching_image_test.go index 575181847..31ebf3748 100644 --- a/cache/caching_image_test.go +++ b/cache/caching_image_test.go @@ -10,10 +10,11 @@ import ( "github.com/buildpacks/imgutil" "github.com/buildpacks/imgutil/fakes" - "github.com/buildpacks/lifecycle/cmd" "github.com/sclevine/spec" "github.com/sclevine/spec/report" + "github.com/buildpacks/lifecycle/cmd" + "github.com/buildpacks/lifecycle/cache" h "github.com/buildpacks/lifecycle/testhelpers" ) diff --git a/cache/common.go b/cache/common.go index d9d4f3293..c37d2f20a 100644 --- a/cache/common.go +++ b/cache/common.go @@ -6,18 +6,22 @@ import ( var errCacheCommitted = errors.New("cache cannot be modified after commit") +// ReadErr is an error type for filesystem read errors. type ReadErr struct { msg string } +// NewReadErr creates a new ReadErr. func NewReadErr(msg string) ReadErr { return ReadErr{msg: msg} } +// Error returns the error message. func (e ReadErr) Error() string { return e.msg } +// IsReadErr checks if an error is a ReadErr. func IsReadErr(err error) (bool, *ReadErr) { var e ReadErr isReadErr := errors.As(err, &e) diff --git a/cache/image_cache.go b/cache/image_cache.go index 8ac04782e..bd9436177 100644 --- a/cache/image_cache.go +++ b/cache/image_cache.go @@ -106,11 +106,13 @@ func (c *ImageCache) ReuseLayer(diffID string) error { return c.newImage.ReuseLayer(diffID) } +// IsLayerNotFound checks if the error is a layer not found error func IsLayerNotFound(err error) bool { var e imgutil.ErrLayerNotFound return errors.As(err, &e) } +// RetrieveLayer retrieves a layer from the cache func (c *ImageCache) RetrieveLayer(diffID string) (io.ReadCloser, error) { closer, err := c.origImage.GetLayer(diffID) if err != nil { @@ -142,6 +144,7 @@ func (c *ImageCache) Commit() error { return nil } +// LayerExists checks if a layer exists in the cache func (c *ImageCache) LayerExists(diffID string) (bool, error) { layers, err := c.origImage.UnderlyingImage().Layers() if err != nil { @@ -161,6 +164,7 @@ func (c *ImageCache) LayerExists(diffID string) (bool, error) { return false, nil } +// Destroy deletes the cache image func (c *ImageCache) Destroy() { c.imageDeleter.DeleteImage(c.origImage) } diff --git a/cache/volume_cache.go b/cache/volume_cache.go index 1a551f2f9..157cbcd92 100644 --- a/cache/volume_cache.go +++ b/cache/volume_cache.go @@ -9,9 +9,10 @@ import ( "runtime" "strings" - "github.com/buildpacks/lifecycle/log" "github.com/pkg/errors" + "github.com/buildpacks/lifecycle/log" + "github.com/buildpacks/lifecycle/internal/fsutil" "github.com/buildpacks/lifecycle/platform" ) @@ -25,6 +26,7 @@ type VolumeCache struct { logger log.Logger } +// NewVolumeCache creates a new VolumeCache func NewVolumeCache(dir string, logger log.Logger) (*VolumeCache, error) { if _, err := os.Stat(dir); err != nil { return nil, err @@ -227,6 +229,7 @@ func (c *VolumeCache) setupStagingDir() error { return os.MkdirAll(c.stagingDir, 0777) } +// LayerExists returns true if the layer with the given diffID exists in the cache func (c *VolumeCache) LayerExists(diffID string) (bool, error) { path := diffIDPath(c.committedDir, diffID) if _, err := os.Stat(path); err != nil { @@ -238,6 +241,7 @@ func (c *VolumeCache) LayerExists(diffID string) (bool, error) { return true, nil } +// Destroy removes the cache directory and all its contents func (c *VolumeCache) Destroy() { if err := os.RemoveAll(c.dir); err != nil { c.logger.Warnf("Unable to delete cache directory: %v", err.Error()) diff --git a/cache/volume_cache_test.go b/cache/volume_cache_test.go index cbcea5707..161d72e2c 100644 --- a/cache/volume_cache_test.go +++ b/cache/volume_cache_test.go @@ -7,11 +7,12 @@ import ( "path/filepath" "testing" - "github.com/buildpacks/lifecycle/cmd" - "github.com/buildpacks/lifecycle/log" "github.com/sclevine/spec" "github.com/sclevine/spec/report" + "github.com/buildpacks/lifecycle/cmd" + "github.com/buildpacks/lifecycle/log" + "github.com/buildpacks/lifecycle/buildpack" "github.com/buildpacks/lifecycle/cache" "github.com/buildpacks/lifecycle/platform" diff --git a/cmd/lifecycle/exporter.go b/cmd/lifecycle/exporter.go index 10d97cba2..06929fe53 100644 --- a/cmd/lifecycle/exporter.go +++ b/cmd/lifecycle/exporter.go @@ -12,7 +12,6 @@ import ( "github.com/buildpacks/imgutil/layout" "github.com/buildpacks/imgutil/local" "github.com/buildpacks/imgutil/remote" - "github.com/buildpacks/lifecycle/log" "github.com/docker/docker/client" "github.com/google/go-containerregistry/pkg/authn" "github.com/google/go-containerregistry/pkg/name" @@ -20,6 +19,8 @@ import ( "github.com/pkg/errors" "golang.org/x/sync/errgroup" + "github.com/buildpacks/lifecycle/log" + "github.com/buildpacks/lifecycle/auth" "github.com/buildpacks/lifecycle/buildpack" "github.com/buildpacks/lifecycle/cache" diff --git a/phase/cache.go b/phase/cache.go index 3d7bff77b..d6e9afff8 100644 --- a/phase/cache.go +++ b/phase/cache.go @@ -106,9 +106,8 @@ func (e *Exporter) addOrReuseCacheLayer(cache Cache, layerDir LayerDir, previous isReadErr, readErr := c.IsReadErr(err) if !isReadErr { return "", errors.Wrapf(err, "reusing layer %s", layer.ID) - } else { - e.Logger.Warnf("%s, skipping reuse", readErr.Error()) } + e.Logger.Warnf("%s, skipping reuse", readErr.Error()) } } e.Logger.Infof("Adding cache layer '%s'\n", layer.ID) diff --git a/phase/connected_factory.go b/phase/connected_factory.go index be749a718..083d4ca0a 100644 --- a/phase/connected_factory.go +++ b/phase/connected_factory.go @@ -4,6 +4,7 @@ import ( "fmt" "github.com/buildpacks/imgutil" + "github.com/buildpacks/lifecycle/log" "github.com/buildpacks/lifecycle/api" diff --git a/phase/restorer.go b/phase/restorer.go index b7dd26c90..094e9fe43 100644 --- a/phase/restorer.go +++ b/phase/restorer.go @@ -3,10 +3,11 @@ package phase import ( "path/filepath" - c "github.com/buildpacks/lifecycle/cache" "github.com/pkg/errors" "golang.org/x/sync/errgroup" + c "github.com/buildpacks/lifecycle/cache" + "github.com/buildpacks/lifecycle/api" "github.com/buildpacks/lifecycle/buildpack" "github.com/buildpacks/lifecycle/internal/layer" From 0101e33b5fe3c2e394ae28b6ee06b7dcb888f696 Mon Sep 17 00:00:00 2001 From: Joey Brown Date: Tue, 9 Jul 2024 10:10:10 -0500 Subject: [PATCH 04/19] Update phase/restorer.go Co-authored-by: Natalie Arellano Signed-off-by: Joey Brown --- phase/restorer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phase/restorer.go b/phase/restorer.go index 094e9fe43..e0203e722 100644 --- a/phase/restorer.go +++ b/phase/restorer.go @@ -104,7 +104,7 @@ func (r *Restorer) Restore(cache Cache) error { if err != nil { isReadErr, readErr := c.IsReadErr(err) if isReadErr { - r.Logger.Warnf("%s, skipping restore", readErr.Error()) + r.Logger.Warnf("Skipping restore for layer %s: %s", bpLayer.Identifier(), readErr.Error()) return nil } return errors.Wrapf(err, "restoring layer %s", bpLayer.Identifier()) From 3d30c91b6b9a4dc3dcca8a073c0bcb72862670c1 Mon Sep 17 00:00:00 2001 From: Joey Brown Date: Tue, 9 Jul 2024 10:10:27 -0500 Subject: [PATCH 05/19] Update phase/cache.go Co-authored-by: Natalie Arellano Signed-off-by: Joey Brown --- phase/cache.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phase/cache.go b/phase/cache.go index d6e9afff8..4d190b58a 100644 --- a/phase/cache.go +++ b/phase/cache.go @@ -107,7 +107,7 @@ func (e *Exporter) addOrReuseCacheLayer(cache Cache, layerDir LayerDir, previous if !isReadErr { return "", errors.Wrapf(err, "reusing layer %s", layer.ID) } - e.Logger.Warnf("%s, skipping reuse", readErr.Error()) + e.Logger.Warnf(r.Logger.Warnf("Skipping re-use for layer %s: %s", layer.ID, readErr.Error())) } } e.Logger.Infof("Adding cache layer '%s'\n", layer.ID) From 8570898b3b9c0cbf34e5ec17dcd1d71cd4e2fcf8 Mon Sep 17 00:00:00 2001 From: Joey Brown Date: Tue, 9 Jul 2024 10:10:39 -0500 Subject: [PATCH 06/19] Update cache/image_cache.go Co-authored-by: Natalie Arellano Signed-off-by: Joey Brown --- cache/image_cache.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cache/image_cache.go b/cache/image_cache.go index bd9436177..461a44463 100644 --- a/cache/image_cache.go +++ b/cache/image_cache.go @@ -117,7 +117,7 @@ func (c *ImageCache) RetrieveLayer(diffID string) (io.ReadCloser, error) { closer, err := c.origImage.GetLayer(diffID) if err != nil { if IsLayerNotFound(err) { - return nil, NewReadErr(fmt.Sprintf("Layer with SHA '%s' not found", diffID)) + return nil, NewReadErr(fmt.Sprintf("failed to find cache layer with SHA '%s'", diffID)) } return nil, NewReadErr(fmt.Sprintf("unknown error retrieving layer with SHA '%s'", diffID)) } From 2c58c8d83ce92a5ea7146742d8f8b370fb1c1d38 Mon Sep 17 00:00:00 2001 From: Joey Brown Date: Tue, 9 Jul 2024 10:11:00 -0500 Subject: [PATCH 07/19] Update cache/volume_cache.go Co-authored-by: Natalie Arellano Signed-off-by: Joey Brown --- cache/volume_cache.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cache/volume_cache.go b/cache/volume_cache.go index 157cbcd92..3e8af6015 100644 --- a/cache/volume_cache.go +++ b/cache/volume_cache.go @@ -149,7 +149,7 @@ func (c *VolumeCache) ReuseLayer(diffID string) error { if os.IsPermission(err) { return NewReadErr(fmt.Sprintf("Cannot read cache layer with SHA '%s' due to insufficient permissions", diffID)) } - return errors.Wrapf(err, "reusing layer (%s)", diffID) + return fmt.Errorf("failed to re-use cache layer with SHA '%s': %w", diffID, err) } if err := os.Link(committedPath, stagingPath); err != nil && !os.IsExist(err) { From 55aa2641a1fbafde63812ccbdde87647124f02b3 Mon Sep 17 00:00:00 2001 From: Joey Brown Date: Tue, 9 Jul 2024 10:11:11 -0500 Subject: [PATCH 08/19] Update cache/volume_cache.go Co-authored-by: Natalie Arellano Signed-off-by: Joey Brown --- cache/volume_cache.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cache/volume_cache.go b/cache/volume_cache.go index 3e8af6015..e588f342f 100644 --- a/cache/volume_cache.go +++ b/cache/volume_cache.go @@ -166,7 +166,7 @@ func (c *VolumeCache) RetrieveLayer(diffID string) (io.ReadCloser, error) { file, err := os.Open(path) if err != nil { if os.IsPermission(err) { - return nil, NewReadErr(fmt.Sprintf("cannot read cache layer with SHA '%s' due to insufficient permissions", diffID)) + return nil, NewReadErr(fmt.Sprintf("failed to read cache layer with SHA '%s' due to insufficient permissions", diffID)) } return nil, NewReadErr(fmt.Sprintf("unknown error retrieving layer with SHA '%s'", diffID)) } From de0806a813eadb16f3cd2a8b217adb4948979b6f Mon Sep 17 00:00:00 2001 From: Joey Brown Date: Tue, 9 Jul 2024 10:11:29 -0500 Subject: [PATCH 09/19] Update cache/volume_cache.go Co-authored-by: Natalie Arellano Signed-off-by: Joey Brown --- cache/volume_cache.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cache/volume_cache.go b/cache/volume_cache.go index e588f342f..0a0bff259 100644 --- a/cache/volume_cache.go +++ b/cache/volume_cache.go @@ -187,7 +187,7 @@ func (c *VolumeCache) RetrieveLayerFile(diffID string) (string, error) { path := diffIDPath(c.committedDir, diffID) if _, err := os.Stat(path); err != nil { if os.IsNotExist(err) { - return "", NewReadErr(fmt.Sprintf("Layer with SHA '%s' not found", diffID)) + return "", NewReadErr(fmt.Sprintf("failed to find cache layer with SHA '%s'", diffID)) } return "", errors.Wrapf(err, "retrieving layer with SHA '%s'", diffID) } From 1c304d5adb643e18e31d94802dbf25b8d5d532f7 Mon Sep 17 00:00:00 2001 From: Joey Brown Date: Tue, 9 Jul 2024 10:11:51 -0500 Subject: [PATCH 10/19] Update cache/volume_cache.go Co-authored-by: Natalie Arellano Signed-off-by: Joey Brown --- cache/volume_cache.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cache/volume_cache.go b/cache/volume_cache.go index 0a0bff259..71834486a 100644 --- a/cache/volume_cache.go +++ b/cache/volume_cache.go @@ -147,7 +147,7 @@ func (c *VolumeCache) ReuseLayer(diffID string) error { return NewReadErr(fmt.Sprintf("Layer with SHA '%s' not found", diffID)) } if os.IsPermission(err) { - return NewReadErr(fmt.Sprintf("Cannot read cache layer with SHA '%s' due to insufficient permissions", diffID)) + return NewReadErr(fmt.Sprintf("failed to read cache layer with SHA '%s' due to insufficient permissions", diffID)) } return fmt.Errorf("failed to re-use cache layer with SHA '%s': %w", diffID, err) } From 80d089fc8ec4c0cccc0cfaa369863ebfc0b3d689 Mon Sep 17 00:00:00 2001 From: Joey Brown Date: Tue, 9 Jul 2024 11:13:56 -0500 Subject: [PATCH 11/19] Update cache/volume_cache.go Co-authored-by: Natalie Arellano Signed-off-by: Joey Brown --- cache/volume_cache.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cache/volume_cache.go b/cache/volume_cache.go index 71834486a..27da502de 100644 --- a/cache/volume_cache.go +++ b/cache/volume_cache.go @@ -144,7 +144,7 @@ func (c *VolumeCache) ReuseLayer(diffID string) error { if _, err := os.Stat(committedPath); err != nil { if os.IsNotExist(err) { - return NewReadErr(fmt.Sprintf("Layer with SHA '%s' not found", diffID)) + return NewReadErr(fmt.Sprintf("failed to find cache layer with SHA '%s'", diffID)) } if os.IsPermission(err) { return NewReadErr(fmt.Sprintf("failed to read cache layer with SHA '%s' due to insufficient permissions", diffID)) From 6a2a590d379d6af194b8b83e4c5c894d90d14601 Mon Sep 17 00:00:00 2001 From: Joey Brown Date: Tue, 9 Jul 2024 11:21:04 -0500 Subject: [PATCH 12/19] update based on feedback Signed-off-by: Joey Brown --- cache/image_cache.go | 27 +-------------------------- cache/image_cache_test.go | 4 ++-- cache/volume_cache.go | 21 +-------------------- cache/volume_cache_test.go | 8 ++++---- phase/exporter.go | 2 -- 5 files changed, 8 insertions(+), 54 deletions(-) diff --git a/cache/image_cache.go b/cache/image_cache.go index 461a44463..7023b7849 100644 --- a/cache/image_cache.go +++ b/cache/image_cache.go @@ -119,7 +119,7 @@ func (c *ImageCache) RetrieveLayer(diffID string) (io.ReadCloser, error) { if IsLayerNotFound(err) { return nil, NewReadErr(fmt.Sprintf("failed to find cache layer with SHA '%s'", diffID)) } - return nil, NewReadErr(fmt.Sprintf("unknown error retrieving layer with SHA '%s'", diffID)) + return nil, fmt.Errorf("failed to get cache layer with SHA '%s'", diffID) } return closer, nil } @@ -143,28 +143,3 @@ func (c *ImageCache) Commit() error { return nil } - -// LayerExists checks if a layer exists in the cache -func (c *ImageCache) LayerExists(diffID string) (bool, error) { - layers, err := c.origImage.UnderlyingImage().Layers() - if err != nil { - return false, errors.Wrap(err, "getting image layers") - } - - for _, layer := range layers { - d, err := layer.DiffID() - if err != nil { - return false, errors.Wrap(err, "getting layer diffID") - } - - if d.String() == diffID { - return true, nil - } - } - return false, nil -} - -// Destroy deletes the cache image -func (c *ImageCache) Destroy() { - c.imageDeleter.DeleteImage(c.origImage) -} diff --git a/cache/image_cache_test.go b/cache/image_cache_test.go index 67e370bf5..3a92c45e5 100644 --- a/cache/image_cache_test.go +++ b/cache/image_cache_test.go @@ -146,7 +146,7 @@ func testImageCache(t *testing.T, when spec.G, it spec.S) { when("layer does not exist", func() { it("returns an error", func() { _, err := subject.RetrieveLayer("some_nonexistent_sha") - h.AssertError(t, err, "unknown error retrieving layer with SHA 'some_nonexistent_sha'") + h.AssertError(t, err, "failed to get cache layer with SHA 'some_nonexistent_sha'") }) }) }) @@ -236,7 +236,7 @@ func testImageCache(t *testing.T, when spec.G, it spec.S) { h.AssertNil(t, subject.AddLayerFile(testLayerTarPath, testLayerSHA)) _, err := subject.RetrieveLayer(testLayerSHA) - h.AssertError(t, err, fmt.Sprintf("unknown error retrieving layer with SHA '%s'", testLayerSHA)) + h.AssertError(t, err, fmt.Sprintf("failed to get cache layer with SHA '%s'", testLayerSHA)) }) }) }) diff --git a/cache/volume_cache.go b/cache/volume_cache.go index 27da502de..ce1a5ee2a 100644 --- a/cache/volume_cache.go +++ b/cache/volume_cache.go @@ -168,7 +168,7 @@ func (c *VolumeCache) RetrieveLayer(diffID string) (io.ReadCloser, error) { if os.IsPermission(err) { return nil, NewReadErr(fmt.Sprintf("failed to read cache layer with SHA '%s' due to insufficient permissions", diffID)) } - return nil, NewReadErr(fmt.Sprintf("unknown error retrieving layer with SHA '%s'", diffID)) + return nil, fmt.Errorf("failed to get cache layer with SHA '%s'", diffID) } return file, nil } @@ -228,22 +228,3 @@ func (c *VolumeCache) setupStagingDir() error { } return os.MkdirAll(c.stagingDir, 0777) } - -// LayerExists returns true if the layer with the given diffID exists in the cache -func (c *VolumeCache) LayerExists(diffID string) (bool, error) { - path := diffIDPath(c.committedDir, diffID) - if _, err := os.Stat(path); err != nil { - if os.IsNotExist(err) { - return false, nil - } - return false, errors.Wrapf(err, "retrieving layer with SHA '%s'", diffID) - } - return true, nil -} - -// Destroy removes the cache directory and all its contents -func (c *VolumeCache) Destroy() { - if err := os.RemoveAll(c.dir); err != nil { - c.logger.Warnf("Unable to delete cache directory: %v", err.Error()) - } -} diff --git a/cache/volume_cache_test.go b/cache/volume_cache_test.go index 161d72e2c..fe14f529e 100644 --- a/cache/volume_cache_test.go +++ b/cache/volume_cache_test.go @@ -211,7 +211,7 @@ func testVolumeCache(t *testing.T, when spec.G, it spec.S) { when("layer does not exist", func() { it("returns an error", func() { _, err := subject.RetrieveLayer("some_nonexistent_sha") - h.AssertError(t, err, "Layer with SHA 'some_nonexistent_sha' not found") + h.AssertError(t, err, "failed to find cache layer with SHA 'some_nonexistent_sha'") }) }) }) @@ -235,7 +235,7 @@ func testVolumeCache(t *testing.T, when spec.G, it spec.S) { when("layer does not exist", func() { it("returns an error", func() { _, err := subject.RetrieveLayerFile("some_nonexistent_sha") - h.AssertError(t, err, "Layer with SHA 'some_nonexistent_sha' not found") + h.AssertError(t, err, "failed to find cache layer with SHA 'some_nonexistent_sha'") }) }) }) @@ -345,7 +345,7 @@ func testVolumeCache(t *testing.T, when spec.G, it spec.S) { h.AssertNil(t, subject.AddLayerFile(tarPath, "some_sha")) _, err := subject.RetrieveLayer("some_sha") - h.AssertError(t, err, "Layer with SHA 'some_sha' not found") + h.AssertError(t, err, "failed to find cache layer with SHA 'some_sha'") }) }) @@ -420,7 +420,7 @@ func testVolumeCache(t *testing.T, when spec.G, it spec.S) { h.AssertNil(t, subject.AddLayer(layerReader, layerSha)) _, err := subject.RetrieveLayer(layerSha) - h.AssertError(t, err, fmt.Sprintf("Layer with SHA '%s' not found", layerSha)) + h.AssertError(t, err, fmt.Sprintf("failed to find cache layer with SHA '%s'", layerSha)) }) }) diff --git a/phase/exporter.go b/phase/exporter.go index a6bce1dd9..4808e8572 100644 --- a/phase/exporter.go +++ b/phase/exporter.go @@ -34,9 +34,7 @@ type Cache interface { AddLayerFile(tarPath string, sha string) error ReuseLayer(sha string) error RetrieveLayer(sha string) (io.ReadCloser, error) - LayerExists(sha string) (bool, error) Commit() error - Destroy() } type Exporter struct { From dcea1b395d7d5d2006e7096403328f66a4d9dfb5 Mon Sep 17 00:00:00 2001 From: Joey Brown Date: Tue, 9 Jul 2024 11:58:11 -0500 Subject: [PATCH 13/19] fix log --- phase/cache.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phase/cache.go b/phase/cache.go index 4d190b58a..4467df0d8 100644 --- a/phase/cache.go +++ b/phase/cache.go @@ -107,7 +107,7 @@ func (e *Exporter) addOrReuseCacheLayer(cache Cache, layerDir LayerDir, previous if !isReadErr { return "", errors.Wrapf(err, "reusing layer %s", layer.ID) } - e.Logger.Warnf(r.Logger.Warnf("Skipping re-use for layer %s: %s", layer.ID, readErr.Error())) + e.Logger.Warnf("Skipping re-use for layer %s: %s", layer.ID, readErr.Error()) } } e.Logger.Infof("Adding cache layer '%s'\n", layer.ID) From dba6b9a5f9729e6567326b75e375e13dc98956a3 Mon Sep 17 00:00:00 2001 From: Joey Brown Date: Thu, 11 Jul 2024 22:29:19 -0500 Subject: [PATCH 14/19] temp fix --- cache/image_cache.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cache/image_cache.go b/cache/image_cache.go index 7023b7849..67a3437d0 100644 --- a/cache/image_cache.go +++ b/cache/image_cache.go @@ -5,6 +5,7 @@ import ( "fmt" "io" "runtime" + "strings" "github.com/buildpacks/imgutil" "github.com/buildpacks/imgutil/remote" @@ -116,7 +117,8 @@ func IsLayerNotFound(err error) bool { func (c *ImageCache) RetrieveLayer(diffID string) (io.ReadCloser, error) { closer, err := c.origImage.GetLayer(diffID) if err != nil { - if IsLayerNotFound(err) { + // TODO: This is a workaround for a bug in the imgutil library where it returns an error when the layer is not found + if IsLayerNotFound(err) || strings.Contains(err.Error(), "EOF") { return nil, NewReadErr(fmt.Sprintf("failed to find cache layer with SHA '%s'", diffID)) } return nil, fmt.Errorf("failed to get cache layer with SHA '%s'", diffID) From 67a8cf5e2b3e4fd54319639f3c03a7af8c58f343 Mon Sep 17 00:00:00 2001 From: Joey Brown Date: Thu, 11 Jul 2024 22:43:25 -0500 Subject: [PATCH 15/19] this does not work as is. I think we need to modify img utils. Image utils should fail with a Layer Not found in both ReuseLayer & GetLayer. For GetLayer, when there is a missing blob, it's return an unexpected EOF error. For ReuseLayer, when there is a missing blob, it's not returning an error but it should. --- cache/image_cache.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/cache/image_cache.go b/cache/image_cache.go index 67a3437d0..4e7c19665 100644 --- a/cache/image_cache.go +++ b/cache/image_cache.go @@ -5,7 +5,6 @@ import ( "fmt" "io" "runtime" - "strings" "github.com/buildpacks/imgutil" "github.com/buildpacks/imgutil/remote" @@ -104,7 +103,14 @@ func (c *ImageCache) ReuseLayer(diffID string) error { if c.committed { return errCacheCommitted } - return c.newImage.ReuseLayer(diffID) + err := c.origImage.ReuseLayer(diffID) + if err != nil { + if IsLayerNotFound(err) { + return NewReadErr(fmt.Sprintf("failed to find cache layer with SHA '%s'", diffID)) + } + return fmt.Errorf("failed to reuse cache layer with SHA '%s'", diffID) + } + return nil } // IsLayerNotFound checks if the error is a layer not found error @@ -117,8 +123,7 @@ func IsLayerNotFound(err error) bool { func (c *ImageCache) RetrieveLayer(diffID string) (io.ReadCloser, error) { closer, err := c.origImage.GetLayer(diffID) if err != nil { - // TODO: This is a workaround for a bug in the imgutil library where it returns an error when the layer is not found - if IsLayerNotFound(err) || strings.Contains(err.Error(), "EOF") { + if IsLayerNotFound(err) { return nil, NewReadErr(fmt.Sprintf("failed to find cache layer with SHA '%s'", diffID)) } return nil, fmt.Errorf("failed to get cache layer with SHA '%s'", diffID) From 1b5aa7ca4448445383f59d77c6329be3287f4a03 Mon Sep 17 00:00:00 2001 From: Joey Brown Date: Mon, 15 Jul 2024 16:29:19 -0500 Subject: [PATCH 16/19] add eof check --- cache/image_cache.go | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/cache/image_cache.go b/cache/image_cache.go index 4e7c19665..e21ce50ad 100644 --- a/cache/image_cache.go +++ b/cache/image_cache.go @@ -99,13 +99,27 @@ func (c *ImageCache) AddLayerFile(tarPath string, diffID string) error { return c.newImage.AddLayerWithDiffID(tarPath, diffID) } +// isLayerNotFound checks if the error is a layer not found error +// +// FIXME: we should not have to rely on trapping ErrUnexpectedEOF. +// If a blob is not present in the registry, we should get imgutil.ErrLayerNotFound, +// but we do not and instead get io.ErrUnexpectedEOF +func isLayerNotFound(err error) bool { + var e imgutil.ErrLayerNotFound + return errors.As(err, &e) || errors.Is(err, io.ErrUnexpectedEOF) +} + func (c *ImageCache) ReuseLayer(diffID string) error { if c.committed { return errCacheCommitted } err := c.origImage.ReuseLayer(diffID) if err != nil { - if IsLayerNotFound(err) { + // FIXME: this path is not currently executed. + // If a blob is not present in the registry, we should get imgutil.ErrLayerNotFound. + // We should then skip attempting to reuse the layer. + // However, we do not get imgutil.ErrLayerNotFound when the blob is not present. + if isLayerNotFound(err) { return NewReadErr(fmt.Sprintf("failed to find cache layer with SHA '%s'", diffID)) } return fmt.Errorf("failed to reuse cache layer with SHA '%s'", diffID) @@ -113,17 +127,11 @@ func (c *ImageCache) ReuseLayer(diffID string) error { return nil } -// IsLayerNotFound checks if the error is a layer not found error -func IsLayerNotFound(err error) bool { - var e imgutil.ErrLayerNotFound - return errors.As(err, &e) -} - // RetrieveLayer retrieves a layer from the cache func (c *ImageCache) RetrieveLayer(diffID string) (io.ReadCloser, error) { closer, err := c.origImage.GetLayer(diffID) if err != nil { - if IsLayerNotFound(err) { + if isLayerNotFound(err) { return nil, NewReadErr(fmt.Sprintf("failed to find cache layer with SHA '%s'", diffID)) } return nil, fmt.Errorf("failed to get cache layer with SHA '%s'", diffID) From 6a779e31e565191da401ffe73bbbad9be5d252f3 Mon Sep 17 00:00:00 2001 From: Joey Brown Date: Tue, 16 Jul 2024 09:36:00 -0500 Subject: [PATCH 17/19] add not exist check --- cache/volume_cache.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cache/volume_cache.go b/cache/volume_cache.go index ce1a5ee2a..3ec0003cc 100644 --- a/cache/volume_cache.go +++ b/cache/volume_cache.go @@ -168,6 +168,9 @@ func (c *VolumeCache) RetrieveLayer(diffID string) (io.ReadCloser, error) { if os.IsPermission(err) { return nil, NewReadErr(fmt.Sprintf("failed to read cache layer with SHA '%s' due to insufficient permissions", diffID)) } + if os.IsNotExist(err) { + return nil, NewReadErr(fmt.Sprintf("failed to find cache layer with SHA '%s'", diffID)) + } return nil, fmt.Errorf("failed to get cache layer with SHA '%s'", diffID) } return file, nil From ba02977ecf91923b27bf8fea2e0d3ec6b5d14470 Mon Sep 17 00:00:00 2001 From: Joey Brown Date: Tue, 16 Jul 2024 14:04:46 -0500 Subject: [PATCH 18/19] reuse layer test --- cache/volume_cache_test.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/cache/volume_cache_test.go b/cache/volume_cache_test.go index fe14f529e..e7df2ef2a 100644 --- a/cache/volume_cache_test.go +++ b/cache/volume_cache_test.go @@ -512,6 +512,21 @@ func testVolumeCache(t *testing.T, when spec.G, it spec.S) { h.AssertEq(t, string(bytes), "existing data") }) }) + + when("the layer does not exist", func() { + it("fails with a read error", func() { + err := subject.ReuseLayer("some_nonexistent_sha") + isReadErr, _ := cache.IsReadErr(err) + h.AssertEq(t, isReadErr, true) + + err = subject.Commit() + h.AssertNil(t, err) + + _, err = subject.RetrieveLayer("some_sha") + isReadErr, _ = cache.IsReadErr(err) + h.AssertEq(t, isReadErr, true) + }) + }) }) when("attempting to commit more than once", func() { From 289442aba7b5164ab1da48dc00e8c5852eaa4986 Mon Sep 17 00:00:00 2001 From: Joey Brown Date: Tue, 16 Jul 2024 14:21:18 -0500 Subject: [PATCH 19/19] fix test regression --- cache/image_cache.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cache/image_cache.go b/cache/image_cache.go index e21ce50ad..a029c87dc 100644 --- a/cache/image_cache.go +++ b/cache/image_cache.go @@ -113,7 +113,7 @@ func (c *ImageCache) ReuseLayer(diffID string) error { if c.committed { return errCacheCommitted } - err := c.origImage.ReuseLayer(diffID) + err := c.newImage.ReuseLayer(diffID) if err != nil { // FIXME: this path is not currently executed. // If a blob is not present in the registry, we should get imgutil.ErrLayerNotFound.