Feature LendingLimit(KEP-1224) #1385

B1F030 · 2023-11-30T08:38:33Z

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

Feature LendingLimit provides a guaranteed resource quota for user.

Which issue(s) this PR fixes:

#1224
KEP-1224

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Add a lendingLimit field in ClusterQueue's quotas, to allow restricting how much of the unused resources by the ClusterQueue can be borrowed by other ClusterQueues in the cohort.
In other words, this allows a quota equal to `nominal-lendingLimit` to be exclusively used by the ClusterQueue.

netlify · 2023-11-30T08:38:41Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`8664c53`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/65c5dac396d6180008b8bf46

k8s-ci-robot · 2023-11-30T08:38:43Z

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-30T08:39:46Z

/cc @kerthcet
/cc @tenzen-y

kerthcet · 2023-11-30T10:20:07Z

/ok-to-test

kerthcet · 2023-12-04T02:41:03Z

/release-note-edit Support reserving guaranteed quota when fair-sharing among clusterQueues

B1F030 · 2023-12-04T10:53:12Z

Just to confirm, here I have 2 cqs and 2 rfs.
In the first case, set whenCanBorrow to Borrow(default)
cq-a.yaml:

apiVersion: kueue.x-k8s.io/v1beta1
kind: ClusterQueue
metadata:
  name: "cq-a"
spec:
  namespaceSelector: {} # match all.
  cohort: "cohort-ab"
  resourceGroups:
  - coveredResources: ["cpu"]
    flavors:
    - name: "arm"
      resources:
      - name: "cpu"
        nominalQuota: 500m
    - name: "x86"
      resources:
      - name: "cpu"
        nominalQuota: 600m
  flavorFungibility:
    whenCanBorrow: Borrow

cq-b.yaml:

apiVersion: kueue.x-k8s.io/v1beta1
kind: ClusterQueue
metadata:
  name: "cq-b"
spec:
  namespaceSelector: {} # match all.
  cohort: "cohort-ab"
  resourceGroups:
  - coveredResources: ["cpu"]
    flavors:
    - name: "arm"
      resources:
      - name: "cpu"
        nominalQuota: 300m
        lendingLimit: 100m
    - name: "x86"
      resources:
      - name: "cpu"
        nominalQuota: 200m
        lendingLimit: 0m

Then a workload with 600m cpu request to lq-a(belongs to cq-a) with the flavor arm(or x86) should be admitted and assigned to the flavor it requested, but 700m shouldn't, right?

And in the second case, set whenCanBorrow to TryNextFlavor.
A workload with 600m cpu request to lq-a with the flavor arm should be admitted and assigned to the flavor x86.

In conclusion, I just want to make sure that: Borrow(or Lend) can only happen in the same flavor(like, horizontal?), right?
If there's only 1 cq and 2 rf:

  - coveredResources: ["cpu"]
    flavors:
    - name: "arm"
      resources:
      - name: "cpu"
        nominalQuota: 500m
    - name: "x86"
      resources:
      - name: "cpu"
        nominalQuota: 600m
        lendingLimit: 100m

Then Borrow(or Lend) should not work in different flavor(like, vertical?), right?

If that so, I'm going to modify KEP-1224 in Integration tests like this:

In a cohort with 2 ClusterQueues cq-a, cq-b and 2 ResourceFlavors rf-a, rf-b:
- In cq-b, when rf-a's LendingLimit set, and cq-a's FlavorFungibility set to whenCanBorrow: Borrow:
  - In cq-a, when rf-a's BorrowingLimit unset, cq-a can borrow as much as rf-a's LendingLimit.
  - In cq-a, when rf-a's BorrowingLimit set, cq-a can borrow as much as min(rf-a's LendingLimit, rf-a's BorrowingLimit).
- In cq-b, when rf-b's LendingLimit set, and cq-a's FlavorFungibility set to whenCanBorrow: TryNextFlavor:
  - In cq-a, when rf-b's BorrowingLimit unset, cq-a can borrow as much as rf-b's LendingLimit.
  - In cq-a, when rf-b's BorrowingLimit set, cq-a can borrow as much as min(rf-b's LendingLimit, rf-b's BorrowingLimit).

B1F030 · 2023-12-04T10:57:18Z

