daemon/oci: Extract side effects from withMounts

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

daemon/container_operations_unix.go

@@ -99,6 +99,45 @@ func (daemon *Daemon) getPIDContainer(id string) (*container.Container, error) {
 	return ctr, nil
 }
 
+// setupContainerDirs sets up base container directories (root, ipc, tmpfs and secrets).
+func (daemon *Daemon) setupContainerDirs(c *container.Container) (_ []container.Mount, err error) {
+	if err := daemon.setupContainerMountsRoot(c); err != nil {
+		return nil, err
+	}
+
+	if err := daemon.setupIPCDirs(c); err != nil {
+		return nil, err
+	}
+
+	if err := daemon.setupSecretDir(c); err != nil {
+		return nil, err
+	}
+	defer func() {
+		if err != nil {
+			daemon.cleanupSecretDir(c)
+		}
+	}()
+
+	var ms []container.Mount
+	if !c.HostConfig.IpcMode.IsPrivate() && !c.HostConfig.IpcMode.IsEmpty() {
+		ms = append(ms, c.IpcMounts()...)
+	}
+
+	tmpfsMounts, err := c.TmpfsMounts()
+	if err != nil {
+		return nil, err
+	}
+	ms = append(ms, tmpfsMounts...)
+
+	secretMounts, err := c.SecretMounts()
+	if err != nil {
+		return nil, err
+	}
+	ms = append(ms, secretMounts...)
+
+	return ms, nil
+}
+
 func (daemon *Daemon) setupIPCDirs(c *container.Container) error {
 	ipcMode := c.HostConfig.IpcMode
 

daemon/oci_linux.go

@@ -532,47 +532,8 @@ func inSlice(slice []string, s string) bool {
 }
 
 // withMounts sets the container's mounts
-func withMounts(daemon *Daemon, daemonCfg *configStore, c *container.Container) coci.SpecOpts {
+func withMounts(daemon *Daemon, daemonCfg *configStore, c *container.Container, ms []container.Mount) coci.SpecOpts {
 	return func(ctx context.Context, _ coci.Client, _ *containers.Container, s *coci.Spec) (err error) {
-		if err := daemon.setupContainerMountsRoot(c); err != nil {
-			return err
-		}
-
-		if err := daemon.setupIPCDirs(c); err != nil {
-			return err
-		}
-
-		defer func() {
-			if err != nil {
-				daemon.cleanupSecretDir(c)
-			}
-		}()
-
-		if err := daemon.setupSecretDir(c); err != nil {
-			return err
-		}
-
-		ms, err := daemon.setupMounts(c)
-		if err != nil {
-			return err
-		}
-
-		if !c.HostConfig.IpcMode.IsPrivate() && !c.HostConfig.IpcMode.IsEmpty() {
-			ms = append(ms, c.IpcMounts()...)
-		}
-
-		tmpfsMounts, err := c.TmpfsMounts()
-		if err != nil {
-			return err
-		}
-		ms = append(ms, tmpfsMounts...)
-
-		secretMounts, err := c.SecretMounts()
-		if err != nil {
-			return err
-		}
-		ms = append(ms, secretMounts...)
-
 		sort.Sort(mounts(ms))
 
 		mounts := ms
@@ -1093,7 +1054,7 @@ func WithUser(c *container.Container) coci.SpecOpts {
 	}
 }
 
-func (daemon *Daemon) createSpec(ctx context.Context, daemonCfg *configStore, c *container.Container) (retSpec *specs.Spec, err error) {
+func (daemon *Daemon) createSpec(ctx context.Context, daemonCfg *configStore, c *container.Container, mounts []container.Mount) (retSpec *specs.Spec, err error) {
 	var (
 		opts []coci.SpecOpts
 		s    = oci.DefaultSpec()
@@ -1108,7 +1069,7 @@ func (daemon *Daemon) createSpec(ctx context.Context, daemonCfg *configStore, c
 		WithNamespaces(daemon, c),
 		WithCapabilities(c),
 		WithSeccomp(daemon, c),
-		withMounts(daemon, daemonCfg, c),
+		withMounts(daemon, daemonCfg, c, mounts),
 		withLibnetwork(daemon, &daemonCfg.Config, c),
 		WithApparmor(c),
 		WithSelinux(c),

daemon/oci_linux_test.go

@@ -92,7 +92,7 @@ func TestTmpfsDevShmNoDupMount(t *testing.T) {
 	}
 	d := setupFakeDaemon(t, c)
 
-	_, err := d.createSpec(context.TODO(), &configStore{}, c)
+	_, err := d.createSpec(context.TODO(), &configStore{}, c, nil)
 	assert.Check(t, err)
 }
 
@@ -110,7 +110,7 @@ func TestIpcPrivateVsReadonly(t *testing.T) {
 	}
 	d := setupFakeDaemon(t, c)
 
-	s, err := d.createSpec(context.TODO(), &configStore{}, c)
+	s, err := d.createSpec(context.TODO(), &configStore{}, c, nil)
 	assert.Check(t, err)
 
 	// Find the /dev/shm mount in ms, check it does not have ro
@@ -139,7 +139,7 @@ func TestSysctlOverride(t *testing.T) {
 	d := setupFakeDaemon(t, c)
 
 	// Ensure that the implicit sysctl is set correctly.
-	s, err := d.createSpec(context.TODO(), &configStore{}, c)
+	s, err := d.createSpec(context.TODO(), &configStore{}, c, nil)
 	assert.NilError(t, err)
 	assert.Equal(t, s.Hostname, "foobar")
 	assert.Equal(t, s.Linux.Sysctl["kernel.domainname"], c.Config.Domainname)
@@ -155,14 +155,14 @@ func TestSysctlOverride(t *testing.T) {
 	assert.Assert(t, c.HostConfig.Sysctls["kernel.domainname"] != c.Config.Domainname)
 	c.HostConfig.Sysctls["net.ipv4.ip_unprivileged_port_start"] = "1024"
 
-	s, err = d.createSpec(context.TODO(), &configStore{}, c)
+	s, err = d.createSpec(context.TODO(), &configStore{}, c, nil)
 	assert.NilError(t, err)
 	assert.Equal(t, s.Hostname, "foobar")
 	assert.Equal(t, s.Linux.Sysctl["kernel.domainname"], c.HostConfig.Sysctls["kernel.domainname"])
 	assert.Equal(t, s.Linux.Sysctl["net.ipv4.ip_unprivileged_port_start"], c.HostConfig.Sysctls["net.ipv4.ip_unprivileged_port_start"])
 
 	// Ensure the ping_group_range is not set on a daemon with user-namespaces enabled
-	s, err = d.createSpec(context.TODO(), &configStore{Config: config.Config{RemappedRoot: "dummy:dummy"}}, c)
+	s, err = d.createSpec(context.TODO(), &configStore{Config: config.Config{RemappedRoot: "dummy:dummy"}}, c, nil)
 	assert.NilError(t, err)
 	_, ok := s.Linux.Sysctl["net.ipv4.ping_group_range"]
 	assert.Assert(t, !ok)
@@ -170,7 +170,7 @@ func TestSysctlOverride(t *testing.T) {
 	// Ensure the ping_group_range is set on a container in "host" userns mode
 	// on a daemon with user-namespaces enabled
 	c.HostConfig.UsernsMode = "host"
-	s, err = d.createSpec(context.TODO(), &configStore{Config: config.Config{RemappedRoot: "dummy:dummy"}}, c)
+	s, err = d.createSpec(context.TODO(), &configStore{Config: config.Config{RemappedRoot: "dummy:dummy"}}, c, nil)
 	assert.NilError(t, err)
 	assert.Equal(t, s.Linux.Sysctl["net.ipv4.ping_group_range"], "0 2147483647")
 }
@@ -189,7 +189,7 @@ func TestSysctlOverrideHost(t *testing.T) {
 	d := setupFakeDaemon(t, c)
 
 	// Ensure that the implicit sysctl is not set
-	s, err := d.createSpec(context.TODO(), &configStore{}, c)
+	s, err := d.createSpec(context.TODO(), &configStore{}, c, nil)
 	assert.NilError(t, err)
 	assert.Equal(t, s.Linux.Sysctl["net.ipv4.ip_unprivileged_port_start"], "")
 	assert.Equal(t, s.Linux.Sysctl["net.ipv4.ping_group_range"], "")
@@ -197,7 +197,7 @@ func TestSysctlOverrideHost(t *testing.T) {
 	// Set an explicit sysctl.
 	c.HostConfig.Sysctls["net.ipv4.ip_unprivileged_port_start"] = "1024"
 
-	s, err = d.createSpec(context.TODO(), &configStore{}, c)
+	s, err = d.createSpec(context.TODO(), &configStore{}, c, nil)
 	assert.NilError(t, err)
 	assert.Equal(t, s.Linux.Sysctl["net.ipv4.ip_unprivileged_port_start"], c.HostConfig.Sysctls["net.ipv4.ip_unprivileged_port_start"])
 }
@@ -225,7 +225,7 @@ func TestDefaultResources(t *testing.T) {
 	}
 	d := setupFakeDaemon(t, c)
 
-	s, err := d.createSpec(context.Background(), &configStore{}, c)
+	s, err := d.createSpec(context.Background(), &configStore{}, c, nil)
 	assert.NilError(t, err)
 	checkResourcesAreUnset(t, s.Linux.Resources)
 }

daemon/oci_windows.go

@@ -30,30 +30,11 @@ const (
 	credentialSpecFileLocation     = "CredentialSpecs"
 )
 
-func (daemon *Daemon) createSpec(ctx context.Context, daemonCfg *configStore, c *container.Container) (*specs.Spec, error) {
-	img, err := daemon.imageService.GetImage(ctx, string(c.ImageID), imagetypes.GetImageOpts{})
-	if err != nil {
-		return nil, err
-	}
-	if err := image.CheckOS(img.OperatingSystem()); err != nil {
-		return nil, err
-	}
-
-	s := oci.DefaultSpec()
-
-	if err := coci.WithAnnotations(c.HostConfig.Annotations)(ctx, nil, nil, &s); err != nil {
-		return nil, err
-	}
-
-	linkedEnv, err := daemon.setupLinkedContainers(c)
-	if err != nil {
-		return nil, err
-	}
-
+// setupContainerDirs sets up base container directories (root, ipc, tmpfs and secrets).
+func (daemon *Daemon) setupContainerDirs(c *container.Container) ([]container.Mount, error) {
 	// Note, unlike Unix, we do NOT call into SetupWorkingDirectory as
 	// this is done in VMCompute. Further, we couldn't do it for Hyper-V
 	// containers anyway.
-
 	if err := daemon.setupSecretDir(c); err != nil {
 		return nil, err
 	}
@@ -62,25 +43,6 @@ func (daemon *Daemon) createSpec(ctx context.Context, daemonCfg *configStore, c
 		return nil, err
 	}
 
-	// In s.Mounts
-	mounts, err := daemon.setupMounts(c)
-	if err != nil {
-		return nil, err
-	}
-
-	var isHyperV bool
-	if c.HostConfig.Isolation.IsDefault() {
-		// Container using default isolation, so take the default from the daemon configuration
-		isHyperV = daemon.defaultIsolation.IsHyperV()
-	} else {
-		// Container may be requesting an explicit isolation mode.
-		isHyperV = c.HostConfig.Isolation.IsHyperV()
-	}
-
-	if isHyperV {
-		s.Windows.HyperV = &specs.WindowsHyperV{}
-	}
-
 	// If the container has not been started, and has configs or secrets
 	// secrets, create symlinks to each config and secret. If it has been
 	// started before, the symlinks should have already been created. Also, it
@@ -90,7 +52,7 @@ func (daemon *Daemon) createSpec(ctx context.Context, daemonCfg *configStore, c
 	if !c.HasBeenStartedBefore && (len(c.SecretReferences) > 0 || len(c.ConfigReferences) > 0) {
 		// The container file system is mounted before this function is called,
 		// except for Hyper-V containers, so mount it here in that case.
-		if isHyperV {
+		if daemon.isHyperV(c) {
 			if err := daemon.Mount(c); err != nil {
 				return nil, err
 			}
@@ -108,6 +70,8 @@ func (daemon *Daemon) createSpec(ctx context.Context, daemonCfg *configStore, c
 	if err != nil {
 		return nil, err
 	}
+
+	var mounts []container.Mount
 	if secretMounts != nil {
 		mounts = append(mounts, secretMounts...)
 	}
@@ -116,6 +80,33 @@ func (daemon *Daemon) createSpec(ctx context.Context, daemonCfg *configStore, c
 		mounts = append(mounts, configMounts...)
 	}
 
+	return mounts, nil
+}
+
+func (daemon *Daemon) isHyperV(c *container.Container) bool {
+	if c.HostConfig.Isolation.IsDefault() {
+		// Container using default isolation, so take the default from the daemon configuration
+		return daemon.defaultIsolation.IsHyperV()
+	}
+	// Container may be requesting an explicit isolation mode.
+	return c.HostConfig.Isolation.IsHyperV()
+}
+
+func (daemon *Daemon) createSpec(ctx context.Context, daemonCfg *configStore, c *container.Container, mounts []container.Mount) (*specs.Spec, error) {
+	img, err := daemon.imageService.GetImage(ctx, string(c.ImageID), imagetypes.GetImageOpts{})
+	if err != nil {
+		return nil, err
+	}
+	if err := image.CheckOS(img.OperatingSystem()); err != nil {
+		return nil, err
+	}
+
+	s := oci.DefaultSpec()
+
+	if err := coci.WithAnnotations(c.HostConfig.Annotations)(ctx, nil, nil, &s); err != nil {
+		return nil, err
+	}
+
 	for _, mount := range mounts {
 		m := specs.Mount{
 			Source:      mount.Source,
@@ -127,6 +118,16 @@ func (daemon *Daemon) createSpec(ctx context.Context, daemonCfg *configStore, c
 		s.Mounts = append(s.Mounts, m)
 	}
 
+	linkedEnv, err := daemon.setupLinkedContainers(c)
+	if err != nil {
+		return nil, err
+	}
+
+	isHyperV := daemon.isHyperV(c)
+	if isHyperV {
+		s.Windows.HyperV = &specs.WindowsHyperV{}
+	}
+
 	// In s.Process
 	s.Process.Cwd = c.Config.WorkingDir
 	s.Process.Env = c.CreateDaemonEnvironment(c.Config.Tty, linkedEnv)

daemon/start.go

@@ -159,7 +159,18 @@ func (daemon *Daemon) containerStart(ctx context.Context, daemonCfg *configStore
 		return err
 	}
 
-	spec, err := daemon.createSpec(ctx, daemonCfg, container)
+	mnts, err := daemon.setupContainerDirs(container)
+	if err != nil {
+		return err
+	}
+
+	m, err := daemon.setupMounts(container)
+	if err != nil {
+		return err
+	}
+	mnts = append(mnts, m...)
+
+	spec, err := daemon.createSpec(ctx, daemonCfg, container, mnts)
 	if err != nil {
 		// Any error that occurs while creating the spec, even if it's the
 		// result of an invalid container config, must be considered a System