-
Notifications
You must be signed in to change notification settings - Fork 230
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature LendingLimit(KEP-1224) #1385
Conversation
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
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 Once the patch is verified, the new status will be reflected by the 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. |
/ok-to-test |
/release-note-edit |
Just to confirm, here I have 2 cqs and 2 rfs.
cq-b.yaml:
Then a workload with And in the second case, set In conclusion, I just want to make sure that: Borrow(or Lend) can only happen in the same flavor(like, horizontal?), right?
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:
|
@tenzen-y Hi! Sorry to bother, but am I understanding correctly? |
@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.
If the workload doesn't have a nodeSelector for the arm flavor, it's correct.
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 |
Thanks a lot! |
Now the feature is basically finished, feel free to review. |
|
I'll wait for an LGTM from @kerthcet before giving another pass. |
@B1F030: The
Use In response to this:
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM.
/assign @alculquicondor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation still needs work, and since we plan to cut a release this Friday, I think it's safer to leave it out.
for resource, value := range qResources { | ||
available := cohortResources[resource] - cohortUsage[resource] | ||
available := c.RequestableCohortQuota(flavor, resource) - c.UsedCohortQuota(flavor, resource) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theRequestableResources
as defined in 1 (same as your approach).- When evaluating a workload admission, we calculate:
excess := workload.request-guaranteed
. The workload fits ifcohort.Usage+excess <= cohort.RequestableResources
.
Both implementations are equivalent, but I think this is easier to document and follow along.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@tenzen-y how do you feel about merging this PR this week? |
Please squash. |
…for user. Co-authored-by: kerthcet <kerthcet@gmail.com> Signed-off-by: B1F030 <b1fzhang@gmail.com>
c5d0b61
to
8664c53
Compare
/lgtm |
LGTM label has been added. Git tree hash: 46c340e4c5aa85efea2ec1508a9f355bfc06d225
|
[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:
Approvers can indicate their approval by writing |
/release-note-edit
|
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?