[jobs][registry]: Warn when propogationpolicy is not set #104080

ravisantoshgudimetla · 2021-08-02T19:31:56Z

What type of PR is this?

What this PR does / why we need it:

Warn if no propagationpolicy set

If no propagation policy has been set, the pods associated
with the jobs are going to linger because of OrphanDependents
policy set currently. This patch ensures that a warning
will be thrown when the user explicitly doesn't set deletionPolicy.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Surface warning when users don't set propagationPolicy for jobs while deleting

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

xref: #103449 (comment)

cc @smarterclayton @liggitt @soltysh

k8s-ci-robot · 2021-08-02T19:32:04Z

@ravisantoshgudimetla: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ravisantoshgudimetla · 2021-08-02T19:35:16Z

pkg/registry/batch/job/storage/storage.go

@@ -117,3 +118,13 @@ func (r *StatusREST) Update(ctx context.Context, name string, objInfo rest.Updat
 func (r *StatusREST) GetResetFields() map[fieldpath.APIVersion]*fieldpath.Set {
 	return r.store.GetResetFields()
 }
+
+func (r *StatusREST) Delete(ctx context.Context, name string, deleteValidation rest.ValidateObjectFunc, options *metav1.DeleteOptions) (runtime.Object, bool, error) {
+	if options.PropagationPolicy == nil {


@liggitt . Especially this line. The other option is to have a method similar to WarningOnCreate:

// WarningsOnDelete returns warnings for the deletion of job object. It can be moved to RESTDeleteStrategy interface but // not sure, if there are any objects that can benefit from this movement. So, keeping this method local to // jobStrategy for now. func (jobStrategy) WarningsOnDelete(ctx context.Context, obj runtime.Object) []string { newJob := obj.(*batch.Job) }

I have a hard time seeing more than this use of a warning on delete, so I probably wouldn't add a general method like WarningsOnDelete yet

Suggested change

if options.PropagationPolicy == nil {

if options.PropagationPolicy == nil && options.OrphanDependents == nil {

So, the approach is fine? Having set the warnings in storage? Because I see no other example of warnings being used in storage.

And isn't OrphanDependents a deprecated one?

So, the approach is fine? Having set the warnings in storage? Because I see no other example of warnings being used in storage.

we tend to add warnings at the level that knows the relevant info... endpoint-level warnings for deprecated APIs are added in k8s.io/apiserver/pkg/endpoints/*, object-level warnings for create/update are added in strategy create and update methods, this is related to delete options the storage/strategy has an opinion about, so adding in delete implementations doesn't seem bad to me

isn't OrphanDependents a deprecated one?

yes, but if the requester has explicitly set that, we don't have to wonder if they are getting implicit behavior they didn't intend

pkg/registry/batch/job/storage/storage.go

ravisantoshgudimetla

@liggitt - This is up for review. Can you PTAL

liggitt · 2021-08-04T13:58:28Z

pkg/registry/batch/job/storage/storage.go

+	if options != nil && options.PropagationPolicy == nil && options.OrphanDependents == nil {
+		// Throw a warning if delete options are not explicitly set as Job deletion strategy by default is orphaning
+		// pods in v1.
+		r.warnings = deleteOptionWarnings


don't mutate r here

liggitt · 2021-08-04T13:58:42Z

pkg/registry/batch/job/storage/storage.go

+		// Throw a warning if delete options are not explicitly set as Job deletion strategy by default is orphaning
+		// pods in v1.
+		r.warnings = deleteOptionWarnings
+		for _, warningMsg := range r.warnings {


no need to range, just add the single warning you want

I started with a string and realized, it'd be easier to extend if we go with a map. Changed it back to string.

liggitt · 2021-08-04T14:01:03Z

pkg/registry/batch/job/storage/storage.go

@@ -91,6 +102,30 @@ func (r *REST) Categories() []string {
 	return []string{"all"}
 }

+func (r *REST) Delete(ctx context.Context, name string, deleteValidation rest.ValidateObjectFunc, options *metav1.DeleteOptions) (runtime.Object, bool, error) {
+	if options != nil && options.PropagationPolicy == nil && options.OrphanDependents == nil {


limit the warning to requests which default to OrphanDependents via batch/v1 by adding this:

&& job.Strategy.DefaultGarbageCollectionPolicy(ctx) == rest.OrphanDependents

liggitt · 2021-08-04T14:02:05Z

pkg/registry/batch/job/storage/storage.go

+	if deleteOptions.PropagationPolicy == nil && deleteOptions.OrphanDependents == nil {
+		// Throw a warning if delete options are not explicitly set as Job deletion strategy by default is orphaning
+		// pods in v1.
+		r.warnings = deleteOptionWarnings


don't mutate r

This part of codebase is new to me. I was a bit hesistant to mutate r but I did not know how to surface the warning. Now, I think I am clear, you want me to mock out recorder and capture warnings from the recorder. Will do that. Thank you :)

liggitt · 2021-08-04T14:02:14Z

pkg/registry/batch/job/storage/storage.go

+		// Throw a warning if delete options are not explicitly set as Job deletion strategy by default is orphaning
+		// pods in v1.
+		r.warnings = deleteOptionWarnings
+		for _, warningMsg := range r.warnings {


don't range, just add the warning you want

liggitt · 2021-08-04T14:02:30Z

pkg/registry/batch/job/storage/storage.go

+}
+
+func (r *REST) DeleteCollection(ctx context.Context, deleteValidation rest.ValidateObjectFunc, deleteOptions *metav1.DeleteOptions, listOptions *internalversion.ListOptions) (runtime.Object, error) {
+	if deleteOptions.PropagationPolicy == nil && deleteOptions.OrphanDependents == nil {


&& job.Strategy.DefaultGarbageCollectionPolicy(ctx) == rest.OrphanDependents

liggitt · 2021-08-04T14:02:50Z

pkg/registry/batch/job/storage/storage_test.go

@@ -132,12 +135,117 @@ func TestUpdate(t *testing.T) {
 	)
 }

-func TestDelete(t *testing.T) {


leave the existing test intact and add a new one to exercise the warning

liggitt · 2021-08-04T14:03:54Z

pkg/registry/batch/job/storage/storage_test.go

-	test := genericregistrytest.New(t, storage.Job.Store)
-	test.TestDelete(validNewJob())
+func TestJobDeletion(t *testing.T) {
+	ctx := genericapirequest.NewDefaultContext()


add a dummy warning recorder to the context (WithWarningRecorder) to intercept whether warnings were added

pkg/registry/batch/job/storage/storage_test.go

liggitt · 2021-08-04T17:54:46Z

pkg/registry/batch/job/storage/storage.go

+var deleteOptionWarnings = "Set propagationPolicy=Orphan, if you want to intentionally orphan pods. If you want to " +
+	"clean up children, you should set propagationPolicy=Background."
+


I'd suggest something more succinct like child pods are preserved by default when jobs are deleted; set propagationPolicy=Background to remove them or set propagationPolicy=Orphan to suppress this warning

Done. Thank you for the quick review

liggitt

a couple nits, then lgtm

liggitt · 2021-08-04T19:28:04Z

pkg/registry/batch/job/storage/storage_test.go

+		requestInfo   *genericapirequest.RequestInfo
+	}{
+		{
+			description:   "DeleteOptions without any policy: warnings expected for batch/v1",


nit: these are really verbose for subtest names (they tend to blow out testgrid views of runs)

suggest something like no policy, v1, warning, no policy, v2, no warning, etc

liggitt · 2021-08-04T19:29:04Z

pkg/registry/batch/job/storage/storage_test.go

+			if err := storage.Job.Storage.Create(ctxWithRecorder, key, job, nil, 0, false); err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			_, _, err := storage.Job.Delete(ctxWithRecorder, job.Name, rest.ValidateAllObjectFunc, test.deleteOptions)


you can simplify the tests to exercise both Delete and DeleteCollection in the same testcase, since they should behave identically

Done. I was thinking of one test per function but I am fine with it.

If no propagation policy has been set, the pods associated with the jobs are going to linger because of OrphanDependents policy set currently. This patch ensures that a warning will be thrown when the user explicitly doesn't set deletionPolicy. More context: kubernetes#103449 (comment)

ravisantoshgudimetla · 2021-08-05T02:32:10Z

/retest

liggitt · 2021-08-05T16:18:24Z

/lgtm
/approve

k8s-ci-robot · 2021-08-05T16:18:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, ravisantoshgudimetla

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:

~~pkg/registry/OWNERS~~ [liggitt]

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

liggitt · 2021-08-05T16:18:47Z

/sig apps
/kind cleanup

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 2, 2021

k8s-ci-robot requested review from deads2k and hongchaodeng August 2, 2021 19:32

ravisantoshgudimetla mentioned this pull request Aug 2, 2021

[registry] Jobs v1 should support DeleteDependents by default #103449

Closed

ravisantoshgudimetla commented Aug 2, 2021

View reviewed changes

liggitt reviewed Aug 2, 2021

View reviewed changes

pkg/registry/batch/job/storage/storage.go Outdated Show resolved Hide resolved

ravisantoshgudimetla force-pushed the warning-job-del branch from e56adce to 224468e Compare August 4, 2021 13:00

ravisantoshgudimetla commented Aug 4, 2021

View reviewed changes

ravisantoshgudimetla changed the title ~~[WIP][jobs][registry]: Warn when propogationpolicy is not set~~ [jobs][registry]: Warn when propogationpolicy is not set Aug 4, 2021

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 4, 2021

liggitt reviewed Aug 4, 2021

View reviewed changes

ravisantoshgudimetla force-pushed the warning-job-del branch 2 times, most recently from 78f35bf to afb95d1 Compare August 4, 2021 17:17

liggitt reviewed Aug 4, 2021

View reviewed changes

ravisantoshgudimetla force-pushed the warning-job-del branch from afb95d1 to bac38da Compare August 4, 2021 18:44

liggitt reviewed Aug 4, 2021

View reviewed changes

ravisantoshgudimetla force-pushed the warning-job-del branch from bac38da to 3e44139 Compare August 5, 2021 01:10

k8s-ci-robot assigned liggitt Aug 5, 2021

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

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

k8s-ci-robot merged commit 8d7a797 into kubernetes:master Aug 5, 2021

k8s-ci-robot added this to the v1.23 milestone Aug 5, 2021

ravisantoshgudimetla deleted the warning-job-del branch August 6, 2021 00:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[jobs][registry]: Warn when propogationpolicy is not set #104080

[jobs][registry]: Warn when propogationpolicy is not set #104080

ravisantoshgudimetla commented Aug 2, 2021 •

edited

k8s-ci-robot commented Aug 2, 2021

ravisantoshgudimetla Aug 2, 2021 •

edited

liggitt Aug 2, 2021 •

edited

liggitt Aug 2, 2021

ravisantoshgudimetla Aug 2, 2021

ravisantoshgudimetla Aug 2, 2021

liggitt Aug 2, 2021

ravisantoshgudimetla left a comment

liggitt Aug 4, 2021

liggitt Aug 4, 2021

ravisantoshgudimetla Aug 4, 2021

liggitt Aug 4, 2021

liggitt Aug 4, 2021

ravisantoshgudimetla Aug 4, 2021

liggitt Aug 4, 2021

liggitt Aug 4, 2021

liggitt Aug 4, 2021

liggitt Aug 4, 2021

liggitt Aug 4, 2021

ravisantoshgudimetla Aug 4, 2021 •

edited

liggitt left a comment

liggitt Aug 4, 2021

liggitt Aug 4, 2021

ravisantoshgudimetla Aug 5, 2021

ravisantoshgudimetla commented Aug 5, 2021

liggitt commented Aug 5, 2021

k8s-ci-robot commented Aug 5, 2021

liggitt commented Aug 5, 2021

	if options.PropagationPolicy == nil {
	if options.PropagationPolicy == nil && options.OrphanDependents == nil {

		var deleteOptionWarnings = "Set propagationPolicy=Orphan, if you want to intentionally orphan pods. If you want to " +
		"clean up children, you should set propagationPolicy=Background."

[jobs][registry]: Warn when propogationpolicy is not set #104080

[jobs][registry]: Warn when propogationpolicy is not set #104080

Conversation

ravisantoshgudimetla commented Aug 2, 2021 • edited

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?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Aug 2, 2021

ravisantoshgudimetla Aug 2, 2021 • edited

Choose a reason for hiding this comment

liggitt Aug 2, 2021 • edited

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

ravisantoshgudimetla 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

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

ravisantoshgudimetla Aug 4, 2021 • edited

Choose a reason for hiding this comment

liggitt 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

ravisantoshgudimetla commented Aug 5, 2021

liggitt commented Aug 5, 2021

k8s-ci-robot commented Aug 5, 2021

liggitt commented Aug 5, 2021

ravisantoshgudimetla commented Aug 2, 2021 •

edited

ravisantoshgudimetla Aug 2, 2021 •

edited

liggitt Aug 2, 2021 •

edited

ravisantoshgudimetla Aug 4, 2021 •

edited