NodeAffinity/NodeUnschedulable QueueingHint may miss Node related events that make Pod schedulable

[sanposhiho]

What happened?

/kind bug
/triage accepted
/priority urgent
/sig scheduling
/assign

NodeAffinity QueueingHint may miss Node related events that make Pod schedulable because of preCheck.
It's similar to: #119177 (comment)

So:

1. Node is added. But, it's filtered out by preCheck (due to node's unready or whatever) and the scheduling queue doesn't receive NodeAdded. (noderesourcefit cannot receive NodeAdded.)
2. Node is updated and ready now. But, this event is NodeUpdated.

In such scenarios, NodeAffinity may return QueueSkip to the event due to (2).
https://github.com/kubernetes/kubernetes/blob/master/pkg/scheduler/framework/plugins/nodeaffinity/node_affinity.go#L94-L131

What did you expect to happen?

We have to loosen the filtering in isSchedulableAfterNodeChange until preCheck is removed.
#110175

How can we reproduce it (as minimally and precisely as possible)?

1. Create a Pod with NodeAffinity under the situation where no Node can accommodate the Pod.
2. Create a new Node.

It may cause the above problematic scenario.

Anything else we need to know?

No response

Kubernetes version

master

Cloud provider

OS version

# On Linux:
$ cat /etc/os-release
# paste output here
$ uname -a
# paste output here

# On Windows:
C:\> wmic os get Caption, Version, BuildNumber, OSArchitecture
# paste output here

Install tools

Container runtime (CRI) and version (if applicable)

Related plugins (CNI, CSI, ...) and versions (if applicable)

[k8s-ci-robot]

@sanposhiho: The label(s) priority/urgent cannot be applied, because the repository doesn't have them.

In response to this:

What happened?

/kind bug
/triage accepted
/priority urgent
/sig scheduling
/assign

NodeAffinity QueueingHint may miss Node related events that make Pod schedulable because of preCheck.
It's similar to: #119177 (comment)

So:

1. Node is added. But, it's filtered out by preCheck (due to node's unready or whatever) and the scheduling queue doesn't receive NodeAdded. (noderesourcefit cannot receive NodeAdded.)
2. Node is updated and ready now. But, this event is NodeUpdated.

In such scenarios, NodeAffinity may return QueueSkip to the event due to (2).
https://github.com/kubernetes/kubernetes/blob/master/pkg/scheduler/framework/plugins/nodeaffinity/node_affinity.go#L94-L131

What did you expect to happen?

We have to loosen the filtering in isSchedulableAfterNodeChange until preCheck is removed.
#110175

How can we reproduce it (as minimally and precisely as possible)?

1. Create a Pod with NodeAffinity under the situation where no Node can accommodate the Pod.
2. Create a new Node.

It may cause the above problematic scenario.

Anything else we need to know?

No response

Kubernetes version

master

Cloud provider

OS version

# On Linux:
$ cat /etc/os-release
# paste output here
$ uname -a
# paste output here

# On Windows:
C:\> wmic os get Caption, Version, BuildNumber, OSArchitecture
# paste output here

Install tools

Container runtime (CRI) and version (if applicable)

Related plugins (CNI, CSI, ...) and versions (if applicable)

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.

[sanposhiho]

/priority critical-urgent

[sanposhiho]

/assign @carlory

I'll have a PR to just revert QHint since the release date is close. But, I'll leave the QHint re-implementation for @carlory.

[sanposhiho]

/cc @kubernetes/sig-scheduling-leads

[sanposhiho]

Wait... It's the same for NodeUnschedulable. I should have noticed during the review 🤦

/retitle NodeAffinity/NodeUnschedulable QueueingHint may miss Node related events that make Pod schedulable

[sanposhiho]

/assign @wackxu
For the NodeUnschedulable QueueingHint re-implementation.

[Vyom-Yadav]

@sanposhiho The release cut is under progress, are we reverting #119396 and #119155?

[sanposhiho]

@Vyom-Yadav
Yes, I submitted the PRs.

• Revert "scheduler/nodeaffinity: reduce pod scheduling latency" #122285
• Revert "scheduler/NodeUnschedulable: reduce pod scheduling latency" #122288

But, unfortunately, I suppose our leads are not around, they're all in US timezone and it's late night now.

[kerthcet]

All node related plugins are influenced.

[sanposhiho]

Yes, I just went through all, and seems these two are the only ones that has got QHint in the last release.

Other QHints:

• NodePort 🙆‍♂️ https://github.com/sanposhiho/kubernetes/blob/7cba35f6519c6e0cf72848185ef3f9a524601467/pkg/scheduler/framework/plugins/nodeports/node_ports.go#L118-L124
• DRA: doesn't have QHint for Node events

[sanposhiho]

Wait... I think #109437 has been silently coming back since it's closed. (We should have mentioned it in the comment somewhere...)
Regardless of whether QHint is implemented or not, we have to make sure all plugins that have Node Add event to have Node Update event. I see some plugins don't follow it. (we must mention it in the doc somewhere.)

[kerthcet]

