A reconciler should not require the DSCI to perform cleanup action to…

… avoid deadlock (#1429)

When the common reconciler detect that a resource is amrked for
deletion, it shoudl not try to lookup the DSCI as it should be possible
to remove a resource even if the DSCI is not present to avoid deadlocks.

To make the code more clear, the Reconcile method has been split in:
- delete, which triggers the execution of the finalizer actions
- apply, which triggers the execution od the reconcile actions

Signed-off-by: Luca Burgazzoli <[email protected]>

controllers/dscinitialization/suite_test.go

@@ -79,7 +79,6 @@ func TestDataScienceClusterInitialization(t *testing.T) {
 
 var testScheme = runtime.NewScheme()
 
-//nolint:fatcontext
 var _ = BeforeSuite(func() {
 	// can't use suite's context as the manager should survive the function
 	//nolint:fatcontext

controllers/webhook/webhook_suite_test.go

@@ -73,7 +73,6 @@ func TestAPIs(t *testing.T) {
 	RunSpecs(t, "Webhook Suite")
 }
 
-//nolint:fatcontext
 var _ = BeforeSuite(func() {
 	// can't use suite's context as the manager should survive the function
 	//nolint:fatcontext

pkg/controller/reconciler/reconciler.go

@@ -108,48 +108,80 @@ func (r *Reconciler[T]) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.R
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
 
-	dscil := dsciv1.DSCInitializationList{}
-	if err := r.Client.List(ctx, &dscil); err != nil {
-		return ctrl.Result{}, err
+	if !res.GetDeletionTimestamp().IsZero() {
+		if err := r.delete(ctx, res); err != nil {
+			return ctrl.Result{}, err
+		}
 	}
 
-	if len(dscil.Items) != 1 {
-		return ctrl.Result{}, errors.New("unable to find DSCInitialization")
+	if err := r.apply(ctx, res); err != nil {
+		return ctrl.Result{}, err
 	}
 
+	return ctrl.Result{}, nil
+}
+
+func (r *Reconciler[T]) delete(ctx context.Context, res client.Object) error {
+	l := log.FromContext(ctx)
+	l.Info("delete")
+
 	rr := types.ReconciliationRequest{
 		Client:    r.Client,
 		Manager:   r.m,
 		Instance:  res,
-		DSCI:      &dscil.Items[0],
 		Release:   r.Release,
 		Manifests: make([]types.ManifestInfo, 0),
+
+		// The DSCI should not be required when deleting a component, if the
+		// component requires some additional info, then such info should be
+		// stored as part of the spec/status
+		DSCI: nil,
 	}
 
-	// Handle deletion
-	if !res.GetDeletionTimestamp().IsZero() {
-		// Execute finalizers
-		for _, action := range r.Finalizer {
-			l.V(3).Info("Executing finalizer", "action", action)
-
-			actx := log.IntoContext(
-				ctx,
-				l.WithName(actions.ActionGroup).WithName(action.String()),
-			)
-
-			if err := action(actx, &rr); err != nil {
-				se := odherrors.StopError{}
-				if !errors.As(err, &se) {
-					l.Error(err, "Failed to execute finalizer", "action", action)
-					return ctrl.Result{}, err
-				}
-
-				l.V(3).Info("detected stop marker", "action", action)
-				break
+	// Execute finalizers
+	for _, action := range r.Finalizer {
+		l.V(3).Info("Executing finalizer", "action", action)
+
+		actx := log.IntoContext(
+			ctx,
+			l.WithName(actions.ActionGroup).WithName(action.String()),
+		)
+
+		if err := action(actx, &rr); err != nil {
+			se := odherrors.StopError{}
+			if !errors.As(err, &se) {
+				l.Error(err, "Failed to execute finalizer", "action", action)
+				return err
 			}
+
+			l.V(3).Info("detected stop marker", "action", action)
+			break
 		}
+	}
+
+	return nil
+}
 
-		return ctrl.Result{}, nil
+func (r *Reconciler[T]) apply(ctx context.Context, res client.Object) error {
+	l := log.FromContext(ctx)
+	l.Info("apply")
+
+	dscil := dsciv1.DSCInitializationList{}
+	if err := r.Client.List(ctx, &dscil); err != nil {
+		return err
+	}
+
+	if len(dscil.Items) != 1 {
+		return errors.New("unable to find DSCInitialization")
+	}
+
+	rr := types.ReconciliationRequest{
+		Client:    r.Client,
+		Manager:   r.m,
+		Instance:  res,
+		DSCI:      &dscil.Items[0],
+		Release:   r.Release,
+		Manifests: make([]types.ManifestInfo, 0),
 	}
 
 	// Execute actions
@@ -165,24 +197,24 @@ func (r *Reconciler[T]) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.R
 			se := odherrors.StopError{}
 			if !errors.As(err, &se) {
 				l.Error(err, "Failed to execute action", "action", action)
-				return ctrl.Result{}, err
+				return err
 			}
 
 			l.V(3).Info("detected stop marker", "action", action)
 			break
 		}
 	}
 
-	err = r.Client.ApplyStatus(
+	err := r.Client.ApplyStatus(
 		ctx,
 		rr.Instance,
 		client.FieldOwner(r.name),
 		client.ForceOwnership,
 	)
 
 	if err != nil {
-		return ctrl.Result{}, err
+		return err
 	}
 
-	return ctrl.Result{}, err
+	return nil
 }