chore: improve distruption for underutilization

[Luke-Smartnews]

Background

we're trying to use karpenter in our production environment, the scaling-out feature is working well, but the scaling-in(disruption) is a problem to us. There're are only two options: WhenUnderutilized | WhenEmpty ,

• WhenEmpty: basically useless
• WhenUnderutilized: we see frequently node deletion and creation which reduces SLA. (Consolidation ttl: spec.disruption.consolidateAfter #735)

Proposals

• add an option to check the node utilization first, normally if a node is fully utilized no need for disruption.
• support consolidateAfter for WhenUnderutilized

[linux-foundation-easycla]

• ❌ - login: @Luke-Smartnews / name: Luke . The commit (e4c9cac, 13dda7c, a3e57ea, 87626aa, 4f2ac72, 81e21fc, 0750608, ec247da, b72e6de, 0943800, f6e9789, 6b3b991, cda94ab) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Luke-Smartnews
Once this PR has been reviewed and has the lgtm label, please assign njtran for approval. For more information see the Kubernetes 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

[k8s-ci-robot]

Welcome @Luke-Smartnews!

It looks like this is your first PR to kubernetes-sigs/karpenter 🎉. 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/karpenter 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]

Hi @Luke-Smartnews. 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/test-infra repository.

[Bryce-Soghigian]

please run make toolchain to update your controller-gen version

[Luke-Smartnews]

Tested in our dev env, working without any issues. Will do more load tests.

[almson]

Can you write a description of how you've implemented this? This seems to drastically change how "WhenUnderutilized" works. Previously, it took effect when the cluster as a whole was underutilized, but now it seems to just mean that the node is underutilized?

[Bryce-Soghigian]

Can you write a description of how you've implemented this? This seems to drastically change how "WhenUnderutilized" works.

Excited to see a POC!

Further than a description it should probably be first proposed as an RFC to get reviewed by the community, and to talk about alternative approaches or if this is behavior the community wants to accept. Seems interesting though :)

[Luke-Smartnews]

will do after I confirm it works on a large scale. also is there a template of RFC?

[Luke-Smartnews]

Test Report

• nodepool settings

   spec:
     disruption:
       budgets:
       - nodes: 10%
       consolidateAfter: 5m
       consolidationPolicy: WhenUnderutilized
       expireAfter: Never
       utilizationThreshold: 65

• confirmed Underutilized added to target nodes

    conditions:
    - lastTransitionTime: "2024-02-23T11:24:05Z"
      status: "True"
      type: Initialized
    - lastTransitionTime: "2024-02-23T11:22:54Z"
      status: "True"
      type: Launched
    - lastTransitionTime: "2024-02-23T11:24:05Z"
      status: "True"
      type: Ready
    - lastTransitionTime: "2024-02-23T11:23:40Z"
      status: "True"
      type: Registered
    - lastTransitionTime: "2024-02-23T11:31:53Z"
      severity: Warning
      status: "True"
      type: Underutilized

• pending pods scheduled in 5m
  

• nodes are removed 5m later after pods deletion
  

[njtran]

Hey @Luke-Smartnews, this is a core difference in how Karpenter considers underutilization. We've intentionally not surfaced a utilization threshold due to its edge cases and how it ends up driving overall lower utilization. This would be a huge change to our disruption logic, and would definitely require opening up a design + RFC. I'm not sure we would be accepting this feature (at least in its current state without a design), but I would love to hear about the use-cases you're trying to solve here.

[barryrobison]

Hey @Luke-Smartnews, this is a core difference in how Karpenter considers underutilization. We've intentionally not surfaced a utilization threshold due to its edge cases and how it ends up driving overall lower utilization. This would be a huge change to our disruption logic, and would definitely require opening up a design + RFC. I'm not sure we would be accepting this feature (at least in its current state without a design), but I would love to hear about the use-cases you're trying to solve here.

@njtran is the current utilisation logic documented somewhere? From my brief reading of the source, it looks like karpenter does a "fake" scheduling run, and if the nodes pods can be scheduled elsewhere it consolidates the node?

Our use case is flink jobs performing data transfer. Those pods are bursty in nature, and we want to insure those pods run for at least 10 minutes so they can reach a checkpoint before getting rescheduled. The current system is reclaiming nodes in under 90s, so they barely have time to get scheduled before karpenter pulls the rug.

Thank you!

[njtran]

@barryrobison I think you want this #752, you're correct in that's how Karpenter considers Consolidation

[Luke-Smartnews]

@njtran
my change only take effect when the nodepool is configured whit UtilizationThreshold, it doesn't change the existing behavior at all.

https://github.com/kubernetes-sigs/karpenter/pull/992/files#diff-c11b5a9240bbfac3dd14fdfff84e098ade0f3bf0a5fc63673de41ff710d3d308R105

[njtran]

Hey @Luke-Smartnews, even if this is just a change to the candidacy, we'd need to review a design/RFC before continuing on this. Would you be able to open a design PR first?

[github-actions]

This PR has been inactive for 14 days. StaleBot will close this stale PR after 14 more days of inactivity.

[k8s-ci-robot]

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/test-infra repository.

[njtran]

Will close this until there's an RFC for this. Please feel free to re-open when you cut the RFC!

[booleanbetrayal]

@njtran - Could you refer to the RFC process needed to unblock this PR? We are seeing worse bin-packing performance from Karpenter than cluster-autoscaler and are debating switching back. Being able to leverage advanced consolidation features (eval time + thresholds) would put Karpenter on an equal footing for us. Please advise.