[kueuectl] Delete the corresponding Job when deleting a Workload. #2992

mbobrovskyi · 2024-09-05T12:51:17Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Create separate command for deleting the Workload and its corresponding Job(s),
Improve WorkloadNameFunc to allow autocompletion for multiple resource names in arguments.

Which issue(s) this PR fixes:

Fixes #2975

Special notes for your reviewer:

Does this PR introduce a user-facing change?

CLI: Delete the corresponding Job when deleting a Workload.

netlify · 2024-09-05T12:51:33Z

✅ Deploy Preview for kubernetes-sigs-kueue ready!

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

mbobrovskyi · 2024-09-05T14:05:20Z

/cc @trasc

mbobrovskyi · 2024-09-10T14:03:38Z

/cc @mimowo @alculquicondor

alculquicondor · 2024-09-10T17:04:10Z

/assign @mwielgus

keps/2076-kueuectl/README.md

cmd/kueuectl/app/delete/delete_workload.go

trasc · 2024-09-18T13:23:17Z

cmd/kueuectl/app/delete/delete_workload.go

+	return workloads, haveAssociatedWorkloads, nil
+}
+
+func (o *WorkloadOptions) getWorkloadResources(workloads []*v1beta1.Workload) (map[*v1beta1.Workload][]GroupVersionResourceName, error) {


Maybe we should not put too much pressure on this now but, the first owner may not be the actual (root) owner of the wl for example in case of FluxMiniclusters, deployments and probably more, both the fist layer owner and the workload will be recreated.

This is a good point! We can address the root owner and delete it. However, a deployment can create multiple workloads. Deleting the deployment will also remove all associated workloads.

To ensure we do not lose context, let's continue the discussion at #3101 (comment).

cmd/kueuectl/app/delete/delete_workload_test.go

cmd/kueuectl/app/passthrough/passthrough.go

mwielgus · 2024-09-19T14:47:40Z

Can you either split the PR or add description what else, apart from delete, is being done there and why these changes need to go together?

mimowo · 2024-09-23T11:23:01Z

/hold
Also in case @mwielgus would like to give it a pass

cmd/kueuectl/app/delete/delete_workload.go

Co-authored-by: Traian Schiau <traian_schiau@epam.com>

trasc

/lgtm

k8s-ci-robot · 2024-09-24T07:08:28Z

LGTM label has been added.

Git tree hash: 8dcd275020e00ad1fe0d52ec4068bc2f3d28afd9

mimowo · 2024-09-24T07:17:28Z

/approve
Thanks for moving this to completion.

/hold
Also in case @mwielgus would like to give it a pass

I believe since Marcin lgtm'ed the KEP we are good to go.
/hold cancel

k8s-ci-robot · 2024-09-24T07:17:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mbobrovskyi, 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:

~~cmd/OWNERS~~ [mimowo]
~~site/OWNERS~~ [mimowo]
~~test/OWNERS~~ [mimowo]

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

…bernetes-sigs#2992) * Delete the corresponding Job when deleting a Workload. * Fix workload completion func. * Add delete confirmation. * Allow to delete all workloads. * Use unit tests instead integration. * Ignore out for delete all workloads. * Fix typos. * Remove unnecessary resources on unit-tests. * Fixes accordingly to the KEP updates. * Rephrase description. Co-authored-by: Traian Schiau <traian_schiau@epam.com> * Delete cascade strategy. --------- Co-authored-by: Traian Schiau <traian_schiau@epam.com>

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

k8s-ci-robot requested review from mimowo and trasc September 5, 2024 12:51

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 5, 2024

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 5, 2024

mbobrovskyi force-pushed the fix/delete-workload-command branch 3 times, most recently from 1ae5e21 to 3df829a Compare September 5, 2024 14:00

mbobrovskyi force-pushed the fix/delete-workload-command branch from 919c219 to db42b2e Compare September 10, 2024 12:16

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 10, 2024

mbobrovskyi force-pushed the fix/delete-workload-command branch 3 times, most recently from 5c70290 to c98e7b0 Compare September 10, 2024 14:02

k8s-ci-robot requested a review from alculquicondor September 10, 2024 14:03

k8s-ci-robot assigned mwielgus Sep 10, 2024

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

mbobrovskyi force-pushed the fix/delete-workload-command branch from c98e7b0 to b5330e4 Compare September 16, 2024 02:31

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

mbobrovskyi force-pushed the fix/delete-workload-command branch 3 times, most recently from d8d7a32 to ac495ed Compare September 16, 2024 15:12

trasc reviewed Sep 18, 2024

View reviewed changes

mbobrovskyi force-pushed the fix/delete-workload-command branch from e76d799 to 4f9f9c6 Compare September 19, 2024 15:12

mbobrovskyi added 7 commits September 23, 2024 14:16

Fix workload completion func.

2627175

Add delete confirmation.

2171bf2

Allow to delete all workloads.

b2d1fc3

Use unit tests instead integration.

b762282

Ignore out for delete all workloads.

55a5b1b

Fix typos.

b1d9b81

Remove unnecessary resources on unit-tests.

d890daa

mbobrovskyi force-pushed the fix/delete-workload-command branch from bc44b76 to d890daa Compare September 23, 2024 11:16

mbobrovskyi requested a review from trasc September 23, 2024 12:52

Fixes accordingly to the KEP updates.

d849ae0

mbobrovskyi force-pushed the fix/delete-workload-command branch from 072be2d to d849ae0 Compare September 23, 2024 13:14

trasc reviewed Sep 23, 2024

View reviewed changes

cmd/kueuectl/app/delete/delete_workload.go Show resolved Hide resolved

Rephrase description.

2b41ac4

Co-authored-by: Traian Schiau <traian_schiau@epam.com>

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 24, 2024

Delete cascade strategy.

15c5642

mbobrovskyi force-pushed the fix/delete-workload-command branch from 882ea4a to 15c5642 Compare September 24, 2024 06:42

mbobrovskyi requested a review from trasc September 24, 2024 06:54

trasc reviewed Sep 24, 2024

View reviewed changes

k8s-ci-robot assigned trasc Sep 24, 2024

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

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

k8s-ci-robot merged commit cf8d196 into kubernetes-sigs:main Sep 24, 2024
16 checks passed

k8s-ci-robot added this to the v0.9 milestone Sep 24, 2024

mbobrovskyi deleted the fix/delete-workload-command branch September 24, 2024 07:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[kueuectl] Delete the corresponding Job when deleting a Workload. #2992

[kueuectl] Delete the corresponding Job when deleting a Workload. #2992

mbobrovskyi commented Sep 5, 2024 •

edited

Loading

netlify bot commented Sep 5, 2024 •

edited

Loading

mbobrovskyi commented Sep 5, 2024

mbobrovskyi commented Sep 10, 2024

alculquicondor commented Sep 10, 2024

trasc Sep 18, 2024

mbobrovskyi Sep 19, 2024

mbobrovskyi Sep 20, 2024 •

edited

Loading

mwielgus commented Sep 19, 2024

mimowo commented Sep 23, 2024

trasc left a comment

k8s-ci-robot commented Sep 24, 2024

mimowo commented Sep 24, 2024

k8s-ci-robot commented Sep 24, 2024

[kueuectl] Delete the corresponding Job when deleting a Workload. #2992

[kueuectl] Delete the corresponding Job when deleting a Workload. #2992

Conversation

mbobrovskyi commented Sep 5, 2024 • 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:

Does this PR introduce a user-facing change?

netlify bot commented Sep 5, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue ready!

mbobrovskyi commented Sep 5, 2024

mbobrovskyi commented Sep 10, 2024

alculquicondor commented Sep 10, 2024

trasc Sep 18, 2024

Choose a reason for hiding this comment

mbobrovskyi Sep 19, 2024

Choose a reason for hiding this comment

mbobrovskyi Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

mwielgus commented Sep 19, 2024

mimowo commented Sep 23, 2024

trasc left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 24, 2024

mimowo commented Sep 24, 2024

k8s-ci-robot commented Sep 24, 2024

mbobrovskyi commented Sep 5, 2024 •

edited

Loading

netlify bot commented Sep 5, 2024 •

edited

Loading

mbobrovskyi Sep 20, 2024 •

edited

Loading