KEP: Support for peremption based on flavor order #810

KunWuLuan · 2023-05-25T06:28:43Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #582

Special notes for your reviewer:

Does this PR introduce a user-facing change?

netlify · 2023-05-25T06:28:51Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`277e73c`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/64fa976c3838600008c525f1

k8s-ci-robot · 2023-05-25T06:28:51Z

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

tenzen-y · 2023-05-25T06:45:11Z

/release-note-none
/ok-to-test

alculquicondor · 2023-05-25T14:00:07Z

cc @trasc for comment

trasc · 2023-05-25T14:24:25Z

keps/582-preempt-based-on-flavor-order/README.md

+
+We will first try `default-flavor1` for cpu and memory resources. If `default-flavor1` doesn't fit, we try preempt in `default-flavor1`. If preemptor return preempt fail, we try to allocate resource in `default-flavor2`.
+
+We will pass preemptor to flavorAssigner and let flavorAssigner try to call public interface of preemptor to get candidate for certain podSet in current flavor if podSet can not find enough resources in current flavor.


How will the preemptor know which flavors are wanted for each call?

Just as a note: currently , the preemptor depends on flavorassigner , not the other way around.

currently preemptor will find resource which need preemption in Do func:

kueue/pkg/scheduler/preemption/preemption.go

Lines 77 to 81 in e2cb8de

