Add APIs for configuring fair sharing #2070

alculquicondor · 2024-04-25T19:28:42Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Add APIs for fair sharing:

In KueueConfiguration, a new FairSharing struct to enable the feature and configure strategies.
In ClusterQueueSpec, a new FairSharing struct containing the weight.

Which issue(s) this PR fixes:

Fixes #1714

Special notes for your reviewer:

At this point, I'm looking for feedback on the types and field documentation.

Does this PR introduce a user-facing change?

Add fair sharing when borrowing unused resources from other ClusterQueues in a cohort.

Fair sharing is based on DRF for usage above nominal quotas.
When fair sharing is enabled, Kueue prefers to admit workloads from ClusterQueues with the lowest share first.
Administrators can enable and configure fair sharing preemption using a combination of two policies: `LessThanOrEqualtoFinalShare`, `LessThanInitialShare`.

You can define a fair sharing `weight` for ClusterQueues. The weight determines how much of the unused resources each ClusterQueue can take in comparison to others.

netlify · 2024-04-25T19:29:05Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`357b499`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/663ba16103576a0008c34782

alculquicondor · 2024-04-25T19:29:08Z

cc @mwielgus @tenzen-y

apis/kueue/v1beta1/clusterqueue_types.go

apis/config/v1beta1/configuration_types.go

tenzen-y

I left a comment for a nit.

pkg/config/validation.go

apis/config/v1beta1/configuration_types.go

alculquicondor · 2024-05-03T18:17:14Z

This is now ready for review 😄

/assign @tenzen-y @mwielgus

tenzen-y

Totally, lgtm
I left a few comments.
By the way, it seems that the fairSharing feature overlaps borrowWithinCohort at some points. So, in the v1 API, we might be able to consolidate both APIs into a single one or provide a comprehensive feature somehow.

pkg/cache/clusterqueue.go

pkg/scheduler/preemption/preemption.go

alculquicondor · 2024-05-03T20:21:25Z

By the way, it seems that the fairSharing feature overlaps borrowWithinCohort at some points. So, in the v1 API, we might be able to consolidate both APIs into a single one or provide a comprehensive feature somehow.

Did you mean that borrowWithinCohort can be replaced by using weight: 0?

If so, I agree. I would like to completely remove borrowWithinCohort once we go to v1, unless we hear some opposing feedback.

Change-Id: I74b24ba8715290753c0bd9b966a109b5b01078b4

Change-Id: If347eabdb17af643d46a1e4ad78b79e73c424011

Change-Id: I6938d83399c55fecf952a570e2e431ad4ab479b2

Change-Id: I8b49d21f0b0e7a7d2607d9589ba49a4a914cd2aa

Change-Id: I68ba0c546ce27d9e6565a30584aacd67fd318ede

Change-Id: I375166b8c7fdc300eeb43ede25fbcbe73298c7a2

Change-Id: I4d56c73f4a949fa5c2b2053fd83a7161553755b0

mimowo · 2024-05-07T07:15:08Z

apis/config/v1beta1/configuration_types.go

+	//   This strategy doesn't depend on the share usage of the workload being preempted.
+	//   As a result, the strategy chooses to preempt workloads with the lowest priority and
+	//   newest start time first.
+	// The default strategy is ["LessThanOrEqualToFinalShare"].


As per offline discussions we consider the two combinations as most likely in use:

[LessThanOrEqualToFinalShare, LessThanInitialShare] and

[LessThanInitialShare]

So, I would suggest setting the default to one of the configurations.

I would vote for the [LessThanOrEqualToFinalShare, LessThanInitialShare] since it matches the KEP.

mimowo · 2024-05-07T07:27:00Z

pkg/scheduler/preemption/preemption.go

+	}
+	if s[0] == config.LessThanInitialShare {
+		result[0] = lessThanInitialShare
+		// This rule is a superset of the other rule, no need to check other strategies.


Since we go for the "mathematical" approach, I would suggest to be precise, and don't make the assumption that Rule B is a superset of Rule A (since unlikely scenarios may happen).

mimowo · 2024-05-07T07:27:47Z

pkg/config/validation.go

+		allErrs = append(allErrs, field.Invalid(fsPreemptionStrategiesPath, fs.PreemptionStrategies, "Must be empty when fair sharing is disabled"))
+	}
+	strategies := sets.New[configapi.PreemptionStrategy]()
+	validStrategies := []configapi.PreemptionStrategy{configapi.LessThanInitialShare, configapi.LessThanOrEqualToFinalShare}


I think we should either validate against non-supported configurations or support them. I'm leaning towards validation, because it will keep the code simpler. IIUC this is the only non-supported option for now: [LessThanInitialShare, LessThanOrEqualToFinalShare] .

Changed validation to have an explicit list of supported combinations.

