diff --git a/pkg/scheduler/algorithm/predicates/max_attachable_volume_predicate_test.go b/pkg/scheduler/algorithm/predicates/max_attachable_volume_predicate_test.go index a4fd8c5245d..eb6b820bcea 100644 --- a/pkg/scheduler/algorithm/predicates/max_attachable_volume_predicate_test.go +++ b/pkg/scheduler/algorithm/predicates/max_attachable_volume_predicate_test.go @@ -23,12 +23,13 @@ import ( "strings" "testing" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" "k8s.io/api/storage/v1beta1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" utilfeature "k8s.io/apiserver/pkg/util/feature" featuregatetesting "k8s.io/component-base/featuregate/testing" + csilibplugins "k8s.io/csi-translation-lib/plugins" "k8s.io/kubernetes/pkg/features" schedulernodeinfo "k8s.io/kubernetes/pkg/scheduler/nodeinfo" volumeutil "k8s.io/kubernetes/pkg/volume/util" @@ -102,6 +103,32 @@ func TestVolumeCountConflicts(t *testing.T) { }, }, } + unboundPVCwithInvalidSCPod := &v1.Pod{ + Spec: v1.PodSpec{ + Volumes: []v1.Volume{ + { + VolumeSource: v1.VolumeSource{ + PersistentVolumeClaim: &v1.PersistentVolumeClaimVolumeSource{ + ClaimName: "unboundPVCwithInvalidSCPod", + }, + }, + }, + }, + }, + } + unboundPVCwithDefaultSCPod := &v1.Pod{ + Spec: v1.PodSpec{ + Volumes: []v1.Volume{ + { + VolumeSource: v1.VolumeSource{ + PersistentVolumeClaim: &v1.PersistentVolumeClaimVolumeSource{ + ClaimName: "unboundPVCwithDefaultSCPod", + }, + }, + }, + }, + }, + } splitVolsPod := &v1.Pod{ Spec: v1.PodSpec{ Volumes: []v1.Volume{ @@ -361,26 +388,43 @@ func TestVolumeCountConflicts(t *testing.T) { newPod: onePVCPod(EBSVolumeFilterType), existingPods: []*v1.Pod{oneVolPod, deletedPVCPod}, filterName: EBSVolumeFilterType, - maxVols: 2, + maxVols: 1, fits: false, - test: "pod with missing PVC is counted towards the PV limit", + test: "missing PVC is not counted towards the PV limit", }, { newPod: onePVCPod(EBSVolumeFilterType), existingPods: []*v1.Pod{oneVolPod, deletedPVCPod}, filterName: EBSVolumeFilterType, - maxVols: 3, + maxVols: 2, fits: true, - test: "pod with missing PVC is counted towards the PV limit", + test: "missing PVC is not counted towards the PV limit", }, { newPod: onePVCPod(EBSVolumeFilterType), existingPods: []*v1.Pod{oneVolPod, twoDeletedPVCPod}, filterName: EBSVolumeFilterType, - maxVols: 3, - fits: false, - test: "pod with missing two PVCs is counted towards the PV limit twice", + maxVols: 2, + fits: true, + test: "two missing PVCs are not counted towards the PV limit twice", }, + { + newPod: unboundPVCwithInvalidSCPod, + existingPods: []*v1.Pod{oneVolPod}, + filterName: EBSVolumeFilterType, + maxVols: 1, + fits: true, + test: "unbound PVC with invalid SC is not counted towards the PV limit", + }, + { + newPod: unboundPVCwithDefaultSCPod, + existingPods: []*v1.Pod{oneVolPod}, + filterName: EBSVolumeFilterType, + maxVols: 1, + fits: true, + test: "unbound PVC from different provisioner is not counted towards the PV limit", + }, + { newPod: onePVCPod(EBSVolumeFilterType), existingPods: []*v1.Pod{oneVolPod, deletedPVPod}, @@ -791,7 +835,11 @@ func TestVolumeCountConflicts(t *testing.T) { // running attachable predicate tests without feature gate and no limit present on nodes for _, test := range tests { os.Setenv(KubeMaxPDVols, strconv.Itoa(test.maxVols)) - pred := NewMaxPDVolumeCountPredicate(test.filterName, getFakePVInfo(test.filterName), getFakePVCInfo(test.filterName)) + pred := NewMaxPDVolumeCountPredicate(test.filterName, + getFakeStorageClassInfo(test.filterName), + getFakePVInfo(test.filterName), + getFakePVCInfo(test.filterName)) + fits, reasons, err := pred(test.newPod, GetPredicateMetadata(test.newPod, nil), schedulernodeinfo.NewNodeInfo(test.existingPods...)) if err != nil { t.Errorf("[%s]%s: unexpected error: %v", test.filterName, test.test, err) @@ -809,7 +857,10 @@ func TestVolumeCountConflicts(t *testing.T) { // running attachable predicate tests with feature gate and limit present on nodes for _, test := range tests { node := getNodeWithPodAndVolumeLimits("node", test.existingPods, int64(test.maxVols), test.filterName) - pred := NewMaxPDVolumeCountPredicate(test.filterName, getFakePVInfo(test.filterName), getFakePVCInfo(test.filterName)) + pred := NewMaxPDVolumeCountPredicate(test.filterName, + getFakeStorageClassInfo(test.filterName), + getFakePVInfo(test.filterName), + getFakePVCInfo(test.filterName)) fits, reasons, err := pred(test.newPod, GetPredicateMetadata(test.newPod, nil), node) if err != nil { t.Errorf("Using allocatable [%s]%s: unexpected error: %v", test.filterName, test.test, err) @@ -823,6 +874,32 @@ func TestVolumeCountConflicts(t *testing.T) { } } +func getFakeStorageClassInfo(sc string) FakeStorageClassInfo { + var provisioner string + switch sc { + case EBSVolumeFilterType: + provisioner = csilibplugins.AWSEBSInTreePluginName + case GCEPDVolumeFilterType: + provisioner = csilibplugins.GCEPDInTreePluginName + case AzureDiskVolumeFilterType: + provisioner = csilibplugins.AzureDiskInTreePluginName + case CinderVolumeFilterType: + provisioner = csilibplugins.CinderInTreePluginName + default: + return FakeStorageClassInfo{} + } + return FakeStorageClassInfo{ + { + ObjectMeta: metav1.ObjectMeta{Name: sc}, + Provisioner: provisioner, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "standard-sc"}, + Provisioner: "standard-sc", + }, + } +} + func getFakePVInfo(filterName string) FakePersistentVolumeInfo { return FakePersistentVolumeInfo{ { @@ -846,27 +923,59 @@ func getFakePVCInfo(filterName string) FakePersistentVolumeClaimInfo { return FakePersistentVolumeClaimInfo{ { ObjectMeta: metav1.ObjectMeta{Name: "some" + filterName + "Vol"}, - Spec: v1.PersistentVolumeClaimSpec{VolumeName: "some" + filterName + "Vol"}, + Spec: v1.PersistentVolumeClaimSpec{ + VolumeName: "some" + filterName + "Vol", + StorageClassName: &filterName, + }, }, { ObjectMeta: metav1.ObjectMeta{Name: "someNon" + filterName + "Vol"}, - Spec: v1.PersistentVolumeClaimSpec{VolumeName: "someNon" + filterName + "Vol"}, + Spec: v1.PersistentVolumeClaimSpec{ + VolumeName: "someNon" + filterName + "Vol", + StorageClassName: &filterName, + }, }, { ObjectMeta: metav1.ObjectMeta{Name: "pvcWithDeletedPV"}, - Spec: v1.PersistentVolumeClaimSpec{VolumeName: "pvcWithDeletedPV"}, + Spec: v1.PersistentVolumeClaimSpec{ + VolumeName: "pvcWithDeletedPV", + StorageClassName: &filterName, + }, }, { ObjectMeta: metav1.ObjectMeta{Name: "anotherPVCWithDeletedPV"}, - Spec: v1.PersistentVolumeClaimSpec{VolumeName: "anotherPVCWithDeletedPV"}, + Spec: v1.PersistentVolumeClaimSpec{ + VolumeName: "anotherPVCWithDeletedPV", + StorageClassName: &filterName, + }, }, { ObjectMeta: metav1.ObjectMeta{Name: "unboundPVC"}, - Spec: v1.PersistentVolumeClaimSpec{VolumeName: ""}, + Spec: v1.PersistentVolumeClaimSpec{ + VolumeName: "", + StorageClassName: &filterName, + }, }, { ObjectMeta: metav1.ObjectMeta{Name: "anotherUnboundPVC"}, - Spec: v1.PersistentVolumeClaimSpec{VolumeName: ""}, + Spec: v1.PersistentVolumeClaimSpec{ + VolumeName: "", + StorageClassName: &filterName, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "unboundPVCwithDefaultSCPod"}, + Spec: v1.PersistentVolumeClaimSpec{ + VolumeName: "", + StorageClassName: utilpointer.StringPtr("standard-sc"), + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "unboundPVCwithInvalidSCPod"}, + Spec: v1.PersistentVolumeClaimSpec{ + VolumeName: "", + StorageClassName: utilpointer.StringPtr("invalid-sc"), + }, }, } } diff --git a/pkg/scheduler/algorithm/predicates/predicates.go b/pkg/scheduler/algorithm/predicates/predicates.go index 2f6cbe3e76f..e74e006d11a 100644 --- a/pkg/scheduler/algorithm/predicates/predicates.go +++ b/pkg/scheduler/algorithm/predicates/predicates.go @@ -287,6 +287,7 @@ type MaxPDVolumeCountChecker struct { maxVolumeFunc func(node *v1.Node) int pvInfo PersistentVolumeInfo pvcInfo PersistentVolumeClaimInfo + scInfo StorageClassInfo // The string below is generated randomly during the struct's initialization. // It is used to prefix volumeID generated inside the predicate() method to @@ -299,6 +300,8 @@ type VolumeFilter struct { // Filter normal volumes FilterVolume func(vol *v1.Volume) (id string, relevant bool) FilterPersistentVolume func(pv *v1.PersistentVolume) (id string, relevant bool) + // MatchProvisioner evaluates if the StorageClass provisioner matches the running predicate + MatchProvisioner func(sc *storagev1.StorageClass) (relevant bool) // IsMigrated returns a boolean specifying whether the plugin is migrated to a CSI driver IsMigrated func(csiNode *storagev1beta1.CSINode) bool } @@ -314,7 +317,7 @@ type VolumeFilter struct { // types, counts the number of unique volumes, and rejects the new pod if it would place the total count over // the maximum. func NewMaxPDVolumeCountPredicate( - filterName string, pvInfo PersistentVolumeInfo, pvcInfo PersistentVolumeClaimInfo) FitPredicate { + filterName string, scInfo StorageClassInfo, pvInfo PersistentVolumeInfo, pvcInfo PersistentVolumeClaimInfo) FitPredicate { var filter VolumeFilter var volumeLimitKey v1.ResourceName @@ -344,6 +347,7 @@ func NewMaxPDVolumeCountPredicate( maxVolumeFunc: getMaxVolumeFunc(filterName), pvInfo: pvInfo, pvcInfo: pvcInfo, + scInfo: scInfo, randomVolumeIDPrefix: rand.String(32), } @@ -428,19 +432,23 @@ func (c *MaxPDVolumeCountChecker) filterVolumes(volumes []v1.Volume, namespace s if pvName == "" { // PVC is not bound. It was either deleted and created again or // it was forcefully unbound by admin. The pod can still use the - // original PV where it was bound to -> log the error and count - // the PV towards the PV limit - klog.V(4).Infof("PVC %s/%s is not bound, assuming PVC matches predicate when counting limits", namespace, pvcName) - filteredVolumes[pvID] = true + // original PV where it was bound to, so we count the volume if + // it belongs to the running predicate. + if c.matchProvisioner(pvc) { + klog.V(4).Infof("PVC %s/%s is not bound, assuming PVC matches predicate when counting limits", namespace, pvcName) + filteredVolumes[pvID] = true + } continue } pv, err := c.pvInfo.GetPersistentVolumeInfo(pvName) if err != nil || pv == nil { - // if the PV is not found, log the error - // and count the PV towards the PV limit - klog.V(4).Infof("Unable to look up PV info for %s/%s/%s, assuming PV matches predicate when counting limits: %v", namespace, pvcName, pvName, err) - filteredVolumes[pvID] = true + // If the PV is invalid and PVC belongs to the running predicate, + // log the error and count the PV towards the PV limit. + if c.matchProvisioner(pvc) { + klog.V(4).Infof("Unable to look up PV info for %s/%s/%s, assuming PV matches predicate when counting limits: %v", namespace, pvcName, pvName, err) + filteredVolumes[pvID] = true + } continue } @@ -453,6 +461,20 @@ func (c *MaxPDVolumeCountChecker) filterVolumes(volumes []v1.Volume, namespace s return nil } +// matchProvisioner helps identify if the given PVC belongs to the running predicate. +func (c *MaxPDVolumeCountChecker) matchProvisioner(pvc *v1.PersistentVolumeClaim) bool { + if pvc.Spec.StorageClassName == nil { + return false + } + + storageClass, err := c.scInfo.GetStorageClassInfo(*pvc.Spec.StorageClassName) + if err != nil || storageClass == nil { + return false + } + + return c.filter.MatchProvisioner(storageClass) +} + func (c *MaxPDVolumeCountChecker) predicate(pod *v1.Pod, meta PredicateMetadata, nodeInfo *schedulernodeinfo.NodeInfo) (bool, []PredicateFailureReason, error) { // If a pod doesn't have any volume attached to it, the predicate will always be true. // Thus we make a fast path for it, to avoid unnecessary computations in this case. @@ -530,6 +552,13 @@ var EBSVolumeFilter = VolumeFilter{ return "", false }, + MatchProvisioner: func(sc *storagev1.StorageClass) (relevant bool) { + if sc.Provisioner == csilibplugins.AWSEBSInTreePluginName { + return true + } + return false + }, + IsMigrated: func(csiNode *storagev1beta1.CSINode) bool { return isCSIMigrationOn(csiNode, csilibplugins.AWSEBSInTreePluginName) }, @@ -551,6 +580,13 @@ var GCEPDVolumeFilter = VolumeFilter{ return "", false }, + MatchProvisioner: func(sc *storagev1.StorageClass) (relevant bool) { + if sc.Provisioner == csilibplugins.GCEPDInTreePluginName { + return true + } + return false + }, + IsMigrated: func(csiNode *storagev1beta1.CSINode) bool { return isCSIMigrationOn(csiNode, csilibplugins.GCEPDInTreePluginName) }, @@ -572,6 +608,13 @@ var AzureDiskVolumeFilter = VolumeFilter{ return "", false }, + MatchProvisioner: func(sc *storagev1.StorageClass) (relevant bool) { + if sc.Provisioner == csilibplugins.AzureDiskInTreePluginName { + return true + } + return false + }, + IsMigrated: func(csiNode *storagev1beta1.CSINode) bool { return isCSIMigrationOn(csiNode, csilibplugins.AzureDiskInTreePluginName) }, @@ -594,6 +637,13 @@ var CinderVolumeFilter = VolumeFilter{ return "", false }, + MatchProvisioner: func(sc *storagev1.StorageClass) (relevant bool) { + if sc.Provisioner == csilibplugins.CinderInTreePluginName { + return true + } + return false + }, + IsMigrated: func(csiNode *storagev1beta1.CSINode) bool { return isCSIMigrationOn(csiNode, csilibplugins.CinderInTreePluginName) }, diff --git a/pkg/scheduler/algorithmprovider/defaults/register_predicates.go b/pkg/scheduler/algorithmprovider/defaults/register_predicates.go index 1e343682963..b1283261b8d 100644 --- a/pkg/scheduler/algorithmprovider/defaults/register_predicates.go +++ b/pkg/scheduler/algorithmprovider/defaults/register_predicates.go @@ -61,21 +61,21 @@ func init() { factory.RegisterFitPredicateFactory( predicates.MaxEBSVolumeCountPred, func(args factory.PluginFactoryArgs) predicates.FitPredicate { - return predicates.NewMaxPDVolumeCountPredicate(predicates.EBSVolumeFilterType, args.PVInfo, args.PVCInfo) + return predicates.NewMaxPDVolumeCountPredicate(predicates.EBSVolumeFilterType, args.StorageClassInfo, args.PVInfo, args.PVCInfo) }, ) // Fit is determined by whether or not there would be too many GCE PD volumes attached to the node factory.RegisterFitPredicateFactory( predicates.MaxGCEPDVolumeCountPred, func(args factory.PluginFactoryArgs) predicates.FitPredicate { - return predicates.NewMaxPDVolumeCountPredicate(predicates.GCEPDVolumeFilterType, args.PVInfo, args.PVCInfo) + return predicates.NewMaxPDVolumeCountPredicate(predicates.GCEPDVolumeFilterType, args.StorageClassInfo, args.PVInfo, args.PVCInfo) }, ) // Fit is determined by whether or not there would be too many Azure Disk volumes attached to the node factory.RegisterFitPredicateFactory( predicates.MaxAzureDiskVolumeCountPred, func(args factory.PluginFactoryArgs) predicates.FitPredicate { - return predicates.NewMaxPDVolumeCountPredicate(predicates.AzureDiskVolumeFilterType, args.PVInfo, args.PVCInfo) + return predicates.NewMaxPDVolumeCountPredicate(predicates.AzureDiskVolumeFilterType, args.StorageClassInfo, args.PVInfo, args.PVCInfo) }, ) factory.RegisterFitPredicateFactory( @@ -87,7 +87,7 @@ func init() { factory.RegisterFitPredicateFactory( predicates.MaxCinderVolumeCountPred, func(args factory.PluginFactoryArgs) predicates.FitPredicate { - return predicates.NewMaxPDVolumeCountPredicate(predicates.CinderVolumeFilterType, args.PVInfo, args.PVCInfo) + return predicates.NewMaxPDVolumeCountPredicate(predicates.CinderVolumeFilterType, args.StorageClassInfo, args.PVInfo, args.PVCInfo) }, )