From 70c193132dc84d85964d8eee0e383dffd9723863 Mon Sep 17 00:00:00 2001 From: Archana Shinde Date: Fri, 22 Mar 2019 16:45:27 -0700 Subject: [PATCH 1/2] mounts: Add check for system volumes We handle system directories differently, if its a bind mount we mount the guest system directory to the container mount and skip the 9p share mount. However, we should not do this for docker volumes which are directories created by Docker. This introduces a Docker specific check, but that is the only information available to us at the OCI layer. Signed-off-by: Archana Shinde --- virtcontainers/container.go | 6 +++++- virtcontainers/mount.go | 16 ++++++++++++++++ virtcontainers/mount_test.go | 10 ++++++++++ 3 files changed, 31 insertions(+), 1 deletion(-) diff --git a/virtcontainers/container.go b/virtcontainers/container.go index 337d07dec7..75fcdc98f2 100644 --- a/virtcontainers/container.go +++ b/virtcontainers/container.go @@ -477,7 +477,11 @@ func (c *Container) mountSharedDirMounts(hostSharedDir, guestSharedDir string) ( var sharedDirMounts []Mount var ignoredMounts []Mount for idx, m := range c.mounts { - if isSystemMount(m.Destination) || m.Type != "bind" { + if isSystemMount(m.Destination) && !IsDockerVolume(m.Source) { + continue + } + + if m.Type != "bind" { continue } diff --git a/virtcontainers/mount.go b/virtcontainers/mount.go index 5ffa7c5713..6dc6b62f38 100644 --- a/virtcontainers/mount.go +++ b/virtcontainers/mount.go @@ -326,3 +326,19 @@ func bindUnmountAllRootfs(ctx context.Context, sharedDir string, sandbox *Sandbo } } } + +const ( + dockerVolumePrefix = "/var/lib/docker/volumes" + dockerVolumeSuffix = "_data" +) + +// IsDockerVolume returns true if the given source path is +// a docker volume. +// This uses a very specific path that is used by docker. +func IsDockerVolume(path string) bool { + if strings.HasPrefix(path, dockerVolumePrefix) && filepath.Base(path) == dockerVolumeSuffix { + return true + } + return false +} + diff --git a/virtcontainers/mount_test.go b/virtcontainers/mount_test.go index 820fd7447c..f39fa3c489 100644 --- a/virtcontainers/mount_test.go +++ b/virtcontainers/mount_test.go @@ -282,3 +282,13 @@ func TestIsDeviceMapper(t *testing.T) { t.Fatal() } } + +func TestIsDockerVolume(t *testing.T) { + path := "/var/lib/docker/volumes/00da1347c7cf4f15db35f/_data" + isDockerVolume := IsDockerVolume(path) + assert.True(t, isDockerVolume) + + path = "/var/lib/testdir" + isDockerVolume := IsDockerVolume(path) + assert.False(t, isDockerVolume) +} From 228d1512d9273b3513e53d6305c598dbad9d7dd0 Mon Sep 17 00:00:00 2001 From: Archana Shinde Date: Mon, 25 Mar 2019 10:44:13 -0700 Subject: [PATCH 2/2] mount: Add check for k8s host empty directory k8s host empty-dir is equivalent to docker volumes. For this case, we should just use the host directory even for system directories. Move the isEphemeral function to virtcontainers to not introduce cyclic dependency. Fixes #1417 Signed-off-by: Archana Shinde --- pkg/katautils/create.go | 2 +- pkg/katautils/create_test.go | 2 +- pkg/katautils/utils.go | 28 -------------------- pkg/katautils/utils_test.go | 31 ---------------------- virtcontainers/container.go | 6 +++-- virtcontainers/mount.go | 50 ++++++++++++++++++++++++++++++++++++ virtcontainers/mount_test.go | 42 +++++++++++++++++++++++++++++- 7 files changed, 97 insertions(+), 64 deletions(-) diff --git a/pkg/katautils/create.go b/pkg/katautils/create.go index 63fab5c859..630b32c991 100644 --- a/pkg/katautils/create.go +++ b/pkg/katautils/create.go @@ -161,7 +161,7 @@ func HandleFactory(ctx context.Context, vci vc.VC, runtimeConfig *oci.RuntimeCon // of the same pod the already existing volume is reused. func SetEphemeralStorageType(ociSpec oci.CompatOCISpec) oci.CompatOCISpec { for idx, mnt := range ociSpec.Mounts { - if IsEphemeralStorage(mnt.Source) { + if vc.IsEphemeralStorage(mnt.Source) { ociSpec.Mounts[idx].Type = "ephemeral" } } diff --git a/pkg/katautils/create_test.go b/pkg/katautils/create_test.go index 3caa54d3e2..9a5c7d5f3c 100644 --- a/pkg/katautils/create_test.go +++ b/pkg/katautils/create_test.go @@ -190,7 +190,7 @@ func TestSetEphemeralStorageType(t *testing.T) { } defer os.RemoveAll(dir) - ephePath := filepath.Join(dir, k8sEmptyDir, "tmp-volume") + ephePath := filepath.Join(dir, vc.K8sEmptyDir, "tmp-volume") err = os.MkdirAll(ephePath, testDirMode) assert.Nil(err) diff --git a/pkg/katautils/utils.go b/pkg/katautils/utils.go index ae3ccecd36..12ab107fad 100644 --- a/pkg/katautils/utils.go +++ b/pkg/katautils/utils.go @@ -14,12 +14,6 @@ import ( "path/filepath" "strings" "syscall" - - vc "github.com/kata-containers/runtime/virtcontainers" -) - -const ( - k8sEmptyDir = "kubernetes.io~empty-dir" ) // FileExists test is a file exiting or not @@ -31,28 +25,6 @@ func FileExists(path string) bool { return true } -// IsEphemeralStorage returns true if the given path -// to the storage belongs to kubernetes ephemeral storage -// -// This method depends on a specific path used by k8s -// to detect if it's of type ephemeral. As of now, -// this is a very k8s specific solution that works -// but in future there should be a better way for this -// method to determine if the path is for ephemeral -// volume type -func IsEphemeralStorage(path string) bool { - splitSourceSlice := strings.Split(path, "/") - if len(splitSourceSlice) > 1 { - storageType := splitSourceSlice[len(splitSourceSlice)-2] - if storageType == k8sEmptyDir { - if _, fsType, _ := vc.GetDevicePathAndFsType(path); fsType == "tmpfs" { - return true - } - } - } - return false -} - // ResolvePath returns the fully resolved and expanded value of the // specified path. func ResolvePath(path string) (string, error) { diff --git a/pkg/katautils/utils_test.go b/pkg/katautils/utils_test.go index 4a1d7a5f11..af2238894e 100644 --- a/pkg/katautils/utils_test.go +++ b/pkg/katautils/utils_test.go @@ -366,34 +366,3 @@ func TestGetFileContents(t *testing.T) { assert.Equal(t, contents, d.contents) } } - -func TestIsEphemeralStorage(t *testing.T) { - if os.Geteuid() != 0 { - t.Skip(testDisabledNeedRoot) - } - - dir, err := ioutil.TempDir(testDir, "foo") - if err != nil { - t.Fatal(err) - } - defer os.RemoveAll(dir) - - sampleEphePath := filepath.Join(dir, k8sEmptyDir, "tmp-volume") - err = os.MkdirAll(sampleEphePath, testDirMode) - assert.Nil(t, err) - - err = syscall.Mount("tmpfs", sampleEphePath, "tmpfs", 0, "") - assert.Nil(t, err) - defer syscall.Unmount(sampleEphePath, 0) - - isEphe := IsEphemeralStorage(sampleEphePath) - if !isEphe { - t.Fatalf("Unable to correctly determine volume type") - } - - sampleEphePath = "/var/lib/kubelet/pods/366c3a75-4869-11e8-b479-507b9ddd5ce4/volumes/cache-volume" - isEphe = IsEphemeralStorage(sampleEphePath) - if isEphe { - t.Fatalf("Unable to correctly determine volume type") - } -} diff --git a/virtcontainers/container.go b/virtcontainers/container.go index 75fcdc98f2..f77662beda 100644 --- a/virtcontainers/container.go +++ b/virtcontainers/container.go @@ -477,8 +477,10 @@ func (c *Container) mountSharedDirMounts(hostSharedDir, guestSharedDir string) ( var sharedDirMounts []Mount var ignoredMounts []Mount for idx, m := range c.mounts { - if isSystemMount(m.Destination) && !IsDockerVolume(m.Source) { - continue + if isSystemMount(m.Destination) { + if !(IsDockerVolume(m.Source) || Isk8sHostEmptyDir(m.Source)) { + continue + } } if m.Type != "bind" { diff --git a/virtcontainers/mount.go b/virtcontainers/mount.go index 6dc6b62f38..c5f71cc211 100644 --- a/virtcontainers/mount.go +++ b/virtcontainers/mount.go @@ -342,3 +342,53 @@ func IsDockerVolume(path string) bool { return false } +const ( + // K8sEmptyDir is the k8s specific path for `empty-dir` volumes + K8sEmptyDir = "kubernetes.io~empty-dir" +) + +// IsEphemeralStorage returns true if the given path +// to the storage belongs to kubernetes ephemeral storage +// +// This method depends on a specific path used by k8s +// to detect if it's of type ephemeral. As of now, +// this is a very k8s specific solution that works +// but in future there should be a better way for this +// method to determine if the path is for ephemeral +// volume type +func IsEphemeralStorage(path string) bool { + if !isEmptyDir(path) { + return false + } + + if _, fsType, _ := GetDevicePathAndFsType(path); fsType == "tmpfs" { + return true + } + + return false +} + +// Isk8sHostEmptyDir returns true if the given path +// to the storage belongs to kubernetes empty-dir of medium "default" +// i.e volumes that are directories on the host. +func Isk8sHostEmptyDir(path string) bool { + if !isEmptyDir(path) { + return false + } + + if _, fsType, _ := GetDevicePathAndFsType(path); fsType != "tmpfs" { + return true + } + return false +} + +func isEmptyDir(path string) bool { + splitSourceSlice := strings.Split(path, "/") + if len(splitSourceSlice) > 1 { + storageType := splitSourceSlice[len(splitSourceSlice)-2] + if storageType == K8sEmptyDir { + return true + } + } + return false +} diff --git a/virtcontainers/mount_test.go b/virtcontainers/mount_test.go index f39fa3c489..7d0b3c4768 100644 --- a/virtcontainers/mount_test.go +++ b/virtcontainers/mount_test.go @@ -9,6 +9,8 @@ import ( "bytes" "context" "fmt" + "github.com/stretchr/testify/assert" + "io/ioutil" "os" "os/exec" "path/filepath" @@ -18,6 +20,11 @@ import ( "testing" ) +const ( + testDisabledNeedRoot = "Test disabled as requires root user" + testDirMode = os.FileMode(0750) +) + func TestIsSystemMount(t *testing.T) { tests := []struct { mnt string @@ -289,6 +296,39 @@ func TestIsDockerVolume(t *testing.T) { assert.True(t, isDockerVolume) path = "/var/lib/testdir" - isDockerVolume := IsDockerVolume(path) + isDockerVolume = IsDockerVolume(path) assert.False(t, isDockerVolume) } + +func TestIsEphemeralStorage(t *testing.T) { + if os.Geteuid() != 0 { + t.Skip(testDisabledNeedRoot) + } + + dir, err := ioutil.TempDir(testDir, "foo") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dir) + + sampleEphePath := filepath.Join(dir, k8sEmptyDir, "tmp-volume") + err = os.MkdirAll(sampleEphePath, testDirMode) + assert.Nil(t, err) + + err = syscall.Mount("tmpfs", sampleEphePath, "tmpfs", 0, "") + assert.Nil(t, err) + defer syscall.Unmount(sampleEphePath, 0) + + isEphe := IsEphemeralStorage(sampleEphePath) + assert.True(t, isEphe) + + isHostEmptyDir := Isk8sHostEmptyDir(sampleEphePath) + assert.False(t, isHostEmptyDir) + + sampleEphePath = "/var/lib/kubelet/pods/366c3a75-4869-11e8-b479-507b9ddd5ce4/volumes/cache-volume" + isEphe = IsEphemeralStorage(sampleEphePath) + assert.False(t, isEphe) + + isHostEmptyDir = Isk8sHostEmptyDir(sampleEphePath) + assert.False(t, isHostEmptyDir) +}