feat: Add support for minValues in requirement for scheduling, consolidation layer and corresponding API changes. #963

nikmohan123 · 2024-01-25T21:43:22Z

Description
Add support for minValues in requirement for scheduling and consolidation layer. This allows users to enforce higher flexibility constraints on Karpenter so that Karpenter adheres to spot best practices. By enforcing minValues on Karpenter NodePools with node.kubernetes.io/instance-type or karpenter.k8s.aws/instance-family , customers can ensure that launches will have a better chance of providing a highly available instance type by giving PCO more options.

How was this change tested?
make presubmit
Will add functional tests once the API structure has been finalized as the minValues is extracted from the requirement of the API.

Also manually tested by injecting the minValues into requirement and tested the scheduling and consolidation path.

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

… NodePool.

…ues to test.

…bility based on minValues in requirement.

k8s-ci-robot · 2024-01-25T21:43:31Z

Hi @nikmohan123. 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.

coveralls · 2024-01-25T21:57:20Z

Pull Request Test Coverage Report for Build 7995389241

Details

-11 of 252 (95.63%) changed or added relevant lines in 17 files are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.4%) to 81.047%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/controllers/disruption/consolidation.go	29	30	96.67%
pkg/test/nodepool.go	1	4	25.0%
pkg/apis/v1beta1/zz_generated.deepcopy.go	9	16	56.25%

Files with Coverage Reduction	New Missed Lines	%
pkg/test/expectations/expectations.go	2	95.96%

Totals
Change from base Build 7977335620:	0.4%
Covered Lines:	8202
Relevant Lines:	10120

💛 - Coveralls

pkg/controllers/provisioning/scheduling/nodeclaim.go

pkg/controllers/disruption/consolidation.go

pkg/controllers/disruption/helpers.go

jonathan-innis · 2024-01-30T04:49:44Z

Consider using the scheduling_benchmark_test.go to see what the performance impact is on this change. Would be worth expanding that suite to add some checks for what happens when you pass requirements with minValues in

jonathan-innis

Nice work 🎉

pkg/scheduling/requirement.go

pkg/controllers/disruption/consolidation.go

pkg/controllers/provisioning/scheduling/nodeclaim.go

pkg/controllers/disruption/helpers.go

pkg/controllers/disruption/multinodeconsolidation.go

pkg/controllers/disruption/consolidation.go

jonathan-innis

Really, really nice work. These changes are absolutely non-trivial

/lgtm
/approve

jonathan-innis · 2024-02-15T23:58:39Z

/hold Wait for other PRs that are being staged to be approved

