From de7c8db7cb450962cfca431db2ac75b895021242 Mon Sep 17 00:00:00 2001 From: kidddddddddddddddddddddd <1062602710@qq.com> Date: Sat, 7 Jan 2023 21:25:48 +0800 Subject: [PATCH 1/3] return skip --- .../plugins/volumezone/volume_zone.go | 8 +- .../plugins/volumezone/volume_zone_test.go | 106 +++++++++--------- 2 files changed, 57 insertions(+), 57 deletions(-) diff --git a/pkg/scheduler/framework/plugins/volumezone/volume_zone.go b/pkg/scheduler/framework/plugins/volumezone/volume_zone.go index 898441461b9..c17876c9b18 100644 --- a/pkg/scheduler/framework/plugins/volumezone/volume_zone.go +++ b/pkg/scheduler/framework/plugins/volumezone/volume_zone.go @@ -98,6 +98,9 @@ func (pl *VolumeZone) PreFilter(ctx context.Context, cs *framework.CycleState, p if !status.IsSuccess() { return nil, status } + if len(podPVTopologies) == 0 { + return nil, framework.NewStatus(framework.Skip) + } cs.Write(preFilterStateKey, &stateData{podPVTopologies: podPVTopologies}) return nil, nil } @@ -186,11 +189,6 @@ func (pl *VolumeZone) PreFilterExtensions() framework.PreFilterExtensions { // require calling out to the cloud provider. It seems that we are moving away // from inline volume declarations anyway. func (pl *VolumeZone) Filter(ctx context.Context, cs *framework.CycleState, pod *v1.Pod, nodeInfo *framework.NodeInfo) *framework.Status { - // 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. - if len(pod.Spec.Volumes) == 0 { - return nil - } var podPVTopologies []pvTopology state, err := getStateData(cs) if err != nil { diff --git a/pkg/scheduler/framework/plugins/volumezone/volume_zone_test.go b/pkg/scheduler/framework/plugins/volumezone/volume_zone_test.go index 3eae1eba6fe..b8ab61e9052 100644 --- a/pkg/scheduler/framework/plugins/volumezone/volume_zone_test.go +++ b/pkg/scheduler/framework/plugins/volumezone/volume_zone_test.go @@ -92,10 +92,11 @@ func TestSingleZone(t *testing.T) { } tests := []struct { - name string - Pod *v1.Pod - Node *v1.Node - wantStatus *framework.Status + name string + Pod *v1.Pod + Node *v1.Node + wantPreFilterStatus *framework.Status + wantFilterStatus *framework.Status }{ { name: "pod without volume", @@ -106,6 +107,7 @@ func TestSingleZone(t *testing.T) { Labels: map[string]string{v1.LabelFailureDomainBetaZone: "us-west1-a"}, }, }, + wantPreFilterStatus: framework.NewStatus(framework.Skip), }, { name: "node without labels", @@ -145,7 +147,7 @@ func TestSingleZone(t *testing.T) { Labels: map[string]string{v1.LabelFailureDomainBetaRegion: "no_us-west1", "uselessLabel": "none"}, }, }, - wantStatus: framework.NewStatus(framework.UnschedulableAndUnresolvable, ErrReasonConflict), + wantFilterStatus: framework.NewStatus(framework.UnschedulableAndUnresolvable, ErrReasonConflict), }, { name: "beta zone label doesn't match", @@ -156,7 +158,7 @@ func TestSingleZone(t *testing.T) { Labels: map[string]string{v1.LabelFailureDomainBetaZone: "no_us-west1-a", "uselessLabel": "none"}, }, }, - wantStatus: framework.NewStatus(framework.UnschedulableAndUnresolvable, ErrReasonConflict), + wantFilterStatus: framework.NewStatus(framework.UnschedulableAndUnresolvable, ErrReasonConflict), }, { name: "zone label matched", @@ -187,7 +189,7 @@ func TestSingleZone(t *testing.T) { Labels: map[string]string{v1.LabelTopologyRegion: "no_us-west1", "uselessLabel": "none"}, }, }, - wantStatus: framework.NewStatus(framework.UnschedulableAndUnresolvable, ErrReasonConflict), + wantFilterStatus: framework.NewStatus(framework.UnschedulableAndUnresolvable, ErrReasonConflict), }, { name: "zone label doesn't match", @@ -198,7 +200,7 @@ func TestSingleZone(t *testing.T) { Labels: map[string]string{v1.LabelTopologyZone: "no_us-west1-a", "uselessLabel": "none"}, }, }, - wantStatus: framework.NewStatus(framework.UnschedulableAndUnresolvable, ErrReasonConflict), + wantFilterStatus: framework.NewStatus(framework.UnschedulableAndUnresolvable, ErrReasonConflict), }, { name: "pv with zone and region, node with only zone", @@ -211,7 +213,7 @@ func TestSingleZone(t *testing.T) { }, }, }, - wantStatus: framework.NewStatus(framework.UnschedulableAndUnresolvable, ErrReasonConflict), + wantFilterStatus: framework.NewStatus(framework.UnschedulableAndUnresolvable, ErrReasonConflict), }, } @@ -229,14 +231,13 @@ func TestSingleZone(t *testing.T) { nil, } - _, gotPreFilterStatus := p.PreFilter(ctx, state, test.Pod) - if !gotPreFilterStatus.IsSuccess() { - if diff := cmp.Diff(gotPreFilterStatus, test.wantStatus); diff != "" { - t.Errorf("status does not match (-want,+got):\n%s", diff) - } - } else { - gotStatus := p.Filter(ctx, state, test.Pod, node) - if diff := cmp.Diff(gotStatus, test.wantStatus); diff != "" { + _, preFilterStatus := p.PreFilter(ctx, state, test.Pod) + if diff := cmp.Diff(preFilterStatus, test.wantPreFilterStatus); diff != "" { + t.Errorf("status does not match (-want,+got):\n%s", diff) + } + if preFilterStatus.IsSuccess() { + filterStatus := p.Filter(ctx, state, test.Pod, node) + if diff := cmp.Diff(filterStatus, test.wantFilterStatus); diff != "" { t.Errorf("status does not match (-want,+got):\n%s", diff) } } @@ -291,10 +292,11 @@ func TestMultiZone(t *testing.T) { } tests := []struct { - name string - Pod *v1.Pod - Node *v1.Node - wantStatus *framework.Status + name string + Pod *v1.Pod + Node *v1.Node + wantPreFilterStatus *framework.Status + wantFilterStatus *framework.Status }{ { name: "node without labels", @@ -324,7 +326,7 @@ func TestMultiZone(t *testing.T) { Labels: map[string]string{v1.LabelFailureDomainBetaZone: "us-west1-b", "uselessLabel": "none"}, }, }, - wantStatus: framework.NewStatus(framework.UnschedulableAndUnresolvable, ErrReasonConflict), + wantFilterStatus: framework.NewStatus(framework.UnschedulableAndUnresolvable, ErrReasonConflict), }, { name: "zone label matched", @@ -345,7 +347,7 @@ func TestMultiZone(t *testing.T) { Labels: map[string]string{v1.LabelTopologyZone: "us-west1-b", "uselessLabel": "none"}, }, }, - wantStatus: framework.NewStatus(framework.UnschedulableAndUnresolvable, ErrReasonConflict), + wantFilterStatus: framework.NewStatus(framework.UnschedulableAndUnresolvable, ErrReasonConflict), }, } @@ -362,14 +364,13 @@ func TestMultiZone(t *testing.T) { pvcLister, nil, } - _, gotPreFilterStatus := p.PreFilter(ctx, state, test.Pod) - if !gotPreFilterStatus.IsSuccess() { - if diff := cmp.Diff(gotPreFilterStatus, test.wantStatus); diff != "" { - t.Errorf("status does not match (-want,+got):\n%s", diff) - } - } else { - gotStatus := p.Filter(context.Background(), state, test.Pod, node) - if diff := cmp.Diff(gotStatus, test.wantStatus); diff != "" { + _, preFilterStatus := p.PreFilter(ctx, state, test.Pod) + if diff := cmp.Diff(preFilterStatus, test.wantPreFilterStatus); diff != "" { + t.Errorf("status does not match (-want,+got):\n%s", diff) + } + if preFilterStatus.IsSuccess() { + filterStatus := p.Filter(context.Background(), state, test.Pod, node) + if diff := cmp.Diff(filterStatus, test.wantFilterStatus); diff != "" { t.Errorf("status does not match (-want,+got):\n%s", diff) } } @@ -432,10 +433,11 @@ func TestWithBinding(t *testing.T) { } tests := []struct { - name string - Pod *v1.Pod - Node *v1.Node - wantStatus *framework.Status + name string + Pod *v1.Pod + Node *v1.Node + wantPreFilterStatus *framework.Status + wantFilterStatus *framework.Status }{ { name: "label zone failure domain matched", @@ -446,26 +448,27 @@ func TestWithBinding(t *testing.T) { name: "unbound volume empty storage class", Pod: createPodWithVolume("pod_1", "vol_1", "PVC_EmptySC"), Node: testNode, - wantStatus: framework.NewStatus(framework.UnschedulableAndUnresolvable, + wantPreFilterStatus: framework.NewStatus(framework.UnschedulableAndUnresolvable, "PersistentVolumeClaim had no pv name and storageClass name"), }, { name: "unbound volume no storage class", Pod: createPodWithVolume("pod_1", "vol_1", "PVC_NoSC"), Node: testNode, - wantStatus: framework.NewStatus(framework.UnschedulableAndUnresolvable, + wantPreFilterStatus: framework.NewStatus(framework.UnschedulableAndUnresolvable, "unable to find storage class: Class_0"), }, { - name: "unbound volume immediate binding mode", - Pod: createPodWithVolume("pod_1", "vol_1", "PVC_ImmediateSC"), - Node: testNode, - wantStatus: framework.NewStatus(framework.UnschedulableAndUnresolvable, "VolumeBindingMode not set for StorageClass \"Class_Immediate\""), + name: "unbound volume immediate binding mode", + Pod: createPodWithVolume("pod_1", "vol_1", "PVC_ImmediateSC"), + Node: testNode, + wantPreFilterStatus: framework.NewStatus(framework.UnschedulableAndUnresolvable, "VolumeBindingMode not set for StorageClass \"Class_Immediate\""), }, { - name: "unbound volume wait binding mode", - Pod: createPodWithVolume("pod_1", "vol_1", "PVC_WaitSC"), - Node: testNode, + name: "unbound volume wait binding mode", + Pod: createPodWithVolume("pod_1", "vol_1", "PVC_WaitSC"), + Node: testNode, + wantPreFilterStatus: framework.NewStatus(framework.Skip), }, } @@ -482,14 +485,13 @@ func TestWithBinding(t *testing.T) { pvcLister, scLister, } - _, gotPreFilterStatus := p.PreFilter(ctx, state, test.Pod) - if !gotPreFilterStatus.IsSuccess() { - if diff := cmp.Diff(gotPreFilterStatus, test.wantStatus); diff != "" { - t.Errorf("status does not match (-want,+got):\n%s", diff) - } - } else { - gotStatus := p.Filter(ctx, state, test.Pod, node) - if diff := cmp.Diff(gotStatus, test.wantStatus); diff != "" { + _, preFilterStatus := p.PreFilter(ctx, state, test.Pod) + if diff := cmp.Diff(preFilterStatus, test.wantPreFilterStatus); diff != "" { + t.Errorf("status does not match (-want,+got):\n%s", diff) + } + if preFilterStatus.IsSuccess() { + filterStatus := p.Filter(ctx, state, test.Pod, node) + if diff := cmp.Diff(filterStatus, test.wantFilterStatus); diff != "" { t.Errorf("status does not match (-want,+got):\n%s", diff) } } From 0abdf6abc211fda4392138207ffc6c7a5ce2da37 Mon Sep 17 00:00:00 2001 From: kidddddddddddddddddddddd <1062602710@qq.com> Date: Sat, 7 Jan 2023 22:30:16 +0800 Subject: [PATCH 2/3] revert check in filter --- pkg/scheduler/framework/plugins/volumezone/volume_zone.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/scheduler/framework/plugins/volumezone/volume_zone.go b/pkg/scheduler/framework/plugins/volumezone/volume_zone.go index c17876c9b18..e2d03865cc3 100644 --- a/pkg/scheduler/framework/plugins/volumezone/volume_zone.go +++ b/pkg/scheduler/framework/plugins/volumezone/volume_zone.go @@ -189,6 +189,11 @@ func (pl *VolumeZone) PreFilterExtensions() framework.PreFilterExtensions { // require calling out to the cloud provider. It seems that we are moving away // from inline volume declarations anyway. func (pl *VolumeZone) Filter(ctx context.Context, cs *framework.CycleState, pod *v1.Pod, nodeInfo *framework.NodeInfo) *framework.Status { + // 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. + if len(pod.Spec.Volumes) == 0 { + return nil + } var podPVTopologies []pvTopology state, err := getStateData(cs) if err != nil { From 733d5695f25dcbffe863237a55091cb1b3534bf0 Mon Sep 17 00:00:00 2001 From: kidddddddddddddddddddddd <1062602710@qq.com> Date: Sun, 8 Jan 2023 11:12:51 +0800 Subject: [PATCH 3/3] always run filter in test --- .../plugins/volumezone/volume_zone_test.go | 36 +++++++++---------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/pkg/scheduler/framework/plugins/volumezone/volume_zone_test.go b/pkg/scheduler/framework/plugins/volumezone/volume_zone_test.go index b8ab61e9052..47711b1c64e 100644 --- a/pkg/scheduler/framework/plugins/volumezone/volume_zone_test.go +++ b/pkg/scheduler/framework/plugins/volumezone/volume_zone_test.go @@ -230,16 +230,13 @@ func TestSingleZone(t *testing.T) { pvcLister, nil, } - _, preFilterStatus := p.PreFilter(ctx, state, test.Pod) if diff := cmp.Diff(preFilterStatus, test.wantPreFilterStatus); diff != "" { - t.Errorf("status does not match (-want,+got):\n%s", diff) + t.Errorf("PreFilter: status does not match (-want,+got):\n%s", diff) } - if preFilterStatus.IsSuccess() { - filterStatus := p.Filter(ctx, state, test.Pod, node) - if diff := cmp.Diff(filterStatus, test.wantFilterStatus); diff != "" { - t.Errorf("status does not match (-want,+got):\n%s", diff) - } + filterStatus := p.Filter(ctx, state, test.Pod, node) + if diff := cmp.Diff(filterStatus, test.wantFilterStatus); diff != "" { + t.Errorf("Filter: status does not match (-want,+got):\n%s", diff) } }) } @@ -366,13 +363,11 @@ func TestMultiZone(t *testing.T) { } _, preFilterStatus := p.PreFilter(ctx, state, test.Pod) if diff := cmp.Diff(preFilterStatus, test.wantPreFilterStatus); diff != "" { - t.Errorf("status does not match (-want,+got):\n%s", diff) + t.Errorf("PreFilter: status does not match (-want,+got):\n%s", diff) } - if preFilterStatus.IsSuccess() { - filterStatus := p.Filter(context.Background(), state, test.Pod, node) - if diff := cmp.Diff(filterStatus, test.wantFilterStatus); diff != "" { - t.Errorf("status does not match (-want,+got):\n%s", diff) - } + filterStatus := p.Filter(ctx, state, test.Pod, node) + if diff := cmp.Diff(filterStatus, test.wantFilterStatus); diff != "" { + t.Errorf("Filter: status does not match (-want,+got):\n%s", diff) } }) } @@ -450,6 +445,8 @@ func TestWithBinding(t *testing.T) { Node: testNode, wantPreFilterStatus: framework.NewStatus(framework.UnschedulableAndUnresolvable, "PersistentVolumeClaim had no pv name and storageClass name"), + wantFilterStatus: framework.NewStatus(framework.UnschedulableAndUnresolvable, + "PersistentVolumeClaim had no pv name and storageClass name"), }, { name: "unbound volume no storage class", @@ -457,12 +454,15 @@ func TestWithBinding(t *testing.T) { Node: testNode, wantPreFilterStatus: framework.NewStatus(framework.UnschedulableAndUnresolvable, "unable to find storage class: Class_0"), + wantFilterStatus: framework.NewStatus(framework.UnschedulableAndUnresolvable, + "unable to find storage class: Class_0"), }, { name: "unbound volume immediate binding mode", Pod: createPodWithVolume("pod_1", "vol_1", "PVC_ImmediateSC"), Node: testNode, wantPreFilterStatus: framework.NewStatus(framework.UnschedulableAndUnresolvable, "VolumeBindingMode not set for StorageClass \"Class_Immediate\""), + wantFilterStatus: framework.NewStatus(framework.UnschedulableAndUnresolvable, "VolumeBindingMode not set for StorageClass \"Class_Immediate\""), }, { name: "unbound volume wait binding mode", @@ -487,13 +487,11 @@ func TestWithBinding(t *testing.T) { } _, preFilterStatus := p.PreFilter(ctx, state, test.Pod) if diff := cmp.Diff(preFilterStatus, test.wantPreFilterStatus); diff != "" { - t.Errorf("status does not match (-want,+got):\n%s", diff) + t.Errorf("PreFilter: status does not match (-want,+got):\n%s", diff) } - if preFilterStatus.IsSuccess() { - filterStatus := p.Filter(ctx, state, test.Pod, node) - if diff := cmp.Diff(filterStatus, test.wantFilterStatus); diff != "" { - t.Errorf("status does not match (-want,+got):\n%s", diff) - } + filterStatus := p.Filter(ctx, state, test.Pod, node) + if diff := cmp.Diff(filterStatus, test.wantFilterStatus); diff != "" { + t.Errorf("Filter: status does not match (-want,+got):\n%s", diff) } }) }