KEP-1224: Lending Limit to the cohort #1331

B1F030 · 2023-11-14T05:06:46Z

What type of PR is this?

/kind document

What this PR does / why we need it:

This proposal provides a guaranteed resource quota for user. By setting lending limit, users can have a reservation of resource quota(nominalQuota - lendingLimit) that will never be borrowed by other clusterqueues in the same cohort.

Which issue(s) this PR fixes:

#1224

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

netlify · 2023-11-14T05:06:53Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`b2ae689`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/6569be496497a90008e36e1e

k8s-ci-robot · 2023-11-14T05:06:55Z

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

B1F030 · 2023-11-14T05:07:24Z

/cc @kerthcet
/hold

kerthcet · 2023-11-15T03:49:31Z

/remove-kind api-change
/kind documentation
/ok-to-test

kerthcet · 2023-11-15T03:50:22Z

This is only a proposal, no user-facing change.

kerthcet

First found of review.

keps/1224-lending-limit/README.md

keps/1224-lending-limit/kep.yaml

keps/1224-lending-limit/README.md

keps/1224-lending-limit/kep.yaml

keps/1224-lending-limit/README.md

kerthcet

Generally LGTM.

keps/1224-lending-limit/README.md

kerthcet · 2023-11-20T10:04:39Z

keps/1224-lending-limit/kep.yaml

+
+# The following PRR answers are required at alpha release
+# List the feature gate name and the components for which it must be enabled
+feature-gates:


We add a feature gate here for experiment.

keps/1224-lending-limit/README.md

kerthcet

Plz don't squash or I have no idea where you have changed since last review.

keps/1224-lending-limit/README.md

kerthcet · 2023-11-22T02:45:50Z

cc @alculquicondor @tenzen-y can you take a look then we can push forward.

Co-authored-by: kerthcet <kerthcet@gmail.com> Signed-off-by: B1F030 <646337422@qq.com>

tenzen-y · 2023-11-29T22:10:30Z

I finally came here. So I will review this KEP this week.

B1F030 · 2023-11-30T06:04:10Z

For more information, WIP, first commit

tenzen-y

Generally, lgtm.
@B1F030 Thanks for this effort :)

The design for test cases was especially helpful in reviewing this proposal.

tenzen-y · 2023-11-30T07:51:14Z

keps/1224-lending-limit/README.md

