Merge pull request #6780 from ykakarap/better-error-handling-tracking

🌱 Better error handling for tracking utilities

internal/controllers/topology/cluster/cluster_controller.go

@@ -353,7 +353,7 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, cluster *clusterv1.Clu
 			// The BeforeClusterDelete hook returned a non-blocking response. Now the cluster is ready to be deleted.
 			// Lets mark the cluster as `ok-to-delete`
 			if err := hooks.MarkAsOkToDelete(ctx, r.Client, cluster); err != nil {
-				return ctrl.Result{}, errors.Wrapf(err, "failed to mark %s/%s cluster as ok to delete", cluster.Namespace, cluster.Name)
+				return ctrl.Result{}, err
 			}
 		}
 	}

internal/controllers/topology/cluster/desired_state.go

@@ -31,7 +31,6 @@ import (
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/controllers/external"
-	runtimecatalog "sigs.k8s.io/cluster-api/exp/runtime/catalog"
 	runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1"
 	"sigs.k8s.io/cluster-api/feature"
 	"sigs.k8s.io/cluster-api/internal/contract"
@@ -338,7 +337,7 @@ func (r *Reconciler) computeControlPlaneVersion(ctx context.Context, s *scope.Sc
 					s.UpgradeTracker.MachineDeployments.HoldUpgrades(true)
 				} else {
 					if err := hooks.MarkAsDone(ctx, r.Client, s.Current.Cluster, runtimehooksv1.AfterControlPlaneUpgrade); err != nil {
-						return "", errors.Wrapf(err, "failed to remove the %s hook from pending hooks tracker", runtimecatalog.HookName(runtimehooksv1.AfterControlPlaneUpgrade))
+						return "", err
 					}
 				}
 			}
@@ -390,7 +389,7 @@ func (r *Reconciler) computeControlPlaneVersion(ctx context.Context, s *scope.Sc
 		// We are picking up the new version here.
 		// Track the intent of calling the AfterControlPlaneUpgrade and the AfterClusterUpgrade hooks once we are done with the upgrade.
 		if err := hooks.MarkAsPending(ctx, r.Client, s.Current.Cluster, runtimehooksv1.AfterControlPlaneUpgrade, runtimehooksv1.AfterClusterUpgrade); err != nil {
-			return "", errors.Wrapf(err, "failed to mark the %s hook as pending", []string{runtimecatalog.HookName(runtimehooksv1.AfterControlPlaneUpgrade), runtimecatalog.HookName(runtimehooksv1.AfterClusterUpgrade)})
+			return "", err
 		}
 	}
 

internal/controllers/topology/cluster/reconcile_state.go

@@ -31,7 +31,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
-	runtimecatalog "sigs.k8s.io/cluster-api/exp/runtime/catalog"
 	runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1"
 	"sigs.k8s.io/cluster-api/feature"
 	"sigs.k8s.io/cluster-api/internal/contract"
