Skip to content

Commit 17b9096

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #1629 from googs1025/fix_sortDomains
fix removepodsviolatingtopologyspreadconstraint plugin sort logic
2 parents 2cce60d + b4b203c commit 17b9096

File tree

2 files changed

+237
-6
lines changed

2 files changed

+237
-6
lines changed

‎pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go

+11-6
Original file line numberDiff line numberDiff line change
@@ -417,18 +417,23 @@ func sortDomains(constraintTopologyPairs map[topologyPair][]*v1.Pod, isEvictable
417417
// followed by the highest priority pods with affinity or nodeSelector
418418
sort.Slice(list, func(i, j int) bool {
419419
// any non-evictable pods should be considered last (ie, first in the list)
420-
if !isEvictable(list[i]) || !isEvictable(list[j]) {
420+
evictableI := isEvictable(list[i])
421+
evictableJ := isEvictable(list[j])
422+
423+
if !evictableI || !evictableJ {
421424
// false - i is the only non-evictable, so return true to put it first
422425
// true - j is non-evictable, so return false to put j before i
423426
// if true and both and non-evictable, order doesn't matter
424-
return !(isEvictable(list[i]) && !isEvictable(list[j]))
427+
return !(evictableI && !evictableJ)
425428
}
426-
429+
hasSelectorOrAffinityI := hasSelectorOrAffinity(*list[i])
430+
hasSelectorOrAffinityJ := hasSelectorOrAffinity(*list[j])
427431
// if both pods have selectors/affinity, compare them by their priority
428-
if hasSelectorOrAffinity(*list[i]) == hasSelectorOrAffinity(*list[j]) {
429-
comparePodsByPriority(list[i], list[j])
432+
if hasSelectorOrAffinityI == hasSelectorOrAffinityJ {
433+
// Sort by priority in ascending order (lower priority Pods first)
434+
return !comparePodsByPriority(list[i], list[j])
430435
}
431-
return hasSelectorOrAffinity(*list[i]) && !hasSelectorOrAffinity(*list[j])
436+
return hasSelectorOrAffinityI && !hasSelectorOrAffinityJ
432437
})
433438
sortedTopologies = append(sortedTopologies, topology{pair: pair, pods: list})
434439
}

‎pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint_test.go

+226
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package removepodsviolatingtopologyspreadconstraint
33
import (
44
"context"
55
"fmt"
6+
"reflect"
67
"sort"
78
"testing"
89

@@ -1476,6 +1477,231 @@ func TestTopologySpreadConstraint(t *testing.T) {
14761477
}
14771478
}
14781479

1480+
func TestSortDomains(t *testing.T) {
1481+
tests := []struct {
1482+
name string
1483+
constraintTopology map[topologyPair][]*v1.Pod
1484+
want []topology
1485+
}{
1486+
{
1487+
name: "empty input",
1488+
constraintTopology: map[topologyPair][]*v1.Pod{},
1489+
want: []topology{},
1490+
},
1491+
{
1492+
name: "single domain with mixed pods",
1493+
constraintTopology: map[topologyPair][]*v1.Pod{
1494+
{"zone", "a"}: {
1495+
{
1496+
ObjectMeta: metav1.ObjectMeta{
1497+
Name: "non-evictable-pod",
1498+
Annotations: map[string]string{"evictable": "false"},
1499+
},
1500+
},
1501+
{
1502+
ObjectMeta: metav1.ObjectMeta{
1503+
Name: "evictable-with-affinity",
1504+
Annotations: map[string]string{"evictable": "true", "hasSelectorOrAffinity": "true"},
1505+
},
1506+
Spec: v1.PodSpec{
1507+
Priority: utilptr.To[int32](10),
1508+
},
1509+
},
1510+
{
1511+
ObjectMeta: metav1.ObjectMeta{
1512+
Name: "evictable-high-priority",
1513+
Annotations: map[string]string{"evictable": "true"},
1514+
},
1515+
Spec: v1.PodSpec{
1516+
Priority: utilptr.To[int32](15),
1517+
},
1518+
},
1519+
},
1520+
},
1521+
want: []topology{
1522+
{pair: topologyPair{"zone", "a"}, pods: []*v1.Pod{
1523+
{
1524+
ObjectMeta: metav1.ObjectMeta{
1525+
Name: "non-evictable-pod",
1526+
Annotations: map[string]string{"evictable": "false"},
1527+
},
1528+
},
1529+
{
1530+
ObjectMeta: metav1.ObjectMeta{
1531+
Name: "evictable-with-affinity",
1532+
Annotations: map[string]string{"evictable": "true", "hasSelectorOrAffinity": "true"},
1533+
},
1534+
Spec: v1.PodSpec{
1535+
Priority: utilptr.To[int32](10),
1536+
},
1537+
},
1538+
{
1539+
ObjectMeta: metav1.ObjectMeta{
1540+
Name: "evictable-high-priority",
1541+
Annotations: map[string]string{"evictable": "true"},
1542+
},
1543+
Spec: v1.PodSpec{
1544+
Priority: utilptr.To[int32](15),
1545+
},
1546+
},
1547+
}},
1548+
},
1549+
},
1550+
{
1551+
name: "multiple domains with different priorities and selectors",
1552+
constraintTopology: map[topologyPair][]*v1.Pod{
1553+
{"zone", "a"}: {
1554+
{
1555+
ObjectMeta: metav1.ObjectMeta{
1556+
Name: "high-priority-affinity",
1557+
Annotations: map[string]string{"evictable": "true"},
1558+
},
1559+
Spec: v1.PodSpec{
1560+
Priority: utilptr.To[int32](20),
1561+
},
1562+
},
1563+
{
1564+
ObjectMeta: metav1.ObjectMeta{
1565+
Name: "low-priority-no-affinity",
1566+
Annotations: map[string]string{"evictable": "true"},
1567+
},
1568+
Spec: v1.PodSpec{
1569+
Priority: utilptr.To[int32](5),
1570+
},
1571+
},
1572+
},
1573+
{"zone", "b"}: {
1574+
{
1575+
ObjectMeta: metav1.ObjectMeta{
1576+
Name: "medium-priority-affinity",
1577+
Annotations: map[string]string{"evictable": "true"},
1578+
},
1579+
Spec: v1.PodSpec{
1580+
Priority: utilptr.To[int32](15),
1581+
},
1582+
},
1583+
{
1584+
ObjectMeta: metav1.ObjectMeta{
1585+
Name: "non-evictable-pod",
1586+
Annotations: map[string]string{"evictable": "false"},
1587+
},
1588+
},
1589+
},
1590+
},
1591+
want: []topology{
1592+
{pair: topologyPair{"zone", "a"}, pods: []*v1.Pod{
1593+
{
1594+
ObjectMeta: metav1.ObjectMeta{
1595+
Name: "low-priority-no-affinity",
1596+
Annotations: map[string]string{"evictable": "true"},
1597+
},
1598+
Spec: v1.PodSpec{
1599+
Priority: utilptr.To[int32](5),
1600+
},
1601+
},
1602+
{
1603+
ObjectMeta: metav1.ObjectMeta{
1604+
Name: "high-priority-affinity",
1605+
Annotations: map[string]string{"evictable": "true"},
1606+
},
1607+
Spec: v1.PodSpec{
1608+
Priority: utilptr.To[int32](20),
1609+
},
1610+
},
1611+
}},
1612+
{pair: topologyPair{"zone", "b"}, pods: []*v1.Pod{
1613+
{
1614+
ObjectMeta: metav1.ObjectMeta{
1615+
Name: "non-evictable-pod",
1616+
Annotations: map[string]string{"evictable": "false"},
1617+
},
1618+
},
1619+
{
1620+
ObjectMeta: metav1.ObjectMeta{
1621+
Name: "medium-priority-affinity",
1622+
Annotations: map[string]string{"evictable": "true"},
1623+
},
1624+
Spec: v1.PodSpec{
1625+
Priority: utilptr.To[int32](15),
1626+
},
1627+
},
1628+
}},
1629+
},
1630+
},
1631+
{
1632+
name: "domain with pods having different selector/affinity",
1633+
constraintTopology: map[topologyPair][]*v1.Pod{
1634+
{"zone", "a"}: {
1635+
{
1636+
ObjectMeta: metav1.ObjectMeta{
1637+
Name: "pod-with-affinity",
1638+
Annotations: map[string]string{"evictable": "true"},
1639+
},
1640+
Spec: v1.PodSpec{
1641+
Affinity: &v1.Affinity{
1642+
NodeAffinity: &v1.NodeAffinity{},
1643+
},
1644+
Priority: utilptr.To[int32](10),
1645+
},
1646+
},
1647+
{
1648+
ObjectMeta: metav1.ObjectMeta{
1649+
Name: "pod-no-affinity",
1650+
Annotations: map[string]string{"evictable": "true"},
1651+
},
1652+
Spec: v1.PodSpec{
1653+
Priority: utilptr.To[int32](15),
1654+
},
1655+
},
1656+
},
1657+
},
1658+
want: []topology{
1659+
{pair: topologyPair{"zone", "a"}, pods: []*v1.Pod{
1660+
{
1661+
ObjectMeta: metav1.ObjectMeta{
1662+
Name: "pod-with-affinity",
1663+
Annotations: map[string]string{"evictable": "true"},
1664+
},
1665+
Spec: v1.PodSpec{
1666+
Affinity: &v1.Affinity{
1667+
NodeAffinity: &v1.NodeAffinity{},
1668+
},
1669+
Priority: utilptr.To[int32](10),
1670+
},
1671+
},
1672+
{
1673+
ObjectMeta: metav1.ObjectMeta{
1674+
Name: "pod-no-affinity",
1675+
Annotations: map[string]string{"evictable": "true"},
1676+
},
1677+
Spec: v1.PodSpec{
1678+
Priority: utilptr.To[int32](15),
1679+
},
1680+
},
1681+
}},
1682+
},
1683+
},
1684+
}
1685+
1686+
for _, tt := range tests {
1687+
t.Run(tt.name, func(t *testing.T) {
1688+
mockIsEvictable := func(pod *v1.Pod) bool {
1689+
if val, exists := pod.Annotations["evictable"]; exists {
1690+
return val == "true"
1691+
}
1692+
return false
1693+
}
1694+
got := sortDomains(tt.constraintTopology, mockIsEvictable)
1695+
sort.Slice(got, func(i, j int) bool {
1696+
return got[i].pair.value < got[j].pair.value
1697+
})
1698+
if !reflect.DeepEqual(got, tt.want) {
1699+
t.Errorf("sortDomains() = %v, want %v", got, tt.want)
1700+
}
1701+
})
1702+
}
1703+
}
1704+
14791705
type testPodList struct {
14801706
count int
14811707
node string

0 commit comments

Comments
 (0)