func (p *Preemptor) Do(ctx context.Context, wl workload.Info, assignment flavorassigner.Assignment, snapshot *cache.Snapshot) (int, error) {

log := ctrl.LoggerFrom(ctx)

resPerFlv := resourcesRequiringPreemption(assignment)

cq := snapshot.ClusterQueues[wl.ClusterQueue]

resPerFlv contains flavor info and resource name, we can modify preemptor to allow preemptor accept resPerFlv as argument directly. Then preemptor will know the flavor.

we can can achieve this by moving resourcesRequiringPreemption and totalRequestsForAssignment to outside.

Please update based on #806

keps/582-preempt-based-on-flavor-order/README.md

keps/582-preempt-based-on-flavor-order/kep.yaml

alculquicondor · 2023-05-26T19:13:24Z

keps/582-preempt-based-on-flavor-order/README.md

+
+We will pass preemptor to flavorAssigner and let flavorAssigner try to call public interface of preemptor to get candidate for certain podSet in current flavor if podSet can not find enough resources in current flavor.
+
+### Test Plan


Once we have some confidence over the API and algorithm design, please fill this up.

Okay, I'll do that

keps/582-preempt-based-on-flavor-order/README.md

tenzen-y · 2023-05-30T12:16:32Z

keps/582-preempt-based-on-flavor-order/README.md

+            gpu: 1
+```
+
+We will first try `default-flavor1` for cpu and memory resources. If `default-flavor1` doesn't fit, we try preempt in `default-flavor1`. If preemptor return preempt fail, we try to allocate resource in `default-flavor2`.


If default-flavor1 doesn't fit, we try preempt in default-flavor1.

In this situation, what happens if we can preempt default-flavor but it only frees up resources for 2 pods of more high-priority podSets?

Do we preempt default-flavor only If we can free up resources for all pods? Or, do we preempt default-flavor even if we can free up resources for part of the pods?

currently we only allow one kind of resource use one flavor (because now there is only one flavor assignment for one kind of resource).
so I think in this pr we only preempt default-flavor only If we can free up resources for all pods.

because now there is only one flavor assignment for one kind of resource

You're right. We probably should take this situation as another feature.

@alculquicondor @kerthcet Do you think creating an issue for this feature (AssignMultipleFlavorsToWorkload?) is worth it?

the preemption logic should not change.
With #806, we can check whether preemption could find enough candidates.
If it's not possible, then we go to the next flavor.

Do you think creating an issue for this feature (AssignMultipleFlavorsToWorkload?) is worth it?

I don't think we should do this. In most scenarios, this could cause pods to land in very different places, with potentially higher network latency. Also, we can't enforce it, as we wouldn't be able to set node selectors for a subset of pods.

I don't think we should do this. In most scenarios, this could cause pods to land in very different places, with potentially higher network latency. Also, we can't enforce it, as we wouldn't be able to set node selectors for a subset of pods.

That makes sense.

alculquicondor · 2023-06-02T12:10:41Z

@KunWuLuan would you mind adding an extension interface proposal for this KEP?

alculquicondor · 2023-06-07T18:14:40Z

keps/582-preempt-based-on-flavor-order/README.md

+}
+```
+
+Length of FlavorFungibility should be either 0 or 3, this means we don't allow users to omit any of these three actions. If FlavorFungibility is empty, we set FlavorFungibility as `Borrow,TryNextFlavor,Preempt`. Actions after `TryNextFlavor` will be executed after all flavors are considered, and actions before `TryNextFlavor` will be executed before next flavor be considered.


I like the idea that all the actions should be listed. However, I'm not aware of any k8s API that looks like this.

I started a thread on slack https://kubernetes.slack.com/archives/CJUQN3E4T/p1686161408595329

Are you sure all permutations are valid? For example, if we try to Borrow before Preempting, there could be priority inversion, similar to this: #763 (comment)

Somehow TryNextFlavor is special. Should it be something like the following?

beforeTryingNextFlavor: ["Preempt", "Borrow"]

Currently we will borrow unused quota from other cluster queues before we try preempting.
Do you mean we will change this behavior?
If we can say that preemption always takes precedence over borrowing, I think plan A is better.

alculquicondor

We were initially targeting the release of 0.4 by the end of June. I think we might be too late for a feature like this for which we don't have a clear API yet.

We could still make progress and target 0.5 for this feature. WDYT?

KunWuLuan · 2023-06-08T01:53:28Z

would you mind adding an extension interface proposal for this KEP?

Do you mean

We were initially targeting the release of 0.4 by the end of June. I think we might be too late for a feature like this for which we don't have a clear API yet.

We could still make progress and target 0.5 for this feature. WDYT?

No problem, I think more discussion lead to a better design.

alculquicondor · 2023-06-15T19:48:12Z

cc @mwielgus

alculquicondor · 2023-08-01T15:50:56Z

keps/582-preempt-based-on-flavor-order/README.md

+instances. If high priority jobs can preempt jobs in standard instances before trying spot instances,
+stability can be achieved.
+
+My use case can be supported by setting `flavorFungibility` to `BeforeNextFlavor`  in the Kueue configuration.


This sentence looks outdated. The current proposal is doing it in the ClusterQueue spec, right?

Still outdated

alculquicondor · 2023-08-15T14:05:44Z

keps/582-preempt-based-on-flavor-order/README.md

+}
+```
+
+If flavorFungibility is nil in configuration, we will set the `WhenCanBorrow` to `true` and set `WhenCanPreempt` to `false` to maintain consistency with the current behavior.


looks outdated

Still outdated

alculquicondor · 2023-08-15T14:09:39Z

keps/582-preempt-based-on-flavor-order/README.md

+```
+
+If flavorFungibility is nil in configuration, we will set the `WhenCanBorrow` to `true` and set `WhenCanPreempt` to `false` to maintain consistency with the current behavior.
+


Explain what's the default policy, which should match the current behavior.

IIUC, the current behavior is:

whenCanBorrow: Borrow whenCanPreempt: TryNextFlavor

alculquicondor · 2023-08-15T14:24:17Z

keps/582-preempt-based-on-flavor-order/README.md

+
+If flavorFungibility is nil in configuration, we will set the `WhenCanBorrow` to `true` and set `WhenCanPreempt` to `false` to maintain consistency with the current behavior.
+
+### Behavior Changes


What are the semantics with respect to Partial Admission?

alculquicondor · 2023-08-15T14:29:03Z

keps/582-preempt-based-on-flavor-order/README.md