+    - When cq-a's BorrowingLimit unset, cq-a can borrow as much as `cq-b's LendingLimit`.
+    - When cq-a's BorrowingLimit set, cq-a can borrow as much as `min(cq-b's LendingLimit, cq-a's BorrowingLimit)`.
+- In a ClusterQueue with 2 ResourceFlavors a, b:
+  - When rf-b's LendingLimit set, and FlavorFungibility set to `WhenCanBorrow: Borrow`:


I would suggest adding tests for the whenCanBorrow: TryNextFlavor case since we recently found some hidden bugs related to fungibility. So, to avoid such a case, I want to add tests for the whenCanBorrow: TryNextFlavor.

However, we can confirm in the implementation PR if it's case is valuable.

It means that I'm ok without updating here now.

tenzen-y · 2023-11-30T07:57:54Z

keps/1224-lending-limit/README.md

+With both BorrowingLimit and LendingLimit configured, one clusterQueue may not be able to borrow up to the limit just because we reserved the lending limit quota of resource.
+
+To reduce confusion, we will recommend to users to only set borrowingLimit or lendingLimit, but not both, even though both will be supported at the same time.


Indeed, there is such a confusion. However, I don't think that we should recommend users not to use both BorrowingLimit and LendingLimit. Because using both BorrowingLimit and LendingLimit would be helpful for jobs with autoscale / elastic semantics.

I would suggest to remove Line 66.
@alculquicondor @B1F030 WDYT?

I'm ok either way. I remember seeing a similar recommendation in YARN not to use borrowingLimit and weight at the same time.

Another alternative would be some form of troubleshooting, for example:

Why is my CQ not borrowing?
Check borrowingLimit of your CQ and lendingLimit of other CQs in the cohort.

I'm ok either way. I remember seeing a similar recommendation in YARN not to use borrowingLimit and weight at the same time.

Another alternative would be some form of troubleshooting, for example:

Why is my CQ not borrowing?
Check borrowingLimit of your CQ and lendingLimit of other CQs in the cohort.

I prefer to take this case (BorrowingLimit and LendingLimit) as a troubleshooting technique.

Sounds reasonable. I agree to remove Line 66.

This is not about we should only set borrowingLimit or lendingLimit or both, but a practice that if we configured the lendingLimit for demand, then borrowingLimit could be omitted for the maximum resource utilization as there's no borrowingLimit.

Plz update the words.

This is not about we should only set borrowingLimit or lendingLimit or both, but a practice that if we configured the lendingLimit for demand, then borrowingLimit could be omitted for the maximum resource utilization as there's no borrowingLimit.

@kerthcet Does this mean the following situation?

cq-a and cq-b belong to the same cohort.

cq-a:
cpu: 10
borrowingLimit: 5

cq-b:
cpu: 5
lendingLimit: 0

You can set the borrowingLimit or lendingLimit by your demands, but if we set the lendingLimit in cq-b, then cq-a can leave the borrowingLimit unset to consume the resources as much as possible, as

- cq-a: cpu: 10 # borrowingLimit: 5 # no need to set this if you want to maximum the resource utilization # and don't need to worry about use too much resources because other clusterQueues can # constrain this by setting lendingLimit. - cq-b: cpu: 5 lendingLimit: X

Ah, it makes sense. Thanks!

alculquicondor

/approve

I'll leave the LGTM to @tenzen-y

alculquicondor · 2023-11-30T21:01:19Z

keps/1224-lending-limit/README.md

+With both BorrowingLimit and LendingLimit configured, one clusterQueue may not be able to borrow up to the limit just because we reserved the lending limit quota of resource.
+
+To reduce confusion, we will recommend to users to only set borrowingLimit or lendingLimit, but not both, even though both will be supported at the same time.


I'm ok either way. I remember seeing a similar recommendation in YARN not to use borrowingLimit and weight at the same time.

Another alternative would be some form of troubleshooting, for example:

Why is my CQ not borrowing?
Check borrowingLimit of your CQ and lendingLimit of other CQs in the cohort.

keps/1224-lending-limit/README.md

Co-authored-by: kerthcet <kerthcet@gmail.com> Signed-off-by: B1F030 <646337422@qq.com>

tenzen-y

@B1F030 Thanks!

/lgtm
/approve

k8s-ci-robot · 2023-12-04T01:41:35Z

LGTM label has been added.

Git tree hash: b093ca59cb765a191d2f11299b34ca1c8d6b5105

k8s-ci-robot · 2023-12-04T01:41:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, B1F030, tenzen-y

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~~ [alculquicondor,tenzen-y]

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

kerthcet · 2023-12-04T02:16:50Z

/hold cancel
Thansk @B1F030

* KEP-1224: Lending Limit to the cohort Co-authored-by: kerthcet <kerthcet@gmail.com> Signed-off-by: B1F030 <646337422@qq.com> * add test for whenCanBorrow: TryNextFlavor Co-authored-by: kerthcet <kerthcet@gmail.com> Signed-off-by: B1F030 <646337422@qq.com> --------- Signed-off-by: B1F030 <646337422@qq.com> Co-authored-by: kerthcet <kerthcet@gmail.com>

k8s-ci-robot requested review from tenzen-y and trasc November 14, 2023 05:06

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 14, 2023

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 Nov 14, 2023

k8s-ci-robot requested a review from kerthcet November 14, 2023 05:07

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

B1F030 changed the title ~~KEP-1224: Lending Limit to the cohort~~ [WIP] KEP-1224: Lending Limit to the cohort Nov 14, 2023

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 14, 2023

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Nov 15, 2023

kerthcet reviewed Nov 15, 2023

View reviewed changes

kerthcet reviewed Nov 21, 2023

View reviewed changes

B1F030 force-pushed the KEP-1224 branch from 915212b to ecc6d2c Compare November 21, 2023 08:18

B1F030 changed the title ~~[WIP] KEP-1224: Lending Limit to the cohort~~ KEP-1224: Lending Limit to the cohort Nov 21, 2023

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 Nov 21, 2023

kerthcet reviewed Nov 22, 2023

View reviewed changes

keps/1224-lending-limit/README.md Outdated Show resolved Hide resolved

keps/1224-lending-limit/README.md Outdated Show resolved Hide resolved

keps/1224-lending-limit/README.md Show resolved Hide resolved

keps/1224-lending-limit/README.md Outdated Show resolved Hide resolved

B1F030 force-pushed the KEP-1224 branch from bdfed76 to 6535d9d Compare November 24, 2023 06:54

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 Nov 24, 2023

KEP-1224: Lending Limit to the cohort

e28793e

Co-authored-by: kerthcet <kerthcet@gmail.com> Signed-off-by: B1F030 <646337422@qq.com>

B1F030 force-pushed the KEP-1224 branch from 6535d9d to e28793e Compare November 24, 2023 06:58

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

tenzen-y reviewed Nov 30, 2023

View reviewed changes

B1F030 mentioned this pull request Nov 30, 2023

Feature LendingLimit(KEP-1224) #1385

Merged

alculquicondor reviewed Nov 30, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 30, 2023

B1F030 and others added 2 commits December 1, 2023 19:05

Merge branch 'kubernetes-sigs:main' into KEP-1224

14a3f57

add test for whenCanBorrow: TryNextFlavor

b2ae689

Co-authored-by: kerthcet <kerthcet@gmail.com> Signed-off-by: B1F030 <646337422@qq.com>

tenzen-y reviewed Dec 4, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 4, 2023

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

k8s-ci-robot merged commit 8458264 into kubernetes-sigs:main Dec 4, 2023
14 checks passed

k8s-ci-robot added this to the v0.6 milestone Dec 4, 2023

B1F030 deleted the KEP-1224 branch December 8, 2023 07:30

B1F030 mentioned this pull request Feb 4, 2024

REQUEST: New membership for B1F030 kubernetes/org#4732

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-1224: Lending Limit to the cohort #1331

KEP-1224: Lending Limit to the cohort #1331

B1F030 commented Nov 14, 2023 •

edited

Loading

netlify bot commented Nov 14, 2023 •

edited

Loading

k8s-ci-robot commented Nov 14, 2023

B1F030 commented Nov 14, 2023

kerthcet commented Nov 15, 2023

kerthcet commented Nov 15, 2023

kerthcet left a comment

kerthcet left a comment

kerthcet Nov 20, 2023

kerthcet left a comment

kerthcet commented Nov 22, 2023

tenzen-y commented Nov 29, 2023

B1F030 commented Nov 30, 2023

tenzen-y left a comment

tenzen-y Nov 30, 2023 •

edited

Loading

tenzen-y Nov 30, 2023

alculquicondor Nov 30, 2023

tenzen-y Nov 30, 2023

B1F030 Dec 1, 2023

kerthcet Dec 1, 2023

kerthcet Dec 1, 2023

tenzen-y Dec 1, 2023

kerthcet Dec 1, 2023

tenzen-y Dec 4, 2023

alculquicondor left a comment

alculquicondor Nov 30, 2023

tenzen-y left a comment

k8s-ci-robot commented Dec 4, 2023

k8s-ci-robot commented Dec 4, 2023

kerthcet commented Dec 4, 2023

		With both BorrowingLimit and LendingLimit configured, one clusterQueue may not be able to borrow up to the limit just because we reserved the lending limit quota of resource.

		To reduce confusion, we will recommend to users to only set borrowingLimit or lendingLimit, but not both, even though both will be supported at the same time.

KEP-1224: Lending Limit to the cohort #1331

KEP-1224: Lending Limit to the cohort #1331

Conversation

B1F030 commented Nov 14, 2023 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

netlify bot commented Nov 14, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

k8s-ci-robot commented Nov 14, 2023

B1F030 commented Nov 14, 2023

kerthcet commented Nov 15, 2023

kerthcet commented Nov 15, 2023

kerthcet left a comment

Choose a reason for hiding this comment

kerthcet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kerthcet left a comment

Choose a reason for hiding this comment

kerthcet commented Nov 22, 2023

tenzen-y commented Nov 29, 2023

B1F030 commented Nov 30, 2023

tenzen-y left a comment

Choose a reason for hiding this comment

tenzen-y Nov 30, 2023 • edited Loading

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

Choose a reason for hiding this comment

alculquicondor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenzen-y left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 4, 2023

k8s-ci-robot commented Dec 4, 2023

kerthcet commented Dec 4, 2023

B1F030 commented Nov 14, 2023 •

edited

Loading

netlify bot commented Nov 14, 2023 •

edited

Loading

tenzen-y Nov 30, 2023 •

edited

Loading