Merge pull request #104080 from ravisantoshgudimetla/warning-job-del

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

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

@@ -18,12 +18,13 @@ package storage
 
 import (
 	"context"
-
+	"k8s.io/apimachinery/pkg/apis/meta/internalversion"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apiserver/pkg/registry/generic"
 	genericregistry "k8s.io/apiserver/pkg/registry/generic/registry"
 	"k8s.io/apiserver/pkg/registry/rest"
+	"k8s.io/apiserver/pkg/warning"
 	"k8s.io/kubernetes/pkg/apis/batch"
 	"k8s.io/kubernetes/pkg/printers"
 	printersinternal "k8s.io/kubernetes/pkg/printers/internalversion"
@@ -51,6 +52,9 @@ func NewStorage(optsGetter generic.RESTOptionsGetter) (JobStorage, error) {
 	}, nil
 }
 
+var deleteOptionWarnings = "child pods are preserved by default when jobs are deleted; " +
+	"set propagationPolicy=Background to remove them or set propagationPolicy=Orphan to suppress this warning"
+
 // REST implements a RESTStorage for jobs against etcd
 type REST struct {
 	*genericregistry.Store
@@ -91,6 +95,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) {
+	//lint:ignore SA1019 backwards compatibility
+	//nolint: staticcheck
+	if options != nil && options.PropagationPolicy == nil && options.OrphanDependents == nil &&
+		job.Strategy.DefaultGarbageCollectionPolicy(ctx) == rest.OrphanDependents {
+		// Throw a warning if delete options are not explicitly set as Job deletion strategy by default is orphaning
+		// pods in v1.
+		warning.AddWarning(ctx, "", deleteOptionWarnings)
+	}
+	return r.Store.Delete(ctx, name, deleteValidation, options)
+}
+
+func (r *REST) DeleteCollection(ctx context.Context, deleteValidation rest.ValidateObjectFunc, deleteOptions *metav1.DeleteOptions, listOptions *internalversion.ListOptions) (runtime.Object, error) {
+	//lint:ignore SA1019 backwards compatibility
+	//nolint: staticcheck
+	if deleteOptions.PropagationPolicy == nil && deleteOptions.OrphanDependents == nil &&
+		job.Strategy.DefaultGarbageCollectionPolicy(ctx) == rest.OrphanDependents {
+		// Throw a warning if delete options are not explicitly set as Job deletion strategy by default is orphaning
+		// pods in v1.
+		warning.AddWarning(ctx, "", deleteOptionWarnings)
+	}
+	return r.Store.DeleteCollection(ctx, deleteValidation, deleteOptions, listOptions)
+}
+
 // StatusREST implements the REST endpoint for changing the status of a resourcequota.
 type StatusREST struct {
 	store *genericregistry.Store

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

@@ -19,13 +19,19 @@ package storage
 import (
 	"testing"
 
+	batchv1 "k8s.io/api/batch/v1"
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/internalversion"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/fields"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
+	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
 	"k8s.io/apiserver/pkg/registry/generic"
 	genericregistrytest "k8s.io/apiserver/pkg/registry/generic/testing"
+	"k8s.io/apiserver/pkg/registry/rest"
 	etcd3testing "k8s.io/apiserver/pkg/storage/etcd3/testing"
+	"k8s.io/apiserver/pkg/warning"
 	"k8s.io/kubernetes/pkg/apis/batch"
 	api "k8s.io/kubernetes/pkg/apis/core"
 	"k8s.io/kubernetes/pkg/registry/registrytest"
@@ -82,6 +88,42 @@ func validNewJob() *batch.Job {
 	}
 }
 
+func validNewV1Job() *batchv1.Job {
+	completions := int32(1)
+	parallelism := int32(1)
+	return &batchv1.Job{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "foo",
+			Namespace: "default",
+		},
+		Spec: batchv1.JobSpec{
+			Completions: &completions,
+			Parallelism: &parallelism,
+			Selector: &metav1.LabelSelector{
+				MatchLabels: map[string]string{"a": "b"},
+			},
+			ManualSelector: newBool(true),
+			Template: corev1.PodTemplateSpec{
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: map[string]string{"a": "b"},
+				},
+				Spec: corev1.PodSpec{
+					Containers: []corev1.Container{
+						{
+							Name:                     "test",
+							Image:                    "test_image",
+							ImagePullPolicy:          corev1.PullIfNotPresent,
+							TerminationMessagePolicy: corev1.TerminationMessageReadFile,
+						},
+					},
+					RestartPolicy: corev1.RestartPolicyOnFailure,
+					DNSPolicy:     corev1.DNSClusterFirst,
+				},
+			},
+		},
+	}
+}
+
 func TestCreate(t *testing.T) {
 	storage, server := newStorage(t)
 	defer server.Terminate(t)
@@ -140,6 +182,145 @@ func TestDelete(t *testing.T) {
 	test.TestDelete(validNewJob())
 }
 
+type dummyRecorder struct {
+	agent string
+	text  string
+}
+
+func (r *dummyRecorder) AddWarning(agent, text string) {
+	r.agent = agent
+	r.text = text
+	return
+}
+
+func (r *dummyRecorder) getWarning() string {
+	return r.text
+}
+
+var _ warning.Recorder = &dummyRecorder{}
+
+func TestJobDeletion(t *testing.T) {
+	orphanDependents := true
+	orphanDeletionPropagation := metav1.DeletePropagationOrphan
+	backgroundDeletionPropagation := metav1.DeletePropagationBackground
+	job := validNewV1Job()
+	ctx := genericapirequest.NewDefaultContext()
+	key := "/jobs/" + metav1.NamespaceDefault + "/foo"
+	tests := []struct {
+		description   string
+		expectWarning bool
+		deleteOptions *metav1.DeleteOptions
+		listOptions   *internalversion.ListOptions
+		requestInfo   *genericapirequest.RequestInfo
+	}{
+		{
+			description:   "deletion: no policy, v1, warning",
+			expectWarning: true,
+			deleteOptions: &metav1.DeleteOptions{},
+			requestInfo:   &genericapirequest.RequestInfo{APIGroup: "batch", APIVersion: "v1"},
+		},
+		{
+			description:   "deletion: no policy, v2, no warning",
+			expectWarning: false,
+			deleteOptions: &metav1.DeleteOptions{},
+			requestInfo:   &genericapirequest.RequestInfo{APIGroup: "batch", APIVersion: "v2"},
+		},
+		{
+			description:   "deletion: no policy, no APIVersion, no warning",
+			expectWarning: false,
+			deleteOptions: &metav1.DeleteOptions{},
+			requestInfo:   &genericapirequest.RequestInfo{APIGroup: "batch", APIVersion: ""},
+		},
+		{
+			description:   "deletion: orphan dependents, no warnings",
+			expectWarning: false,
+			deleteOptions: &metav1.DeleteOptions{OrphanDependents: &orphanDependents},
+			requestInfo:   &genericapirequest.RequestInfo{APIGroup: "batch", APIVersion: "v1"},
+		},
+		{
+			description:   "deletion: orphan deletion, no warnings",
+			expectWarning: false,
+			deleteOptions: &metav1.DeleteOptions{PropagationPolicy: &orphanDeletionPropagation},
+			requestInfo:   &genericapirequest.RequestInfo{APIGroup: "batch", APIVersion: "v1"},
+		},
+		{
+			description:   "deletion: background deletion, no warnings",
+			expectWarning: false,
+			deleteOptions: &metav1.DeleteOptions{PropagationPolicy: &backgroundDeletionPropagation},
+			requestInfo:   &genericapirequest.RequestInfo{APIGroup: "batch", APIVersion: "v1"},
+		},
+		{
+			description:   "deleteCollection: no policy, v1, warning",
+			expectWarning: true,
+			deleteOptions: &metav1.DeleteOptions{},
+			listOptions:   &internalversion.ListOptions{},
+			requestInfo:   &genericapirequest.RequestInfo{APIGroup: "batch", APIVersion: "v1"},
+		},
+		{
+			description:   "deleteCollection: no policy, v2, no warning",
+			expectWarning: false,
+			deleteOptions: &metav1.DeleteOptions{},
+			listOptions:   &internalversion.ListOptions{},
+			requestInfo:   &genericapirequest.RequestInfo{APIGroup: "batch", APIVersion: "v2"},
+		},
+		{
+			description:   "deleteCollection: no policy, no APIVersion, no warning",
+			expectWarning: false,
+			deleteOptions: &metav1.DeleteOptions{},
+			listOptions:   &internalversion.ListOptions{},
+			requestInfo:   &genericapirequest.RequestInfo{APIGroup: "batch", APIVersion: ""},
+		},
+		{
+			description:   "deleteCollection: orphan dependents, no warnings",
+			expectWarning: false,
+			deleteOptions: &metav1.DeleteOptions{OrphanDependents: &orphanDependents},
+			listOptions:   &internalversion.ListOptions{},
+			requestInfo:   &genericapirequest.RequestInfo{APIGroup: "batch", APIVersion: "v1"},
+		},
+		{
+			description:   "deletionCollection: orphan deletion, no warnings",
+			expectWarning: false,
+			deleteOptions: &metav1.DeleteOptions{PropagationPolicy: &orphanDeletionPropagation},
+			listOptions:   &internalversion.ListOptions{},
+			requestInfo:   &genericapirequest.RequestInfo{APIGroup: "batch", APIVersion: "v1"},
+		},
+		{
+			description:   "deletionCollection: background deletion, no warnings",
+			expectWarning: false,
+			deleteOptions: &metav1.DeleteOptions{PropagationPolicy: &backgroundDeletionPropagation},
+			listOptions:   &internalversion.ListOptions{},
+			requestInfo:   &genericapirequest.RequestInfo{APIGroup: "batch", APIVersion: "v1"},
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.description, func(t *testing.T) {
+			storage, server := newStorage(t)
+			defer server.Terminate(t)
+			defer storage.Job.Store.DestroyFunc()
+			dc := dummyRecorder{agent: "", text: ""}
+			ctx = genericapirequest.WithRequestInfo(ctx, test.requestInfo)
+			ctxWithRecorder := warning.WithWarningRecorder(ctx, &dc)
+			// Create the object
+			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)
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			_, err = storage.Job.DeleteCollection(ctxWithRecorder, rest.ValidateAllObjectFunc, test.deleteOptions, test.listOptions)
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if test.expectWarning {
+				if dc.getWarning() != deleteOptionWarnings {
+					t.Fatalf("expected delete option warning but did not get one")
+				}
+			}
+		})
+	}
+}
+
 func TestGet(t *testing.T) {
 	storage, server := newStorage(t)
 	defer server.Terminate(t)