From 791e175c79930a34cfbb2048fbcaa8493fd2c86b Mon Sep 17 00:00:00 2001 From: Claudiu Belu Date: Mon, 20 Sep 2021 18:43:51 +0300 Subject: [PATCH] Windows: Fixes Windows containers with image volumes 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 --- mount/mount_windows.go | 17 +++++++---- pkg/cri/opts/container.go | 56 +++++++++++++++++++++++++----------- pkg/cri/opts/spec_windows.go | 20 +++++++++---- 3 files changed, 67 insertions(+), 26 deletions(-) diff --git a/mount/mount_windows.go b/mount/mount_windows.go index 5de25c4e0..57e589f03 100644 --- 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 index 472104b04..ebe840a00 100644 --- 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 } } - }() + } + + 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 + } + + defer unmounter(m.Source) + } + } else { + mountPaths = append(mountPaths, root) + if err := mount.All(mounts, root); err != nil { + return errors.Wrap(err, "failed to mount") + } + defer unmounter(root) + } 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 + // 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 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 index 6beb7e4e5..e4e56d9c4 100644 --- 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