WIP: [nodeutilization]: allow to set/remove a soft taint from overutilized nodes #1625

tiraboschi · 2025-02-06T16:49:54Z

Dynamically set/remove a soft taint (effect: PreferNoSchedule) to/from nodes that the descheduler identified as overutilized according to the nodeutilization plugin.
After #1555 the nodeutilization plugin can consume utilization data from actual kubernetes metrics and not only static reservations (requests).
On the other side, the default kube-scheduler is only going to ensure that Pod's specific resource requests can be satisfied.
The soft taint will simply gave an hint to the scheduler to try avoiding nodes that looks overutilized at
descheduler eyes.
Since it's just a soft taint (do it just if possible) there is no need to restrict it only to a certain amount of nodes in the cluster or be strict on error handling.
With a fine-grained ValidatingAdmissionPolicy (GA in k8s 1.30)
we can enforce that the descheduler-sa is only allowed
to apply/remove soft-taints (enforcing PreferNoSchedule effect)
and with a predefined key-prefix (eventually customizable with a parameter).

TODO: add more unit and e2e tests

Fixes: #1626

k8s-ci-robot · 2025-02-06T16:50:04Z

Welcome @tiraboschi!

It looks like this is your first PR to kubernetes-sigs/descheduler 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/descheduler has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2025-02-06T16:50:05Z

Hi @tiraboschi. Thanks for your PR.

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

k8s-ci-robot · 2025-02-06T16:50:11Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign seanmalloy 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:

OWNERS

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

googs1025 · 2025-02-10T03:08:26Z

/ok-to-test

googs1025 · 2025-02-12T02:56:21Z

/cc
I will try to understand this pr

k8s-ci-robot · 2025-02-12T02:56:24Z

@googs1025: GitHub didn't allow me to request PR reviews from the following users: I, will, try, to, understand, this, pr.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc I will try to understand this 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-sigs/prow repository.

googs1025 · 2025-02-12T14:59:01Z

README.md

