Windows: Fixes Windows containers with image volumes

claudiubelu · claudiubelu · commit 791e175c7993 · 2021-10-01T09:02:18.000Z
Currently, there are few issues that preventing containers
with image volumes to properly start on Windows.

- Unlike the Linux implementation, the Container volume mount paths
  were not created if they didn't exist. Those paths are now created.

- while copying the image volume contents to the container volume,
  the layers were not properly deactivated, which means that the
  container can't start since those layers are still open. The layers
  are now properly deactivated, allowing the container to start.

- even if the above issue didn't exist, the Windows implementation of
  mount/Mount.Mount deactivates the layers, which wouldn't allow us
  to copy files from them. The layers are now deactivated after we've
  copied the necessary files from them.

- the target argument of the Windows implementation of mount/Mount.Mount
  was unused, which means that folder was always empty. We're now
  symlinking the Layer Mount Path into the target folder.

- hcsshim needs its Container Mount Paths to be properly formated, to be
  prefixed by C:. This was an issue for Volumes defined with Linux-like
  paths (e.g.: /test_dir). filepath.Abs solves this issue.

Signed-off-by: Claudiu Belu &lt;cbelu@cloudbasesolutions.com&gt;
diff --git a/mount/mount_windows.go b/mount/mount_windows.go
@@ -18,6 +18,7 @@ package mount
 
 import (
 	"encoding/json"
+	"os"
 	"path/filepath"
 	"strings"
 
@@ -50,15 +51,21 @@ func (m *Mount) Mount(target string) error {
 	if err = hcsshim.ActivateLayer(di, layerID); err != nil {
 		return errors.Wrapf(err, "failed to activate layer %s", m.Source)
 	}
-	defer func() {
-		if err != nil {
-			hcsshim.DeactivateLayer(di, layerID)
-		}
-	}()
 
 	if err = hcsshim.PrepareLayer(di, layerID, parentLayerPaths); err != nil {
 		return errors.Wrapf(err, "failed to prepare layer %s", m.Source)
 	}
+
+	// We can link the layer mount path to the given target. It is an UNC path, and it needs
+	// a trailing backslash.
+	mountPath, err := hcsshim.GetLayerMountPath(di, layerID)
+	if err != nil {
+		return errors.Wrapf(err, "failed to get layer mount path for %s", m.Source)
+	}
+	mountPath = mountPath + `\`
+	if err = os.Symlink(mountPath, target); err != nil {
+		return errors.Wrapf(err, "failed to link mount to taget %s", target)
+	}
 	return nil
 }
 
diff --git a/pkg/cri/opts/container.go b/pkg/cri/opts/container.go
@@ -21,6 +21,8 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	goruntime "runtime"
+	"strings"
 
 	"github.com/containerd/containerd"
 	"github.com/containerd/containerd/containers"
@@ -76,29 +78,51 @@ func WithVolumes(volumeMounts map[string]string) containerd.NewContainerOpts {
 		// refer to https://github.com/containerd/containerd/pull/1868
 		// https://github.com/containerd/containerd/pull/1785
 		defer os.Remove(root) // nolint: errcheck
-		if err := mount.All(mounts, root); err != nil {
-			return errors.Wrap(err, "failed to mount")
-		}
-		defer func() {
-			if uerr := mount.Unmount(root, 0); uerr != nil {
-				log.G(ctx).WithError(uerr).Errorf("Failed to unmount snapshot %q", c.SnapshotKey)
+
+		unmounter := func(mountPath string) {
+			if uerr := mount.Unmount(mountPath, 0); uerr != nil {
+				log.G(ctx).WithError(uerr).Errorf("Failed to unmount snapshot %q", root)
 				if err == nil {
 					err = uerr
 				}
 			}
-		}()
+		}
 
-		for host, volume := range volumeMounts {
-			src := filepath.Join(root, volume)
-			if _, err := os.Stat(src); err != nil {
-				if os.IsNotExist(err) {
-					// Skip copying directory if it does not exist.
-					continue
+		var mountPaths []string
+		if goruntime.GOOS == "windows" {
+			for _, m := range mounts {
+				// appending the layerID to the root.
+				mountPath := filepath.Join(root, filepath.Base(m.Source))
+				mountPaths = append(mountPaths, mountPath)
+				if err := m.Mount(mountPath); err != nil {
+					return err
 				}
-				return errors.Wrap(err, "stat volume in rootfs")
+
+				defer unmounter(m.Source)
+			}
+		} else {
+			mountPaths = append(mountPaths, root)
+			if err := mount.All(mounts, root); err != nil {
+				return errors.Wrap(err, "failed to mount")
 			}
-			if err := copyExistingContents(src, host); err != nil {
-				return errors.Wrap(err, "taking runtime copy of volume")
+			defer unmounter(root)
+		}
+
+		for host, volume := range volumeMounts {
+			// The volume may have been defined with a C: prefix, which we can't use here.
+			volume = strings.TrimPrefix(volume, "C:")
+			for _, mountPath := range mountPaths {
+				src := filepath.Join(mountPath, volume)
+				if _, err := os.Stat(src); err != nil {
+					if os.IsNotExist(err) {
+						// Skip copying directory if it does not exist.
+						continue
+					}
+					return errors.Wrap(err, "stat volume in rootfs")
+				}
+				if err := copyExistingContents(src, host); err != nil {
+					return errors.Wrap(err, "taking runtime copy of volume")
+				}
 			}
 		}
 		return nil
diff --git a/pkg/cri/opts/spec_windows.go b/pkg/cri/opts/spec_windows.go
@@ -18,6 +18,7 @@ package opts
 
 import (
 	"context"
+	"os"
 	"path/filepath"
 	"sort"
 	"strings"
@@ -119,19 +120,28 @@ func WithWindowsMounts(osi osinterface.OS, config *runtime.ContainerConfig, extr
 			// paths, so don't use it.
 			if !namedPipePath(src) {
 				if _, err := osi.Stat(src); err != nil {
-					// If the source doesn't exist, return an error instead
-					// of creating the source. This aligns with Docker's
-					// behavior on windows.
-					return errors.Wrapf(err, "failed to stat %q", src)
+					// Create the host path if it doesn't exist. This will align
+					// the behavior with the Linux implementation, but it doesn't
+					// align with Docker's behavior on Windows.
+					if !os.IsNotExist(err) {
+						return errors.Wrapf(err, "failed to stat %q", src)
+					}
+					if err := osi.MkdirAll(src, 0755); err != nil {
+						return errors.Wrapf(err, "failed to mkdir %q", src)
+					}
 				}
 				var err error
 				src, err = osi.ResolveSymbolicLink(src)
 				if err != nil {
 					return errors.Wrapf(err, "failed to resolve symlink %q", src)
 				}
-				// hcsshim requires clean path, especially '/' -> '\'.
+				// hcsshim requires clean path, especially '/' -> '\'. Additionally,
+				// for the destination, absolute paths should have the C: prefix.
 				src = filepath.Clean(src)
 				dst = filepath.Clean(dst)
+				if dst[0] == '\\' {
+					dst = "C:" + dst
+				}
 			}
 
 			var options []string
