scheduler: use generic ephemeral volume helper functions

The name concatenation and ownership check were originally considered small
enough to not warrant dedicated functions, but the intent of the code is more
readable with them.
This commit is contained in:
Patrick Ohly 2021-08-31 08:39:55 +02:00
parent 466dcdfcf6
commit bc263f3ba5
6 changed files with 35 additions and 26 deletions

View File

@ -22,11 +22,11 @@ import (
v1 "k8s.io/api/core/v1"
storagev1 "k8s.io/api/storage/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/rand"
corelisters "k8s.io/client-go/listers/core/v1"
storagelisters "k8s.io/client-go/listers/storage/v1"
ephemeral "k8s.io/component-helpers/storage/ephemeral"
storagehelpers "k8s.io/component-helpers/storage/volume"
csitrans "k8s.io/csi-translation-lib"
"k8s.io/klog/v2"
@ -152,7 +152,7 @@ func (pl *CSILimits) filterAttachableVolumes(
for _, vol := range pod.Spec.Volumes {
// CSI volumes can only be used through a PVC.
pvcName := ""
ephemeral := false
isEphemeral := false
switch {
case vol.PersistentVolumeClaim != nil:
pvcName = vol.PersistentVolumeClaim.ClaimName
@ -167,8 +167,8 @@ func (pl *CSILimits) filterAttachableVolumes(
// just with a computed name and certain ownership.
// That is checked below once the pvc object is
// retrieved.
pvcName = pod.Name + "-" + vol.Name
ephemeral = true
pvcName = ephemeral.VolumeClaimName(pod, &vol)
isEphemeral = true
default:
continue
}
@ -193,8 +193,10 @@ func (pl *CSILimits) filterAttachableVolumes(
}
// The PVC for an ephemeral volume must be owned by the pod.
if ephemeral && !metav1.IsControlledBy(pvc, pod) {
return fmt.Errorf("PVC %s/%s is not owned by pod", pod.Namespace, pvcName)
if isEphemeral {
if err := ephemeral.VolumeIsForPod(pod, pvc); err != nil {
return err
}
}
driverName, volumeHandle := pl.getCSIDriverInfo(csiNode, pvc)

View File

@ -568,7 +568,7 @@ func TestCSILimits(t *testing.T) {
extraClaims: []v1.PersistentVolumeClaim{*conflictingClaim},
driverNames: []string{ebsCSIDriverName},
test: "ephemeral volume not owned",
wantStatus: framework.NewStatus(framework.Error, "PVC test/abc-xyz is not owned by pod"),
wantStatus: framework.NewStatus(framework.Error, "PVC test/abc-xyz was not created for pod test/abc (pod is not owner)"),
},
{
newPod: ephemeralVolumePod,

View File

@ -26,13 +26,13 @@ import (
v1 "k8s.io/api/core/v1"
storage "k8s.io/api/storage/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/rand"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/client-go/informers"
corelisters "k8s.io/client-go/listers/core/v1"
storagelisters "k8s.io/client-go/listers/storage/v1"
"k8s.io/component-helpers/storage/ephemeral"
csilibplugins "k8s.io/csi-translation-lib/plugins"
"k8s.io/klog/v2"
"k8s.io/kubernetes/pkg/scheduler/framework"
@ -288,7 +288,7 @@ func (pl *nonCSILimits) filterVolumes(pod *v1.Pod, newPod bool, filteredVolumes
}
pvcName := ""
ephemeral := false
isEphemeral := false
switch {
case vol.PersistentVolumeClaim != nil:
pvcName = vol.PersistentVolumeClaim.ClaimName
@ -303,8 +303,8 @@ func (pl *nonCSILimits) filterVolumes(pod *v1.Pod, newPod bool, filteredVolumes
// just with a computed name and certain ownership.
// That is checked below once the pvc object is
// retrieved.
pvcName = pod.Name + "-" + vol.Name
ephemeral = true
pvcName = ephemeral.VolumeClaimName(pod, vol)
isEphemeral = true
default:
continue
}
@ -332,8 +332,10 @@ func (pl *nonCSILimits) filterVolumes(pod *v1.Pod, newPod bool, filteredVolumes
}
// The PVC for an ephemeral volume must be owned by the pod.
if ephemeral && !metav1.IsControlledBy(pvc, pod) {
return fmt.Errorf("PVC %s/%s is not owned by pod", pod.Namespace, pvcName)
if isEphemeral {
if err := ephemeral.VolumeIsForPod(pod, pvc); err != nil {
return err
}
}
pvName := pvc.Spec.VolumeName

View File

@ -102,7 +102,7 @@ func TestEphemeralLimits(t *testing.T) {
ephemeralEnabled: true,
extraClaims: []v1.PersistentVolumeClaim{*conflictingClaim},
test: "volume not owned",
wantStatus: framework.NewStatus(framework.Error, "PVC test/abc-xyz is not owned by pod"),
wantStatus: framework.NewStatus(framework.Error, "PVC test/abc-xyz was not created for pod test/abc (pod is not owner)"),
},
{
newPod: ephemeralVolumePod,

View File

@ -40,6 +40,7 @@ import (
corelisters "k8s.io/client-go/listers/core/v1"
storagelisters "k8s.io/client-go/listers/storage/v1"
storagelistersv1beta1 "k8s.io/client-go/listers/storage/v1beta1"
"k8s.io/component-helpers/storage/ephemeral"
storagehelpers "k8s.io/component-helpers/storage/volume"
csitrans "k8s.io/csi-translation-lib"
csiplugins "k8s.io/csi-translation-lib/plugins"
@ -686,7 +687,7 @@ func (b *volumeBinder) checkBindings(pod *v1.Pod, bindings []*BindingInfo, claim
func (b *volumeBinder) isVolumeBound(pod *v1.Pod, vol *v1.Volume) (bound bool, pvc *v1.PersistentVolumeClaim, err error) {
pvcName := ""
ephemeral := false
isEphemeral := false
switch {
case vol.PersistentVolumeClaim != nil:
pvcName = vol.PersistentVolumeClaim.ClaimName
@ -699,16 +700,18 @@ func (b *volumeBinder) isVolumeBound(pod *v1.Pod, vol *v1.Volume) (bound bool, p
}
// Generic ephemeral inline volumes also use a PVC,
// just with a computed name, and...
pvcName = pod.Name + "-" + vol.Name
ephemeral = true
pvcName = ephemeral.VolumeClaimName(pod, vol)
isEphemeral = true
default:
return true, nil, nil
}
bound, pvc, err = b.isPVCBound(pod.Namespace, pvcName)
// ... the PVC must be owned by the pod.
if ephemeral && err == nil && pvc != nil && !metav1.IsControlledBy(pvc, pod) {
return false, nil, fmt.Errorf("PVC %s/%s is not owned by pod", pod.Namespace, pvcName)
if isEphemeral && err == nil && pvc != nil {
if err := ephemeral.VolumeIsForPod(pod, pvc); err != nil {
return false, nil, err
}
}
return
}

View File

@ -25,9 +25,9 @@ import (
v1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
corelisters "k8s.io/client-go/listers/core/v1"
"k8s.io/component-helpers/storage/ephemeral"
"k8s.io/klog/v2"
"k8s.io/kubernetes/pkg/scheduler/apis/config"
"k8s.io/kubernetes/pkg/scheduler/apis/config/validation"
@ -127,13 +127,13 @@ func (pl *VolumeBinding) podHasPVCs(pod *v1.Pod) (bool, error) {
hasPVC := false
for _, vol := range pod.Spec.Volumes {
var pvcName string
ephemeral := false
isEphemeral := false
switch {
case vol.PersistentVolumeClaim != nil:
pvcName = vol.PersistentVolumeClaim.ClaimName
case vol.Ephemeral != nil && pl.GenericEphemeralVolumeFeatureEnabled:
pvcName = pod.Name + "-" + vol.Name
ephemeral = true
pvcName = ephemeral.VolumeClaimName(pod, &vol)
isEphemeral = true
default:
// Volume is not using a PVC, ignore
continue
@ -144,7 +144,7 @@ func (pl *VolumeBinding) podHasPVCs(pod *v1.Pod) (bool, error) {
// The error usually has already enough context ("persistentvolumeclaim "myclaim" not found"),
// but we can do better for generic ephemeral inline volumes where that situation
// is normal directly after creating a pod.
if ephemeral && apierrors.IsNotFound(err) {
if isEphemeral && apierrors.IsNotFound(err) {
err = fmt.Errorf("waiting for ephemeral volume controller to create the persistentvolumeclaim %q", pvcName)
}
return hasPVC, err
@ -158,8 +158,10 @@ func (pl *VolumeBinding) podHasPVCs(pod *v1.Pod) (bool, error) {
return hasPVC, fmt.Errorf("persistentvolumeclaim %q is being deleted", pvc.Name)
}
if ephemeral && !metav1.IsControlledBy(pvc, pod) {
return hasPVC, fmt.Errorf("persistentvolumeclaim %q was not created for the pod", pvc.Name)
if isEphemeral {
if err := ephemeral.VolumeIsForPod(pod, pvc); err != nil {
return hasPVC, err
}
}
}
return hasPVC, nil