Merge pull request #814 from Random-Liu/cherrypick-#811

Fix empty volume ownership.

integration/images/volume-ownership/Dockerfile

@@ -0,0 +1,18 @@
+# Copyright 2018 The Containerd Authors.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+FROM busybox
+RUN mkdir -p /test_dir && \
+    chown -R nobody:nogroup /test_dir
+VOLUME /test_dir

integration/images/volume-ownership/Makefile

@@ -0,0 +1,27 @@
+# Copyright 2018 The Containerd Authors.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+all: build
+
+PROJ=gcr.io/k8s-cri-containerd
+VERSION=1.0
+IMAGE=$(PROJ)/volume-ownership:$(VERSION)
+
+build:
+	docker build -t $(IMAGE) .
+
+push:
+	gcloud docker -- push $(IMAGE)
+
+.PHONY: build push

integration/volume_copy_up_test.go

@@ -70,7 +70,7 @@ func TestVolumeCopyUp(t *testing.T) {
 	assert.Equal(t, "test_content\n", string(stdout))
 
 	t.Logf("Check host path of the volume")
-	hostCmd := fmt.Sprintf("ls %s/containers/%s/volumes/*/test_file | xargs cat", *criRoot, cn)
+	hostCmd := fmt.Sprintf("find %s/containers/%s/volumes/*/test_file | xargs cat", *criRoot, cn)
 	output, err := exec.Command("sh", "-c", hostCmd).CombinedOutput()
 	require.NoError(t, err)
 	assert.Equal(t, "test_content\n", string(output))
@@ -88,3 +88,51 @@ func TestVolumeCopyUp(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, "new_content\n", string(output))
 }
+
+func TestVolumeOwnership(t *testing.T) {
+	const (
+		testImage   = "gcr.io/k8s-cri-containerd/volume-ownership:1.0"
+		execTimeout = time.Minute
+	)
+
+	t.Logf("Create a sandbox")
+	sbConfig := PodSandboxConfig("sandbox", "volume-ownership")
+	sb, err := runtimeService.RunPodSandbox(sbConfig)
+	require.NoError(t, err)
+	defer func() {
+		assert.NoError(t, runtimeService.StopPodSandbox(sb))
+		assert.NoError(t, runtimeService.RemovePodSandbox(sb))
+	}()
+
+	t.Logf("Pull test image")
+	_, err = imageService.PullImage(&runtime.ImageSpec{Image: testImage}, nil)
+	require.NoError(t, err)
+
+	t.Logf("Create a container with volume-ownership test image")
+	cnConfig := ContainerConfig(
+		"container",
+		testImage,
+		WithCommand("tail", "-f", "/dev/null"),
+	)
+	cn, err := runtimeService.CreateContainer(sb, cnConfig, sbConfig)
+	require.NoError(t, err)
+
+	t.Logf("Start the container")
+	require.NoError(t, runtimeService.StartContainer(cn))
+
+	// gcr.io/k8s-cri-containerd/volume-ownership:1.0 contains a test_dir
+	// volume, which is owned by nobody:nogroup.
+	t.Logf("Check ownership of test directory inside container")
+	stdout, stderr, err := runtimeService.ExecSync(cn, []string{
+		"stat", "-c", "%U:%G", "/test_dir",
+	}, execTimeout)
+	require.NoError(t, err)
+	assert.Empty(t, stderr)
+	assert.Equal(t, "nobody:nogroup\n", string(stdout))
+
+	t.Logf("Check ownership of test directory on the host")
+	hostCmd := fmt.Sprintf("find %s/containers/%s/volumes/* | xargs stat -c %%U:%%G", *criRoot, cn)
+	output, err := exec.Command("sh", "-c", hostCmd).CombinedOutput()
+	require.NoError(t, err)
+	assert.Equal(t, "nobody:nogroup\n", string(output))
+}

pkg/containerd/opts/container.go

@@ -53,7 +53,6 @@ func WithNewSnapshot(id string, i containerd.Image) containerd.NewContainerOpts
 // 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.
-// TODO(random-liu): Figure out whether we need to copy volume content.
 func WithVolumes(volumeMounts map[string]string) containerd.NewContainerOpts {
 	return func(ctx context.Context, client *containerd.Client, c *containers.Container) (err error) {
 		if c.Snapshotter == "" {
@@ -108,14 +107,6 @@ func WithVolumes(volumeMounts map[string]string) containerd.NewContainerOpts {
 // copyExistingContents copies from the source to the destination and
 // ensures the ownership is appropriately set.
 func copyExistingContents(source, destination string) error {
-	srcList, err := ioutil.ReadDir(source)
-	if err != nil {
-		return err
-	}
-	if len(srcList) == 0 {
-		// Skip copying if source directory is empty.
-		return nil
-	}
 	dstList, err := ioutil.ReadDir(destination)
 	if err != nil {
 		return err