scheduler: fix TestIncomingPodsMetrics unit test #120434

pohly · 2023-09-05T18:55:55Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

addUnschedulablePodBackToBackoffQ happened to put the pod into the backoff queue because

the pod was not popped earlier and thus not in flight
the PodInfo had UnschedulablePlugins set
determineSchedulingHintForInFlightPod has code for "if UnschedulablePlugins is set and pod not in flight -> internal error, use backoff"

Relying on such special code is not good. A better way to force backoff is by recording some concurrent event. isPodWorthRequeuing then calls the queueHintReturnQueueAfterBackoff function and the pod goes to the backoff queue.

Which issue(s) this PR fixes:

Related-to: #120413 (comment)

Does this PR introduce a user-facing change?

NONE

k8s-ci-robot · 2023-09-05T18:56:04Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

alculquicondor · 2023-09-05T19:14:22Z

/cc

pohly · 2023-09-06T09:09:49Z

/retest

alculquicondor · 2023-09-08T15:12:20Z

pkg/scheduler/internal/queue/scheduling_queue_test.go

 	addUnschedulablePodBackToUnschedulablePods = func(logger klog.Logger, queue *PriorityQueue, pInfo *framework.QueuedPodInfo) {
 		// To simulate the pod is failed in scheduling in the real world, Pop() the pod from activeQ before AddUnschedulableIfNotPresent() below.
-		queue.activeQ.Add(queue.newQueuedPodInfo(pInfo.Pod))
-		if p, err := queue.Pop(); err != nil || p.Pod != pInfo.Pod {
+		expectNoError("Add", queue.activeQ.Add(queue.newQueuedPodInfo(pInfo.Pod)))


https://github.com/kubernetes/community/blob/master/sig-scheduling/CONTRIBUTING.md#technical-and-style-guidelines

Use t.Fatal

I would have preferred that myself. But t is not passed down into these operations. I could change that, but then the diff becomes larger.

Or did you mean not having a expectNoError function (DAMP)?

I find expectNoError more readable than an if check. But if that's just me, then I can change to if ... t.Fatal.

Both done (using t.Fatal and removal of expectNoError).

Thanks! More importantly I wanted you to use t.Fatal.
But I also don't like adding "assert" libraries.

alculquicondor · 2023-09-08T15:16:35Z

pkg/scheduler/internal/queue/scheduling_queue_test.go

+		expectNoError("Add", queue.activeQ.Add(queue.newQueuedPodInfo(pInfo.Pod)))
+		p, err := queue.Pop()
+		expectNoError("Pop", err)
+		if p.Pod != pInfo.Pod {
 			panic(fmt.Sprintf("Expected: %v after Pop, but got: %v", pInfo.Pod.Name, p.Pod.Name))


uhm... this one slipped, please use t.Fatal

alculquicondor · 2023-09-08T15:25:42Z

pkg/scheduler/internal/queue/scheduling_queue_test.go

+		if err != nil {
+			panic(fmt.Sprintf("Unexpected error from %s: %v", what, err))
+		}
+	}
 	addUnschedulablePodBackToUnschedulablePods = func(logger klog.Logger, queue *PriorityQueue, pInfo *framework.QueuedPodInfo) {


let's change the function name to: popAndRequeueAsUnschedulable. Similarly for the other one

alculquicondor · 2023-09-08T15:27:37Z

pkg/scheduler/internal/queue/scheduling_queue_test.go

+		}
+
+		// A concurrent event forces the pod into the backoff queue instead of the unschedulable queue.
+		queue.MoveAllToActiveOrBackoffQueue(logger, NodeAdd, nil, nil, nil)


I don't like the fact that, to understand what each test case is doing, I have to go back and read this function.
I would prefer if we would just have:

operations: []{pop, moveAll, addUnschedulable, ...}

But up to you if you want to do it or leave it for someone else to follow up.

Someone else, please 😓

pohly · 2023-09-11T14:04:06Z

/retest

alculquicondor

/lgtm
/approve

k8s-ci-robot · 2023-09-11T15:22:49Z

LGTM label has been added.

Git tree hash: 7ad32faa35c49b86862d918ca068b3f5f9b2c9c1

pacoxu · 2023-09-12T01:57:31Z

/test pull-kubernetes-node-e2e-containerd

pohly · 2023-09-12T05:56:15Z

The tests pass in their branch, but not when merged into master. That's because of 0d3eafd: together with that, some pods in the unit tests no longer go to the expected queues:

   --- FAIL: TestIncomingPodsMetrics/add_pods_to_unschedulablePods_and_then_move_all_to_activeQ (0.10s)
        scheduling_queue.go:617: I0912 07:53:19.010475] Checking events for in-flight pod pod="ns1/test-pod-1" unschedulablePlugins={} inFlightEventsSize=1 inFlightPodsSize=1
        scheduling_queue.go:752: I0912 07:53:19.010547] Pod moved to an internal scheduling queue pod="ns1/test-pod-1" event="ScheduleAttemptFailure" queue="Backoff" schedulingCycle=1 hint="QueueAfterBackoff"
        scheduling_queue.go:617: I0912 07:53:19.010586] Checking events for in-flight pod pod="ns2/test-pod-2" unschedulablePlugins={} inFlightEventsSize=1 inFlightPodsSize=1
        scheduling_queue.go:752: I0912 07:53:19.010620] Pod moved to an internal scheduling queue pod="ns2/test-pod-2" event="ScheduleAttemptFailure" queue="Backoff" schedulingCycle=1 hint="QueueAfterBackoff"
        scheduling_queue.go:617: I0912 07:53:19.010652] Checking events for in-flight pod pod="ns3/test-pod-3" unschedulablePlugins={} inFlightEventsSize=1 inFlightPodsSize=1
        scheduling_queue.go:752: I0912 07:53:19.010681] Pod moved to an internal scheduling queue pod="ns3/test-pod-3" event="ScheduleAttemptFailure" queue="Backoff" schedulingCycle=1 hint="QueueAfterBackoff"
        scheduling_queue_test.go:2939: unexpected collecting result:
            
            
            Diff:
            --- metric output does not match expectation; want
            +++ got:
            @@ -2,4 +2,3 @@
             # TYPE scheduler_queue_incoming_pods_total counter
            -scheduler_queue_incoming_pods_total{event="ScheduleAttemptFailure",queue="unschedulable"} 3
            -scheduler_queue_incoming_pods_total{event="UnschedulableTimeout",queue="active"} 3
            +scheduler_queue_incoming_pods_total{event="ScheduleAttemptFailure",queue="backoff"} 3

