Skip to content
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

Add FlavorAssignmentStrategy or flavor priorities to ClusterQueue #312

Open
2 of 3 tasks
Tracked by #360
alculquicondor opened this issue Aug 4, 2022 · 23 comments
Open
2 of 3 tasks
Tracked by #360
Assignees
Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.

Comments

@alculquicondor
Copy link
Contributor

What would you like to be added:

A field in ClusterQueueSpec to influence how flavors are selected when there is already some quota in use.

Why is this needed:

Currently, we strictly go in the order determined by the ClusterQueue resource. But cluster administrators might need different optimization criteria, for example:

  • InOrder: current behavior. It could be used to minimize cost. Cheaper resources can be listed first.
  • MinimizeBorrowing: Choose the flavor that minimizes borrowing. Since a flavor could involve multiple resources, they borrowing can be expressed as percentage of the total request. This can be used to minimize disruptions once preemption is supported.

Completion requirements:

This enhancement requires the following artifacts:

  • Design doc
  • API change
  • Docs update

The artifacts should be linked in subsequent comments.

@alculquicondor alculquicondor added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 4, 2022
@alculquicondor
Copy link
Contributor Author

/kind api-change

@k8s-ci-robot k8s-ci-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Aug 4, 2022
@kerthcet
Copy link
Contributor

It seems like it's part of #171, I'd like to take a look so we can release 0.2.0 soon.
/assign

@kerthcet
Copy link
Contributor

kerthcet commented Aug 19, 2022

Sorry, it actually should be #328, but I can also take a look of this.

@alculquicondor
Copy link
Contributor Author

alculquicondor commented Aug 19, 2022

This is not part of #328. Yes, it will need to be validated, but we don't have this field yet.

@kerthcet
Copy link
Contributor

kerthcet commented Aug 22, 2022

I thought about it a little, I think minimizeBorrowing should be the default policy we should follow, we can borrow resources but we should avoid it if possible for sharing between clusterQueues can be break up.

Besides, rather than prioritize the flavors by the slice order, maybe we should a new field priority, this is a more common style and easy to organize the configurations. And yes, this should also be a default policy.

I don't know whether we should introduce policies like binpack/spread/balanced to FlavorAssignmentStrategy(or FlavorManageStrategy), if we do, it seems we reinvent the wheels in scheduling. I think we can let this fly for a while until community users really need it.

@kerthcet
Copy link
Contributor

cc @alculquicondor for thoughts

@alculquicondor
Copy link
Contributor Author

We can only change the default behavior if we introduce a new API version. We can probably do it as we launch v1beta1.

maybe we should a new field priority

I don't think that adds much value. Going in order is a clear API.

I don't know whether we should introduce policies like binpack/spread/balanced to FlavorAssignmentStrategy

As long as there is a valid use case, we can consider it. Otherwise, I would start with the obvious ones: InOrder, MinimizeBorrowing.

@kerthcet
Copy link
Contributor

maybe we should a new field priority

I don't think that adds much value. Going in order is a clear API.

They can't express the meaning that two flavors are of the same priority. This is useful when we want to apply more policies on flavors, like minimizeBorrowing by priority.

But as you say, let's wait to see if we really need them.

@alculquicondor
Copy link
Contributor Author

but if you use minimizeBorrowing, we would first sort by how much they borrow. If there are multiple flavors that borrow the same (or borrow nothing), we fallback to the order in the list. It is comprehensive enough, in my opinion.

@kerthcet
Copy link
Contributor

kerthcet commented Aug 24, 2022

I think priority should be the first decision factor, then we apply other policies.
Whatever, I'll try to implement these two strategies as mentioned and evolve gradually.

@alculquicondor
Copy link
Contributor Author

I see what you mean. You want to do minimizeBorrowing within a group of flavors. Then, if it doesn't fit, go to the next group of flavors which have lower priority.

To keep some level of "backwards compatibility", we can add a validation rule that priorities can only be decreasing in the list.

@kerthcet
Copy link
Contributor

So the conclusion is:

  1. Add a new field Priority to api field Flavor
type Flavor struct {
	Name ResourceFlavorReference `json:"name"`
	Quota Quota `json:"quota"`
        Priority `json:"priority"`
}
  1. Add a strategy minimizeBorrowing. (We may consider it as a default policy when graduating to a new version)

  2. Add a validation rule that priorities can only be decreasing in the list. (We may cancel this when graduating to a new version?)

Right?

@alculquicondor
Copy link
Contributor Author

  1. I agree, although probably a pointer.
  2. Do we have another strategy? Once you add priority, minimizeBorrowing seems like the only reasonable option for flavors with the same priority.
  3. I would always keep the validation. It makes the API easier to interpret by a human. Also it would be nice if you don't have to define priorities if you don't need them. Then I'm not sure if we should add default priorities (all zero or decreasing)?

Maybe worth a design doc after all? It doesn't have to be long.

@kerthcet
Copy link
Contributor

Yes, I'll write a simplify KEP for this.

@alculquicondor alculquicondor changed the title Add FlavorAssignmentStrategy to ClusterQueue Add FlavorAssignmentStrategy or flavor priorities to ClusterQueue Sep 6, 2022
@kerthcet
Copy link
Contributor

kerthcet commented Sep 20, 2022

FYI: I'm already working on this, but considering the k/k KEP deadline is approaching and I'll have a holiday in Oct., let me focus on k/k first, and turn back ASAP.

@alculquicondor
Copy link
Contributor Author

yup, k/k deadlines are tight

FYI #401

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 19, 2022
@tenzen-y
Copy link
Member

/lifecycle frozen

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 21, 2022
@maaft
Copy link

maaft commented Feb 7, 2023

Hi, any eta for this? I'd really like to use preemption and as far as I see, this is the only blocker left for v0.3! 🚀

@kerthcet
Copy link
Contributor

kerthcet commented Feb 7, 2023

This might be after #532, and the design may also be changed(TBD) since we changed the API, so likely this will not be included in v0.3(preemption is already big enough), but I'll try my best, Let's see. I'm working on another feature now.

@maaft
Copy link

maaft commented Feb 7, 2023

just to confirm: preemption will be included in v0.3 regardless of this feature? That'll be great :)

@kerthcet
Copy link
Contributor

kerthcet commented Feb 7, 2023

just to confirm: preemption will be included in v0.3 regardless of this feature? That'll be great :)

It's not great, it's my bad... :(

@alculquicondor
Copy link
Contributor Author

Yes, preemption is independent of this feature.
I'm also working on #532 before the release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.
Projects
None yet
Development

No branches or pull requests

6 participants