daemon/restart: Don't mount with c8d snapshotters#47093
daemon/restart: Don't mount with c8d snapshotters#47093vvoland wants to merge 1 commit intomoby:masterfrom
Conversation
|
Looks like this helped these two: -=== FAIL: github.com/docker/docker/integration-cli TestDockerCLIRestartSuite/TestRestartContainerwithRestartPolicy (12.24s)
- docker_cli_restart_test.go:293: assertion failed:
- Command: D:\a\moby\moby\out\docker.exe restart 1bc60617e320cd5c23a4c90fbc01760b0cf4458f75c4b5f657ab7688cf558803
- ExitCode: 1
- Error: exit status 1
- Stdout:
- Stderr: Error response from daemon: Cannot restart container 1bc60617e320cd5c23a4c90fbc01760b0cf4458f75c4b5f657ab7688cf558803: failed to mount C:\Users\runneradmin\AppData\Local\Temp\moby-root\rootfs\windows\1bc60617e320cd5c23a4c90fbc01760b0cf4458f75c4b5f657ab7688cf558803: failed to activate layer C:\Users\RUNNER~1\AppData\Local\Temp\ctn-root\io.containerd.snapshotter.v1.windows\snapshots\104: hcsshim::ActivateLayer failed in Win32: The process cannot access the file because it is being used by another process. (0x20)
-
-
- Failures:
- ExitCode was 1 expected 0
- Expected no error
-
-=== FAIL: github.com/docker/docker/integration-cli TestDockerCLIRestartSuite/TestRestartStoppedContainer (3.24s)
- docker_cli_restart_test.go:38: assertion failed:
- Command: D:\a\moby\moby\out\docker.exe restart 0a31f0c99e4109084b2970acaedae4335c3b71d93edc8de235217ee4558fcded
- ExitCode: 1
- Error: exit status 1
- Stdout:
- Stderr: Error response from daemon: Cannot restart container 0a31f0c99e4109084b2970acaedae4335c3b71d93edc8de235217ee4558fcded: failed to mount C:\Users\runneradmin\AppData\Local\Temp\moby-root\rootfs\windows\0a31f0c99e4109084b2970acaedae4335c3b71d93edc8de235217ee4558fcded: failed to activate layer C:\Users\RUNNER~1\AppData\Local\Temp\ctn-root\io.containerd.snapshotter.v1.windows\snapshots\120: hcsshim::ActivateLayer failed in Win32: The process cannot access the file because it is being used by another process. (0x20)
-
-
- Failures:
- ExitCode was 1 expected 0
- Expected no error
-
-=== FAIL: github.com/docker/docker/integration-cli TestDockerCLIRestartSuite (39.97s)
+=== RUN TestDockerCLIRestartSuite/TestRestartContainerwithRestartPolicy
+--- PASS: TestDockerCLIRestartSuite/TestRestartContainerwithRestartPolicy (23.78s)
+=== RUN TestDockerCLIRestartSuite/TestRestartStoppedContainer
+--- PASS: TestDockerCLIRestartSuite/TestRestartStoppedContainer (5.43s)Will rerun the CI a couple more times to see if they pass consistently. |
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
813a2a6 to
f083dce
Compare
| // VM. | ||
| if !containertypes.Isolation.IsHyperV(actualIsolation) { | ||
| if !daemon.UsesSnapshotter() && !containertypes.Isolation.IsHyperV(actualIsolation) { |
There was a problem hiding this comment.
We should probably update the comment above to describe why we're not doing it with containerd snapshotters as well now.
There was a problem hiding this comment.
Am I following this logic correctly and daemon.Mount is incrementing a reference count if the given container's storage is already mounted to the host, i.e. the current Linux case. The only way I understand this logic is if that's the case, and daemon.containerStop does an implicit daemon.Unmount, and daemon.containerStart does an implicit daemon.Mount.
The current change proposed here, as far as I understand, will reintroduce the needless (but otherwise harmless?) unmount/mount for Linux containerd-storage modes, as they still currently rely on host-mounting to function.
Would it be better to simply say "Only take a reference if the container is already mounted", or is that not knowable/thread-safe here? That would isolate this logic from host-mount-related flow-changes made elsewhere, e.g. moving to WithSnapshot.
That said, I'm not clear why this would fail on Windows in process isolation, since we currently also use a host-mount in that case, so this should be the same simple ref-count-only change, and not lead to calling ActivateLayer as seen in the failures... Is it perhaps elsewhere doing a path-name comparison, and getting a false-negative over something like what we use getBackingDeviceForContainerdMount to fix, and hence trying to create a second mount for the same snapshot?
There was a problem hiding this comment.
Actually, it looks like it could be caused by our ref-counted mount having a whitelist of filesystems. I opened a PR to reverse the logic and use the refcounted mounter by default (which we already talked about in the past): #47105
If these tests pass on the mentioned PR, we can close this one.
|
Closing in favor of: #47105 |
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)