fix: when PreFilter returns UnschedulableAndUnresolvable, copy the state in all nodes in statusmap #119778

sanposhiho · 2023-08-06T01:58:28Z

What type of PR is this?

/kind bug
/triage accepted
/priority important-soon

What this PR does / why we need it:

When PreFilter returns UnschedulableAndUnresolvable, we don't need to run the preemption, but we run.
This unexpected preemption could cause errors from the preemption by trying to read non-exist PreFilter data.

As @Huang-Wei pointed out, we didn't run the preemption in such cases, but #110894 introduced this bug.

#119777 adds integ test for this scenario and #119780 proves this patch fixing the bug.

Which issue(s) this PR fixes:

Fixes: #119782

Special notes for your reviewer:

We should cherry-pick this to v1.26 and v1.27.

Does this PR introduce a user-facing change?

Fixed a 1.26 regression scheduling bug by ensuring that preemption is skipped when a PreFilter plugin returns `UnschedulableAndUnresolvable`

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2023-08-06T01:58:32Z

Please note that we're already in Test Freeze for the release-1.28 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.28.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Sat Aug 5 22:30:03 UTC 2023.

sanposhiho · 2023-08-06T02:01:01Z

will soon add UTs

sanposhiho · 2023-08-06T06:16:06Z

/cc @Huang-Wei

Huang-Wei

LGTM. Could you incorporate the integration test in (#119777) to this PR?

sanposhiho · 2023-08-07T15:03:11Z

Sure, added.

Huang-Wei · 2023-08-07T17:32:11Z

test/integration/scheduler/preemption/preemption_test.go

+	if _, err := state.Read(tokenFilterName); err != nil {
+		// Should be bug.
+		// We don't store state only when PreFilter returned Skip.
+		// In other words, if reaches here, PreFilter returned Skip but somehow Filter is called.
+		return framework.NewStatus(framework.Error, err.Error())
+	}


Place this to AddPod/RemovePod would be more close to real world usage.

Huang-Wei · 2023-08-07T17:45:38Z

test/integration/scheduler/preemption/preemption_test.go

-	Unresolvable bool
+	Tokens                 int
+	PreFilterStatus        *framework.Status
+	PreFilterResult        *framework.PreFilterResult


this parameter is not used in any test?

Huang-Wei · 2023-08-07T18:12:15Z

test/integration/scheduler/preemption/preemption_test.go

 }

 func (fp *tokenFilter) PreFilter(ctx context.Context, state *framework.CycleState, pod *v1.Pod) (*framework.PreFilterResult, *framework.Status) {
+	if pod.Name == fp.PreFilterTargetPodName && fp.PreFilterStatus.IsSkip() {


I think here we should only check is: (not quite a fan to check the pod name if we can achieve this by the following logic)

if fp.PreFilterStatus.Code() == framework.UnschedulableAndUnresolvable || fp.PreFilterStatus.IsSkip() { return nil, fp.PreFilterStatus }

Huang-Wei · 2023-08-07T18:12:56Z

test/integration/scheduler/preemption/preemption_test.go

+
+	state.Write(tokenFilterName, &stateData{})
+
+	if pod.Name == fp.PreFilterTargetPodName {


ditto, I think we're able to get rid of PreFilterTargetPodName

Huang-Wei · 2023-08-07T18:21:18Z

test/integration/scheduler/preemption/preemption_test.go

+					v1.ResourceMemory: *resource.NewQuantity(200, resource.DecimalSI)},
+				},
+			}),
+			preemptedPodIndexes: map[int]struct{}{},


if this is nil, it seems the existing test logic is problematic: (w/o the fix, the test would also pass)

The else loop should be employing a "Eventually" semantics; ohterwise it would always pass. In other words, a) the p in else block should be obtained from api server and b) we should wait a bit to ensure the preemption doesn't happen. But this is an existing test issue.

// Wait for preemption of pods and make sure the other ones are not preempted. for i, p := range pods { if _, found := test.preemptedPodIndexes[i]; found { ... } else { if p.DeletionTimestamp != nil { t.Errorf("Didn't expect pod %v to get preempted.", p.Name) } } }

Huang-Wei · 2023-08-07T18:23:46Z

Given the integration test has been wonky for a while, and the UT should suffice to verify the fix. I'm inclined with a separate PR to polish the integration test: including