In conclusion, I just want to make sure that: Borrow(or Lend) can only happen in the same flavor(like, horizontal?), right?
Borrow(or Lend) should not work in different flavor(like, vertical?), right?

@tenzen-y Hi! Sorry to bother, but am I understanding correctly?

tenzen-y · 2023-12-05T02:49:47Z

Then a workload with 600m cpu request to lq-a(belongs to cq-a) with the flavor arm(or x86) should be admitted and assigned to the flavor it requested, but 700m shouldn't, right?

@B1F030 If there are no admitted workloads, the workload should be assigned to arm flavor since the flavors are evaluated in order. Also, as you say, workloads with 700m CPU requests wouldn't admitted by any clusterQueue.

And in the second case, set whenCanBorrow to TryNextFlavor.
A workload with 600m cpu request to lq-a with the flavor arm should be admitted and assigned to the flavor x86.

If the workload doesn't have a nodeSelector for the arm flavor, it's correct.

In conclusion, I just want to make sure that: Borrow(or Lend) can only happen in the same flavor(like, horizontal?), right?

Yes, the Borrow will affect only the same flavors of each resource, like CPU. A workload can have only a single flavor of each resource. In other words, a workload can not have multiple flavors against a single resource, although a workload can have multiple flavors with multiple resources.

It means cpu=flavor-a and memory=flavor-b will happen, although cpu=flavor-a and flavor-b won't happen.

B1F030 · 2023-12-05T04:25:38Z

Yes, the Borrow will affect only the same flavors of each resource, like CPU. A workload can have only a single flavor of each resource. In other words, a workload can not have multiple flavors against a single resource, although a workload can have multiple flavors with multiple resources.

Thanks a lot!

B1F030 · 2023-12-05T11:55:05Z

Now the feature is basically finished, feel free to review.
I will add some tests next.

B1F030 · 2023-12-06T09:59:07Z

pkg/cache/: 2023-12-6 - 89.0%
pkg/metrics/: 2023-12-6 - 55.6%
pkg/scheduler/: 2023-12-6 - 81.2%

pkg/cache/snapshot.go

pkg/cache/clusterqueue.go

pkg/cache/clusterqueue_test.go

pkg/cache/snapshot_test.go

pkg/cache/clusterqueue_test.go

pkg/metrics/metrics.go

pkg/scheduler/scheduler_test.go

test/integration/scheduler/scheduler_test.go

alculquicondor · 2024-01-25T15:40:12Z

I'll wait for an LGTM from @kerthcet before giving another pass.

k8s-ci-robot · 2024-01-29T09:38:13Z

@B1F030: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

/test pull-kueue-build-image-main
/test pull-kueue-test-e2e-main-1-26
/test pull-kueue-test-e2e-main-1-27
/test pull-kueue-test-e2e-main-1-28
/test pull-kueue-test-e2e-main-1-29
/test pull-kueue-test-integration-main
/test pull-kueue-test-unit-main
/test pull-kueue-verify-main

Use /test all to run all jobs.

In response to this:

/retest pull-kueue-test-e2e-main-1-29

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.

kerthcet

Generally LGTM.

pkg/cache/cache_test.go

pkg/cache/clusterqueue.go

test/integration/scheduler/preemption_test.go

kerthcet · 2024-01-30T09:37:54Z

/assign @alculquicondor

alculquicondor

This implementation still needs work, and since we plan to cut a release this Friday, I think it's safer to leave it out.

apis/kueue/v1beta1/clusterqueue_types.go

keps/1224-lending-limit/README.md

alculquicondor · 2024-02-06T20:27:48Z