alculquicondor · 2024-05-07T19:35:04Z

borrowWithinCohort once we go to v1, unless we

But why, do we have similar capacity covered this?

Yes, the biggest motivation to have borrowWithinCohort was to allow certain ClusterQueues to always be preempted whenever some other CQ needs the resources.

And this can be easily achieved when enabling fair sharing, by setting the weight on these "preemptible" CQs to zero.

Change-Id: I6893b7854656601e80f157b60caea27e27dfe4ea

Change-Id: I6379c4304cfb45a08940d57a24b79c6b1564ccca

alculquicondor · 2024-05-08T13:09:06Z

@tenzen-y anything to add?

mimowo · 2024-05-08T13:11:48Z

config/components/manager/controller_manager_config.yaml

@@ -50,3 +50,6 @@ integrations:
 #       - key: kubernetes.io/metadata.name
 #         operator: NotIn
 #         values: [ kube-system, kueue-system ]
+# fairSharing:
+#   enable: true
+#   preemptionStrategies: [LessThanOrEqualToFinalShare]


nit: Maybe align this with the current default?

mimowo · 2024-05-08T13:14:12Z

pkg/config/validation.go

+	if !fs.Enable && len(fs.PreemptionStrategies) != 0 {
+		allErrs = append(allErrs, field.Invalid(fsPreemptionStrategiesPath, fs.PreemptionStrategies, "Must be empty when fair sharing is disabled"))
+	}


Do we need it? I guess it can be handy to keep th configuration when temporarily disabling the feature.
Similarly as for waitForPodsReady.

If there are no drawbacks (similar to additional computation, and potentially bugs in preemptor), I would prefer to apply the @mimowo proposal here.

mimowo · 2024-05-08T13:37:03Z

I did another pass for fair sharing, lgtm, just the two nit-like comments

tenzen-y

LGTM
Thank you!

apis/config/v1beta1/configuration_types.go

tenzen-y · 2024-05-08T15:42:57Z

pkg/config/validation.go

+	if !fs.Enable && len(fs.PreemptionStrategies) != 0 {
+		allErrs = append(allErrs, field.Invalid(fsPreemptionStrategiesPath, fs.PreemptionStrategies, "Must be empty when fair sharing is disabled"))
+	}


If there are no drawbacks (similar to additional computation, and potentially bugs in preemptor), I would prefer to apply the @mimowo proposal here.

Change-Id: I1ec9229cb866d31ece5046741ac7686157639758

tenzen-y

Hugely LGTM!
The implementation of the FairSharing feature inspired by Dominant Resource Fairness was not a really easy journey! Awsome 🎉

/lgtm
/approve

k8s-ci-robot · 2024-05-08T16:04:27Z

LGTM label has been added.

Git tree hash: 07fccade5bb30526c1bae6d4b147b6d21d43dcf8

k8s-ci-robot · 2024-05-08T16:04:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, tenzen-y

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,tenzen-y]

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

* Add APIs for configuring fair sharing Change-Id: I74b24ba8715290753c0bd9b966a109b5b01078b4 * Validation and defaulting for fair sharing Change-Id: If347eabdb17af643d46a1e4ad78b79e73c424011 * Implement multiple fair strategies Change-Id: I6938d83399c55fecf952a570e2e431ad4ab479b2 * Implement fair sharing weight Change-Id: I8b49d21f0b0e7a7d2607d9589ba49a4a914cd2aa * Fix flaky integration Change-Id: I68ba0c546ce27d9e6565a30584aacd67fd318ede * review Change-Id: I375166b8c7fdc300eeb43ede25fbcbe73298c7a2 * Update documentation for strategies Change-Id: I4d56c73f4a949fa5c2b2053fd83a7161553755b0 * Change default configuration to match KEP Change-Id: I6893b7854656601e80f157b60caea27e27dfe4ea * Disable fair sharing by default Change-Id: I6379c4304cfb45a08940d57a24b79c6b1564ccca * Relax validation Change-Id: I1ec9229cb866d31ece5046741ac7686157639758

k8s-ci-robot requested review from kerthcet and mimowo April 25, 2024 19:28

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 25, 2024

tenzen-y reviewed Apr 26, 2024

View reviewed changes

apis/config/v1beta1/configuration_types.go Outdated Show resolved Hide resolved

alculquicondor force-pushed the fair-sharing-api branch 4 times, most recently from cdb3926 to 87453cf Compare April 26, 2024 18:57

tenzen-y reviewed Apr 29, 2024

View reviewed changes

pkg/config/validation.go Outdated Show resolved Hide resolved

tenzen-y reviewed Apr 29, 2024

View reviewed changes

apis/config/v1beta1/configuration_types.go Outdated Show resolved Hide resolved