@@ -204,7 +203,7 @@ func (r *Reconciler) callAfterControlPlaneInitialized(ctx context.Context, s *sc
 	// If the cluster topology is being created then track to intent to call the AfterControlPlaneInitialized hook so that we can call it later.
 	if s.Current.Cluster.Spec.InfrastructureRef == nil && s.Current.Cluster.Spec.ControlPlaneRef == nil {
 		if err := hooks.MarkAsPending(ctx, r.Client, s.Current.Cluster, runtimehooksv1.AfterControlPlaneInitialized); err != nil {
-			return errors.Wrapf(err, "failed to remove the %s hook from pending hooks tracker", runtimecatalog.HookName(runtimehooksv1.AfterControlPlaneInitialized))
+			return err
 		}
 	}
 

internal/hooks/tracking.go

@@ -34,16 +34,17 @@ import (
 // MarkAsPending adds to the object's PendingHooksAnnotation the intent to execute a hook after an operation completes.
 // Usually this function is called when an operation is starting in order to track the intent to call an After<operation> hook later in the process.
 func MarkAsPending(ctx context.Context, c client.Client, obj client.Object, hooks ...runtimecatalog.Hook) error {
+	hookNames := []string{}
+	for _, hook := range hooks {
+		hookNames = append(hookNames, runtimecatalog.HookName(hook))
+	}
+
 	patchHelper, err := patch.NewHelper(obj, c)
 	if err != nil {
-		return errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: obj})
+		return errors.Wrapf(err, "failed to mark %q hook(s) as pending: failed to create patch helper for %s", strings.Join(hookNames, ","), tlog.KObj{Obj: obj})
 	}
 
 	// Read the annotation of the objects and add the hook to the comma separated list
-	hookNames := []string{}
-	for _, hook := range hooks {
-		hookNames = append(hookNames, runtimecatalog.HookName(hook))
-	}
 	annotations := obj.GetAnnotations()
 	if annotations == nil {
 		annotations = map[string]string{}
@@ -52,7 +53,7 @@ func MarkAsPending(ctx context.Context, c client.Client, obj client.Object, hook
 	obj.SetAnnotations(annotations)
 
 	if err := patchHelper.Patch(ctx, obj); err != nil {
-		return errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: obj})
+		return errors.Wrapf(err, "failed to mark %q hook(s) as pending: failed to patch %s", strings.Join(hookNames, ","), tlog.KObj{Obj: obj})
 	}
 
 	return nil
@@ -72,16 +73,17 @@ func IsPending(hook runtimecatalog.Hook, obj client.Object) bool {
 // Usually this func is called after all the registered extensions for the Hook returned an answer without requests
 // to hold on to the object's lifecycle (retryAfterSeconds).
 func MarkAsDone(ctx context.Context, c client.Client, obj client.Object, hooks ...runtimecatalog.Hook) error {
+	hookNames := []string{}
+	for _, hook := range hooks {
+		hookNames = append(hookNames, runtimecatalog.HookName(hook))
+	}
+
 	patchHelper, err := patch.NewHelper(obj, c)
 	if err != nil {
-		return errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: obj})
+		return errors.Wrapf(err, "failed to mark %q hook(s) as done: failed to create patch helper for %s", strings.Join(hookNames, ","), tlog.KObj{Obj: obj})
 	}
 
 	// Read the annotation of the objects and add the hook to the comma separated list
-	hookNames := []string{}
-	for _, hook := range hooks {
-		hookNames = append(hookNames, runtimecatalog.HookName(hook))
-	}
 	annotations := obj.GetAnnotations()
 	if annotations == nil {
 		annotations = map[string]string{}
@@ -93,7 +95,7 @@ func MarkAsDone(ctx context.Context, c client.Client, obj client.Object, hooks .
 	obj.SetAnnotations(annotations)
 
 	if err := patchHelper.Patch(ctx, obj); err != nil {
-		return errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: obj})
+		return errors.Wrapf(err, "failed to mark %q hook(s) as done: failed to patch %s", strings.Join(hookNames, ","), tlog.KObj{Obj: obj})
 	}
 
 	return nil
@@ -115,7 +117,7 @@ func IsOkToDelete(obj client.Object) bool {
 func MarkAsOkToDelete(ctx context.Context, c client.Client, obj client.Object) error {
 	patchHelper, err := patch.NewHelper(obj, c)
 	if err != nil {
-		return errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: obj})
+		return errors.Wrapf(err, "failed to mark %s as ok to delete: failed to create patch helper", tlog.KObj{Obj: obj})
 	}
 
 	annotations := obj.GetAnnotations()
@@ -126,7 +128,7 @@ func MarkAsOkToDelete(ctx context.Context, c client.Client, obj client.Object) e
 	obj.SetAnnotations(annotations)
 
 	if err := patchHelper.Patch(ctx, obj); err != nil {
-		return errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: obj})
+		return errors.Wrapf(err, "failed to mark %s as ok to delete: failed to patch", tlog.KObj{Obj: obj})
 	}
 
 	return nil