… the NodePool requirement. (#1) * Initial commit for introducing the API changes and modified tests to incorporate the API changes. * Wire the minValues from NodePool spec requirement into scheduling requirement. * Add functional tests and add validations. * Add more documentation. * Take in latest changes from main. * Bump the version of CRD. * Add more documentation. * Add additional logic to fail close. * Add minValues to the error string of requirement. * Restrict the minValues to 50. * Add tests for consolidation related to minValues in requirement. * Addressed few comments. * Added topology test. * Addressed rest of the comments. * Addressed more comments. * Addressed few more comments. * Fix error message order.

jonathan-innis · 2024-02-21T00:45:49Z

/ok-to-test

jonathan-innis

/lgtm
/approve

Really really awesome work here! 🎉

jonathan-innis

/lgtm
/approve

jonathan-innis · 2024-02-21T05:07:10Z

/remove-hold

Nice work! Ship it!

nikmohan123 · 2024-02-21T20:50:17Z

Created an issue to fix the corner case of the topology spread not behaving as expected with minValues : #1034.

jonathan-innis

/lgtm

k8s-ci-robot · 2024-02-21T21:29:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jonathan-innis, nikmohan123

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~~ [jonathan-innis]

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

nikmohan123 added 8 commits January 23, 2024 11:41

Generic implementation to consider min requirements for launch and in…

bafda0e

… NodePool.

Take the requirements from nodepool to check for minValues.

92047ad

Removed unwanted vairables.

0181e95

Initialize map if its values are empty and add dummy values to MinVal…

d7b11a5

…ues to test.

Add more documentation.

b3ac8da

Add more documentation and remove the testing minValues.

e5ca121

Optimize the code and add more documentation.

f7fe4c5

Add the logic in consolidation layer to pick the MinInstanceTypeFlexi…

4ad8531

…bility based on minValues in requirement.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 25, 2024

k8s-ci-robot requested review from engedaam and jmdeal January 25, 2024 21:43

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 Jan 25, 2024

Merge branch 'kubernetes-sigs:main' into cps_final

154ce08

jonathan-innis reviewed Jan 30, 2024

View reviewed changes

nikmohan123 and others added 2 commits January 30, 2024 14:06

Merge branch 'kubernetes-sigs:main' into cps_final

9939665

Addressed comments.

f50c6f0

nikmohan123 requested a review from jonathan-innis January 31, 2024 01:41

nikmohan123 and others added 3 commits January 31, 2024 13:44

Merge branch 'kubernetes-sigs:main' into cps_final

c0b282c

Merge branch 'kubernetes-sigs:main' into cps_final

53bedd7

Fix code after merging from main.

b472084

nikmohan123 mentioned this pull request Feb 9, 2024

feat: Add a new field called minValues that introduces flexibility to the NodePool requirement. nikmohan123/karpenter#1

Merged

jonathan-innis reviewed Feb 12, 2024

View reviewed changes

nikmohan123 added 2 commits February 12, 2024 21:18

Address comments.

01ee207

Add more documentation.

2918f84

nikmohan123 requested a review from jonathan-innis February 13, 2024 18:47

Update documentation.

0fd3e0a

jonathan-innis approved these changes Feb 15, 2024

View reviewed changes

k8s-ci-robot assigned jonathan-innis Feb 15, 2024

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 15, 2024

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 15, 2024

Merge branch 'kubernetes-sigs:main' into cps_final

0671f75

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2024

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 21, 2024

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 21, 2024

nikmohan123 changed the title ~~feat: Add support for minValues in requirement for scheduling and consolidation layer.~~ feat: Add support for minValues in requirement for scheduling, consolidation layer and corresponding API changes. Feb 21, 2024

jonathan-innis approved these changes Feb 21, 2024

View reviewed changes

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

Reset the maxInstanceTypes so as to not impact the tests.

49482e8

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2024

jonathan-innis approved these changes Feb 21, 2024

View reviewed changes

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

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 21, 2024

Remove topology test.

25d9078

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2024

jonathan-innis approved these changes Feb 21, 2024

View reviewed changes

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

k8s-ci-robot merged commit 0fea7ce into kubernetes-sigs:main Feb 21, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for minValues in requirement for scheduling, consolidation layer and corresponding API changes. #963

feat: Add support for minValues in requirement for scheduling, consolidation layer and corresponding API changes. #963

nikmohan123 commented Jan 25, 2024 •

edited

Loading

k8s-ci-robot commented Jan 25, 2024

coveralls commented Jan 25, 2024 •

edited

Loading

jonathan-innis commented Jan 30, 2024

jonathan-innis left a comment •

edited

Loading

jonathan-innis left a comment

jonathan-innis commented Feb 15, 2024

jonathan-innis commented Feb 21, 2024

jonathan-innis left a comment

jonathan-innis left a comment

jonathan-innis commented Feb 21, 2024

nikmohan123 commented Feb 21, 2024

jonathan-innis left a comment

k8s-ci-robot commented Feb 21, 2024

feat: Add support for minValues in requirement for scheduling, consolidation layer and corresponding API changes. #963

feat: Add support for minValues in requirement for scheduling, consolidation layer and corresponding API changes. #963

Conversation

nikmohan123 commented Jan 25, 2024 • edited Loading

k8s-ci-robot commented Jan 25, 2024

coveralls commented Jan 25, 2024 • edited Loading

Pull Request Test Coverage Report for Build 7995389241

Details

💛 - Coveralls

jonathan-innis commented Jan 30, 2024

jonathan-innis left a comment • edited Loading

Choose a reason for hiding this comment

jonathan-innis left a comment

Choose a reason for hiding this comment

jonathan-innis commented Feb 15, 2024

jonathan-innis commented Feb 21, 2024

jonathan-innis left a comment

Choose a reason for hiding this comment

jonathan-innis left a comment

Choose a reason for hiding this comment

jonathan-innis commented Feb 21, 2024

nikmohan123 commented Feb 21, 2024

jonathan-innis left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 21, 2024

nikmohan123 commented Jan 25, 2024 •

edited

Loading

coveralls commented Jan 25, 2024 •

edited

Loading

jonathan-innis left a comment •

edited

Loading