github/kubernetes
1
0
Fork 0
mirror of https://github.com/k3s-io/kubernetes.git synced 2025-07-30 06:54:01 +00:00
Code Issues Projects Releases Wiki Activity

[jobs][registry]: Warn if no propagationpolicy set

Browse Source 
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
This commit is contained in:
ravisantoshgudimetla 2021-08-02 15:30:19 -04:00
parent 5be21c50c2
commit 3e44139ae4
2 changed files with 210 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

30
pkg/registry/batch/job/storage/storage.go
View File

@ -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

181
pkg/registry/batch/job/storage/storage_test.go
View File

@ -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)