[jobframework] Deployment integration #2813

vladikkuzn · 2024-08-11T01:05:39Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Adds deployment integration to job framework

Which issue(s) this PR fixes:

Fixes #2717

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Add integration for Deployment, where each Pod is treated as a separate Workload.

k8s-ci-robot · 2024-08-11T01:05:41Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

vladikkuzn · 2024-08-11T01:05:55Z

/assign

netlify · 2024-08-11T01:06:00Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`62d3ae3`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/66d1d2e3af3c7500082a6ee4

pkg/controller/jobs/noop/noop_controller.go

pkg/controller/jobs/deployment/deployment_controller.go

pkg/controller/jobs/noop/noop_controller.go

pkg/controller/jobs/deployment/deployment_controller.go

pkg/controller/jobs/deployment/deployment_webhook.go

vladikkuzn · 2024-08-15T09:03:57Z

/cc @alculquicondor

alculquicondor · 2024-08-16T17:09:43Z

/release-note-edit

Add integration for Deployment, where each Pod is treated as a separate Workload.

apis/config/v1beta1/configuration_types.go

cmd/kueue/main.go

pkg/controller/jobframework/setup.go

cmd/kueue/main.go

pkg/controller/jobs/deployment/deployment_webhook.go

pkg/controller/jobs/noop/noop_controller.go

pkg/controller/jobs/noop/noop_webhook.go

pkg/controller/jobs/noop/noop_controller.go

pkg/controller/jobs/deployment/deployment_controller.go

* Set up deployment integration in main.go

* Move noop to jobframework

* Noop renamings * Remove deployment integration options

pkg/controller/jobs/deployment/deployment_webhook.go

test/integration/controller/jobs/deployment/deployment_webhook_test.go

* Remove _ * Rename webhook endpoints * Deployment integration test

tenzen-y

Basically, lgtm

pkg/controller/jobs/deployment/deployment_webhook.go

* skip cq label setting if not set

tenzen-y

Mostly lgtm.
Just some nits.

@alculquicondor Do you have any other comments?

test/integration/controller/jobs/deployment/deployment_webhook_test.go

tenzen-y · 2024-08-30T10:04:52Z

test/integration/controller/jobs/deployment/deployment_webhook_test.go

+				jobframework.WithKubeServerVersion(serverVersionFetcher),
+				jobframework.WithIntegrationOptions(
+					corev1.SchemeGroupVersion.WithKind("Pod").String(),
+					&configapi.PodIntegrationOptions{},


Suggested change

&configapi.PodIntegrationOptions{},

Is there any reason why we need to set empty PodIntegrationOptions?
If not, let's remove them.

It's mandatory

Can you just not call WithIntegrationOptions?

I guess you can't because of this

kueue/pkg/controller/jobs/pod/pod_webhook.go

Line 94 in 243e572

return configapi.PodIntegrationOptions{}, errPodOptsNotFound

I guess we should change that function to work with an empty struct, but we can leave that for a follow up.

/lgtm
/approve

PR was opened: #2945

* remove error from eventually

k8s-ci-robot · 2024-08-30T14:33:40Z

LGTM label has been added.

Git tree hash: bcb04fc0aa33e796adbb2e95e1b2fbaa55ac387a

tenzen-y

/lgtm
/approve

/hold cancel

k8s-ci-robot · 2024-08-30T16:19:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, tenzen-y, vladikkuzn

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,tenzen-y]

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

* [jobframework] Deployment integration * [jobframework] Deployment integration * Set up deployment integration in main.go * [jobframework] Deployment integration * Move noop to jobframework * [jobframework] Deployment integration * Noop renamings * Remove deployment integration options * [jobframework] Deployment integration * Remove _ * Rename webhook endpoints * Deployment integration test * [jobframework] Deployment integration * skip cq label setting if not set * [jobframework] Deployment integration * remove error from eventually

k8s-ci-robot requested review from denkensk and mimowo August 11, 2024 01:05

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

k8s-ci-robot assigned vladikkuzn Aug 11, 2024

vladikkuzn force-pushed the serving-deployment branch from 1a49194 to 2ebfdea Compare August 12, 2024 09:37

trasc reviewed Aug 13, 2024

View reviewed changes

vladikkuzn force-pushed the serving-deployment branch 2 times, most recently from fb81c75 to 00ae860 Compare August 14, 2024 09:35

vladikkuzn marked this pull request as ready for review August 14, 2024 09:35

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 14, 2024

vladikkuzn force-pushed the serving-deployment branch from 00ae860 to a8486e9 Compare August 14, 2024 11:35

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 Aug 14, 2024

