From 0b0a995d9d45fed35725d8ec58cf9fe1e0cdc54f Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 21 Jan 2022 13:48:44 +0100 Subject: [PATCH 1/2] distribution: remove RootFSDownloadManager interface This interface only had a single implementation (xfer.LayerDownloadManager), and all places where it was used already imported the xfer package. Removing the interface, also makes it a closer match to the "upload" part, as `xfer.LayerUploadManager()` did not use an interface. Signed-off-by: Sebastiaan van Stijn --- builder/builder-next/adapters/containerimage/pull.go | 3 +-- builder/builder-next/worker/worker.go | 3 +-- daemon/images/service.go | 3 +-- distribution/config.go | 11 +---------- 4 files changed, 4 insertions(+), 16 deletions(-) diff --git a/builder/builder-next/adapters/containerimage/pull.go b/builder/builder-next/adapters/containerimage/pull.go index 0926b36ade..0cf7358822 100644 --- a/builder/builder-next/adapters/containerimage/pull.go +++ b/builder/builder-next/adapters/containerimage/pull.go @@ -22,7 +22,6 @@ import ( "github.com/containerd/containerd/remotes/docker/schema1" distreference "github.com/docker/distribution/reference" dimages "github.com/docker/docker/daemon/images" - "github.com/docker/docker/distribution" "github.com/docker/docker/distribution/metadata" "github.com/docker/docker/distribution/xfer" "github.com/docker/docker/image" @@ -52,7 +51,7 @@ type SourceOpt struct { ContentStore content.Store CacheAccessor cache.Accessor ReferenceStore reference.Store - DownloadManager distribution.RootFSDownloadManager + DownloadManager *xfer.LayerDownloadManager MetadataStore metadata.V2MetadataService ImageStore image.Store RegistryHosts docker.RegistryHosts diff --git a/builder/builder-next/worker/worker.go b/builder/builder-next/worker/worker.go index 9476cb20a9..b02e206470 100644 --- a/builder/builder-next/worker/worker.go +++ b/builder/builder-next/worker/worker.go @@ -14,7 +14,6 @@ import ( "github.com/containerd/containerd/platforms" "github.com/containerd/containerd/rootfs" "github.com/docker/docker/builder/builder-next/adapters/containerimage" - "github.com/docker/docker/distribution" distmetadata "github.com/docker/docker/distribution/metadata" "github.com/docker/docker/distribution/xfer" "github.com/docker/docker/image" @@ -70,7 +69,7 @@ type Opt struct { ContentStore content.Store CacheManager cache.Manager ImageSource *containerimage.Source - DownloadManager distribution.RootFSDownloadManager + DownloadManager *xfer.LayerDownloadManager V2MetadataService distmetadata.V2MetadataService Transport nethttp.RoundTripper Exporter exporter.Exporter diff --git a/daemon/images/service.go b/daemon/images/service.go index 85c3bd5d40..67883df81d 100644 --- a/daemon/images/service.go +++ b/daemon/images/service.go @@ -11,7 +11,6 @@ import ( "github.com/docker/docker/api/types/filters" "github.com/docker/docker/container" daemonevents "github.com/docker/docker/daemon/events" - "github.com/docker/docker/distribution" "github.com/docker/docker/distribution/metadata" "github.com/docker/docker/distribution/xfer" "github.com/docker/docker/image" @@ -95,7 +94,7 @@ type ImageService struct { // DistributionServices provides daemon image storage services type DistributionServices struct { - DownloadManager distribution.RootFSDownloadManager + DownloadManager *xfer.LayerDownloadManager V2MetadataService metadata.V2MetadataService LayerStore layer.Store ImageStore image.Store diff --git a/distribution/config.go b/distribution/config.go index 48b0b46e26..220697f060 100644 --- a/distribution/config.go +++ b/distribution/config.go @@ -56,7 +56,7 @@ type ImagePullConfig struct { Config // DownloadManager manages concurrent pulls. - DownloadManager RootFSDownloadManager + DownloadManager *xfer.LayerDownloadManager // Schema2Types is the valid schema2 configuration types allowed // by the pull operation. Schema2Types []string @@ -107,15 +107,6 @@ type PushLayer interface { Release() } -// RootFSDownloadManager handles downloading of the rootfs -type RootFSDownloadManager interface { - // Download downloads the layers into the given initial rootfs and - // returns the final rootfs. - // Given progress output to track download progress - // Returns function to release download resources - Download(ctx context.Context, initialRootFS image.RootFS, os string, layers []xfer.DownloadDescriptor, progressOutput progress.Output) (image.RootFS, func(), error) -} - type imageConfigStore struct { image.Store } From bb66ebd62120efea46a8f012db2320531852cb5d Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 21 Jan 2022 13:34:37 +0100 Subject: [PATCH 2/2] distribution: xfer.LayerDownloadManager.Download(): remove "os" argument This argument was added for LCOW support, but it was only used to verify if the passed platform (OS) matched the host. Given that all uses of this function (except for one) passed runtime.GOOS, we may as well move the check to that location. We should do more cleaning up after this, and perform such validations early, instead of passing platform around in too many places where it's only used for similar validations. This is a first step in that direction. Signed-off-by: Sebastiaan van Stijn --- .../builder-next/adapters/containerimage/pull.go | 3 +-- builder/builder-next/worker/worker.go | 3 +-- distribution/pull_v2.go | 10 ++++++++-- distribution/xfer/download.go | 14 +------------- distribution/xfer/download_test.go | 6 +++--- 5 files changed, 14 insertions(+), 22 deletions(-) diff --git a/builder/builder-next/adapters/containerimage/pull.go b/builder/builder-next/adapters/containerimage/pull.go index 0cf7358822..9deb5c21a5 100644 --- a/builder/builder-next/adapters/containerimage/pull.go +++ b/builder/builder-next/adapters/containerimage/pull.go @@ -6,7 +6,6 @@ import ( "fmt" "io" "path" - "runtime" "sync" "time" @@ -563,7 +562,7 @@ func (p *puller) Snapshot(ctx context.Context, g session.Group) (cache.Immutable }() r := image.NewRootFS() - rootFS, release, err := p.is.DownloadManager.Download(ctx, *r, runtime.GOOS, layers, pkgprogress.ChanOutput(pchan)) + rootFS, release, err := p.is.DownloadManager.Download(ctx, *r, layers, pkgprogress.ChanOutput(pchan)) stopProgress() if err != nil { return nil, err diff --git a/builder/builder-next/worker/worker.go b/builder/builder-next/worker/worker.go index b02e206470..ebf149d351 100644 --- a/builder/builder-next/worker/worker.go +++ b/builder/builder-next/worker/worker.go @@ -5,7 +5,6 @@ import ( "fmt" "io" nethttp "net/http" - "runtime" "strings" "time" @@ -356,7 +355,7 @@ func (w *Worker) FromRemote(ctx context.Context, remote *solver.Remote) (cache.I }() r := image.NewRootFS() - rootFS, release, err := w.DownloadManager.Download(ctx, *r, runtime.GOOS, layers, &discardProgress{}) + rootFS, release, err := w.DownloadManager.Download(ctx, *r, layers, &discardProgress{}) if err != nil { return nil, err } diff --git a/distribution/pull_v2.go b/distribution/pull_v2.go index 296f47372b..07cc1e5b86 100644 --- a/distribution/pull_v2.go +++ b/distribution/pull_v2.go @@ -547,7 +547,7 @@ func (p *v2Puller) pullSchema1(ctx context.Context, ref reference.Reference, unv descriptors = append(descriptors, layerDescriptor) } - resultRootFS, release, err := p.config.DownloadManager.Download(ctx, *rootFS, runtime.GOOS, descriptors, p.config.ProgressOutput) + resultRootFS, release, err := p.config.DownloadManager.Download(ctx, *rootFS, descriptors, p.config.ProgressOutput) if err != nil { return "", "", err } @@ -665,6 +665,12 @@ func (p *v2Puller) pullSchema2Layers(ctx context.Context, target distribution.De } } + // Assume that the operating system is the host OS if blank, and validate it + // to ensure we don't cause a panic by an invalid index into the layerstores. + if layerStoreOS != "" && !system.IsOSSupported(layerStoreOS) { + return "", system.ErrNotSupportedOperatingSystem + } + if p.config.DownloadManager != nil { go func() { var ( @@ -672,7 +678,7 @@ func (p *v2Puller) pullSchema2Layers(ctx context.Context, target distribution.De rootFS image.RootFS ) downloadRootFS := *image.NewRootFS() - rootFS, release, err = p.config.DownloadManager.Download(ctx, downloadRootFS, layerStoreOS, descriptors, p.config.ProgressOutput) + rootFS, release, err = p.config.DownloadManager.Download(ctx, downloadRootFS, descriptors, p.config.ProgressOutput) if err != nil { // Intentionally do not cancel the config download here // as the error from config download (if there is one) diff --git a/distribution/xfer/download.go b/distribution/xfer/download.go index 847152d081..6eddfac0dd 100644 --- a/distribution/xfer/download.go +++ b/distribution/xfer/download.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "io" - "runtime" "time" "github.com/docker/distribution" @@ -14,7 +13,6 @@ import ( "github.com/docker/docker/pkg/archive" "github.com/docker/docker/pkg/ioutils" "github.com/docker/docker/pkg/progress" - "github.com/docker/docker/pkg/system" "github.com/sirupsen/logrus" ) @@ -106,7 +104,7 @@ type DownloadDescriptorWithRegistered interface { // Download method is called to get the layer tar data. Layers are then // registered in the appropriate order. The caller must call the returned // release function once it is done with the returned RootFS object. -func (ldm *LayerDownloadManager) Download(ctx context.Context, initialRootFS image.RootFS, os string, layers []DownloadDescriptor, progressOutput progress.Output) (image.RootFS, func(), error) { +func (ldm *LayerDownloadManager) Download(ctx context.Context, initialRootFS image.RootFS, layers []DownloadDescriptor, progressOutput progress.Output) (image.RootFS, func(), error) { var ( topLayer layer.Layer topDownload *downloadTransfer @@ -116,16 +114,6 @@ func (ldm *LayerDownloadManager) Download(ctx context.Context, initialRootFS ima downloadsByKey = make(map[string]*downloadTransfer) ) - // Assume that the operating system is the host OS if blank, and validate it - // to ensure we don't cause a panic by an invalid index into the layerstores. - // TODO remove now that LCOW is no longer a thing - if os == "" { - os = runtime.GOOS - } - if !system.IsOSSupported(os) { - return image.RootFS{}, nil, system.ErrNotSupportedOperatingSystem - } - rootFS := initialRootFS for _, descriptor := range layers { key := descriptor.Key() diff --git a/distribution/xfer/download_test.go b/distribution/xfer/download_test.go index fbf7f541af..8253c75f1a 100644 --- a/distribution/xfer/download_test.go +++ b/distribution/xfer/download_test.go @@ -293,7 +293,7 @@ func TestSuccessfulDownload(t *testing.T) { } firstDescriptor.diffID = l.DiffID() - rootFS, releaseFunc, err := ldm.Download(context.Background(), *image.NewRootFS(), runtime.GOOS, descriptors, progress.ChanOutput(progressChan)) + rootFS, releaseFunc, err := ldm.Download(context.Background(), *image.NewRootFS(), descriptors, progress.ChanOutput(progressChan)) if err != nil { t.Fatalf("download error: %v", err) } @@ -348,7 +348,7 @@ func TestCancelledDownload(t *testing.T) { }() descriptors := downloadDescriptors(nil) - _, _, err := ldm.Download(ctx, *image.NewRootFS(), runtime.GOOS, descriptors, progress.ChanOutput(progressChan)) + _, _, err := ldm.Download(ctx, *image.NewRootFS(), descriptors, progress.ChanOutput(progressChan)) if err != context.Canceled { t.Fatal("expected download to be cancelled") } @@ -412,7 +412,7 @@ func TestMaxDownloadAttempts(t *testing.T) { descriptors := downloadDescriptors(¤tDownloads) descriptors[4].(*mockDownloadDescriptor).simulateRetries = tc.simulateRetries - _, _, err := ldm.Download(context.Background(), *image.NewRootFS(), runtime.GOOS, descriptors, progress.ChanOutput(progressChan)) + _, _, err := ldm.Download(context.Background(), *image.NewRootFS(), descriptors, progress.ChanOutput(progressChan)) if tc.expectedErr == "" { assert.NilError(t, err) } else {