feat: disruption controls by reason #991

Bryce-Soghigian · 2024-02-06T08:46:24Z

Fixes #924

Description
This PR implements the design for disruption-controls-by-reason by introducing a new field Reasons into the v1beta1 Nodepool API. This change allows users to define lists of reasons alongside their node disruption budgets.

These reasons that can be defined are "underutilized", "drifted", "emptied" and "expired" which all correlate with different disruption methods.

How was this change tested?

Kwok Provider Scale up and scale down, validated that budgets were restricted
Parity with existing Budgets feature via no modified budgets tests, and code still behaves properly
Tests for DisruptionBudgetMapping structure
make presubmit

TODO

PR To Docs
Test on AKS Provider

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

k8s-ci-robot · 2024-02-06T08:46:27Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

pkg/controllers/disruption/disruptionbudgetmapping.go

njtran

Left some preliminary comments, but I anticipate this changing after a bit more comments on the RFC so I'll hold off for a bit

kwok/charts/values.yaml

pkg/apis/crds/karpenter.sh_nodepools.yaml

pkg/apis/v1beta1/nodepool.go

pkg/controllers/state/statenode.go

coveralls · 2024-02-11T23:48:14Z

Pull Request Test Coverage Report for Build 9520161962

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

103 of 112 (91.96%) changed or added relevant lines in 12 files are covered.
7 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.09%) to 81.197%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/apis/v1beta1/nodepool.go	14	15	93.33%
pkg/controllers/disruption/multinodeconsolidation.go	5	6	83.33%
pkg/controllers/disruption/singlenodeconsolidation.go	4	5	80.0%
pkg/controllers/disruption/helpers.go	48	50	96.0%
pkg/apis/v1beta1/zz_generated.deepcopy.go	1	5	20.0%

Files with Coverage Reduction	New Missed Lines	%
pkg/test/expectations/expectations.go	2	93.69%
pkg/controllers/disruption/expiration.go	2	89.66%
pkg/apis/v1beta1/nodepool.go	3	95.24%

Totals
Change from base Build 9487761453:	-0.09%
Covered Lines:	8313
Relevant Lines:	10238

💛 - Coveralls

charliedmcb

Overall, like the progress here.

My main concern is somewhat confusing difference between the default shared disruption budgets, and the non-shared budgets when multiple reasons are expressed for the same budget (same budget applies to all).

Both of those forms make logical sense on their own, and I would like to be able to create a budget to get mirrored by multiple reasons. However, I don't think the existing "Default" shared budget idea meshes well with that. Wondering if that could be rephased as something like MaxNodePoolDisruption, and always taken into account as an overall shared max disruption regardless of what the individual reasons disruptions were. I think this area need a bit more design to finalize.

pkg/apis/v1beta1/nodepool.go

pkg/controllers/disruption/disruptionbudgetmapping.go

pkg/apis/v1beta1/nodepool.go

pkg/controllers/disruption/disruptionbudgetmapping.go

Bryce-Soghigian · 2024-02-18T02:16:24Z

However, I don't think the existing "Default" shared budget idea meshes well with that. Wondering if that could be rephased as something like MaxNodePoolDisruption, and always taken into account as an overall shared max disruption regardless of what the individual reasons disruptions were. I think this area need a bit more design to finalize.

Default now is just when its unspecified and meshes with the idea fine imo. I updated the design doc to explain this relationship a bit better. LMK if you have any strong opinions

…isruption commands fix: ignoring emptiness candidates in the single node and multi-node disruption commands Revert "fix: ignoring emptiness candidates in the single node and multi-node disruption commands" This reverts commit b88efe7.

…i-node disruption commands" This reverts commit db8c8ab.

…lidation controllers

Co-authored-by: Nick Tran <10810510+njtran@users.noreply.github.com>

pkg/controllers/disruption/validation.go

This reverts commit da22d40.

njtran

Just one comment. The other nits should be fine. I'll approve after the last comment :)

pkg/apis/v1beta1/nodepool.go

pkg/controllers/disruption/consolidation_test.go

pkg/controllers/disruption/events/events.go

pkg/controllers/disruption/expiration.go

pkg/controllers/disruption/multinodeconsolidation.go

Co-authored-by: Nick Tran <10810510+njtran@users.noreply.github.com>

…ingle and multi node consolidation

njtran

/lgtm
/approve

k8s-ci-robot · 2024-06-17T21:19:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bryce-Soghigian, njtran

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:

~~OWNERS~~ [njtran]

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

ellistarn · 2024-06-17T21:38:08Z

🥂

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. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 6, 2024

k8s-ci-robot requested review from jmdeal and tallaxes February 6, 2024 08:46

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Feb 6, 2024

Bryce-Soghigian mentioned this pull request Feb 6, 2024

docs(RFC): disruption controls by reason #936

Merged

Bryce-Soghigian force-pushed the bsoghigian/disruption-by-method-poc branch from f7dea78 to 18c8dee Compare February 6, 2024 10:31

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2024

Bryce-Soghigian commented Feb 6, 2024

View reviewed changes

pkg/controllers/disruption/disruptionbudgetmapping.go Outdated Show resolved Hide resolved

