diff --git a/pkg/shim/utils/volumes.go b/pkg/shim/utils/volumes.go index de776e653..cc575263a 100644 --- a/pkg/shim/utils/volumes.go +++ b/pkg/shim/utils/volumes.go @@ -81,11 +81,6 @@ func volumeSourceKey(volume string) string { return volumeKeyPrefix + volume + ".source" } -// volumeLifecycleKey constructs the annotation key for volume lifecycle. -func volumeLifecycleKey(volume string) string { - return volumeKeyPrefix + volume + ".lifecycle" -} - // volumePath searches the volume path in the kubelet pod directory. func volumePath(volume, uid string) (string, error) { // TODO: Support subpath when gvisor supports pod volume bind mount. @@ -109,6 +104,23 @@ func isVolumePath(volume, path string) (bool, error) { // UpdateVolumeAnnotations add necessary OCI annotations for gvisor // volume optimization. Returns true if the spec was modified. +// +// Note about EmptyDir handling: +// The admission controller sets mount annotations for EmptyDir as follows: +// - For EmptyDir volumes with medium=Memory, the "type" field is set to tmpfs. +// - For EmptyDir volumes with medium="", the "type" field is set to bind. +// +// The container spec has EmptyDir mount points as bind mounts. This method +// modifies the spec as follows: +// - The "type" mount annotation for all EmptyDirs is changed to tmpfs. +// - The mount type in spec.Mounts[i].Type is changed as follows: +// - For EmptyDir volumes with medium=Memory, we change it to tmpfs. +// - For EmptyDir volumes with medium="", we leave it as a bind mount. +// - (Essentially we set it to what the admission controller said.) +// +// runsc should use these two setting to infer EmptyDir medium: +// - tmpfs annotation type + tmpfs mount type = memory-backed EmptyDir +// - tmpfs annotation type + bind mount type = disk-backed EmptyDir func UpdateVolumeAnnotations(s *specs.Spec) (bool, error) { var uid string if IsSandbox(s) { @@ -130,25 +142,22 @@ func UpdateVolumeAnnotations(s *specs.Spec) (bool, error) { } volume := volumeName(k) if uid != "" { - // This is a sandbox. Add source and lifecycle annotations for volumes. + // This is the root (first) container. Mount annotations are only + // consumed from this container's spec. So fix mount annotations by: + // 1. Adding source annotation. + // 2. Fixing type annotation. path, err := volumePath(volume, uid) if err != nil { return false, fmt.Errorf("get volume path for %q: %w", volume, err) } s.Annotations[volumeSourceKey(volume)] = path - // TODO(b/142076984): Remove the lifecycle setting logic after it has - // been adopted in GKE admission plugin. - lifecycleKey := volumeLifecycleKey(volume) - if _, ok := s.Annotations[lifecycleKey]; !ok { - // Only set lifecycle annotation if not already set. - if strings.Contains(path, emptyDirVolumesDir) { - // Emptydir is created and destroyed with the pod. - s.Annotations[lifecycleKey] = "pod" - } + if strings.Contains(path, emptyDirVolumesDir) { + s.Annotations[k] = "tmpfs" // See note about EmptyDir. } updated = true } else { - // This is a container. + // This is a sub-container. Mount annotations are ignored. So no need to + // bother fixing those. for i := range s.Mounts { // An error is returned for sandbox if source annotation is not // successfully applied, so it is guaranteed that the source annotation @@ -160,7 +169,8 @@ func UpdateVolumeAnnotations(s *specs.Spec) (bool, error) { // TODO: Pass podUID down to shim for containers to do more accurate // matching. if yes, _ := isVolumePath(volume, s.Mounts[i].Source); yes { - // Container mount type must match the sandbox's mount type. + // Container mount type must match the mount type specified by + // admission controller. See note about EmptyDir. specutils.ChangeMountType(&s.Mounts[i], v) updated = true } @@ -211,7 +221,6 @@ func configureShm(s *specs.Spec) (bool, error) { s.Annotations[volumeKeyPrefix+devshmName+".source"] = m.Source s.Annotations[volumeKeyPrefix+devshmName+".type"] = devshmType s.Annotations[volumeKeyPrefix+devshmName+".share"] = "pod" - s.Annotations[volumeKeyPrefix+devshmName+".lifecycle"] = "pod" // Given that we don't have visibility into mount options for all // containers, assume broad access for the master mount (it's tmpfs // inside the sandbox anyways) and apply options to subcontainers as diff --git a/pkg/shim/utils/volumes_test.go b/pkg/shim/utils/volumes_test.go index 2e09098e5..aadb9533c 100644 --- a/pkg/shim/utils/volumes_test.go +++ b/pkg/shim/utils/volumes_test.go @@ -64,13 +64,12 @@ func TestUpdateVolumeAnnotations(t *testing.T) { }, expected: &specs.Spec{ Annotations: map[string]string{ - sandboxLogDirAnnotation: testLogDirPath, - ContainerTypeAnnotation: containerTypeSandbox, - volumeKeyPrefix + testVolumeName + ".share": "pod", - volumeKeyPrefix + testVolumeName + ".type": "tmpfs", - volumeKeyPrefix + testVolumeName + ".options": "ro", - volumeKeyPrefix + testVolumeName + ".source": testVolumePath, - volumeKeyPrefix + testVolumeName + ".lifecycle": "pod", + sandboxLogDirAnnotation: testLogDirPath, + ContainerTypeAnnotation: containerTypeSandbox, + volumeKeyPrefix + testVolumeName + ".share": "pod", + volumeKeyPrefix + testVolumeName + ".type": "tmpfs", + volumeKeyPrefix + testVolumeName + ".options": "ro", + volumeKeyPrefix + testVolumeName + ".source": testVolumePath, }, }, expectUpdate: true, @@ -88,13 +87,12 @@ func TestUpdateVolumeAnnotations(t *testing.T) { }, expected: &specs.Spec{ Annotations: map[string]string{ - sandboxLogDirAnnotation: testLegacyLogDirPath, - ContainerTypeAnnotation: containerTypeSandbox, - volumeKeyPrefix + testVolumeName + ".share": "pod", - volumeKeyPrefix + testVolumeName + ".type": "tmpfs", - volumeKeyPrefix + testVolumeName + ".options": "ro", - volumeKeyPrefix + testVolumeName + ".source": testVolumePath, - volumeKeyPrefix + testVolumeName + ".lifecycle": "pod", + sandboxLogDirAnnotation: testLegacyLogDirPath, + ContainerTypeAnnotation: containerTypeSandbox, + volumeKeyPrefix + testVolumeName + ".share": "pod", + volumeKeyPrefix + testVolumeName + ".type": "tmpfs", + volumeKeyPrefix + testVolumeName + ".options": "ro", + volumeKeyPrefix + testVolumeName + ".source": testVolumePath, }, }, expectUpdate: true, @@ -275,12 +273,11 @@ func TestUpdateVolumeAnnotations(t *testing.T) { name: "bind options removed", spec: &specs.Spec{ Annotations: map[string]string{ - ContainerTypeAnnotation: ContainerTypeContainer, - volumeKeyPrefix + testVolumeName + ".share": "pod", - volumeKeyPrefix + testVolumeName + ".type": "tmpfs", - volumeKeyPrefix + testVolumeName + ".options": "ro", - volumeKeyPrefix + testVolumeName + ".source": testVolumePath, - volumeKeyPrefix + testVolumeName + ".lifecycle": "pod", + ContainerTypeAnnotation: ContainerTypeContainer, + volumeKeyPrefix + testVolumeName + ".share": "pod", + volumeKeyPrefix + testVolumeName + ".type": "tmpfs", + volumeKeyPrefix + testVolumeName + ".options": "ro", + volumeKeyPrefix + testVolumeName + ".source": testVolumePath, }, Mounts: []specs.Mount{ { @@ -293,12 +290,11 @@ func TestUpdateVolumeAnnotations(t *testing.T) { }, expected: &specs.Spec{ Annotations: map[string]string{ - ContainerTypeAnnotation: ContainerTypeContainer, - volumeKeyPrefix + testVolumeName + ".share": "pod", - volumeKeyPrefix + testVolumeName + ".type": "tmpfs", - volumeKeyPrefix + testVolumeName + ".options": "ro", - volumeKeyPrefix + testVolumeName + ".source": testVolumePath, - volumeKeyPrefix + testVolumeName + ".lifecycle": "pod", + ContainerTypeAnnotation: ContainerTypeContainer, + volumeKeyPrefix + testVolumeName + ".share": "pod", + volumeKeyPrefix + testVolumeName + ".type": "tmpfs", + volumeKeyPrefix + testVolumeName + ".options": "ro", + volumeKeyPrefix + testVolumeName + ".source": testVolumePath, }, Mounts: []specs.Mount{ { @@ -329,13 +325,12 @@ func TestUpdateVolumeAnnotations(t *testing.T) { }, expected: &specs.Spec{ Annotations: map[string]string{ - sandboxLogDirAnnotation: testLogDirPath, - ContainerTypeAnnotation: containerTypeSandbox, - volumeKeyPrefix + devshmName + ".share": "pod", - volumeKeyPrefix + devshmName + ".type": "tmpfs", - volumeKeyPrefix + devshmName + ".options": "rw", - volumeKeyPrefix + devshmName + ".source": testVolumePath, - volumeKeyPrefix + devshmName + ".lifecycle": "pod", + sandboxLogDirAnnotation: testLogDirPath, + ContainerTypeAnnotation: containerTypeSandbox, + volumeKeyPrefix + devshmName + ".share": "pod", + volumeKeyPrefix + devshmName + ".type": "tmpfs", + volumeKeyPrefix + devshmName + ".options": "rw", + volumeKeyPrefix + devshmName + ".source": testVolumePath, }, Mounts: []specs.Mount{ { diff --git a/runsc/boot/mount_hints.go b/runsc/boot/mount_hints.go index 036018e6b..4760ce82c 100644 --- a/runsc/boot/mount_hints.go +++ b/runsc/boot/mount_hints.go @@ -62,37 +62,6 @@ func (s ShareType) String() string { } } -// LifecycleType indicates whether creation/deletion of the volume is tied to -// the pod or container's lifecycle. -type LifecycleType int - -const ( - // sharedLife indicates that the volume's lifecycle is not tied to the pod. - // The volume persists beyond the pod's life. This is the safe default. - sharedLife LifecycleType = iota - - // podLife indicates that the volume's lifecycle is tied to the pod's - // lifecycle. The volume is destroyed with the pod. - podLife - - // containerLife indicates that the volume's lifecycle is tied to the - // container's lifecycle. The volume is destroyed with the container. - containerLife -) - -func (o LifecycleType) String() string { - switch o { - case sharedLife: - return "shared" - case podLife: - return "pod" - case containerLife: - return "container" - default: - return fmt.Sprintf("invalid lifecycle value %d", o) - } -} - // PodMountHints contains a collection of mountHints for the pod. type PodMountHints struct { Mounts map[string]*MountHint `json:"mounts"` @@ -141,20 +110,6 @@ func NewPodMountHints(spec *specs.Spec) (*PodMountHints, error) { } } - // Convert mount types. - for _, m := range mnts { - if m.Mount.Type == Bind && - // This mount is only accessed within the sandbox. - (m.Share == container || m.Share == pod) && - // The mount is created and deleted within the pod's lifecycle. - (m.Lifecycle == containerLife || m.Lifecycle == podLife) { - // Use a file-backed tmpfs mount for such a mount, because it is isolated - // to the sandbox and is empty on startup. - log.Infof("Converting %s hint to tmpfs", m.Name) - m.Mount.Type = tmpfs.Name - } - } - return &PodMountHints{Mounts: mnts}, nil } @@ -163,10 +118,9 @@ func NewPodMountHints(spec *specs.Spec) (*PodMountHints, error) { // so that mounts can be correctly shared inside the pod. // It is part of the sandbox.Sandbox struct, so it must be serializable. type MountHint struct { - Name string `json:"name"` - Share ShareType `json:"share"` - Mount specs.Mount `json:"mount"` - Lifecycle LifecycleType `json:"lifecycle"` + Name string `json:"name"` + Share ShareType `json:"share"` + Mount specs.Mount `json:"mount"` } func (m *MountHint) setField(key, val string) error { @@ -182,8 +136,6 @@ func (m *MountHint) setField(key, val string) error { return m.setShare(val) case "options": m.Mount.Options = specutils.FilterMountOptions(strings.Split(val, ",")) - case "lifecycle": - return m.setLifecycle(val) default: return fmt.Errorf("invalid mount annotation: %s=%s", key, val) } @@ -214,20 +166,6 @@ func (m *MountHint) setShare(val string) error { return nil } -func (m *MountHint) setLifecycle(val string) error { - switch val { - case containerLife.String(): - m.Lifecycle = containerLife - case podLife.String(): - m.Lifecycle = podLife - case sharedLife.String(): - m.Lifecycle = sharedLife - default: - return fmt.Errorf("invalid lifecycle %q", val) - } - return nil -} - // ShouldShareMount returns true if this mount should be configured as a shared // mount that is shared among multiple containers in a pod. func (m *MountHint) ShouldShareMount() bool { diff --git a/runsc/boot/mount_hints_test.go b/runsc/boot/mount_hints_test.go index 395c2808d..682e03f36 100644 --- a/runsc/boot/mount_hints_test.go +++ b/runsc/boot/mount_hints_test.go @@ -29,11 +29,10 @@ func TestPodMountHintsHappy(t *testing.T) { MountPrefix + "mount1.type": "tmpfs", MountPrefix + "mount1.share": "pod", - MountPrefix + "mount2.source": "bar", - MountPrefix + "mount2.type": "bind", - MountPrefix + "mount2.share": "container", - MountPrefix + "mount2.options": "rw,private", - MountPrefix + "mount2.lifecycle": "pod", + MountPrefix + "mount2.source": "bar", + MountPrefix + "mount2.type": "bind", + MountPrefix + "mount2.share": "container", + MountPrefix + "mount2.options": "rw,private", }, } podHints, err := NewPodMountHints(spec) @@ -58,9 +57,6 @@ func TestPodMountHintsHappy(t *testing.T) { if want := []string(nil); !reflect.DeepEqual(want, mount1.Mount.Options) { t.Errorf("mount1 type, want: %q, got: %q", want, mount1.Mount.Options) } - if want := sharedLife; want != mount1.Lifecycle { - t.Errorf("mount1 lifecycle, want: %q, got: %q", want, mount1.Lifecycle) - } mount2 := podHints.Mounts["mount2"] if want := "mount2"; want != mount2.Name { @@ -69,7 +65,7 @@ func TestPodMountHintsHappy(t *testing.T) { if want := "bar"; want != mount2.Mount.Source { t.Errorf("mount2 source, want: %q, got: %q", want, mount2.Mount.Source) } - if want := "tmpfs"; want != mount2.Mount.Type { + if want := "bind"; want != mount2.Mount.Type { t.Errorf("mount2 type, want: %q, got: %q", want, mount2.Mount.Type) } if want := container; want != mount2.Share { @@ -78,9 +74,6 @@ func TestPodMountHintsHappy(t *testing.T) { if want := []string{"rw", "private"}; !reflect.DeepEqual(want, mount2.Mount.Options) { t.Errorf("mount2 type, want: %q, got: %q", want, mount2.Mount.Options) } - if want := podLife; want != mount2.Lifecycle { - t.Errorf("mount2 lifecycle, want: %q, got: %q", want, mount2.Lifecycle) - } } func TestPodMountHintsErrors(t *testing.T) { diff --git a/runsc/boot/vfs_test.go b/runsc/boot/vfs_test.go index 3322d3691..85380c489 100644 --- a/runsc/boot/vfs_test.go +++ b/runsc/boot/vfs_test.go @@ -47,17 +47,7 @@ func TestGetMountAccessType(t *testing.T) { want: config.FileAccessShared, }, { - name: "pod+podLife=shared", - annotations: map[string]string{ - MountPrefix + "mount1.source": source, - MountPrefix + "mount1.type": "bind", - MountPrefix + "mount1.share": "pod", - MountPrefix + "mount1.lifecycle": "pod", - }, - want: config.FileAccessExclusive, - }, - { - name: "shared=shared", + name: "share=shared", annotations: map[string]string{ MountPrefix + "mount1.source": source, MountPrefix + "mount1.type": "bind", @@ -74,6 +64,15 @@ func TestGetMountAccessType(t *testing.T) { }, want: config.FileAccessShared, }, + { + name: "tmpfs+container=exclusive", + annotations: map[string]string{ + MountPrefix + "mount1.source": source, + MountPrefix + "mount1.type": "tmpfs", + MountPrefix + "mount1.share": "container", + }, + want: config.FileAccessExclusive, + }, { name: "tmpfs+pod=exclusive", annotations: map[string]string{ diff --git a/runsc/container/multi_container_test.go b/runsc/container/multi_container_test.go index 961c8c5b8..b99b0b58f 100644 --- a/runsc/container/multi_container_test.go +++ b/runsc/container/multi_container_test.go @@ -131,9 +131,8 @@ func createSharedMount(mount specs.Mount, name string, pod ...*specs.Spec) { } for _, spec := range pod { spec.Annotations[boot.MountPrefix+name+".source"] = mount.Source - spec.Annotations[boot.MountPrefix+name+".type"] = mount.Type + spec.Annotations[boot.MountPrefix+name+".type"] = "tmpfs" spec.Annotations[boot.MountPrefix+name+".share"] = share - spec.Annotations[boot.MountPrefix+name+".lifecycle"] = "pod" if len(mount.Options) > 0 { spec.Annotations[boot.MountPrefix+name+".options"] = strings.Join(mount.Options, ",") }