WIP: add preemptBasedOnFlavorOrder #730

KunWuLuan · 2023-04-27T02:33:42Z

k8s-ci-robot · 2023-04-27T02:33:47Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: KunWuLuan
Once this PR has been reviewed and has the lgtm label, please assign alculquicondor for approval. For more information see the Kubernetes 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

netlify · 2023-04-27T02:33:50Z

✅ Deploy Preview for kubernetes-sigs-kueue ready!

Name	Link
🔨 Latest commit	`66b83ad`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/64642bcea6ea3c000839b602
😎 Deploy Preview	https://deploy-preview-730--kubernetes-sigs-kueue.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

k8s-ci-robot · 2023-04-27T02:33: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.

KunWuLuan · 2023-04-27T02:34:12Z

I will add test in another commit later.

alculquicondor · 2023-04-27T12:33:36Z

/ok-to-test

alculquicondor · 2023-04-27T12:34:21Z

Unrelated to the PR: what's your username in the k8s slack?

KunWuLuan · 2023-04-28T08:53:27Z

@alculquicondor Hi, aldo. I just joined the Kubernetes slack. My username is KunWuLuan

alculquicondor

/retitle WIP: add preemptBasedOnFlavorOrder

alculquicondor · 2023-05-03T19:34:09Z

apis/kueue/v1beta1/clusterqueue_types.go

@@ -272,6 +272,9 @@ type ClusterQueuePreemption struct {
 	// +kubebuilder:default=Never
 	// +kubebuilder:validation:Enum=Never;LowerPriority
 	WithinClusterQueue PreemptionPolicy `json:"withinClusterQueue,omitempty"`
+
+	// +kubebuilder:default="flase"
+	PreemptBasedOnFlavorOrder bool `json:"preemptBasedOnFlavorOrder"`


In general, booleans might not be the best API https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#primitive-types

For example:

flavorFungibility: NextFlavorBeforePreemption | PreemptionBeforeNexFlavor

I don't really like the names I'm proposing, but I'll leave it to you to come up with other ideas.

how about

PreemptionTiming: BeforeNextFlavor | AfterAllFlavors

alculquicondor · 2023-05-03T19:39:21Z

pkg/scheduler/flavorassigner/flavorassigner.go

@@ -368,6 +368,9 @@ func (a *Assignment) findFlavorForResourceGroup(
 			}
 		}

