[KEP] Update KEP of the flavor fungibility feature#6133
Conversation
|
Skipping CI for Draft Pull Request. |
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
| After we try to schedule a podset in a resource flavor, we decide whether to traverse to the next flavor base on the `flavorFungibility`. If the assignment mode is `NoFit`, we will always try the next flavor until the last one. When the assignment mode is `Preempt`, we can return the current assignment if `WhenCanPreempt` is `Preempt`. Otherwise if the assignment mode is `Fit`, we try the next flavor only when we need borrowing in the current flavor and `WhenCanBorrow` is `TryNextFlavor`. | ||
| - (`Fit`, `NoBorrow`) is the most prefered | ||
| - (`Fit`, `Borrow`) is prefered over (`Preempt`, `NoBorrow`) if `WhenCanPreemptAndBorrow = AvoidPreemption` | ||
| - (`Preempt`, `NoBorrow`) is prefered over (`Fit`, `Borrow`) if `WhenCanPreemptAndBorrow = AvoidBorrowing` |
There was a problem hiding this comment.
(
Preempt,NoBorrow) is prefered over (Fit,Borrow) ifWhenCanPreemptAndBorrow = AvoidBorrowing
What does this mean? Does the whenCanPreemptAndBorrow: AvoidBorrowing respect Preempt and NoBorrow over the Fit? Or does this strategy avoid only Fit with borrow?
IIUC, Fit contains 2 patterns: (1 the workload fits nominalQuota (2 the workload fits borrowed quotas.
There was a problem hiding this comment.
Right, so in my notation we have 4 interesting cases (Fit, NoBorrow), (Fit, Borrow), (Preempt, NoBorrow), (Preempt, Borrow).
WhenCanPreemptAndBorrow = PreferBorrowing(current) the order is (Fit,NoBorrow), (Fit,Borrow), (Preempt,NoBorrow), (Preempt,Borrow)WhenCanPreemptAndBorrow = PreferPreemption(new) the order is (Fit,NoBorrow), (Preempt,NoBorrow), (Fit,Borrow), (Preempt,Borrow)
| TryNextFlavor FlavorFungibilityPolicy = "TryNextFlavor" | ||
| ) | ||
|
|
||
| type FlavorSelectionStrategy string |
There was a problem hiding this comment.
Do you have any reason to choose strategy suffix?
Could we select FlavorSelectionPolicy to align with FlavorFungibilityPolicy?
There was a problem hiding this comment.
Oh, of course. Changed to FlavorSelectionStrategy
| AvoidPreemption FlavorSelectionStrategy = "AvoidPreemption" | ||
| AvoidBorrowing FlavorSelectionStrategy = "AvoidBorrowing" |
There was a problem hiding this comment.
As also asked in the PR impl: #6132 (comment), I'm not convinced to the naming, the "Avoid" seems stronger than prefer. So, it is likely users would interpret like "do nothing" when all flavors are (Preempt, NoBorrow).
So I would be leaning to "PreferPreemption", "PreferBorrowing", or an enum field "preference: Preemption/Borrowing", wdyt?
There was a problem hiding this comment.
I also think that new strategies should not be "Avoid" since it would be better to mention what will happen rather than what will NOT happen.
There was a problem hiding this comment.
OK. So I guess more people prefer prefer :) Updated.
| // other workloads. | ||
| // - `PreferPreemption`: prefer to allocate in a flavor that uses only the nominal quota | ||
| // even if it means preempting other workloads. | ||
| // +kubebuilder:validation:Enum={PreferBorrowing,PreferPreemption} |
There was a problem hiding this comment.
Why not just Borrow,Preempt? Since the field is already whenCanPreemptAndBorrow it is already clear we have the two options.
| // even if it means preempting other workloads. | ||
| // +kubebuilder:validation:Enum={PreferBorrowing,PreferPreemption} | ||
| // +kubebuilder:default="PreferBorrowing" | ||
| WhenCanPreemptAndBorrow FlavorSelectionPolicy `json:"preferredStrategy,omitempty"` |
There was a problem hiding this comment.
Actually, I find this API still confusing a lot. Its form is similar to whenCanPreempt and whenCanBorrow, but the semantics is very differenent. whenCanPreempt and whenCanBorrow indicate actually: stop looking for new flavors. They mean when flavor which preempt -> do something, when flavor which can borrow -> do something.
The naming convention suggests the new field has the same nature, when a flavor which can preempt and borrow is encountered -> do something.
However, the nature of the field is very different, it means "once the set of flavors is determined, choose the one fitting the preference: Borrow or Preempt".
Let me propose something.
There was a problem hiding this comment.
The API I'm thinking towards is
flavorFungability:
whenCanBorrow: Stop / TryNextFlavor
whenCanPreempt: Stop / TryNextFlavor
preferenceOrder: ["Preempt", "PreemptWhileBorrowing", "Borrow"]Now, the field is preferenceOrder because some users may want to position "PreemptWhileBorrowing" differently. So, by saying just "Preempt / Borrow" we are too constraining for the future.
I think what happens here is some implicit ordering currently "Borrow", "Preempt", "PreemptWhileBorrowing".
As mentioned in https://github.com/kubernetes-sigs/kueue/pull/6133/files#r2225166242 the story is really for a user which is using FairSharing, and we got feedback due to the very nature of fair sharing they want "Preempt", "PreemptWhileBorrowing", "Borrow".
However, I can imagine some users will want ["Preempt", "PreemptWhileBorrowing", "Borrow"] (say outside of FairSharing), so long term something like preferenceOrder might be helpful.
So, I propose to do not introduce new API for 0.13, just a beta level feature gate (for now), FairSharingWithFlavorFungabilityPrefersPreemption. I think this is a sensible default when FairSharing is used for the reasons explained in the Story. The feature gate will give us bailout option if some users are unhappy with the change (I think this is unlikely), yet will give us time to think of better API. I'm also ok to make the feature gate alpha for now.
There was a problem hiding this comment.
Ok, let me expand on the dependency on FairSharing as it was, rightfully, questioned when discussing with @pajakd @gabesaba .
What we want with this feature is to avoid borrowing when reclaim is enabled. Because if a workload borrows it is likely a preemption target of the workloads in CQs which are entitles to reclaim.
Initially I thought, and was mistaken, that FairSharing enables all CQs in the system to reclaim (which is the common configuration), but in fact a CQ may still disabled preemption.reclaimWithinCohort.
Let me rework the feature gate idea then. I think what makes sense it still to use implicit defaults until we have a better idea of what is needed.
So my proposal is:
whenCanBorrow: Borrow&whenCanPreempt: TryNextFlavor-> implicit default to for preference to BorrowwhenCanBorrow: TryNextFlavor&whenCanPreempt: Preempt-> implicit default to for preference to PreemptwhenCanBorrow: Borrow&whenCanPreempt: Preempt-> implicit default does not matter because we just choose firstwhenCanBorrow: TryNextFlavor&whenCanPreempt: TryNextFlavor-> make implicit default depending on the enablement of FairSharing. This is enough to capture the idea of FairSharing to mix borrowing with reclaim, yet will only be scoped to CQs which havewhenCanBorrow: TryNextFlavor&whenCanPreempt: TryNextFlavor.
EDIT: The intention for tying the default to FairSharing (in 4.) is the guesstimate of user preferences for "Preempt". The reason is that FairSharing with reclamation disabled is rather corner case usage, so I think it is reasonable to assume users who use FairSharing will prefer "Preempt" over "Borrowing".
All of this I would hide behind feature gate FlavorFungabilityImplicitPreferenceDefault.
There was a problem hiding this comment.
Once we hear from users no complains for a couple of releases (most likely scenario) we can just graduate FlavorFungabilityImplicitPreferenceDefault. If some users report issues we can think of preferenceOrder idea to override the implicit default. However, for now I think we can improve the default behavior using feature gate without expanding the API.
There was a problem hiding this comment.
I don't think that we should graduate FlavorFungabilityImplicitPreferenceDefault FG to default enablement ones (beta, ga) since IIRC, pattern 2 (whenCanBorrow: TryNextFlavor & whenCanPreempt: Preempt) breaks the current behavior.
If we want to ship this behavior as a default, we should extend the API.
There was a problem hiding this comment.
To base the discussion on the k8s guidelines, I think this would be it: Deprecating a feature or behavior.
In our case we are not deprecating the feature, but "behavior". I think the relevant rule would be "Rule #7: Deprecated behaviors must function for no less than 1 year after their announced deprecation.", which I'm reading: if we graduate FlavorFungabilityImplicitPreferenceDefault to Beta, then we need to keep it for 1 year in Beta to allow users to return to the old behavior. So, I think the path exists. wdyt?
Still, if you feel strongly about it, I'm ok with API, but I wouldn't like to rush with new APIs. So far all the API proposals I've seen have some drawbacks. So, I'm ok to make the feature gate as "Alpha", but with a note that we intend to replace it with an API. wdyt?
There was a problem hiding this comment.
fyi: we synced on slack with @tenzen-y, and we agreed to introduce an alpha feature gate as a temporary measure to collect feedback from users.
However, going forward instead of graduating the feature gate to Beta, we will introduce an API. This will give us more time to design and discuss the API without rushing. We can add a comment to the feature gate that it is not meant for graduation and its role should be replaced with API.
Unless someone proposes an API which everyone loves, but it seems challenging. So far all proposals have some drawbacks IMO.
There was a problem hiding this comment.
@mimowo Thank you for summarizing our discussion.
As Michal mentioned in the above comment, we introduce a temporary FG (FlavorFungabilityImplicitPreferenceDefault ) for early feedback users for v0,13. However, to avoid the significant specification changes (Borrow -> Preemption), we do not change the behavior as a default. So, we will revist the API consideration for the v0.14. After we define the APIs for new behavior for v0.14, we will remove the FG (FlavorFungabilityImplicitPreferenceDefault).
There was a problem hiding this comment.
Updated the implementation PR (#6132) accordingly. Now I'll update this PR to include the decisions.
|
/lgtm |
|
LGTM label has been added. DetailsGit tree hash: 27b2ffca085aa18641a1e08976ac648dd27d09ca |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mimowo, pajakd The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind documentation
What this PR does / why we need it:
Which issue(s) this PR fixes:
Part of #5424
Special notes for your reviewer:
Does this PR introduce a user-facing change?