Add FairSharing config to Cohort #4288

gabesaba · 2025-02-17T14:02:28Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Add FairSharing to Cohort API. Update shared structure and respective documentation documentation which are used now by both Cohort and ClusterQueue.

Updating cache and scheduling logic to use this change will be done in subsequent PRs, to keep PR size managable.

Which issue(s) this PR fixes:

Part of #3759

Special notes for your reviewer:

No release note for this change, as we will do a single release note for the entire feature.

Does this PR introduce a user-facing change?

NONE

netlify · 2025-02-17T14:02:49Z

✅ Deploy Preview for kubernetes-sigs-kueue ready!

Name	Link
🔨 Latest commit	`6c74150`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/67b4aae54f08140008c81aff
😎 Deploy Preview	https://deploy-preview-4288--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 configuration.

PBundyra · 2025-02-17T20:07:38Z

pkg/util/testing/wrappers.go

+	if c.Spec.FairSharing == nil {
+		c.Spec.FairSharing = &kueue.FairSharing{}
+	}
+	c.Spec.FairSharing.Weight = ptr.To(w)


Suggested change

c.Spec.FairSharing.Weight = ptr.To(w)

c.Spec.FairSharing.Weight = &w

Done, also for ClusterQueue.

PBundyra · 2025-02-17T20:07:59Z

/lgtm

k8s-ci-robot · 2025-02-17T20:08:05Z

LGTM label has been added.

Git tree hash: 973952d7333724994df200d98cb0b3c3012bd58c

tenzen-y

Thanks, mostly lgtm

tenzen-y · 2025-02-17T20:04:33Z

apis/kueue/v1beta1/fair_sharing.go

Could you rename this to fairsharing.go? Acutally, we use localqueue_type.go not local_queue_type.go.

Or maybe even better to fairsharing_types.go

tenzen-y · 2025-02-17T20:05:05Z

apis/kueue/v1alpha1/cohort_types.go

