[multikueue] Use batch/Job `spec.managedBy` field. #2331

trasc · 2024-05-30T14:42:42Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Use spec.managedBy to detect delegatable jobs.
Enable live status updates for batch/Jobs.
No longer keep the multikueue admission check state Pending when the job is running in a worker cluster.

Which issue(s) this PR fixes:

Relates to #693

Special notes for your reviewer:

Does this PR introduce a user-facing change?

MultiKueue: Use batch/Job `spec.managedBy` field

netlify · 2024-05-30T14:43:05Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`359134f`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/66605e66018a98000870a75b

alculquicondor · 2024-06-03T18:41:31Z

/assign @mimowo

Makefile-test.mk

hack/multikueue/manager-cluster.kind-1.30.0.yaml

pkg/controller/admissionchecks/multikueue/batchjob_adapter.go

pkg/controller/admissionchecks/multikueue/workload_test.go

trasc · 2024-06-04T15:12:34Z

@alculquicondor @mimowo @mwielgus

We could also keep the old integration approach and put the spec.managedBy support under kueue featuregate (say MultiKueueBetchJobWithManage) WDYT?

mimowo · 2024-06-04T15:27:32Z

@alculquicondor @mimowo @mwielgus

We could also keep the old integration approach and put the spec.managedBy support under kueue featuregate (say MultiKueueBetchJobWithManage) WDYT?

+1 sounds like a good idea.

It will let users configure kueue according to their k8s configuration. Yet, we will not introduce API configuration which may turn out unnecessary over time.

tenzen-y · 2024-06-05T07:33:21Z

@trasc Thank you for responding to my questions. It seems that I missed reading codes somehow.
Now, everything is clear. So feel free to close my thread.

tenzen-y · 2024-06-05T07:33:48Z

@alculquicondor @mimowo @mwielgus

We could also keep the old integration approach and put the spec.managedBy support under kueue featuregate (say MultiKueueBetchJobWithManage) WDYT?

SGTM

pkg/features/kube_features.go

pkg/controller/admissionchecks/multikueue/batchjob_adapter.go

test/integration/multikueue/multikueue_test.go

pkg/controller/admissionchecks/multikueue/workload.go

Use `spec.managedBy` to detect delegatable jobs. Enable live status updates for batch/Jobs. No longer keep the multikueue admission check state `Pending` when the job is running in a worker cluster.

mimowo

LGTM, one naming nit from me.

I think we should also have defaulting of managedBy field for Jobs, as we do for JobSet. when the feature gate is enabled.

@trasc would you like to extend this PR or follow up?

pkg/features/kube_features.go

trasc · 2024-06-05T11:35:43Z

I think we should also have defaulting of managedBy field for Jobs, as we do for JobSet. when the feature gate is enabled.

@trasc would you like to extend this PR or follow up?

Foillow-up I think @mbobrovskyi has some experience around this topic.

mimowo · 2024-06-05T11:53:44Z

pkg/features/kube_features.go

+	PrioritySortingWithinCohort:    {Default: true, PreRelease: featuregate.Beta},
+	MultiKueue:                     {Default: false, PreRelease: featuregate.Alpha},
+	LendingLimit:                   {Default: false, PreRelease: featuregate.Alpha},
+	MultiKueueBatchJobWithManagadBy: {Default: false, PreRelease: featuregate.Alpha},


Suggested change

MultiKueueBatchJobWithManagadBy: {Default: false, PreRelease: featuregate.Alpha},

MultiKueueBatchJobWithManagedBy: {Default: false, PreRelease: featuregate.Alpha},

almost there :)

mimowo · 2024-06-05T13:05:42Z

/lgtm
/approve

k8s-ci-robot · 2024-06-05T13:05:47Z

LGTM label has been added.

Git tree hash: c5295e560277a3d5f7d1a565c48a78ec03f90d2e

k8s-ci-robot · 2024-06-05T13:05:50Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

) * [multikueue] Use batch/Job `spec.managedBy` field. Use `spec.managedBy` to detect delegatable jobs. Enable live status updates for batch/Jobs. No longer keep the multikueue admission check state `Pending` when the job is running in a worker cluster. * Review remarks * [multikueue] Add `MultiKueueBatchJobWithManageBy` featuregate * Review remarks * Review remarks * Fix linter issues.

alculquicondor · 2024-07-18T20:52:30Z

/release-note-edit

MultiKueue: Use batch/Job `spec.managedBy` field

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels May 30, 2024

k8s-ci-robot requested review from kerthcet and mimowo May 30, 2024 14:42

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

k8s-ci-robot assigned mimowo Jun 3, 2024

tenzen-y mentioned this pull request Jun 4, 2024

