From 43fc6b5bdb0c319933da4a9746bc6853e6101c27 Mon Sep 17 00:00:00 2001 From: Sharpz7 Date: Wed, 6 Sep 2023 03:05:14 +0000 Subject: [PATCH] Added suggests changes --- pkg/controller/job/job_controller.go | 8 ++++---- pkg/controller/job/job_controller_test.go | 12 ++++++------ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/pkg/controller/job/job_controller.go b/pkg/controller/job/job_controller.go index 0c7547c1cfe..3ba3f89dcc9 100644 --- a/pkg/controller/job/job_controller.go +++ b/pkg/controller/job/job_controller.go @@ -470,9 +470,9 @@ func (jm *Controller) updateJob(logger klog.Logger, old, cur interface{}) { } // The job shouldn't be marked as finished until all pod finalizers are removed. - // This is a backup operation + // This is a backup operation in this case. if IsJobFinished(curJob) { - jm.backupRemovePodFinalizers(curJob) + jm.cleanupPodFinalizers(curJob) } // check if need to add a new rsync for ActiveDeadlineSeconds @@ -509,7 +509,7 @@ func (jm *Controller) deleteJob(logger klog.Logger, obj interface{}) { return } } - jm.backupRemovePodFinalizers(jobObj) + jm.cleanupPodFinalizers(jobObj) } // enqueueSyncJobImmediately tells the Job controller to invoke syncJob @@ -1874,7 +1874,7 @@ func onlyReplaceFailedPods(job *batch.Job) bool { return feature.DefaultFeatureGate.Enabled(features.JobPodFailurePolicy) && job.Spec.PodFailurePolicy != nil } -func (jm *Controller) backupRemovePodFinalizers(job *batch.Job) { +func (jm *Controller) cleanupPodFinalizers(job *batch.Job) { // Listing pods shouldn't really fail, as we are just querying the informer cache. selector, err := metav1.LabelSelectorAsSelector(job.Spec.Selector) if err != nil { diff --git a/pkg/controller/job/job_controller_test.go b/pkg/controller/job/job_controller_test.go index 1bb78ea422f..9676064e9b5 100644 --- a/pkg/controller/job/job_controller_test.go +++ b/pkg/controller/job/job_controller_test.go @@ -5172,7 +5172,7 @@ func TestFinalizersRemovedExpectations(t *testing.T) { } } -func TestBackupFinalizerRemoval(t *testing.T) { +func TestFinalizerCleanup(t *testing.T) { _, ctx := ktesting.NewTestContext(t) ctx, cancel := context.WithCancel(ctx) defer cancel() @@ -5183,7 +5183,9 @@ func TestBackupFinalizerRemoval(t *testing.T) { manager.podStoreSynced = alwaysReady manager.jobStoreSynced = alwaysReady - go manager.Run(context.TODO(), 0) + // Initialize the controller with 0 workers to make sure the + // pod finalizers are not removed by the "syncJob" function. + go manager.Run(ctx, 0) // Start the Pod and Job informers. sharedInformers.Start(ctx.Done()) @@ -5214,8 +5216,8 @@ func TestBackupFinalizerRemoval(t *testing.T) { t.Fatalf("Updating job status: %v", err) } - // Since Pod was not correctly tracked, the the backup code - // should remove the finalizer. (backupRemovePodFinalizers) + // Verify the pod finalizer is removed for a finished Job, + // even if the jobs pods are not tracked by the main reconciliation loop. if err := wait.PollUntilContextTimeout(ctx, 100*time.Millisecond, wait.ForeverTestTimeout, true, func(ctx context.Context) (bool, error) { p, err := clientset.CoreV1().Pods(pod.Namespace).Get(ctx, pod.Name, metav1.GetOptions{}) if err != nil { @@ -5224,8 +5226,6 @@ func TestBackupFinalizerRemoval(t *testing.T) { return !hasJobTrackingFinalizer(p), nil }); err != nil { if errors.Is(err, context.DeadlineExceeded) { - t.Errorf("Timed out waiting for Pod to get the finalizer removed") - } else { t.Errorf("Waiting for Pod to get the finalizer removed: %v", err) } }