@@ -291,19 +291,28 @@ like `kubectl top`) may differ from the calculated consumption, due to these com
 actual usage metrics. Metrics-based descheduling can be enabled by setting `metricsUtilization.metricsServer` field.
 In order to have the plugin consume the metrics the metric collector needs to be configured as well.
 See `metricsCollector` field at [Top Level configuration](#top-level-configuration) for available options.
+That gap can only be reduced using the SoftTainter to dinamically apply/remove soft taints (`effect: PreferNoSchedule`) to/from nodes.


I'm not sure if setting taint to node has left the descheduler itself 🤔

Here the key is on Soft (effect: PreferNoSchedule) taints only. I tend to this that having the descheduler setting also hard taints is a bad idea.
In the last update I also added a ValidatingAdmissionPolicy (GA in k8s 1.30) with a few rules to enforce that the ServiceAcoount used by the scheduler is only allowed to set/delete/amend its own taints (by a fixed key prefix) and only if with effect: PreferNoSchedule.

What I am concerned about is whether this is not a feature of the descheduler. I understand that after rescheduling, new pods may still be scheduled to this node again. But we need to think whether adding taints to the descheduler is a good choice. Maybe we should use a custom component to accomplish this? 🤔 I don't seem to see other plugins that have similar operations for modifying node fields or taints (maybe I'm wrong, I haven't delved into all the plugins).

similar issue from : kubernetes/node-problem-detector#565

There are similar discussions in the NDP project. I understand that the two projects are different, but when we introduce similar feature, do we have to think clearly or consider it more general? 🤔

On the NPD front the discussion was more about hard taints "effect": "NoSchedule" to filter out problematic nodes and yes, in that case all the concerns are valid since, for instance, hard tainting a relevant amount of nodes could lead the cluster in a position where it could not have enough resource to run expected pods.
Here instead we are only talking about soft taints as an hint for the scheduler to avoid overutilized nodes if possible.
Please notice that in the scheduler (intolerable) soft taints are simply counted for the scoring step:
https://github.com/kubernetes/kubernetes/blob/master/pkg/scheduler/framework/plugins/tainttoleration/taint_toleration.go#L198
But they are not going to affect the filtering step.
And since the soft taints are already counted to weight a node, neither having two independent controllers setting them is really an issue since a node with more than one soft taint will symple receive a worst score than a node with just a single intolerable soft taint.
We can eventually also think about leveraging that counting mechanism also here applying a variable number of soft taints according to how much a node is over utilized to get nodes slightly overutilized preferred against nodes that are considerably over utilized.

What I mean is: Does this deviate from the original intention of the descheduler? But I understand that the feature is reasonable. 🤔

googs1025 · 2025-02-12T15:00:42Z

pkg/descheduler/node/node.go

+// IsNodeSoftTainted checks if the node is already tainted
+// with the provided key and value and PreferNoSchedule effect.
+// If a match is found, it returns true.
+func IsNodeSoftTainted(node *v1.Node, taintKey, taintValue string) bool {


IsNodeSoftTainted AddOrUpdateSoftTaint RemoveSoftTaint maybe we can remove Soft

It was just to emphasize that the descheduler is only supposed to add Soft taints as an hint for the scheduler to avoid overutilized nodes if possible without really interfering with the scheduling process.

got this. I missed TaintEffectPreferNoSchedule

…lized nodes Dynamically set/remove a soft taint (effect: PreferNoSchedule) to/from nodes that the descheduler identified as overutilized according to the nodeutilization plugin. After kubernetes-sigs#1555 the nodeutilization plugin can consume utilization data from actual kubernetes metrics and not only static reservations (requests). On the other side, the default kube-scheduler is only going to ensure that Pod's specific resource requests can be satified. The soft taint will simply gave an hint to the scheduler to try avoiding nodes that looks overutilized at the descheduler eyes. Since it's just a soft taint (do it just if possible) there is no need to restrict it only to a certain amount of nodes in the cluster or be strict on error handling. With a fine-grained `ValidatingAdmissionPolicy` (GA in k8s 1.30) we can enforce that the `deschduler-sa` is only allowed to apply/remove soft-taints (enforcing `PreferNoSchedule` effect) and with a predefined key-prefix (eventually customizable with a parameter). TODO: add more unit and e2e tests Signed-off-by: Simone Tiraboschi <[email protected]>

JaneLiuL · 2025-03-05T08:05:28Z

welcome for First-time contributor~~ look forward to see you more and more PR and contribute~~

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 6, 2025

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 6, 2025

k8s-ci-robot requested review from a7i and damemi February 6, 2025 16:50

tiraboschi force-pushed the softtainter branch from 3fdbda3 to fdf1a3a Compare February 6, 2025 17:38

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 10, 2025

tiraboschi force-pushed the softtainter branch 2 times, most recently from 61f2819 to 00d0c86 Compare February 11, 2025 15:07

googs1025 reviewed Feb 12, 2025

View reviewed changes

tiraboschi force-pushed the softtainter branch from 00d0c86 to 1375b73 Compare February 12, 2025 15:48

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 12, 2025

tiraboschi force-pushed the softtainter branch 4 times, most recently from 9cbd483 to f57c4e8 Compare February 17, 2025 14:01

tiraboschi force-pushed the softtainter branch from f57c4e8 to 3bddb9d Compare February 17, 2025 16:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: [nodeutilization]: allow to set/remove a soft taint from overutilized nodes #1625

WIP: [nodeutilization]: allow to set/remove a soft taint from overutilized nodes #1625

tiraboschi commented Feb 6, 2025 •

edited

Loading

k8s-ci-robot commented Feb 6, 2025

k8s-ci-robot commented Feb 6, 2025

k8s-ci-robot commented Feb 6, 2025

googs1025 commented Feb 10, 2025

googs1025 commented Feb 12, 2025 •

edited

Loading

k8s-ci-robot commented Feb 12, 2025

googs1025 Feb 12, 2025

tiraboschi Feb 12, 2025

googs1025 Feb 13, 2025

googs1025 Feb 13, 2025

tiraboschi Feb 13, 2025

googs1025 Feb 13, 2025

googs1025 Feb 12, 2025

tiraboschi Feb 12, 2025

googs1025 Feb 13, 2025

JaneLiuL commented Mar 5, 2025

WIP: [nodeutilization]: allow to set/remove a soft taint from overutilized nodes #1625

Are you sure you want to change the base?

WIP: [nodeutilization]: allow to set/remove a soft taint from overutilized nodes #1625

Conversation

tiraboschi commented Feb 6, 2025 • edited Loading

k8s-ci-robot commented Feb 6, 2025

k8s-ci-robot commented Feb 6, 2025

k8s-ci-robot commented Feb 6, 2025

googs1025 commented Feb 10, 2025

googs1025 commented Feb 12, 2025 • edited Loading

k8s-ci-robot commented Feb 12, 2025

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

JaneLiuL commented Mar 5, 2025

tiraboschi commented Feb 6, 2025 •

edited

Loading

googs1025 commented Feb 12, 2025 •

edited

Loading