[KEP] FlavorFungability: replace FlavorFungibilityImplicitPreferenceDefault feature gate with API

[vladikkuzn]

What type of PR is this?

/kind documentation

What this PR does / why we need it:

Which issue(s) this PR fixes:

Part of #6184

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

[k8s-ci-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[netlify]

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	cd85557
🔍 Latest deploy log	https://app.netlify.com/projects/kubernetes-sigs-kueue/deploys/69056187f206ed0008ab4b9e

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vladikkuzn
Once this PR has been reviewed and has the lgtm label, please assign gabesaba for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

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

[vladikkuzn]

/cc @pajakd
Pls help me understand if there's a need for preferenceOrder and if separate KEP is needed to be created

[vladikkuzn]

/cc @gabesaba

[pajakd]

In my opinion, no need to create a separate, KEP. You can update KEP-582 with the new API definition.

If the objective is to remove the feature gate FungibilityImplicitPreference, the API needs to be able to express the user preferences (between Preemption and Borrowing). The new field as described here is sufficient for that.

One thing to note is that in the API with additional field, the users will be able to express some configurations that make little sense. For example:

flavorFungibility:
  whenCanBorrow: TryNextFlavor
  whenCanPreempt: MayStopSearch
  preferenceOrder: ["Borrow", "Preempt", "PreemptWhileBorrowing"]

The whenCanBorrow: TryNextFlavor whenCanPreempt: MayStopSearch suggest that we should find a flavor that does not borrow. But the preferenceOrder defines a completely different preference. So, we have to add validation to make sure, that such combinations are not allowed.

[vladikkuzn]

So, we have to add validation to make sure, that such combinations are not allowed.

@pajakd Could you clarify which specific validations we should add to prevent inconsistent configurations like the example above?

[pajakd]

There are 4 options of the whenCanBorrow/whenCanPreempt

1. whenCanBorrow: MayStopSearch whenCanPreempt: MayStopSearch here any preferenceOrder is valid. But any preferenceOrder will yield the same result since we just return first possible flavor.
2. whenCanBorrow: TryNextFlavor whenCanPreempt: MayStopSearch here we should validate that Borrow is after Preempt in the preferenceOrder
3. whenCanBorrow: MayStopSearch whenCanPreempt: TryNextFlavor here we should validate that Preempt is after Borrow in the preferenceOrder
4. whenCanBorrow: TryNextFlavor whenCanPreempt: TryNextFlavor here any preferenceOrder is valid.

We should (I think) also validate that PreemptWhileBorrowing is after Preempt.

So there are 3 validations.

I'm not sure what should be the default value of the preferenceOrder. There is no static value that would be valid for all combinations of whenCanBorrow, whenCanPreempt. @mimowo any idea?

[mimowo]

Ok, I think there are two main approaches which are not very clear for me how to reconcile:
A. flexibility first, introduce preferenceOrder: [] and validate against unnecessary options
B. minialistic api, introduce preference: Borrowing / Preempting

Let me dive deeper how the API could look like in the both cases:

1. whenCanBorrow: MayStopSearch whenCanPreempt: MayStopSearch

A) block preferenceOrder,
B) block preference
the choice does not matter

2. whenCanBorrow: TryNextFlavor whenCanPreempt: MayStopSearch

A) block preferenceOrder; implies: [Preempt, Borrow, PreemptWhileBorrowing]
B) block preference, implies: Preemption

3. whenCanBorrow: MayStopSearch whenCanPreempt: TryNextFlavor

A) block preferenceOrder; implies: [Borrow, Preempt, PreemptWhileBorrowing]
B) block preference, implies: Borrowing

4. whenCanBorrow: TryNextFlavor whenCanPreempt: TryNextFlavor

A) allow preferenceOrder - allow any order
B) allow preference: Borrowing / Preemption, implies [Borrow, Preempt, PreemptWhileBorrowing] or [Preempt, Borrow, PreemptWhileBorrowing], respectively

Given that for now we are using the feature gate which basically is a switch as (B), and it seems to cover the known use cases I would be leaning now to the approach of minimalistic API - the preference field which is only available in (4.).

[vladikkuzn]

@mimowo So is PreemptWhileBorrowing always last? And we only choose between preemption and borrowing?

[mimowo]

IIUC this is the current behavior when the feature gate is used, wdyt @pajakd ?

But basically I'm thinking of minimal API which replaces the FG, @vladikkuzn please also check how you understand the current logic, we should align on that.

[vladikkuzn]

Given current needs and that the gate is already acting as a “preference switch”, let's adopt B (minimal field) and validate it’s only set when both whenCan* are TryNextFlavor. If later we need finer control, we can evolve to A. WDYT @mimowo @pajakd?

[mimowo]

Given current needs and that the gate is already acting as a “preference switch”, let's adopt B (minimal field) and validate it’s only set when both whenCan* are TryNextFlavor. If later we need finer control, we can evolve to A. WDYT @mimowo @pajakd?

This makes sense, we can always relax the validation in the future if needed. For now I think @amy is using the new feature gate and so a simple switch to API seems preferrable to me.

[pajakd]

I agree, I think a simple field preference will do the job.

Please add a comment next to this field that preference: Borrowing does not mean that we will choose Borrowing over NoBorrowing but rather that we will choose Borrowing over Preemption.

[vladikkuzn]

/assign
/cc @amy

[mimowo]

I think this is likely the approach so @vladikkuzn feel free to start drafting the implementation

[mimowo]

cc @tenzen-y any additional comments here?

[tenzen-y]

Additionally, does this include the deprecation of FlavorFungibilityImplicitPreferenceDefault FG?
If yes, could you add a Note as I proposed in another thread?

Suggested change

	The feature gate `FlavorFungibilityImplicitPreferenceDefault` was a temporary measure.
	Preference is now configured per ClusterQueue via `spec.flavorFungibility.preference`; the gate has been removed.
	The feature gate `FlavorFungibilityImplicitPreferenceDefault` was supported between v0.13 and v0.14 and it was removed in v0.15.
	Since v0.15, the Preference is now configured per ClusterQueue via `spec.flavorFungibility.preference`.

[mimowo]

Yes the FG should be deprecated and removed in 0.17. The filed overrides the meaning of the FG until then. Please update

[vladikkuzn]

@mimowo 0.17 or 0.15? should I keep the feature gate then until 0.17?

[mimowo]

I think let's go with 0.16. I think it makes sense to keep it in 0.15 to allow smooth upgrade. So, in 0.14 some users will use the FG, they cannot remove. After upgrade they need time to add the new preference field. So by supporting in 0.16 we can allow smooth transition.

[mimowo]

OTOH, this FG is just Alpha so we don't have a strong commitment to support the smooth transition. So, if we encounter complications I'm good with 0.15.

[tenzen-y]

@vladikkuzn Once #7317 (comment) and #7317 (comment) are addressed, we can merge this PR.

[vladikkuzn]

@tenzen-y Already applied in draft implementation, let me apply here as well 😉