[Hierarchical Cohorts] Define Cohort API #2693

gabesaba · 2024-07-25T08:52:34Z

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

Define Cohort API #79

Special notes for your reviewer:

Webhook and Reconciliation logic will be defined in follow-up PRs, to keep this PR small.

Does this PR introduce a user-facing change?

Hierarchical Cohorts, introduced with the v1alpha1 Cohorts API, allow users to group resources in an arbitrary tree structure. Additionally, quotas and limits can now be defined directly at the Cohort level. See #79 for more details.

netlify · 2024-07-25T08:52:49Z

✅ Deploy Preview for kubernetes-sigs-kueue ready!

Name	Link
🔨 Latest commit	`28c785b`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/66a248cb1668a4000953f88c
😎 Deploy Preview	https://deploy-preview-2693--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.

mimowo · 2024-07-25T09:05:36Z

apis/kueue/v1alpha1/cohort_types.go

+	//+kubebuilder:validation:MaxLength=253
+	//+kubebuilder:validation:Pattern="^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$"
+	//
+	Parent string `json:"parent,omitempty"`


This probably should be a pointer, right?

CQ also defines like this

kueue/apis/kueue/v1beta1/clusterqueue_types.go

Line 54 in cb92b42

Cohort string `json:"cohort,omitempty"`

I see, an empty string plays the role of no parent. That's fine, consistency is important.

mimowo · 2024-07-25T09:07:08Z

apis/kueue/v1alpha1/cohort_types.go

