Dang. Because I touched integration-cli (moved some tests) CI is seeing them as new tests and rejects validation.

Specifically, it checks:

$ git diff origin/master...HEAD --diff-filter=ACMR --unified=0 -- 'integration-cli/*_api_*.go' 'integration-cli/*_cli_*.go' | grep -E '^\+func (.*) Test'
+func (s *DockerSuite) TestRunVolumesMountedAsShared(c *testing.T) {
+func (s *DockerSuite) TestRunVolumesMountedAsSlave(c *testing.T) {

but there's also

$ git diff origin/master...HEAD --diff-filter=ACMR --unified=0 -- 'integration-cli/*_api_*.go' 'integration-cli/*_cli_*.go' | grep -E '^\-func (.*) Test'
-func (s *DockerSuite) TestRunVolumesMountedAsShared(c *testing.T) {
-func (s *DockerSuite) TestRunVolumesMountedAsSlave(c *testing.T) {

because I just moved those tests.

I'm not sure how to bypass this, short of changing the validation to check that no net new tests have been added, or something. And it means the rest of the validation pipeline doesn't run. -_-

We should be ok to ignore the RS1 failures (RS1 is not running by default, I triggered it manually to see if there's no huge problem), and doesn't look like a regression caused by this PR;

https://ci-next.docker.com/public/blue/rest/organizations/jenkins/pipelines/moby/branches/PR-41638/runs/4/nodes/316/log/?start=0

--- FAIL: TestBuildWithRemoveAndForceRemove (0.02s)
    --- PASS: TestBuildWithRemoveAndForceRemove/failed_build_with_no_removal (44.44s)
    --- FAIL: TestBuildWithRemoveAndForceRemove/successful_build_with_remove (44.77s)
        build_test.go:113: assertion failed: 0 (c.numberOfIntermediateContainers int) != 1 (int): Expected 0 remaining intermediate containers, got 1
    --- FAIL: TestBuildWithRemoveAndForceRemove/successful_build_with_remove_and_force_remove (45.41s)
        build_test.go:113: assertion failed: 0 (c.numberOfIntermediateContainers int) != 1 (int): Expected 0 remaining intermediate containers, got 1
    --- FAIL: TestBuildWithRemoveAndForceRemove/successful_build_with_no_removal (45.41s)
        build_test.go:113: assertion failed: 0 (c.numberOfIntermediateContainers int) != 1 (int): Expected 0 remaining intermediate containers, got 1
    --- PASS: TestBuildWithRemoveAndForceRemove/failed_build_with_remove (26.78s)
    --- PASS: TestBuildWithRemoveAndForceRemove/failed_build_with_remove_and_force_remove (26.59s)

Same test passes on RS5; https://ci-next.docker.com/public/blue/rest/organizations/jenkins/pipelines/moby/branches/PR-41638/runs/4/nodes/286/log/?start=0

--- PASS: TestBuildWithRemoveAndForceRemove (0.02s)
    --- PASS: TestBuildWithRemoveAndForceRemove/failed_build_with_no_removal (17.32s)
    --- PASS: TestBuildWithRemoveAndForceRemove/successful_build_with_no_removal (17.68s)
    --- PASS: TestBuildWithRemoveAndForceRemove/failed_build_with_remove_and_force_remove (17.93s)
    --- PASS: TestBuildWithRemoveAndForceRemove/failed_build_with_remove (17.94s)
    --- PASS: TestBuildWithRemoveAndForceRemove/successful_build_with_remove_and_force_remove (9.07s)
    --- PASS: TestBuildWithRemoveAndForceRemove/successful_build_with_remove (8.88s)

Unit tests passed on both RS1 and RS5;

DONE 1768 tests, 45 skipped in 287.554s
INFO: make.ps1 ended at 11/05/2020 19:45:45
INFO: Unit tests results(bundles\junit-report-unit-tests.xml) exist. pwd=D:\gopath\src\github.com\docker\docker
INFO: Unit tests ended at 11/05/2020 19:45:48. Duration:00:05:12.1634473
INFO: Building busybox
Sending build context to Docker daemon   5.12kB


DONE 1768 tests, 45 skipped in 236.483s
INFO: make.ps1 ended at 11/05/2020 19:35:13
INFO: Unit tests results(bundles\junit-report-unit-tests.xml) exist. pwd=D:\gopath\src\github.com\docker\docker
INFO: Unit tests ended at 11/05/2020 19:35:15. Duration:00:04:16.1988449
INFO: Building busybox
Sending build context to Docker daemon   5.12kB

And integration tests on RS5;

DONE 1094 tests, 547 skipped in 2761.701s
INFO: Integration tests ended at 11/05/2020 20:28:27. Duration:00:46:01.7842500
INFO: Docker info of the daemon under test at end of run

I manually ran the docker-py tests;

= 375 passed, 5 skipped, 1 deselected, 4 xfailed, 4 xpassed in 484.38 seconds ==
---> Making bundle: .integration-daemon-stop (in bundles/test-docker-py)

And cross;

root@3b8fca845230:/go/src/github.com/docker/docker# hack/make.sh cross

Removing bundles/

---> Making bundle: cross (in bundles/cross)
Cross building: bundles/cross/linux/amd64
Building: bundles/cross/linux/amd64/dockerd-dev
GOOS="linux" GOARCH="amd64" GOARM=""
Created binary: bundles/cross/linux/amd64/dockerd-dev
Copying nested executables into bundles/cross/linux/amd64
Cross building: bundles/cross/windows/amd64
Building: bundles/cross/windows/amd64/dockerd-dev.exe
GOOS="windows" GOARCH="amd64" GOARM=""
Created binary: bundles/cross/windows/amd64/dockerd-dev.exe
Cloning into '/go/src/github.com/docker/windows-container-utility'...
remote: Enumerating objects: 2, done.
remote: Counting objects: 100% (2/2), done.
remote: Compressing objects: 100% (2/2), done.
remote: Total 23 (delta 0), reused 0 (delta 0), pack-reused 21
Unpacking objects: 100% (23/23), done.
Building: bundles/cross/windows/amd64/containerutility.exe

Dang. Because I touched integration-cli (moved some tests) CI is seeing them as new tests and rejects validation.

So what we did on previous occasions, is to push a temporary commit to disable that check, and revert afterwards in a follow-up PR.

See my above comment; I tried the important tests that were skipped locally, and I'd be comfortable merging as-is; let me run CI once more to see if the RS1 failure is something to look into, or just some race condition (as RS1 is generally slower)

I'm happy to merge as-is. Once this lands, I'll be rebasing #41636 onto it, and if any later validation errors do show up, I will see them there and follow up ASAP.

On that note, did 20.10 drop support for RS1? I thought I read that somewhere, but am not sure where off-hand. No, that was Docker Desktop for Windows dropping RS2, for reasons unrelated to Docker Engine.

Currently, Docker doesn't publish Windows Server packages (they used to be part of Docker Enterprise), only Docker Desktop (which I know some people install on Windows Server); bringing back Windows packages is being discussed, but no ETA yet. We might drop Windows 2016 ("rs1") at that point as well (or at least; it wouldn't be a main point of focus), as it's ending it's shelve-life anyway, and overall experience is so much worse than newer versions of Windows.

Or we could move the tests to integration/ 😄
Or just get rid of the check on integration-cli altogether.

I think the check is useful if we don't want the integration-cli to be added to(and we did miss that on occasion); it should be easier to override though.

Moving the test would be ok of course; point is that currently, master is broken, and I'd love to see that fixed (and not put the burden of additional work on @TBBle)

I am in no way committing to do this, but for my general understanding, it looks like those two tests would be best moved to https://github.com/moby/moby/blob/master/integration/container/mounts_linux_test.go?

Edit: I decided to give the move a try, it looked like a pretty simple translation. We'll see if CI agrees with me.

Edit: Looks good. I can see the moved tests passing in CI, and the validation pipeline passed, so I think I'm clear. Might be worth someone who knows what they're doing making sure I didn't do the test-migration wrong in 219557e and accidentally add a tautology.

The only oddity is the shared-mount setup. The mount utility generates separate mount syscalls for each propagation flag, in order, while if we pass multiple propagation flags to mount.Mount, it unifies them into a single mount syscall, which fails since private and shared are mutually-exclusive, I think. I assume we're trying to create a mount namespace and prevent any propagation events leaking out of the namespace although with a bind-mount rather than a volume-mount. Anyway, I just kept the syscall-level behaviour the same as the CLI tests had.

Edit: Dang. Rootless failed.

[2020-11-07T08:23:08.121Z] === FAIL: amd64.integration.container TestContainerVolumesMountedAsShared (0.11s)

[2020-11-07T08:23:08.121Z]     mounts_linux_test.go:312: assertion failed: error is not nil: Error response from daemon: path /tmp/volume-source-203284251 is mounted on /tmp but it is not a shared mount

[2020-11-07T08:23:08.121Z] 

[2020-11-07T08:23:08.121Z] === FAIL: amd64.integration.container TestContainerVolumesMountedAsSlave (0.09s)

[2020-11-07T08:23:08.121Z]     mounts_linux_test.go:368: assertion failed: error is not nil: Error response from daemon: path /tmp/volume-source-531853758 is mounted on /tmp but it is not a shared or slave mount

On examination, Rootless doesn't run the Integration-CLI test suite, so maybe this test is invalid under rootless...

Edit: Ah, the other user of mount.Mount in this file already skips Rootless, because of rootless-containers/rootlesskit#97. That issue has been resolved, but for whatever reason our CI system has not yet applied the new --propagation=rslave option, assuming that would unblock the various tests skipped for this issue. I guess the --propagation=rslave flag in dockerd-rootless.sh wasn't sufficient to fix that issue after all.

Final edit: CI Passed except "Build e2e image", which passed on the previous build. The only change after that was disabling the two rewritten tests on rootless and rebasing onto master, so I think it's good-to-go.

Post-final edit: After rebase, this time it's the ppc64-le build-dev-image that fails with some kind of network timeout on CI. Definitely not a problem caused by this PR.