Skip to content

koord-descheduler: fixes namespace object limiter #2160

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
koordinator-bot merged 2 commits into from
Sep 2, 2024
Merged

koord-descheduler: fixes namespace object limiter #2160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4bf287f
fixes namespace object limiter
songtao98 Aug 6, 2024
04ed872
Merge branch 'main' into fix_namespace_object_limiter
hormes Sep 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions pkg/descheduler/apis/config/types_pluginargs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,8 @@ type MigrationObjectLimiter struct {
// MaxMigrating indicates the maximum number of migrations/evictions allowed within the window time.
// If configured as nil or 0, the maximum number will be calculated according to MaxMigratingPerWorkload.
MaxMigrating *intstr.IntOrString
// Burst indicates the limiter allows bursts of up to 'burst' to exceed within the time window.
Burst int
}

// ArbitrationArgs holds arguments used to configure the Arbitration Mechanism.
Expand Down
2 changes: 2 additions & 0 deletions pkg/descheduler/apis/config/v1alpha2/types_pluginargs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,8 @@ type MigrationObjectLimiter struct {
// MaxMigrating indicates the maximum number of migrations/evictions allowed within the window time.
// If configured as 0, the maximum number will be calculated according to MaxMigratingPerWorkload.
MaxMigrating *intstr.IntOrString `json:"maxMigrating,omitempty"`
// Burst indicates the limiter allows bursts of up to 'burst' to exceed within the time window.
Burst int `json:"burst,omitempty"`
}

// ArbitrationArgs holds arguments used to configure the Arbitration Mechanism.
Expand Down
2 changes: 2 additions & 0 deletions pkg/descheduler/apis/config/v1alpha2/zz_generated.conversion.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 11 additions & 5 deletions pkg/descheduler/controllers/migration/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -872,8 +872,13 @@ func (r *Reconciler) trackEvictedPod(pod *corev1.Pod) {
continue
}
var maxMigratingReplicas int
if expectedReplicas, err := r.controllerFinder.GetExpectedScaleForPod(pod); err == nil {
maxMigrating := objectLimiterArgs.MaxMigrating
maxMigrating := objectLimiterArgs.MaxMigrating
// namespace limiter should only accept int value. If a percent value is provided, it will be parsed as 0, thus just return.
if limiterType == deschedulerconfig.MigrationLimitObjectNamespace {
if maxMigrating != nil {
maxMigratingReplicas = maxMigrating.IntValue()
}
} else if expectedReplicas, err := r.controllerFinder.GetExpectedScaleForPod(pod); err == nil {
if maxMigrating == nil {
maxMigrating = r.args.MaxMigratingPerWorkload
}
Expand All @@ -883,12 +888,13 @@ func (r *Reconciler) trackEvictedPod(pod *corev1.Pod) {
return
}
limit := rate.Limit(maxMigratingReplicas) / rate.Limit(objectLimiterArgs.Duration.Seconds())
burst := util.GetLimiterBurst(objectLimiterArgs.Burst)

r.track(limit, limiterKey, processScope, limiterType, maxMigratingReplicas)
r.track(limit, limiterKey, processScope, limiterType, burst)
}
}

