hotfix when a plugin (in-tree or out-of-tree) return non-existent/illegal nodes, the pod scheduling flow will abort immediately.

[chengjoey]

What type of PR is this?

/kind bug

What this PR does / why we need it:

this is a minimum fix from #119779
In versions 1.27-1.29, if there is a non-existent node in the PreFilterResult, the pod schedule will fail.

Which issue(s) this PR fixes:

Fixes #123465

Special notes for your reviewer: @Huang-Wei @alculquicondor

Does this PR introduce a user-facing change?

Fixed a bug that a pod may remain unscheduled if any PreFilter plugin returns nodes that do not exist.

[k8s-ci-robot]

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.

[k8s-ci-robot]

Hi @chengjoey. Thanks for your PR.

I'm waiting for a kubernetes 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.

[alculquicondor]

/ok-to-test

[alculquicondor]

Also, let's wait for #124539 to merge and also cherry-pick in this same PR.

[chengjoey]

/test pull-kubernetes-unit

[chengjoey]

/test pull-kubernetes-unit-go-compatibility

[alculquicondor]

I don't think the tests are flaky. Maybe they are making some assumptions about the nodes existing.

If it becomes too much burden to fix them, I think we should just cherry-pick the entire original PR #119779

[chengjoey]

I don't think the tests are flaky. Maybe they are making some assumptions about the nodes existing.

If it becomes too much burden to fix them, I think we should just cherry-pick the entire original PR #119779

the tests are not flaky, but the original PR 119779 also modified the test, but the test was not changed just now, so it is normal now

[Huang-Wei]

It's b/c @sanposhiho 's PR introduced a change to set UnschedulableAndUnresolvable to each node that's filtered out by PreFilterResult:

kubernetes/pkg/scheduler/schedule_one.go

Lines 486 to 497 in 7d880fd

	if !preRes.AllNodes() {
	nodes = make([]*framework.NodeInfo, 0, len(preRes.NodeNames))
	for _, n := range allNodes {
	if !preRes.NodeNames.Has(n.Node().Name) {
	// We consider Nodes that are filtered out by PreFilterResult as rejected via UnschedulableAndUnresolvable.
	// We have to record them in NodeToStatusMap so that they won't be considered as candidates in the preemption.
	diagnosis.NodeToStatusMap[n.Node().Name] = framework.NewStatus(framework.UnschedulableAndUnresolvable, "node is filtered out by the prefilter result")
	continue
	}
	nodes = append(nodes, n)
	}
	}

And the logic of counting processedNodes is based on feasibleNodes and diagnosis.NodeToStatusMap:

kubernetes/pkg/scheduler/schedule_one.go

Line 501 in 7d880fd

processedNodes := len(feasibleNodes) + len(diagnosis.NodeToStatusMap)

So it caused the UT's change, but it's not a big issue IMHO.

@chengjoey could you also incorporate d382150 into this PR?

[chengjoey]

cherry-pick #124714
preallocation for NodeToStatusMap

[sanposhiho]

cherry-pick #124714
preallocation for NodeToStatusMap

@chengjoey Please exclude it from this PR. The cheery-pick for mine should be separated. I'll prepare the one.

[sanposhiho]

Oh, wait. @alculquicondor, did you intend to include a cherry-pick for #124709 in this PR?
I thought we should prepare a separate one not to include multiple bug fixes in one PR.

Sorry, this will require more work. See #124709 for details
#124559 (comment)

[chengjoey]

/test pull-kubernetes-integration-go-compatibility

[alculquicondor]

/hold cancel
#124753 targets 1.30.

This PR targets 1.29 and older. We need to include #124714 here.

[alculquicondor]

I thought we should prepare a separate one not to include multiple bug fixes in one PR.

1.29 and older are currently unaffected. They will only be affected if this PR merges. Thus, we should include the performance fix.

[alculquicondor]

/approve
/lgtm
/cc kubernetes/release-managers

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 693334c8e36df57a41a6a3bbb24eab62fa8f607a

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, chengjoey

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 [alculquicondor]
• test/e2e/scheduling/OWNERS [alculquicondor]
• test/integration/scheduler/OWNERS [alculquicondor]

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

[alculquicondor]

Anything we are missing? otherwise, I think you can prepare the cherry-picks for 1.28 and 1.27 based on this PR

[chengjoey]

there are some conflicts in schedule_one_test, in 1.27 and 1.28, so manual cherry-pick

[alculquicondor]

/kind regression

[Verolop]

/lgtm
(Release Managers)

[k8s-triage-robot]

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

• The PR does have any do-not-merge/* labels
• The PR does not have the needs-ok-to-test label
• The PR is mergeable (does not have a needs-rebase label)
• The PR is approved (has cncf-cla: yes, lgtm, approved labels)
• The PR is failing tests required for merge

You can:

• Review the full test history for this PR
• Prevent this bot from retesting with /lgtm cancel or /hold
• Help make our tests less flaky by following our Flaky Tests Guide

/retest