I'll rebase and check whether the unit tests made some invalid assumptions about how the queue works.

addUnschedulablePodBackToBackoffQ happened to put the pod into the backoff queue because - the pod was not popped earlier and thus not in flight - the PodInfo had UnschedulablePlugins set - determineSchedulingHintForInFlightPod has code for "if UnschedulablePlugins is set and pod not in flight -> internal error, use backoff" Relying on such special code is not good. A better way to force backoff is by recording some concurrent event. isPodWorthRequeuing then calls the queueHintReturnQueueAfterBackoff function and the pod goes to the backoff queue.

pohly · 2023-09-12T06:41:50Z

The relevant difference is that pods with no unschedulable plugins now go into the backoff queue. To simulate putting a pod into the unschedulable queue, pInfo.UnschedulablePlugins must be set. Also, a concurrent event is no longer needed when the backoff queue is the intended target:

diff --git a/pkg/scheduler/internal/queue/scheduling_queue_test.go b/pkg/scheduler/internal/queue/scheduling_queue_test.go
index 13a9a89bb02..b17e5a3c3cc 100644
--- a/pkg/scheduler/internal/queue/scheduling_queue_test.go
+++ b/pkg/scheduler/internal/queue/scheduling_queue_test.go
@@ -2288,6 +2288,8 @@ var (
        }
        popAndRequeueAsUnschedulable = func(t *testing.T, logger klog.Logger, queue *PriorityQueue, pInfo *framework.QueuedPodInfo) {
                // To simulate the pod is failed in scheduling in the real world, Pop() the pod from activeQ before AddUnschedulableIfNotPresent() below.
+               // UnschedulablePlugins will get cleared by Pop, so make a copy first.
+               unschedulablePlugins := pInfo.UnschedulablePlugins.Clone()
                if err := queue.activeQ.Add(queue.newQueuedPodInfo(pInfo.Pod)); err != nil {
                        t.Fatalf("Unexpected error during Add: %v", err)
                }
@@ -2298,6 +2300,8 @@ var (
                if p.Pod != pInfo.Pod {
                        t.Fatalf("Expected: %v after Pop, but got: %v", pInfo.Pod.Name, p.Pod.Name)
                }
+               // Simulate plugins that are waiting for some events.
+               p.UnschedulablePlugins = unschedulablePlugins
                if err := queue.AddUnschedulableIfNotPresent(logger, p, 1); err != nil {
                        t.Fatalf("Unexpected error during AddUnschedulableIfNotPresent: %v", err)
                }
@@ -2314,10 +2318,7 @@ var (
                if p.Pod != pInfo.Pod {
                        t.Fatalf("Expected: %v after Pop, but got: %v", pInfo.Pod.Name, p.Pod.Name)
                }
-
-               // A concurrent event forces the pod into the backoff queue instead of the unschedulable queue.
-               queue.MoveAllToActiveOrBackoffQueue(logger, NodeAdd, nil, nil, nil)
-
+               // When there is no known unschedulable plugin, pods always go to the backoff queue.
                if err := queue.AddUnschedulableIfNotPresent(logger, p, 1); err != nil {
                        t.Fatalf("Unexpected error during AddUnschedulableIfNotPresent: %v", err)
                }

alculquicondor · 2023-09-12T12:55:03Z

/approve cancel
I'll leave this review to @sanposhiho

sanposhiho · 2023-09-13T01:00:31Z

/assign

sanposhiho

/lgtm
/approve

k8s-ci-robot · 2023-09-18T06:30:51Z

LGTM label has been added.

Git tree hash: eeb1b2270bc6d52f11ae720e7eb784398d93b2e8

k8s-ci-robot · 2023-09-18T06:31:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pohly, sanposhiho

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:

~~pkg/scheduler/OWNERS~~ [sanposhiho]

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 · 2023-09-18T09:34:44Z

@pohly: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-e2e-kind	`819edda`	link	unknown	`/test pull-kubernetes-e2e-kind`
pull-kubernetes-e2e-gce	`819edda`	link	unknown	`/test pull-kubernetes-e2e-gce`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

pohly · 2023-09-18T09:59:16Z

/retest

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Sep 5, 2023

k8s-ci-robot requested review from chendave and denkensk September 5, 2023 18:56

k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 5, 2023

k8s-ci-robot requested a review from alculquicondor September 5, 2023 19:14

pohly force-pushed the scheduler-backoff-metric-test branch 2 times, most recently from 7e2fb1e to 7a5000e Compare September 6, 2023 07:27

alculquicondor reviewed Sep 8, 2023

View reviewed changes

pohly force-pushed the scheduler-backoff-metric-test branch from 7a5000e to 4ffc9a4 Compare September 11, 2023 11:12

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 11, 2023

alculquicondor reviewed Sep 11, 2023

View reviewed changes

k8s-ci-robot assigned alculquicondor Sep 11, 2023

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

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

pohly force-pushed the scheduler-backoff-metric-test branch from 4ffc9a4 to 819edda Compare September 12, 2023 06:39

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 12, 2023

k8s-ci-robot requested a review from alculquicondor September 12, 2023 06:39

k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 12, 2023

k8s-ci-robot assigned sanposhiho Sep 13, 2023

sanposhiho approved these changes Sep 18, 2023

View reviewed changes

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

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

k8s-ci-robot merged commit 3cfdf3c into kubernetes:master Sep 18, 2023

k8s-ci-robot added this to the v1.29 milestone Sep 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scheduler: fix TestIncomingPodsMetrics unit test #120434

scheduler: fix TestIncomingPodsMetrics unit test #120434

pohly commented Sep 5, 2023

k8s-ci-robot commented Sep 5, 2023

alculquicondor commented Sep 5, 2023

pohly commented Sep 6, 2023

alculquicondor Sep 8, 2023

pohly Sep 8, 2023

pohly Sep 11, 2023

pohly Sep 11, 2023

alculquicondor Sep 11, 2023

alculquicondor Sep 8, 2023

pohly Sep 11, 2023

alculquicondor Sep 8, 2023

pohly Sep 11, 2023

alculquicondor Sep 8, 2023

pohly Sep 11, 2023

pohly commented Sep 11, 2023

alculquicondor left a comment

k8s-ci-robot commented Sep 11, 2023

pacoxu commented Sep 12, 2023

pohly commented Sep 12, 2023

pohly commented Sep 12, 2023 •

edited

Loading

alculquicondor commented Sep 12, 2023

sanposhiho commented Sep 13, 2023

sanposhiho left a comment

k8s-ci-robot commented Sep 18, 2023

k8s-ci-robot commented Sep 18, 2023

k8s-ci-robot commented Sep 18, 2023

pohly commented Sep 18, 2023

scheduler: fix TestIncomingPodsMetrics unit test #120434

scheduler: fix TestIncomingPodsMetrics unit test #120434

Conversation

pohly commented Sep 5, 2023

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Sep 5, 2023

alculquicondor commented Sep 5, 2023

pohly commented Sep 6, 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

pohly commented Sep 11, 2023

alculquicondor left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 11, 2023

pacoxu commented Sep 12, 2023

pohly commented Sep 12, 2023

pohly commented Sep 12, 2023 • edited Loading

alculquicondor commented Sep 12, 2023

sanposhiho commented Sep 13, 2023

sanposhiho left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 18, 2023

k8s-ci-robot commented Sep 18, 2023

k8s-ci-robot commented Sep 18, 2023

pohly commented Sep 18, 2023

pohly commented Sep 12, 2023 •

edited

Loading