+}
+```
+
+We will store the scheduling context in workload info so that we can start from where we stop in previous scheduling attempts. This will be useful to avoid to waste time in one flavor all the time if we try to preempt in a flavor and failed. Scheduling context will contain the `LastScheduledFlavorIdx`, `ResourceFlavors` attached to the CQ, `ClusterQueueUsage` of the CQ and `CohortUsage`. Any changes to these properties will lead to a scheduling from the first flavor.


Some implementation detail: if the flavors in the CQ change, we should wipe the stored context. Maybe we can keep a hash to detect the changes.

alculquicondor · 2023-08-15T14:32:03Z

keps/582-preempt-based-on-flavor-order/README.md

+### Implementation
+
+```
+func assignFlavors(log logr.Logger, requests []workload.PodSetResources, podSets []kueue.PodSet, resourceFlavors map[kueue.ResourceFlavorReference]*kueue.ResourceFlavor, cq *cache.ClusterQueue, lastSchedule *workload.LastScheduleClusterQueueState) Assignment {


Remove the code here.

KunWuLuan · 2023-08-31T07:03:57Z

work in #849

alculquicondor · 2023-08-31T12:19:34Z

We still should have the accompanying KEP merged. Could you have it up-to-date?

KunWuLuan · 2023-09-01T08:22:32Z

We still should have the accompanying KEP merged. Could you have it up-to-date?

Sorry I had a misoperation. I have updated the kep.

alculquicondor · 2023-09-01T19:19:20Z

Can you run make toc-update?

keps/582-preempt-based-on-flavor-order/README.md

alculquicondor · 2023-09-01T19:22:27Z

keps/582-preempt-based-on-flavor-order/README.md

+instances. If high priority jobs can preempt jobs in standard instances before trying spot instances,
+stability can be achieved.
+
+My use case can be supported by setting `flavorFungibility` to `BeforeNextFlavor`  in the Kueue configuration.


Still outdated

keps/582-preempt-based-on-flavor-order/README.md

alculquicondor · 2023-09-01T19:24:05Z

keps/582-preempt-based-on-flavor-order/README.md

+}
+```
+
+If flavorFungibility is nil in configuration, we will set the `WhenCanBorrow` to `true` and set `WhenCanPreempt` to `false` to maintain consistency with the current behavior.


Still outdated

keps/582-preempt-based-on-flavor-order/README.md

keps/582-preempt-based-on-flavor-order/kep.yaml

KunWuLuan · 2023-09-04T02:03:30Z

Readme is updated already. All comments were resolved.

keps/582-preempt-based-on-flavor-order/kep.yaml

keps/582-preempt-based-on-flavor-order/README.md

alculquicondor · 2023-09-06T14:50:48Z

/approve
Please run make toc-update again.

k8s-ci-robot · 2023-09-06T14:50:56Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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 · 2023-09-06T14:51:27Z

/label tide/merge-method-squash

KunWuLuan · 2023-09-08T03:43:42Z

Have run make toc-update

alculquicondor · 2023-09-08T13:20:50Z

/lgtm

alculquicondor · 2023-09-08T14:25:47Z

/retest
due to #1090

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 25, 2023

k8s-ci-robot requested review from alculquicondor and denkensk May 25, 2023 06:28

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 May 25, 2023

KunWuLuan mentioned this pull request May 25, 2023

WIP: add preemptBasedOnFlavorOrder #730

Closed

trasc reviewed May 25, 2023

View reviewed changes

alculquicondor reviewed May 26, 2023

View reviewed changes

tenzen-y reviewed May 30, 2023

View reviewed changes

alculquicondor reviewed Jun 7, 2023

View reviewed changes

KunWuLuan force-pushed the kep/preemptionFlavorOrder branch from 2ed8aa0 to 25c3c05 Compare June 13, 2023 09:58

alculquicondor reviewed Aug 15, 2023

View reviewed changes

alculquicondor mentioned this pull request Aug 21, 2023

KEP: add flavorAssignmentStrategy to localQueue #376

Closed

KunWuLuan closed this Aug 31, 2023

alculquicondor reviewed Sep 1, 2023

View reviewed changes

KunWuLuan added 7 commits September 4, 2023 10:01

kep: support for peremption based on flavor order

bfc1f77

Modify FlavorFungibility API based on review comments

2f02cd1

add design details

0aa9626

remove plan b

b45185f

add some details

b41554c

update the details

a8da46c

update kep readme

a45ffdf

KunWuLuan force-pushed the kep/preemptionFlavorOrder branch from 32a23b4 to a45ffdf Compare September 4, 2023 02:01

update toc

38932d5

KunWuLuan requested a review from alculquicondor September 4, 2023 02:04

alculquicondor reviewed Sep 5, 2023

View reviewed changes

keps/582-preempt-based-on-flavor-order/kep.yaml Outdated Show resolved Hide resolved

keps/582-preempt-based-on-flavor-order/kep.yaml Outdated Show resolved Hide resolved

keps/582-preempt-based-on-flavor-order/README.md Outdated Show resolved Hide resolved

add integration test description

4b66a7a

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

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Sep 6, 2023

update toc

277e73c

k8s-ci-robot assigned alculquicondor Sep 8, 2023

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

k8s-ci-robot merged commit 1a7e294 into kubernetes-sigs:main Sep 8, 2023
15 checks passed

k8s-ci-robot added this to the v0.5 milestone Sep 8, 2023

KunWuLuan deleted the kep/preemptionFlavorOrder branch September 12, 2023 03:00


		We will first try `default-flavor1` for cpu and memory resources. If `default-flavor1` doesn't fit, we try preempt in `default-flavor1`. If preemptor return preempt fail, we try to allocate resource in `default-flavor2`.

		We will pass preemptor to flavorAssigner and let flavorAssigner try to call public interface of preemptor to get candidate for certain podSet in current flavor if podSet can not find enough resources in current flavor.

	func (p Preemptor) Do(ctx context.Context, wl workload.Info, assignment flavorassigner.Assignment, snapshot cache.Snapshot) (int, error) {
	log := ctrl.LoggerFrom(ctx)

	resPerFlv := resourcesRequiringPreemption(assignment)
	cq := snapshot.ClusterQueues[wl.ClusterQueue]


		We will pass preemptor to flavorAssigner and let flavorAssigner try to call public interface of preemptor to get candidate for certain podSet in current flavor if podSet can not find enough resources in current flavor.

		### Test Plan

		```

		If flavorFungibility is nil in configuration, we will set the `WhenCanBorrow` to `true` and set `WhenCanPreempt` to `false` to maintain consistency with the current behavior.


		If flavorFungibility is nil in configuration, we will set the `WhenCanBorrow` to `true` and set `WhenCanPreempt` to `false` to maintain consistency with the current behavior.

		### Behavior Changes

