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)