diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go index a78e9e2c1fe..d8e7a1a42a6 100644 --- a/pkg/api/pod/util.go +++ b/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 { diff --git a/pkg/controller/volume/scheduling/scheduler_binder.go b/pkg/controller/volume/scheduling/scheduler_binder.go index 0cd05ddce0d..ae277ef5b31 100644 --- a/pkg/controller/volume/scheduling/scheduler_binder.go +++ b/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 && - utilfeature.DefaultFeatureGate.Enabled(features.GenericEphemeralVolume): + case vol.Ephemeral != nil: + 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 diff --git a/pkg/controller/volume/scheduling/scheduler_binder_test.go b/pkg/controller/volume/scheduling/scheduler_binder_test.go index bbae217664b..ce4694cc3b1 100644 --- a/pkg/controller/volume/scheduling/scheduler_binder_test.go +++ b/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() diff --git a/pkg/kubelet/volumemanager/populator/desired_state_of_world_populator.go b/pkg/kubelet/volumemanager/populator/desired_state_of_world_populator.go index 6cd4d58761b..b222f9224bf 100644 --- a/pkg/kubelet/volumemanager/populator/desired_state_of_world_populator.go +++ b/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 && - utilfeature.DefaultFeatureGate.Enabled(features.GenericEphemeralVolume) { + podVolume.VolumeSource.Ephemeral != nil { + 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 diff --git a/pkg/kubelet/volumemanager/populator/desired_state_of_world_populator_test.go b/pkg/kubelet/volumemanager/populator/desired_state_of_world_populator_test.go index f2f6660b3fd..258bc9f66ba 100644 --- a/pkg/kubelet/volumemanager/populator/desired_state_of_world_populator_test.go +++ b/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: Actual: <%v>", + expectedVolumeName, + volumeExists) + } + + if podExistsInVolume := fakesDSW.PodExistsInVolume( + podName, expectedVolumeName); !podExistsInVolume { + t.Fatalf( + "DSW PodExistsInVolume returned incorrect value. Expected: 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: Actual: <%v>", + expectedVolumeName, + volumeExists) + } + + if podExistsInVolume := fakesDSW.PodExistsInVolume( + podName, expectedVolumeName); !podExistsInVolume { + t.Fatalf( + "DSW PodExistsInVolume returned incorrect value. Expected: 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: Actual: <%v>", + expectedVolumeName, + volumeExists) + } + + if podExistsInVolume := fakesDSW.PodExistsInVolume( + podName, expectedVolumeName); podExistsInVolume { + t.Fatalf( + "DSW PodExistsInVolume returned incorrect value. Expected: 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) diff --git a/pkg/registry/core/pod/strategy_test.go b/pkg/registry/core/pod/strategy_test.go index f7f0dad477b..77616b9f341 100644 --- a/pkg/registry/core/pod/strategy_test.go +++ b/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"), + }, + }, + }, + }, + }, + }, + }, + }, + }, + } +}