From 142b2b785b4ee50f7d1998daa21230b51a3a6c68 Mon Sep 17 00:00:00 2001 From: "Paul \"TBBle\" Hampson" Date: Thu, 5 Nov 2020 20:39:40 +1100 Subject: [PATCH 1/5] Add TestBuildWCOWSandboxSize integration test This test validates that `RUN` and `COPY` both target a read-write sandbox on Windows that is configured according to the daemon's `storage-opts` setting. Sadly, this is a slow test, so we need to bump the timeout to 60 minutes from the default of 10 minutes. Signed-off-by: Paul "TBBle" Hampson --- hack/make.ps1 | 2 +- integration/build/build_test.go | 40 +++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/hack/make.ps1 b/hack/make.ps1 index d7e4dfa9c1..ea6d5269b3 100644 --- a/hack/make.ps1 +++ b/hack/make.ps1 @@ -363,7 +363,7 @@ Function Run-IntegrationTests() { $pinfo.FileName = "gotestsum.exe" $pinfo.WorkingDirectory = "$($PWD.Path)" $pinfo.UseShellExecute = $false - $pinfo.Arguments = "--format=standard-verbose --jsonfile=$jsonFilePath --junitfile=$xmlFilePath -- $env:INTEGRATION_TESTFLAGS" + $pinfo.Arguments = "--format=standard-verbose --jsonfile=$jsonFilePath --junitfile=$xmlFilePath -- -test.timeout=60m $env:INTEGRATION_TESTFLAGS" $p = New-Object System.Diagnostics.Process $p.StartInfo = $pinfo $p.Start() | Out-Null diff --git a/integration/build/build_test.go b/integration/build/build_test.go index b916a25206..be4ce47fe6 100644 --- a/integration/build/build_test.go +++ b/integration/build/build_test.go @@ -523,6 +523,46 @@ RUN for g in $(seq 0 8); do dd if=/dev/urandom of=rnd bs=1K count=1 seek=$((1024 assert.Check(t, is.Contains(out.String(), "Successfully built")) } +func TestBuildWCOWSandboxSize(t *testing.T) { + skip.If(t, testEnv.DaemonInfo.OSType != "windows", "only Windows has sandbox size control") + ctx := context.TODO() + defer setupTest(t)() + + dockerfile := `FROM busybox AS intermediate +WORKDIR C:\\stuff +# Create and delete a 21GB file +RUN fsutil file createnew C:\\stuff\\bigfile_0.txt 22548578304 && del bigfile_0.txt +# Create three 7GB files +RUN fsutil file createnew C:\\stuff\\bigfile_1.txt 7516192768 +RUN fsutil file createnew C:\\stuff\\bigfile_2.txt 7516192768 +RUN fsutil file createnew C:\\stuff\\bigfile_3.txt 7516192768 +# Copy that 21GB of data out into a new target +FROM busybox +COPY --from=intermediate C:\\stuff C:\\stuff +` + + buf := bytes.NewBuffer(nil) + w := tar.NewWriter(buf) + writeTarRecord(t, w, "Dockerfile", dockerfile) + err := w.Close() + assert.NilError(t, err) + + apiclient := testEnv.APIClient() + resp, err := apiclient.ImageBuild(ctx, + buf, + types.ImageBuildOptions{ + Remove: true, + ForceRemove: true, + }) + + out := bytes.NewBuffer(nil) + assert.NilError(t, err) + _, err = io.Copy(out, resp.Body) + resp.Body.Close() + assert.NilError(t, err) + assert.Check(t, is.Contains(out.String(), "Successfully built")) +} + func TestBuildWithEmptyDockerfile(t *testing.T) { skip.If(t, versions.LessThan(testEnv.DaemonAPIVersion(), "1.40"), "broken in earlier versions") ctx := context.TODO() From 56d378a88f27b42336d990764b3da9116584f296 Mon Sep 17 00:00:00 2001 From: "Paul \"TBBle\" Hampson" Date: Fri, 6 Nov 2020 19:51:44 +1100 Subject: [PATCH 2/5] Apply a 127GB default WCOW Sandbox size globally This applies the 127GB default WCOW Sandbox size to not just `RUN` under `docker build` (as was previously the case) but to `COPY` and `ADD` under `docker build` and also to `docker run`. It also removes an inconsistency that the 127GB size was not applied when `--platform windows` was not passed to `docker build`, but WCOW was still used as a platform default, e.g. Docker Desktop for Windows in Windows Containers mode. Signed-off-by: Paul "TBBle" Hampson --- builder/dockerfile/internals.go | 16 ++--------- daemon/graphdriver/windows/windows.go | 39 ++++++++++++++++++++------- 2 files changed, 32 insertions(+), 23 deletions(-) diff --git a/builder/dockerfile/internals.go b/builder/dockerfile/internals.go index 3376b0bcdd..77b51d35d9 100644 --- a/builder/dockerfile/internals.go +++ b/builder/dockerfile/internals.go @@ -11,7 +11,6 @@ import ( "os" "path" "path/filepath" - "runtime" "strings" "github.com/docker/docker/api/types" @@ -448,8 +447,7 @@ func (b *Builder) probeAndCreate(dispatchState *dispatchState, runConfig *contai func (b *Builder) create(runConfig *container.Config) (string, error) { logrus.Debugf("[BUILDER] Command to be executed: %v", runConfig.Cmd) - isWCOW := runtime.GOOS == "windows" && b.platform != nil && b.platform.OS == "windows" - hostConfig := hostConfigFromOptions(b.options, isWCOW) + hostConfig := hostConfigFromOptions(b.options) container, err := b.containerManager.Create(runConfig, hostConfig) if err != nil { return "", err @@ -462,7 +460,7 @@ func (b *Builder) create(runConfig *container.Config) (string, error) { return container.ID, nil } -func hostConfigFromOptions(options *types.ImageBuildOptions, isWCOW bool) *container.HostConfig { +func hostConfigFromOptions(options *types.ImageBuildOptions) *container.HostConfig { resources := container.Resources{ CgroupParent: options.CgroupParent, CPUShares: options.CPUShares, @@ -485,16 +483,6 @@ func hostConfigFromOptions(options *types.ImageBuildOptions, isWCOW bool) *conta LogConfig: defaultLogConfig, ExtraHosts: options.ExtraHosts, } - - // For WCOW, the default of 20GB hard-coded in the platform - // is too small for builder scenarios where many users are - // using RUN statements to install large amounts of data. - // Use 127GB as that's the default size of a VHD in Hyper-V. - if isWCOW { - hc.StorageOpt = make(map[string]string) - hc.StorageOpt["size"] = "127GB" - } - return hc } diff --git a/daemon/graphdriver/windows/windows.go b/daemon/graphdriver/windows/windows.go index a5f614dfae..5213dda6f0 100644 --- a/daemon/graphdriver/windows/windows.go +++ b/daemon/graphdriver/windows/windows.go @@ -38,8 +38,15 @@ import ( "golang.org/x/sys/windows" ) -// filterDriver is an HCSShim driver type for the Windows Filter driver. -const filterDriver = 1 +const ( + // filterDriver is an HCSShim driver type for the Windows Filter driver. + filterDriver = 1 + // For WCOW, the default of 20GB hard-coded in the platform + // is too small for builder scenarios where many users are + // using RUN or COPY statements to install large amounts of data. + // Use 127GB as that's the default size of a VHD in Hyper-V. + defaultSandboxSize = "127GB" +) var ( // mutatedFiles is a list of files that are mutated by the import process @@ -73,6 +80,10 @@ func (c *checker) IsMounted(path string) bool { return false } +type storageOptions struct { + size uint64 +} + // Driver represents a windows graph driver. type Driver struct { // info stores the shim driver information @@ -80,8 +91,9 @@ type Driver struct { ctr *graphdriver.RefCounter // it is safe for windows to use a cache here because it does not support // restoring containers when the daemon dies. - cacheMu sync.Mutex - cache map[string]string + cacheMu sync.Mutex + cache map[string]string + defaultStorageOpts *storageOptions } // InitFilter returns a new Windows storage filter driver. @@ -100,6 +112,11 @@ func InitFilter(home string, options []string, uidMaps, gidMaps []idtools.IDMap) return nil, fmt.Errorf("windowsfilter failed to create '%s': %v", home, err) } + size, err := units.RAMInBytes(defaultSandboxSize) + if err != nil { + return nil, fmt.Errorf("windowsfilter failed to parse default size '%s': %v", defaultSandboxSize, err) + } + d := &Driver{ info: hcsshim.DriverInfo{ HomeDir: home, @@ -107,6 +124,9 @@ func InitFilter(home string, options []string, uidMaps, gidMaps []idtools.IDMap) }, cache: make(map[string]string), ctr: graphdriver.NewRefCounter(&checker{}), + defaultStorageOpts: &storageOptions{ + size: uint64(size), + }, } return d, nil } @@ -231,8 +251,13 @@ func (d *Driver) create(id, parent, mountLabel string, readOnly bool, storageOpt return fmt.Errorf("Failed to parse storage options - %s", err) } + sandboxSize := d.defaultStorageOpts.size if storageOptions.size != 0 { - if err := hcsshim.ExpandSandboxSize(d.info, id, storageOptions.size); err != nil { + sandboxSize = storageOptions.size + } + + if sandboxSize != 0 { + if err := hcsshim.ExpandSandboxSize(d.info, id, sandboxSize); err != nil { return err } } @@ -935,10 +960,6 @@ func (d *Driver) DiffGetter(id string) (graphdriver.FileGetCloser, error) { return &fileGetCloserWithBackupPrivileges{d.dir(id)}, nil } -type storageOptions struct { - size uint64 -} - func parseStorageOpt(storageOpt map[string]string) (*storageOptions, error) { options := storageOptions{} From db7b7f6df9151f0eca88af087514825b1a493f32 Mon Sep 17 00:00:00 2001 From: "Paul \"TBBle\" Hampson" Date: Fri, 6 Nov 2020 21:53:51 +1100 Subject: [PATCH 3/5] Parse storage-opt in GraphDriver init on Windows This ensures the storage-opts applies to all operations by the graph drivers, replacing the merging of storage-opts into container storage config at container-creation time, and hence applying storage-opts to non-container operations like `COPY` and `ADD` in the builder. Signed-off-by: Paul "TBBle" Hampson --- daemon/create.go | 17 ----------------- daemon/graphdriver/lcow/lcow.go | 16 ++++++++++++++-- daemon/graphdriver/windows/windows.go | 20 +++++++++++++------- 3 files changed, 27 insertions(+), 26 deletions(-) diff --git a/daemon/create.go b/daemon/create.go index d4aeb77c7b..a190ab4e3d 100644 --- a/daemon/create.go +++ b/daemon/create.go @@ -187,23 +187,6 @@ func (daemon *Daemon) create(opts createOpts) (retC *container.Container, retErr ctr.HostConfig.StorageOpt = opts.params.HostConfig.StorageOpt - // Fixes: https://github.com/moby/moby/issues/34074 and - // https://github.com/docker/for-win/issues/999. - // Merge the daemon's storage options if they aren't already present. We only - // do this on Windows as there's no effective sandbox size limit other than - // physical on Linux. - if isWindows { - if ctr.HostConfig.StorageOpt == nil { - ctr.HostConfig.StorageOpt = make(map[string]string) - } - for _, v := range daemon.configStore.GraphOptions { - opt := strings.SplitN(v, "=", 2) - if _, ok := ctr.HostConfig.StorageOpt[opt[0]]; !ok { - ctr.HostConfig.StorageOpt[opt[0]] = opt[1] - } - } - } - // Set RWLayer for container after mount labels have been set rwLayer, err := daemon.imageService.CreateLayer(ctr, setupInitLayer(daemon.idMapping)) if err != nil { diff --git a/daemon/graphdriver/lcow/lcow.go b/daemon/graphdriver/lcow/lcow.go index 1bf48fed2a..f7af11ed26 100644 --- a/daemon/graphdriver/lcow/lcow.go +++ b/daemon/graphdriver/lcow/lcow.go @@ -124,6 +124,7 @@ type Driver struct { cachedScratchMutex sync.Mutex // Protects race conditions from multiple threads creating the cached scratch. options []string // Graphdriver options we are initialised with. globalMode bool // Indicates if running in an unsafe/global service VM mode. + defaultSandboxSize uint64 // The default sandbox size to use if one is not specified // NOTE: It is OK to use a cache here because Windows does not support // restoring containers when the daemon dies. @@ -163,7 +164,8 @@ func InitDriver(dataRoot string, options []string, _, _ []idtools.IDMap) (graphd serviceVms: &serviceVMMap{ svms: make(map[string]*serviceVMMapItem), }, - globalMode: false, + globalMode: false, + defaultSandboxSize: client.DefaultVhdxSizeGB, } // Looks for relevant options @@ -178,6 +180,16 @@ func InitDriver(dataRoot string, options []string, _, _ []idtools.IDMap) (graphd return nil, fmt.Errorf("%s failed to parse value for 'lcow.globalmode' - must be 'true' or 'false'", title) } break + case "lcow.sandboxsize": + var err error + d.defaultSandboxSize, err = strconv.ParseUint(opt[1], 10, 32) + if err != nil { + return nil, fmt.Errorf("%s failed to parse value '%s' for 'lcow.sandboxsize'", title, v) + } + if d.defaultSandboxSize < client.DefaultVhdxSizeGB { + return nil, fmt.Errorf("%s 'lcow.sandboxsize' option cannot be less than %d", title, client.DefaultVhdxSizeGB) + } + break } } } @@ -517,7 +529,7 @@ func (d *Driver) CreateReadWrite(id, parent string, opts *graphdriver.CreateOpts } // Look for an explicit sandbox size option. - sandboxSize := uint64(client.DefaultVhdxSizeGB) + sandboxSize := d.defaultSandboxSize for k, v := range opts.StorageOpt { switch strings.ToLower(k) { case "lcow.sandboxsize": diff --git a/daemon/graphdriver/windows/windows.go b/daemon/graphdriver/windows/windows.go index 5213dda6f0..04a027f69b 100644 --- a/daemon/graphdriver/windows/windows.go +++ b/daemon/graphdriver/windows/windows.go @@ -112,9 +112,17 @@ func InitFilter(home string, options []string, uidMaps, gidMaps []idtools.IDMap) return nil, fmt.Errorf("windowsfilter failed to create '%s': %v", home, err) } - size, err := units.RAMInBytes(defaultSandboxSize) + storageOpt := make(map[string]string) + storageOpt["size"] = defaultSandboxSize + + for _, v := range options { + opt := strings.SplitN(v, "=", 2) + storageOpt[strings.ToLower(opt[0])] = opt[1] + } + + storageOptions, err := parseStorageOpt(storageOpt) if err != nil { - return nil, fmt.Errorf("windowsfilter failed to parse default size '%s': %v", defaultSandboxSize, err) + return nil, fmt.Errorf("windowsfilter failed to parse default storage options - %s", err) } d := &Driver{ @@ -122,11 +130,9 @@ func InitFilter(home string, options []string, uidMaps, gidMaps []idtools.IDMap) HomeDir: home, Flavour: filterDriver, }, - cache: make(map[string]string), - ctr: graphdriver.NewRefCounter(&checker{}), - defaultStorageOpts: &storageOptions{ - size: uint64(size), - }, + cache: make(map[string]string), + ctr: graphdriver.NewRefCounter(&checker{}), + defaultStorageOpts: storageOptions, } return d, nil } From 695b151a18299ade7c70dfe3babc8d77b7f5aba6 Mon Sep 17 00:00:00 2001 From: "Paul \"TBBle\" Hampson" Date: Mon, 9 Nov 2020 02:48:04 +1100 Subject: [PATCH 4/5] Work around small disk on Windows-RS5 CI nodes The free disk space on the Windows RS5 CI nodes appears to be just the right size that the TestBuildWCOWSandboxSize test can generate 21GB of layers, and then a 21GB sandbox inside a container, and then runs out of space while committing the layer. Helpfully, this failure is distinguishable in the logs from a failure when the sandbox is too small, so we can do that. TODO: Revert this if-and-when the Windows-RS5 CI nodes have more free space. Signed-off-by: Paul "TBBle" Hampson --- integration/build/build_test.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/integration/build/build_test.go b/integration/build/build_test.go index be4ce47fe6..14b4bfa58e 100644 --- a/integration/build/build_test.go +++ b/integration/build/build_test.go @@ -560,7 +560,16 @@ COPY --from=intermediate C:\\stuff C:\\stuff _, err = io.Copy(out, resp.Body) resp.Body.Close() assert.NilError(t, err) - assert.Check(t, is.Contains(out.String(), "Successfully built")) + // The test passes if either: + // - the image build succeeded; or + // - The "COPY --from=intermediate" step ran out of space during re-exec'd writing of the transport layer information to hcsshim's temp directory + // The latter case means we finished the COPY operation, so the sandbox must have been larger than 20GB, which was the test, + // and _then_ ran out of space on the host during `importLayer` in the WindowsFilter graph driver, while committing the layer. + // See https://github.com/moby/moby/pull/41636#issuecomment-723038517 for more details on the operations being done here. + // Specifically, this happens on the Docker Jenkins CI Windows-RS5 build nodes. + // The two parts of the acceptable-failure case are on different lines, so we need two regexp checks. + assert.Check(t, is.Regexp("Successfully built|COPY --from=intermediate", out.String())) + assert.Check(t, is.Regexp("Successfully built|re-exec error: exit status 1: output: write.*daemon\\\\\\\\tmp\\\\\\\\hcs.*bigfile_[1-3].txt: There is not enough space on the disk.", out.String())) } func TestBuildWithEmptyDockerfile(t *testing.T) { From 1571e9331b4c56cc350bdf339b9aacd72eb3bbbd Mon Sep 17 00:00:00 2001 From: "Paul \"TBBle\" Hampson" Date: Tue, 10 Nov 2020 20:00:48 +1100 Subject: [PATCH 5/5] Use specific APIs for shared mount-point behaviour Thanks to @cpuguy83 for pointing these APIs out in #41638. Signed-off-by: Paul "TBBle" Hampson --- integration/container/mounts_linux_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/integration/container/mounts_linux_test.go b/integration/container/mounts_linux_test.go index d65bc99f0d..3c7aabda24 100644 --- a/integration/container/mounts_linux_test.go +++ b/integration/container/mounts_linux_test.go @@ -285,7 +285,7 @@ func TestContainerVolumesMountedAsShared(t *testing.T) { // Convert this directory into a shared mount point so that we do // not rely on propagation properties of parent mount. - if err := mount.Mount(tmpDir1.Path(), tmpDir1.Path(), "none", "bind,private"); err != nil { + if err := mount.MakePrivate(tmpDir1.Path()); err != nil { t.Fatal(err) } defer func() { @@ -293,7 +293,7 @@ func TestContainerVolumesMountedAsShared(t *testing.T) { t.Fatal(err) } }() - if err := mount.Mount("none", tmpDir1.Path(), "none", "shared"); err != nil { + if err := mount.MakeShared(tmpDir1.Path()); err != nil { t.Fatal(err) } @@ -342,7 +342,7 @@ func TestContainerVolumesMountedAsSlave(t *testing.T) { // Convert this directory into a shared mount point so that we do // not rely on propagation properties of parent mount. - if err := mount.Mount(tmpDir1.Path(), tmpDir1.Path(), "none", "bind,private"); err != nil { + if err := mount.MakePrivate(tmpDir1.Path()); err != nil { t.Fatal(err) } defer func() { @@ -350,7 +350,7 @@ func TestContainerVolumesMountedAsSlave(t *testing.T) { t.Fatal(err) } }() - if err := mount.Mount("none", tmpDir1.Path(), "none", "shared"); err != nil { + if err := mount.MakeShared(tmpDir1.Path()); err != nil { t.Fatal(err) }