alculquicondor force-pushed the fair-sharing-api branch from 87453cf to 2dac953 Compare May 3, 2024 16:55

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 3, 2024

alculquicondor changed the title ~~WIP: Add APIs for configuring fair sharing~~ Add APIs for configuring fair sharing May 3, 2024

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 3, 2024

alculquicondor force-pushed the fair-sharing-api branch 2 times, most recently from 9cb7292 to fc94535 Compare May 3, 2024 18:02

k8s-ci-robot assigned mwielgus and tenzen-y May 3, 2024

tenzen-y reviewed May 3, 2024

View reviewed changes

pkg/cache/clusterqueue.go Outdated Show resolved Hide resolved

pkg/scheduler/preemption/preemption.go Outdated Show resolved Hide resolved

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

alculquicondor added 7 commits May 6, 2024 18:55

Add APIs for configuring fair sharing

97e42c3

Change-Id: I74b24ba8715290753c0bd9b966a109b5b01078b4

Validation and defaulting for fair sharing

9f70827

Change-Id: If347eabdb17af643d46a1e4ad78b79e73c424011

Implement multiple fair strategies

5e56937

Change-Id: I6938d83399c55fecf952a570e2e431ad4ab479b2

Implement fair sharing weight

6ca8506

Change-Id: I8b49d21f0b0e7a7d2607d9589ba49a4a914cd2aa

Fix flaky integration

b0b7f7e

Change-Id: I68ba0c546ce27d9e6565a30584aacd67fd318ede

review

6dce54c

Change-Id: I375166b8c7fdc300eeb43ede25fbcbe73298c7a2

Update documentation for strategies

a93e785

Change-Id: I4d56c73f4a949fa5c2b2053fd83a7161553755b0

alculquicondor force-pushed the fair-sharing-api branch from e1b68d8 to a93e785 Compare May 6, 2024 19:19

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

mimowo reviewed May 7, 2024

View reviewed changes

Change default configuration to match KEP

1adb8e0

Change-Id: I6893b7854656601e80f157b60caea27e27dfe4ea

alculquicondor force-pushed the fair-sharing-api branch from 2f4311c to 1adb8e0 Compare May 7, 2024 19:55

Disable fair sharing by default

a59ed8d

Change-Id: I6379c4304cfb45a08940d57a24b79c6b1564ccca

mimowo reviewed May 8, 2024

View reviewed changes

tenzen-y reviewed May 8, 2024

View reviewed changes

Relax validation

357b499

Change-Id: I1ec9229cb866d31ece5046741ac7686157639758

tenzen-y reviewed May 8, 2024

View reviewed changes

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

k8s-ci-robot merged commit 54d1c01 into kubernetes-sigs:main May 8, 2024
15 checks passed

k8s-ci-robot added this to the v0.7 milestone May 8, 2024

tenzen-y mentioned this pull request May 8, 2024

Reorganize validations for the frameworks and the externalFrameworks #2130

Merged

mimowo mentioned this pull request May 29, 2024

Enable randomization in the queuing strategy #2297

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add APIs for configuring fair sharing #2070

Add APIs for configuring fair sharing #2070

alculquicondor commented Apr 25, 2024 •

edited

Loading

netlify bot commented Apr 25, 2024 •

edited

Loading

alculquicondor commented Apr 25, 2024

tenzen-y left a comment

alculquicondor commented May 3, 2024

tenzen-y left a comment

alculquicondor commented May 3, 2024

mimowo May 7, 2024

alculquicondor May 7, 2024

mimowo May 7, 2024

mimowo May 7, 2024

alculquicondor May 7, 2024

alculquicondor commented May 7, 2024

alculquicondor commented May 8, 2024

mimowo May 8, 2024

alculquicondor May 8, 2024

mimowo May 8, 2024

tenzen-y May 8, 2024

alculquicondor May 8, 2024

mimowo commented May 8, 2024

tenzen-y left a comment

tenzen-y May 8, 2024

tenzen-y left a comment

k8s-ci-robot commented May 8, 2024

k8s-ci-robot commented May 8, 2024

Add APIs for configuring fair sharing #2070

Add APIs for configuring fair sharing #2070

Conversation

alculquicondor commented Apr 25, 2024 • edited Loading

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 Apr 25, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

alculquicondor commented Apr 25, 2024

tenzen-y left a comment

Choose a reason for hiding this comment

alculquicondor commented May 3, 2024

tenzen-y left a comment

Choose a reason for hiding this comment

alculquicondor commented May 3, 2024

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 May 7, 2024

alculquicondor commented May 8, 2024

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

mimowo commented May 8, 2024

tenzen-y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenzen-y left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 8, 2024

k8s-ci-robot commented May 8, 2024

alculquicondor commented Apr 25, 2024 •

edited

Loading

netlify bot commented Apr 25, 2024 •

edited

Loading