Skip to content

Commit be4c20e

Browse files
googs1025googs1025
committed
fix(removepodsviolatingtopologyspreadconstraint): fix removepodsviolatingtopologyspreadconstraint plugin sort logic
Signed-off-by: googs1025 <[email protected]>
1 parent 2c75a90 commit be4c20e

File tree

2 files changed

+233
-9
lines changed

2 files changed

+233
-9
lines changed

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

+7-9
Original file line numberDiff line numberDiff line change
@@ -418,22 +418,20 @@ func sortDomains(constraintTopologyPairs map[topologyPair][]*v1.Pod, isEvictable
418418
sort.Slice(list, func(i, j int) bool {
419419
evictableI := isEvictable(list[i])
420420
evictableJ := isEvictable(list[j])
421-
// any non-evictable pods should be considered last (ie, first in the list)
422-
// i is non-evictable and should come before j
423-
if !evictableI && evictableJ {
424-
return true
425-
}
426-
// j is non-evictable and should come before i
427-
if evictableI && !evictableJ {
428-
return false
421+
422+
if !evictableI || !evictableJ {
423+
// false - i is the only non-evictable, so return true to put it first
424+
// true - j is non-evictable, so return false to put j before i
425+
// if true and both and non-evictable, order doesn't matter
426+
return !(evictableI && !evictableJ)
429427
}
430428

431429
hasSelectorOrAffinityI := hasSelectorOrAffinity(*list[i])
432430
hasSelectorOrAffinityJ := hasSelectorOrAffinity(*list[j])
433431
// if both pods have selectors/affinity, compare them by their priority
434432
if hasSelectorOrAffinityI == hasSelectorOrAffinityJ {
435433
// Sort by priority in ascending order (lower priority Pods first)
436-
comparePodsByPriority(list[i], list[j])
434+
return !comparePodsByPriority(list[i], list[j])
437435
}
438436
return hasSelectorOrAffinityI && !hasSelectorOrAffinityJ
439437
})

‎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

@@ -1514,6 +1515,231 @@ func createTestPods(testPods []testPodList) []*v1.Pod {
15141515
return pods
15151516
}
15161517

1518+
func TestSortDomains(t *testing.T) {
1519+
tests := []struct {
1520+
name string
1521+
constraintTopology map[topologyPair][]*v1.Pod
1522+
want []topology
1523+
}{
1524+
{
1525+
name: "empty input",
1526+
constraintTopology: map[topologyPair][]*v1.Pod{},
1527+
want: []topology{},
1528+
},
1529+
{
1530+
name: "single domain with mixed pods",
1531+
constraintTopology: map[topologyPair][]*v1.Pod{
1532+
{"zone", "a"}: {
1533+
{
1534+
ObjectMeta: metav1.ObjectMeta{
1535+
Name: "non-evictable-pod",
1536+
Annotations: map[string]string{"evictable": "false"},
1537+
},
1538+
},
1539+
{
1540+
ObjectMeta: metav1.ObjectMeta{
1541+
Name: "evictable-with-affinity",
1542+
Annotations: map[string]string{"evictable": "true", "hasSelectorOrAffinity": "true"},
1543+
},
1544+
Spec: v1.PodSpec{
1545+
Priority: utilptr.To[int32](10),
1546+
},
1547+
},
1548+
{
1549+
ObjectMeta: metav1.ObjectMeta{
1550+
Name: "evictable-high-priority",
1551+
Annotations: map[string]string{"evictable": "true"},
1552+
},
1553+
Spec: v1.PodSpec{
1554+
Priority: utilptr.To[int32](15),
1555+
},
1556+
},
1557+
},
1558+
},
1559+
want: []topology{
1560+
{pair: topologyPair{"zone", "a"}, pods: []*v1.Pod{
1561+
{
1562+
ObjectMeta: metav1.ObjectMeta{
1563+
Name: "non-evictable-pod",
1564+
Annotations: map[string]string{"evictable": "false"},
1565+
},
1566+
},
1567+
{
1568+
ObjectMeta: metav1.ObjectMeta{
1569+
Name: "evictable-with-affinity",
1570+
Annotations: map[string]string{"evictable": "true", "hasSelectorOrAffinity": "true"},
1571+
},
1572+
Spec: v1.PodSpec{
1573+
Priority: utilptr.To[int32](10),
1574+
},
1575+
},
1576+
{
1577+
ObjectMeta: metav1.ObjectMeta{
1578+
Name: "evictable-high-priority",
1579+
Annotations: map[string]string{"evictable": "true"},
1580+
},
1581+
Spec: v1.PodSpec{
1582+
Priority: utilptr.To[int32](15),
1583+
},
1584+
},
1585+
}},
1586+
},
1587+
},
1588+
{
1589+
name: "multiple domains with different priorities and selectors",
1590+
constraintTopology: map[topologyPair][]*v1.Pod{
1591+
{"zone", "a"}: {
1592+
{
1593+
ObjectMeta: metav1.ObjectMeta{
1594+
Name: "high-priority-affinity",
1595+
Annotations: map[string]string{"evictable": "true"},
1596+
},
1597+
Spec: v1.PodSpec{
1598+
Priority: utilptr.To[int32](20),
1599+
},
1600+
},
1601+
{
1602+
ObjectMeta: metav1.ObjectMeta{
1603+
Name: "low-priority-no-affinity",
1604+
Annotations: map[string]string{"evictable": "true"},
1605+
},
1606+
Spec: v1.PodSpec{
1607+
Priority: utilptr.To[int32](5),
1608+
},
1609+
},
1610+
},
1611+
{"zone", "b"}: {
1612+
{
1613+
ObjectMeta: metav1.ObjectMeta{
1614+
Name: "medium-priority-affinity",
1615+
Annotations: map[string]string{"evictable": "true"},
1616+
},
1617+
Spec: v1.PodSpec{
1618+
Priority: utilptr.To[int32](15),
1619+
},
1620+
},
1621+
{
1622+
ObjectMeta: metav1.ObjectMeta{
1623+
Name: "non-evictable-pod",
1624+
Annotations: map[string]string{"evictable": "false"},
1625+
},
1626+
},
1627+
},
1628+
},
1629+
want: []topology{
1630+
{pair: topologyPair{"zone", "a"}, pods: []*v1.Pod{
1631+
{
1632+
ObjectMeta: metav1.ObjectMeta{
1633+
Name: "low-priority-no-affinity",
1634+
Annotations: map[string]string{"evictable": "true"},
1635+
},
1636+
Spec: v1.PodSpec{
1637+
Priority: utilptr.To[int32](5),
1638+
},
1639+
},
1640+
{
1641+
ObjectMeta: metav1.ObjectMeta{
1642+
Name: "high-priority-affinity",
1643+
Annotations: map[string]string{"evictable": "true"},
1644+
},
1645+
Spec: v1.PodSpec{
1646+
Priority: utilptr.To[int32](20),
1647+
},
1648+
},
1649+
}},
1650+
{pair: topologyPair{"zone", "b"}, pods: []*v1.Pod{
1651+
{
1652+
ObjectMeta: metav1.ObjectMeta{
1653+
Name: "non-evictable-pod",
1654+
Annotations: map[string]string{"evictable": "false"},
1655+
},
1656+
},
1657+
{
1658+
ObjectMeta: metav1.ObjectMeta{
1659+
Name: "medium-priority-affinity",
1660+
Annotations: map[string]string{"evictable": "true"},
1661+
},
1662+
Spec: v1.PodSpec{
1663+
Priority: utilptr.To[int32](15),
1664+
},
1665+
},
1666+
}},
1667+
},
1668+
},
1669+
{
1670+
name: "domain with pods having different selector/affinity",
1671+
constraintTopology: map[topologyPair][]*v1.Pod{
1672+
{"zone", "a"}: {
1673+
{
1674+
ObjectMeta: metav1.ObjectMeta{
1675+
Name: "pod-with-affinity",
1676+
Annotations: map[string]string{"evictable": "true"},
1677+
},
1678+
Spec: v1.PodSpec{
1679+
Affinity: &v1.Affinity{
1680+
NodeAffinity: &v1.NodeAffinity{},
1681+
},
1682+
Priority: utilptr.To[int32](10),
1683+
},
1684+
},
1685+
{
1686+
ObjectMeta: metav1.ObjectMeta{
1687+
Name: "pod-no-affinity",
1688+
Annotations: map[string]string{"evictable": "true"},
1689+
},
1690+
Spec: v1.PodSpec{
1691+
Priority: utilptr.To[int32](15),
1692+
},
1693+
},
1694+
},
1695+
},
1696+
want: []topology{
1697+
{pair: topologyPair{"zone", "a"}, pods: []*v1.Pod{
1698+
{
1699+
ObjectMeta: metav1.ObjectMeta{
1700+
Name: "pod-with-affinity",
1701+
Annotations: map[string]string{"evictable": "true"},
1702+
},
1703+
Spec: v1.PodSpec{
1704+
Affinity: &v1.Affinity{
1705+
NodeAffinity: &v1.NodeAffinity{},
1706+
},
1707+
Priority: utilptr.To[int32](10),
1708+
},
1709+
},
1710+
{
1711+
ObjectMeta: metav1.ObjectMeta{
1712+
Name: "pod-no-affinity",
1713+
Annotations: map[string]string{"evictable": "true"},
1714+
},
1715+
Spec: v1.PodSpec{
1716+
Priority: utilptr.To[int32](15),
1717+
},
1718+
},
1719+
}},
1720+
},
1721+
},
1722+
}
1723+
1724+
for _, tt := range tests {
1725+
t.Run(tt.name, func(t *testing.T) {
1726+
mockIsEvictable := func(pod *v1.Pod) bool {
1727+
if val, exists := pod.Annotations["evictable"]; exists {
1728+
return val == "true"
1729+
}
1730+
return false
1731+
}
1732+
got := sortDomains(tt.constraintTopology, mockIsEvictable)
1733+
sort.Slice(got, func(i, j int) bool {
1734+
return got[i].pair.value < got[j].pair.value
1735+
})
1736+
if !reflect.DeepEqual(got, tt.want) {
1737+
t.Errorf("sortDomains() = %v, want %v", got, tt.want)
1738+
}
1739+
})
1740+
}
1741+
}
1742+
15171743
func getLabelSelector(key string, values []string, operator metav1.LabelSelectorOperator) *metav1.LabelSelector {
15181744
return &metav1.LabelSelector{
15191745
MatchExpressions: []metav1.LabelSelectorRequirement{{Key: key, Operator: operator, Values: values}},

0 commit comments

Comments
 (0)