Merge pull request #8362 from gabriel-samfira/fix-non-c-volume

Fix non C volumes on Windows

integration/images/image_list.go

@@ -52,7 +52,7 @@ func initImages(imageListFile string) {
 		BusyBox:          "ghcr.io/containerd/busybox:1.36",
 		Pause:            "registry.k8s.io/pause:3.9",
 		ResourceConsumer: "registry.k8s.io/e2e-test-images/resource-consumer:1.10",
-		VolumeCopyUp:     "ghcr.io/containerd/volume-copy-up:2.1",
+		VolumeCopyUp:     "ghcr.io/containerd/volume-copy-up:2.2",
 		VolumeOwnership:  "ghcr.io/containerd/volume-ownership:2.1",
 		ArgsEscaped:      "cplatpublic.azurecr.io/args-escaped-test-image-ns:1.0",
 	}

integration/main_test.go

@@ -66,7 +66,6 @@ var (
 )
 
 var criEndpoint = flag.String("cri-endpoint", "unix:///run/containerd/containerd.sock", "The endpoint of cri plugin.")
-var criRoot = flag.String("cri-root", "/var/lib/containerd/io.containerd.grpc.v1.cri", "The root directory of cri plugin.")
 var runtimeHandler = flag.String("runtime-handler", "", "The runtime handler to use in the test.")
 var containerdBin = flag.String("containerd-bin", "containerd", "The containerd binary name. The name is used to restart containerd during test.")
 

integration/volume_copy_up_test.go

