From b98634c2da6b8e6ebecedc682c9a4a3468e21a3e Mon Sep 17 00:00:00 2001 From: YamasouA Date: Wed, 24 Jul 2024 23:50:46 +0900 Subject: [PATCH 1/6] 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) + } + }) + } +} From b4d9fe39571597c7995865171ca078e570a126c7 Mon Sep 17 00:00:00 2001 From: YamasouA Date: Fri, 13 Sep 2024 22:48:46 +0900 Subject: [PATCH 2/6] delete framework.Add --- pkg/scheduler/framework/plugins/volumebinding/volume_binding.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/scheduler/framework/plugins/volumebinding/volume_binding.go b/pkg/scheduler/framework/plugins/volumebinding/volume_binding.go index 6d5f647342f..c565905eb11 100644 --- a/pkg/scheduler/framework/plugins/volumebinding/volume_binding.go +++ b/pkg/scheduler/framework/plugins/volumebinding/volume_binding.go @@ -112,7 +112,7 @@ func (pl *VolumeBinding) EventsToRegister(_ context.Context) ([]framework.Cluste // Pods may fail because of missing or mis-configured storage class // (e.g., allowedTopologies, volumeBindingMode), and hence may become // schedulable upon StorageClass Add or Update events. - {Event: framework.ClusterEvent{Resource: framework.StorageClass, ActionType: framework.Add | framework.Update}, QueueingHintFn: pl.isSchedulableAfterStorageClassChange}, + {Event: framework.ClusterEvent{Resource: framework.StorageClass, ActionType: framework.Update}, QueueingHintFn: pl.isSchedulableAfterStorageClassChange}, // We bind PVCs with PVs, so any changes may make the pods schedulable. {Event: framework.ClusterEvent{Resource: framework.PersistentVolumeClaim, ActionType: framework.Add | framework.Update}, QueueingHintFn: pl.isSchedulableAfterPersistentVolumeClaimChange}, From 84f45c81ca4f47a56ca51120c2405263440a7d15 Mon Sep 17 00:00:00 2001 From: YamasouA Date: Fri, 13 Sep 2024 22:51:51 +0900 Subject: [PATCH 3/6] tweak --- .../framework/plugins/volumebinding/volume_binding.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/scheduler/framework/plugins/volumebinding/volume_binding.go b/pkg/scheduler/framework/plugins/volumebinding/volume_binding.go index c565905eb11..3eff2df8b0e 100644 --- a/pkg/scheduler/framework/plugins/volumebinding/volume_binding.go +++ b/pkg/scheduler/framework/plugins/volumebinding/volume_binding.go @@ -112,7 +112,7 @@ func (pl *VolumeBinding) EventsToRegister(_ context.Context) ([]framework.Cluste // Pods may fail because of missing or mis-configured storage class // (e.g., allowedTopologies, volumeBindingMode), and hence may become // schedulable upon StorageClass Add or Update events. - {Event: framework.ClusterEvent{Resource: framework.StorageClass, ActionType: framework.Update}, QueueingHintFn: pl.isSchedulableAfterStorageClassChange}, + {Event: framework.ClusterEvent{Resource: framework.StorageClass, ActionType: framework.Add | framework.Update}, QueueingHintFn: pl.isSchedulableAfterStorageClassChange}, // We bind PVCs with PVs, so any changes may make the pods schedulable. {Event: framework.ClusterEvent{Resource: framework.PersistentVolumeClaim, ActionType: framework.Add | framework.Update}, QueueingHintFn: pl.isSchedulableAfterPersistentVolumeClaimChange}, @@ -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}, QueueingHintFn: pl.isSchedulableAfterCSIDriverChange}, + {Event: framework.ClusterEvent{Resource: framework.CSIDriver, ActionType: framework.Update}, QueueingHintFn: pl.isSchedulableAfterCSIDriverChange}, {Event: framework.ClusterEvent{Resource: framework.CSIStorageCapacity, ActionType: framework.Add | framework.Update}, QueueingHintFn: pl.isSchedulableAfterCSIStorageCapacityChange}, } return events, nil From c2ba2ea383dc013ed999706eadb19b1114915730 Mon Sep 17 00:00:00 2001 From: YamasouA Date: Wed, 25 Sep 2024 22:34:38 +0900 Subject: [PATCH 4/6] fix unit test --- pkg/scheduler/scheduler_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/scheduler/scheduler_test.go b/pkg/scheduler/scheduler_test.go index b146afbbd7c..1a8fcd2a456 100644 --- a/pkg/scheduler/scheduler_test.go +++ b/pkg/scheduler/scheduler_test.go @@ -874,7 +874,7 @@ func Test_UnionedGVKs(t *testing.T) { framework.Pod: framework.Add | framework.UpdatePodLabel | framework.Delete, framework.Node: framework.Add | framework.UpdateNodeAllocatable | framework.UpdateNodeLabel | framework.UpdateNodeTaint | framework.Delete, framework.CSINode: framework.All - framework.Delete, - framework.CSIDriver: framework.All - framework.Delete, + framework.CSIDriver: framework.Update framework.CSIStorageCapacity: framework.All - framework.Delete, framework.PersistentVolume: framework.All - framework.Delete, framework.PersistentVolumeClaim: framework.All - framework.Delete, @@ -888,7 +888,7 @@ func Test_UnionedGVKs(t *testing.T) { framework.Pod: framework.Add | framework.UpdatePodLabel | framework.UpdatePodScaleDown | framework.Delete, framework.Node: framework.Add | framework.UpdateNodeAllocatable | framework.UpdateNodeLabel | framework.UpdateNodeTaint | framework.Delete, framework.CSINode: framework.All - framework.Delete, - framework.CSIDriver: framework.All - framework.Delete, + framework.CSIDriver: framework.Update framework.CSIStorageCapacity: framework.All - framework.Delete, framework.PersistentVolume: framework.All - framework.Delete, framework.PersistentVolumeClaim: framework.All - framework.Delete, @@ -903,7 +903,7 @@ func Test_UnionedGVKs(t *testing.T) { framework.Pod: framework.Add | framework.UpdatePodLabel | framework.UpdatePodScaleDown | framework.UpdatePodTolerations | framework.UpdatePodSchedulingGatesEliminated | framework.Delete, framework.Node: framework.Add | framework.UpdateNodeAllocatable | framework.UpdateNodeLabel | framework.UpdateNodeTaint | framework.Delete, framework.CSINode: framework.All - framework.Delete, - framework.CSIDriver: framework.All - framework.Delete, + framework.CSIDriver: framework.Update framework.CSIStorageCapacity: framework.All - framework.Delete, framework.PersistentVolume: framework.All - framework.Delete, framework.PersistentVolumeClaim: framework.All - framework.Delete, From c50884a2f9c40f75340797268044052732afc574 Mon Sep 17 00:00:00 2001 From: YamasouA Date: Wed, 25 Sep 2024 23:09:06 +0900 Subject: [PATCH 5/6] tweak --- pkg/scheduler/scheduler_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/scheduler/scheduler_test.go b/pkg/scheduler/scheduler_test.go index 1a8fcd2a456..e8e2c833892 100644 --- a/pkg/scheduler/scheduler_test.go +++ b/pkg/scheduler/scheduler_test.go @@ -874,7 +874,7 @@ func Test_UnionedGVKs(t *testing.T) { framework.Pod: framework.Add | framework.UpdatePodLabel | framework.Delete, framework.Node: framework.Add | framework.UpdateNodeAllocatable | framework.UpdateNodeLabel | framework.UpdateNodeTaint | framework.Delete, framework.CSINode: framework.All - framework.Delete, - framework.CSIDriver: framework.Update + framework.CSIDriver: framework.Update, framework.CSIStorageCapacity: framework.All - framework.Delete, framework.PersistentVolume: framework.All - framework.Delete, framework.PersistentVolumeClaim: framework.All - framework.Delete, @@ -888,7 +888,7 @@ func Test_UnionedGVKs(t *testing.T) { framework.Pod: framework.Add | framework.UpdatePodLabel | framework.UpdatePodScaleDown | framework.Delete, framework.Node: framework.Add | framework.UpdateNodeAllocatable | framework.UpdateNodeLabel | framework.UpdateNodeTaint | framework.Delete, framework.CSINode: framework.All - framework.Delete, - framework.CSIDriver: framework.Update + framework.CSIDriver: framework.Update, framework.CSIStorageCapacity: framework.All - framework.Delete, framework.PersistentVolume: framework.All - framework.Delete, framework.PersistentVolumeClaim: framework.All - framework.Delete, @@ -903,7 +903,7 @@ func Test_UnionedGVKs(t *testing.T) { framework.Pod: framework.Add | framework.UpdatePodLabel | framework.UpdatePodScaleDown | framework.UpdatePodTolerations | framework.UpdatePodSchedulingGatesEliminated | framework.Delete, framework.Node: framework.Add | framework.UpdateNodeAllocatable | framework.UpdateNodeLabel | framework.UpdateNodeTaint | framework.Delete, framework.CSINode: framework.All - framework.Delete, - framework.CSIDriver: framework.Update + framework.CSIDriver: framework.Update, framework.CSIStorageCapacity: framework.All - framework.Delete, framework.PersistentVolume: framework.All - framework.Delete, framework.PersistentVolumeClaim: framework.All - framework.Delete, From 6dbaa5660ea62c6b0634dbf20794550e0f5f5756 Mon Sep 17 00:00:00 2001 From: YamasouA Date: Wed, 2 Oct 2024 22:50:39 +0900 Subject: [PATCH 6/6] fix test --- .../framework/plugins/volumebinding/volume_binding_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/scheduler/framework/plugins/volumebinding/volume_binding_test.go b/pkg/scheduler/framework/plugins/volumebinding/volume_binding_test.go index e5d167dd08a..1f115b52a82 100644 --- a/pkg/scheduler/framework/plugins/volumebinding/volume_binding_test.go +++ b/pkg/scheduler/framework/plugins/volumebinding/volume_binding_test.go @@ -1418,7 +1418,7 @@ func TestIsSchedulableAfterCSIDriverChange(t *testing.T) { Name: "test2", }, Spec: storagev1.CSIDriverSpec{ - StorageCapacity: ptr.To(true), + StorageCapacity: ptr.To(false), }, }, oldObj: &storagev1.CSIDriver{ @@ -1426,7 +1426,7 @@ func TestIsSchedulableAfterCSIDriverChange(t *testing.T) { Name: "test2", }, Spec: storagev1.CSIDriverSpec{ - StorageCapacity: ptr.To(false), + StorageCapacity: ptr.To(true), }, }, err: false,