vladikkuzn force-pushed the serving-deployment branch from a8486e9 to 13946f5 Compare August 14, 2024 21:28

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 14, 2024

k8s-ci-robot requested a review from alculquicondor August 15, 2024 09:03

alculquicondor reviewed Aug 16, 2024

View reviewed changes

vladikkuzn force-pushed the serving-deployment branch from 48f494f to e0080bf Compare August 18, 2024 23:18

alculquicondor reviewed Aug 19, 2024

View reviewed changes

pkg/controller/jobs/noop/noop_controller.go Outdated Show resolved Hide resolved

alculquicondor reviewed Aug 19, 2024

View reviewed changes

pkg/controller/jobs/deployment/deployment_controller.go Outdated Show resolved Hide resolved

vladikkuzn added 3 commits August 29, 2024 02:21

[jobframework] Deployment integration

a171ce6

* Set up deployment integration in main.go

[jobframework] Deployment integration

1c416f6

* Move noop to jobframework

[jobframework] Deployment integration

32082eb

* Noop renamings * Remove deployment integration options

vladikkuzn force-pushed the serving-deployment branch from 98d8de5 to d37fe2c Compare August 28, 2024 23:22

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 28, 2024

tenzen-y reviewed Aug 29, 2024

View reviewed changes

[jobframework] Deployment integration

33217ec

* Remove _ * Rename webhook endpoints * Deployment integration test

vladikkuzn force-pushed the serving-deployment branch from d37fe2c to 33217ec Compare August 29, 2024 09:27

vladikkuzn requested review from tenzen-y and alculquicondor August 29, 2024 09:28

tenzen-y reviewed Aug 29, 2024

View reviewed changes

pkg/controller/jobs/deployment/deployment_webhook.go Outdated Show resolved Hide resolved

[jobframework] Deployment integration

495e8de

* skip cq label setting if not set

tenzen-y reviewed Aug 30, 2024

View reviewed changes

[jobframework] Deployment integration

62d3ae3

* remove error from eventually

k8s-ci-robot assigned alculquicondor Aug 30, 2024

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

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

tenzen-y reviewed Aug 30, 2024

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

k8s-ci-robot assigned tenzen-y Aug 30, 2024

k8s-ci-robot merged commit 54f4662 into kubernetes-sigs:main Aug 30, 2024
16 checks passed

k8s-ci-robot added this to the v0.9 milestone Aug 30, 2024

vladikkuzn deleted the serving-deployment branch August 30, 2024 21:52

tenzen-y mentioned this pull request Aug 30, 2024

Allow an empty PodIntegrationOptions #2945

Merged

alaypatel07 mentioned this pull request Sep 4, 2024

deployment webhook is broken #2987

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[jobframework] Deployment integration #2813

[jobframework] Deployment integration #2813

vladikkuzn commented Aug 11, 2024 •

edited by k8s-ci-robot

Loading

k8s-ci-robot commented Aug 11, 2024

vladikkuzn commented Aug 11, 2024

netlify bot commented Aug 11, 2024 •

edited

Loading

vladikkuzn commented Aug 15, 2024

alculquicondor commented Aug 16, 2024

tenzen-y left a comment

tenzen-y left a comment

tenzen-y Aug 30, 2024

vladikkuzn Aug 30, 2024

alculquicondor Aug 30, 2024

alculquicondor Aug 30, 2024

tenzen-y Aug 30, 2024

k8s-ci-robot commented Aug 30, 2024

tenzen-y left a comment

k8s-ci-robot commented Aug 30, 2024

[jobframework] Deployment integration #2813

[jobframework] Deployment integration #2813

Conversation

vladikkuzn commented Aug 11, 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?

k8s-ci-robot commented Aug 11, 2024

vladikkuzn commented Aug 11, 2024

netlify bot commented Aug 11, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

vladikkuzn commented Aug 15, 2024

alculquicondor commented Aug 16, 2024

tenzen-y left a comment

Choose a reason for hiding this comment

tenzen-y left a comment

Choose a reason for hiding this comment

tenzen-y Aug 30, 2024

Choose a reason for hiding this comment

vladikkuzn Aug 30, 2024

Choose a reason for hiding this comment

alculquicondor Aug 30, 2024

Choose a reason for hiding this comment

alculquicondor Aug 30, 2024

Choose a reason for hiding this comment

tenzen-y Aug 30, 2024

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 30, 2024

tenzen-y left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 30, 2024

vladikkuzn commented Aug 11, 2024 •

edited by k8s-ci-robot

Loading

netlify bot commented Aug 11, 2024 •

edited

Loading