func (r *Reconciler) track(limit rate.Limit, limiterKey, processScope string, limiterType deschedulerconfig.MigrationLimitObjectType, maxMigratingReplicas int) {
func (r *Reconciler) track(limit rate.Limit, limiterKey, processScope string, limiterType deschedulerconfig.MigrationLimitObjectType, burst int) {
r.limiterLock.Lock()
defer r.limiterLock.Unlock()

Expand All @@ -899,7 +905,7 @@ func (r *Reconciler) track(limit rate.Limit, limiterKey, processScope string, li
}
limiter := limiters[limiterKey]
if limiter == nil {
limiter = rate.NewLimiter(limit, maxMigratingReplicas)
limiter = rate.NewLimiter(limit, burst)
limiters[limiterKey] = limiter
} else if limiter.Limit() != limit {
limiter.SetLimit(limit)
Expand Down
126 changes: 67 additions & 59 deletions pkg/descheduler/controllers/migration/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1635,7 +1635,7 @@ func TestRequeueJobIfObjectLimiterFailed(t *testing.T) {
}
testObjectLimiters := deschedulerconfig.ObjectLimiterMap{
deschedulerconfig.MigrationLimitObjectWorkload: {
Duration: metav1.Duration{Duration: 1 * time.Minute},
Duration: metav1.Duration{Duration: 1 * time.Second},
MaxMigrating: &intstr.IntOrString{Type: intstr.Int, IntVal: 10},
},
}
Expand All @@ -1652,7 +1652,7 @@ func TestRequeueJobIfObjectLimiterFailed(t *testing.T) {
want bool
}{
{
name: "less than default maxMigrating",
name: "less than workload limiter",
totalReplicas: 100,
objectLimiters: testObjectLimiters,
sleepDuration: 100 * time.Millisecond,
Expand All @@ -1675,13 +1675,14 @@ func TestRequeueJobIfObjectLimiterFailed(t *testing.T) {
},
},
},
evictedPodsCount: 6,
evictedPodsCount: 1,
want: false,
},
{
name: "exceeded default maxMigrating",
name: "exceeded workload limiter",
totalReplicas: 100,
objectLimiters: testObjectLimiters,
sleepDuration: 10 * time.Millisecond,
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
OwnerReferences: ownerReferences1,
Expand All @@ -1701,14 +1702,14 @@ func TestRequeueJobIfObjectLimiterFailed(t *testing.T) {
},
},
},
evictedPodsCount: 11,
evictedPodsCount: 1,
want: true,
},
{
name: "other than workload",
name: "other workload",
totalReplicas: 100,
objectLimiters: testObjectLimiters,
sleepDuration: 100 * time.Millisecond,
sleepDuration: 10 * time.Millisecond,
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
OwnerReferences: ownerReferences1,
Expand All @@ -1728,21 +1729,22 @@ func TestRequeueJobIfObjectLimiterFailed(t *testing.T) {
},
},
},
evictedPodsCount: 11,
evictedPodsCount: 1,
evictedWorkload: &otherOwnerReferences,
want: false,
},
{
name: "disable workloadObjectLimiters",
totalReplicas: 100,
sleepDuration: 10 * time.Millisecond,
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
OwnerReferences: ownerReferences1,
Name: "test-pod",
Namespace: "test-namespace",
},
},
evictedPodsCount: 11,
evictedPodsCount: 1,
objectLimiters: deschedulerconfig.ObjectLimiterMap{
deschedulerconfig.MigrationLimitObjectWorkload: deschedulerconfig.MigrationObjectLimiter{
Duration: metav1.Duration{Duration: 0},
Expand All @@ -1763,8 +1765,9 @@ func TestRequeueJobIfObjectLimiterFailed(t *testing.T) {
want: false,
},
{
name: "default limiter",
name: "default workload limiter",
totalReplicas: 100,
sleepDuration: 10 * time.Millisecond,
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
OwnerReferences: ownerReferences1,
Expand All @@ -1784,9 +1787,42 @@ func TestRequeueJobIfObjectLimiterFailed(t *testing.T) {
},
},
},
evictedPodsCount: 11,
evictedPodsCount: 1,
want: true,
},
{
name: "workload limiter with burst",
totalReplicas: 100,
sleepDuration: 10 * time.Millisecond,
objectLimiters: deschedulerconfig.ObjectLimiterMap{
deschedulerconfig.MigrationLimitObjectWorkload: deschedulerconfig.MigrationObjectLimiter{
Duration: metav1.Duration{Duration: 1 * time.Second},
MaxMigrating: &intstr.IntOrString{Type: intstr.Int, IntVal: 10},
Burst: 2,
},
},
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
OwnerReferences: ownerReferences1,
Name: "test-pod",
Namespace: "test-namespace",
},
},
job: &sev1alpha1.PodMigrationJob{
ObjectMeta: metav1.ObjectMeta{
Name: "test",
CreationTimestamp: metav1.Time{Time: time.Now()},
},
Spec: sev1alpha1.PodMigrationJobSpec{
PodRef: &corev1.ObjectReference{
Namespace: "test-namespace",
Name: "test-pod",
},
},
},
evictedPodsCount: 1,
want: false,
},
}