+		if cq.Preemption.PreemptBasedOnFlavorOrder && representativeMode == Preempt {


The idea should be that if preemption is not possible, we proceed to the next flavor. But the flavor assigner doesn't really know about whether preemption is possible.
We might need to change the implementation of framework assigner to return a list of assignments that the scheduler and preemption algorithm. Or maybe we need to add tighter dependencies between the flavorAssigner and the preemption algorithms.

I think we can just keep track in-memory of whether preemption was possible. If it was not, we put the workload back into the head of the queue to try the next flavor in the next iteration. And once we have passed through all the flavors, we requeue the workload as inadmissible.

Thanks for your time and help. I will continue to improve this submission. 😸

consider writing a design for this feature, following this template https://github.com/kubernetes-sigs/kueue/tree/main/keps/NNNN-template

No problem, I will submit a proposal in next week

k8s-ci-robot · 2023-05-03T19:39:43Z

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

KunWuLuan · 2023-05-25T01:45:47Z

@alculquicondor @mortent Hi, I have submited a kep for this feature. If you have time, could you take a look and give me your opinions? Thank you!

tenzen-y · 2023-05-25T03:58:41Z

@alculquicondor @mortent Hi, I have submited a kep for this feature. If you have time, could you take a look and give me your opinions? Thank you!

@KunWuLuan Thank you for submitting a KEP.
Can you create a separate PR for a KEP instead of including this PR?

KunWuLuan · 2023-05-25T06:24:53Z

@alculquicondor @mortent Hi, I have submited a kep for this feature. If you have time, could you take a look and give me your opinions? Thank you!

@KunWuLuan Thank you for submitting a KEP. Can you create a separate PR for a KEP instead of including this PR?

@tenzen-y OK, I will create a separate RP to track the KEP, thanks

KunWuLuan · 2023-05-25T06:31:09Z

I have created a new PR #810 to track the kep

k8s-ci-robot · 2023-05-27T05:35:19Z

PR needs rebase.

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.

k8s-ci-robot · 2023-06-14T13:41:26Z

@KunWuLuan: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kueue-verify-main	`66b83ad`	link	true	`/test pull-kueue-verify-main`
pull-kueue-test-integration-main	`66b83ad`	link	true	`/test pull-kueue-test-integration-main`
pull-kueue-test-e2e-main-1-26	`66b83ad`	link	true	`/test pull-kueue-test-e2e-main-1-26`
pull-kueue-test-e2e-main-1-24	`66b83ad`	link	true	`/test pull-kueue-test-e2e-main-1-24`
pull-kueue-test-e2e-main-1-25	`66b83ad`	link	true	`/test pull-kueue-test-e2e-main-1-25`
pull-kueue-test-e2e-main-1-27	`66b83ad`	link	true	`/test pull-kueue-test-e2e-main-1-27`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

alculquicondor · 2023-08-16T14:29:04Z

@KunWuLuan I think the other approach is better. Can we close this?

KunWuLuan · 2023-08-17T01:30:18Z

the other approach is better #849

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

k8s-ci-robot requested review from alculquicondor and ArangoGutierrez April 27, 2023 02:33

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 Apr 27, 2023

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 27, 2023

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 Apr 27, 2023

alculquicondor reviewed May 3, 2023

View reviewed changes

k8s-ci-robot changed the title ~~add preemptBasedOnFlavorOrder knob~~ WIP: add preemptBasedOnFlavorOrder May 3, 2023

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 3, 2023

KunWuLuan added 2 commits May 16, 2023 14:39

add preemptBasedOnFlavorOrder knob

1a14a6d

add kep

ba16983

KunWuLuan force-pushed the feature/preemptionFlavorOrder branch from 2d998f4 to ba16983 Compare May 16, 2023 12:53

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 16, 2023

add kep.yaml

66b83ad

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 27, 2023

KunWuLuan closed this Aug 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: add preemptBasedOnFlavorOrder #730

WIP: add preemptBasedOnFlavorOrder #730

KunWuLuan commented Apr 27, 2023

k8s-ci-robot commented Apr 27, 2023

netlify bot commented Apr 27, 2023 •

edited

Loading

k8s-ci-robot commented Apr 27, 2023

KunWuLuan commented Apr 27, 2023

alculquicondor commented Apr 27, 2023

alculquicondor commented Apr 27, 2023

KunWuLuan commented Apr 28, 2023

alculquicondor left a comment

alculquicondor May 3, 2023

KunWuLuan May 8, 2023

alculquicondor May 3, 2023

alculquicondor May 3, 2023

KunWuLuan May 8, 2023

alculquicondor May 8, 2023

KunWuLuan May 9, 2023

k8s-ci-robot commented May 3, 2023

KunWuLuan commented May 25, 2023

tenzen-y commented May 25, 2023

KunWuLuan commented May 25, 2023

KunWuLuan commented May 25, 2023

k8s-ci-robot commented May 27, 2023

k8s-ci-robot commented Jun 14, 2023

alculquicondor commented Aug 16, 2023

KunWuLuan commented Aug 17, 2023

WIP: add preemptBasedOnFlavorOrder #730

WIP: add preemptBasedOnFlavorOrder #730

Conversation

KunWuLuan commented Apr 27, 2023

k8s-ci-robot commented Apr 27, 2023

netlify bot commented Apr 27, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue ready!

k8s-ci-robot commented Apr 27, 2023

KunWuLuan commented Apr 27, 2023

alculquicondor commented Apr 27, 2023

alculquicondor commented Apr 27, 2023

KunWuLuan commented Apr 28, 2023

alculquicondor left a comment

Choose a reason for hiding this comment

alculquicondor May 3, 2023

Choose a reason for hiding this comment

KunWuLuan May 8, 2023

Choose a reason for hiding this comment

alculquicondor May 3, 2023

Choose a reason for hiding this comment

alculquicondor May 3, 2023

Choose a reason for hiding this comment

KunWuLuan May 8, 2023

Choose a reason for hiding this comment

alculquicondor May 8, 2023

Choose a reason for hiding this comment

KunWuLuan May 9, 2023

Choose a reason for hiding this comment

k8s-ci-robot commented May 3, 2023

KunWuLuan commented May 25, 2023

tenzen-y commented May 25, 2023

KunWuLuan commented May 25, 2023

KunWuLuan commented May 25, 2023

k8s-ci-robot commented May 27, 2023

k8s-ci-robot commented Jun 14, 2023

alculquicondor commented Aug 16, 2023

KunWuLuan commented Aug 17, 2023

netlify bot commented Apr 27, 2023 •

edited

Loading