From f286d71166ae80c77a39830d4b74125c78f72ea6 Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Wed, 18 Oct 2023 14:33:39 -0700 Subject: [PATCH] Delete lifecycle mount annotation. Earlier, NewPodMountHints() was modifying the mount hints based on the lifecycle. This change moves that annotation modification work to runsc shim. It is more consistent for the shim to do all OCI spec modification work. Also added more documentation about how EmptyDir is optimized in runsc. This also allows us to delete the lifecycle mount annotation as it is no longer used anywhere else. PiperOrigin-RevId: 574600949 --- pkg/shim/utils/volumes.go | 45 +++++++++------- pkg/shim/utils/volumes_test.go | 61 ++++++++++------------ runsc/boot/mount_hints.go | 68 ++----------------------- runsc/boot/mount_hints_test.go | 17 ++----- runsc/boot/vfs_test.go | 21 ++++---- runsc/container/multi_container_test.go | 3 +- 6 files changed, 74 insertions(+), 141 deletions(-) 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, ",") }