Skip to content

update node info processors to include unschedulable nodes #8520

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

update node info processors to include unschedulable nodes #8520

wants to merge 1 commit into from
+37 −32

Conversation

elmiko
Copy link
Contributor

@elmiko elmiko commented Sep 10, 2025

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR adds a new lister for ready unschedulable nodes, it also connects that lister to a new parameter in the node info processors Process function. This change enables the autoscaler to use unschedulable, but otherwise ready, nodes as a last resort when creating node templates for scheduling simulation.

Which issue(s) this PR fixes:

Fixes #8380

Special notes for your reviewer:

I'm not sure if this is the best way to solve this problem, but i am proposing this for further discussion and design.

Does this PR introduce a user-facing change?

Node groups where all the nodes are ready but unschedulable will be processed as potential candidates for scaling when simulating cluster scheduling.

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

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/bug Categorizes issue or PR as related to a bug. do-not-merge/needs-area cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 10, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: elmiko
Once this PR has been reviewed and has the lgtm label, please assign aleksandra-malinowska for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

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 k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed do-not-merge/needs-area labels Sep 10, 2025
@elmiko
Copy link
Contributor Author

elmiko commented Sep 10, 2025

i'm working on adding more unit tests for this behavior, but i wanted to share this solution so we could start talking about it.

@elmiko elmiko force-pushed the unschedulable-nodes-fix branch from a0ebb28 to 3270172 Compare October 2, 2025 20:50
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 2, 2025
@elmiko
Copy link
Contributor Author

elmiko commented Oct 2, 2025

i've rewritten this patch to use all nodes as the secondary value instead of using a new list of ready unschedulable nodes.

@elmiko elmiko changed the title WIP update to include unschedulable nodes update node info processors to include unschedulable nodes Oct 2, 2025
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 2, 2025
@elmiko
Copy link
Contributor Author

elmiko commented Oct 2, 2025

i need to do a little more testing on this locally, but i think this is fine for review.

elmiko
elmiko commented Oct 3, 2025
View reviewed changes
cluster-autoscaler/processors/nodeinfosprovider/mixed_nodeinfos_processor.go Outdated
// Last resort - unready/unschedulable nodes.
for _, node := range nodes {
// we want to check not only the ready nodes, but also ready unschedulable nodes.
for _, node := range append(nodes, allNodes...) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not sure that this is appropriate to append these. theoretically the allNodes should already contain nodes. i'm going to test this out using just allNodes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

due to filtering that happens in obtainNodeLists, we need to combine both lists of nodes here.

@elmiko elmiko force-pushed the unschedulable-nodes-fix branch from 3270172 to cb2649a Compare October 3, 2025 16:37
@elmiko
Copy link
Contributor Author

elmiko commented Oct 3, 2025

i updated the argument names in the Process function to make the source of the nodes more clear. i also changed the mixed node info processor to not double count the nodes for the unschedulable/unready detection clause.

@elmiko
Copy link
Contributor Author

elmiko commented Oct 3, 2025

it seems like the update to the mixed node processor needs a little more investigation.

@elmiko
update node info processor to include unschedulable nodes
fd53c0b 
This change updates the `Process` function of the node info processor
interface so that it can accept a second list of nodes. The second list
contains all the nodes that are not in the first list. This will allow
the mixed node info processor to properly detect unready and
unschedulable nodes for use as templates.
@elmiko elmiko force-pushed the unschedulable-nodes-fix branch from cb2649a to fd53c0b Compare October 3, 2025 16:59
@elmiko
Copy link
Contributor Author

elmiko commented Oct 3, 2025

it looks like we need both the readyNodes and allNodes lists due to the filtering that happens in the core.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 7, 2025
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

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-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vadasambar vadasambar Awaiting requested review from vadasambar

@x13n x13n Awaiting requested review from x13n

Assignees
No one assigned
Labels
area/cluster-autoscaler cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CA potential for skipped node template info when a node group contains only non-ready nodes
2 participants
@elmiko @k8s-ci-robot