Skip to content

Commit 3e65fe4

Browse files
chendavechendave
committed
Change the exception to avoid the cost of preemption
node's labels doesn't contain the required topologyKeys in `Constraints` cannot be resolved by preempting the pods on that pods. One use case that could easily reproduce the issue is, - set `alwaysCheckAllPredicates` to true. - one node contains all the required topologyKeys but is failed in predicates such as 'taint'. - another node doesn't hold all the required topologyKeys, and thus return `Unschedulable` status code. - scheduler will try to preempt the pods on the above node with lower priorities. Signed-off-by: Dave Chen <[email protected]>
1 parent 36083e4 commit 3e65fe4

File tree

4 files changed

+131
-94
lines changed

4 files changed

+131
-94
lines changed

‎pkg/scheduler/framework/plugins/defaultpreemption/default_preemption_test.go‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -958,7 +958,7 @@ func TestNodesWherePreemptionMightHelp(t *testing.T) {
958958
expected: map[string]bool{"node4": true},
959959
},
960960
{
961-
name: "ErrTopologySpreadConstraintsNotMatch should be tried as it indicates that the pod is unschedulable due to topology spread constraints",
961+
name: "ErrReasonConstraintsNotMatch should be tried as it indicates that the pod is unschedulable due to topology spread constraints",
962962
nodesStatuses: framework.NodeToStatusMap{
963963
"node1": framework.NewStatus(framework.Unschedulable, podtopologyspread.ErrReasonConstraintsNotMatch),
964964
"node2": framework.NewStatus(framework.UnschedulableAndUnresolvable, nodename.ErrReason),
@@ -975,6 +975,15 @@ func TestNodesWherePreemptionMightHelp(t *testing.T) {
975975
},
976976
expected: map[string]bool{"node1": true, "node3": true},
977977
},
978+
{
979+
name: "ErrReasonNodeLabelNotMatch should not be tried as it indicates that the pod is unschedulable due to node doesn't have the required label",
980+
nodesStatuses: framework.NodeToStatusMap{
981+
"node2": framework.NewStatus(framework.UnschedulableAndUnresolvable, podtopologyspread.ErrReasonNodeLabelNotMatch),
982+
"node3": framework.NewStatus(framework.Unschedulable, ""),
983+
"node4": framework.NewStatus(framework.UnschedulableAndUnresolvable, ""),
984+
},
985+
expected: map[string]bool{"node1": true, "node3": true},
986+
},
978987
}
979988

