refactor: standardize logging keys and improve job handling in AppDeployment (#32)

Author: ninjadq

internal/controller/appdeployment_controller.go

@@ -58,7 +58,7 @@ type AppDeploymentReconciler struct {
 // For more details, check Reconcile and its Result here:
 // - https://pkg.go.dev/sigs.k8s.io/[email protected]/pkg/reconcile
 func (r *AppDeploymentReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
-	logger := klog.FromContext(ctx).WithValues(log.AppDeploymentJobName, req.NamespacedName)
+	logger := klog.FromContext(ctx).WithValues(log.FieldKeyAppDeploymentJobName, req.NamespacedName)
 	appdeployment := &v1alpha1.AppDeployment{}
 	if err := r.Get(ctx, req.NamespacedName, appdeployment); err != nil {
 		return ctrl.Result{}, client.IgnoreNotFound(err)

internal/handler/appdeployment.go

@@ -2,7 +2,9 @@ package handler
 
 import (
 	"context"
+	"errors"
 	"fmt"
+	"strings"
 
 	"github.com/go-logr/logr"
 	batchv1 "k8s.io/api/batch/v1"
@@ -21,6 +23,10 @@ import (
 
 type AppdeploymentHandlerContextKey struct{}
 
+var (
+	ErrJobFailed = errors.New("job failed")
+)
+
 //go:generate mockgen -destination=./mocks/mock_appdeployment.go -package=mocks github.com/Azure/operation-cache-controller/internal/handler AppDeploymentHandlerInterface
 type AppDeploymentHandlerInterface interface {
 	EnsureApplicationValid(ctx context.Context) (reconciler.OperationResult, error)
@@ -166,7 +172,15 @@ func (a *AppDeploymentHandler) initializeJobAndAwaitCompletion(ctx context.Conte
 			a.recorder.Event(a.appDeployment, "Error", "FailedDeleteJob", err.Error())
 			return fmt.Errorf("failed to delete job %s: %w", job.Name, err)
 		}
-		// create a new job
+		// complete the job if it is a teardown job
+		if strings.HasPrefix(jobTemplate.Name, ctrlutils.JobTypeTeardown) {
+			a.logger.Error(ErrJobFailed, "teardown job failed", log.FieldKeyAppDeploymentJobName, jobTemplate.Name)
+			a.recorder.Event(a.appDeployment, "Warning", "TeardownJobFailed", fmt.Sprintf("Teardown job %s failed, requeuing for retry", jobTemplate.Name))
+			// return nil to make the teardown job complete
+			return nil
+		}
+
+		// create a new job if it is not a teardown job
 		if err := ctrl.SetControllerReference(a.appDeployment, jobTemplate, a.client.Scheme()); err != nil {
 			return fmt.Errorf("failed to set controller reference for job %s: %w", job.Name, err)
 		}
@@ -203,7 +217,7 @@ func (a *AppDeploymentHandler) EnsureDeployingFinished(ctx context.Context) (rec
 		a.appDeployment.Status.Phase = v1alpha1.AppDeploymentPhaseReady
 		return reconciler.RequeueOnErrorOrContinue(a.client.Status().Update(ctx, a.appDeployment))
 	case errJobNotCompleted:
-		a.logger.V(1).WithValues(log.AppDeploymentJobName, provisionJob.Name).Info("provision job is not completed yet")
+		a.logger.V(1).WithValues(log.FieldKeyAppDeploymentJobName, provisionJob.Name).Info("provision job is not completed yet")
 		return reconciler.Requeue()
 	default:
 		a.logger.Error(err, "provision job failed %s", provisionJob.Name)
@@ -224,10 +238,10 @@ func (a *AppDeploymentHandler) EnsureTeardownFinished(ctx context.Context) (reco
 		a.appDeployment.Status.Phase = v1alpha1.AppDeploymentPhaseDeleted
 		return reconciler.RequeueOnErrorOrContinue(a.client.Status().Update(ctx, a.appDeployment))
 	case errJobNotCompleted:
-		a.logger.V(1).WithValues(log.AppDeploymentJobName, teardownJob.Name).Info("teardown job is not completed yet")
+		a.logger.V(1).WithValues(log.FieldKeyAppDeploymentJobName, teardownJob.Name).Info("teardown job is not completed yet")
 		return reconciler.Requeue()
 	default:
-		a.logger.WithValues(log.AppDeploymentJobName, teardownJob.Name).Error(err, "teardown job failed %s")
+		a.logger.WithValues(log.FieldKeyAppDeploymentJobName, teardownJob.Name).Error(err, "teardown job failed %s")
 		return reconciler.RequeueWithError(err)
 	}
 }

internal/handler/appdeployment_test.go

@@ -728,6 +728,10 @@ func TestAppDeploymentAdapter_EnsureTeardownFinished(t *testing.T) {
 					Type:   batchv1.JobFailed,
 					Status: "True",
 				},
+				{
+					Type:   batchv1.JobComplete,
+					Status: "True",
+				},
 			},
 			Failed: 1,
 		},
@@ -837,13 +841,13 @@ func TestAppDeploymentAdapter_EnsureTeardownFinished(t *testing.T) {
 				return nil
 			})
 		mockClient.EXPECT().Delete(ctx, gomock.Any(), gomock.Any()).Return(nil)
-		mockClient.EXPECT().Create(ctx, gomock.Any()).Return(nil)
-		mockRecorder.EXPECT().Event(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes()
+		mockRecorder.EXPECT().Event(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(1)
 		mockStatusWriter.EXPECT().Update(gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
 		res, err := adapter.EnsureTeardownFinished(ctx)
 		assert.NoError(t, err)
-		assert.Equal(t, reconciler.OperationResult{RequeueDelay: reconciler.DefaultRequeueDelay, RequeueRequest: true}, res)
+		assert.Equal(t, reconciler.OperationResult{RequeueDelay: reconciler.DefaultRequeueDelay, RequeueRequest: false}, res)
 	})
+
 }
 
 func TestAppDeploymentAdapter_EnsureTeardownFinished_JobErrors(t *testing.T) {

internal/log/const.go

@@ -2,6 +2,6 @@ package log
 
 const (
 	// log keys
-	AppDeploymentJobName = "jobName"
-	AppDeploymentName    = "appDeploymentName"
+	FieldKeyAppDeploymentJobName = "jobName"
+	AppDeploymentName            = "appDeploymentName"
 )