From 37aff025ff8702f5dad1ada8d0d9dd08c211174f Mon Sep 17 00:00:00 2001 From: Paco Xu Date: Wed, 22 Feb 2023 17:17:47 +0800 Subject: [PATCH 1/3] cleanup: remove Clear for fake job controller --- pkg/controller/cronjob/injection.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/pkg/controller/cronjob/injection.go b/pkg/controller/cronjob/injection.go index 83b26d66ccc..515b6bb55b5 100644 --- a/pkg/controller/cronjob/injection.go +++ b/pkg/controller/cronjob/injection.go @@ -187,11 +187,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 -} From a890724f9efb55c32bab346bef7b36fb365fc967 Mon Sep 17 00:00:00 2001 From: Paco Xu Date: Wed, 22 Feb 2023 17:32:16 +0800 Subject: [PATCH 2/3] cronjob: return immediately when failed to create job for the namespace is terminating --- .../cronjob/cronjob_controllerv2.go | 3 ++ pkg/controller/cronjob/injection.go | 35 ------------------- 2 files changed, 3 insertions(+), 35 deletions(-) diff --git a/pkg/controller/cronjob/cronjob_controllerv2.go b/pkg/controller/cronjob/cronjob_controllerv2.go index cb706bbae45..60cde9c3d53 100644 --- 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 klog.InfoS("Job already exists", "cronjob", klog.KRef(cronJob.GetNamespace(), cronJob.GetName()), "job", klog.KRef(jobReq.GetNamespace(), jobReq.GetName())) diff --git a/pkg/controller/cronjob/injection.go b/pkg/controller/cronjob/injection.go index 515b6bb55b5..ee34b513223 100644 --- 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() From ade63dd7647dc0089925da7d2edc3c904fde7c42 Mon Sep 17 00:00:00 2001 From: Paco Xu Date: Wed, 22 Mar 2023 18:09:42 +0800 Subject: [PATCH 3/3] cronjob: add ut for namespace terminating cause no extra log --- .../cronjob/cronjob_controllerv2_test.go | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/pkg/controller/cronjob/cronjob_controllerv2_test.go b/pkg/controller/cronjob/cronjob_controllerv2_test.go index 507f0b722c8..3021ad87ace 100644 --- 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" @@ -1165,6 +1166,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