Migrate controllers to v1beta1 #604

alculquicondor · 2023-03-03T17:05:43Z

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

Uses the v1beta1 API throughout the code base and changes the storage version.
The code is functionally the same, but there are some differences:

Use of client.Status() when admitting or evicting workloads.
The concept of "codependent resources" disappears in favor of the API's resourceGroups.
The queue name in a Job can be specified as a label or annotation.

Makes use of APIs introduced in #532 (contained in the first two commits).

Which issue(s) this PR fixes:

Fixes #23

Special notes for your reviewer:

I heavily changed the testing wrappers in a attempt to make the test cases more slim.
The wrappers also add coveredResources automatically and panic if you define flavors with different resources.

To be done if follow ups:

Swap Admitted condition value when setting .status.admission #603
Documentation updates.
Remove v1alpha2

Change-Id: Ic4cc56f07e8ef5caa7b55ac02901a54708c35f20

Change-Id: I9a12e50eaa66c1897406b44e59f9e0813382645d

netlify · 2023-03-03T17:05:48Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`cfaf110`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/6408adbbb8915b00089939d4

k8s-ci-robot · 2023-03-03T17:05:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor

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]

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

alculquicondor · 2023-03-03T17:06:06Z

/hold
for all the eyes that want to have a look :)

cc @ahg-g

alculquicondor · 2023-03-03T20:53:57Z

/test pull-kueue-test-integration-main

ahg-g

Unbelievable effort, but hard to review :)

I think this could have been made easier to review if we introduced the changes to v1beta1 over different PRs (for example, first add v1beta1 as an exact copy of v1alpha2, then do the Workload Admission move, then the ResourceFlavor changes, then the ClusterQueue etc.), the wrappers refactoring could have also been introduced in a separate PR.

As for the logic itself, I wonder if we actually needed to make all those changes at once? maybe by copying the external type to the existing internal types we could have avoided doing all those changes in one go?

Regardless, since this doesn't intend to introduce new behavior, our only hope is that we have enough test coverage for validation :)

ahg-g · 2023-03-08T05:01:51Z

pkg/cache/cache.go

 	qKey := workload.QueueKey(wi.Obj)
 	if _, ok := c.admittedWorkloadsPerQueue[qKey]; ok {
 		c.admittedWorkloadsPerQueue[qKey] += int(m)
 	}
 }