njtran reviewed Feb 7, 2024

View reviewed changes

kwok/charts/values.yaml Outdated Show resolved Hide resolved

pkg/apis/crds/karpenter.sh_nodepools.yaml Outdated Show resolved Hide resolved

pkg/apis/v1beta1/nodepool.go Outdated Show resolved Hide resolved

Bryce-Soghigian commented Feb 7, 2024

View reviewed changes

pkg/controllers/state/statenode.go Outdated Show resolved Hide resolved

Bryce-Soghigian force-pushed the bsoghigian/disruption-by-method-poc branch 2 times, most recently from 6e5bd23 to 04d9f95 Compare February 12, 2024 20:04

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 13, 2024

charliedmcb reviewed Feb 15, 2024

View reviewed changes

Bryce-Soghigian changed the title ~~[WIP] feat: disruption controls by reason~~ feat: disruption controls by reason Feb 18, 2024

Bryce-Soghigian force-pushed the bsoghigian/disruption-by-method-poc branch 2 times, most recently from 5458e56 to 6b06089 Compare February 20, 2024 08:41

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 20, 2024

Bryce-Soghigian marked this pull request as ready for review February 20, 2024 09:08

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 Feb 20, 2024

k8s-ci-robot requested a review from engedaam February 20, 2024 09:08

Bryce-Soghigian and others added 13 commits June 12, 2024 11:50

refactor: removing useless if statement

38b5e96

Revert "fix: ignoring emptiness canidates in the single node and mult…

29e555e

…i-node disruption commands" This reverts commit db8c8ab.

feat: filtering out empty nodes from single node and multi-node conso…

d1ea3c6

…lidation controllers

fix: rebase changes for migration away from knative status conditions

131882a

Update pkg/controllers/disruption/multinodeconsolidation.go

95672b3

Co-authored-by: Nick Tran <10810510+njtran@users.noreply.github.com>

Update pkg/controllers/disruption/singlenodeconsolidation.go

7c943c3

Co-authored-by: Nick Tran <10810510+njtran@users.noreply.github.com>

test(unit): adding disruption budget mapping tests for all reasons

760ed90

feat: adding reasons api to kwok provider

955d390

Update pkg/controllers/disruption/events/events.go

28c59bb

fix: ci

9cd0b1f

fix: use multi-err when building disruption budgets

523c52d

feat: removing disruption budgets for expirationN

da22d40

Bryce-Soghigian force-pushed the bsoghigian/disruption-by-method-poc branch from 8e8ac29 to da22d40 Compare June 12, 2024 18:50

Bryce-Soghigian commented Jun 12, 2024

View reviewed changes

pkg/controllers/disruption/validation.go Show resolved Hide resolved

Revert "feat: removing disruption budgets for expirationN"

73ec274

This reverts commit da22d40.

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 12, 2024

njtran reviewed Jun 14, 2024

View reviewed changes

Bryce-Soghigian and others added 4 commits June 13, 2024 18:04

Update pkg/apis/v1beta1/nodepool.go

62c1a76

Co-authored-by: Nick Tran <10810510+njtran@users.noreply.github.com>

Update pkg/controllers/disruption/consolidation_test.go

a0fe098

Co-authored-by: Nick Tran <10810510+njtran@users.noreply.github.com>

feat: only consider underutilized candidates and not empty ones for s…

ee6babd

…ingle and multi node consolidation

chore: generating chart updates for github suggested comments

233c1bb

garvinp-stripe mentioned this pull request Jun 17, 2024

docs: add RFC on making expiration forceful #1303

Merged

njtran reviewed Jun 17, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 17, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 17, 2024

k8s-ci-robot merged commit aa088a6 into kubernetes-sigs:main Jun 17, 2024
13 checks passed

jonathan-innis mentioned this pull request Jul 1, 2024

more customize-able budget schedule aws/karpenter-provider-aws#6429

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: disruption controls by reason #991

feat: disruption controls by reason #991

Bryce-Soghigian commented Feb 6, 2024 •

edited

Loading

k8s-ci-robot commented Feb 6, 2024

njtran left a comment

coveralls commented Feb 11, 2024 •

edited

Loading

charliedmcb left a comment

Bryce-Soghigian commented Feb 18, 2024

njtran left a comment

njtran left a comment

k8s-ci-robot commented Jun 17, 2024

ellistarn commented Jun 17, 2024

feat: disruption controls by reason #991

feat: disruption controls by reason #991

Conversation

Bryce-Soghigian commented Feb 6, 2024 • edited Loading

k8s-ci-robot commented Feb 6, 2024

njtran left a comment

Choose a reason for hiding this comment

coveralls commented Feb 11, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9520161962

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

charliedmcb left a comment

Choose a reason for hiding this comment

Bryce-Soghigian commented Feb 18, 2024

njtran left a comment

Choose a reason for hiding this comment

njtran left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 17, 2024

ellistarn commented Jun 17, 2024

Bryce-Soghigian commented Feb 6, 2024 •

edited

Loading

coveralls commented Feb 11, 2024 •

edited

Loading