From 19e1ffb349a60dd62434ec1d1ca6b735ba4dfcc0 Mon Sep 17 00:00:00 2001 From: Jim Ehrismann Date: Thu, 16 May 2019 14:46:41 -0400 Subject: [PATCH 01/35] explicitly set filesystem type for mount to avoid 'invalid argument' error on arm Signed-off-by: Jim Ehrismann (cherry picked from commit d7de1a8b9fffd90f9f6a81a28d28637dd2e3cda6) Signed-off-by: Sebastiaan van Stijn Upstream-commit: 51086aad82d7ed9bfaa504b52db7da8ed7d09032 Component: engine --- components/engine/integration-cli/docker_cli_daemon_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/engine/integration-cli/docker_cli_daemon_test.go b/components/engine/integration-cli/docker_cli_daemon_test.go index d1a709b736..96cd2dc78a 100644 --- a/components/engine/integration-cli/docker_cli_daemon_test.go +++ b/components/engine/integration-cli/docker_cli_daemon_test.go @@ -1777,7 +1777,7 @@ func (s *DockerDaemonSuite) TestDaemonNoSpaceLeftOnDeviceError(c *check.C) { dockerCmd(c, "run", "--rm", "-v", testDir+":/test", "busybox", "sh", "-c", "dd of=/test/testfs.img bs=1M seek=3 count=0") icmd.RunCommand("mkfs.ext4", "-F", filepath.Join(testDir, "testfs.img")).Assert(c, icmd.Success) - dockerCmd(c, "run", "--privileged", "--rm", "-v", testDir+":/test:shared", "busybox", "sh", "-c", "mkdir -p /test/test-mount/vfs && mount -n /test/testfs.img /test/test-mount/vfs") + dockerCmd(c, "run", "--privileged", "--rm", "-v", testDir+":/test:shared", "busybox", "sh", "-c", "mkdir -p /test/test-mount/vfs && mount -n -t ext4 /test/testfs.img /test/test-mount/vfs") defer mount.Unmount(filepath.Join(testDir, "test-mount")) s.d.Start(c, "--storage-driver", "vfs", "--data-root", filepath.Join(testDir, "test-mount")) From 8852e9cdc4543c22c3dfcc284e7ffd25b96ecaad Mon Sep 17 00:00:00 2001 From: frankyang Date: Tue, 14 May 2019 15:21:55 +0800 Subject: [PATCH 02/35] bugfix: fetch the right device number which great than 255 Signed-off-by: frankyang (cherry picked from commit b9f31912deb511e732763e4fa5ecd0208b104eb2) Signed-off-by: Sebastiaan van Stijn Upstream-commit: e4cf15b4f51b3b6ef380dbfe5061568bc84eece1 Component: engine --- components/engine/daemon/daemon_unix.go | 8 +-- components/engine/daemon/daemon_unix_test.go | 63 ++++++++++++++++++++ 2 files changed, 67 insertions(+), 4 deletions(-) diff --git a/components/engine/daemon/daemon_unix.go b/components/engine/daemon/daemon_unix.go index 5234201c82..8b59b5278a 100644 --- a/components/engine/daemon/daemon_unix.go +++ b/components/engine/daemon/daemon_unix.go @@ -174,8 +174,8 @@ func getBlkioWeightDevices(config containertypes.Resources) ([]specs.LinuxWeight } weight := weightDevice.Weight d := specs.LinuxWeightDevice{Weight: &weight} - d.Major = int64(stat.Rdev / 256) - d.Minor = int64(stat.Rdev % 256) + d.Major = int64(unix.Major(stat.Rdev)) + d.Minor = int64(unix.Minor(stat.Rdev)) blkioWeightDevices = append(blkioWeightDevices, d) } @@ -245,8 +245,8 @@ func getBlkioThrottleDevices(devs []*blkiodev.ThrottleDevice) ([]specs.LinuxThro return nil, err } d := specs.LinuxThrottleDevice{Rate: d.Rate} - d.Major = int64(stat.Rdev / 256) - d.Minor = int64(stat.Rdev % 256) + d.Major = int64(unix.Major(stat.Rdev)) + d.Minor = int64(unix.Minor(stat.Rdev)) throttleDevices = append(throttleDevices, d) } diff --git a/components/engine/daemon/daemon_unix_test.go b/components/engine/daemon/daemon_unix_test.go index 36c6030988..9ea2560d97 100644 --- a/components/engine/daemon/daemon_unix_test.go +++ b/components/engine/daemon/daemon_unix_test.go @@ -6,11 +6,16 @@ import ( "errors" "io/ioutil" "os" + "path/filepath" "testing" + "github.com/docker/docker/api/types/blkiodev" containertypes "github.com/docker/docker/api/types/container" "github.com/docker/docker/container" "github.com/docker/docker/daemon/config" + "golang.org/x/sys/unix" + "gotest.tools/assert" + is "gotest.tools/assert/cmp" ) type fakeContainerGetter struct { @@ -266,3 +271,61 @@ func TestNetworkOptions(t *testing.T) { t.Fatal("Expected networkOptions error, got nil") } } + +const ( + // prepare major 0x1FD(509 in decimal) and minor 0x130(304) + DEVNO = 0x11FD30 + MAJOR = 509 + MINOR = 304 + WEIGHT = 1024 +) + +func deviceTypeMock(t *testing.T, testAndCheck func(string)) { + if os.Getuid() != 0 { + t.Skip("root required") // for mknod + } + + t.Parallel() + + tempDir, err := ioutil.TempDir("", "tempDevDir"+t.Name()) + assert.NilError(t, err, "create temp file") + tempFile := filepath.Join(tempDir, "dev") + + defer os.RemoveAll(tempDir) + + if err = unix.Mknod(tempFile, unix.S_IFCHR, DEVNO); err != nil { + t.Fatalf("mknod error %s(%x): %v", tempFile, DEVNO, err) + } + + testAndCheck(tempFile) +} + +func TestGetBlkioWeightDevices(t *testing.T) { + deviceTypeMock(t, func(tempFile string) { + mockResource := containertypes.Resources{ + BlkioWeightDevice: []*blkiodev.WeightDevice{{Path: tempFile, Weight: WEIGHT}}, + } + + weightDevs, err := getBlkioWeightDevices(mockResource) + + assert.NilError(t, err, "getBlkioWeightDevices") + assert.Check(t, is.Len(weightDevs, 1), "getBlkioWeightDevices") + assert.Check(t, weightDevs[0].Major == MAJOR, "get major device type") + assert.Check(t, weightDevs[0].Minor == MINOR, "get minor device type") + assert.Check(t, *weightDevs[0].Weight == WEIGHT, "get device weight") + }) +} + +func TestGetBlkioThrottleDevices(t *testing.T) { + deviceTypeMock(t, func(tempFile string) { + mockDevs := []*blkiodev.ThrottleDevice{{Path: tempFile, Rate: WEIGHT}} + + retDevs, err := getBlkioThrottleDevices(mockDevs) + + assert.NilError(t, err, "getBlkioThrottleDevices") + assert.Check(t, is.Len(retDevs, 1), "getBlkioThrottleDevices") + assert.Check(t, retDevs[0].Major == MAJOR, "get major device type") + assert.Check(t, retDevs[0].Minor == MINOR, "get minor device type") + assert.Check(t, retDevs[0].Rate == WEIGHT, "get device rate") + }) +} From 6752cc9f2f8350ce4759353faee13556e948d62a Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 21 May 2019 01:33:15 -0700 Subject: [PATCH 03/35] int-cli/TestSearchCmdOptions: fail earlier Sometimes this test fails (allegedly due to problems with Docker Hub), but it fails later than it should, for example: > 01:20:34.845 assertion failed: expression is false: strings.Count(outSearchCmdStars, "[OK]") <= strings.Count(outSearchCmd, "[OK]"): The quantity of images with stars should be less than that of all images: <...> This, with non-empty list of images following, means that the initial `docker search busybox` command returned not enough results. So, add a check that `docker search busybox` returns something. While at it, * raise the number of stars to 10; * simplify check for number of lines (no need to count [OK]'s); * improve error message. Signed-off-by: Kir Kolyshkin (cherry picked from commit 4f80a1953d66265d9a1cbf4e172b8fa58e3e403a) Signed-off-by: Sebastiaan van Stijn Upstream-commit: 8a369c735b2bf07da53275fe9fe61d429320f233 Component: engine --- components/engine/integration-cli/docker_cli_search_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/components/engine/integration-cli/docker_cli_search_test.go b/components/engine/integration-cli/docker_cli_search_test.go index 2f811d4b96..13870a8fce 100644 --- a/components/engine/integration-cli/docker_cli_search_test.go +++ b/components/engine/integration-cli/docker_cli_search_test.go @@ -51,8 +51,8 @@ func (s *DockerSuite) TestSearchCmdOptions(c *check.C) { assert.Assert(c, strings.Contains(out, "Usage:\tdocker search [OPTIONS] TERM")) outSearchCmd, _ := dockerCmd(c, "search", "busybox") + assert.Assert(c, strings.Count(outSearchCmd, "\n") > 3, outSearchCmd) outSearchCmdNotrunc, _ := dockerCmd(c, "search", "--no-trunc=true", "busybox") - assert.Assert(c, len(outSearchCmd) <= len(outSearchCmdNotrunc), "The no-trunc option can't take effect.") outSearchCmdautomated, _ := dockerCmd(c, "search", "--filter", "is-automated=true", "busybox") //The busybox is a busybox base image, not an AUTOMATED image. @@ -72,8 +72,8 @@ func (s *DockerSuite) TestSearchCmdOptions(c *check.C) { assert.Equal(c, len(outSearchCmdOfficialSlice), 3) // 1 header, 1 line, 1 carriage return assert.Assert(c, strings.HasPrefix(outSearchCmdOfficialSlice[1], "busybox "), "The busybox is an OFFICIAL image: %s", outSearchCmdOfficial) - outSearchCmdStars, _ := dockerCmd(c, "search", "--filter", "stars=2", "busybox") - assert.Assert(c, strings.Count(outSearchCmdStars, "[OK]") <= strings.Count(outSearchCmd, "[OK]"), "The quantity of images with stars should be less than that of all images: %s", outSearchCmdStars) + outSearchCmdStars, _ := dockerCmd(c, "search", "--filter", "stars=10", "busybox") + assert.Assert(c, strings.Count(outSearchCmdStars, "\n") <= strings.Count(outSearchCmd, "\n"), "Number of images with 10+ stars should be less than that of all images:\noutSearchCmdStars: %s\noutSearch: %s\n", outSearchCmdStars, outSearchCmd) dockerCmd(c, "search", "--filter", "is-automated=true", "--filter", "stars=2", "--no-trunc=true", "busybox") From 76f4d953bbacbdda3b1c4fc21f170282f882f464 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 22 May 2019 13:18:10 +0200 Subject: [PATCH 04/35] Update TestRunWithDaemonDefaultSeccompProfile for ARM64 `chmod` is a legacy syscall, and not present on arm64, which caused this test to fail. Add `fchmodat` to the profile so that this test can run both on x64 and arm64. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 4bd8964b23f616a1aef63ea0acebbdc4b6ea90e6) Signed-off-by: Sebastiaan van Stijn Upstream-commit: 1b642b8c45a734d4c2d9af2f6849289db556d003 Component: engine --- components/engine/integration-cli/docker_cli_run_unix_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/components/engine/integration-cli/docker_cli_run_unix_test.go b/components/engine/integration-cli/docker_cli_run_unix_test.go index cd3f5e356b..46dff49161 100644 --- a/components/engine/integration-cli/docker_cli_run_unix_test.go +++ b/components/engine/integration-cli/docker_cli_run_unix_test.go @@ -1544,6 +1544,10 @@ func (s *DockerDaemonSuite) TestRunWithDaemonDefaultSeccompProfile(c *check.C) { { "name": "chmod", "action": "SCMP_ACT_ERRNO" + }, + { + "name": "fchmodat", + "action": "SCMP_ACT_ERRNO" } ] }` From 9a388b1aa0e7da978fc4028a4eebd69473f11817 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 22 May 2019 14:47:40 +0200 Subject: [PATCH 05/35] Remove TestSearchCmdOptions test This test is dependent on the search results returned by Docker Hub, which can change at any moment, and causes this test to be unpredictable. Removing this test instead of trying to catch up with Docker Hub any time the results change, because it's effectively testing Docker Hub, and not the daemon. Unit tests are already in place to test the core functionality of the daemon, so it should be safe to remove this test. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 21e662c77468d1e3fc4efaa049021e2ad0e49b4d) Signed-off-by: Sebastiaan van Stijn Upstream-commit: 0d052bc40e908f1f596b0f16840b4e93fc6a9ec2 Component: engine --- .../integration-cli/docker_cli_search_test.go | 48 ------------------- 1 file changed, 48 deletions(-) diff --git a/components/engine/integration-cli/docker_cli_search_test.go b/components/engine/integration-cli/docker_cli_search_test.go index 13870a8fce..95ad9ce1b2 100644 --- a/components/engine/integration-cli/docker_cli_search_test.go +++ b/components/engine/integration-cli/docker_cli_search_test.go @@ -44,54 +44,6 @@ func (s *DockerSuite) TestSearchStarsOptionWithWrongParameter(c *check.C) { assert.Assert(c, strings.Contains(out, "invalid syntax"), "couldn't find the invalid value warning") } -func (s *DockerSuite) TestSearchCmdOptions(c *check.C) { - testRequires(c, Network, DaemonIsLinux) - - out, _ := dockerCmd(c, "search", "--help") - assert.Assert(c, strings.Contains(out, "Usage:\tdocker search [OPTIONS] TERM")) - - outSearchCmd, _ := dockerCmd(c, "search", "busybox") - assert.Assert(c, strings.Count(outSearchCmd, "\n") > 3, outSearchCmd) - outSearchCmdNotrunc, _ := dockerCmd(c, "search", "--no-trunc=true", "busybox") - assert.Assert(c, len(outSearchCmd) <= len(outSearchCmdNotrunc), "The no-trunc option can't take effect.") - - outSearchCmdautomated, _ := dockerCmd(c, "search", "--filter", "is-automated=true", "busybox") //The busybox is a busybox base image, not an AUTOMATED image. - outSearchCmdautomatedSlice := strings.Split(outSearchCmdautomated, "\n") - for i := range outSearchCmdautomatedSlice { - assert.Assert(c, !strings.HasPrefix(outSearchCmdautomatedSlice[i], "busybox "), "The busybox is not an AUTOMATED image: %s", outSearchCmdautomated) - } - - outSearchCmdNotOfficial, _ := dockerCmd(c, "search", "--filter", "is-official=false", "busybox") //The busybox is a busybox base image, official image. - outSearchCmdNotOfficialSlice := strings.Split(outSearchCmdNotOfficial, "\n") - for i := range outSearchCmdNotOfficialSlice { - assert.Assert(c, !strings.HasPrefix(outSearchCmdNotOfficialSlice[i], "busybox "), "The busybox is not an OFFICIAL image: %s", outSearchCmdNotOfficial) - } - - outSearchCmdOfficial, _ := dockerCmd(c, "search", "--filter", "is-official=true", "busybox") //The busybox is a busybox base image, official image. - outSearchCmdOfficialSlice := strings.Split(outSearchCmdOfficial, "\n") - assert.Equal(c, len(outSearchCmdOfficialSlice), 3) // 1 header, 1 line, 1 carriage return - assert.Assert(c, strings.HasPrefix(outSearchCmdOfficialSlice[1], "busybox "), "The busybox is an OFFICIAL image: %s", outSearchCmdOfficial) - - outSearchCmdStars, _ := dockerCmd(c, "search", "--filter", "stars=10", "busybox") - assert.Assert(c, strings.Count(outSearchCmdStars, "\n") <= strings.Count(outSearchCmd, "\n"), "Number of images with 10+ stars should be less than that of all images:\noutSearchCmdStars: %s\noutSearch: %s\n", outSearchCmdStars, outSearchCmd) - - dockerCmd(c, "search", "--filter", "is-automated=true", "--filter", "stars=2", "--no-trunc=true", "busybox") - - // --automated deprecated since Docker 1.13 - outSearchCmdautomated1, _ := dockerCmd(c, "search", "--automated=true", "busybox") //The busybox is a busybox base image, not an AUTOMATED image. - outSearchCmdautomatedSlice1 := strings.Split(outSearchCmdautomated1, "\n") - for i := range outSearchCmdautomatedSlice1 { - assert.Assert(c, !strings.HasPrefix(outSearchCmdautomatedSlice1[i], "busybox "), "The busybox is not an AUTOMATED image: %s", outSearchCmdautomated) - } - - // -s --stars deprecated since Docker 1.13 - outSearchCmdStars1, _ := dockerCmd(c, "search", "--stars=2", "busybox") - assert.Assert(c, strings.Count(outSearchCmdStars1, "[OK]") <= strings.Count(outSearchCmd, "[OK]"), "The quantity of images with stars should be less than that of all images: %s", outSearchCmdStars1) - - // -s --stars deprecated since Docker 1.13 - dockerCmd(c, "search", "--stars=2", "--automated=true", "--no-trunc=true", "busybox") -} - // search for repos which start with "ubuntu-" on the central registry func (s *DockerSuite) TestSearchOnCentralRegistryWithDash(c *check.C) { testRequires(c, Network, DaemonIsLinux) From 0c1bb16a07deb25c5b676a0588fb43f3bde61098 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 29 Dec 2018 20:46:33 +0100 Subject: [PATCH 06/35] Skip TestInfoAPIWarnings on remote daemons This test starts a new daemon, which will fail when testing against a remote daemon; --- FAIL: TestInfoAPIWarnings (0.00s) info_test.go:53: failed to start daemon with arguments [-H=0.0.0.0:23756 -H=unix:///tmp/docker-integration/d5153ebcf89ef.sock] : [d5153ebcf89ef] could not find docker binary in $PATH: exec: "dockerd": executable file not found in $PATH Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 056840c2a644cc37f91392103f5c3ae05d14a695) Signed-off-by: Sebastiaan van Stijn Upstream-commit: 5e024a2fcd10585611ab1ea792d0256c6477c87c Component: engine --- components/engine/integration/system/info_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/components/engine/integration/system/info_test.go b/components/engine/integration/system/info_test.go index 80058523a3..8130361988 100644 --- a/components/engine/integration/system/info_test.go +++ b/components/engine/integration/system/info_test.go @@ -44,6 +44,7 @@ func TestInfoAPI(t *testing.T) { } func TestInfoAPIWarnings(t *testing.T) { + skip.If(t, testEnv.IsRemoteDaemon, "cannot run daemon when remote daemon") skip.If(t, testEnv.DaemonInfo.OSType == "windows", "FIXME") d := daemon.New(t) c := d.NewClientT(t) From 7f53e081902f0142bff2527919134436c8e17337 Mon Sep 17 00:00:00 2001 From: "Iskander (Alex) Sharipov" Date: Fri, 8 Mar 2019 10:39:58 +0300 Subject: [PATCH 07/35] image: do actual RootFS.DiffIDs copying in Clone() append(newRoot.DiffIDs) without element does nothing, so it's probably not what was intended. Changed code to perform a slice copying instead. Fixes #38834. Signed-off-by: Iskander Sharipov (cherry picked from commit 3429e9993085520ac902fef2ef6aabd57080bd36) Signed-off-by: Sebastiaan van Stijn Upstream-commit: 1c755a73bfd63f63d6fe3363d8237f9db634633d Component: engine --- components/engine/image/rootfs.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/components/engine/image/rootfs.go b/components/engine/image/rootfs.go index 84843e10c6..f73a0660fa 100644 --- a/components/engine/image/rootfs.go +++ b/components/engine/image/rootfs.go @@ -38,7 +38,8 @@ func (r *RootFS) Append(id layer.DiffID) { func (r *RootFS) Clone() *RootFS { newRoot := NewRootFS() newRoot.Type = r.Type - newRoot.DiffIDs = append(r.DiffIDs) + newRoot.DiffIDs = make([]layer.DiffID, len(r.DiffIDs)) + copy(newRoot.DiffIDs, r.DiffIDs) return newRoot } From 8a9991333654df7d737660af00499df63e00fb9a Mon Sep 17 00:00:00 2001 From: corbin-coleman Date: Mon, 27 Aug 2018 18:34:19 +0000 Subject: [PATCH 08/35] Add ability to override the version in make.ps1 Checks for environment variable VERSION if it exists then it sets dockerVersion to VERSION Signed-off-by: corbin-coleman Signed-off-by: Eli Uriegas (cherry picked from commit edc639e99f21d0dd814581858d02787c23ba1599) Signed-off-by: Sebastiaan van Stijn Upstream-commit: 238951ac3a78a8517d7764de0f48b8ded28441e2 Component: engine --- components/engine/hack/make.ps1 | 2 ++ 1 file changed, 2 insertions(+) diff --git a/components/engine/hack/make.ps1 b/components/engine/hack/make.ps1 index 6221e364d6..9abe18cfa5 100644 --- a/components/engine/hack/make.ps1 +++ b/components/engine/hack/make.ps1 @@ -351,6 +351,8 @@ Try { # Get the version of docker (eg 17.04.0-dev) $dockerVersion="0.0.0-dev" + # Overwrite dockerVersion if VERSION Environment variable is available + if (Test-Path Env:\VERSION) { $dockerVersion=$env:VERSION } # Give a warning if we are not running in a container and are building binaries or running unit tests. # Not relevant for validation tests as these are fine to run outside of a container. From 38b3e62527f262c4a6d349ceca7e2675672beb57 Mon Sep 17 00:00:00 2001 From: Daniel Sweet Date: Thu, 9 May 2019 13:14:20 -0400 Subject: [PATCH 09/35] Ensure all integration daemon logging happens before test exit As of Go 1.12, the `testing` package panics if a goroutine logs to a `testing.T` after the relevant test has completed. This was not documented as a change at all; see the commit 95d06ab6c982f58b127b14a52c3325acf0bd3926 in the Go repository for the relevant change. At any point in the integration tests, tests could panic with the message "Log in goroutine after TEST_FUNCTION has completed". This was exacerbated by less direct logging I/O, e.g. running `make test` with its output piped instead of attached to a TTY. The most common cause of panics was that there was a race condition between an exit logging goroutine and the `StopWithError` method: `StopWithError` could return, causing the calling test method to return, causing the `testing.T` to be marked as finished, before the goroutine could log that the test daemon had exited. The fix is simple: capture the result of `cmd.Wait()`, _then_ log, _then_ send the captured result over the `Wait` channel. This ensures that the message is logged before `StopWithError` can return, blocking the test method so that the target `testing.T` is not marked as finished. Signed-off-by: Daniel Sweet (cherry picked from commit 7546322e994c4a23ea3cae0cf0a2a8019de12c03) Signed-off-by: Sebastiaan van Stijn Upstream-commit: b2168eec8b37619a1d28d0bb44cf052d4f9883c7 Component: engine --- components/engine/internal/test/daemon/daemon.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/components/engine/internal/test/daemon/daemon.go b/components/engine/internal/test/daemon/daemon.go index c0b5c483f9..61fbdb4e74 100644 --- a/components/engine/internal/test/daemon/daemon.go +++ b/components/engine/internal/test/daemon/daemon.go @@ -268,8 +268,11 @@ func (d *Daemon) StartWithLogFile(out *os.File, providedArgs ...string) error { wait := make(chan error) go func() { - wait <- d.cmd.Wait() + ret := d.cmd.Wait() d.log.Logf("[%s] exiting daemon", d.id) + // If we send before logging, we might accidentally log _after_ the test is done. + // As of Go 1.12, this incurs a panic instead of silently being dropped. + wait <- ret close(wait) }() From d6aaa8daf198a3f3fe3460f897bbbed792fb8b0e Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Fri, 3 May 2019 09:59:54 -0700 Subject: [PATCH 10/35] layer: protect mountedLayer.references Add a mutex to protect concurrent access to mountedLayer.references map. Signed-off-by: Kir Kolyshkin (cherry picked from commit f73b5cb4e8b9a23ad6700577840582d66017a733) Signed-off-by: Sebastiaan van Stijn Upstream-commit: bb80a60be2a9236b0db27222310f568e1d3fba11 Component: engine --- components/engine/layer/mounted_layer.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/components/engine/layer/mounted_layer.go b/components/engine/layer/mounted_layer.go index d6858c662c..c5d9e0e488 100644 --- a/components/engine/layer/mounted_layer.go +++ b/components/engine/layer/mounted_layer.go @@ -2,6 +2,7 @@ package layer // import "github.com/docker/docker/layer" import ( "io" + "sync" "github.com/docker/docker/pkg/archive" "github.com/docker/docker/pkg/containerfs" @@ -15,6 +16,7 @@ type mountedLayer struct { path string layerStore *layerStore + sync.Mutex references map[RWLayer]*referencedRWLayer } @@ -62,16 +64,24 @@ func (ml *mountedLayer) getReference() RWLayer { ref := &referencedRWLayer{ mountedLayer: ml, } + ml.Lock() ml.references[ref] = ref + ml.Unlock() return ref } func (ml *mountedLayer) hasReferences() bool { - return len(ml.references) > 0 + ml.Lock() + ret := len(ml.references) > 0 + ml.Unlock() + + return ret } func (ml *mountedLayer) deleteReference(ref RWLayer) error { + ml.Lock() + defer ml.Unlock() if _, ok := ml.references[ref]; !ok { return ErrLayerNotRetained } @@ -81,7 +91,9 @@ func (ml *mountedLayer) deleteReference(ref RWLayer) error { func (ml *mountedLayer) retakeReference(r RWLayer) { if ref, ok := r.(*referencedRWLayer); ok { + ml.Lock() ml.references[ref] = ref + ml.Unlock() } } From 89f76f3cc6036a3ad400690ebe56be9f6efd5c91 Mon Sep 17 00:00:00 2001 From: Xinfeng Liu Date: Tue, 23 Apr 2019 02:33:20 +0000 Subject: [PATCH 11/35] layer: optimize layerStore mountL Goroutine stack analisys shown some lock contention while doing massively (100 instances of `docker rm`) parallel image removal, with many goroutines waiting for the mountL mutex. Optimize it. With this commit, the above operation is about 3x faster, with no noticeable change to container creation times (tested on aufs and overlay2). kolyshkin@: - squashed commits - added description - protected CreateRWLayer against name collisions by temporary assiging nil to ls.mounts[name], and treating nil as "non-existent" in all the other functions. Signed-off-by: Kir Kolyshkin (cherry picked from commit 05250a4f0094e6802dd7d338d632ea632d0c7e34) Signed-off-by: Sebastiaan van Stijn Upstream-commit: 78cbf4d1388a4eef155f08da0d2422e3a2cad11f Component: engine --- components/engine/layer/layer_store.go | 50 ++++++++++++++++---------- components/engine/layer/migration.go | 6 ++-- 2 files changed, 35 insertions(+), 21 deletions(-) diff --git a/components/engine/layer/layer_store.go b/components/engine/layer/layer_store.go index 1601465c04..737283cc2d 100644 --- a/components/engine/layer/layer_store.go +++ b/components/engine/layer/layer_store.go @@ -189,7 +189,9 @@ func (ls *layerStore) loadLayer(layer ChainID) (*roLayer, error) { } func (ls *layerStore) loadMount(mount string) error { - if _, ok := ls.mounts[mount]; ok { + ls.mountL.Lock() + defer ls.mountL.Unlock() + if m := ls.mounts[mount]; m != nil { return nil } @@ -477,7 +479,7 @@ func (ls *layerStore) Release(l Layer) ([]Metadata, error) { return ls.releaseLayer(layer) } -func (ls *layerStore) CreateRWLayer(name string, parent ChainID, opts *CreateRWLayerOpts) (RWLayer, error) { +func (ls *layerStore) CreateRWLayer(name string, parent ChainID, opts *CreateRWLayerOpts) (_ RWLayer, err error) { var ( storageOpt map[string]string initFunc MountInit @@ -491,13 +493,21 @@ func (ls *layerStore) CreateRWLayer(name string, parent ChainID, opts *CreateRWL } ls.mountL.Lock() - defer ls.mountL.Unlock() - m, ok := ls.mounts[name] - if ok { + if _, ok := ls.mounts[name]; ok { + ls.mountL.Unlock() return nil, ErrMountNameConflict } + // Avoid name collision by temporary assigning nil + ls.mounts[name] = nil + ls.mountL.Unlock() + defer func() { + if err != nil { + ls.mountL.Lock() + delete(ls.mounts, name) + ls.mountL.Unlock() + } + }() - var err error var pid string var p *roLayer if string(parent) != "" { @@ -517,7 +527,7 @@ func (ls *layerStore) CreateRWLayer(name string, parent ChainID, opts *CreateRWL }() } - m = &mountedLayer{ + m := &mountedLayer{ name: name, parent: p, mountID: ls.mountID(name), @@ -528,7 +538,7 @@ func (ls *layerStore) CreateRWLayer(name string, parent ChainID, opts *CreateRWL if initFunc != nil { pid, err = ls.initMount(m.mountID, pid, mountLabel, initFunc, storageOpt) if err != nil { - return nil, err + return } m.initID = pid } @@ -538,10 +548,10 @@ func (ls *layerStore) CreateRWLayer(name string, parent ChainID, opts *CreateRWL } if err = ls.driver.CreateReadWrite(m.mountID, pid, createOpts); err != nil { - return nil, err + return } if err = ls.saveMount(m); err != nil { - return nil, err + return } return m.getReference(), nil @@ -549,9 +559,9 @@ func (ls *layerStore) CreateRWLayer(name string, parent ChainID, opts *CreateRWL func (ls *layerStore) GetRWLayer(id string) (RWLayer, error) { ls.mountL.Lock() - defer ls.mountL.Unlock() - mount, ok := ls.mounts[id] - if !ok { + mount := ls.mounts[id] + ls.mountL.Unlock() + if mount == nil { return nil, ErrMountDoesNotExist } @@ -561,8 +571,8 @@ func (ls *layerStore) GetRWLayer(id string) (RWLayer, error) { func (ls *layerStore) GetMountID(id string) (string, error) { ls.mountL.Lock() defer ls.mountL.Unlock() - mount, ok := ls.mounts[id] - if !ok { + mount := ls.mounts[id] + if mount == nil { return "", ErrMountDoesNotExist } logrus.Debugf("GetMountID id: %s -> mountID: %s", id, mount.mountID) @@ -572,9 +582,9 @@ func (ls *layerStore) GetMountID(id string) (string, error) { func (ls *layerStore) ReleaseRWLayer(l RWLayer) ([]Metadata, error) { ls.mountL.Lock() - defer ls.mountL.Unlock() - m, ok := ls.mounts[l.Name()] - if !ok { + m := ls.mounts[l.Name()] + ls.mountL.Unlock() + if m == nil { return []Metadata{}, nil } @@ -606,7 +616,9 @@ func (ls *layerStore) ReleaseRWLayer(l RWLayer) ([]Metadata, error) { return nil, err } + ls.mountL.Lock() delete(ls.mounts, m.Name()) + ls.mountL.Unlock() ls.layerL.Lock() defer ls.layerL.Unlock() @@ -634,7 +646,9 @@ func (ls *layerStore) saveMount(mount *mountedLayer) error { } } + ls.mountL.Lock() ls.mounts[mount.name] = mount + ls.mountL.Unlock() return nil } diff --git a/components/engine/layer/migration.go b/components/engine/layer/migration.go index 2668ea96bb..775e552fee 100644 --- a/components/engine/layer/migration.go +++ b/components/engine/layer/migration.go @@ -18,9 +18,9 @@ import ( // after migration the layer may be retrieved by the given name. func (ls *layerStore) CreateRWLayerByGraphID(name, graphID string, parent ChainID) (err error) { ls.mountL.Lock() - defer ls.mountL.Unlock() - m, ok := ls.mounts[name] - if ok { + m := ls.mounts[name] + ls.mountL.Unlock() + if m != nil { if m.parent.chainID != parent { return errors.New("name conflict, mismatched parent") } From 8b3a1bef0d23ea84e15406d4780c4b92f4a5bc86 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 16 May 2019 13:51:15 -0700 Subject: [PATCH 12/35] layer/CreateRWLayerByGraphID: remove This is an additon to commit 1fea38856a ("Remove v1.10 migrator") aka PR #38265. Since that one, CreateRWLayerByGraphID() is not used anywhere, so let's drop it. Signed-off-by: Kir Kolyshkin (cherry picked from commit b4e9b507655e7dbdfb44d4ee284dcd658b859b3f) Signed-off-by: Sebastiaan van Stijn Upstream-commit: 1576eaba3357b7aab5121b51b9ebc34e731082fe Component: engine --- components/engine/layer/migration.go | 59 -------- components/engine/layer/migration_test.go | 160 ---------------------- 2 files changed, 219 deletions(-) diff --git a/components/engine/layer/migration.go b/components/engine/layer/migration.go index 775e552fee..12500694f0 100644 --- a/components/engine/layer/migration.go +++ b/components/engine/layer/migration.go @@ -3,7 +3,6 @@ package layer // import "github.com/docker/docker/layer" import ( "compress/gzip" "errors" - "fmt" "io" "os" @@ -13,64 +12,6 @@ import ( "github.com/vbatts/tar-split/tar/storage" ) -// CreateRWLayerByGraphID creates a RWLayer in the layer store using -// the provided name with the given graphID. To get the RWLayer -// after migration the layer may be retrieved by the given name. -func (ls *layerStore) CreateRWLayerByGraphID(name, graphID string, parent ChainID) (err error) { - ls.mountL.Lock() - m := ls.mounts[name] - ls.mountL.Unlock() - if m != nil { - if m.parent.chainID != parent { - return errors.New("name conflict, mismatched parent") - } - if m.mountID != graphID { - return errors.New("mount already exists") - } - - return nil - } - - if !ls.driver.Exists(graphID) { - return fmt.Errorf("graph ID does not exist: %q", graphID) - } - - var p *roLayer - if string(parent) != "" { - p = ls.get(parent) - if p == nil { - return ErrLayerDoesNotExist - } - - // Release parent chain if error - defer func() { - if err != nil { - ls.layerL.Lock() - ls.releaseLayer(p) - ls.layerL.Unlock() - } - }() - } - - // TODO: Ensure graphID has correct parent - - m = &mountedLayer{ - name: name, - parent: p, - mountID: graphID, - layerStore: ls, - references: map[RWLayer]*referencedRWLayer{}, - } - - // Check for existing init layer - initID := fmt.Sprintf("%s-init", graphID) - if ls.driver.Exists(initID) { - m.initID = initID - } - - return ls.saveMount(m) -} - func (ls *layerStore) ChecksumForGraphID(id, parent, oldTarDataPath, newTarDataPath string) (diffID DiffID, size int64, err error) { defer func() { if err != nil { diff --git a/components/engine/layer/migration_test.go b/components/engine/layer/migration_test.go index 923166371c..2b5c3301f8 100644 --- a/components/engine/layer/migration_test.go +++ b/components/engine/layer/migration_test.go @@ -3,7 +3,6 @@ package layer // import "github.com/docker/docker/layer" import ( "bytes" "compress/gzip" - "fmt" "io" "io/ioutil" "os" @@ -12,7 +11,6 @@ import ( "testing" "github.com/docker/docker/daemon/graphdriver" - "github.com/docker/docker/pkg/archive" "github.com/docker/docker/pkg/stringid" "github.com/vbatts/tar-split/tar/asm" "github.com/vbatts/tar-split/tar/storage" @@ -269,161 +267,3 @@ func TestLayerMigrationNoTarsplit(t *testing.T) { assertMetadata(t, metadata, createMetadata(layer2a)) } - -func TestMountMigration(t *testing.T) { - // TODO Windows: Figure out why this is failing (obvious - paths... needs porting) - if runtime.GOOS == "windows" { - t.Skip("Failing on Windows") - } - ls, _, cleanup := newTestStore(t) - defer cleanup() - - baseFiles := []FileApplier{ - newTestFile("/root/.bashrc", []byte("# Boring configuration"), 0644), - newTestFile("/etc/profile", []byte("# Base configuration"), 0644), - } - initFiles := []FileApplier{ - newTestFile("/etc/hosts", []byte{}, 0644), - newTestFile("/etc/resolv.conf", []byte{}, 0644), - } - mountFiles := []FileApplier{ - newTestFile("/etc/hosts", []byte("localhost 127.0.0.1"), 0644), - newTestFile("/root/.bashrc", []byte("# Updated configuration"), 0644), - newTestFile("/root/testfile1.txt", []byte("nothing valuable"), 0644), - } - - initTar, err := tarFromFiles(initFiles...) - if err != nil { - t.Fatal(err) - } - - mountTar, err := tarFromFiles(mountFiles...) - if err != nil { - t.Fatal(err) - } - - graph := ls.(*layerStore).driver - - layer1, err := createLayer(ls, "", initWithFiles(baseFiles...)) - if err != nil { - t.Fatal(err) - } - - graphID1 := layer1.(*referencedCacheLayer).cacheID - - containerID := stringid.GenerateRandomID() - containerInit := fmt.Sprintf("%s-init", containerID) - - if err := graph.Create(containerInit, graphID1, nil); err != nil { - t.Fatal(err) - } - if _, err := graph.ApplyDiff(containerInit, graphID1, bytes.NewReader(initTar)); err != nil { - t.Fatal(err) - } - - if err := graph.Create(containerID, containerInit, nil); err != nil { - t.Fatal(err) - } - if _, err := graph.ApplyDiff(containerID, containerInit, bytes.NewReader(mountTar)); err != nil { - t.Fatal(err) - } - - if err := ls.(*layerStore).CreateRWLayerByGraphID("migration-mount", containerID, layer1.ChainID()); err != nil { - t.Fatal(err) - } - - rwLayer1, err := ls.GetRWLayer("migration-mount") - if err != nil { - t.Fatal(err) - } - - if _, err := rwLayer1.Mount(""); err != nil { - t.Fatal(err) - } - - changes, err := rwLayer1.Changes() - if err != nil { - t.Fatal(err) - } - - if expected := 5; len(changes) != expected { - t.Logf("Changes %#v", changes) - t.Fatalf("Wrong number of changes %d, expected %d", len(changes), expected) - } - - sortChanges(changes) - - assertChange(t, changes[0], archive.Change{ - Path: "/etc", - Kind: archive.ChangeModify, - }) - assertChange(t, changes[1], archive.Change{ - Path: "/etc/hosts", - Kind: archive.ChangeModify, - }) - assertChange(t, changes[2], archive.Change{ - Path: "/root", - Kind: archive.ChangeModify, - }) - assertChange(t, changes[3], archive.Change{ - Path: "/root/.bashrc", - Kind: archive.ChangeModify, - }) - assertChange(t, changes[4], archive.Change{ - Path: "/root/testfile1.txt", - Kind: archive.ChangeAdd, - }) - - if _, err := ls.CreateRWLayer("migration-mount", layer1.ChainID(), nil); err == nil { - t.Fatal("Expected error creating mount with same name") - } else if err != ErrMountNameConflict { - t.Fatal(err) - } - - rwLayer2, err := ls.GetRWLayer("migration-mount") - if err != nil { - t.Fatal(err) - } - - if getMountLayer(rwLayer1) != getMountLayer(rwLayer2) { - t.Fatal("Expected same layer from get with same name as from migrate") - } - - if _, err := rwLayer2.Mount(""); err != nil { - t.Fatal(err) - } - - if _, err := rwLayer2.Mount(""); err != nil { - t.Fatal(err) - } - - if metadata, err := ls.Release(layer1); err != nil { - t.Fatal(err) - } else if len(metadata) > 0 { - t.Fatalf("Expected no layers to be deleted, deleted %#v", metadata) - } - - if err := rwLayer1.Unmount(); err != nil { - t.Fatal(err) - } - - if _, err := ls.ReleaseRWLayer(rwLayer1); err != nil { - t.Fatal(err) - } - - if err := rwLayer2.Unmount(); err != nil { - t.Fatal(err) - } - if err := rwLayer2.Unmount(); err != nil { - t.Fatal(err) - } - metadata, err := ls.ReleaseRWLayer(rwLayer2) - if err != nil { - t.Fatal(err) - } - if len(metadata) == 0 { - t.Fatal("Expected base layer to be deleted when deleting mount") - } - - assertMetadata(t, metadata, createMetadata(layer1)) -} From e0456faac20cfc17d301da046d427e5b1641dce3 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 20 May 2019 14:46:54 -0700 Subject: [PATCH 13/35] layer: protect from same-name races As pointed out by Tonis, there's a race between ReleaseRWLayer() and GetRWLayer(): ``` ----- goroutine 1 ----- ----- goroutine 2 ----- ReleaseRWLayer() m := ls.mounts[l.Name()] ... m.deleteReference(l) m.hasReferences() ... GetRWLayer() ... mount := ls.mounts[id] ls.driver.Remove(m.mountID) ls.store.RemoveMount(m.name) return mount.getReference() delete(ls.mounts, m.Name()) ----------------------- ----------------------- ``` When something like this happens, GetRWLayer will return an RWLayer without a storage. Oops. There might be more races like this, and it seems the best solution is to lock by layer id/name by using pkg/locker. With this in place, name collision could not happen, so remove the part of previous commit that protected against it in CreateRWLayer (temporary nil assigmment and associated rollback). So, now we have * layerStore.mountL sync.Mutex to protect layerStore.mount map[] (against concurrent access); * mountedLayer's embedded `sync.Mutex` to protect its references map[]; * layerStore.layerL (which I haven't touched); * per-id locker, to avoid name conflicts and concurrent operations on the same rw layer. The whole rig seems to look more readable now (mutexes use is straightforward, no nested locks). Reported-by: Tonis Tiigi Signed-off-by: Kir Kolyshkin Signed-off-by: Kir Kolyshkin (cherry picked from commit af433dd200f8287305b1531d5058780be36b7e2e) Signed-off-by: Sebastiaan van Stijn Upstream-commit: 1ebe324c6a481de86a5b84494057a639773624f1 Component: engine --- components/engine/layer/layer_store.go | 42 +++++++++++++++----------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/components/engine/layer/layer_store.go b/components/engine/layer/layer_store.go index 737283cc2d..81730e9d92 100644 --- a/components/engine/layer/layer_store.go +++ b/components/engine/layer/layer_store.go @@ -10,6 +10,7 @@ import ( "github.com/docker/distribution" "github.com/docker/docker/daemon/graphdriver" "github.com/docker/docker/pkg/idtools" + "github.com/docker/docker/pkg/locker" "github.com/docker/docker/pkg/plugingetter" "github.com/docker/docker/pkg/stringid" "github.com/docker/docker/pkg/system" @@ -36,7 +37,11 @@ type layerStore struct { mounts map[string]*mountedLayer mountL sync.Mutex - os string + + // protect *RWLayer() methods from operating on the same name/id + locker *locker.Locker + + os string } // StoreOptions are the options used to create a new Store instance @@ -92,6 +97,7 @@ func newStoreFromGraphDriver(root string, driver graphdriver.Driver, os string) driver: driver, layerMap: map[ChainID]*roLayer{}, mounts: map[string]*mountedLayer{}, + locker: locker.New(), useTarSplit: !caps.ReproducesExactDiffs, os: os, } @@ -191,7 +197,7 @@ func (ls *layerStore) loadLayer(layer ChainID) (*roLayer, error) { func (ls *layerStore) loadMount(mount string) error { ls.mountL.Lock() defer ls.mountL.Unlock() - if m := ls.mounts[mount]; m != nil { + if _, ok := ls.mounts[mount]; ok { return nil } @@ -492,21 +498,15 @@ func (ls *layerStore) CreateRWLayer(name string, parent ChainID, opts *CreateRWL initFunc = opts.InitFunc } + ls.locker.Lock(name) + defer ls.locker.Unlock(name) + ls.mountL.Lock() - if _, ok := ls.mounts[name]; ok { - ls.mountL.Unlock() + _, ok := ls.mounts[name] + ls.mountL.Unlock() + if ok { return nil, ErrMountNameConflict } - // Avoid name collision by temporary assigning nil - ls.mounts[name] = nil - ls.mountL.Unlock() - defer func() { - if err != nil { - ls.mountL.Lock() - delete(ls.mounts, name) - ls.mountL.Unlock() - } - }() var pid string var p *roLayer @@ -558,6 +558,9 @@ func (ls *layerStore) CreateRWLayer(name string, parent ChainID, opts *CreateRWL } func (ls *layerStore) GetRWLayer(id string) (RWLayer, error) { + ls.locker.Lock(id) + defer ls.locker.Unlock(id) + ls.mountL.Lock() mount := ls.mounts[id] ls.mountL.Unlock() @@ -570,8 +573,9 @@ func (ls *layerStore) GetRWLayer(id string) (RWLayer, error) { func (ls *layerStore) GetMountID(id string) (string, error) { ls.mountL.Lock() - defer ls.mountL.Unlock() mount := ls.mounts[id] + ls.mountL.Unlock() + if mount == nil { return "", ErrMountDoesNotExist } @@ -581,8 +585,12 @@ func (ls *layerStore) GetMountID(id string) (string, error) { } func (ls *layerStore) ReleaseRWLayer(l RWLayer) ([]Metadata, error) { + name := l.Name() + ls.locker.Lock(name) + defer ls.locker.Unlock(name) + ls.mountL.Lock() - m := ls.mounts[l.Name()] + m := ls.mounts[name] ls.mountL.Unlock() if m == nil { return []Metadata{}, nil @@ -617,7 +625,7 @@ func (ls *layerStore) ReleaseRWLayer(l RWLayer) ([]Metadata, error) { } ls.mountL.Lock() - delete(ls.mounts, m.Name()) + delete(ls.mounts, name) ls.mountL.Unlock() ls.layerL.Lock() From edfabc9b63f69299182e4391e47bf43f49a9fb76 Mon Sep 17 00:00:00 2001 From: Olli Janatuinen Date: Wed, 22 May 2019 15:54:01 +0300 Subject: [PATCH 14/35] Windows CI - Corrected LOCALAPPDATA location Signed-off-by: Olli Janatuinen (cherry picked from commit 61815f676391ca24bbbdf3cebcb47cefad404035) Signed-off-by: Sebastiaan van Stijn Upstream-commit: d0beadc90cc797d9a05d29b880a3c8ece5f8bca8 Component: engine --- components/engine/hack/ci/windows.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/engine/hack/ci/windows.ps1 b/components/engine/hack/ci/windows.ps1 index c2c937f4cb..8f8b9192bd 100644 --- a/components/engine/hack/ci/windows.ps1 +++ b/components/engine/hack/ci/windows.ps1 @@ -409,7 +409,7 @@ Try { # Redirect to a temporary location. $TEMPORIG=$env:TEMP $env:TEMP="$env:TESTRUN_DRIVE`:\$env:TESTRUN_SUBDIR\CI-$COMMITHASH" - $env:LOCALAPPDATA="$TEMP\localappdata" + $env:LOCALAPPDATA="$env:TEMP\localappdata" $errorActionPreference='Stop' New-Item -ItemType Directory "$env:TEMP" -ErrorAction SilentlyContinue | Out-Null New-Item -ItemType Directory "$env:TEMP\userprofile" -ErrorAction SilentlyContinue | Out-Null From 741575eb3891c8b2e27227f74e108795447b9a1d Mon Sep 17 00:00:00 2001 From: Drew Erny Date: Mon, 3 Jun 2019 11:33:01 -0500 Subject: [PATCH 15/35] Increase max recv gRPC message size for nodes and secrets Increases the max recieved gRPC message size for Node and Secret list operations. This has already been done for the other swarm types, but was not done for these. Signed-off-by: Drew Erny (cherry picked from commit a0903e1fa3eca32065c7dbfda8d1e0879cbfbb8f) Signed-off-by: Sebastiaan van Stijn Upstream-commit: a987f31fbc42526bf2cccf1381066f075b18dfe0 Component: engine --- components/engine/daemon/cluster/nodes.go | 5 ++++- components/engine/daemon/cluster/secrets.go | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/components/engine/daemon/cluster/nodes.go b/components/engine/daemon/cluster/nodes.go index 3c073b0bac..dffd7556f0 100644 --- a/components/engine/daemon/cluster/nodes.go +++ b/components/engine/daemon/cluster/nodes.go @@ -8,6 +8,7 @@ import ( "github.com/docker/docker/daemon/cluster/convert" "github.com/docker/docker/errdefs" swarmapi "github.com/docker/swarmkit/api" + "google.golang.org/grpc" ) // GetNodes returns a list of all nodes known to a cluster. @@ -30,7 +31,9 @@ func (c *Cluster) GetNodes(options apitypes.NodeListOptions) ([]types.Node, erro r, err := state.controlClient.ListNodes( ctx, - &swarmapi.ListNodesRequest{Filters: filters}) + &swarmapi.ListNodesRequest{Filters: filters}, + grpc.MaxCallRecvMsgSize(defaultRecvSizeForListResponse), + ) if err != nil { return nil, err } diff --git a/components/engine/daemon/cluster/secrets.go b/components/engine/daemon/cluster/secrets.go index c6fd842081..6f652eb54d 100644 --- a/components/engine/daemon/cluster/secrets.go +++ b/components/engine/daemon/cluster/secrets.go @@ -7,6 +7,7 @@ import ( types "github.com/docker/docker/api/types/swarm" "github.com/docker/docker/daemon/cluster/convert" swarmapi "github.com/docker/swarmkit/api" + "google.golang.org/grpc" ) // GetSecret returns a secret from a managed swarm cluster @@ -44,7 +45,9 @@ func (c *Cluster) GetSecrets(options apitypes.SecretListOptions) ([]types.Secret defer cancel() r, err := state.controlClient.ListSecrets(ctx, - &swarmapi.ListSecretsRequest{Filters: filters}) + &swarmapi.ListSecretsRequest{Filters: filters}, + grpc.MaxCallRecvMsgSize(defaultRecvSizeForListResponse), + ) if err != nil { return nil, err } From 6f78462c1a513ee1471e53aebf5b117df5fa7220 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 24 Oct 2018 17:29:03 -0700 Subject: [PATCH 16/35] UnmountIpcMount: simplify As standard mount.Unmount does what we need, let's use it. In addition, this adds ignoring "not mounted" condition, which was previously implemented (see PR#33329, commit cfa2591d3f26) via a very expensive call to mount.Mounted(). Signed-off-by: Kir Kolyshkin (cherry picked from commit 77bc327e24a60791fe7e87980faf704cf7273cf9) Upstream-commit: 893b24b80db170279d5c9532ed508a81c328de5e Component: engine --- components/engine/container/container_unix.go | 10 ++++------ components/engine/container/container_windows.go | 2 +- components/engine/daemon/container_operations_unix.go | 4 ---- .../engine/daemon/container_operations_windows.go | 4 ---- components/engine/daemon/start.go | 2 +- 5 files changed, 6 insertions(+), 16 deletions(-) diff --git a/components/engine/container/container_unix.go b/components/engine/container/container_unix.go index 1a184555ee..7ee0a7e7da 100644 --- a/components/engine/container/container_unix.go +++ b/components/engine/container/container_unix.go @@ -175,8 +175,8 @@ func (container *Container) HasMountFor(path string) bool { return false } -// UnmountIpcMount uses the provided unmount function to unmount shm if it was mounted -func (container *Container) UnmountIpcMount(unmount func(pth string) error) error { +// UnmountIpcMount unmounts shm if it was mounted +func (container *Container) UnmountIpcMount() error { if container.HasMountFor("/dev/shm") { return nil } @@ -190,10 +190,8 @@ func (container *Container) UnmountIpcMount(unmount func(pth string) error) erro if shmPath == "" { return nil } - if err = unmount(shmPath); err != nil && !os.IsNotExist(err) { - if mounted, mErr := mount.Mounted(shmPath); mounted || mErr != nil { - return errors.Wrapf(err, "umount %s", shmPath) - } + if err = mount.Unmount(shmPath); err != nil && !os.IsNotExist(err) { + return errors.Wrapf(err, "umount %s", shmPath) } return nil } diff --git a/components/engine/container/container_windows.go b/components/engine/container/container_windows.go index b5bdb5bc34..090db12c20 100644 --- a/components/engine/container/container_windows.go +++ b/components/engine/container/container_windows.go @@ -22,7 +22,7 @@ const ( // UnmountIpcMount unmounts Ipc related mounts. // This is a NOOP on windows. -func (container *Container) UnmountIpcMount(unmount func(pth string) error) error { +func (container *Container) UnmountIpcMount() error { return nil } diff --git a/components/engine/daemon/container_operations_unix.go b/components/engine/daemon/container_operations_unix.go index c0aab72342..03b01b8061 100644 --- a/components/engine/daemon/container_operations_unix.go +++ b/components/engine/daemon/container_operations_unix.go @@ -351,10 +351,6 @@ func killProcessDirectly(cntr *container.Container) error { return nil } -func detachMounted(path string) error { - return unix.Unmount(path, unix.MNT_DETACH) -} - func isLinkable(child *container.Container) bool { // A container is linkable only if it belongs to the default network _, ok := child.NetworkSettings.Networks[runconfig.DefaultDaemonNetworkMode().NetworkName()] diff --git a/components/engine/daemon/container_operations_windows.go b/components/engine/daemon/container_operations_windows.go index 349d3a1566..10bfd53d6e 100644 --- a/components/engine/daemon/container_operations_windows.go +++ b/components/engine/daemon/container_operations_windows.go @@ -78,10 +78,6 @@ func (daemon *Daemon) mountVolumes(container *container.Container) error { return nil } -func detachMounted(path string) error { - return nil -} - func (daemon *Daemon) setupSecretDir(c *container.Container) (setupErr error) { if len(c.SecretReferences) == 0 { return nil diff --git a/components/engine/daemon/start.go b/components/engine/daemon/start.go index e2265a4fae..e2416efe5e 100644 --- a/components/engine/daemon/start.go +++ b/components/engine/daemon/start.go @@ -229,7 +229,7 @@ func (daemon *Daemon) containerStart(container *container.Container, checkpoint func (daemon *Daemon) Cleanup(container *container.Container) { daemon.releaseNetwork(container) - if err := container.UnmountIpcMount(detachMounted); err != nil { + if err := container.UnmountIpcMount(); err != nil { logrus.Warnf("%s cleanup: failed to unmount IPC: %s", container.ID, err) } From 51371cb25220f530414d6c2b5aca4d34e35da7ef Mon Sep 17 00:00:00 2001 From: Omri Shiv Date: Fri, 30 Nov 2018 12:58:10 -0800 Subject: [PATCH 17/35] fix typo Signed-off-by: Omri Shiv (cherry picked from commit fe1083d4622658e9e5bf7319d0f94873019fced2) Upstream-commit: b941f081523cce7defafd4457f380442e10fa345 Component: engine --- components/engine/container/container_unix.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/engine/container/container_unix.go b/components/engine/container/container_unix.go index 7ee0a7e7da..592a6dc375 100644 --- a/components/engine/container/container_unix.go +++ b/components/engine/container/container_unix.go @@ -381,7 +381,7 @@ func (container *Container) DetachAndUnmount(volumeEventLog func(name, action st for _, mountPath := range mountPaths { if err := mount.Unmount(mountPath); err != nil { - logrus.Warnf("%s unmountVolumes: Failed to do lazy umount fo volume '%s': %v", container.ID, mountPath, err) + logrus.Warnf("%s unmountVolumes: Failed to do lazy umount for volume '%s': %v", container.ID, mountPath, err) } } return container.UnmountVolumes(volumeEventLog) From e213748382c523e9c6a42dd7f4a2d968629a9d82 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 25 Oct 2018 10:44:28 -0700 Subject: [PATCH 18/35] pkg/mount: refactor Unmount() It has been pointed out that we're ignoring EINVAL from umount(2) everywhere, so let's move it to a lower-level function. Also, its implementation should be the same for any UNIX incarnation, so let's consolidate it. Signed-off-by: Kir Kolyshkin (cherry picked from commit 90be078fe59a8cfeff2bcc5dc2f34a00309837b6) Upstream-commit: 47c51447e1b6dacf92b40574f6f929958ca9d621 Component: engine --- components/engine/pkg/mount/mount.go | 27 ++++--------------- .../engine/pkg/mount/mounter_freebsd.go | 6 ----- components/engine/pkg/mount/mounter_linux.go | 4 --- components/engine/pkg/mount/unmount_unix.go | 17 ++++++++++++ 4 files changed, 22 insertions(+), 32 deletions(-) create mode 100644 components/engine/pkg/mount/unmount_unix.go diff --git a/components/engine/pkg/mount/mount.go b/components/engine/pkg/mount/mount.go index 874aff6545..2d79bc390d 100644 --- a/components/engine/pkg/mount/mount.go +++ b/components/engine/pkg/mount/mount.go @@ -3,7 +3,6 @@ package mount // import "github.com/docker/docker/pkg/mount" import ( "sort" "strings" - "syscall" "github.com/sirupsen/logrus" ) @@ -89,12 +88,7 @@ func ForceMount(device, target, mType, options string) error { // Unmount lazily unmounts a filesystem on supported platforms, otherwise // does a normal unmount. func Unmount(target string) error { - err := unmount(target, mntDetach) - if err == syscall.EINVAL { - // ignore "not mounted" error - err = nil - } - return err + return unmount(target, mntDetach) } // RecursiveUnmount unmounts the target and all mounts underneath, starting with @@ -114,25 +108,14 @@ func RecursiveUnmount(target string) error { logrus.Debugf("Trying to unmount %s", m.Mountpoint) err = unmount(m.Mountpoint, mntDetach) if err != nil { - // If the error is EINVAL either this whole package is wrong (invalid flags passed to unmount(2)) or this is - // not a mountpoint (which is ok in this case). - // Meanwhile calling `Mounted()` is very expensive. - // - // We've purposefully used `syscall.EINVAL` here instead of `unix.EINVAL` to avoid platform branching - // Since `EINVAL` is defined for both Windows and Linux in the `syscall` package (and other platforms), - // this is nicer than defining a custom value that we can refer to in each platform file. - if err == syscall.EINVAL { - continue - } - if i == len(mounts)-1 { + if i == len(mounts)-1 { // last mount if mounted, e := Mounted(m.Mountpoint); e != nil || mounted { return err } - continue + } else { + // This is some submount, we can ignore this error for now, the final unmount will fail if this is a real problem + logrus.WithError(err).Warnf("Failed to unmount submount %s", m.Mountpoint) } - // This is some submount, we can ignore this error for now, the final unmount will fail if this is a real problem - logrus.WithError(err).Warnf("Failed to unmount submount %s", m.Mountpoint) - continue } logrus.Debugf("Unmounted %s", m.Mountpoint) diff --git a/components/engine/pkg/mount/mounter_freebsd.go b/components/engine/pkg/mount/mounter_freebsd.go index b6ab83a230..8a46eaf454 100644 --- a/components/engine/pkg/mount/mounter_freebsd.go +++ b/components/engine/pkg/mount/mounter_freebsd.go @@ -14,8 +14,6 @@ import ( "fmt" "strings" "unsafe" - - "golang.org/x/sys/unix" ) func allocateIOVecs(options []string) []C.struct_iovec { @@ -54,7 +52,3 @@ func mount(device, target, mType string, flag uintptr, data string) error { } return nil } - -func unmount(target string, flag int) error { - return unix.Unmount(target, flag) -} diff --git a/components/engine/pkg/mount/mounter_linux.go b/components/engine/pkg/mount/mounter_linux.go index 631daf10a5..2c189de0ae 100644 --- a/components/engine/pkg/mount/mounter_linux.go +++ b/components/engine/pkg/mount/mounter_linux.go @@ -51,7 +51,3 @@ func mount(device, target, mType string, flags uintptr, data string) error { return nil } - -func unmount(target string, flag int) error { - return unix.Unmount(target, flag) -} diff --git a/components/engine/pkg/mount/unmount_unix.go b/components/engine/pkg/mount/unmount_unix.go new file mode 100644 index 0000000000..d027e3086d --- /dev/null +++ b/components/engine/pkg/mount/unmount_unix.go @@ -0,0 +1,17 @@ +// +build !windows + +package mount // import "github.com/docker/docker/pkg/mount" + +import "golang.org/x/sys/unix" + +func unmount(target string, flags int) error { + err := unix.Unmount(target, flags) + if err == unix.EINVAL { + // Ignore "not mounted" error here. Note the same error + // can be returned if flags are invalid, so this code + // assumes that the flags value is always correct. + err = nil + } + + return err +} From bb4beac3ed44dcd4d3ec9b9f434a7bbf0993f91d Mon Sep 17 00:00:00 2001 From: John Howard Date: Tue, 4 Jun 2019 13:38:52 -0700 Subject: [PATCH 19/35] Windows: Don't attempt detach VHD for R/O layers Signed-off-by: John Howard (cherry picked from commit 293c74ba79f0008f48073985507b34af59b45fa6) Signed-off-by: Sebastiaan van Stijn Upstream-commit: 91f5be57af815df372371e1e989d00963ce4d02f Component: engine --- .../engine/daemon/graphdriver/windows/windows.go | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/components/engine/daemon/graphdriver/windows/windows.go b/components/engine/daemon/graphdriver/windows/windows.go index 52b0c6d845..699e434f4c 100644 --- a/components/engine/daemon/graphdriver/windows/windows.go +++ b/components/engine/daemon/graphdriver/windows/windows.go @@ -338,11 +338,14 @@ func (d *Driver) Remove(id string) error { // If permission denied, it's possible that the scratch is still mounted, an // artifact after a hard daemon crash for example. Worth a shot to try detaching it // before retrying the rename. - if detachErr := vhd.DetachVhd(filepath.Join(layerPath, "sandbox.vhdx")); detachErr != nil { - return errors.Wrapf(err, "failed to detach VHD: %s", detachErr) - } - if renameErr := os.Rename(layerPath, tmpLayerPath); renameErr != nil && !os.IsNotExist(renameErr) { - return errors.Wrapf(err, "second rename attempt following detach failed: %s", renameErr) + sandbox := filepath.Join(layerPath, "sandbox.vhdx") + if _, statErr := os.Stat(sandbox); statErr == nil { + if detachErr := vhd.DetachVhd(sandbox); detachErr != nil { + return errors.Wrapf(err, "failed to detach VHD: %s", detachErr) + } + if renameErr := os.Rename(layerPath, tmpLayerPath); renameErr != nil && !os.IsNotExist(renameErr) { + return errors.Wrapf(err, "second rename attempt following detach failed: %s", renameErr) + } } } if err := hcsshim.DestroyLayer(d.info, tmpID); err != nil { From c81448bb56c0083877ec45fffe2fc4d196bbde3a Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 22 Oct 2018 18:30:34 -0700 Subject: [PATCH 20/35] pkg/mount: wrap mount/umount errors The errors returned from Mount and Unmount functions are raw syscall.Errno errors (like EPERM or EINVAL), which provides no context about what has happened and why. Similar to os.PathError type, introduce mount.Error type with some context. The error messages will now look like this: > mount /tmp/mount-tests/source:/tmp/mount-tests/target, flags: 0x1001: operation not permitted or > mount tmpfs:/tmp/mount-test-source-516297835: operation not permitted Before this patch, it was just > operation not permitted [v2: add Cause()] [v3: rename MountError to Error, document Cause()] [v4: fixes; audited all users] [v5: make Error type private; changes after @cpuguy83 reviews] Signed-off-by: Kir Kolyshkin (cherry picked from commit 65331369617e89ce54cc9be080dba70f3a883d1c) Signed-off-by: Kir Kolyshkin Upstream-commit: 7f1c6bf5a745c8faeba695d3556dff4c4ff5f473 Component: engine --- components/engine/container/container_unix.go | 5 +-- .../engine/daemon/graphdriver/btrfs/btrfs.go | 2 +- .../daemon/graphdriver/devmapper/deviceset.go | 4 +-- .../daemon/graphdriver/devmapper/driver.go | 7 +--- .../engine/daemon/graphdriver/zfs/zfs.go | 3 +- components/engine/pkg/mount/mount.go | 35 +++++++++++++++++++ .../engine/pkg/mount/mounter_freebsd.go | 11 ++++-- components/engine/pkg/mount/mounter_linux.go | 25 +++++++++++-- .../pkg/mount/sharedsubtree_linux_test.go | 3 +- components/engine/pkg/mount/unmount_unix.go | 11 ++++-- components/engine/plugin/manager_linux.go | 2 +- components/engine/volume/local/local.go | 2 +- components/engine/volume/local/local_unix.go | 2 +- 13 files changed, 88 insertions(+), 24 deletions(-) diff --git a/components/engine/container/container_unix.go b/components/engine/container/container_unix.go index 592a6dc375..6d402be3a0 100644 --- a/components/engine/container/container_unix.go +++ b/components/engine/container/container_unix.go @@ -191,7 +191,7 @@ func (container *Container) UnmountIpcMount() error { return nil } if err = mount.Unmount(shmPath); err != nil && !os.IsNotExist(err) { - return errors.Wrapf(err, "umount %s", shmPath) + return err } return nil } @@ -381,7 +381,8 @@ func (container *Container) DetachAndUnmount(volumeEventLog func(name, action st for _, mountPath := range mountPaths { if err := mount.Unmount(mountPath); err != nil { - logrus.Warnf("%s unmountVolumes: Failed to do lazy umount for volume '%s': %v", container.ID, mountPath, err) + logrus.WithError(err).WithField("container", container.ID). + Warn("Unable to unmount") } } return container.UnmountVolumes(volumeEventLog) diff --git a/components/engine/daemon/graphdriver/btrfs/btrfs.go b/components/engine/daemon/graphdriver/btrfs/btrfs.go index 7ce7edef36..fcaedc6eab 100644 --- a/components/engine/daemon/graphdriver/btrfs/btrfs.go +++ b/components/engine/daemon/graphdriver/btrfs/btrfs.go @@ -178,7 +178,7 @@ func (d *Driver) Cleanup() error { } if umountErr != nil { - return errors.Wrapf(umountErr, "error unmounting %s", d.home) + return umountErr } return nil diff --git a/components/engine/daemon/graphdriver/devmapper/deviceset.go b/components/engine/daemon/graphdriver/devmapper/deviceset.go index 5dc01d71d9..c41b50c119 100644 --- a/components/engine/daemon/graphdriver/devmapper/deviceset.go +++ b/components/engine/daemon/graphdriver/devmapper/deviceset.go @@ -1200,7 +1200,7 @@ func (devices *DeviceSet) growFS(info *devInfo) error { options = joinMountOptions(options, devices.mountOptions) if err := mount.Mount(info.DevName(), fsMountPoint, devices.BaseDeviceFilesystem, options); err != nil { - return fmt.Errorf("Error mounting '%s' on '%s' (fstype='%s' options='%s'): %s\n%v", info.DevName(), fsMountPoint, devices.BaseDeviceFilesystem, options, err, string(dmesg.Dmesg(256))) + return errors.Wrapf(err, "Failed to mount; dmesg: %s", string(dmesg.Dmesg(256))) } defer unix.Unmount(fsMountPoint, unix.MNT_DETACH) @@ -2381,7 +2381,7 @@ func (devices *DeviceSet) MountDevice(hash, path, mountLabel string) error { options = joinMountOptions(options, label.FormatMountLabel("", mountLabel)) if err := mount.Mount(info.DevName(), path, fstype, options); err != nil { - return fmt.Errorf("devmapper: Error mounting '%s' on '%s' (fstype='%s' options='%s'): %s\n%v", info.DevName(), path, fstype, options, err, string(dmesg.Dmesg(256))) + return errors.Wrapf(err, "Failed to mount; dmesg: %s", string(dmesg.Dmesg(256))) } if fstype == "xfs" && devices.xfsNospaceRetries != "" { diff --git a/components/engine/daemon/graphdriver/devmapper/driver.go b/components/engine/daemon/graphdriver/devmapper/driver.go index 899b1f8670..a42a03ba90 100644 --- a/components/engine/daemon/graphdriver/devmapper/driver.go +++ b/components/engine/daemon/graphdriver/devmapper/driver.go @@ -16,7 +16,6 @@ import ( "github.com/docker/docker/pkg/locker" "github.com/docker/docker/pkg/mount" "github.com/docker/go-units" - "github.com/pkg/errors" "github.com/sirupsen/logrus" "golang.org/x/sys/unix" ) @@ -129,11 +128,7 @@ func (d *Driver) Cleanup() error { return err } - if umountErr != nil { - return errors.Wrapf(umountErr, "error unmounting %s", d.home) - } - - return nil + return umountErr } // CreateReadWrite creates a layer that is writable for use as a container diff --git a/components/engine/daemon/graphdriver/zfs/zfs.go b/components/engine/daemon/graphdriver/zfs/zfs.go index 8a798778d2..c83446cf8f 100644 --- a/components/engine/daemon/graphdriver/zfs/zfs.go +++ b/components/engine/daemon/graphdriver/zfs/zfs.go @@ -19,6 +19,7 @@ import ( "github.com/docker/docker/pkg/parsers" "github.com/mistifyio/go-zfs" "github.com/opencontainers/selinux/go-selinux/label" + "github.com/pkg/errors" "github.com/sirupsen/logrus" "golang.org/x/sys/unix" ) @@ -390,7 +391,7 @@ func (d *Driver) Get(id, mountLabel string) (_ containerfs.ContainerFS, retErr e } if err := mount.Mount(filesystem, mountpoint, "zfs", options); err != nil { - return nil, fmt.Errorf("error creating zfs mount of %s to %s: %v", filesystem, mountpoint, err) + return nil, errors.Wrap(err, "error creating zfs mount") } // this could be our first mount after creation of the filesystem, and the root dir may still have root diff --git a/components/engine/pkg/mount/mount.go b/components/engine/pkg/mount/mount.go index 2d79bc390d..4afd63c427 100644 --- a/components/engine/pkg/mount/mount.go +++ b/components/engine/pkg/mount/mount.go @@ -2,11 +2,46 @@ package mount // import "github.com/docker/docker/pkg/mount" import ( "sort" + "strconv" "strings" "github.com/sirupsen/logrus" ) +// mountError records an error from mount or unmount operation +type mountError struct { + op string + source, target string + flags uintptr + data string + err error +} + +func (e *mountError) Error() string { + out := e.op + " " + + if e.source != "" { + out += e.source + ":" + e.target + } else { + out += e.target + } + + if e.flags != uintptr(0) { + out += ", flags: 0x" + strconv.FormatUint(uint64(e.flags), 16) + } + if e.data != "" { + out += ", data: " + e.data + } + + out += ": " + e.err.Error() + return out +} + +// Cause returns the underlying cause of the error +func (e *mountError) Cause() error { + return e.err +} + // FilterFunc is a type defining a callback function // to filter out unwanted entries. It takes a pointer // to an Info struct (not fully populated, currently diff --git a/components/engine/pkg/mount/mounter_freebsd.go b/components/engine/pkg/mount/mounter_freebsd.go index 8a46eaf454..09ad360608 100644 --- a/components/engine/pkg/mount/mounter_freebsd.go +++ b/components/engine/pkg/mount/mounter_freebsd.go @@ -11,8 +11,8 @@ package mount // import "github.com/docker/docker/pkg/mount" import "C" import ( - "fmt" "strings" + "syscall" "unsafe" ) @@ -47,8 +47,13 @@ func mount(device, target, mType string, flag uintptr, data string) error { } if errno := C.nmount(&rawOptions[0], C.uint(len(options)), C.int(flag)); errno != 0 { - reason := C.GoString(C.strerror(*C.__error())) - return fmt.Errorf("Failed to call nmount: %s", reason) + return &mountError{ + op: "mount", + source: device, + target: target, + flags: flag, + err: syscall.Errno(errno), + } } return nil } diff --git a/components/engine/pkg/mount/mounter_linux.go b/components/engine/pkg/mount/mounter_linux.go index 2c189de0ae..48837adde5 100644 --- a/components/engine/pkg/mount/mounter_linux.go +++ b/components/engine/pkg/mount/mounter_linux.go @@ -33,20 +33,41 @@ func mount(device, target, mType string, flags uintptr, data string) error { // Initial call applying all non-propagation flags for mount // or remount with changed data if err := unix.Mount(device, target, mType, oflags, data); err != nil { - return err + return &mountError{ + op: "mount", + source: device, + target: target, + flags: oflags, + data: data, + err: err, + } } } if flags&ptypes != 0 { // Change the propagation type. if err := unix.Mount("", target, "", flags&pflags, ""); err != nil { + return &mountError{ + op: "remount", + target: target, + flags: flags & pflags, + err: err, + } return err } } if oflags&broflags == broflags { // Remount the bind to apply read only. - return unix.Mount("", target, "", oflags|unix.MS_REMOUNT, "") + if err := unix.Mount("", target, "", oflags|unix.MS_REMOUNT, ""); err != nil { + return &mountError{ + op: "remount-ro", + target: target, + flags: oflags | unix.MS_REMOUNT, + err: err, + } + + } } return nil diff --git a/components/engine/pkg/mount/sharedsubtree_linux_test.go b/components/engine/pkg/mount/sharedsubtree_linux_test.go index 019514491f..7a37f66098 100644 --- a/components/engine/pkg/mount/sharedsubtree_linux_test.go +++ b/components/engine/pkg/mount/sharedsubtree_linux_test.go @@ -7,6 +7,7 @@ import ( "path" "testing" + "github.com/pkg/errors" "golang.org/x/sys/unix" ) @@ -326,7 +327,7 @@ func TestSubtreeUnbindable(t *testing.T) { }() // then attempt to mount it to target. It should fail - if err := Mount(sourceDir, targetDir, "none", "bind,rw"); err != nil && err != unix.EINVAL { + if err := Mount(sourceDir, targetDir, "none", "bind,rw"); err != nil && errors.Cause(err) != unix.EINVAL { t.Fatal(err) } else if err == nil { t.Fatalf("%q should not have been bindable", sourceDir) diff --git a/components/engine/pkg/mount/unmount_unix.go b/components/engine/pkg/mount/unmount_unix.go index d027e3086d..4be4276851 100644 --- a/components/engine/pkg/mount/unmount_unix.go +++ b/components/engine/pkg/mount/unmount_unix.go @@ -6,12 +6,17 @@ import "golang.org/x/sys/unix" func unmount(target string, flags int) error { err := unix.Unmount(target, flags) - if err == unix.EINVAL { + if err == nil || err == unix.EINVAL { // Ignore "not mounted" error here. Note the same error // can be returned if flags are invalid, so this code // assumes that the flags value is always correct. - err = nil + return nil } - return err + return &mountError{ + op: "umount", + target: target, + flags: uintptr(flags), + err: err, + } } diff --git a/components/engine/plugin/manager_linux.go b/components/engine/plugin/manager_linux.go index df1fe5b73d..86ada8d02f 100644 --- a/components/engine/plugin/manager_linux.go +++ b/components/engine/plugin/manager_linux.go @@ -61,7 +61,7 @@ func (pm *Manager) enable(p *v2.Plugin, c *controller, force bool) error { if err := pm.executor.Create(p.GetID(), *spec, stdout, stderr); err != nil { if p.PluginObj.Config.PropagatedMount != "" { if err := mount.Unmount(propRoot); err != nil { - logrus.Warnf("Could not unmount %s: %v", propRoot, err) + logrus.WithField("plugin", p.Name()).WithError(err).Warn("Failed to unmount vplugin propagated mount root") } } return errors.WithStack(err) diff --git a/components/engine/volume/local/local.go b/components/engine/volume/local/local.go index 7190de9ed6..d3119cb2ff 100644 --- a/components/engine/volume/local/local.go +++ b/components/engine/volume/local/local.go @@ -344,7 +344,7 @@ func (v *localVolume) unmount() error { if v.opts != nil { if err := mount.Unmount(v.path); err != nil { if mounted, mErr := mount.Mounted(v.path); mounted || mErr != nil { - return errdefs.System(errors.Wrapf(err, "error while unmounting volume path '%s'", v.path)) + return errdefs.System(err) } } v.active.mounted = false diff --git a/components/engine/volume/local/local_unix.go b/components/engine/volume/local/local_unix.go index b1c68b931b..5ee2ed894b 100644 --- a/components/engine/volume/local/local_unix.go +++ b/components/engine/volume/local/local_unix.go @@ -86,7 +86,7 @@ func (v *localVolume) mount() error { } } err := mount.Mount(v.opts.MountDevice, v.path, v.opts.MountType, mountOpts) - return errors.Wrapf(err, "error while mounting volume with options: %s", v.opts) + return errors.Wrap(err, "failed to mount local volume") } func (v *localVolume) CreatedAt() (time.Time, error) { From e03614a1565da2853447e0e954eb3203c4b6d337 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 24 Oct 2018 18:45:27 -0700 Subject: [PATCH 21/35] aufs: get rid of mount() The function is not needed as it's just a shallow wrapper around unix.Mount(). Signed-off-by: Kir Kolyshkin (cherry picked from commit 2f98b5f51fb0a21e1bd930c0e660c4a7c4918aa5) Upstream-commit: 023b63a0f276b79277c1c961d8c98d4d5b39525d Component: engine --- components/engine/daemon/graphdriver/aufs/aufs.go | 10 +++++----- .../engine/daemon/graphdriver/aufs/mount_linux.go | 7 ------- .../daemon/graphdriver/aufs/mount_unsupported.go | 12 ------------ 3 files changed, 5 insertions(+), 24 deletions(-) delete mode 100644 components/engine/daemon/graphdriver/aufs/mount_linux.go delete mode 100644 components/engine/daemon/graphdriver/aufs/mount_unsupported.go diff --git a/components/engine/daemon/graphdriver/aufs/aufs.go b/components/engine/daemon/graphdriver/aufs/aufs.go index 114aa9a615..11ca939904 100644 --- a/components/engine/daemon/graphdriver/aufs/aufs.go +++ b/components/engine/daemon/graphdriver/aufs/aufs.go @@ -43,7 +43,7 @@ import ( "github.com/docker/docker/pkg/directory" "github.com/docker/docker/pkg/idtools" "github.com/docker/docker/pkg/locker" - mountpk "github.com/docker/docker/pkg/mount" + "github.com/docker/docker/pkg/mount" "github.com/docker/docker/pkg/system" rsystem "github.com/opencontainers/runc/libcontainer/system" "github.com/opencontainers/selinux/go-selinux/label" @@ -598,7 +598,7 @@ func (a *Driver) Cleanup() error { logger.Debugf("error unmounting %s: %s", m, err) } } - return mountpk.RecursiveUnmount(a.root) + return mount.RecursiveUnmount(a.root) } func (a *Driver) aufsMount(ro []string, rw, target, mountLabel string) (err error) { @@ -632,14 +632,14 @@ func (a *Driver) aufsMount(ro []string, rw, target, mountLabel string) (err erro opts += ",dirperm1" } data := label.FormatMountLabel(fmt.Sprintf("%s,%s", string(b[:bp]), opts), mountLabel) - if err = mount("none", target, "aufs", 0, data); err != nil { + if err = unix.Mount("none", target, "aufs", 0, data); err != nil { return } for ; index < len(ro); index++ { layer := fmt.Sprintf(":%s=ro+wh", ro[index]) data := label.FormatMountLabel(fmt.Sprintf("append%s", layer), mountLabel) - if err = mount("none", target, "aufs", unix.MS_REMOUNT, data); err != nil { + if err = unix.Mount("none", target, "aufs", unix.MS_REMOUNT, data); err != nil { return } } @@ -666,7 +666,7 @@ func useDirperm() bool { defer os.RemoveAll(union) opts := fmt.Sprintf("br:%s,dirperm1,xino=/dev/shm/aufs.xino", base) - if err := mount("none", union, "aufs", 0, opts); err != nil { + if err := unix.Mount("none", union, "aufs", 0, opts); err != nil { return } enableDirperm = true diff --git a/components/engine/daemon/graphdriver/aufs/mount_linux.go b/components/engine/daemon/graphdriver/aufs/mount_linux.go deleted file mode 100644 index 8d5ad8f32d..0000000000 --- a/components/engine/daemon/graphdriver/aufs/mount_linux.go +++ /dev/null @@ -1,7 +0,0 @@ -package aufs // import "github.com/docker/docker/daemon/graphdriver/aufs" - -import "golang.org/x/sys/unix" - -func mount(source string, target string, fstype string, flags uintptr, data string) error { - return unix.Mount(source, target, fstype, flags, data) -} diff --git a/components/engine/daemon/graphdriver/aufs/mount_unsupported.go b/components/engine/daemon/graphdriver/aufs/mount_unsupported.go deleted file mode 100644 index cf7f58c29e..0000000000 --- a/components/engine/daemon/graphdriver/aufs/mount_unsupported.go +++ /dev/null @@ -1,12 +0,0 @@ -// +build !linux - -package aufs // import "github.com/docker/docker/daemon/graphdriver/aufs" - -import "errors" - -// MsRemount declared to specify a non-linux system mount. -const MsRemount = 0 - -func mount(source string, target string, fstype string, flags uintptr, data string) (err error) { - return errors.New("mount is not implemented on this platform") -} From a829a04518b9898fbfb655fd0f443e50fc7b4ec1 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 17 Apr 2019 19:29:32 -0700 Subject: [PATCH 22/35] aufs: remove extra locking Both mount and unmount calls are already protected by fine-grained (per id) locks in Get()/Put() introduced in commit fc1cf1911bb ("Add more locking to storage drivers"), so there's no point in having a global lock in mount/unmount. The only place from which unmount is called without any locking is Cleanup() -- this is to be addressed in the next patch. This reverts commit 824c24e6802ad3ed7e26b4f16e5ae81869b98185. Signed-off-by: Kir Kolyshkin (cherry picked from commit f93750b2c4d5f6144f0790ffa89291da3c097b80) Upstream-commit: 701939112efc71a0c867a626f0a56df9c56030db Component: engine --- components/engine/daemon/graphdriver/aufs/aufs.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/components/engine/daemon/graphdriver/aufs/aufs.go b/components/engine/daemon/graphdriver/aufs/aufs.go index 11ca939904..6c929fff35 100644 --- a/components/engine/daemon/graphdriver/aufs/aufs.go +++ b/components/engine/daemon/graphdriver/aufs/aufs.go @@ -72,7 +72,6 @@ func init() { // Driver contains information about the filesystem mounted. type Driver struct { - sync.Mutex root string uidMaps []idtools.IDMap gidMaps []idtools.IDMap @@ -547,9 +546,6 @@ func (a *Driver) getParentLayerPaths(id string) ([]string, error) { } func (a *Driver) mount(id string, target string, mountLabel string, layers []string) error { - a.Lock() - defer a.Unlock() - // If the id is mounted or we get an error return if mounted, err := a.mounted(target); err != nil || mounted { return err @@ -564,9 +560,6 @@ func (a *Driver) mount(id string, target string, mountLabel string, layers []str } func (a *Driver) unmount(mountPath string) error { - a.Lock() - defer a.Unlock() - if mounted, err := a.mounted(mountPath); err != nil || !mounted { return err } From 4fdc82012a1a9de2557737e5757b357c59187fe2 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 17 Apr 2019 19:52:36 -0700 Subject: [PATCH 23/35] aufs: use mount.Unmount 1. Use mount.Unmount() which ignores EINVAL ("not mounted") error, and provides better error diagnostics (so we don't have to explicitly add target to error messages). 2. Since we're ignoring "not mounted" error, we can call multiple unmounts without any locking -- but since "auplink flush" is still involved and can produce an error in logs, let's keep the check for fs being mounted (it's just a statfs so should be fast). 2. While at it, improve the "can't unmount" error message in Put(). Signed-off-by: Kir Kolyshkin (cherry picked from commit 4beee98026feabe4f4f0468215b8fd9b56f90d5e) Upstream-commit: 5b68d00abc4cad8b707a9a01dde9a420ca6eb995 Component: engine --- components/engine/daemon/graphdriver/aufs/aufs.go | 8 ++++---- components/engine/daemon/graphdriver/aufs/mount.go | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/components/engine/daemon/graphdriver/aufs/aufs.go b/components/engine/daemon/graphdriver/aufs/aufs.go index 6c929fff35..38df774225 100644 --- a/components/engine/daemon/graphdriver/aufs/aufs.go +++ b/components/engine/daemon/graphdriver/aufs/aufs.go @@ -326,11 +326,11 @@ func (a *Driver) Remove(id string) error { break } - if err != unix.EBUSY { - return errors.Wrapf(err, "aufs: unmount error: %s", mountpoint) + if errors.Cause(err) != unix.EBUSY { + return errors.Wrap(err, "aufs: unmount error") } if retries >= 5 { - return errors.Wrapf(err, "aufs: unmount error after retries: %s", mountpoint) + return errors.Wrap(err, "aufs: unmount error after retries") } // If unmount returns EBUSY, it could be a transient error. Sleep and retry. retries++ @@ -436,7 +436,7 @@ func (a *Driver) Put(id string) error { err := a.unmount(m) if err != nil { - logger.Debugf("Failed to unmount %s aufs: %v", id, err) + logger.WithError(err).WithField("method", "Put()").Warn() } return err } diff --git a/components/engine/daemon/graphdriver/aufs/mount.go b/components/engine/daemon/graphdriver/aufs/mount.go index 9f5510380c..4e85cf235d 100644 --- a/components/engine/daemon/graphdriver/aufs/mount.go +++ b/components/engine/daemon/graphdriver/aufs/mount.go @@ -5,7 +5,7 @@ package aufs // import "github.com/docker/docker/daemon/graphdriver/aufs" import ( "os/exec" - "golang.org/x/sys/unix" + "github.com/docker/docker/pkg/mount" ) // Unmount the target specified. @@ -13,5 +13,5 @@ func Unmount(target string) error { if err := exec.Command("auplink", target, "flush").Run(); err != nil { logger.WithError(err).Warnf("Couldn't run auplink before unmount %s", target) } - return unix.Unmount(target, 0) + return mount.Unmount(target) } From 5db6e4ad9bff09b428483f785423c62b27a9666c Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 18 Apr 2019 15:55:55 -0700 Subject: [PATCH 24/35] aufs: aufsMount: better errors for unix.Mount() Signed-off-by: Kir Kolyshkin (cherry picked from commit 5873768dbe3be2733874b8cf68cb492817f81a94) Upstream-commit: 4ab3020e8df18b5c12c668df6ef3e21b648ce04b Component: engine --- components/engine/daemon/graphdriver/aufs/aufs.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/components/engine/daemon/graphdriver/aufs/aufs.go b/components/engine/daemon/graphdriver/aufs/aufs.go index 38df774225..5a95ef42e0 100644 --- a/components/engine/daemon/graphdriver/aufs/aufs.go +++ b/components/engine/daemon/graphdriver/aufs/aufs.go @@ -626,6 +626,7 @@ func (a *Driver) aufsMount(ro []string, rw, target, mountLabel string) (err erro } data := label.FormatMountLabel(fmt.Sprintf("%s,%s", string(b[:bp]), opts), mountLabel) if err = unix.Mount("none", target, "aufs", 0, data); err != nil { + err = errors.Wrap(err, "mount target="+target+" data="+data) return } @@ -633,6 +634,7 @@ func (a *Driver) aufsMount(ro []string, rw, target, mountLabel string) (err erro layer := fmt.Sprintf(":%s=ro+wh", ro[index]) data := label.FormatMountLabel(fmt.Sprintf("append%s", layer), mountLabel) if err = unix.Mount("none", target, "aufs", unix.MS_REMOUNT, data); err != nil { + err = errors.Wrap(err, "mount target="+target+" flags=MS_REMOUNT data="+data) return } } From 5ada897229ec476206c06b1ef976d9b39d550260 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 22 Apr 2019 18:34:24 +0000 Subject: [PATCH 25/35] aufs: add lock around mount Apparently there is some kind of race in aufs kernel module code, which leads to the errors like: [98221.158606] aufs au_xino_create2:186:dockerd[25801]: aufs.xino create err -17 [98221.162128] aufs au_xino_set:1229:dockerd[25801]: I/O Error, failed creating xino(-17). [98362.239085] aufs au_xino_create2:186:dockerd[6348]: aufs.xino create err -17 [98362.243860] aufs au_xino_set:1229:dockerd[6348]: I/O Error, failed creating xino(-17). [98373.775380] aufs au_xino_create:767:dockerd[27435]: open /dev/shm/aufs.xino(-17) [98389.015640] aufs au_xino_create2:186:dockerd[26753]: aufs.xino create err -17 [98389.018776] aufs au_xino_set:1229:dockerd[26753]: I/O Error, failed creating xino(-17). [98424.117584] aufs au_xino_create:767:dockerd[27105]: open /dev/shm/aufs.xino(-17) So, we have to have a lock around mount syscall. While at it, don't call the whole Unmount() on an error path, as it leads to bogus error from auplink flush. Signed-off-by: Kir Kolyshkin (cherry picked from commit 5cd62852fa199f272b542d828c8c5d1db427ea53) Upstream-commit: bb4b9fe29eba57ed71d8105ec0e5dacc8e72129e Component: engine --- components/engine/daemon/graphdriver/aufs/aufs.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/components/engine/daemon/graphdriver/aufs/aufs.go b/components/engine/daemon/graphdriver/aufs/aufs.go index 5a95ef42e0..fd2cf58063 100644 --- a/components/engine/daemon/graphdriver/aufs/aufs.go +++ b/components/engine/daemon/graphdriver/aufs/aufs.go @@ -80,6 +80,7 @@ type Driver struct { pathCache map[string]string naiveDiff graphdriver.DiffDriver locker *locker.Locker + mntL sync.Mutex } // Init returns a new AUFS driver. @@ -597,7 +598,7 @@ func (a *Driver) Cleanup() error { func (a *Driver) aufsMount(ro []string, rw, target, mountLabel string) (err error) { defer func() { if err != nil { - Unmount(target) + mount.Unmount(target) } }() @@ -625,7 +626,10 @@ func (a *Driver) aufsMount(ro []string, rw, target, mountLabel string) (err erro opts += ",dirperm1" } data := label.FormatMountLabel(fmt.Sprintf("%s,%s", string(b[:bp]), opts), mountLabel) - if err = unix.Mount("none", target, "aufs", 0, data); err != nil { + a.mntL.Lock() + err = unix.Mount("none", target, "aufs", 0, data) + a.mntL.Unlock() + if err != nil { err = errors.Wrap(err, "mount target="+target+" data="+data) return } @@ -633,7 +637,10 @@ func (a *Driver) aufsMount(ro []string, rw, target, mountLabel string) (err erro for ; index < len(ro); index++ { layer := fmt.Sprintf(":%s=ro+wh", ro[index]) data := label.FormatMountLabel(fmt.Sprintf("append%s", layer), mountLabel) - if err = unix.Mount("none", target, "aufs", unix.MS_REMOUNT, data); err != nil { + a.mntL.Lock() + err = unix.Mount("none", target, "aufs", unix.MS_REMOUNT, data) + a.mntL.Unlock() + if err != nil { err = errors.Wrap(err, "mount target="+target+" flags=MS_REMOUNT data="+data) return } From 3b8f3b67f29aa2b3f697dbba8a9f038eebf6d7f7 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 23 Apr 2019 02:18:17 +0000 Subject: [PATCH 26/35] aufs: optimize lots of layers case In case there are a big number of layers, so that mount data won't fit into a single memory page (4096 bytes on most platforms, which is good enough for about 40 layers, depending on how long graphdriver root path is), we supply additional layers with O_REMOUNT, as described in aufs documentation. Problem is, the current implementation does that one layer at a time (i.e. there is one mount syscall per each additional layer). Optimize the code to supply as many layers as we can fit in one page (basically reusing the same code as for the original mount). Note, per aufs docs, "[a]t remount-time, the options are interpreted in the given order, e.g. left to right" so we should be good. Tested on an image with ~100 layers. Before (35 syscalls): > [pid 22756] 1556919088.686955 mount("none", "/mnt/volume_sfo2_09/docker-aufs/aufs/mnt/a86f8c9dd0ec2486293119c20b0ec026e19bbc4d51332c554f7cf05d777c9866", "aufs", 0, "br:/mnt/volume_sfo2_09/docker-au"...) = 0 <0.000504> > [pid 22756] 1556919088.687643 mount("none", "/mnt/volume_sfo2_09/docker-aufs/aufs/mnt/a86f8c9dd0ec2486293119c20b0ec026e19bbc4d51332c554f7cf05d777c9866", 0xc000c451b0, MS_REMOUNT, "append:/mnt/volume_sfo2_09/docke"...) = 0 <0.000105> > [pid 22756] 1556919088.687851 mount("none", "/mnt/volume_sfo2_09/docker-aufs/aufs/mnt/a86f8c9dd0ec2486293119c20b0ec026e19bbc4d51332c554f7cf05d777c9866", 0xc000c451ba, MS_REMOUNT, "append:/mnt/volume_sfo2_09/docke"...) = 0 <0.000098> > ..... (~30 lines skipped for clarity) > [pid 22756] 1556919088.696182 mount("none", "/mnt/volume_sfo2_09/docker-aufs/aufs/mnt/a86f8c9dd0ec2486293119c20b0ec026e19bbc4d51332c554f7cf05d777c9866", 0xc000c45310, MS_REMOUNT, "append:/mnt/volume_sfo2_09/docke"...) = 0 <0.000266> After (2 syscalls): > [pid 24352] 1556919361.799889 mount("none", "/mnt/volume_sfo2_09/docker-aufs/aufs/mnt/8e7ba189e347a834e99eea4ed568f95b86cec809c227516afdc7c70286ff9a20", "aufs", 0, "br:/mnt/volume_sfo2_09/docker-au"...) = 0 <0.001717> > [pid 24352] 1556919361.801761 mount("none", "/mnt/volume_sfo2_09/docker-aufs/aufs/mnt/8e7ba189e347a834e99eea4ed568f95b86cec809c227516afdc7c70286ff9a20", 0xc000dbecb0, MS_REMOUNT, "append:/mnt/volume_sfo2_09/docke"...) = 0 <0.001358> Signed-off-by: Kir Kolyshkin (cherry picked from commit d58c434bffef76e48bff75ede290937874488dd3) Upstream-commit: 75488521735325e4564e66d9cf9c2b1bc1c2c64b Component: engine --- components/engine/daemon/graphdriver/aufs/aufs.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/components/engine/daemon/graphdriver/aufs/aufs.go b/components/engine/daemon/graphdriver/aufs/aufs.go index fd2cf58063..9a0a6207f0 100644 --- a/components/engine/daemon/graphdriver/aufs/aufs.go +++ b/components/engine/daemon/graphdriver/aufs/aufs.go @@ -634,9 +634,16 @@ func (a *Driver) aufsMount(ro []string, rw, target, mountLabel string) (err erro return } - for ; index < len(ro); index++ { - layer := fmt.Sprintf(":%s=ro+wh", ro[index]) - data := label.FormatMountLabel(fmt.Sprintf("append%s", layer), mountLabel) + for index < len(ro) { + bp = 0 + for ; index < len(ro); index++ { + layer := fmt.Sprintf("append:%s=ro+wh,", ro[index]) + if bp+len(layer) > len(b) { + break + } + bp += copy(b[bp:], layer) + } + data := label.FormatMountLabel(string(b[:bp]), mountLabel) a.mntL.Lock() err = unix.Mount("none", target, "aufs", unix.MS_REMOUNT, data) a.mntL.Unlock() From c70089ebb32fa6c6755f151dd54498c9f0ac5dc0 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 17 Apr 2019 19:56:48 -0700 Subject: [PATCH 27/35] aufs.Cleanup: optimize Do not use filepath.Walk() as there's no requirement to recursively go into every directory under mnt -- a (non-recursive) list of directories in mnt is sufficient. With filepath.Walk(), in case some container will fail to unmount, it'll go through the whole container filesystem which is both excessive and useless. This is similar to commit f1a459229724f5e8e440b49f ("devmapper.shutdown: optimize") While at it, raise the priority of "unmount error" message from debug to a warning. Note we don't have to explicitly add `m` as unmount error (from pkg/mount) will have it. Signed-off-by: Kir Kolyshkin (cherry picked from commit 8fda12c6078ed5c86be0822a7a980c6fbc9220bf) Upstream-commit: b85d4a4f09badf40201ef7fcbd5b930de216190d Component: engine --- .../engine/daemon/graphdriver/aufs/aufs.go | 25 ++++++++----------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/components/engine/daemon/graphdriver/aufs/aufs.go b/components/engine/daemon/graphdriver/aufs/aufs.go index 9a0a6207f0..bbd19a82b0 100644 --- a/components/engine/daemon/graphdriver/aufs/aufs.go +++ b/components/engine/daemon/graphdriver/aufs/aufs.go @@ -573,23 +573,20 @@ func (a *Driver) mounted(mountpoint string) (bool, error) { // Cleanup aufs and unmount all mountpoints func (a *Driver) Cleanup() error { - var dirs []string - if err := filepath.Walk(a.mntPath(), func(path string, info os.FileInfo, err error) error { - if err != nil { - return err - } - if !info.IsDir() { - return nil - } - dirs = append(dirs, path) - return nil - }); err != nil { - return err + dir := a.mntPath() + files, err := ioutil.ReadDir(dir) + if err != nil { + return errors.Wrap(err, "aufs readdir error") } + for _, f := range files { + if !f.IsDir() { + continue + } + + m := path.Join(dir, f.Name()) - for _, m := range dirs { if err := a.unmount(m); err != nil { - logger.Debugf("error unmounting %s: %s", m, err) + logger.WithError(err).WithField("method", "Cleanup()").Warn() } } return mount.RecursiveUnmount(a.root) From 00419438192534eb6f5aa8ae458a225a536c46e4 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Fri, 10 May 2019 17:19:49 -0700 Subject: [PATCH 28/35] aufs: retry auplink flush Running a bundled aufs benchmark sometimes results in this warning: > WARN[0001] Couldn't run auplink before unmount /tmp/aufs-tests/aufs/mnt/XXXXX error="exit status 22" storage-driver=aufs If we take a look at what aulink utility produces on stderr, we'll see: > auplink:proc_mnt.c:96: /tmp/aufs-tests/aufs/mnt/XXXXX: Invalid argument and auplink exits with exit code of 22 (EINVAL). Looking into auplink source code, what happens is it tries to find a record in /proc/self/mounts corresponding to the mount point (by using setmntent()/getmntent_r() glibc functions), and it fails. Some manual testing, as well as runtime testing with lots of printf added on mount/unmount, as well as calls to check the superblock fs magic on mount point (as in graphdriver.Mounted(graphdriver.FsMagicAufs, target) confirmed that this record is in fact there, but sometimes auplink can't find it. I was also able to reproduce the same error (inability to find a mount in /proc/self/mounts that should definitely be there) using a small C program, mocking what `auplink` does: ```c #include #include #include #include #include int main(int argc, char **argv) { FILE *fp; struct mntent m, *p; char a[4096]; char buf[4096 + 1024]; int found =0, lines = 0; if (argc != 2) { fprintf(stderr, "Usage: %s \n", argv[0]); exit(1); } fp = setmntent("/proc/self/mounts", "r"); if (!fp) { err(1, "setmntent"); } setvbuf(fp, a, _IOLBF, sizeof(a)); while ((p = getmntent_r(fp, &m, buf, sizeof(buf)))) { lines++; if (!strcmp(p->mnt_dir, argv[1])) { found++; } } printf("found %d entries for %s (%d lines seen)\n", found, argv[1], lines); return !found; } ``` I have also wrote a few other C proggies -- one that reads /proc/self/mounts directly, one that reads /proc/self/mountinfo instead. They are also prone to the same occasional error. It is not perfectly clear why this happens, but so far my best theory is when a lot of mounts/unmounts happen in parallel with reading contents of /proc/self/mounts, sometimes the kernel fails to provide continuity (i.e. it skips some part of file or mixes it up in some other way). In other words, this is a kernel bug (which is probably hard to fix unless some other interface to get a mount entry is added). Now, there is no real fix, and a workaround I was able to come up with is to retry when we got EINVAL. It usually works on the second attempt, although I've once seen it took two attempts to go through. Signed-off-by: Kir Kolyshkin (cherry picked from commit ae431b10a9508e2bf3b1782e9d435855e3e13977) Upstream-commit: c303e63ca6c3d25d29b8992451898734fa6e4e7c Component: engine --- .../engine/daemon/graphdriver/aufs/mount.go | 28 +++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/components/engine/daemon/graphdriver/aufs/mount.go b/components/engine/daemon/graphdriver/aufs/mount.go index 4e85cf235d..fc20a5eca6 100644 --- a/components/engine/daemon/graphdriver/aufs/mount.go +++ b/components/engine/daemon/graphdriver/aufs/mount.go @@ -4,14 +4,38 @@ package aufs // import "github.com/docker/docker/daemon/graphdriver/aufs" import ( "os/exec" + "syscall" "github.com/docker/docker/pkg/mount" ) // Unmount the target specified. func Unmount(target string) error { - if err := exec.Command("auplink", target, "flush").Run(); err != nil { - logger.WithError(err).Warnf("Couldn't run auplink before unmount %s", target) + const ( + EINVAL = 22 // if auplink returns this, + retries = 3 // retry a few times + ) + + for i := 0; ; i++ { + out, err := exec.Command("auplink", target, "flush").CombinedOutput() + if err == nil { + break + } + rc := 0 + if exiterr, ok := err.(*exec.ExitError); ok { + if status, ok := exiterr.Sys().(syscall.WaitStatus); ok { + rc = status.ExitStatus() + } + } + if i >= retries || rc != EINVAL { + logger.WithError(err).WithField("method", "Unmount").Warnf("auplink flush failed: %s", out) + break + } + // auplink failed to find target in /proc/self/mounts because + // kernel can't guarantee continuity while reading from it + // while mounts table is being changed + logger.Debugf("auplink flush error (retrying %d/%d): %s", i+1, retries, out) } + return mount.Unmount(target) } From 2271e6653d3c97bea2eddb8830b38b282c7f0422 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 5 Jun 2019 17:02:44 +0200 Subject: [PATCH 29/35] Harden TestPsListContainersFilterExited This test runs on a daemon also used by other tests so make sure we don't get failures if another test doesn't cleanup or is running in parallel. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 915acffdb4cf95b934dd9872c1f54ea4487819b7) Signed-off-by: Sebastiaan van Stijn Upstream-commit: 05599804157a38db2eda84d3e8060a879b755ab7 Component: engine --- .../integration-cli/docker_cli_ps_test.go | 25 ++++++++----------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/components/engine/integration-cli/docker_cli_ps_test.go b/components/engine/integration-cli/docker_cli_ps_test.go index f34cd0f8a8..cab75f4696 100644 --- a/components/engine/integration-cli/docker_cli_ps_test.go +++ b/components/engine/integration-cli/docker_cli_ps_test.go @@ -439,15 +439,11 @@ func (s *DockerSuite) TestPsListContainersFilterLabel(c *check.C) { func (s *DockerSuite) TestPsListContainersFilterExited(c *check.C) { runSleepingContainer(c, "--name=sleep") - dockerCmd(c, "run", "--name", "zero1", "busybox", "true") - firstZero := getIDByName(c, "zero1") - - dockerCmd(c, "run", "--name", "zero2", "busybox", "true") - secondZero := getIDByName(c, "zero2") + firstZero, _ := dockerCmd(c, "run", "-d", "busybox", "true") + secondZero, _ := dockerCmd(c, "run", "-d", "busybox", "true") out, _, err := dockerCmdWithError("run", "--name", "nonzero1", "busybox", "false") c.Assert(err, checker.NotNil, check.Commentf("Should fail.", out, err)) - firstNonZero := getIDByName(c, "nonzero1") out, _, err = dockerCmdWithError("run", "--name", "nonzero2", "busybox", "false") @@ -456,17 +452,16 @@ func (s *DockerSuite) TestPsListContainersFilterExited(c *check.C) { // filter containers by exited=0 out, _ = dockerCmd(c, "ps", "-a", "-q", "--no-trunc", "--filter=exited=0") - ids := strings.Split(strings.TrimSpace(out), "\n") - c.Assert(ids, checker.HasLen, 2, check.Commentf("Should be 2 zero exited containers got %d: %s", len(ids), out)) - c.Assert(ids[0], checker.Equals, secondZero, check.Commentf("First in list should be %q, got %q", secondZero, ids[0])) - c.Assert(ids[1], checker.Equals, firstZero, check.Commentf("Second in list should be %q, got %q", firstZero, ids[1])) + c.Assert(out, checker.Contains, strings.TrimSpace(firstZero)) + c.Assert(out, checker.Contains, strings.TrimSpace(secondZero)) + c.Assert(out, checker.Not(checker.Contains), strings.TrimSpace(firstNonZero)) + c.Assert(out, checker.Not(checker.Contains), strings.TrimSpace(secondNonZero)) out, _ = dockerCmd(c, "ps", "-a", "-q", "--no-trunc", "--filter=exited=1") - ids = strings.Split(strings.TrimSpace(out), "\n") - c.Assert(ids, checker.HasLen, 2, check.Commentf("Should be 2 zero exited containers got %d", len(ids))) - c.Assert(ids[0], checker.Equals, secondNonZero, check.Commentf("First in list should be %q, got %q", secondNonZero, ids[0])) - c.Assert(ids[1], checker.Equals, firstNonZero, check.Commentf("Second in list should be %q, got %q", firstNonZero, ids[1])) - + c.Assert(out, checker.Contains, strings.TrimSpace(firstNonZero)) + c.Assert(out, checker.Contains, strings.TrimSpace(secondNonZero)) + c.Assert(out, checker.Not(checker.Contains), strings.TrimSpace(firstZero)) + c.Assert(out, checker.Not(checker.Contains), strings.TrimSpace(secondZero)) } func (s *DockerSuite) TestPsRightTagName(c *check.C) { From f29ee03a99a25c96b387b62459ac9d7fa13733ca Mon Sep 17 00:00:00 2001 From: Dominic Tubach Date: Thu, 23 May 2019 15:14:34 +0200 Subject: [PATCH 30/35] API: Change type of RemotrAddrs to array of strings in operation SwarmJoin Signed-off-by: Dominic Tubach (cherry picked from commit d5f6bdb027596b44244a6ce50555664b3a5ee4a7) Signed-off-by: Sebastiaan van Stijn Upstream-commit: d359834555628f67693cdf7eda246835b659a41c Component: engine --- components/engine/api/swagger.yaml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/components/engine/api/swagger.yaml b/components/engine/api/swagger.yaml index ca9d29e021..6aeb711e59 100644 --- a/components/engine/api/swagger.yaml +++ b/components/engine/api/swagger.yaml @@ -8932,7 +8932,9 @@ paths: type: "string" RemoteAddrs: description: "Addresses of manager nodes already participating in the swarm." - type: "string" + type: "array" + items: + type: "string" JoinToken: description: "Secret token for joining this swarm." type: "string" From 02856b256b61b6a78e6b06490bb6f39510656fb4 Mon Sep 17 00:00:00 2001 From: Dominic Tubach Date: Thu, 23 May 2019 16:03:48 +0200 Subject: [PATCH 31/35] API: Move "x-nullable: true" from type PortBinding to type PortMap Currently the API spec would allow `"443/tcp": [null]`, but what should be allowed is `"443/tcp": null` Signed-off-by: Dominic Tubach (cherry picked from commit 32b5d296ea5d392c28affe2854b9d4201166bc27) Signed-off-by: Sebastiaan van Stijn Upstream-commit: 3ee1e060fc9a31844064035cc3e8b76fddb8b341 Component: engine --- components/engine/api/swagger.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/engine/api/swagger.yaml b/components/engine/api/swagger.yaml index 6aeb711e59..f8adfe9feb 100644 --- a/components/engine/api/swagger.yaml +++ b/components/engine/api/swagger.yaml @@ -1082,6 +1082,7 @@ definitions: type: "object" additionalProperties: type: "array" + x-nullable: true items: $ref: "#/definitions/PortBinding" example: @@ -1106,7 +1107,6 @@ definitions: PortBinding represents a binding between a host IP address and a host port. type: "object" - x-nullable: true properties: HostIp: description: "Host IP address that the container's port is mapped to." From 3e919def477cecbc408669b455c5cce9d80c6086 Mon Sep 17 00:00:00 2001 From: Adam Dobrawy Date: Thu, 23 May 2019 19:11:46 +0200 Subject: [PATCH 32/35] Update docs to remove restriction of tty resize Signed-off-by: Adam Dobrawy (cherry picked from commit 4898f493d86129684c29d6cdfc6f65b49eb2ed29) Signed-off-by: Sebastiaan van Stijn Upstream-commit: 2d1aa033a38e6a8b44b4a55a9285ebd80908e725 Component: engine --- components/engine/api/swagger.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/engine/api/swagger.yaml b/components/engine/api/swagger.yaml index f8adfe9feb..988f472dd8 100644 --- a/components/engine/api/swagger.yaml +++ b/components/engine/api/swagger.yaml @@ -5351,7 +5351,7 @@ paths: /containers/{id}/resize: post: summary: "Resize a container TTY" - description: "Resize the TTY for a container. You must restart the container for the resize to take effect." + description: "Resize the TTY for a container." operationId: "ContainerResize" consumes: - "application/octet-stream" From 861c5d670eb47348e367076a37ae979dfa5343d5 Mon Sep 17 00:00:00 2001 From: Dominic Tubach Date: Tue, 21 May 2019 17:09:18 +0200 Subject: [PATCH 33/35] API: Set format of body parameter in operation PutContainerArchive to "binary" Signed-off-by: Dominic Tubach (cherry picked from commit fa6f63e79b60d971460ee1fb8d449bdccc66cdfe) Signed-off-by: Sebastiaan van Stijn Upstream-commit: c3bf976a20ae18b628247088ff9935ce615337b8 Component: engine --- components/engine/api/swagger.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/components/engine/api/swagger.yaml b/components/engine/api/swagger.yaml index 988f472dd8..5faabdc4a0 100644 --- a/components/engine/api/swagger.yaml +++ b/components/engine/api/swagger.yaml @@ -6111,6 +6111,7 @@ paths: description: "The input stream must be a tar archive compressed with one of the following algorithms: identity (no compression), gzip, bzip2, xz." schema: type: "string" + format: "binary" tags: ["Container"] /containers/prune: post: From 223d3e0c756314bcf079eb68f981224e9b23cce6 Mon Sep 17 00:00:00 2001 From: zhangyue Date: Wed, 29 May 2019 16:33:56 +0800 Subject: [PATCH 34/35] fix: fix lack of copyUIDGID in swagger.yaml Signed-off-by: Zhang Yue Signed-off-by: zhangyue (cherry picked from commit a4f828cb8905f42c8b8975ce88e4d7aa8cd9bf74) Signed-off-by: Sebastiaan van Stijn Upstream-commit: 705cc95eb1247c1983c39477f2ecf5cb4eb5edbb Component: engine --- components/engine/api/swagger.yaml | 4 ++++ components/engine/docs/api/version-history.md | 1 + 2 files changed, 5 insertions(+) diff --git a/components/engine/api/swagger.yaml b/components/engine/api/swagger.yaml index 5faabdc4a0..1cf310d689 100644 --- a/components/engine/api/swagger.yaml +++ b/components/engine/api/swagger.yaml @@ -6105,6 +6105,10 @@ paths: in: "query" description: "If “1”, “true”, or “True” then it will be an error if unpacking the given content would cause an existing directory to be replaced with a non-directory and vice versa." type: "string" + - name: "copyUIDGID" + in: "query" + description: "If “1”, “true”, then it will copy UID/GID maps to the dest file or dir" + type: "string" - name: "inputStream" in: "body" required: true diff --git a/components/engine/docs/api/version-history.md b/components/engine/docs/api/version-history.md index 6f0083e90e..d98c16943a 100644 --- a/components/engine/docs/api/version-history.md +++ b/components/engine/docs/api/version-history.md @@ -159,6 +159,7 @@ keywords: "API, Docker, rcli, REST, documentation" * `GET /events` now supports service, node and secret events which are emitted when users create, update and remove service, node and secret * `GET /events` now supports network remove event which is emitted when users remove a swarm scoped network * `GET /events` now supports a filter type `scope` in which supported value could be swarm and local +* `PUT /containers/(name)/archive` now accepts a `copyUIDGID` parameter to allow copy UID/GID maps to dest file or dir. ## v1.29 API changes From c82a61bcb36be91d2510c20760d1914bf5cb5ec4 Mon Sep 17 00:00:00 2001 From: Olli Janatuinen Date: Tue, 14 May 2019 23:01:12 +0300 Subject: [PATCH 35/35] Enable integrations API tests for Windows CI Signed-off-by: Olli Janatuinen (cherry picked from commit 2f22247cad9237d255a4b541a974705802abdad8) Signed-off-by: Sebastiaan van Stijn Upstream-commit: 69503ef832fed199e28ccfd731281a4e439a8eeb Component: engine --- components/engine/hack/ci/windows.ps1 | 23 ++++++++-- components/engine/hack/make.ps1 | 44 ++++++++++++++++++- .../internal/container/container.go | 7 ++- .../engine/integration/volume/volume_test.go | 4 ++ 4 files changed, 71 insertions(+), 7 deletions(-) diff --git a/components/engine/hack/ci/windows.ps1 b/components/engine/hack/ci/windows.ps1 index c2c937f4cb..ea984bb780 100644 --- a/components/engine/hack/ci/windows.ps1 +++ b/components/engine/hack/ci/windows.ps1 @@ -119,6 +119,7 @@ $FinallyColour="Cyan" #$env:INTEGRATION_IN_CONTAINER="yes" #$env:WINDOWS_BASE_IMAGE="" #$env:SKIP_COPY_GO="yes" +#$env:INTEGRATION_TESTFLAGS="-test.v" Function Nuke-Everything { $ErrorActionPreference = 'SilentlyContinue' @@ -825,18 +826,32 @@ Try { docker ` "`$env`:PATH`='c`:\target;'+`$env:PATH`; `$env:DOCKER_HOST`='tcp`://'+(ipconfig | select -last 1).Substring(39)+'`:2357'; c:\target\runIntegrationCLI.ps1" | Out-Host } ) } else { - Write-Host -ForegroundColor Green "INFO: Integration tests being run from the host:" - Set-Location "$env:SOURCES_DRIVE`:\$env:SOURCES_SUBDIR\src\github.com\docker\docker\integration-cli" $env:DOCKER_HOST=$DASHH_CUT $env:PATH="$env:TEMP\binary;$env:PATH;" # Force to use the test binaries, not the host ones. - Write-Host -ForegroundColor Green "INFO: $c" Write-Host -ForegroundColor Green "INFO: DOCKER_HOST at $DASHH_CUT" + + $ErrorActionPreference = "SilentlyContinue" + Write-Host -ForegroundColor Cyan "INFO: Integration API tests being run from the host:" + if (!($env:INTEGRATION_TESTFLAGS)) { + $env:INTEGRATION_TESTFLAGS = "-test.v" + } + Set-Location "$env:SOURCES_DRIVE`:\$env:SOURCES_SUBDIR\src\github.com\docker\docker" + $start=(Get-Date); Invoke-Expression ".\hack\make.ps1 -TestIntegration"; $Duration=New-Timespan -Start $start -End (Get-Date) + $ErrorActionPreference = "Stop" + if (-not($LastExitCode -eq 0)) { + Throw "ERROR: Integration API tests failed at $(Get-Date). Duration`:$Duration" + } + + $ErrorActionPreference = "SilentlyContinue" + Write-Host -ForegroundColor Green "INFO: Integration CLI tests being run from the host:" + Write-Host -ForegroundColor Green "INFO: $c" + Set-Location "$env:SOURCES_DRIVE`:\$env:SOURCES_SUBDIR\src\github.com\docker\docker\integration-cli" # Explicit to not use measure-command otherwise don't get output as it goes $start=(Get-Date); Invoke-Expression $c; $Duration=New-Timespan -Start $start -End (Get-Date) } $ErrorActionPreference = "Stop" if (-not($LastExitCode -eq 0)) { - Throw "ERROR: Integration tests failed at $(Get-Date). Duration`:$Duration" + Throw "ERROR: Integration CLI tests failed at $(Get-Date). Duration`:$Duration" } Write-Host -ForegroundColor Green "INFO: Integration tests ended at $(Get-Date). Duration`:$Duration" } else { diff --git a/components/engine/hack/make.ps1 b/components/engine/hack/make.ps1 index 6221e364d6..e752febe0c 100644 --- a/components/engine/hack/make.ps1 +++ b/components/engine/hack/make.ps1 @@ -60,6 +60,9 @@ .PARAMETER TestUnit Runs unit tests. +.PARAMETER TestIntegration + Runs integration tests. + .PARAMETER All Runs everything this script knows about that can run in a container. @@ -84,6 +87,7 @@ param( [Parameter(Mandatory=$False)][switch]$PkgImports, [Parameter(Mandatory=$False)][switch]$GoFormat, [Parameter(Mandatory=$False)][switch]$TestUnit, + [Parameter(Mandatory=$False)][switch]$TestIntegration, [Parameter(Mandatory=$False)][switch]$All ) @@ -320,6 +324,39 @@ Function Run-UnitTests() { if ($LASTEXITCODE -ne 0) { Throw "Unit tests failed" } } +# Run the integration tests +Function Run-IntegrationTests() { + $env:DOCKER_INTEGRATION_DAEMON_DEST = $root + "\bundles\tmp" + $dirs = Get-ChildItem -Path integration -Directory -Recurse + $integration_api_dirs = @() + ForEach($dir in $dirs) { + $RelativePath = "." + $dir.FullName -replace "$($PWD.Path -replace "\\","\\")","" + If ($RelativePath -notmatch '(^.\\integration($|\\internal)|\\testdata)') { + $integration_api_dirs += $dir + Write-Host "Building test suite binary $RelativePath" + go test -c -o "$RelativePath\test.exe" $RelativePath + } + } + + ForEach($dir in $integration_api_dirs) { + Set-Location $dir.FullName + Write-Host "Running $($PWD.Path)" + $pinfo = New-Object System.Diagnostics.ProcessStartInfo + $pinfo.FileName = "$($PWD.Path)\test.exe" + $pinfo.RedirectStandardError = $true + $pinfo.UseShellExecute = $false + $pinfo.Arguments = $env:INTEGRATION_TESTFLAGS + $p = New-Object System.Diagnostics.Process + $p.StartInfo = $pinfo + $p.Start() | Out-Null + $p.WaitForExit() + $err = $p.StandardError.ReadToEnd() + if (($LASTEXITCODE -ne 0) -and ($err -notlike "*warning: no tests to run*")) { + Throw "Integration tests failed: $err" + } + } +} + # Start of main code. Try { Write-Host -ForegroundColor Cyan "INFO: make.ps1 starting at $(Get-Date)" @@ -331,13 +368,13 @@ Try { # Handle the "-All" shortcut to turn on all things we can handle. # Note we expressly only include the items which can run in a container - the validations tests cannot # as they require the .git directory which is excluded from the image by .dockerignore - if ($All) { $Client=$True; $Daemon=$True; $TestUnit=$True } + if ($All) { $Client=$True; $Daemon=$True; $TestUnit=$True; } # Handle the "-Binary" shortcut to build both client and daemon. if ($Binary) { $Client = $True; $Daemon = $True } # Default to building the daemon if not asked for anything explicitly. - if (-not($Client) -and -not($Daemon) -and -not($DCO) -and -not($PkgImports) -and -not($GoFormat) -and -not($TestUnit)) { $Daemon=$True } + if (-not($Client) -and -not($Daemon) -and -not($DCO) -and -not($PkgImports) -and -not($GoFormat) -and -not($TestUnit) -and -not($TestIntegration)) { $Daemon=$True } # Verify git is installed if ($(Get-Command git -ErrorAction SilentlyContinue) -eq $nil) { Throw "Git does not appear to be installed" } @@ -423,6 +460,9 @@ Try { # Run unit tests if ($TestUnit) { Run-UnitTests } + # Run integration tests + if ($TestIntegration) { Run-IntegrationTests } + # Gratuitous ASCII art. if ($Daemon -or $Client) { Write-Host diff --git a/components/engine/integration/internal/container/container.go b/components/engine/integration/internal/container/container.go index 20ad774242..85e6a24fe7 100644 --- a/components/engine/integration/internal/container/container.go +++ b/components/engine/integration/internal/container/container.go @@ -2,6 +2,7 @@ package container import ( "context" + "runtime" "testing" "github.com/docker/docker/api/types" @@ -24,10 +25,14 @@ type TestContainerConfig struct { // nolint: golint func Create(t *testing.T, ctx context.Context, client client.APIClient, ops ...func(*TestContainerConfig)) string { // nolint: golint t.Helper() + cmd := []string{"top"} + if runtime.GOOS == "windows" { + cmd = []string{"sleep", "240"} + } config := &TestContainerConfig{ Config: &container.Config{ Image: "busybox", - Cmd: []string{"top"}, + Cmd: cmd, }, HostConfig: &container.HostConfig{}, NetworkingConfig: &network.NetworkingConfig{}, diff --git a/components/engine/integration/volume/volume_test.go b/components/engine/integration/volume/volume_test.go index 4ee109e675..3b2babad19 100644 --- a/components/engine/integration/volume/volume_test.go +++ b/components/engine/integration/volume/volume_test.go @@ -26,6 +26,10 @@ func TestVolumesCreateAndList(t *testing.T) { ctx := context.Background() name := t.Name() + // Windows file system is case insensitive + if testEnv.OSType == "windows" { + name = strings.ToLower(name) + } vol, err := client.VolumeCreate(ctx, volumetypes.VolumeCreateBody{ Name: name, })