Merge pull request #99446 from pohly/generic-ephemeral-enablement

Generic ephemeral volume enablement

pkg/api/pod/util.go

@@ -566,7 +566,7 @@ func dropDisabledCSIVolumeSourceAlphaFields(podSpec, oldPodSpec *api.PodSpec) {
 // dropDisabledEphemeralVolumeSourceAlphaFields removes disabled alpha fields from []EphemeralVolumeSource.
 // This should be called from PrepareForCreate/PrepareForUpdate for all pod specs resources containing a EphemeralVolumeSource
 func dropDisabledEphemeralVolumeSourceAlphaFields(podSpec, oldPodSpec *api.PodSpec) {
-	if !utilfeature.DefaultFeatureGate.Enabled(features.GenericEphemeralVolume) && !csiInUse(oldPodSpec) {
+	if !utilfeature.DefaultFeatureGate.Enabled(features.GenericEphemeralVolume) && !ephemeralInUse(oldPodSpec) {
 		for i := range podSpec.Volumes {
 			podSpec.Volumes[i].Ephemeral = nil
 		}
@@ -772,6 +772,19 @@ func csiInUse(podSpec *api.PodSpec) bool {
 	return false
 }
 
+// ephemeralInUse returns true if any pod's spec include inline CSI volumes.
+func ephemeralInUse(podSpec *api.PodSpec) bool {
+	if podSpec == nil {
+		return false
+	}
+	for i := range podSpec.Volumes {
+		if podSpec.Volumes[i].Ephemeral != nil {
+			return true
+		}
+	}
+	return false
+}
+
 // podPriorityInUse returns true if status is not nil and number of PodIPs is greater than one
 func multiplePodIPsInUse(podStatus *api.PodStatus) bool {
 	if podStatus == nil {

pkg/controller/volume/scheduling/scheduler_binder.go

@@ -674,8 +674,13 @@ func (b *volumeBinder) isVolumeBound(pod *v1.Pod, vol *v1.Volume) (bound bool, p
 	switch {
 	case vol.PersistentVolumeClaim != nil:
 		pvcName = vol.PersistentVolumeClaim.ClaimName
-	case vol.Ephemeral != nil &&
+	case vol.Ephemeral != nil:
-		utilfeature.DefaultFeatureGate.Enabled(features.GenericEphemeralVolume):
+		if !utilfeature.DefaultFeatureGate.Enabled(features.GenericEphemeralVolume) {
+			return false, nil, fmt.Errorf(
+				"volume %s is a generic ephemeral volume, but that feature is disabled in kube-scheduler",
+				vol.Name,
+			)
+		}
 		// Generic ephemeral inline volumes also use a PVC,
 		// just with a computed name, and...
 		pvcName = pod.Name + "-" + vol.Name

pkg/controller/volume/scheduling/scheduler_binder_test.go

@@ -77,6 +77,11 @@ var (
 	boundMigrationPVC     = makeTestPVC("pvc-migration-bound", "1G", "", pvcBound, "pv-migration-bound", "1", &waitClass)
 	provMigrationPVCBound = makeTestPVC("pvc-migration-provisioned", "1Gi", "", pvcBound, "pv-migration-bound", "1", &waitClassWithProvisioner)
 
+	// PVCs and PV for GenericEphemeralVolume
+	conflictingGenericPVC = makeGenericEphemeralPVC("test-volume", false /* not owned*/)
+	correctGenericPVC     = makeGenericEphemeralPVC("test-volume", true /* owned */)
+	pvBoundGeneric        = makeTestPV("pv-bound", "node1", "1G", "1", correctGenericPVC, waitClass)
+
 	// PVs for manual binding
 	pvNode1a                   = makeTestPV("pv-node1a", "node1", "5G", "1", nil, waitClass)
 	pvNode1b                   = makeTestPV("pv-node1b", "node1", "10G", "1", nil, waitClass)
@@ -583,6 +588,22 @@ const (
 	pvcSelectedNode
 )
 
+func makeGenericEphemeralPVC(volumeName string, owned bool) *v1.PersistentVolumeClaim {
+	pod := makePodWithGenericEphemeral()
+	pvc := makeTestPVC(pod.Name+"-"+volumeName, "1G", "", pvcBound, "pv-bound", "1", &immediateClass)
+	if owned {
+		controller := true
+		pvc.OwnerReferences = []metav1.OwnerReference{
+			{
+				Name:       pod.Name,
+				UID:        pod.UID,
+				Controller: &controller,
+			},
+		}
+	}
+	return pvc
+}
+
 func makeTestPVC(name, size, node string, pvcBoundState int, pvName, resourceVersion string, className *string) *v1.PersistentVolumeClaim {
 	fs := v1.PersistentVolumeFilesystem
 	pvc := &v1.PersistentVolumeClaim{
@@ -784,6 +805,19 @@ func makePodWithoutPVC() *v1.Pod {
 	return pod
 }
 
+func makePodWithGenericEphemeral(volumeNames ...string) *v1.Pod {
+	pod := makePod(nil)
+	for _, volumeName := range volumeNames {
+		pod.Spec.Volumes = append(pod.Spec.Volumes, v1.Volume{
+			Name: volumeName,
+			VolumeSource: v1.VolumeSource{
+				Ephemeral: &v1.EphemeralVolumeSource{},
+			},
+		})
+	}
+	return pod
+}
+
 func makeBinding(pvc *v1.PersistentVolumeClaim, pv *v1.PersistentVolume) *BindingInfo {
 	return &BindingInfo{pvc: pvc.DeepCopy(), pv: pv.DeepCopy()}
 }
@@ -857,6 +891,9 @@ func TestFindPodVolumesWithoutProvisioning(t *testing.T) {
 		// If nil, makePod with podPVCs
 		pod *v1.Pod
 
+		// GenericEphemeralVolume feature enabled?
+		ephemeral bool
+
 		// Expected podBindingCache fields
 		expectedBindings []*BindingInfo
 
@@ -953,6 +990,31 @@ func TestFindPodVolumesWithoutProvisioning(t *testing.T) {
 			podPVCs:    []*v1.PersistentVolumeClaim{immediateUnboundPVC, unboundPVC},
 			shouldFail: true,
 		},
+		"generic-ephemeral,no-pvc": {
+			pod:        makePodWithGenericEphemeral("no-such-pvc"),
+			ephemeral:  true,
+			shouldFail: true,
+		},
+		"generic-ephemeral,with-pvc": {
+			pod:       makePodWithGenericEphemeral("test-volume"),
+			cachePVCs: []*v1.PersistentVolumeClaim{correctGenericPVC},
+			pvs:       []*v1.PersistentVolume{pvBoundGeneric},
+			ephemeral: true,
+		},
+		"generic-ephemeral,wrong-pvc": {
+			pod:        makePodWithGenericEphemeral("test-volume"),
+			cachePVCs:  []*v1.PersistentVolumeClaim{conflictingGenericPVC},
+			pvs:        []*v1.PersistentVolume{pvBoundGeneric},
+			ephemeral:  true,
+			shouldFail: true,
+		},
+		"generic-ephemeral,disabled": {
+			pod:        makePodWithGenericEphemeral("test-volume"),
+			cachePVCs:  []*v1.PersistentVolumeClaim{correctGenericPVC},
+			pvs:        []*v1.PersistentVolume{pvBoundGeneric},
+			ephemeral:  false,
+			shouldFail: true,
+		},
 	}
 
 	testNode := &v1.Node{
@@ -965,6 +1027,8 @@ func TestFindPodVolumesWithoutProvisioning(t *testing.T) {
 	}
 
 	run := func(t *testing.T, scenario scenarioType, csiStorageCapacity bool, csiDriver *storagev1.CSIDriver) {
+		defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.GenericEphemeralVolume, scenario.ephemeral)()
+
 		ctx, cancel := context.WithCancel(context.Background())
 		defer cancel()
 

pkg/kubelet/volumemanager/populator/desired_state_of_world_populator.go

@@ -512,8 +512,18 @@ func (dswp *desiredStateOfWorldPopulator) createVolumeSpec(
 	pvcSource := podVolume.VolumeSource.PersistentVolumeClaim
 	ephemeral := false
 	if pvcSource == nil &&
-		podVolume.VolumeSource.Ephemeral != nil &&
+		podVolume.VolumeSource.Ephemeral != nil {
-		utilfeature.DefaultFeatureGate.Enabled(features.GenericEphemeralVolume) {
+		if !utilfeature.DefaultFeatureGate.Enabled(features.GenericEphemeralVolume) {
+			// Provide an unambiguous error message that
+			// explains why the volume cannot be
+			// processed. If we just ignore the volume
+			// source, the error is just a vague "unknown
+			// volume source".
+			return nil, nil, "", fmt.Errorf(
+				"volume %s is a generic ephemeral volume, but that feature is disabled in kubelet",
+				podVolume.Name,
+			)
+		}
 		// Generic ephemeral inline volumes are handled the
 		// same way as a PVC reference. The only additional
 		// constraint (checked below) is that the PVC must be

pkg/kubelet/volumemanager/populator/desired_state_of_world_populator_test.go

@@ -22,6 +22,7 @@ import (
 
 	"fmt"
 
+	"github.com/stretchr/testify/require"
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -454,6 +455,124 @@ func TestFindAndRemoveNonattachableVolumes(t *testing.T) {
 	}
 }
 
+func TestEphemeralVolumeOwnerCheck(t *testing.T) {
+	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.GenericEphemeralVolume, true)()
+
+	// create dswp
+	pod, pv, pvc := createEphemeralVolumeObjects("dswp-test-pod", "dswp-test-volume-name", false /* not owned */)
+	dswp, fakePodManager, _ := createDswpWithVolume(t, pv, pvc)
+	fakePodManager.AddPod(pod)
+
+	podName := util.GetUniquePodName(pod)
+
+	dswp.findAndAddNewPods()
+	if dswp.pods.processedPods[podName] {
+		t.Fatalf("%s should not have been processed by the populator", podName)
+	}
+	require.Equal(t,
+		[]string{fmt.Sprintf("error processing PVC %s/%s: not the ephemeral PVC for the pod", pvc.Namespace, pvc.Name)},
+		dswp.desiredStateOfWorld.PopPodErrors(podName),
+	)
+}
+
+func TestEphemeralVolumeEnablement(t *testing.T) {
+	// create dswp
+	pod, pv, pvc := createEphemeralVolumeObjects("dswp-test-pod", "dswp-test-volume-name", true /* owned */)
+	dswp, fakePodManager, fakesDSW := createDswpWithVolume(t, pv, pvc)
+	fakePodManager.AddPod(pod)
+
+	podName := util.GetUniquePodName(pod)
+	volumeName := pod.Spec.Volumes[0].Name
+	generatedVolumeName := "fake-plugin/" + volumeName
+
+	// Feature disabled -> refuse to process pod.
+	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.GenericEphemeralVolume, false)()
+	dswp.findAndAddNewPods()
+	if dswp.pods.processedPods[podName] {
+		t.Fatalf("%s should not have been processed by the populator", podName)
+	}
+	require.Equal(t,
+		[]string{fmt.Sprintf("volume %s is a generic ephemeral volume, but that feature is disabled in kubelet", volumeName)},
+		dswp.desiredStateOfWorld.PopPodErrors(podName),
+	)
+
+	// Enabled -> process pod.
+	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.GenericEphemeralVolume, true)()
+	dswp.findAndAddNewPods()
+	if !dswp.pods.processedPods[podName] {
+		t.Fatalf("Failed to record that the volumes for the specified pod: %s have been processed by the populator", podName)
+	}
+
+	expectedVolumeName := v1.UniqueVolumeName(generatedVolumeName)
+
+	volumeExists := fakesDSW.VolumeExists(expectedVolumeName)
+	if !volumeExists {
+		t.Fatalf(
+			"VolumeExists(%q) failed. Expected: <true> Actual: <%v>",
+			expectedVolumeName,
+			volumeExists)
+	}
+
+	if podExistsInVolume := fakesDSW.PodExistsInVolume(
+		podName, expectedVolumeName); !podExistsInVolume {
+		t.Fatalf(
+			"DSW PodExistsInVolume returned incorrect value. Expected: <true> Actual: <%v>",
+			podExistsInVolume)
+	}
+
+	verifyVolumeExistsInVolumesToMount(
+		t, v1.UniqueVolumeName(generatedVolumeName), false /* expectReportedInUse */, fakesDSW)
+
+	//let the pod be terminated
+	podGet, exist := fakePodManager.GetPodByName(pod.Namespace, pod.Name)
+	if !exist {
+		t.Fatalf("Failed to get pod by pod name: %s and namespace: %s", pod.Name, pod.Namespace)
+	}
+	podGet.Status.Phase = v1.PodFailed
+
+	// Pretend again that the feature is disabled.
+	// Removal of the pod and volumes is expected to work.
+	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.GenericEphemeralVolume, false)()
+
+	dswp.findAndRemoveDeletedPods()
+	// Although Pod status is terminated, pod still exists in pod manager and actual state does not has this pod and volume information
+	// desired state populator will fail to delete this pod and volume first
+	volumeExists = fakesDSW.VolumeExists(expectedVolumeName)
+	if !volumeExists {
+		t.Fatalf(
+			"VolumeExists(%q) failed. Expected: <true> Actual: <%v>",
+			expectedVolumeName,
+			volumeExists)
+	}
+
+	if podExistsInVolume := fakesDSW.PodExistsInVolume(
+		podName, expectedVolumeName); !podExistsInVolume {
+		t.Fatalf(
+			"DSW PodExistsInVolume returned incorrect value. Expected: <true> Actual: <%v>",
+			podExistsInVolume)
+	}
+
+	// reconcile with actual state so that volume is added into the actual state
+	// desired state populator now can successfully delete the pod and volume
+	fakeASW := dswp.actualStateOfWorld
+	reconcileASW(fakeASW, fakesDSW, t)
+	dswp.findAndRemoveDeletedPods()
+	volumeExists = fakesDSW.VolumeExists(expectedVolumeName)
+	if volumeExists {
+		t.Fatalf(
+			"VolumeExists(%q) failed. Expected: <false> Actual: <%v>",
+			expectedVolumeName,
+			volumeExists)
+	}
+
+	if podExistsInVolume := fakesDSW.PodExistsInVolume(
+		podName, expectedVolumeName); podExistsInVolume {
+		t.Fatalf(
+			"DSW PodExistsInVolume returned incorrect value. Expected: <false> Actual: <%v>",
+			podExistsInVolume)
+	}
+}
+
 func TestFindAndAddNewPods_FindAndRemoveDeletedPods_Valid_Block_VolumeDevices(t *testing.T) {
 	// create dswp
 	mode := v1.PersistentVolumeBlock
@@ -1076,6 +1195,74 @@ func createPodWithVolume(pod, pv, pvc string, containers []v1.Container) *v1.Pod
 	}
 }
 
+func createEphemeralVolumeObjects(podName, volumeName string, owned bool) (pod *v1.Pod, pv *v1.PersistentVolume, pvc *v1.PersistentVolumeClaim) {
+	pod = &v1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      podName,
+			UID:       "dswp-test-pod-uid",
+			Namespace: "dswp-test",
+		},
+		Spec: v1.PodSpec{
+			Volumes: []v1.Volume{
+				{
+					Name: volumeName,
+					VolumeSource: v1.VolumeSource{
+						Ephemeral: &v1.EphemeralVolumeSource{
+							VolumeClaimTemplate: &v1.PersistentVolumeClaimTemplate{},
+						},
+					},
+				},
+			},
+			Containers: []v1.Container{
+				{
+					VolumeMounts: []v1.VolumeMount{
+						{
+							Name:      volumeName,
+							MountPath: "/mnt",
+						},
+					},
+				},
+			},
+		},
+		Status: v1.PodStatus{
+			Phase: v1.PodPhase("Running"),
+		},
+	}
+	mode := v1.PersistentVolumeFilesystem
+	pv = &v1.PersistentVolume{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: volumeName,
+		},
+		Spec: v1.PersistentVolumeSpec{
+			ClaimRef:   &v1.ObjectReference{Namespace: "ns", Name: "file-bound"},
+			VolumeMode: &mode,
+		},
+	}
+	pvc = &v1.PersistentVolumeClaim{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      podName + "-" + volumeName,
+			Namespace: pod.Namespace,
+		},
+		Spec: v1.PersistentVolumeClaimSpec{
+			VolumeName: "dswp-test-volume-name",
+		},
+		Status: v1.PersistentVolumeClaimStatus{
+			Phase: v1.ClaimBound,
+		},
+	}
+	if owned {
+		controller := true
+		pvc.OwnerReferences = []metav1.OwnerReference{
+			{
+				UID:        pod.UID,
+				Name:       podName,
+				Controller: &controller,
+			},
+		}
+	}
+	return
+}
+
 func createDswpWithVolume(t *testing.T, pv *v1.PersistentVolume, pvc *v1.PersistentVolumeClaim) (*desiredStateOfWorldPopulator, kubepod.Manager, cache.DesiredStateOfWorld) {
 	fakeVolumePluginMgr, _ := volumetesting.GetTestKubeletVolumePluginMgr(t)
 	dswp, fakePodManager, fakesDSW := createDswpWithVolumeWithCustomPluginMgr(t, pv, pvc, fakeVolumePluginMgr)

pkg/registry/core/pod/strategy_test.go

@@ -25,7 +25,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -1103,3 +1103,97 @@ func TestApplySeccompVersionSkew(t *testing.T) {
 		test.validation(t, test.pod)
 	}
 }
+
+// TestEphemeralVolumeEnablement checks the behavior of the API server
+// when the GenericEphemeralVolume feature is turned on and then off:
+// the Ephemeral struct must be preserved even during updates.
+func TestEphemeralVolumeEnablement(t *testing.T) {
+	// Enable the Feature Gate during the first pod creation
+	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.GenericEphemeralVolume, true)()
+
+	pod := createPodWithGenericEphemeralVolume()
+	expectedPod := pod.DeepCopy()
+
+	Strategy.PrepareForCreate(context.Background(), pod)
+	require.Equal(t, expectedPod.Spec, pod.Spec, "pod spec")
+
+	errs := Strategy.Validate(context.Background(), pod)
+	require.Empty(t, errs, "errors from validation")
+
+	// Now let's disable the Feature Gate, update some other field from the Pod and expect the volume to remain present
+	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.GenericEphemeralVolume, false)()
+	updatePod := testUpdatePod(t, pod, "aaa")
+
+	// And let's enable the FG again, add another from and check if the volume is still present
+	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.GenericEphemeralVolume, true)()
+	testUpdatePod(t, updatePod, "bbb")
+}
+
+// TestEphemeralVolumeDisabled checks the behavior of the API server
+// when the GenericEphemeralVolume is off: the Ephemeral struct gets dropped,
+// validation fails.
+func TestEphemeralVolumeDisabled(t *testing.T) {
+	// Disable the Feature Gate during the first pod creation
+	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.GenericEphemeralVolume, false)()
+
+	pod := createPodWithGenericEphemeralVolume()
+	expectedPod := pod.DeepCopy()
+	expectedPod.Spec.Volumes[0].VolumeSource.Ephemeral = nil
+
+	Strategy.PrepareForCreate(context.Background(), pod)
+	require.Equal(t, expectedPod.Spec, pod.Spec, "pod spec")
+
+	errs := Strategy.Validate(context.Background(), pod)
+	require.NotEmpty(t, errs, "no errors from validation")
+}
+
+func testUpdatePod(t *testing.T, oldPod *api.Pod, labelValue string) *api.Pod {
+	updatedPod := oldPod.DeepCopy()
+	updatedPod.Labels = map[string]string{"XYZ": labelValue}
+	expectedPod := updatedPod.DeepCopy()
+	Strategy.PrepareForUpdate(context.Background(), updatedPod, oldPod)
+	require.Equal(t, expectedPod.Spec, updatedPod.Spec, "updated pod spec")
+	errs := Strategy.Validate(context.Background(), updatedPod)
+	require.Empty(t, errs, "errors from validation")
+	return updatedPod
+}
+
+func createPodWithGenericEphemeralVolume() *api.Pod {
+	return &api.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: "ns",
+			Name:      "pod",
+		},
+		Spec: api.PodSpec{
+			RestartPolicy: api.RestartPolicyAlways,
+			DNSPolicy:     api.DNSClusterFirst,
+			Containers: []api.Container{{
+				Name:                     "foo",
+				Image:                    "example",
+				TerminationMessagePolicy: api.TerminationMessageReadFile,
+				ImagePullPolicy:          api.PullAlways,
+			}},
+			Volumes: []api.Volume{
+				{
+					Name: "ephemeral",
+					VolumeSource: api.VolumeSource{
+						Ephemeral: &api.EphemeralVolumeSource{
+							VolumeClaimTemplate: &api.PersistentVolumeClaimTemplate{
+								Spec: api.PersistentVolumeClaimSpec{
+									AccessModes: []api.PersistentVolumeAccessMode{
+										api.ReadWriteOnce,
+									},
+									Resources: api.ResourceRequirements{
+										Requests: api.ResourceList{
+											api.ResourceStorage: resource.MustParse("1Gi"),
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+}