980989
for _, tt := range tests {

‎pkg/scheduler/framework/plugins/podtopologyspread/filtering.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -295,7 +295,7 @@ func (pl *PodTopologySpread) Filter(ctx context.Context, cycleState *framework.C
295295
tpVal, ok := node.Labels[c.TopologyKey]
296296
if !ok {
297297
klog.V(5).Infof("node '%s' doesn't have required label '%s'", node.Name, tpKey)
298-
return framework.NewStatus(framework.Unschedulable, ErrReasonConstraintsNotMatch)
298+
return framework.NewStatus(framework.UnschedulableAndUnresolvable, ErrReasonNodeLabelNotMatch)
299299
}
300300

301301
selfMatchNum := int32(0)

‎pkg/scheduler/framework/plugins/podtopologyspread/filtering_test.go‎

Lines changed: 118 additions & 92 deletions
Original file line numberDiff line numberDiff line change
@@ -1145,11 +1145,11 @@ func mustConvertLabelSelectorAsSelector(t *testing.T, ls *metav1.LabelSelector)
11451145

11461146
func TestSingleConstraint(t *testing.T) {
11471147
tests := []struct {
1148-
name string
1149-
pod *v1.Pod
1150-
nodes []*v1.Node
1151-
existingPods []*v1.Pod
1152-
fits map[string]bool
1148+
name string
1149+
pod *v1.Pod
1150+
nodes []*v1.Node
1151+
existingPods []*v1.Pod
1152+
wantStatusCode map[string]framework.Code
11531153
}{
11541154
{
11551155
name: "no existing pods",
@@ -1162,11 +1162,11 @@ func TestSingleConstraint(t *testing.T) {
11621162
st.MakeNode().Name("node-x").Label("zone", "zone2").Label("node", "node-x").Obj(),
11631163
st.MakeNode().Name("node-y").Label("zone", "zone2").Label("node", "node-y").Obj(),
11641164
},
1165-
fits: map[string]bool{
1166-
"node-a": true,
1167-
"node-b": true,
1168-
"node-x": true,
1169-
"node-y": true,
1165+
wantStatusCode: map[string]framework.Code{
1166+
"node-a": framework.Success,
1167+
"node-b": framework.Success,
1168+
"node-x": framework.Success,
1169+
"node-y": framework.Success,
11701170
},
11711171
},
11721172
{
@@ -1180,11 +1180,11 @@ func TestSingleConstraint(t *testing.T) {
11801180
st.MakeNode().Name("node-x").Label("zone", "zone2").Label("node", "node-x").Obj(),
11811181
st.MakeNode().Name("node-y").Label("zone", "zone2").Label("node", "node-y").Obj(),
11821182
},
1183-
fits: map[string]bool{
1184-
"node-a": true,
1185-
"node-b": true,
1186-
"node-x": true,
1187-
"node-y": true,
1183+
wantStatusCode: map[string]framework.Code{
1184+
"node-a": framework.Success,
1185+
"node-b": framework.Success,
1186+
"node-x": framework.Success,
1187+
"node-y": framework.Success,
11881188
},
11891189
},
11901190
{
@@ -1204,11 +1204,11 @@ func TestSingleConstraint(t *testing.T) {
12041204
st.MakePod().Name("p-x1").Node("node-x").Label("foo", "").Obj(),
12051205
st.MakePod().Name("p-y1").Node("node-y").Label("foo", "").Obj(),
12061206
},
1207-
fits: map[string]bool{
1208-
"node-a": true,
1209-
"node-b": true,
1210-
"node-x": false,
1211-
"node-y": false,
1207+
wantStatusCode: map[string]framework.Code{
1208+
"node-a": framework.Success,
1209+
"node-b": framework.Success,
1210+
"node-x": framework.Unschedulable,
1211+
"node-y": framework.Unschedulable,
12121212
},
12131213
},
12141214
{
@@ -1230,11 +1230,11 @@ func TestSingleConstraint(t *testing.T) {
12301230
st.MakePod().Name("p-y2").Node("node-y").Label("foo", "").Obj(),
12311231
st.MakePod().Name("p-y3").Node("node-y").Label("foo", "").Obj(),
12321232
},
1233-
fits: map[string]bool{
1234-
"node-a": true,
1235-
"node-b": true,
1236-
"node-x": true,
1237-
"node-y": true,
1233+
wantStatusCode: map[string]framework.Code{
1234+
"node-a": framework.Success,
1235+
"node-b": framework.Success,
1236+
"node-x": framework.Success,
1237+
"node-y": framework.Success,
12381238
},
12391239
},
12401240
{
@@ -1256,11 +1256,11 @@ func TestSingleConstraint(t *testing.T) {
12561256
st.MakePod().Name("p-x1").Node("node-x").Label("foo", "").Obj(),
12571257
st.MakePod().Name("p-y1").Node("node-y").Label("foo", "").Obj(),
12581258
},
1259-
fits: map[string]bool{
1260-
"node-a": true,
1261-
"node-b": false,
1262-
"node-x": false,
1263-
"node-y": false,
1259+
wantStatusCode: map[string]framework.Code{
1260+
"node-a": framework.Success,
1261+
"node-b": framework.UnschedulableAndUnresolvable,
1262+
"node-x": framework.Unschedulable,
1263+
"node-y": framework.Unschedulable,
12641264
},
12651265
},
12661266
{
@@ -1282,11 +1282,11 @@ func TestSingleConstraint(t *testing.T) {
12821282
st.MakePod().Name("p-y2").Node("node-y").Label("foo", "").Obj(),
12831283
st.MakePod().Name("p-y3").Node("node-y").Label("foo", "").Obj(),
12841284
},
1285-
fits: map[string]bool{
1286-
"node-a": false,
1287-
"node-b": false,
1288-
"node-x": true,
1289-
"node-y": false,
1285+
wantStatusCode: map[string]framework.Code{
1286+
"node-a": framework.Unschedulable,
1287+
"node-b": framework.Unschedulable,
1288+
"node-x": framework.Success,
1289+
"node-y": framework.Unschedulable,
12901290
},
12911291
},
12921292
{
@@ -1308,11 +1308,11 @@ func TestSingleConstraint(t *testing.T) {
13081308
st.MakePod().Name("p-y2").Node("node-y").Label("foo", "").Obj(),
13091309
st.MakePod().Name("p-y3").Node("node-y").Label("foo", "").Obj(),
13101310
},
1311-
fits: map[string]bool{
1312-
"node-a": false,
1313-
"node-b": true,
1314-
"node-x": true,
1315-
"node-y": false,
1311+
wantStatusCode: map[string]framework.Code{
1312+
"node-a": framework.Unschedulable,
1313+
"node-b": framework.Success,
1314+
"node-x": framework.Success,
1315+
"node-y": framework.Unschedulable,
13161316
},
13171317
},
13181318
{
@@ -1338,11 +1338,11 @@ func TestSingleConstraint(t *testing.T) {
13381338
st.MakePod().Name("p-y2").Node("node-y").Label("foo", "").Obj(),
13391339
st.MakePod().Name("p-y3").Node("node-y").Label("foo", "").Obj(),
13401340
},
1341-
fits: map[string]bool{
1342-
"node-a": false,
1343-
"node-b": true,
1344-
"node-x": true,
1345-
"node-y": false,
1341+
wantStatusCode: map[string]framework.Code{
1342+
"node-a": framework.Unschedulable,
1343+
"node-b": framework.Success,
1344+
"node-x": framework.Success,
1345+
"node-y": framework.Unschedulable,
13461346
},
13471347
},
13481348
{
@@ -1370,11 +1370,11 @@ func TestSingleConstraint(t *testing.T) {
13701370
st.MakePod().Name("p-y2").Node("node-y").Label("foo", "").Obj(),
13711371
st.MakePod().Name("p-y3").Node("node-y").Label("foo", "").Obj(),
13721372
},
1373-
fits: map[string]bool{
1374-
"node-a": true,
1375-
"node-b": true, // in real case, it's false
1376-
"node-x": true, // in real case, it's false
1377-
"node-y": false,
1373+
wantStatusCode: map[string]framework.Code{
1374+
"node-a": framework.Success,
1375+
"node-b": framework.Success, // in real case, it's false
1376+
"node-x": framework.Success, // in real case, it's false
1377+
"node-y": framework.Unschedulable,
13781378
},
13791379
},
13801380
{
@@ -1390,9 +1390,9 @@ func TestSingleConstraint(t *testing.T) {
13901390
st.MakePod().Name("p-a").Node("node-a").Label("foo", "").Terminating().Obj(),
13911391
st.MakePod().Name("p-b").Node("node-b").Label("foo", "").Obj(),
13921392
},
1393-
fits: map[string]bool{
1394-
"node-a": true,
1395-
"node-b": false,
1393+
wantStatusCode: map[string]framework.Code{
1394+
"node-a": framework.Success,
1395+
"node-b": framework.Unschedulable,
13961396
},
13971397
},
13981398
}
@@ -1409,8 +1409,8 @@ func TestSingleConstraint(t *testing.T) {
14091409
for _, node := range tt.nodes {
14101410
nodeInfo, _ := snapshot.NodeInfos().Get(node.Name)
14111411
status := p.Filter(context.Background(), state, tt.pod, nodeInfo)
1412-
if status.IsSuccess() != tt.fits[node.Name] {
1413-
t.Errorf("[%s]: expected %v got %v", node.Name, tt.fits[node.Name], status.IsSuccess())
1412+
if len(tt.wantStatusCode) != 0 && status.Code() != tt.wantStatusCode[node.Name] {
1413+
t.Errorf("[%s]: expected status code %v got %v", node.Name, tt.wantStatusCode[node.Name], status.Code())
14141414
}
14151415
}
14161416
})
@@ -1419,11 +1419,11 @@ func TestSingleConstraint(t *testing.T) {
14191419

14201420
func TestMultipleConstraints(t *testing.T) {
14211421
tests := []struct {
1422-
name string
1423-
pod *v1.Pod
1424-
nodes []*v1.Node
1425-
existingPods []*v1.Pod
1426-
fits map[string]bool
1422+
name string
1423+
pod *v1.Pod
1424+
nodes []*v1.Node
1425+
existingPods []*v1.Pod
1426+
wantStatusCode map[string]framework.Code
14271427
}{
14281428
{
14291429
// 1. to fulfil "zone" constraint, incoming pod can be placed on any zone (hence any node)
@@ -1448,11 +1448,11 @@ func TestMultipleConstraints(t *testing.T) {
14481448
st.MakePod().Name("p-y2").Node("node-y").Label("foo", "").Obj(),
14491449
st.MakePod().Name("p-y3").Node("node-y").Label("foo", "").Obj(),
14501450
},
1451-
fits: map[string]bool{
1452-
"node-a": false,
1453-
"node-b": false,
1454-
"node-x": true,
1455-
"node-y": false,
1451+
wantStatusCode: map[string]framework.Code{
1452+
"node-a": framework.Unschedulable,
1453+
"node-b": framework.Unschedulable,
1454+
"node-x": framework.Success,
1455+
"node-y": framework.Unschedulable,
14561456
},
14571457
},
14581458
{
@@ -1479,11 +1479,11 @@ func TestMultipleConstraints(t *testing.T) {
14791479
st.MakePod().Name("p-y3").Node("node-y").Label("foo", "").Obj(),
14801480
st.MakePod().Name("p-y4").Node("node-y").Label("foo", "").Obj(),
14811481
},
1482-
fits: map[string]bool{
1483-
"node-a": false,
1484-
"node-b": false,
1485-
"node-x": false,
1486-
"node-y": false,
1482+
wantStatusCode: map[string]framework.Code{
1483+
"node-a": framework.Unschedulable,
1484+
"node-b": framework.Unschedulable,
1485+
"node-x": framework.Unschedulable,
1486+
"node-y": framework.Unschedulable,
14871487
},
14881488
},
14891489
{
@@ -1505,11 +1505,11 @@ func TestMultipleConstraints(t *testing.T) {
15051505
st.MakePod().Name("p-a1").Node("node-a").Label("foo", "").Obj(),
15061506
st.MakePod().Name("p-y1").Node("node-y").Label("bar", "").Obj(),
15071507
},
1508-
fits: map[string]bool{
1509-
"node-a": false,
1510-
"node-b": false,
1511-
"node-x": true,
1512-
"node-y": false,
1508+
wantStatusCode: map[string]framework.Code{
1509+
"node-a": framework.Unschedulable,
1510+
"node-b": framework.Unschedulable,
1511+
"node-x": framework.Success,
1512+
"node-y": framework.Unschedulable,
15131513
},
15141514
},
15151515
{
@@ -1532,11 +1532,11 @@ func TestMultipleConstraints(t *testing.T) {
15321532
st.MakePod().Name("p-x1").Node("node-x").Label("bar", "").Obj(),
15331533
st.MakePod().Name("p-y1").Node("node-y").Label("bar", "").Obj(),
15341534
},
1535-
fits: map[string]bool{
1536-
"node-a": false,
1537-
"node-b": false,
1538-
"node-x": false,
1539-
"node-y": false,
1535+
wantStatusCode: map[string]framework.Code{
1536+
"node-a": framework.Unschedulable,
1537+
"node-b": framework.Unschedulable,
1538+
"node-x": framework.Unschedulable,
1539+
"node-y": framework.Unschedulable,
15401540
},
15411541
},
15421542
{
@@ -1561,11 +1561,11 @@ func TestMultipleConstraints(t *testing.T) {
15611561
st.MakePod().Name("p-y2").Node("node-y").Label("foo", "").Label("bar", "").Obj(),
15621562
st.MakePod().Name("p-y3").Node("node-y").Label("foo", "").Obj(),
15631563
},
1564-
fits: map[string]bool{
1565-
"node-a": false,
1566-
"node-b": true,
1567-
"node-x": false,
1568-
"node-y": false,
1564+
wantStatusCode: map[string]framework.Code{
1565+
"node-a": framework.Unschedulable,
1566+
"node-b": framework.Success,
1567+
"node-x": framework.Unschedulable,
1568+
"node-y": framework.Unschedulable,
15691569
},
15701570
},
15711571
{
@@ -1588,11 +1588,37 @@ func TestMultipleConstraints(t *testing.T) {
15881588
st.MakePod().Name("p-x1").Node("node-x").Label("bar", "").Obj(),
15891589
st.MakePod().Name("p-y1").Node("node-y").Label("bar", "").Obj(),
15901590
},
1591-
fits: map[string]bool{
1592-
"node-a": true,
1593-
"node-b": true,
1594-
"node-x": false,
1595-
"node-y": false,
1591+
wantStatusCode: map[string]framework.Code{
1592+
"node-a": framework.Success,
1593+
"node-b": framework.Success,
1594+
"node-x": framework.Unschedulable,
1595+
"node-y": framework.Unschedulable,
1596+
},
1597+
},
1598+
{
1599+
// 1. to fulfil "zone" constraint, incoming pod can be placed on any zone (hence any node)
1600+
// 2. to fulfil "node" constraint, incoming pod can be placed on node-b (node-x doesn't have the required label)
1601+
// intersection of (1) and (2) returns node-b
1602+
name: "two Constraints on zone and node, absence of label 'node' on node-x, spreads = [1/1, 1/0/0/1]",
1603+
pod: st.MakePod().Name("p").Label("foo", "").
1604+
SpreadConstraint(1, "zone", v1.DoNotSchedule, st.MakeLabelSelector().Exists("foo").Obj()).
1605+
SpreadConstraint(1, "node", v1.DoNotSchedule, st.MakeLabelSelector().Exists("foo").Obj()).
1606+
Obj(),
1607+
nodes: []*v1.Node{
1608+
st.MakeNode().Name("node-a").Label("zone", "zone1").Label("node", "node-a").Obj(),
1609+
st.MakeNode().Name("node-b").Label("zone", "zone1").Label("node", "node-b").Obj(),
1610+
st.MakeNode().Name("node-x").Label("zone", "zone2").Obj(),
1611+
st.MakeNode().Name("node-y").Label("zone", "zone2").Label("node", "node-y").Obj(),
1612+
},
1613+
existingPods: []*v1.Pod{
1614+
st.MakePod().Name("p-a1").Node("node-a").Label("foo", "").Obj(),
1615+
st.MakePod().Name("p-y3").Node("node-y").Label("foo", "").Obj(),
1616+
},
1617+
wantStatusCode: map[string]framework.Code{
1618+
"node-a": framework.Unschedulable,
1619+
"node-b": framework.Success,
1620+
"node-x": framework.UnschedulableAndUnresolvable,
1621+
"node-y": framework.Unschedulable,
15961622
},
15971623
},
15981624
}
@@ -1609,8 +1635,8 @@ func TestMultipleConstraints(t *testing.T) {
16091635
for _, node := range tt.nodes {
16101636
nodeInfo, _ := snapshot.NodeInfos().Get(node.Name)
16111637
status := p.Filter(context.Background(), state, tt.pod, nodeInfo)
1612-
if status.IsSuccess() != tt.fits[node.Name] {
1613-
t.Errorf("[%s]: expected %v got %v", node.Name, tt.fits[node.Name], status.IsSuccess())
1638+
if len(tt.wantStatusCode) != 0 && status.Code() != tt.wantStatusCode[node.Name] {
1639+
t.Errorf("[%s]: expected error code %v got %v", node.Name, tt.wantStatusCode[node.Name], status.Code())
16141640
}
16151641
}
16161642
})

‎pkg/scheduler/framework/plugins/podtopologyspread/plugin.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@ import (
3131
const (
3232
// ErrReasonConstraintsNotMatch is used for PodTopologySpread filter error.
3333
ErrReasonConstraintsNotMatch = "node(s) didn't match pod topology spread constraints"
34+
// ErrReasonNodeLabelNotMatch is used when the node doesn't hold the required label.
35+
ErrReasonNodeLabelNotMatch = ErrReasonConstraintsNotMatch + " (missing required label)"
3436
)
3537

3638
// PodTopologySpread is a plugin that ensures pod's topologySpreadConstraints is satisfied.

0 commit comments

Comments
 (0)