KEP: Support for peremption based on flavor order #810

KEP: Support for peremption based on flavor order #810

Conversation

KunWuLuan commented May 25, 2023

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 May 25, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

k8s-ci-robot commented May 25, 2023

tenzen-y commented May 25, 2023

alculquicondor commented May 25, 2023

Choose a reason for hiding this comment

KunWuLuan May 26, 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

tenzen-y May 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

alculquicondor commented Jun 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor left a comment • edited Loading

Choose a reason for hiding this comment

KunWuLuan commented Jun 8, 2023

alculquicondor commented Jun 15, 2023

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

KunWuLuan commented Aug 31, 2023

alculquicondor commented Aug 31, 2023

KunWuLuan commented Sep 1, 2023

alculquicondor commented Sep 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KunWuLuan commented Sep 4, 2023

alculquicondor commented Sep 6, 2023

k8s-ci-robot commented Sep 6, 2023

alculquicondor commented Sep 6, 2023

KunWuLuan commented Sep 8, 2023

alculquicondor commented Sep 8, 2023

alculquicondor commented Sep 8, 2023

netlify bot commented May 25, 2023 •

edited

Loading

KunWuLuan May 26, 2023 •

edited

Loading

tenzen-y May 30, 2023 •

edited

Loading

alculquicondor left a comment •

edited

Loading