From 5b527d0f166a68264c0526afa3033294a67bcbdf Mon Sep 17 00:00:00 2001 From: Emily Casey Date: Fri, 4 Dec 2020 09:53:59 -0500 Subject: [PATCH 1/6] Fallback to slower behavior if performance hack fails When saving an image to the docker daemon we currently omit base image layers because docker does not require them and fetching them requires a slow image save. However, some runtimes that implement the daemon api have stricter validation so we will fallback to the slower behavior if the first save fails Signed-off-by: Emily Casey --- go.sum | 2 + local/local.go | 204 ++++++++++++++++++-------------------------- local/local_test.go | 6 +- remote/remote.go | 2 +- 4 files changed, 87 insertions(+), 127 deletions(-) diff --git a/go.sum b/go.sum index d28ef220..8af7617d 100644 --- a/go.sum +++ b/go.sum @@ -196,10 +196,12 @@ github.com/google/go-cmp v0.5.2 h1:X2ev0eStA3AbceY54o37/0PQ/UWqKEiiO2dKL5OPaFM= github.com/google/go-cmp v0.5.2/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/google/go-cmp v0.5.3 h1:x95R7cp+rSeeqAMI2knLtQ0DKlaBhv2NrtrOvafPHRo= github.com/google/go-cmp v0.5.3/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= +github.com/google/go-cmp v0.5.4 h1:L8R9j+yAqZuZjsqh/z+F1NCffTKKLShY6zXTItVIZ8M= github.com/google/go-cmp v0.5.4/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/google/go-containerregistry v0.1.4 h1:fZm+V2pYnvb8NMPM1YOsyxr31XKfpHTun5oVTRnG8qc= github.com/google/go-containerregistry v0.1.4/go.mod h1:6EGiuQp36pL82lX6rFN0s9AJOVL0Mlgx/DAsYZW5X3s= github.com/google/go-containerregistry v0.2.0/go.mod h1:Ts3Wioz1r5ayWx8sS6vLcWltWcM1aqFjd/eVrkFhrWM= +github.com/google/go-containerregistry v0.2.1 h1:LLZgLTDguTVJ9eEHh/zTtr347CpFhH6MSYculNas5bY= github.com/google/go-containerregistry v0.2.1/go.mod h1:Ts3Wioz1r5ayWx8sS6vLcWltWcM1aqFjd/eVrkFhrWM= github.com/google/gofuzz v0.0.0-20161122191042-44d81051d367/go.mod h1:HP5RmnzzSNb993RKQDq4+1A4ia9nllfqcQFTQJedwGI= github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= diff --git a/local/local.go b/local/local.go index cb710a4e..20dad6c9 100644 --- a/local/local.go +++ b/local/local.go @@ -27,19 +27,12 @@ import ( ) type Image struct { - repoName string - docker client.CommonAPIClient - inspect types.ImageInspect - layerPaths []string - downloadOnce *sync.Once - prevName string - prevImage *FileSystemLocalImage - easyAddLayers []string -} - -type FileSystemLocalImage struct { - dir string - layersMap map[string]string + repoName string + docker client.CommonAPIClient + inspect types.ImageInspect + layerPaths []string + prevImage *Image // reused layers will be fetched from prevImage + downloadBaseOnce *sync.Once } type ImageOption func(image *Image) (*Image, error) @@ -50,7 +43,11 @@ func WithPreviousImage(imageName string) ImageOption { return i, err } - i.prevName = imageName + prevImage, err := NewImage(imageName, i.docker, FromBaseImage(imageName)) + if err != nil { + return nil, err + } + i.prevImage = prevImage return i, nil } @@ -74,7 +71,7 @@ func FromBaseImage(imageName string) ImageOption { } } -func NewImage(repoName string, dockerClient client.CommonAPIClient, ops ...ImageOption) (imgutil.Image, error) { +func NewImage(repoName string, dockerClient client.CommonAPIClient, ops ...ImageOption) (*Image, error) { var err error inspect, err := defaultInspect(dockerClient) @@ -83,11 +80,11 @@ func NewImage(repoName string, dockerClient client.CommonAPIClient, ops ...Image } image := &Image{ - docker: dockerClient, - repoName: repoName, - inspect: inspect, - layerPaths: make([]string, len(inspect.RootFS.Layers)), - downloadOnce: &sync.Once{}, + docker: dockerClient, + repoName: repoName, + inspect: inspect, + layerPaths: make([]string, len(inspect.RootFS.Layers)), + downloadBaseOnce: &sync.Once{}, } for _, v := range ops { @@ -136,28 +133,9 @@ func (i *Image) Architecture() (string, error) { } func (i *Image) Rename(name string) { - i.easyAddLayers = nil - if prevInspect, _, err := i.docker.ImageInspectWithRaw(context.TODO(), name); err == nil { - if i.sameBase(prevInspect) { - i.easyAddLayers = prevInspect.RootFS.Layers[len(i.inspect.RootFS.Layers):] - } - } - i.repoName = name } -func (i *Image) sameBase(prevInspect types.ImageInspect) bool { - if len(prevInspect.RootFS.Layers) < len(i.inspect.RootFS.Layers) { - return false - } - for i, baseLayer := range i.inspect.RootFS.Layers { - if baseLayer != prevInspect.RootFS.Layers[i] { - return false - } - } - return true -} - func (i *Image) Name() string { return i.repoName } @@ -186,50 +164,29 @@ func (i *Image) Rebase(baseTopLayer string, newBase imgutil.Image) error { ctx := context.Background() // FIND TOP LAYER - keepLayers := -1 + var keepLayersIdx int for idx, diffID := range i.inspect.RootFS.Layers { if diffID == baseTopLayer { - keepLayers = len(i.inspect.RootFS.Layers) - idx - 1 + keepLayersIdx = idx + 1 break } } - if keepLayers == -1 { + if keepLayersIdx == 0 { return fmt.Errorf("'%s' not found in '%s' during rebase", baseTopLayer, i.repoName) } - // SWITCH BASE LAYERS - newBaseInspect, _, err := i.docker.ImageInspectWithRaw(ctx, newBase.Name()) - if err != nil { - return errors.Wrap(err, "analyze read previous image config") - } - i.inspect.RootFS.Layers = newBaseInspect.RootFS.Layers - i.layerPaths = make([]string, len(i.inspect.RootFS.Layers)) - // DOWNLOAD IMAGE - if err := i.downloadImageOnce(i.repoName); err != nil { + if err := i.downloadImageOnce(); err != nil { return err } - // READ MANIFEST.JSON - b, err := ioutil.ReadFile(filepath.Join(i.prevImage.dir, "manifest.json")) + // SWITCH BASE LAYERS + newBaseInspect, _, err := i.docker.ImageInspectWithRaw(ctx, newBase.Name()) if err != nil { - return err - } - var manifest []struct{ Layers []string } - if err := json.Unmarshal(b, &manifest); err != nil { - return err - } - if len(manifest) != 1 { - return fmt.Errorf("expected 1 image received %d", len(manifest)) - } - - // ADD EXISTING LAYERS - for _, filename := range manifest[0].Layers[(len(manifest[0].Layers) - keepLayers):] { - if err := i.AddLayer(filepath.Join(i.prevImage.dir, filename)); err != nil { - return err - } + return errors.Wrap(err, "analyze read previous image config") } - + i.inspect.RootFS.Layers = append(newBaseInspect.RootFS.Layers, i.inspect.RootFS.Layers[keepLayersIdx:]...) + i.layerPaths = append(make([]string, len(newBaseInspect.RootFS.Layers)), i.layerPaths[keepLayersIdx:]...) return nil } @@ -310,16 +267,20 @@ func (i *Image) TopLayer() (string, error) { } func (i *Image) GetLayer(diffID string) (io.ReadCloser, error) { - err := i.downloadImageOnce(i.repoName) - if err != nil { - return nil, err + for l := range i.inspect.RootFS.Layers { + if i.inspect.RootFS.Layers[l] != diffID { + continue + } + if i.layerPaths[l] == "" { + err := i.downloadImageOnce() + if err != nil { + return nil, err + } + } + return os.Open(i.layerPaths[l]) } - layerID, ok := i.prevImage.layersMap[diffID] - if !ok { - return nil, fmt.Errorf("image '%s' does not contain layer with diff ID '%s'", i.repoName, diffID) - } - return os.Open(filepath.Join(i.prevImage.dir, layerID)) + return nil, fmt.Errorf("image '%s' does not contain layer with diff ID '%s'", i.repoName, diffID) } func (i *Image) AddLayer(path string) error { @@ -339,44 +300,47 @@ func (i *Image) AddLayer(path string) error { func (i *Image) AddLayerWithDiffID(path, diffID string) error { i.inspect.RootFS.Layers = append(i.inspect.RootFS.Layers, diffID) i.layerPaths = append(i.layerPaths, path) - i.easyAddLayers = nil return nil } func (i *Image) ReuseLayer(diffID string) error { - if len(i.easyAddLayers) > 0 && i.easyAddLayers[0] == diffID { - i.inspect.RootFS.Layers = append(i.inspect.RootFS.Layers, diffID) - i.layerPaths = append(i.layerPaths, "") - i.easyAddLayers = i.easyAddLayers[1:] - return nil - } - - if i.prevName == "" { + if i.prevImage == nil || !i.prevImage.Found() { return errors.New("no previous image provided to reuse layers from") } - err := i.downloadImageOnce(i.prevName) + err := i.prevImage.downloadImageOnce() if err != nil { return err } - reuseLayer, ok := i.prevImage.layersMap[diffID] - if !ok { - return fmt.Errorf("SHA %s was not found in %s", diffID, i.repoName) + for l := range i.prevImage.inspect.RootFS.Layers { + if i.prevImage.inspect.RootFS.Layers[l] == diffID { + return i.AddLayerWithDiffID(i.prevImage.layerPaths[l], diffID) + } } - - return i.AddLayer(filepath.Join(i.prevImage.dir, reuseLayer)) + return fmt.Errorf("SHA %s was not found in %s", diffID, i.prevImage.Name()) } func (i *Image) Save(additionalNames ...string) error { + // during the first save attempt some layers may be excluded. The docker daemon allows this if the given set + // of layers already exists in the daemon in the given order inspect, err := i.doSave() if err != nil { - saveErr := imgutil.SaveError{} - for _, n := range append([]string{i.Name()}, additionalNames...) { - saveErr.Errors = append(saveErr.Errors, imgutil.SaveDiagnostic{ImageName: n, Cause: err}) + // populate all layer paths and try again without the above performance optimization. + if err := i.downloadImageOnce(); err != nil { + return err + } + + inspect, err = i.doSave() + if err != nil { + saveErr := imgutil.SaveError{} + for _, n := range append([]string{i.Name()}, additionalNames...) { + saveErr.Errors = append(saveErr.Errors, imgutil.SaveDiagnostic{ImageName: n, Cause: err}) + } + return saveErr } - return saveErr } + i.inspect = inspect var errs []imgutil.SaveDiagnostic @@ -513,38 +477,41 @@ func (i *Image) Delete() error { return err } -func (i *Image) downloadImageOnce(imageName string) error { +// downloadImageOnce exports the base image from the daemon and populates layerPaths the first time it is called. +// subsequent calls do nothing. +func (i *Image) downloadImageOnce() error { var err error - i.downloadOnce.Do(func() { - var fsimg *FileSystemLocalImage - fsimg, err = downloadImage(i.docker, imageName) - i.prevImage = fsimg + if !i.Found() { + return nil + } + i.downloadBaseOnce.Do(func() { + err = i.downloadImage() }) return err } -func downloadImage(docker client.CommonAPIClient, imageName string) (*FileSystemLocalImage, error) { +func (i *Image) downloadImage() error { ctx := context.Background() - imageReader, err := docker.ImageSave(ctx, []string{imageName}) + imageReader, err := i.docker.ImageSave(ctx, []string{i.inspect.ID}) if err != nil { - return nil, err + return err } defer ensureReaderClosed(imageReader) tmpDir, err := ioutil.TempDir("", "imgutil.local.image.") if err != nil { - return nil, errors.Wrap(err, "local reuse-layer create temp dir") + return errors.Wrap(err, "local reuse-layer create temp dir") } err = untar(imageReader, tmpDir) if err != nil { - return nil, err + return err } mf, err := os.Open(filepath.Join(tmpDir, "manifest.json")) if err != nil { - return nil, err + return err } defer mf.Close() @@ -553,16 +520,16 @@ func downloadImage(docker client.CommonAPIClient, imageName string) (*FileSystem Layers []string } if err := json.NewDecoder(mf).Decode(&manifest); err != nil { - return nil, err + return err } if len(manifest) != 1 { - return nil, fmt.Errorf("manifest.json had unexpected number of entries: %d", len(manifest)) + return fmt.Errorf("manifest.json had unexpected number of entries: %d", len(manifest)) } df, err := os.Open(filepath.Join(tmpDir, manifest[0].Config)) if err != nil { - return nil, err + return err } defer df.Close() @@ -573,23 +540,14 @@ func downloadImage(docker client.CommonAPIClient, imageName string) (*FileSystem } if err = json.NewDecoder(df).Decode(&details); err != nil { - return nil, err - } - - if len(manifest[0].Layers) != len(details.RootFS.DiffIDs) { - return nil, fmt.Errorf("layers and diff IDs do not match, there are %d layers and %d diffIDs", len(manifest[0].Layers), len(details.RootFS.DiffIDs)) + return err } - layersMap := make(map[string]string, len(manifest[0].Layers)) - for i, diffID := range details.RootFS.DiffIDs { - layerID := manifest[0].Layers[i] - layersMap[diffID] = layerID + for l := range details.RootFS.DiffIDs { + i.layerPaths[l] = filepath.Join(tmpDir, manifest[0].Layers[l]) } - return &FileSystemLocalImage{ - dir: tmpDir, - layersMap: layersMap, - }, nil + return nil } func addTextToTar(tw *tar.Writer, name string, contents []byte) error { diff --git a/local/local_test.go b/local/local_test.go index 960641e1..d7333859 100644 --- a/local/local_test.go +++ b/local/local_test.go @@ -1171,12 +1171,12 @@ func testImage(t *testing.T, when spec.G, it spec.S) { when("image does NOT exist", func() { it("returns error", func() { - image, err := local.NewImage(newTestImageName(), dockerClient) + image, err := local.NewImage("not-exist", dockerClient) h.AssertNil(t, err) - readCloser, err := image.GetLayer(h.RandString(10)) + readCloser, err := image.GetLayer("some-layer") h.AssertNil(t, readCloser) - h.AssertError(t, err, "No such image") + h.AssertError(t, err, "image 'not-exist' does not contain layer with diff ID 'some-layer'") }) }) }) diff --git a/remote/remote.go b/remote/remote.go index 8b04c1ca..11d2a6bd 100644 --- a/remote/remote.go +++ b/remote/remote.go @@ -61,7 +61,7 @@ func FromBaseImage(imageName string) ImageOption { } } -func NewImage(repoName string, keychain authn.Keychain, ops ...ImageOption) (imgutil.Image, error) { +func NewImage(repoName string, keychain authn.Keychain, ops ...ImageOption) (*Image, error) { image, err := emptyImage() if err != nil { return nil, err From e5ba86963279b0db1be0003800ccea4c52287774 Mon Sep 17 00:00:00 2001 From: Emily Casey Date: Fri, 4 Dec 2020 10:29:18 -0500 Subject: [PATCH 2/6] Only pull test images if they don't already exist in the daemon * Tests run faster * Avoids dockerhub rate limiting Signed-off-by: Emily Casey --- acceptance/reproducibility_test.go | 5 +---- local/local_test.go | 4 ++-- testhelpers/docker_registry.go | 2 +- testhelpers/testhelpers.go | 23 +++++++++++++---------- 4 files changed, 17 insertions(+), 17 deletions(-) diff --git a/acceptance/reproducibility_test.go b/acceptance/reproducibility_test.go index 00aa2645..08efb28d 100644 --- a/acceptance/reproducibility_test.go +++ b/acceptance/reproducibility_test.go @@ -57,8 +57,7 @@ func testReproducibility(t *testing.T, when spec.G, it spec.S) { daemonOS := daemonInfo.OSType runnableBaseImageName = h.RunnableBaseImage(daemonOS) - - h.AssertNil(t, h.PullImage(dockerClient, runnableBaseImageName)) + h.PullIfMissing(t, dockerClient, runnableBaseImageName) imageName1 = newTestImageName() imageName2 = newTestImageName() @@ -107,7 +106,6 @@ func testReproducibility(t *testing.T, when spec.G, it spec.S) { }) it("local/local", func() { - h.AssertNil(t, h.PullImage(dockerClient, runnableBaseImageName)) img1, err := local.NewImage(imageName1, dockerClient, local.FromBaseImage(runnableBaseImageName)) h.AssertNil(t, err) mutateAndSave(t, img1) @@ -126,7 +124,6 @@ func testReproducibility(t *testing.T, when spec.G, it spec.S) { h.AssertNil(t, err) mutateAndSave(t, img1) - h.AssertNil(t, h.PullImage(dockerClient, runnableBaseImageName)) img2, err := local.NewImage(imageName2, dockerClient, local.FromBaseImage(runnableBaseImageName)) h.AssertNil(t, err) mutateAndSave(t, img2) diff --git a/local/local_test.go b/local/local_test.go index d7333859..362a2927 100644 --- a/local/local_test.go +++ b/local/local_test.go @@ -55,7 +55,7 @@ func testImage(t *testing.T, when spec.G, it spec.S) { daemonOS = daemonInfo.OSType runnableBaseImageName = h.RunnableBaseImage(daemonOS) - h.AssertNil(t, h.PullImage(dockerClient, runnableBaseImageName)) + h.PullIfMissing(t, dockerClient, runnableBaseImageName) }) when("#NewImage", func() { @@ -155,7 +155,7 @@ func testImage(t *testing.T, when spec.G, it spec.S) { expectedOSVersion = "10.0.17763.1040" } - h.AssertNil(t, h.PullImage(dockerClient, armBaseImageName)) + h.PullIfMissing(t, dockerClient, armBaseImageName) img, err := local.NewImage(newTestImageName(), dockerClient, local.FromBaseImage(armBaseImageName)) h.AssertNil(t, err) diff --git a/testhelpers/docker_registry.go b/testhelpers/docker_registry.go index b045a809..5f037a6e 100644 --- a/testhelpers/docker_registry.go +++ b/testhelpers/docker_registry.go @@ -54,7 +54,7 @@ func (r *DockerRegistry) Start(t *testing.T) { AssertNil(t, err) registryImageName := registryImageNames[daemonInfo.OSType] - AssertNil(t, PullImage(DockerCli(t), registryImageName)) + PullIfMissing(t, DockerCli(t), registryImageName) var htpasswdTar io.ReadCloser registryEnv := []string{"REGISTRY_STORAGE_DELETE_ENABLED=true"} diff --git a/testhelpers/testhelpers.go b/testhelpers/testhelpers.go index 66ffeea7..4e605e13 100644 --- a/testhelpers/testhelpers.go +++ b/testhelpers/testhelpers.go @@ -141,19 +141,22 @@ func Eventually(t *testing.T, test func() bool, every time.Duration, timeout tim } } -func PullImage(dockerCli dockercli.CommonAPIClient, ref string) error { - rc, err := dockerCli.ImagePull(context.Background(), ref, dockertypes.ImagePullOptions{}) +func PullIfMissing(t *testing.T, docker dockercli.CommonAPIClient, ref string) { + t.Helper() + inspect, _, err := docker.ImageInspectWithRaw(context.TODO(), ref) + AssertNil(t, err) + if inspect.ID != "" { + return + } + + rc, err := docker.ImagePull(context.Background(), ref, dockertypes.ImagePullOptions{}) if err != nil { // Retry - rc, err = dockerCli.ImagePull(context.Background(), ref, dockertypes.ImagePullOptions{}) - if err != nil { - return err - } - } - if _, err := io.Copy(ioutil.Discard, rc); err != nil { - return err + rc, err = docker.ImagePull(context.Background(), ref, dockertypes.ImagePullOptions{}) + AssertNil(t, err) } - return rc.Close() + _, err = io.Copy(ioutil.Discard, rc) + AssertNil(t, err) } func DockerRmi(dockerCli dockercli.CommonAPIClient, repoNames ...string) error { From d14de784b053c72d231c45ede83c5cdace8b2880 Mon Sep 17 00:00:00 2001 From: Emily Casey Date: Fri, 4 Dec 2020 13:04:17 -0500 Subject: [PATCH 3/6] Reset download once after rebase Fixes performance hack fallback edge case where GetLayer is called before Rebase. Signed-off-by: Emily Casey --- local/local.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/local/local.go b/local/local.go index 20dad6c9..0becbf97 100644 --- a/local/local.go +++ b/local/local.go @@ -183,8 +183,10 @@ func (i *Image) Rebase(baseTopLayer string, newBase imgutil.Image) error { // SWITCH BASE LAYERS newBaseInspect, _, err := i.docker.ImageInspectWithRaw(ctx, newBase.Name()) if err != nil { - return errors.Wrap(err, "analyze read previous image config") + return errors.Wrapf(err, "read config for new base image '%s'", newBase) } + i.inspect.ID = newBaseInspect.ID + i.downloadBaseOnce = &sync.Once{} i.inspect.RootFS.Layers = append(newBaseInspect.RootFS.Layers, i.inspect.RootFS.Layers[keepLayersIdx:]...) i.layerPaths = append(make([]string, len(newBaseInspect.RootFS.Layers)), i.layerPaths[keepLayersIdx:]...) return nil From 9c3b4cc30ed40006afff6d4b3eba76245ab2aa37 Mon Sep 17 00:00:00 2001 From: Emily Casey Date: Fri, 4 Dec 2020 13:19:24 -0500 Subject: [PATCH 4/6] Improve error messages Signed-off-by: Emily Casey --- local/local.go | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/local/local.go b/local/local.go index 0becbf97..22a88e9a 100644 --- a/local/local.go +++ b/local/local.go @@ -45,7 +45,7 @@ func WithPreviousImage(imageName string) ImageOption { prevImage, err := NewImage(imageName, i.docker, FromBaseImage(imageName)) if err != nil { - return nil, err + return nil, errors.Wrapf(err, "failed to get previous image '%s'", imageName) } i.prevImage = prevImage @@ -176,7 +176,7 @@ func (i *Image) Rebase(baseTopLayer string, newBase imgutil.Image) error { } // DOWNLOAD IMAGE - if err := i.downloadImageOnce(); err != nil { + if err := i.downloadBaseLayersOnce(); err != nil { return err } @@ -274,8 +274,7 @@ func (i *Image) GetLayer(diffID string) (io.ReadCloser, error) { continue } if i.layerPaths[l] == "" { - err := i.downloadImageOnce() - if err != nil { + if err := i.downloadBaseLayersOnce(); err != nil { return nil, err } } @@ -306,12 +305,14 @@ func (i *Image) AddLayerWithDiffID(path, diffID string) error { } func (i *Image) ReuseLayer(diffID string) error { - if i.prevImage == nil || !i.prevImage.Found() { - return errors.New("no previous image provided to reuse layers from") + if i.prevImage == nil { + return errors.New("failed to reuse layer because no previous image was provided") + } + if !i.prevImage.Found() { + return fmt.Errorf("failed to reuse layer because previous image '%s' was not found in daemon", i.prevImage.repoName) } - err := i.prevImage.downloadImageOnce() - if err != nil { + if err := i.prevImage.downloadBaseLayersOnce(); err != nil { return err } @@ -329,7 +330,7 @@ func (i *Image) Save(additionalNames ...string) error { inspect, err := i.doSave() if err != nil { // populate all layer paths and try again without the above performance optimization. - if err := i.downloadImageOnce(); err != nil { + if err := i.downloadBaseLayersOnce(); err != nil { return err } @@ -342,7 +343,6 @@ func (i *Image) Save(additionalNames ...string) error { return saveErr } } - i.inspect = inspect var errs []imgutil.SaveDiagnostic @@ -479,31 +479,34 @@ func (i *Image) Delete() error { return err } -// downloadImageOnce exports the base image from the daemon and populates layerPaths the first time it is called. +// downloadBaseLayersOnce exports the base image from the daemon and populates layerPaths the first time it is called. // subsequent calls do nothing. -func (i *Image) downloadImageOnce() error { +func (i *Image) downloadBaseLayersOnce() error { var err error if !i.Found() { return nil } i.downloadBaseOnce.Do(func() { - err = i.downloadImage() + err = i.downloadBaseLayers() }) + if err != nil { + return errors.Wrap(err, "failed to fetch base layers") + } return err } -func (i *Image) downloadImage() error { +func (i *Image) downloadBaseLayers() error { ctx := context.Background() imageReader, err := i.docker.ImageSave(ctx, []string{i.inspect.ID}) if err != nil { - return err + return errors.Wrapf(err, "failed to save base image with ID '%s' from the docker daemon", i.inspect.ID) } defer ensureReaderClosed(imageReader) tmpDir, err := ioutil.TempDir("", "imgutil.local.image.") if err != nil { - return errors.Wrap(err, "local reuse-layer create temp dir") + return errors.Wrap(err, "failed to create temp dir") } err = untar(imageReader, tmpDir) From 10b462b09602a4c4b614d044aaebd23ce02faa06 Mon Sep 17 00:00:00 2001 From: Emily Casey Date: Fri, 4 Dec 2020 13:35:58 -0500 Subject: [PATCH 5/6] Fixes PullIfMissing testhelper Signed-off-by: Emily Casey --- testhelpers/testhelpers.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/testhelpers/testhelpers.go b/testhelpers/testhelpers.go index 4e605e13..b215682d 100644 --- a/testhelpers/testhelpers.go +++ b/testhelpers/testhelpers.go @@ -143,11 +143,13 @@ func Eventually(t *testing.T, test func() bool, every time.Duration, timeout tim func PullIfMissing(t *testing.T, docker dockercli.CommonAPIClient, ref string) { t.Helper() - inspect, _, err := docker.ImageInspectWithRaw(context.TODO(), ref) - AssertNil(t, err) - if inspect.ID != "" { + _, _, err := docker.ImageInspectWithRaw(context.TODO(), ref) + if err == nil { return } + if !dockercli.IsErrNotFound(err) { + t.Fatalf("failed inspecting image '%s': %s", ref, err) + } rc, err := docker.ImagePull(context.Background(), ref, dockertypes.ImagePullOptions{}) if err != nil { From 9ccc7e390da19b48a455531b9ab163bd6508a58b Mon Sep 17 00:00:00 2001 From: Emily Casey Date: Mon, 7 Dec 2020 14:52:16 -0500 Subject: [PATCH 6/6] return meaningful error if layer is missing after download Signed-off-by: Emily Casey --- local/local.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/local/local.go b/local/local.go index 22a88e9a..dbb08dab 100644 --- a/local/local.go +++ b/local/local.go @@ -277,6 +277,9 @@ func (i *Image) GetLayer(diffID string) (io.ReadCloser, error) { if err := i.downloadBaseLayersOnce(); err != nil { return nil, err } + if i.layerPaths[l] == "" { + return nil, fmt.Errorf("failed to fetch layer '%s' from daemon", diffID) + } } return os.Open(i.layerPaths[l]) } @@ -552,6 +555,12 @@ func (i *Image) downloadBaseLayers() error { i.layerPaths[l] = filepath.Join(tmpDir, manifest[0].Layers[l]) } + for l := range i.layerPaths { + if i.layerPaths[l] == "" { + return errors.New("failed to download all base layers from daemon") + } + } + return nil }