KEP-4136: Admission Fair Sharing #4252

mwielgus · 2025-02-12T23:35:49Z

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

KEP for a new resource fair sharing method.

Which issue(s) this PR fixes:

Fixes #4136

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

netlify · 2025-02-12T23:36:06Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`ba587fa`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/680275236f513d0008c66cdc

mimowo · 2025-02-13T10:19:20Z

/assign @PBundyra @gabesaba
To help with the review. I appreciate it is planned for 0.12 as we already have a plan for 0.11: #4249
and this seems big.

keps/4136-admission-fair-sharing/README.md

gabesaba · 2025-02-14T10:05:22Z

keps/4136-admission-fair-sharing/README.md

+admission logic. If there are two AdmissionScopes on the path from CQ/Cohort to the top of
+the hierarchy tree, the higher one is used.
+
+Const (


fix formatting

keps/4136-admission-fair-sharing/README.md

gabesaba · 2025-02-14T10:24:42Z

keps/4136-admission-fair-sharing/README.md

+
+* Create a new struct AdmissionScope and make it an optional field for CQ and Cohort Spec. If
+not provided, CQ or Cohort is not considered an AdmissionScope and is not a subject for new
+admission logic. If there are two AdmissionScopes on the path from CQ/Cohort to the top of


Why? Let's make this invalid state, as the lower scope is doing nothing?

Because someone may be changing the scope. Scope change is not atomic and we cannot block the entire hierarchy in the meantime.

gabesaba · 2025-02-14T10:31:40Z

keps/4136-admission-fair-sharing/README.md

+or following is possible then that workload is admitted, under condition that it might get preempted.
+
+3. AdmissionScope at Cohort level - Kueue operates in a mixed mode. Inside CQ workloads are
+selected according to their AdmissionMode (if specified). If a workload fits entirely into


Inside CQ workloads are selected according to their AdmissionMode (if specified)

Only highest AdmissionScope is used. Do you mean Queueing Policy (FIFO/BestEffort)?

The candidates inside individual CQs are selected based on the specified logic and bubbled up.

keps/4136-admission-fair-sharing/README.md

keps/4136-admission-fair-sharing/kep.yaml

keps/4136-admission-fair-sharing/README.md

keps/4136-admission-fair-sharing/kep.yaml

keps/4136-admission-fair-sharing/README.md

mimowo

First pass. For now trying to wrap my head around the overlap with the classical fair sharing, and the enablement scope. Also, I'm not entirely clear if we support reclamation in the new mode.

keps/4136-admission-fair-sharing/README.md

PBundyra · 2025-04-18T14:36:50Z

/lgtm

k8s-ci-robot · 2025-04-18T14:36:56Z

LGTM label has been added.

Git tree hash: 79e3e4ee380e8a269461ace0f105f414e8f9a438

mimowo · 2025-04-18T15:23:22Z

/lgtm
/approve
I think this is a very important feature to provide Fair Sharing based on past cumulative usage.

I'm not quite sure about the abstraction of "Admission Fair Sharing", but this is something we will need to figure out as we go.

k8s-ci-robot · 2025-04-18T15:23:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mimowo, mwielgus

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~~ [mimowo]

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

tenzen-y

/hold
Basically lgtm

one comment for API typed

keps/4136-admission-fair-sharing/README.md

tenzen-y · 2025-04-18T15:41:36Z

keps/4136-admission-fair-sharing/README.md

+* Modify CQ’s FairSharing struct with 
+
+```go
+type FairSharing struct {
+	// Weight denotes how important the given queue when competing against other queues 
+// for unused shared resources. The exact impact of the weight in fair share calculations
+// depends on the fair share algorithm used. Default = 1.
+	Weight *resource.Quantity `json:"weight,omitempty"`
+}
+```


IIUC, we already have this field

kueue/apis/kueue/v1beta1/clusterqueue_types.go

Lines 128 to 132 in 636d57a

// fairSharing defines the properties of the ClusterQueue when

// participating in FairSharing. The values are only relevant

// if FairSharing is enabled in the Kueue configuration.

// +optional

FairSharing *FairSharing `json:"fairSharing,omitempty"`

kueue/apis/kueue/v1beta1/fairsharing_types.go

Line 39 in 636d57a

Weight *resource.Quantity `json:"weight,omitempty"`

Yes, there is. I change the meaning a bit to be more generic.

I see. We will change only the meaning, and not change the API's looking. Thanks.

keps/4136-admission-fair-sharing/README.md

Co-authored-by: Yuki Iwai <[email protected]>

tenzen-y · 2025-04-18T15:50:25Z

keps/4136-admission-fair-sharing/README.md

+	// LastUpdate is the time when share and consumed resources were updated.
+	LastUpdate metav1.Time `json:"lastUpdate,omitempty"`


Why not use conditions on each resource instead of this dedicated LastUpdate?
In this approach, I think If we want to add any reason and message for FairShare, we need to add those condition similarity fields here.

Conditions have last transition, not last update.

// lastTransitionTime is the last time the condition transitioned from one status to another. // This should be when the underlying condition changed.

Here there is no transition, just updates.

I see. You mean this is the time when kueue takes a snapshot of consumed usage.
In that case, we probably want to say usageSnapshotAt, usageCalculationAt or something so that we can obviously clarify what is this time.

WDYT?

Personally I see LastUpdate time as more straightforward and potentially covering also WeightedShare or whatever else we add in the future. UsageSnapshotAt limits us, and if any new field is added then we will need another timestamp.

Co-authored-by: Yuki Iwai <[email protected]>

tenzen-y

Other than https://github.com/kubernetes-sigs/kueue/pull/4252/files#r2050817373

lgtm, thanks

mimowo · 2025-04-24T10:56:27Z

/lgtm
/unhold
To avoid blocking the implementation work on the design which has only minor questions ope.

@tenzen-y I believe we can address this later or during the implementation , and then we update KEP to align.

k8s-ci-robot · 2025-04-24T10:56:32Z

LGTM label has been added.

Git tree hash: 73d6215304b15875284d34eb036170d3f524c32f

gabesaba · 2025-04-24T11:30:56Z

/lgtm

tenzen-y · 2025-04-28T19:23:16Z

@tenzen-y I believe we can address this later or during the implementation , and then we update KEP to align.

That makes sense.
If I can see anything in the implementation PR, let me leave comments on the PR.

k8s-ci-robot requested review from PBundyra and gabesaba February 12, 2025 23:35

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 12, 2025

k8s-ci-robot assigned gabesaba and PBundyra Feb 13, 2025

gabesaba reviewed Feb 14, 2025

View reviewed changes

PBundyra reviewed Feb 17, 2025

View reviewed changes

keps/4136-admission-fair-sharing/README.md Show resolved Hide resolved

PBundyra reviewed Feb 17, 2025

View reviewed changes

keps/4136-admission-fair-sharing/README.md Outdated Show resolved Hide resolved

PBundyra reviewed Feb 17, 2025

View reviewed changes

keps/4136-admission-fair-sharing/README.md Outdated Show resolved Hide resolved

PBundyra reviewed Feb 17, 2025

View reviewed changes

keps/4136-admission-fair-sharing/README.md Show resolved Hide resolved

PBundyra reviewed Feb 17, 2025

View reviewed changes

keps/4136-admission-fair-sharing/README.md Show resolved Hide resolved

PBundyra reviewed Feb 17, 2025

View reviewed changes

keps/4136-admission-fair-sharing/kep.yaml Show resolved Hide resolved

PBundyra reviewed Feb 17, 2025

View reviewed changes

keps/4136-admission-fair-sharing/README.md Outdated Show resolved Hide resolved

PBundyra reviewed Feb 21, 2025

View reviewed changes

keps/4136-admission-fair-sharing/README.md Show resolved Hide resolved

mwielgus force-pushed the afs branch from 2e57c1c to 696cbc6 Compare February 24, 2025 15:18

KPostOffice reviewed Feb 24, 2025

View reviewed changes

keps/4136-admission-fair-sharing/README.md Show resolved Hide resolved

mwielgus force-pushed the afs branch from 696cbc6 to b0617c4 Compare February 26, 2025 10:54

PBundyra reviewed Mar 19, 2025

View reviewed changes

keps/4136-admission-fair-sharing/README.md Outdated Show resolved Hide resolved

PBundyra reviewed Mar 19, 2025

View reviewed changes

keps/4136-admission-fair-sharing/README.md Outdated Show resolved Hide resolved

PBundyra reviewed Mar 19, 2025

View reviewed changes

keps/4136-admission-fair-sharing/kep.yaml Outdated Show resolved Hide resolved

PBundyra reviewed Mar 19, 2025

View reviewed changes

keps/4136-admission-fair-sharing/kep.yaml Outdated Show resolved Hide resolved

PBundyra mentioned this pull request Mar 19, 2025

Introduce FS at admission time #4687

Merged

pajakd reviewed Mar 21, 2025

View reviewed changes

keps/4136-admission-fair-sharing/README.md Outdated Show resolved Hide resolved

mimowo reviewed Apr 8, 2025

View reviewed changes

KEP-4136: Admission Fair Sharing

9192bef

mwielgus force-pushed the afs branch from 6146f64 to 9192bef Compare April 18, 2025 14:05

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 18, 2025

k8s-ci-robot assigned mimowo Apr 18, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 18, 2025

tenzen-y reviewed Apr 18, 2025

View reviewed changes

keps/4136-admission-fair-sharing/README.md Outdated Show resolved Hide resolved

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 18, 2025

tenzen-y reviewed Apr 18, 2025

View reviewed changes

keps/4136-admission-fair-sharing/README.md Outdated Show resolved Hide resolved

Update keps/4136-admission-fair-sharing/README.md

350ccd4

Co-authored-by: Yuki Iwai <[email protected]>

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 18, 2025

k8s-ci-robot requested review from PBundyra, gabesaba and mimowo April 18, 2025 15:48

tenzen-y reviewed Apr 18, 2025

View reviewed changes

Update keps/4136-admission-fair-sharing/README.md

ba587fa

Co-authored-by: Yuki Iwai <[email protected]>

tenzen-y reviewed Apr 18, 2025

View reviewed changes

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Apr 24, 2025

k8s-ci-robot merged commit f61f8e9 into kubernetes-sigs:main Apr 24, 2025
7 checks passed

k8s-ci-robot added this to the v0.12 milestone Apr 24, 2025

gabesaba mentioned this pull request Aug 12, 2025

Self-Nominate gabesaba@ as Full Approver #6555

Merged

	// fairSharing defines the properties of the ClusterQueue when
	// participating in FairSharing. The values are only relevant
	// if FairSharing is enabled in the Kueue configuration.
	// +optional
	FairSharing *FairSharing `json:"fairSharing,omitempty"`

		// LastUpdate is the time when share and consumed resources were updated.
		LastUpdate metav1.Time `json:"lastUpdate,omitempty"`

KEP-4136: Admission Fair Sharing #4252

KEP-4136: Admission Fair Sharing #4252

Uh oh!

Conversation

mwielgus commented Feb 12, 2025

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?

Uh oh!

netlify bot commented Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Uh oh!

mimowo commented Feb 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mimowo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PBundyra commented Apr 18, 2025

Uh oh!

k8s-ci-robot commented Apr 18, 2025

Uh oh!

mimowo commented Apr 18, 2025

Uh oh!

k8s-ci-robot commented Apr 18, 2025

Uh oh!

tenzen-y left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tenzen-y Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

netlify bot commented Feb 12, 2025 •

edited

Loading

tenzen-y Apr 18, 2025 •

edited

Loading

tenzen-y Apr 18, 2025 •

edited

Loading