pkg/cache/clusterqueue.go

 			for resource, value := range qResources {
-				available := cohortResources[resource] - cohortUsage[resource]
+				available := c.RequestableCohortQuota(flavor, resource) - c.UsedCohortQuota(flavor, resource)


I would prefer we call the feature gate here, preserving the old code.

And these functions really work together, so it might be better to have a single function called c.AvailableQuotaWithCohortBorrowing(flavor, resource)

Also, an alternative implementation is as follows:

cohort.RequestableResources only contains the sum of lendingLimits (same as your approach).

cohort.Usage only contains the usage within the RequestableResources as defined in 1 (same as your approach).

When evaluating a workload admission, we calculate: excess := workload.request-guaranteed. The workload fits if cohort.Usage+excess <= cohort.RequestableResources.

Both implementations are equivalent, but I think this is easier to document and follow along.

And these functions really work together, so it might be better to have a single function called c.AvailableQuotaWithCohortBorrowing(flavor, resource)

RequestableCohortQuota could not be moved into a single function: here cohortAvailable needs to be calculated individually. And UsedCohortQuota here.

I would prefer we call the feature gate here, preserving the old code.

We can change this part like this:

for flavor, qResources := range q { if cohortResources, flavorFound := c.RequestableResources[flavor]; flavorFound { cohortUsage := c.Usage[flavor] for resource, value := range qResources { available := cohortResources[resource] - cohortUsage[resource] if features.Enabled(features.LendingLimit) { available = c.RequestableCohortQuota(flavor, resource) - c.UsedCohortQuota(flavor, resource) }

But since we already have featureGates inside of c.RequestableCohortQuota and c.UsedCohortQuota(RequestableCohortQuota, guaranteedQuota, UsedCohortQuota), is it still necessary to add featureGates here?

But since we already have featureGates inside of c.RequestableCohortQuota and c.UsedCohortQuota(RequestableCohortQuota, guaranteedQuota, UsedCohortQuota), is it still necessary to add featureGates here?

The fact that the usage of the feature gate is scattered among multiple functions makes it hard to visually verify if the old behavior is preserved. It's generally a good practice to use feature gates in a coarse manner.

You could actually remove the feature gate checks from the new functions and check the feature gate in the flavorassigner code as well.

pkg/cache/clusterqueue.go

pkg/cache/snapshot.go

pkg/scheduler/flavorassigner/flavorassigner.go

pkg/cache/snapshot.go

pkg/scheduler/flavorassigner/flavorassigner_test.go

test/integration/scheduler/scheduler_test.go

test/integration/scheduler/preemption_test.go

test/integration/scheduler/scheduler_test.go

test/integration/scheduler/preemption_test.go

alculquicondor · 2024-02-07T16:33:20Z

@tenzen-y how do you feel about merging this PR this week?

alculquicondor · 2024-02-08T19:51:25Z

Please squash.

…for user. Co-authored-by: kerthcet <kerthcet@gmail.com> Signed-off-by: B1F030 <b1fzhang@gmail.com>

alculquicondor · 2024-02-09T14:14:20Z

/lgtm
/approve

k8s-ci-robot · 2024-02-09T14:14:26Z

LGTM label has been added.

Git tree hash: 46c340e4c5aa85efea2ec1508a9f355bfc06d225

k8s-ci-robot · 2024-02-09T14:14:29Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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]

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

alculquicondor · 2024-02-13T19:51:13Z

/release-note-edit

Add a lendingLimit field in ClusterQueue's quotas, to allow restricting how much of the unused resources by the ClusterQueue can be borrowed by other ClusterQueues in the cohort.
In other words, this allows a quota equal to `nominal-lendingLimit` to be exclusively used by the ClusterQueue.

pkg/cache/snapshot.go

…for user. (kubernetes-sigs#1385) Signed-off-by: B1F030 <b1fzhang@gmail.com> Co-authored-by: kerthcet <kerthcet@gmail.com>

k8s-ci-robot requested review from mimowo and trasc November 30, 2023 08:38

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

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 30, 2023

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 30, 2023

k8s-ci-robot requested review from kerthcet and tenzen-y November 30, 2023 08:39

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

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 Dec 5, 2023

B1F030 changed the title ~~[WIP]Feature LendingLimit(KEP-1224)~~ Feature LendingLimit(KEP-1224) Dec 5, 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 Dec 5, 2023

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 6, 2023

kerthcet reviewed Jan 24, 2024

View reviewed changes

kerthcet reviewed Jan 25, 2024

View reviewed changes

pkg/scheduler/scheduler_test.go Outdated Show resolved Hide resolved

pkg/scheduler/scheduler_test.go Outdated Show resolved Hide resolved

kerthcet reviewed Jan 25, 2024

View reviewed changes

kerthcet reviewed Jan 29, 2024

View reviewed changes

k8s-ci-robot assigned alculquicondor Jan 30, 2024

B1F030 mentioned this pull request Feb 4, 2024

REQUEST: New membership for B1F030 kubernetes/org#4732

Closed

9 tasks

alculquicondor reviewed Feb 6, 2024

View reviewed changes

alculquicondor reviewed Feb 7, 2024

View reviewed changes

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 7, 2024

[KEP-1224] Feature LendingLimit provides a guaranteed resource quota …

8664c53

…for user. Co-authored-by: kerthcet <kerthcet@gmail.com> Signed-off-by: B1F030 <b1fzhang@gmail.com>

B1F030 force-pushed the feature-lendinglimit branch from c5d0b61 to 8664c53 Compare February 9, 2024 07:56

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

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

k8s-ci-robot merged commit 1c45fcb into kubernetes-sigs:main Feb 9, 2024
14 checks passed

k8s-ci-robot added this to the v0.6 milestone Feb 9, 2024

B1F030 deleted the feature-lendinglimit branch February 18, 2024 01:24

alculquicondor reviewed Feb 20, 2024

View reviewed changes

pkg/cache/snapshot.go Show resolved Hide resolved

kerthcet mentioned this pull request Feb 26, 2024

Fix incorrect snapshot usage when lendingLimit enabled #1770

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature LendingLimit(KEP-1224) #1385

Feature LendingLimit(KEP-1224) #1385

B1F030 commented Nov 30, 2023 •

edited by k8s-ci-robot

Loading

netlify bot commented Nov 30, 2023 •

edited

Loading

k8s-ci-robot commented Nov 30, 2023

B1F030 commented Nov 30, 2023

kerthcet commented Nov 30, 2023

kerthcet commented Dec 4, 2023

B1F030 commented Dec 4, 2023

B1F030 commented Dec 4, 2023

tenzen-y commented Dec 5, 2023

B1F030 commented Dec 5, 2023

B1F030 commented Dec 5, 2023

B1F030 commented Dec 6, 2023

alculquicondor commented Jan 25, 2024

k8s-ci-robot commented Jan 29, 2024

kerthcet left a comment

kerthcet commented Jan 30, 2024

alculquicondor left a comment

alculquicondor Feb 6, 2024

alculquicondor Feb 6, 2024

B1F030 Feb 7, 2024

B1F030 Feb 8, 2024

alculquicondor Feb 8, 2024

alculquicondor commented Feb 7, 2024

alculquicondor commented Feb 8, 2024

alculquicondor commented Feb 9, 2024

k8s-ci-robot commented Feb 9, 2024

k8s-ci-robot commented Feb 9, 2024

alculquicondor commented Feb 13, 2024

Feature LendingLimit(KEP-1224) #1385

Feature LendingLimit(KEP-1224) #1385

Conversation

B1F030 commented Nov 30, 2023 • edited by k8s-ci-robot 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 30, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

k8s-ci-robot commented Nov 30, 2023

B1F030 commented Nov 30, 2023

kerthcet commented Nov 30, 2023

kerthcet commented Dec 4, 2023

B1F030 commented Dec 4, 2023

B1F030 commented Dec 4, 2023

tenzen-y commented Dec 5, 2023

B1F030 commented Dec 5, 2023

B1F030 commented Dec 5, 2023

B1F030 commented Dec 6, 2023

alculquicondor commented Jan 25, 2024

k8s-ci-robot commented Jan 29, 2024

kerthcet left a comment

Choose a reason for hiding this comment

kerthcet commented Jan 30, 2024

alculquicondor left a comment

Choose a reason for hiding this comment

alculquicondor Feb 6, 2024

Choose a reason for hiding this comment

alculquicondor Feb 6, 2024

Choose a reason for hiding this comment

B1F030 Feb 7, 2024

Choose a reason for hiding this comment

B1F030 Feb 8, 2024

Choose a reason for hiding this comment

alculquicondor Feb 8, 2024

Choose a reason for hiding this comment

alculquicondor commented Feb 7, 2024

alculquicondor commented Feb 8, 2024

alculquicondor commented Feb 9, 2024

k8s-ci-robot commented Feb 9, 2024

k8s-ci-robot commented Feb 9, 2024

alculquicondor commented Feb 13, 2024

B1F030 commented Nov 30, 2023 •

edited by k8s-ci-robot

Loading

netlify bot commented Nov 30, 2023 •

edited

Loading