for _, tt := range tests {
Expand Down Expand Up @@ -1845,20 +1881,13 @@ func TestRequeueJobIfObjectLimiterFailedWithNamespace(t *testing.T) {
UID: uuid.NewUUID(),
},
}
otherOwnerReferences := metav1.OwnerReference{
APIVersion: "apps/v1",
Controller: pointer.Bool(true),
Kind: "StatefulSet",
Name: "test-2",
UID: uuid.NewUUID(),
}
testObjectLimiters := deschedulerconfig.ObjectLimiterMap{
deschedulerconfig.MigrationLimitObjectWorkload: {
Duration: metav1.Duration{Duration: 1 * time.Minute},
Duration: metav1.Duration{Duration: 1 * time.Second},
MaxMigrating: &intstr.IntOrString{Type: intstr.Int, IntVal: 10},
},
deschedulerconfig.MigrationLimitObjectNamespace: {
Duration: metav1.Duration{Duration: 1 * time.Minute},
Duration: metav1.Duration{Duration: 1 * time.Second},
MaxMigrating: &intstr.IntOrString{Type: intstr.Int, IntVal: 5},
},
}
Expand All @@ -1871,7 +1900,7 @@ func TestRequeueJobIfObjectLimiterFailedWithNamespace(t *testing.T) {
pod *corev1.Pod
job *sev1alpha1.PodMigrationJob
evictedPodsCount int
evictedWorkload *metav1.OwnerReference
evictedNamespace string
want bool
}{
{
Expand All @@ -1898,14 +1927,14 @@ func TestRequeueJobIfObjectLimiterFailedWithNamespace(t *testing.T) {
},
},
},
evictedPodsCount: 6,
evictedPodsCount: 1,
want: true,
},
{
name: "less than both",
totalReplicas: 100,
objectLimiters: testObjectLimiters,
sleepDuration: 100 * time.Millisecond,
sleepDuration: 200 * time.Millisecond,
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
OwnerReferences: ownerReferences1,
Expand All @@ -1925,21 +1954,26 @@ func TestRequeueJobIfObjectLimiterFailedWithNamespace(t *testing.T) {
},
},
},
evictedPodsCount: 4,
evictedPodsCount: 1,
want: false,
},
{
name: "disable namespaceObjectLimiters",
totalReplicas: 100,
sleepDuration: 100 * time.Millisecond,
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
OwnerReferences: ownerReferences1,
Name: "test-pod",
Namespace: "test-namespace",
},
},
evictedPodsCount: 6,
evictedPodsCount: 1,
objectLimiters: deschedulerconfig.ObjectLimiterMap{
deschedulerconfig.MigrationLimitObjectWorkload: {
Duration: metav1.Duration{Duration: 1 * time.Second},
MaxMigrating: &intstr.IntOrString{Type: intstr.Int, IntVal: 10},
},
deschedulerconfig.MigrationLimitObjectNamespace: deschedulerconfig.MigrationObjectLimiter{
Duration: metav1.Duration{Duration: 0},
},
Expand All @@ -1961,17 +1995,18 @@ func TestRequeueJobIfObjectLimiterFailedWithNamespace(t *testing.T) {
{
name: "disable namespaceObjectLimiters and exceed workload",
totalReplicas: 100,
sleepDuration: 10 * time.Millisecond,
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
OwnerReferences: ownerReferences1,
Name: "test-pod",
Namespace: "test-namespace",
},
},
evictedPodsCount: 11,
evictedPodsCount: 1,
objectLimiters: deschedulerconfig.ObjectLimiterMap{
deschedulerconfig.MigrationLimitObjectWorkload: {
Duration: metav1.Duration{Duration: 1 * time.Minute},
Duration: metav1.Duration{Duration: 1 * time.Second},
MaxMigrating: &intstr.IntOrString{Type: intstr.Int, IntVal: 10},
},
deschedulerconfig.MigrationLimitObjectNamespace: deschedulerconfig.MigrationObjectLimiter{
Expand All @@ -1993,7 +2028,7 @@ func TestRequeueJobIfObjectLimiterFailedWithNamespace(t *testing.T) {
want: true,
},
{
name: "other than workload",
name: "other namespace",
totalReplicas: 100,
objectLimiters: testObjectLimiters,
sleepDuration: 100 * time.Millisecond,
Expand All @@ -2016,33 +2051,8 @@ func TestRequeueJobIfObjectLimiterFailedWithNamespace(t *testing.T) {
},
},
},
evictedPodsCount: 11,
evictedWorkload: &otherOwnerReferences,
want: true,
},
{
name: "default limiter",
totalReplicas: 100,
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
OwnerReferences: ownerReferences1,
Name: "test-pod",
Namespace: "test-namespace",
},
},
job: &sev1alpha1.PodMigrationJob{
ObjectMeta: metav1.ObjectMeta{
Name: "test",
CreationTimestamp: metav1.Time{Time: time.Now()},
},
Spec: sev1alpha1.PodMigrationJobSpec{
PodRef: &corev1.ObjectReference{
Namespace: "test-namespace",
Name: "test-pod",
},
},
},
evictedPodsCount: 6,
evictedPodsCount: 1,
evictedNamespace: "test-namespace-other",
want: false,
},
}
Expand Down Expand Up @@ -2076,10 +2086,8 @@ func TestRequeueJobIfObjectLimiterFailedWithNamespace(t *testing.T) {
if tt.evictedPodsCount > 0 {
for i := 0; i < tt.evictedPodsCount; i++ {
pod := tt.pod.DeepCopy()
if tt.evictedWorkload != nil {
pod.OwnerReferences = []metav1.OwnerReference{
*tt.evictedWorkload,
}
if tt.evictedNamespace != "" {
pod.Namespace = tt.evictedNamespace
}
reconciler.trackEvictedPod(pod)
if tt.sleepDuration > 0 {
Expand Down
7 changes: 7 additions & 0 deletions pkg/descheduler/controllers/migration/util/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,13 @@ func GetMaxMigrating(replicas int, intOrPercent *intstr.IntOrString) (int, error
return GetMaxUnavailable(replicas, intOrPercent)
}

func GetLimiterBurst(burst int) int {
if burst == 0 {
return 1
}
return burst
}

// FilterPodWithMaxEvictionCost rejects if pod's eviction cost is math.MaxInt32
func FilterPodWithMaxEvictionCost(pod *corev1.Pod) bool {
cost, _ := extension.GetEvictionCost(pod.Annotations)
Expand Down