If you mean the newly added plugins like the DRA, I think yes, let me check that in.

[sanposhiho]

Yes, I meant that. For newly added plugins like DRA, they have NodeAdd, but not NodeUpdated.
I haven't checked all, but NodeUnschedulable is also the one that reverting QHint is not enough, but adding Node Update event is necessary to be added to completely fix the issue.

[sanposhiho]

We discussed at slack:
https://kubernetes.slack.com/archives/C2C40FMNF/p1702438650646789

And decided we'll make the feature gate for QHint disable by default. (There was a discussion of disablement for this before actually because we observed a memory consumption issue: #120622) This issue reinforces that idea. We'll make it enable by default once all issues here + memory consumption issue are addressed.

[sanposhiho]

Also, one general problem is that we have few integration (or e2e) tests for requeueing scenario.

[kerthcet]

Related problems we faced now:

1. preCheck has some potential bugs as scheduler: move all preCheck to QueueingHint #110175 described, so we're planning to remove preCheck someday(without obvious performance degradation)
2. currently, node related plugins like nodeUnschedulable & nodeAffinity & nodeResourceFit with tight validations in hintFunc is conflict with preCheck as this issue mentioned.
3. Also because of preCheck, node plugins register nodeAdd events should also register the nodeUpdate event, DRA is one plugin we missed. (I'll raise a quick fix with the consistent policy we applied now)

To the 2nd question, a feasible approach is only check the newObj (we're now validating the oldObj and newObj both, only the oldObj doesn't match but the newObj does, then we'll return Queue), still better than before when we only watch for the event. But the disadvantage is not that efficient, like we have two plugins A & B, also two nodes Node1, Node2. When pod is scheduling, Node1 is failed by pluginA, Node2 is failed by pluginB, so when Node2 updated, pluginA will pass the validation however, pod is still unschedulable.

[sanposhiho]

Regarding (3), what do you think about this idea: when plugins register nodeAdd, we automatically register nodeUpdated (until preCheck is removed) so that we could technically prevent #109437 happening again in any plugins (including both in-tree and external).

[sanposhiho]

The action items in my mind (correct me if I am mistaken or missed something.)

• plugins that have nodeAdd should have nodeUpdate as well. (maybe we'll fix it via register Node/UpdateTaint event to plugins which has Node/Add only and doesn't have Node/UpdateTaint #122292)
• revert PRs of NodeAffinity/NodeUnschedulable QHints (Revert "scheduler/nodeaffinity: reduce pod scheduling latency" #122285 and Revert "scheduler/NodeUnschedulable: reduce pod scheduling latency" #122288) and cherry-pick them.
• re-implement QHints for NodeAffinity/NodeUnschedulable.
• have a sufficient number of integration tests for QueueingHint to catch this kind of bugs.
• have a UT for QHint feature gate disabled. (fix: disable SchedulerQueueingHints feature flag by default #122289 (comment))
• remove preCheck eventually. (tracked in scheduler: move all preCheck to QueueingHint #110175)

I'll probably create other issues corresponding to them, not to put a lot of context in this issue later.

[kerthcet]

What I hope is we can have a quick fix for No.3 and cherry-picked, then let's work out the whole stuff in the next release, no longer need to patch anything additionally, and longer.

[alculquicondor]

Also, one general problem is that we have few integration (or e2e) tests for requeueing scenario.

Let's make it a requirement to increase integration coverage of requeuing before re-enabling the feature again.

[sanposhiho]

I created the issues based on the above checklist #122284 (comment)

• re-create unit tests for the scheduling queue for the cases QueueingHint disabled #122304
• Implement the integration tests for requeueing scenarios #122305
• plugins that register nodeAdd in EventsToRegister must register nodeUpdate #122306

---

re-implement QHints for NodeAffinity/NodeUnschedulable.

@wackxu @carlory
Let's track this in #118893, rather than creating another issue. I'll remove ✔️ from these plugins after reverting ↓ are done.

• Revert "scheduler/nodeaffinity: reduce pod scheduling latency" #122285
• Revert "scheduler/NodeUnschedulable: reduce pod scheduling latency" #122288

Other things, memory consumption issue and the removal of preCheck are already tracked:

• scheduler: move all preCheck to QueueingHint #110175
• scheduler: handle in-flight Pods with less memory #120622

[carlory]

@sanposhiho here's the re-implementation of the NodeAffinity plugin, please take a look.

[sanposhiho]

Reverting is done and we have issues for other action items, I believe we can close it.
Feel free to reopen if anyone disagree.

/close

[k8s-ci-robot]

@sanposhiho: Closing this issue.

In response to this:

Reverting is done and we have issues for other action items, I believe we can close it.
Feel free to reopen if anyone disagree.

/close

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.

[Huang-Wei]

@sanposhiho do you want (or already have) to create an umbrella issue tracking all on-going items to re-enable QueueingHint feature. In particular, we don't really want to re-stomp on issue like #109437.

[sanposhiho]

We have all issues individually, but don't have ☔ one. Let me create it for a better tracking.

[sanposhiho]

#122597
I'll organize the list later.