moby icon indicating copy to clipboard operation
moby copied to clipboard

c8d/daemon: Mount root and fill BaseFS

Open thaJeztah opened this issue 3 years ago • 2 comments

  • upstreaming https://github.com/rumpl/moby/pull/65

⚠️ Changes compared to that patch:

diff --git a/daemon/containerd/mount.go b/daemon/containerd/mount.go
index b0d4af19db..c5476adc60 100644
--- a/daemon/containerd/mount.go
+++ b/daemon/containerd/mount.go
@@ -13,7 +13,7 @@ import (
 
 // Mount mounts and sets container base filesystem
 func (i *ImageService) Mount(ctx context.Context, container *container.Container) error {
-	snapshotter := i.client.SnapshotService(i.snapshotter)
+	snapshotter := i.client.SnapshotService(container.Driver)
 	mounts, err := snapshotter.Mounts(ctx, container.ID)
 	if err != nil {
 		return err
@@ -35,7 +35,7 @@ func (i *ImageService) Mount(ctx context.Context, container *container.Container
 }
 
 // Unmount unmounts the container base filesystem
-func (i *ImageService) Unmount(ctx context.Context, container *container.Container) error {
+func (i *ImageService) Unmount(_ context.Context, container *container.Container) error {
 	root := container.BaseFS.Path()
 
 	if err := mount.UnmountAll(root, 0); err != nil {
diff --git a/daemon/daemon_unix.go b/daemon/daemon_unix.go
index 6e2b13cd3a..a301055f23 100644
--- a/daemon/daemon_unix.go
+++ b/daemon/daemon_unix.go
@@ -1364,19 +1364,19 @@ func (daemon *Daemon) registerLinks(container *container.Container, hostConfig *
 // conditionalMountOnStart is a platform specific helper function during the
 // container start to call mount.
 func (daemon *Daemon) conditionalMountOnStart(container *container.Container) error {
-	if !daemon.UsesSnapshotter() {
-		return daemon.Mount(container)
+	if daemon.UsesSnapshotter() {
+		return nil
 	}
-	return nil
+	return daemon.Mount(container)
 }
 
 // conditionalUnmountOnCleanup is a platform specific helper function called
 // during the cleanup of a container to unmount.
 func (daemon *Daemon) conditionalUnmountOnCleanup(container *container.Container) error {
-	if !daemon.UsesSnapshotter() {
-		return daemon.Unmount(container)
+	if daemon.UsesSnapshotter() {
+		return nil
 	}
-	return nil
+	return daemon.Unmount(container)
 }
 
 func copyBlkioEntry(entries []*statsV1.BlkIOEntry) []types.BlkioStatEntry {
diff --git a/daemon/images/mount.go b/daemon/images/mount.go
index 383c18e713..04ff19aff0 100644
--- a/daemon/images/mount.go
+++ b/daemon/images/mount.go
@@ -28,11 +28,11 @@ func (i *ImageService) Mount(ctx context.Context, container *container.Container
 		// on non-Windows operating systems.
 		if runtime.GOOS != "windows" {
 			i.Unmount(ctx, container)
-			return fmt.Errorf("Error: driver %s is returning inconsistent paths for container %s ('%s' then '%s')",
-				i.GraphDriverName(), container.ID, container.BaseFS, dir)
+			return fmt.Errorf("driver %s is returning inconsistent paths for container %s ('%s' then '%s')",
+				container.Driver, container.ID, container.BaseFS, dir)
 		}
 	}

This fixes things that were broken due to nil BaseFS like docker cp and running a container with workdir override.

This is more of a temporary hack than a real solution. The correct fix would be to refactor the code to make BaseFS and LayerRW an implementation detail of the old image store implementation and use the temporary mounts for the c8d implementation instead. That requires more work though.

- A picture of a cute animal (not mandatory but encouraged)

thaJeztah avatar Sep 01 '22 12:09 thaJeztah

Bunch of failures that could be related, e.g.;

=== RUN   TestDockerSuite/TestVolumeCLICreateWithOpts
    check_test.go:402: assertion failed: error is not nil: Error response from daemon: remove test: volume has active mounts: failed to remove volume test
    --- FAIL: TestDockerSuite/TestVolumeCLICreateWithOpts (0.48s)

=== RUN   TestDockerSuite/TestVolumeCLIRm
    docker_cli_volume_test.go:197: assertion failed: 
        Command:  /usr/local/cli/docker volume rm test
        ExitCode: 1
        Error:    exit status 1
        Stdout:   
        Stderr:   Error response from daemon: remove test: volume has active mounts
        
        
        Failures:
        ExitCode was 1 expected 0
        Expected no error
    check_test.go:402: assertion failed: error is not nil: Error response from daemon: remove test: volume has active mounts: failed to remove volume test
    --- FAIL: TestDockerSuite/TestVolumeCLIRm (0.05s)

thaJeztah avatar Sep 01 '22 15:09 thaJeztah

Rebased this one to look at the remaining diff in here that was not included in https://github.com/moby/moby/pull/44804

thaJeztah avatar Mar 06 '23 21:03 thaJeztah

Closing this one, the upstream PR was already upstreamed

rumpl avatar Apr 04 '23 15:04 rumpl