-func updateUsage(wi *workload.Info, usedResources ResourceQuantities, m int64) {
+func updateUsage(wi *workload.Info, cqUsage FlavorResourceQuantities, m int64) {


make this a member function of ClusterQueue since it updates the Usage of a CQ?

No, because the snapshot also uses this function to update the usage of the cohort, not just the clusterQueue.

ahg-g · 2023-03-08T05:08:33Z

pkg/cache/cache.go

-				}
+			cqFlv, cqFlvExist := cqUsage[wlResFlv]
+			if cqFlvExist && wlResExist {
+				cqFlv[wlRes] += v * m


a similar check to the previous one is still applicable:

if _, exist := cqFlv[wlRes]; exist {

ahg-g · 2023-03-08T06:21:35Z

pkg/controller/workload/job/job_controller.go

+	v, ok := job.Labels[constants.QueueLabel]
+	if ok {


Suggested change

v, ok := job.Labels[constants.QueueLabel]

if ok {

if v, ok := job.Labels[constants.QueueLabel]; ok {

ahg-g · 2023-03-08T06:35:09Z

test/integration/webhook/clusterqueue_test.go

@@ -11,7 +11,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */

-package v1alpha2
+package webhook


why did we move them?

Maybe when there are conversion webhooks it'll make sense to the tests to the version, but for now it doesn't make much sense.

ahg-g · 2023-03-08T06:49:44Z

should we delete v1alpha2? it is orphaned now, right?

alculquicondor · 2023-03-08T13:35:39Z

I think this could have been made easier to review if we introduced the changes to v1beta1 over different PRs (for example, first add v1beta1 as an exact copy of v1alpha2, then do the Workload Admission move, then the ResourceFlavor changes, then the ClusterQueue etc.), the wrappers refactoring could have also been introduced in a separate PR.

Yeah, I was too far ahead with changes once I realized the PR was too big :(

The wrapping changes was an attempt to make it easier for me to re-write the unit tests.

As for the logic itself, I wonder if we actually needed to make all those changes at once? maybe by copying the external type to the existing internal types we could have avoided doing all those changes in one go?

There are no internal types in CRDs. Only a "storage version", which is v1beta1 now.

should we delete v1alpha2? it is orphaned now, right?

Next PR

ahg-g · 2023-03-08T15:02:49Z

There are no internal types in CRDs. Only a "storage version", which is v1beta1 now.

we have internal types in the code itself, not sure how much we use the api types directly though in the core logic.

ahg-g · 2023-03-08T15:03:01Z

/hold
/lgtm

tenzen-y

Thank you for the great work!

Maybe for a follow up, we must update the samples:

https://github.com/kubernetes-sigs/kueue/tree/main/config/samples

tenzen-y · 2023-03-08T01:05:11Z

Makefile

@@ -210,7 +210,7 @@ clean-manifests = (cd config/components/manager && $(KUSTOMIZE) edit set image c

 .PHONY: install
 install: manifests kustomize ## Install CRDs into the K8s cluster specified in ~/.kube/config.
-	$(KUSTOMIZE) build config/components/crd | kubectl apply -f -
+	$(KUSTOMIZE) build config/components/crd | kubectl apply --server-side -f -


Maybe for a follow-up, we need to update docs as well.

https://github.com/kubernetes-sigs/kueue/blob/cf76bfe02ae58e5005403821689592c6b967ce5a/site/content/en/docs/installation/_index.md#install-the-latest-development-version

Yes, that's next step :)

tenzen-y · 2023-03-08T15:18:04Z

pkg/controller/workload/job/job_controller.go

 }

 func queueName(job *batchv1.Job) string {
+	if v, ok := job.Labels[constants.QueueLabel]; ok {
+		return v
+	}
 	return job.Annotations[constants.QueueAnnotation]


Why take the queue name from annotations? For backward compatible?

alculquicondor · 2023-03-08T15:45:13Z

/hold cancel

alculquicondor · 2023-03-08T15:45:32Z

/hold
for squash

Change-Id: If389945ad5833d8b869823afa1816772af22663c

alculquicondor · 2023-03-08T15:46:14Z

squashed in two commits
/hold cancel

tenzen-y · 2023-03-08T15:49:05Z

/lgtm

tenzen-y · 2023-03-08T15:50:18Z

/lgtm

alculquicondor added 2 commits March 3, 2023 11:49

Copy v1alpha2 into v1beta1

4e3fff4

Change-Id: Ic4cc56f07e8ef5caa7b55ac02901a54708c35f20

v1beta1 API update proposals

6136eed

Change-Id: I9a12e50eaa66c1897406b44e59f9e0813382645d

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Mar 3, 2023

k8s-ci-robot requested review from denkensk and tenzen-y March 3, 2023 17:05

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 3, 2023

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

alculquicondor force-pushed the beta1-controllers branch 7 times, most recently from 03be22c to 28aa2ee Compare March 3, 2023 20:32

alculquicondor mentioned this pull request Mar 7, 2023

Allow borrowing while preemption is applied to assigned cluster queue #614

Merged

ahg-g reviewed Mar 8, 2023

View reviewed changes

k8s-ci-robot assigned ahg-g Mar 8, 2023

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

tenzen-y reviewed Mar 8, 2023

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 8, 2023

Migrate controllers to v1beta1 APIs

cfaf110

Change-Id: If389945ad5833d8b869823afa1816772af22663c

alculquicondor force-pushed the beta1-controllers branch from 2e9f12f to cfaf110 Compare March 8, 2023 15:46

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

alculquicondor mentioned this pull request Mar 8, 2023

Add v1beta1 API #532

Closed

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 8, 2023

k8s-ci-robot assigned tenzen-y Mar 8, 2023

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 8, 2023

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

k8s-ci-robot merged commit 1bb8e0d into kubernetes-sigs:main Mar 8, 2023

tenzen-y mentioned this pull request Mar 10, 2023

Will the CRD’s v1beta1 of the master branch change before release-0.3.0 is released? #621

Closed

tenzen-y mentioned this pull request May 17, 2024

[kueuectl] Pass-through commands. #2181

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate controllers to v1beta1 #604

Migrate controllers to v1beta1 #604

alculquicondor commented Mar 3, 2023 •

edited

Loading

netlify bot commented Mar 3, 2023 •

edited

Loading

k8s-ci-robot commented Mar 3, 2023

alculquicondor commented Mar 3, 2023

alculquicondor commented Mar 3, 2023

ahg-g left a comment •

edited

Loading

ahg-g Mar 8, 2023

alculquicondor Mar 8, 2023

ahg-g Mar 8, 2023

alculquicondor Mar 8, 2023

ahg-g Mar 8, 2023

alculquicondor Mar 8, 2023

ahg-g Mar 8, 2023

alculquicondor Mar 8, 2023

ahg-g commented Mar 8, 2023

alculquicondor commented Mar 8, 2023 •

edited

Loading

ahg-g commented Mar 8, 2023

ahg-g commented Mar 8, 2023

tenzen-y left a comment

tenzen-y Mar 8, 2023

alculquicondor Mar 8, 2023

tenzen-y Mar 8, 2023

alculquicondor Mar 8, 2023

tenzen-y Mar 8, 2023

alculquicondor commented Mar 8, 2023

alculquicondor commented Mar 8, 2023

alculquicondor commented Mar 8, 2023

tenzen-y commented Mar 8, 2023

tenzen-y commented Mar 8, 2023

	v, ok := job.Labels[constants.QueueLabel]
	if ok {
	if v, ok := job.Labels[constants.QueueLabel]; ok {

Migrate controllers to v1beta1 #604

Migrate controllers to v1beta1 #604

Conversation

alculquicondor commented Mar 3, 2023 • 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:

netlify bot commented Mar 3, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

k8s-ci-robot commented Mar 3, 2023

alculquicondor commented Mar 3, 2023

alculquicondor commented Mar 3, 2023

ahg-g left a comment • 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

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

ahg-g commented Mar 8, 2023

alculquicondor commented Mar 8, 2023 • edited Loading

ahg-g commented Mar 8, 2023

ahg-g commented Mar 8, 2023

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

alculquicondor commented Mar 8, 2023

alculquicondor commented Mar 8, 2023

alculquicondor commented Mar 8, 2023

tenzen-y commented Mar 8, 2023

tenzen-y commented Mar 8, 2023

alculquicondor commented Mar 3, 2023 •

edited

Loading

netlify bot commented Mar 3, 2023 •

edited

Loading

ahg-g left a comment •

edited

Loading

alculquicondor commented Mar 8, 2023 •

edited

Loading