From 3e44139ae467fd55c887eb6e799735b1e508c9ad Mon Sep 17 00:00:00 2001 From: ravisantoshgudimetla Date: Mon, 2 Aug 2021 15:30:19 -0400 Subject: [PATCH] [jobs][registry]: 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. More context: https://github.com/kubernetes/kubernetes/pull/103449#discussion_r675820335 --- pkg/registry/batch/job/storage/storage.go | 30 ++- .../batch/job/storage/storage_test.go | 181 ++++++++++++++++++ 2 files changed, 210 insertions(+), 1 deletion(-) diff --git a/pkg/registry/batch/job/storage/storage.go b/pkg/registry/batch/job/storage/storage.go index 66ce3f58126..2fdb108158f 100644 --- a/pkg/registry/batch/job/storage/storage.go +++ b/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 diff --git a/pkg/registry/batch/job/storage/storage_test.go b/pkg/registry/batch/job/storage/storage_test.go index 7974b6a4be1..765b3179509 100644 --- a/pkg/registry/batch/job/storage/storage_test.go +++ b/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: ¶llelism, + 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)