@@ -60,6 +60,11 @@ type CohortSpec struct {
 	//+listType=atomic
 	//+kubebuilder:validation:MaxItems=16
 	ResourceGroups []kueuebeta.ResourceGroup `json:"resourceGroups,omitempty"`
+
+	// fairSharing defines the properties of the Cohort when
+	// participating in FairSharing.  The values are only relevant


Suggested change

// participating in FairSharing. The values are only relevant

// participating in FairSharing. The values are only relevant

tenzen-y · 2025-02-17T20:05:53Z

apis/kueue/v1alpha1/cohort_types.go

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


NOTE

This matches to design proposal:

kueue/keps/1714-fair-sharing/README.md

Lines 222 to 228 in 06d4c76

Weights will be added to ClusterQueueSpec and CohortSpec in the following optional struct:

```go

type struct FairSharing {

Weight *Quantity // Default is 1.

}

```

tenzen-y · 2025-02-17T20:13:16Z

test/integration/singlecluster/webhook/core/cohort_test.go

I think that we can move almost cases to UTs to check fingrained rejected reason and improve tests duration.
For integration tests, we can just have one valid and one invalid case each for CRUD operation.
But that is out of the scope of this PR.

Would you mind working on that in a follow-up?

I remember thinking about this when implementing Cohort webhook - the issue is that we are doing some validation using kubebuilder, which I don't think we can exercise during unit tests. Also in ClusterQueue; e.g.

kueue/apis/kueue/v1beta1/clusterqueue_types.go

Line 43 in 4c8484e

// +kubebuilder:validation:XValidation:rule="!has(self.cohort) && has(self.resourceGroups) ? self.resourceGroups.all(rg, rg.flavors.all(f, f.resources.all(r, !has(r.borrowingLimit)))) : true", message="borrowingLimit must be nil when cohort is empty"

Yeah, the CEL should be verified in integration testing. But for webhook cases, we can move those to UTs.
That would be useful to reduce long-running CI and testing.

PBundyra · 2025-02-18T15:10:31Z

/lgtm

k8s-ci-robot · 2025-02-18T15:10:38Z

LGTM label has been added.

Git tree hash: c69d8cc6c1fa877dab7109978cc3177e201099db

tenzen-y

one nit

tenzen-y · 2025-02-18T15:23:20Z

apis/kueue/v1alpha1/cohort_types.go

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


Suggested change

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

// +optional

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

Indeed, we forget to add +optional marker when we introduce fairSharing.

tenzen-y · 2025-02-18T15:24:09Z

apis/kueue/v1beta1/clusterqueue_types.go

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


Could you address +optional marker for CQ in a follow-up PR, and then cherry-pick that to release branch?

I will create pull request once this PR merges, so that I do not need to resolve conflict

Thanks!
/lgtm
/approve

k8s-ci-robot · 2025-02-18T15:46:37Z

LGTM label has been added.

Git tree hash: 39af0a096975888e4db470d4f577be12b9b61709

k8s-ci-robot · 2025-02-18T15:46:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gabesaba, 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~~ [tenzen-y]

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

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. labels Feb 17, 2025

k8s-ci-robot requested review from mimowo and PBundyra February 17, 2025 14:02

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 17, 2025

gabesaba force-pushed the cohort_api_fair_sharing branch 2 times, most recently from 6fc9f4e to 23da91e Compare February 17, 2025 14:29

PBundyra reviewed Feb 17, 2025

View reviewed changes

k8s-ci-robot assigned PBundyra Feb 17, 2025

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

tenzen-y reviewed Feb 17, 2025

View reviewed changes

gabesaba force-pushed the cohort_api_fair_sharing branch from 23da91e to 4b2237c Compare February 18, 2025 11:17

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

k8s-ci-robot requested a review from PBundyra February 18, 2025 11:17

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

tenzen-y reviewed Feb 18, 2025

View reviewed changes

Add FairSharing config to Cohort

6c74150

gabesaba force-pushed the cohort_api_fair_sharing branch from 4b2237c to 6c74150 Compare February 18, 2025 15:44

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

k8s-ci-robot assigned tenzen-y Feb 18, 2025

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

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

k8s-ci-robot merged commit 8b4affc into kubernetes-sigs:main Feb 18, 2025
19 checks passed

k8s-ci-robot added this to the v0.11 milestone Feb 18, 2025

gabesaba deleted the cohort_api_fair_sharing branch February 18, 2025 17:25

gabesaba mentioned this pull request Feb 18, 2025

Make FairSharing optional in ClusterQueue #4304

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FairSharing config to Cohort #4288

Add FairSharing config to Cohort #4288

gabesaba commented Feb 17, 2025

netlify bot commented Feb 17, 2025 •

edited

Loading

PBundyra Feb 17, 2025

gabesaba Feb 18, 2025

PBundyra commented Feb 17, 2025

k8s-ci-robot commented Feb 17, 2025

tenzen-y left a comment

tenzen-y Feb 17, 2025

PBundyra Feb 17, 2025

gabesaba Feb 18, 2025

tenzen-y Feb 17, 2025

gabesaba Feb 18, 2025

tenzen-y Feb 17, 2025

tenzen-y Feb 17, 2025

gabesaba Feb 18, 2025

tenzen-y Feb 18, 2025

PBundyra commented Feb 18, 2025

k8s-ci-robot commented Feb 18, 2025

tenzen-y left a comment

tenzen-y Feb 18, 2025

gabesaba Feb 18, 2025

tenzen-y Feb 18, 2025 •

edited

Loading

gabesaba Feb 18, 2025

gabesaba Feb 18, 2025

tenzen-y Feb 18, 2025

gabesaba Feb 19, 2025

k8s-ci-robot commented Feb 18, 2025

k8s-ci-robot commented Feb 18, 2025

	c.Spec.FairSharing.Weight = ptr.To(w)
	c.Spec.FairSharing.Weight = &w

	// participating in FairSharing. The values are only relevant
	// participating in FairSharing. The values are only relevant

	Weights will be added to ClusterQueueSpec and CohortSpec in the following optional struct:

	```go
	type struct FairSharing {
	Weight *Quantity // Default is 1.
	}
	```

	FairSharing *kueuebeta.FairSharing `json:"fairSharing,omitempty"`
	// +optional
	FairSharing *kueuebeta.FairSharing `json:"fairSharing,omitempty"`

Add FairSharing config to Cohort #4288

Add FairSharing config to Cohort #4288

Conversation

gabesaba commented Feb 17, 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?

netlify bot commented Feb 17, 2025 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PBundyra commented Feb 17, 2025

k8s-ci-robot commented Feb 17, 2025

tenzen-y left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PBundyra commented Feb 18, 2025

k8s-ci-robot commented Feb 18, 2025

tenzen-y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenzen-y Feb 18, 2025 • 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

k8s-ci-robot commented Feb 18, 2025

k8s-ci-robot commented Feb 18, 2025

netlify bot commented Feb 17, 2025 •

edited

Loading

tenzen-y Feb 18, 2025 •

edited

Loading