correct its semantics first (mentioned in fix: when PreFilter returns UnschedulableAndUnresolvable, copy the state in all nodes in statusmap #119778 (comment))
add a new test to cover PreFilter plugin returning UnschedulableAndUnresolvable

alculquicondor · 2023-08-10T14:14:42Z

pkg/scheduler/schedule_one.go

+		// needed to avoid this copy.
+		for _, n := range allNodes {
+			diagnosis.NodeToStatusMap[n.Node().Name] = s
+		}
 		// Record the messages from PreFilter in Diagnosis.PreFilterMsg.
 		msg := s.Message()
 		diagnosis.PreFilterMsg = msg


Could this cause the message to be duplicated in the event/log?

Good question. @sanposhiho could you double check the event/log is still rendered properly?

It's a good catch. I changed type.go so that we won't make duplicates in the message.

alculquicondor · 2023-08-10T14:15:45Z

test/integration/scheduler/preemption/preemption_test.go

@@ -100,8 +100,11 @@ func waitForNominatedNodeName(cs clientset.Interface, pod *v1.Pod) error {
 const tokenFilterName = "token-filter"

 type tokenFilter struct {


Can you add a comment describing this plugin?

Given it's likely this PR will proceed with UT only, I will add comment in #119769.

alculquicondor · 2023-08-10T14:17:15Z

+1 on a quick fix with UTs that can be merged ASAP.

alculquicondor · 2023-08-10T19:10:33Z

The effect here would be that the preemption fails with an error, causing the pod to be re-queued into backoff, instead of unschedulable. Correct?

Huang-Wei · 2023-08-10T19:49:02Z

The effect here would be that the preemption fails with an error, causing the pod to be re-queued into backoff, instead of unschedulable. Correct?

In most cases, yes - just unnecessary cycles spent on preemption which should have been avoided.

But, with a plugin returning UnschedulableAndResolvable in PreFilter, and due to the regression, preemption may be able to find a node to host the pod by preempting pods (b/c dryrun preemption runs Filter only). In this edge case, it'd cause unexpected disruption to existing pods.

alculquicondor · 2023-08-10T19:55:40Z

Hopefully we can merge before the rc.1 tomorrow

…ate in all nodes in statusmap

k8s-ci-robot · 2023-08-12T06:59:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

sanposhiho · 2023-08-12T07:02:24Z

@alculquicondor @Huang-Wei
Sorry, I didn't have much time on this for a few days.

As suggested,

add changes on type.go so that this PR won't make duplicates in the message.
- - add test for this.
remove the integration test change from this PR for now.

PTAL again.

sanposhiho · 2023-08-12T07:40:08Z

/retest

Huang-Wei · 2023-08-13T23:40:30Z

/release-note-edit release-note Fix a regression to ensure preemption is skipped when a PreFilter plugin returns UnschedulableAndUnresolvable

Huang-Wei · 2023-08-13T23:41:35Z

@sanposhiho LGTM. Could you polish the release note a bit? like ^^

/lgtm

k8s-ci-robot · 2023-08-13T23:41:45Z

LGTM label has been added.

Git tree hash: 0b5316f8f51ce76a8524cab2db4a640bdba927ea

sanposhiho · 2023-08-14T00:56:21Z

@Huang-Wei done.

alculquicondor · 2023-08-14T12:24:45Z

Let's keep an eye for the end of the code freeze.
I think we could prepare the cherry-picks already.

sftim · 2023-08-14T16:50:06Z

Changelog suggestion

Fixed a scheduling bug by ensuring that preemption is skipped when a PreFilter plugin returns `UnschedulableAndUnresolvable`

sanposhiho · 2023-08-15T13:34:46Z

cherry-picks:

1.26: Automated cherry pick of #119778: fix: when PreFilter returns UnschedulableAndUnresolvable, copy the state in all nodes in statusmap #119953
1.27: Automated cherry pick of #119778: fix: when PreFilter returns UnschedulableAndUnresolvable, copy the state in all nodes in statusmap #119952
1.28: Automated cherry pick of #119778: fix: when PreFilter returns UnschedulableAndUnresolvable, copy the state in all nodes in statusmap #119951

…119778-upstream-release-1.26 Automated cherry pick of #119778: fix: when PreFilter returns UnschedulableAndUnresolvable, copy the state in all nodes in statusmap

…119778-upstream-release-1.28 Automated cherry pick of #119778: fix: when PreFilter returns UnschedulableAndUnresolvable, copy the state in all nodes in statusmap

…119778-upstream-release-1.27 Automated cherry pick of #119778: fix: when PreFilter returns UnschedulableAndUnresolvable, copy the state in all nodes in statusmap

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 6, 2023

k8s-ci-robot requested review from chendave and damemi August 6, 2023 01:58

k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 6, 2023

sanposhiho mentioned this pull request Aug 6, 2023

[not for merge] reproduce the preemption bugs + proven it's fixed by patches #119780

Closed

sanposhiho mentioned this pull request Aug 6, 2023

add tests to reproduce bugs in preemption #119777

Closed

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 6, 2023

sanposhiho force-pushed the bugfix-unschedulableandunresolvable branch from e00983f to 3320a0a Compare August 6, 2023 06:05

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

This was referenced Aug 6, 2023

the preemption happens even when PreFilter returns UnschedulableAndUnresolvable #119782

Closed

[discussion] potential bug in the scheduling path when one PreFilter plugin returns Unschedulable #119770

Closed

k8s-ci-robot requested a review from Huang-Wei August 6, 2023 06:16

sanposhiho force-pushed the bugfix-unschedulableandunresolvable branch from 3320a0a to e08e6e9 Compare August 6, 2023 09:41

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 Aug 6, 2023

Huang-Wei reviewed Aug 6, 2023

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 7, 2023

Huang-Wei reviewed Aug 7, 2023

View reviewed changes

alculquicondor reviewed Aug 10, 2023

View reviewed changes

fix: when PreFilter returns UnschedulableAndUnresolvable, copy the st…

b008223

…ate in all nodes in statusmap

sanposhiho force-pushed the bugfix-unschedulableandunresolvable branch from 9730596 to b008223 Compare August 12, 2023 06:59

k8s-ci-robot assigned Huang-Wei Aug 13, 2023

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

k8s-ci-robot merged commit 719d1a8 into kubernetes:master Aug 16, 2023
12 checks passed

k8s-ci-robot added this to the v1.29 milestone Aug 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: when PreFilter returns UnschedulableAndUnresolvable, copy the state in all nodes in statusmap #119778

fix: when PreFilter returns UnschedulableAndUnresolvable, copy the state in all nodes in statusmap #119778

sanposhiho commented Aug 6, 2023 •

edited by liggitt

k8s-ci-robot commented Aug 6, 2023

sanposhiho commented Aug 6, 2023

sanposhiho commented Aug 6, 2023

Huang-Wei left a comment

sanposhiho commented Aug 7, 2023

Huang-Wei Aug 7, 2023

Huang-Wei Aug 7, 2023

Huang-Wei Aug 7, 2023

Huang-Wei Aug 7, 2023

Huang-Wei Aug 7, 2023

Huang-Wei commented Aug 7, 2023

alculquicondor Aug 10, 2023 •

edited

Huang-Wei Aug 10, 2023

sanposhiho Aug 12, 2023

alculquicondor Aug 10, 2023

Huang-Wei Aug 10, 2023

alculquicondor commented Aug 10, 2023

alculquicondor commented Aug 10, 2023

Huang-Wei commented Aug 10, 2023

alculquicondor commented Aug 10, 2023

k8s-ci-robot commented Aug 12, 2023

sanposhiho commented Aug 12, 2023 •

edited

sanposhiho commented Aug 12, 2023

Huang-Wei commented Aug 13, 2023

Huang-Wei commented Aug 13, 2023

k8s-ci-robot commented Aug 13, 2023

sanposhiho commented Aug 14, 2023

alculquicondor commented Aug 14, 2023

sftim commented Aug 14, 2023

sanposhiho commented Aug 15, 2023


		state.Write(tokenFilterName, &stateData{})

		if pod.Name == fp.PreFilterTargetPodName {

		@@ -100,8 +100,11 @@ func waitForNominatedNodeName(cs clientset.Interface, pod *v1.Pod) error {
		const tokenFilterName = "token-filter"

		type tokenFilter struct {

fix: when PreFilter returns UnschedulableAndUnresolvable, copy the state in all nodes in statusmap #119778

fix: when PreFilter returns UnschedulableAndUnresolvable, copy the state in all nodes in statusmap #119778

Conversation

sanposhiho commented Aug 6, 2023 • edited by liggitt

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Aug 6, 2023

sanposhiho commented Aug 6, 2023

sanposhiho commented Aug 6, 2023

Huang-Wei left a comment

Choose a reason for hiding this comment

sanposhiho commented Aug 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

Huang-Wei commented Aug 7, 2023

alculquicondor Aug 10, 2023 • edited

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

alculquicondor commented Aug 10, 2023

alculquicondor commented Aug 10, 2023

Huang-Wei commented Aug 10, 2023

alculquicondor commented Aug 10, 2023

k8s-ci-robot commented Aug 12, 2023

sanposhiho commented Aug 12, 2023 • edited

sanposhiho commented Aug 12, 2023

Huang-Wei commented Aug 13, 2023

Huang-Wei commented Aug 13, 2023

k8s-ci-robot commented Aug 13, 2023

sanposhiho commented Aug 14, 2023

alculquicondor commented Aug 14, 2023

sftim commented Aug 14, 2023

sanposhiho commented Aug 15, 2023

sanposhiho commented Aug 6, 2023 •

edited by liggitt

alculquicondor Aug 10, 2023 •

edited

sanposhiho commented Aug 12, 2023 •

edited