+//+kubebuilder:resource:scope=Cluster
+
+// Cohort is the Schema for the cohorts API
+type Cohort struct {


I think I would prefer to have some prototype or at least minimal functionality implemented before merging the API. It would increase the confidence in the API. Still, we can merge it as a separate PR, but would be good to see the implementation more on the horizon. WDYT @tenzen-y ?

That is possible, but my intention was to keep the PRs as small as possible for reviewability. Also, this is in v1alpha1, and not yet cut into a minor release, so I'd argue we can change it freely for some time.

I would like to select either approach

a single big bung PR containing all implementations and APIs.

multiple small PRs, but API changes PR are merged in the final phase.

TBH, I would prefer to opt 2 since it's challenging to review the opt 1 PR.
In the case of opt 2, we should expose APIs in the last phase since it is better not to expose the unimplemented APIs.

I will need this type for development of the rest of the features - in this case, should I just define it in pkg for now, and then move it to api later?

... my intention was to keep the PRs as small as possible for reviewability.

That's for sure, I was also thinking about merging this PR separately, but once seeing some PoC implementation to increase the confidence the API can be released.

Also, this is in v1alpha1

Good point, and the first iteration of the API looks quite minimal

The api was discussed and merged in https://github.com/kubernetes-sigs/kueue/tree/main/keps/79-hierarchical-cohorts. I'm not sure if adding basic implementation to this PR (that has very little to do with the API - it is mostly about scheduling and fitting the workloads) would make it more convincing.

I guess that you can learn how we should go here: #1714

Yeah, with the caveat that fair sharing for API only introduced fields rather than API. For a feature which requires new API it might be harder to develop it's logic without that API merged.

The api was discussed and merged in https://github.com/kubernetes-sigs/kueue/tree/main/keps/79-hierarchical-cohorts.

Indeed, together with the fact this is still alpha API to reduce the burden of rebases I would be leaning to merge it.

The only downside is that in case we need to release 0.9 urgently we will release hollow alpha API.Are we good with this @mwielgus @tenzen-y ?

The only downside is that in case we need to release 0.9 urgently we will release hollow alpha API.Are we good with this @mwielgus @tenzen-y ?

That is my primary concern, as I mentioned above.

In the case of opt 2, we should expose APIs in the last phase since it is better not to expose the unimplemented APIs.

In the case of urgently minor release, let's revert all PRs related to Hierarchical Cohorts...
I hope that we never face the situation...

In the case of urgently minor release, let's revert all PRs related to Hierarchical Cohorts... I hope that we never face the situation...

I guess it will be dependent on the completeness of the feature at the moment of releasing 0.9.

I synced with @gabesaba and we are ok to rollback the PRs related to the new API if the feature is still vastly unfinished when doing 0.9.

mwielgus · 2024-07-25T10:05:12Z

apis/kueue/v1alpha1/cohort_types.go

+// CohortSpec defines the desired state of Cohort
+type CohortSpec struct {
+	// Parent references the name of the Cohort's parent, if
+	// any. It satisfies one of three cases:


What about a cycle. What happens in that case?

We disable all members of the Cohort graph. I updated documentation.

mwielgus · 2024-07-25T10:08:15Z

apis/kueue/v1alpha1/cohort_types.go

+	//
+	// BorrowingLimit limits how much members of this Cohort
+	// subtree can borrow from the parent subtree. This limit must
+	// only be set when the Cohort has a parent.


What happens otherwise? We catch it at validation phase, invalidate the cohort or let it be?

This will be validated by the webhook, and we will reject the create/update. Updated the documentation.

mwielgus · 2024-07-25T10:14:04Z

apis/kueue/v1alpha1/cohort_types.go

+}
+
+// CohortStatus defines the observed state of Cohort
+type CohortStatus struct {


Do you expect it to be left empty? If yes - drop the struct for now. If not - please add the expected content.

Added the Conditions field, and the CohortActive field. One note: I modified the condition from the KEP slightly, to match ClusterQueue

KEP

CohortActive = "CohortActive"

This PR

CohortActive = "Active"

mimowo · 2024-07-25T14:37:33Z

/lgtm
/approve
Based on the discussion in #2693 (comment)
I believe all comments are addressed, if you have more remarks @tenzen-y or @mwielgus we can address in a follow up. I believe the API will make it easier for @gabesaba in the meanwhile to work on the changes on top of it.

k8s-ci-robot · 2024-07-25T14:37:39Z

LGTM label has been added.

Git tree hash: ccee3899d6dcb722430ab44b1a0a1b7f90a9ffe5

k8s-ci-robot · 2024-07-25T14:37:43Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

gabesaba · 2024-07-26T07:42:32Z

thanks for the reviews!

gabesaba · 2024-09-20T15:32:03Z

/release-note-edit

Hierarchical Cohorts, introduced with the v1alpha1 Cohorts API, allow users to group resources in an arbitrary tree structure. Additionally, quotas and limits can now be defined directly at the Cohort level. See #79 for more details.

k8s-ci-robot requested review from mimowo and trasc July 25, 2024 08:52

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 25, 2024

gabesaba force-pushed the cohort_api branch from 313ceea to fef1cc1 Compare July 25, 2024 08:55

mimowo reviewed Jul 25, 2024

View reviewed changes

gabesaba force-pushed the cohort_api branch from fef1cc1 to 7f9e162 Compare July 25, 2024 09:22

mwielgus reviewed Jul 25, 2024

View reviewed changes

Define Cohort API

28c785b

gabesaba force-pushed the cohort_api branch from 7f9e162 to 28c785b Compare July 25, 2024 12:44

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 Jul 25, 2024

k8s-ci-robot assigned mimowo Jul 25, 2024

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 25, 2024

k8s-ci-robot merged commit 176e1dd into kubernetes-sigs:main Jul 25, 2024
16 checks passed

k8s-ci-robot added this to the v0.9 milestone Jul 25, 2024

gabesaba deleted the cohort_api branch July 26, 2024 07:42

mbobrovskyi mentioned this pull request Aug 16, 2024

[kueuectl] Added kueuectl list pods command #2280

Merged

trasc mentioned this pull request Sep 5, 2024

[config] Add cohorts CRD in the kustomization #2995

Merged

alculquicondor mentioned this pull request Sep 5, 2024

Implement Cohort Webhook #2982

Merged

gabesaba mentioned this pull request Sep 20, 2024

Support Hierarchical Cohorts in Cache/Snapshot #3006

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Hierarchical Cohorts] Define Cohort API #2693

[Hierarchical Cohorts] Define Cohort API #2693

gabesaba commented Jul 25, 2024 •

edited by k8s-ci-robot

Loading

netlify bot commented Jul 25, 2024 •

edited

Loading

mimowo Jul 25, 2024

gabesaba Jul 25, 2024

mimowo Jul 25, 2024

mimowo Jul 25, 2024

gabesaba Jul 25, 2024

tenzen-y Jul 25, 2024

gabesaba Jul 25, 2024

mimowo Jul 25, 2024

mwielgus Jul 25, 2024

mimowo Jul 25, 2024

mimowo Jul 25, 2024

tenzen-y Jul 25, 2024

mimowo Jul 25, 2024

mwielgus Jul 25, 2024

gabesaba Jul 25, 2024

mwielgus Jul 25, 2024

gabesaba Jul 25, 2024

mwielgus Jul 25, 2024

gabesaba Jul 25, 2024

mimowo commented Jul 25, 2024

k8s-ci-robot commented Jul 25, 2024

k8s-ci-robot commented Jul 25, 2024

gabesaba commented Jul 26, 2024

gabesaba commented Sep 20, 2024

[Hierarchical Cohorts] Define Cohort API #2693

[Hierarchical Cohorts] Define Cohort API #2693

Conversation

gabesaba commented Jul 25, 2024 • edited by k8s-ci-robot Loading

What type of PR is this?

What this PR does / why we need it:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

netlify bot commented Jul 25, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue ready!

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

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

mimowo commented Jul 25, 2024

k8s-ci-robot commented Jul 25, 2024

k8s-ci-robot commented Jul 25, 2024

gabesaba commented Jul 26, 2024

gabesaba commented Sep 20, 2024

gabesaba commented Jul 25, 2024 •

edited by k8s-ci-robot

Loading

netlify bot commented Jul 25, 2024 •

edited

Loading