Merge pull request #115952 from pacoxu/cleanup-cronjob

cronjob: return immediately when failed to create job for the namespace is terminating
kubernetes · Jul 19, 2023 · 88c8bcb · 88c8bcb
2 parents 8c1dc65 + ade63dd
commit 88c8bcb
Show file tree

Hide file tree

Showing 3 changed files with 24 additions and 43 deletions.
diff --git a/pkg/controller/cronjob/cronjob_controllerv2.go b/pkg/controller/cronjob/cronjob_controllerv2.go
@@ -601,6 +601,9 @@ func (jm *ControllerV2) syncCronJob(
 	jobResp, err := jm.jobControl.CreateJob(cronJob.Namespace, jobReq)
 	switch {
 	case errors.HasStatusCause(err, corev1.NamespaceTerminatingCause):
+		// if the namespace is being terminated, we don't have to do
+		// anything because any creation will fail
+		return cronJob, nil, updateStatus, err
 	case errors.IsAlreadyExists(err):
 		// If the job is created by other actor, assume  it has updated the cronjob status accordingly
 		logger.Info("Job already exists", "cronjob", klog.KObj(cronJob), "job", klog.KObj(jobReq))

diff --git a/pkg/controller/cronjob/cronjob_controllerv2_test.go b/pkg/controller/cronjob/cronjob_controllerv2_test.go
@@ -18,6 +18,7 @@ package cronjob
 
 import (
 	"context"
+	"fmt"
 	"reflect"
 	"strings"
 	"testing"
@@ -1157,6 +1158,26 @@ func TestControllerV2SyncCronJob(t *testing.T) {
 			expectUpdateStatus:         true,
 			jobPresentInCJActiveStatus: true,
 		},
+		"do nothing if the namespace is terminating": {
+			jobCreateError: &errors.StatusError{ErrStatus: metav1.Status{Details: &metav1.StatusDetails{Causes: []metav1.StatusCause{
+				{
+					Type:    v1.NamespaceTerminatingCause,
+					Message: fmt.Sprintf("namespace %s is being terminated", metav1.NamespaceDefault),
+					Field:   "metadata.namespace",
+				}}}}},
+			concurrencyPolicy:          "Allow",
+			schedule:                   onTheHour,
+			deadline:                   noDead,
+			ranPreviously:              true,
+			stillActive:                true,
+			jobCreationTime:            justAfterThePriorHour(),
+			now:                        *justAfterTheHour(),
+			expectActive:               0,
+			expectRequeueAfter:         false,
+			expectUpdateStatus:         false,
+			expectErr:                  true,
+			jobPresentInCJActiveStatus: false,
+		},
 	}
 	for name, tc := range testCases {
 		name := name

diff --git a/pkg/controller/cronjob/injection.go b/pkg/controller/cronjob/injection.go
@@ -24,7 +24,6 @@ import (
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
-	"k8s.io/apimachinery/pkg/types"
 	clientset "k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/tools/record"
 )
@@ -84,10 +83,6 @@ type jobControlInterface interface {
 	GetJob(namespace, name string) (*batchv1.Job, error)
 	// CreateJob creates new Jobs according to the spec.
 	CreateJob(namespace string, job *batchv1.Job) (*batchv1.Job, error)
-	// UpdateJob updates a Job.
-	UpdateJob(namespace string, job *batchv1.Job) (*batchv1.Job, error)
-	// PatchJob patches a Job.
-	PatchJob(namespace string, name string, pt types.PatchType, data []byte, subresources ...string) (*batchv1.Job, error)
 	// DeleteJob deletes the Job identified by name.
 	// TODO: delete by UID?
 	DeleteJob(namespace string, name string) error
@@ -105,14 +100,6 @@ func (r realJobControl) GetJob(namespace, name string) (*batchv1.Job, error) {
 	return r.KubeClient.BatchV1().Jobs(namespace).Get(context.TODO(), name, metav1.GetOptions{})
 }
 
-func (r realJobControl) UpdateJob(namespace string, job *batchv1.Job) (*batchv1.Job, error) {
-	return r.KubeClient.BatchV1().Jobs(namespace).Update(context.TODO(), job, metav1.UpdateOptions{})
-}
-
-func (r realJobControl) PatchJob(namespace string, name string, pt types.PatchType, data []byte, subresources ...string) (*batchv1.Job, error) {
-	return r.KubeClient.BatchV1().Jobs(namespace).Patch(context.TODO(), name, pt, data, metav1.PatchOptions{}, subresources...)
-}
-
 func (r realJobControl) CreateJob(namespace string, job *batchv1.Job) (*batchv1.Job, error) {
 	return r.KubeClient.BatchV1().Jobs(namespace).Create(context.TODO(), job, metav1.CreateOptions{})
 }
@@ -156,28 +143,6 @@ func (f *fakeJobControl) GetJob(namespace, name string) (*batchv1.Job, error) {
 	return f.Job, nil
 }
 
-func (f *fakeJobControl) UpdateJob(namespace string, job *batchv1.Job) (*batchv1.Job, error) {
-	f.Lock()
-	defer f.Unlock()
-	if f.Err != nil {
-		return nil, f.Err
-	}
-	f.UpdateJobName = append(f.UpdateJobName, job.Name)
-	return job, nil
-}
-
-func (f *fakeJobControl) PatchJob(namespace string, name string, pt types.PatchType, data []byte, subresources ...string) (*batchv1.Job, error) {
-	f.Lock()
-	defer f.Unlock()
-	if f.Err != nil {
-		return nil, f.Err
-	}
-	f.PatchJobName = append(f.PatchJobName, name)
-	f.Patches = append(f.Patches, data)
-	// We don't have anything to return. Just return something non-nil.
-	return &batchv1.Job{}, nil
-}
-
 func (f *fakeJobControl) DeleteJob(namespace string, name string) error {
 	f.Lock()
 	defer f.Unlock()
@@ -187,11 +152,3 @@ func (f *fakeJobControl) DeleteJob(namespace string, name string) error {
 	f.DeleteJobName = append(f.DeleteJobName, name)
 	return nil
 }
-
-func (f *fakeJobControl) Clear() {
-	f.Lock()
-	defer f.Unlock()
-	f.DeleteJobName = []string{}
-	f.Jobs = []batchv1.Job{}
-	f.Err = nil
-}