From b98634c2da6b8e6ebecedc682c9a4a3468e21a3e Mon Sep 17 00:00:00 2001 From: YamasouA Date: Wed, 24 Jul 2024 23:50:46 +0900 Subject: [PATCH] volumebinding: scheduler queueing hints - CSIDriver fix if condition add test add log eliminate unnecessary args from log fix Queue condition check original pod status fix return value when can scheduleable fix tweak fix testcase --- .../plugins/volumebinding/test_utils.go | 11 ++ .../plugins/volumebinding/volume_binding.go | 29 ++- .../volumebinding/volume_binding_test.go | 186 ++++++++++++++++++ 3 files changed, 225 insertions(+), 1 deletion(-) diff --git a/pkg/scheduler/framework/plugins/volumebinding/test_utils.go b/pkg/scheduler/framework/plugins/volumebinding/test_utils.go index a1c968d76fc..23675f969d6 100644 --- a/pkg/scheduler/framework/plugins/volumebinding/test_utils.go +++ b/pkg/scheduler/framework/plugins/volumebinding/test_utils.go @@ -204,3 +204,14 @@ func (pb podBuilder) withGenericEphemeralVolume(name string) podBuilder { }) return pb } + +func (pb podBuilder) withCSI(driver string) podBuilder { + pb.Pod.Spec.Volumes = append(pb.Pod.Spec.Volumes, v1.Volume{ + VolumeSource: v1.VolumeSource{ + CSI: &v1.CSIVolumeSource{ + Driver: driver, + }, + }, + }) + return pb +} diff --git a/pkg/scheduler/framework/plugins/volumebinding/volume_binding.go b/pkg/scheduler/framework/plugins/volumebinding/volume_binding.go index 8bafaa52123..6d5f647342f 100644 --- a/pkg/scheduler/framework/plugins/volumebinding/volume_binding.go +++ b/pkg/scheduler/framework/plugins/volumebinding/volume_binding.go @@ -126,7 +126,7 @@ func (pl *VolumeBinding) EventsToRegister(_ context.Context) ([]framework.Cluste // When CSIStorageCapacity is enabled, pods may become schedulable // on CSI driver & storage capacity changes. - {Event: framework.ClusterEvent{Resource: framework.CSIDriver, ActionType: framework.Add | framework.Update}}, + {Event: framework.ClusterEvent{Resource: framework.CSIDriver, ActionType: framework.Add | framework.Update}, QueueingHintFn: pl.isSchedulableAfterCSIDriverChange}, {Event: framework.ClusterEvent{Resource: framework.CSIStorageCapacity, ActionType: framework.Add | framework.Update}, QueueingHintFn: pl.isSchedulableAfterCSIStorageCapacityChange}, } return events, nil @@ -270,6 +270,33 @@ func (pl *VolumeBinding) isSchedulableAfterCSIStorageCapacityChange(logger klog. return framework.QueueSkip, nil } +func (pl *VolumeBinding) isSchedulableAfterCSIDriverChange(logger klog.Logger, pod *v1.Pod, oldObj, newObj interface{}) (framework.QueueingHint, error) { + originalCSIDriver, modifiedCSIDriver, err := util.As[*storagev1.CSIDriver](oldObj, newObj) + if err != nil { + return framework.Queue, err + } + + logger = klog.LoggerWithValues( + logger, + "Pod", klog.KObj(pod), + "CSIDriver", klog.KObj(modifiedCSIDriver), + ) + + for _, vol := range pod.Spec.Volumes { + if vol.CSI == nil || vol.CSI.Driver != modifiedCSIDriver.Name { + continue + } + if (originalCSIDriver.Spec.StorageCapacity != nil && *originalCSIDriver.Spec.StorageCapacity) && + (modifiedCSIDriver.Spec.StorageCapacity == nil || !*modifiedCSIDriver.Spec.StorageCapacity) { + logger.V(5).Info("CSIDriver was updated and storage capacity got disabled, which may make the pod schedulable") + return framework.Queue, nil + } + } + + logger.V(5).Info("CSIDriver was created or updated but it doesn't make this pod schedulable") + return framework.QueueSkip, nil +} + // podHasPVCs returns 2 values: // - the first one to denote if the given "pod" has any PVC defined. // - the second one to return any error if the requested PVC is illegal. diff --git a/pkg/scheduler/framework/plugins/volumebinding/volume_binding_test.go b/pkg/scheduler/framework/plugins/volumebinding/volume_binding_test.go index 127c2c606cc..e5d167dd08a 100644 --- a/pkg/scheduler/framework/plugins/volumebinding/volume_binding_test.go +++ b/pkg/scheduler/framework/plugins/volumebinding/volume_binding_test.go @@ -35,6 +35,7 @@ import ( "k8s.io/kubernetes/pkg/scheduler/framework/plugins/feature" "k8s.io/kubernetes/pkg/scheduler/framework/runtime" tf "k8s.io/kubernetes/pkg/scheduler/testing/framework" + "k8s.io/utils/ptr" ) var ( @@ -1372,3 +1373,188 @@ func TestIsSchedulableAfterCSIStorageCapacityChange(t *testing.T) { }) } } + +func TestIsSchedulableAfterCSIDriverChange(t *testing.T) { + table := []struct { + name string + pod *v1.Pod + newObj interface{} + oldObj interface{} + err bool + expect framework.QueueingHint + }{ + { + name: "pod has no CSIDriver", + pod: makePod("pod-a").Pod, + newObj: &storagev1.CSIDriver{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test1", + }, + Spec: storagev1.CSIDriverSpec{ + StorageCapacity: ptr.To(true), + }, + }, + oldObj: &storagev1.CSIDriver{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test1", + }, + }, + err: false, + expect: framework.QueueSkip, + }, + { + name: "unexpected objects are passed", + pod: makePod("pod-a").Pod, + newObj: new(struct{}), + oldObj: new(struct{}), + err: true, + expect: framework.Queue, + }, + { + name: "driver name in pod and csidriver name are different", + pod: makePod("pod-a").withCSI("test1").Pod, + newObj: &storagev1.CSIDriver{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test2", + }, + Spec: storagev1.CSIDriverSpec{ + StorageCapacity: ptr.To(true), + }, + }, + oldObj: &storagev1.CSIDriver{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test2", + }, + Spec: storagev1.CSIDriverSpec{ + StorageCapacity: ptr.To(false), + }, + }, + err: false, + expect: framework.QueueSkip, + }, + { + name: "original StorageCapacity is nil", + pod: makePod("pod-a").withCSI("test1").Pod, + newObj: &storagev1.CSIDriver{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test1", + }, + Spec: storagev1.CSIDriverSpec{ + StorageCapacity: nil, + }, + }, + oldObj: &storagev1.CSIDriver{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test1", + }, + Spec: storagev1.CSIDriverSpec{ + StorageCapacity: nil, + }, + }, + err: false, + expect: framework.QueueSkip, + }, + { + name: "original StorageCapacity is false", + pod: makePod("pod-a").withCSI("test1").Pod, + newObj: &storagev1.CSIDriver{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test1", + }, + Spec: storagev1.CSIDriverSpec{ + StorageCapacity: ptr.To(false), + }, + }, + oldObj: &storagev1.CSIDriver{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test1", + }, + Spec: storagev1.CSIDriverSpec{ + StorageCapacity: ptr.To(false), + }, + }, + err: false, + expect: framework.QueueSkip, + }, + { + name: "modified StorageCapacity is nil", + pod: makePod("pod-a").withCSI("test1").Pod, + newObj: &storagev1.CSIDriver{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test1", + }, + Spec: storagev1.CSIDriverSpec{ + StorageCapacity: nil, + }, + }, + oldObj: &storagev1.CSIDriver{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test1", + }, + Spec: storagev1.CSIDriverSpec{ + StorageCapacity: ptr.To(true), + }, + }, + err: false, + expect: framework.Queue, + }, + { + name: "modified StorageCapacity is true", + pod: makePod("pod-a").withCSI("test1").Pod, + newObj: &storagev1.CSIDriver{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test1", + }, + Spec: storagev1.CSIDriverSpec{ + StorageCapacity: ptr.To(true), + }, + }, + oldObj: &storagev1.CSIDriver{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test1", + }, + Spec: storagev1.CSIDriverSpec{ + StorageCapacity: ptr.To(true), + }, + }, + err: false, + expect: framework.QueueSkip, + }, + + { + name: "modified StorageCapacity is false", + pod: makePod("pod-a").withCSI("test1").Pod, + newObj: &storagev1.CSIDriver{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test1", + }, + Spec: storagev1.CSIDriverSpec{ + StorageCapacity: ptr.To(false), + }, + }, + oldObj: &storagev1.CSIDriver{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test1", + }, + Spec: storagev1.CSIDriverSpec{ + StorageCapacity: ptr.To(true), + }, + }, + err: false, + expect: framework.Queue, + }, + } + for _, item := range table { + t.Run(item.name, func(t *testing.T) { + pl := &VolumeBinding{} + logger, _ := ktesting.NewTestContext(t) + qhint, err := pl.isSchedulableAfterCSIDriverChange(logger, item.pod, item.oldObj, item.newObj) + if (err != nil) != item.err { + t.Errorf("isSchedulableAfterCSINodeChange failed - got: %q", err) + } + if qhint != item.expect { + t.Errorf("QHint does not match: %v, want: %v", qhint, item.expect) + } + }) + } +}