Provide a way to implement a MultiKueue support for the custom Jobs #2349

Open

5 tasks

tenzen-y reviewed Jun 4, 2024

View reviewed changes

mimowo reviewed Jun 4, 2024

View reviewed changes

pkg/controller/admissionchecks/multikueue/workload_test.go Outdated Show resolved Hide resolved

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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 Jun 5, 2024

trasc force-pushed the multikueue-job-managed-by branch from 5387923 to 1ce0cf4 Compare June 5, 2024 06:50

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

mimowo reviewed Jun 5, 2024

View reviewed changes

pkg/features/kube_features.go Outdated Show resolved Hide resolved

trasc force-pushed the multikueue-job-managed-by branch from 1ce0cf4 to e1ded24 Compare June 5, 2024 08:25

tenzen-y reviewed Jun 5, 2024

View reviewed changes

mimowo reviewed Jun 5, 2024

View reviewed changes

pkg/controller/admissionchecks/multikueue/workload.go Outdated Show resolved Hide resolved

trasc added 4 commits June 5, 2024 13:13

[multikueue] Use batch/Job spec.managedBy field.

39ada6f

Use `spec.managedBy` to detect delegatable jobs. Enable live status updates for batch/Jobs. No longer keep the multikueue admission check state `Pending` when the job is running in a worker cluster.

Review remarks

56c40a4

[multikueue] Add MultiKueueBatchJobWithManageBy featuregate

0126b2e

Review remarks

cbb7107

trasc force-pushed the multikueue-job-managed-by branch from e1ded24 to cbb7107 Compare June 5, 2024 10:34

mimowo reviewed Jun 5, 2024

View reviewed changes

pkg/features/kube_features.go Outdated Show resolved Hide resolved

mimowo reviewed Jun 5, 2024

View reviewed changes

Review remarks

0486135

trasc force-pushed the multikueue-job-managed-by branch from 8beb725 to 0486135 Compare June 5, 2024 12:10

Fix linter issues.

359134f

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

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

k8s-ci-robot merged commit d49cb51 into kubernetes-sigs:main Jun 5, 2024
16 checks passed

k8s-ci-robot added this to the v0.8 milestone Jun 5, 2024

trasc deleted the multikueue-job-managed-by branch June 5, 2024 13:14

vladikkuzn mentioned this pull request Jun 11, 2024

[MultiKueue] Default the managedBy field for batch Jobs managed by MultiKueue #2401

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[multikueue] Use batch/Job `spec.managedBy` field. #2331

[multikueue] Use batch/Job `spec.managedBy` field. #2331

trasc commented May 30, 2024 •

edited by k8s-ci-robot

Loading

netlify bot commented May 30, 2024 •

edited

Loading

alculquicondor commented Jun 3, 2024

trasc commented Jun 4, 2024 •

edited

Loading

mimowo commented Jun 4, 2024

tenzen-y commented Jun 5, 2024

tenzen-y commented Jun 5, 2024

mimowo left a comment

trasc commented Jun 5, 2024

mimowo Jun 5, 2024

trasc Jun 5, 2024

mimowo commented Jun 5, 2024

k8s-ci-robot commented Jun 5, 2024

k8s-ci-robot commented Jun 5, 2024

alculquicondor commented Jul 18, 2024

	MultiKueueBatchJobWithManagadBy: {Default: false, PreRelease: featuregate.Alpha},
	MultiKueueBatchJobWithManagedBy: {Default: false, PreRelease: featuregate.Alpha},

[multikueue] Use batch/Job spec.managedBy field. #2331

[multikueue] Use batch/Job spec.managedBy field. #2331

Conversation

trasc commented May 30, 2024 • edited by k8s-ci-robot 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 May 30, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

alculquicondor commented Jun 3, 2024

trasc commented Jun 4, 2024 • edited Loading

mimowo commented Jun 4, 2024

tenzen-y commented Jun 5, 2024

tenzen-y commented Jun 5, 2024

mimowo left a comment

Choose a reason for hiding this comment

trasc commented Jun 5, 2024

mimowo Jun 5, 2024

Choose a reason for hiding this comment

trasc Jun 5, 2024

Choose a reason for hiding this comment

mimowo commented Jun 5, 2024

k8s-ci-robot commented Jun 5, 2024

k8s-ci-robot commented Jun 5, 2024

alculquicondor commented Jul 18, 2024

[multikueue] Use batch/Job `spec.managedBy` field. #2331

[multikueue] Use batch/Job `spec.managedBy` field. #2331

trasc commented May 30, 2024 •

edited by k8s-ci-robot

Loading

netlify bot commented May 30, 2024 •

edited

Loading

trasc commented Jun 4, 2024 •

edited

Loading