add podGroup backoff time for coscheduling #559

KunWuLuan · 2023-03-29T02:07:15Z

Adding a backoff in co-scheduling (#429 (comment)) helps to avoid infinite scheduling loop.

Add a new coscheduling plugin argument `podGroupBackoffSeconds` to configure backoff timer for failed PodGroup. User needs to explicitly specify a positive integer to enable this feature.

k8s-ci-robot · 2023-03-29T02:07:25Z

Hi @KunWuLuan. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

denkensk · 2023-04-03T13:22:58Z

/ok-to-test

KunWuLuan · 2023-04-07T09:54:54Z

/test all

Huang-Wei · 2023-06-27T23:28:22Z

test/integration/noderesourcetopology_cache_test.go

@@ -485,7 +485,7 @@ func TestTopologyCachePluginWithoutUpdates(t *testing.T) {
 					checkPod = podIsPending
 				}

-				err = checkPod(1*time.Second, 20, cs, p.pod.Namespace, p.pod.Name)


I'd expect to see a new (sub-)integration test to simulate what was described in the original issue.

Otherwise, I'd postpone the impl. to next release.

Have added a new integration test for the case

Thanks. But even without introducing the backoff, the integration test seems to be also passing.

weih@m1max:~/go/src/sigs.k8s.io/scheduler-plugins|master⚡ ⇒ go test ./test/integration/... -run TestPodgroupBackoff -count 1 ok sigs.k8s.io/scheduler-plugins/test/integration 27.100s weih@m1max:~/go/src/sigs.k8s.io/scheduler-plugins|master⚡ ⇒ gst On branch master Your branch is ahead of 'origin/master' by 2 commits. (use "git push" to publish your local commits) Changes not staged for commit: (use "git add <file>..." to update what will be committed) (use "git restore <file>..." to discard changes in working directory) modified: test/integration/coscheduling_test.go

Could you update the test to pass and only pass with the backoff changes? in other words, the test is expected to fail on master.

pkg/coscheduling/core/core.go

Huang-Wei · 2023-06-30T22:47:21Z

pkg/coscheduling/coscheduling.go

+	pods, err := cs.frameworkHandler.SharedInformerFactory().Core().V1().Pods().Lister().Pods(pod.Namespace).List(
+		labels.SelectorFromSet(labels.Set{v1alpha1.PodGroupLabel: util.GetPodGroupLabel(pod)}),
+	)
+	if err == nil && len(pods) >= int(pg.Spec.MinMember) {


can we remove the check here? (given assigned < pg.Spec.MinMember per L150)

I think we can not remove the check here.

assigned < pg.Spec.MinMember check whether the gang has been satisfied and len(pods) >= int(pg.Spec.MinMember) check whether there is enough pods to schedule.

When the first pod come, scheduler may list not all pods. We will reject the first pod in preFilter

scheduler-plugins/pkg/coscheduling/core/core.go

Lines 153 to 156 in 6ef6f63

if len(pods) < int(pg.Spec.MinMember) {

return fmt.Errorf("pre-filter pod %v cannot find enough sibling pods, "+

"current pods number: %v, minMember of group: %v", pod.Name, len(pods), pg.Spec.MinMember)

}

But we should not add this pod group to Backoff. If we add this pod group to Backoff, all following pods will be rejected.

apis/config/v1/types.go

Huang-Wei · 2023-07-03T18:58:41Z

pkg/coscheduling/coscheduling.go

@@ -71,6 +72,7 @@ func New(obj runtime.Object, handle framework.Handle) (framework.Plugin, error)
 	podInformer := handle.SharedInformerFactory().Core().V1().Pods()

 	scheduleTimeDuration := time.Duration(args.PermitWaitingTimeSeconds) * time.Second
+	pgBackoff := time.Duration(args.PodGroupBackoffTime) * time.Second


Can you check if it's 0/nil here, and if yes, don't instantiate pgBackoff. So that we can check if pgBackoff is nil at L172 and only proceed get pods when it's not nil:

var pgBackoff ... if args.PodGroupBackoffTime != 0 { ... }

Huang-Wei · 2023-07-03T19:00:22Z

pkg/coscheduling/core/core.go

-func (pgMgr *PodGroupManager) AddToPodGroupBackoff(pgName string) {
-	pgMgr.podGroupBackoff.Add(pgName, nil, 10*time.Second)
+func (pgMgr *PodGroupManager) BackoffPodGroup(pgName string, backoff time.Duration) {
+	if backoff == time.Duration(0) {


commented elsewhere: IMO we should return as early as possible.

we can check whether cs.pgBackoff != nil outside, It's already done

Huang-Wei · 2023-07-03T19:00:56Z

pkg/coscheduling/coscheduling.go

+	pods, err := cs.frameworkHandler.SharedInformerFactory().Core().V1().Pods().Lister().Pods(pod.Namespace).List(
+		labels.SelectorFromSet(labels.Set{v1alpha1.PodGroupLabel: util.GetPodGroupLabel(pod)}),
+	)
+	if err == nil && len(pods) >= int(pg.Spec.MinMember) {


apis/config/scheme/scheme_test.go

Huang-Wei · 2023-07-03T19:03:13Z

apis/config/scheme/scheme_test.go

@@ -1022,6 +1024,7 @@ profiles:
      apiVersion: kubescheduler.config.k8s.io/v1
      kind: CoschedulingArgs
      permitWaitingTimeSeconds: 10
+      podGroupBackoffTime: 0


I'd expect without explicitly specifying it, UT would also pass. Could you check if you want to add explicit defaulting logic? (convert nil from versioned args to 0 in internal args)

podGroupBackoffTime: 0 means we will not backoff pod groups.
I think maybe we don't have to add explicit defaulting logic, because by default nil will be convert to 0

That's correct. The previous comment was more about "how it behaves without explicitly setting the argument", and hence I'd like to see this UT passes w/o specifying it to 0.

Ok, I added a test for the case

sry for confusing, I misread - this is the test for encode.

could you apply this diff - we want to test both defaulting logic (in decode), and encode for non-default logic: https://gist.github.com/Huang-Wei/d86f188b4e85b48ad4e3e4aa4bd427da

pkg/coscheduling/coscheduling.go

Huang-Wei · 2023-07-06T00:50:43Z

One comment ^^ and #559 (comment)

Huang-Wei

Final comments. (you can resolve them along with squashing into two commits: one for core changes, the other for code generation). Thanks.

apis/config/v1/types.go

Huang-Wei · 2023-07-11T20:00:06Z

apis/config/scheme/scheme_test.go

@@ -1022,6 +1024,7 @@ profiles:
      apiVersion: kubescheduler.config.k8s.io/v1
      kind: CoschedulingArgs
      permitWaitingTimeSeconds: 10
+      podGroupBackoffTime: 0


sry for confusing, I misread - this is the test for encode.

could you apply this diff - we want to test both defaulting logic (in decode), and encode for non-default logic: https://gist.github.com/Huang-Wei/d86f188b4e85b48ad4e3e4aa4bd427da

KunWuLuan · 2023-07-12T01:44:30Z

No problem, but in this diff, we set default backoff time to 60, not to 0, if there is not any args in profile, is this same with our expectation?

Signed-off-by: KunWuLuan <kunwuluan@gmail.com>

Huang-Wei · 2023-07-25T17:15:38Z

No problem, but in this diff, we set default backoff time to 60, not to 0, if there is not any args in profile, is this same with our expectation?

That was "PermitWaitingTimeSeconds" :) not backoff time. PermitWaitingTimeSeconds defaults to 60 as-is; Backoff time defaults to 0. The logic is accurate.

Huang-Wei · 2023-07-25T17:20:02Z

Added release note.

/lgtm
/approve

k8s-ci-robot · 2023-07-25T17:20:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Huang-Wei, KunWuLuan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Huang-Wei]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from swatisehgal and zwpaper March 29, 2023 02:07

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 29, 2023

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 3, 2023

k8s-ci-robot added the do-not-merge/contains-merge-commits label May 5, 2023

KunWuLuan force-pushed the master branch from 8612ae5 to 92e4998 Compare May 5, 2023 12:27

k8s-ci-robot removed the do-not-merge/contains-merge-commits label May 5, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 23, 2023

Huang-Wei reviewed Jun 27, 2023

View reviewed changes

KunWuLuan force-pushed the master branch from 4d543c6 to 72ed834 Compare June 28, 2023 12:08

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 28, 2023

KunWuLuan force-pushed the master branch 2 times, most recently from d721c00 to 3588a53 Compare June 29, 2023 02:12

Huang-Wei reviewed Jun 30, 2023

View reviewed changes

Huang-Wei reviewed Jul 3, 2023

View reviewed changes

apis/config/scheme/scheme_test.go Show resolved Hide resolved

Huang-Wei reviewed Jul 3, 2023

View reviewed changes

Huang-Wei reviewed Jul 6, 2023

View reviewed changes

pkg/coscheduling/coscheduling.go Outdated Show resolved Hide resolved

Huang-Wei reviewed Jul 11, 2023

View reviewed changes

KunWuLuan force-pushed the master branch from 642cfe7 to a81aeec Compare July 12, 2023 01:58

KunWuLuan added 2 commits July 12, 2023 10:21

add podGroup backoff time for coscheduling

dbad1a1

update generated code

a0bf090

Signed-off-by: KunWuLuan <kunwuluan@gmail.com>

KunWuLuan force-pushed the master branch from a81aeec to a0bf090 Compare July 12, 2023 02:21

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jul 25, 2023

k8s-ci-robot assigned Huang-Wei Jul 25, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 25, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 25, 2023

Huang-Wei mentioned this pull request Jul 25, 2023

coscheduling queue sort plugin starves pods #110

Closed

k8s-ci-robot merged commit fb9bf41 into kubernetes-sigs:master Jul 25, 2023

KunWuLuan mentioned this pull request Sep 12, 2023

[coscheduling] Infinite scheduling loop of distributed tasks block all other pods #429

Closed

Huang-Wei mentioned this pull request Dec 7, 2023

Group of unschedulable pods causes scheduler (coscheduling) to be blocked #682

Closed

4 tasks

KunWuLuan mentioned this pull request Oct 16, 2024

REQUEST: New membership for kunwuluan kubernetes/org#5217

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add podGroup backoff time for coscheduling #559

add podGroup backoff time for coscheduling #559

KunWuLuan commented Mar 29, 2023 •

edited by Huang-Wei

Loading

k8s-ci-robot commented Mar 29, 2023

denkensk commented Apr 3, 2023

KunWuLuan commented Apr 7, 2023

Huang-Wei Jun 27, 2023

KunWuLuan Jun 29, 2023

Huang-Wei Jun 29, 2023

KunWuLuan Jun 29, 2023

KunWuLuan Jun 30, 2023

Huang-Wei Jun 30, 2023

Huang-Wei Jul 3, 2023

KunWuLuan Jul 4, 2023

Huang-Wei Jul 3, 2023

Huang-Wei Jul 3, 2023

KunWuLuan Jul 4, 2023

Huang-Wei Jul 3, 2023

Huang-Wei Jul 3, 2023

KunWuLuan Jul 4, 2023

Huang-Wei Jul 6, 2023

KunWuLuan Jul 10, 2023

Huang-Wei Jul 11, 2023

Huang-Wei commented Jul 6, 2023

Huang-Wei left a comment •

edited

Loading

Huang-Wei Jul 11, 2023

KunWuLuan commented Jul 12, 2023

Huang-Wei commented Jul 25, 2023

Huang-Wei commented Jul 25, 2023

k8s-ci-robot commented Jul 25, 2023

	if len(pods) < int(pg.Spec.MinMember) {
	return fmt.Errorf("pre-filter pod %v cannot find enough sibling pods, "+
	"current pods number: %v, minMember of group: %v", pod.Name, len(pods), pg.Spec.MinMember)
	}

add podGroup backoff time for coscheduling #559

add podGroup backoff time for coscheduling #559

Conversation

KunWuLuan commented Mar 29, 2023 • edited by Huang-Wei Loading

k8s-ci-robot commented Mar 29, 2023

denkensk commented Apr 3, 2023

KunWuLuan commented Apr 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Huang-Wei commented Jul 6, 2023

Huang-Wei left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KunWuLuan commented Jul 12, 2023

Huang-Wei commented Jul 25, 2023

Huang-Wei commented Jul 25, 2023

k8s-ci-robot commented Jul 25, 2023

KunWuLuan commented Mar 29, 2023 •

edited by Huang-Wei

Loading

Huang-Wei left a comment •

edited

Loading