@@ -17,16 +17,20 @@
 package integration
 
 import (
+	"context"
+	"encoding/json"
 	"fmt"
 	"os"
 	"path/filepath"
-	goruntime "runtime"
+	"runtime"
 	"testing"
 	"time"
 
 	"github.com/containerd/containerd/integration/images"
+	specs "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	v1 "k8s.io/cri-api/pkg/apis/runtime/v1"
 )
 
 const (
@@ -36,6 +40,16 @@ const (
 	containerUserSID = "S-1-5-93-2-2"
 )
 
+type volumeFile struct {
+	fileName string
+	contents string
+}
+
+type containerVolume struct {
+	containerPath string
+	files         []volumeFile
+}
+
 func TestVolumeCopyUp(t *testing.T) {
 	var (
 		testImage   = images.Get(images.VolumeCopyUp)
@@ -59,23 +73,85 @@ func TestVolumeCopyUp(t *testing.T) {
 	t.Logf("Start the container")
 	require.NoError(t, runtimeService.StartContainer(cn))
 
-	// ghcr.io/containerd/volume-copy-up:2.1 contains a test_dir
+	expectedVolumes := []containerVolume{
-	// volume, which contains a test_file with content "test_content".
+		{
-	t.Logf("Check whether volume contains the test file")
+			containerPath: "/test_dir",
-	stdout, stderr, err := runtimeService.ExecSync(cn, []string{
+			files: []volumeFile{
-		"cat",
+				{
-		"/test_dir/test_file",
+					fileName: "test_file",
-	}, execTimeout)
+					contents: "test_content\n",
+				},
+			},
+		},
+		{
+			containerPath: "/:colon_prefixed",
+			files: []volumeFile{
+				{
+					fileName: "colon_prefixed_file",
+					contents: "test_content\n",
+				},
+			},
+		},
+		{
+			containerPath: "C:/weird_test_dir",
+			files: []volumeFile{
+				{
+					fileName: "weird_test_file",
+					contents: "test_content\n",
+				},
+			},
+		},
+	}
+
+	if runtime.GOOS == "windows" {
+		expectedVolumes = []containerVolume{
+			{
+				containerPath: "C:\\test_dir",
+				files: []volumeFile{
+					{
+						fileName: "test_file",
+						contents: "test_content\n",
+					},
+				},
+			},
+			{
+				containerPath: "D:",
+				files:         []volumeFile{},
+			},
+		}
+	}
+
+	volumeMappings, err := getContainerBindVolumes(t, cn)
 	require.NoError(t, err)
-	assert.Empty(t, stderr)
-	assert.Equal(t, "test_content\n", string(stdout))
 
 	t.Logf("Check host path of the volume")
-	volumePaths, err := getHostPathForVolumes(*criRoot, cn)
+	for _, vol := range expectedVolumes {
-	require.NoError(t, err)
+		_, ok := volumeMappings[vol.containerPath]
-	assert.Equal(t, len(volumePaths), 1, "expected exactly 1 volume")
+		assert.Equalf(t, true, ok, "expected to find volume %s", vol.containerPath)
+	}
 
-	testFilePath := filepath.Join(volumePaths[0], "test_file")
+	// ghcr.io/containerd/volume-copy-up:2.2 contains 3 volumes on Linux and 2 volumes on Windows.
+	// On linux, each of the volumes contains a single file, all with the same conrent. On Windows,
+	// non C volumes defined in the image start out as empty.
+	for _, vol := range expectedVolumes {
+		files, err := os.ReadDir(volumeMappings[vol.containerPath])
+		require.NoError(t, err)
+		assert.Equal(t, len(vol.files), len(files))
+
+		for _, file := range vol.files {
+			t.Logf("Check whether volume %s contains the test file %s", vol.containerPath, file.fileName)
+			stdout, stderr, err := runtimeService.ExecSync(cn, []string{
+				"cat",
+				filepath.ToSlash(filepath.Join(vol.containerPath, file.fileName)),
+			}, execTimeout)
+			require.NoError(t, err)
+			assert.Empty(t, stderr)
+			assert.Equal(t, file.contents, string(stdout))
+		}
+	}
+
+	testFilePath := filepath.Join(volumeMappings[expectedVolumes[0].containerPath], expectedVolumes[0].files[0].fileName)
+	inContainerPath := filepath.Join(expectedVolumes[0].containerPath, expectedVolumes[0].files[0].fileName)
 	contents, err := os.ReadFile(testFilePath)
 	require.NoError(t, err)
 	assert.Equal(t, "test_content\n", string(contents))
@@ -84,7 +160,7 @@ func TestVolumeCopyUp(t *testing.T) {
 	_, _, err = runtimeService.ExecSync(cn, []string{
 		"sh",
 		"-c",
-		"echo new_content > /test_dir/test_file",
+		fmt.Sprintf("echo new_content > %s", filepath.ToSlash(inContainerPath)),
 	}, execTimeout)
 	require.NoError(t, err)
 
@@ -124,15 +200,17 @@ func TestVolumeOwnership(t *testing.T) {
 	// exist inside the container that returns the owner in the form of USERNAME:SID.
 	t.Logf("Check ownership of test directory inside container")
 
+	volumePath := "/test_dir"
 	cmd := []string{
-		"stat", "-c", "%u:%g", "/test_dir",
+		"stat", "-c", "%u:%g", volumePath,
 	}
 	expectedContainerOutput := "65534:65534\n"
 	expectedHostOutput := "65534:65534\n"
-	if goruntime.GOOS == "windows" {
+	if runtime.GOOS == "windows" {
+		volumePath = "C:\\volumes\\test_dir"
 		cmd = []string{
 			"C:\\bin\\get_owner.exe",
-			"C:\\volumes\\test_dir",
+			volumePath,
 		}
 		expectedContainerOutput = fmt.Sprintf("%s:%s", containerUserName, containerUserSID)
 		// The username is unknown on the host, but we can still get the SID.
@@ -144,34 +222,36 @@ func TestVolumeOwnership(t *testing.T) {
 	assert.Equal(t, expectedContainerOutput, string(stdout))
 
 	t.Logf("Check ownership of test directory on the host")
-	volumePaths, err := getHostPathForVolumes(*criRoot, cn)
+	volumePaths, err := getContainerBindVolumes(t, cn)
 	require.NoError(t, err)
-	assert.Equal(t, len(volumePaths), 1, "expected exactly 1 volume")
 
-	output, err := getOwnership(volumePaths[0])
+	output, err := getOwnership(volumePaths[volumePath])
 	require.NoError(t, err)
 	assert.Equal(t, expectedHostOutput, output)
 }
 
-func getHostPathForVolumes(criRoot, containerID string) ([]string, error) {
+func getContainerBindVolumes(t *testing.T, containerID string) (map[string]string, error) {
-	hostPath := filepath.Join(criRoot, "containers", containerID, "volumes")
+	client, err := RawRuntimeClient()
-	if _, err := os.Stat(hostPath); err != nil {
+	require.NoError(t, err, "failed to get raw grpc runtime service client")
-		return nil, err
+	request := &v1.ContainerStatusRequest{
+		ContainerId: containerID,
+		Verbose:     true,
 	}
+	response, err := client.ContainerStatus(context.TODO(), request)
+	require.NoError(t, err)
+	ret := make(map[string]string)
 
-	volumes, err := os.ReadDir(hostPath)
+	mounts := struct {
-	if err != nil {
+		RuntimeSpec struct {
-		return nil, err
+			Mounts []specs.Mount `json:"mounts"`
+		} `json:"runtimeSpec"`
+	}{}
+
+	info := response.Info["info"]
+	err = json.Unmarshal([]byte(info), &mounts)
+	require.NoError(t, err)
+	for _, mount := range mounts.RuntimeSpec.Mounts {
+		ret[mount.Destination] = mount.Source
 	}
-
+	return ret, nil
-	if len(volumes) == 0 {
-		return []string{}, nil
-	}
-
-	volumePaths := make([]string, len(volumes))
-	for idx, volume := range volumes {
-		volumePaths[idx] = filepath.Join(hostPath, volume.Name())
-	}
-
-	return volumePaths, nil
 }

pkg/cri/opts/container.go

@@ -24,6 +24,7 @@ import (
 	"strings"
 
 	"github.com/containerd/continuity/fs"
+	imagespec "github.com/opencontainers/image-spec/specs-go/v1"
 
 	"github.com/containerd/containerd"
 	"github.com/containerd/containerd/containers"
@@ -55,7 +56,7 @@ func WithNewSnapshot(id string, i containerd.Image, opts ...snapshots.Opt) conta
 // WithVolumes copies ownership of volume in rootfs to its corresponding host path.
 // It doesn't update runtime spec.
 // The passed in map is a host path to container path map for all volumes.
-func WithVolumes(volumeMounts map[string]string) containerd.NewContainerOpts {
+func WithVolumes(volumeMounts map[string]string, platform imagespec.Platform) containerd.NewContainerOpts {
 	return func(ctx context.Context, client *containerd.Client, c *containers.Container) (err error) {
 		if c.Snapshotter == "" {
 			return errors.New("no snapshotter set for container")
@@ -97,8 +98,24 @@ func WithVolumes(volumeMounts map[string]string) containerd.NewContainerOpts {
 		}()
 
 		for host, volume := range volumeMounts {
-			// The volume may have been defined with a C: prefix, which we can't use here.
+			if platform.OS == "windows" {
-			volume = strings.TrimPrefix(volume, "C:")
+				// Windows allows volume mounts in subfolders under C: and as any other drive letter like D:, E:, etc.
+				// An image may contain files inside a folder defined as a VOLUME in a Dockerfile. On Windows, images
+				// can only contain pre-existing files for volumes situated on the root filesystem, which is C:.
+				// For any other volumes, we need to skip attempting to copy existing contents.
+				//
+				// C:\some\volume --> \some\volume
+				// D:\some\volume --> skip
+				if len(volume) >= 2 && string(volume[1]) == ":" {
+					// Perform a case insensitive comparison to "C", and skip non-C mounted volumes.
+					if !strings.EqualFold(string(volume[0]), "c") {
+						continue
+					}
+					// This is a volume mounted somewhere under C:\. We strip the drive letter and allow fs.RootPath()
+					// to append the remaining path to the rootfs path as seen by the host OS.
+					volume = volume[2:]
+				}
+			}
 			src, err := fs.RootPath(root, volume)
 			if err != nil {
 				return fmt.Errorf("rootpath on mountPath %s, volume %s: %w", root, volume, err)

pkg/cri/sbserver/container_create.go

@@ -223,7 +223,7 @@ func (c *criService) CreateContainer(ctx context.Context, r *runtime.CreateConta
 		for _, v := range volumeMounts {
 			mountMap[filepath.Clean(v.HostPath)] = v.ContainerPath
 		}
-		opts = append(opts, customopts.WithVolumes(mountMap))
+		opts = append(opts, customopts.WithVolumes(mountMap, platform))
 	}
 	meta.ImageRef = image.ID
 	meta.StopSignal = image.ImageSpec.Config.StopSignal

pkg/cri/server/container_create.go

@@ -203,7 +203,14 @@ func (c *criService) CreateContainer(ctx context.Context, r *runtime.CreateConta
 		for _, v := range volumeMounts {
 			mountMap[filepath.Clean(v.HostPath)] = v.ContainerPath
 		}
-		opts = append(opts, customopts.WithVolumes(mountMap))
+		platform := imagespec.Platform{
+			OS:           image.ImageSpec.OS,
+			Architecture: image.ImageSpec.Architecture,
+			OSVersion:    image.ImageSpec.OSVersion,
+			OSFeatures:   image.ImageSpec.OSFeatures,
+			Variant:      image.ImageSpec.Variant,
+		}
+		opts = append(opts, customopts.WithVolumes(mountMap, platform))
 	}
 	meta.ImageRef = image.ID
 	meta.StopSignal = image.ImageSpec.Config.StopSignal

script/test/cri-integration.sh

@@ -37,7 +37,6 @@ fi
 # RUNTIME is the runtime handler to use in the test.
 RUNTIME=${RUNTIME:-""}
 
-CRI_ROOT="${CONTAINERD_ROOT}/io.containerd.grpc.v1.cri"
 mkdir -p "${REPORT_DIR}"
 test_setup "${REPORT_DIR}"
 
@@ -54,7 +53,6 @@ CMD+="${PWD}/bin/cri-integration.test"
 
 ${CMD} --test.run="${FOCUS}" --test.v \
   --cri-endpoint="${CONTAINERD_SOCK}" \
-  --cri-root="${CRI_ROOT}" \
   --runtime-handler="${RUNTIME}" \
   --containerd-bin="${CONTAINERD_BIN}" \
   --image-list="${TEST_IMAGE_LIST:-}" && test_exit_code=$? || test_exit_code=$?