controller: fix requeueReconcileErr handling if it ever contains a reason

internal/controller/clusterpolicy_controller.go

@@ -69,6 +69,48 @@ type requeueReconcileErr struct {
 	error
 }
 
+// Is makes errors.Is(err, requeueReconcileErr{}) match any requeueReconcileErr, not just
+// the zero value. Without it errors.Is falls back to == comparison, so a
+// requeueReconcileErr wrapping a real error would never be recognised as a requeue
+// request and would be reported as a reconcile failure instead.
+func (requeueReconcileErr) Is(target error) bool {
+	_, ok := target.(requeueReconcileErr)
+
+	return ok
+}
+
+// Error tolerates a nil wrapped error. Sub-controllers return a bare requeueReconcileErr{}
+// when they only need another pass and have no failure to report, and the promoted Error
+// method would dereference that nil.
+func (e requeueReconcileErr) Error() string {
+	if e.error == nil {
+		return "sub-controller requested a requeue"
+	}
+
+	return e.error.Error()
+}
+
+// Unwrap exposes the wrapped cause to errors.As and errors.Unwrap. A bare requeue request
+// returns nil, which simply terminates the chain.
+func (e requeueReconcileErr) Unwrap() error {
+	return e.error
+}
+
+// logRequeue logs a sub-controller's requeue request under msg, appending the wrapped cause
+// when the request carried one. Without this the cause would be dropped: the requeue path
+// returns a nil error, so nothing else reports it.
+func logRequeue(err error, msg string) {
+	var requeueErr requeueReconcileErr
+
+	if errors.As(err, &requeueErr) && requeueErr.error != nil {
+		klog.Infof("%s: %v", msg, requeueErr.error)
+
+		return
+	}
+
+	klog.Info(msg)
+}
+
 type SubControllerInterface interface {
 	Reconcile(ctx context.Context, cp *v1alpha.ClusterPolicy) (ctrl.Result, error)
 }
@@ -216,7 +258,7 @@ func (r *ClusterPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 		for _, subController := range subControllers {
 			if ret, err := subController.Reconcile(ctx, cp); err != nil {
 				if errors.Is(err, requeueReconcileErr{}) {
-					klog.Info("Requeueing deletion reconciliation after sub-controller request")
+					logRequeue(err, "Requeueing deletion reconciliation after sub-controller request")
 
 					return ret, nil
 				}
@@ -247,7 +289,8 @@ func (r *ClusterPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 	for _, subController := range subControllers {
 		if ret, err := subController.Reconcile(ctx, cp); err != nil {
 			if errors.Is(err, requeueReconcileErr{}) {
-				klog.Info("Requeueing reconciliation after sub-controller request")
+				logRequeue(err, "Requeueing reconciliation after sub-controller request")
+
 				return ret, nil
 			}
 

internal/controller/clusterpolicy_controller_test.go

@@ -18,6 +18,7 @@ package controller
 
 import (
 	"context"
+	stderrors "errors"
 	"fmt"
 	"time"
 
@@ -373,3 +374,66 @@ var _ = Describe("draPodToClusterPolicy", func() {
 		Expect(reqs).To(BeNil())
 	})
 })
+
+var _ = Describe("requeueReconcileErr", func() {
+	cause := stderrors.New("waiting for resource claims to drain")
+
+	Context("sentinel matching", func() {
+		It("matches a bare requeue request", func() {
+			Expect(stderrors.Is(error(requeueReconcileErr{}), requeueReconcileErr{})).To(BeTrue())
+		})
+
+		It("matches a requeue request carrying a cause", func() {
+			// Without the Is method errors.Is falls back to == against the zero value,
+			// so this would be misreported as a reconcile failure.
+			Expect(stderrors.Is(error(requeueReconcileErr{cause}), requeueReconcileErr{})).To(BeTrue())
+		})
+
+		It("matches through an additional wrap", func() {
+			err := fmt.Errorf("dra sub-controller: %w", requeueReconcileErr{cause})
+			Expect(stderrors.Is(err, requeueReconcileErr{})).To(BeTrue())
+		})
+
+		It("does not match an unrelated error", func() {
+			Expect(stderrors.Is(cause, requeueReconcileErr{})).To(BeFalse())
+		})
+	})
+
+	Context("Error", func() {
+		It("describes a bare requeue request without dereferencing the nil cause", func() {
+			err := error(requeueReconcileErr{})
+
+			Expect(err.Error()).NotTo(BeEmpty())
+			// fmt recovers a panicking Error method and renders a PANIC marker instead
+			// of crashing, so assert on the formatted output rather than on a panic.
+			Expect(fmt.Sprintf("%v", err)).NotTo(ContainSubstring("PANIC"))
+		})
+
+		It("reports the cause when one was wrapped", func() {
+			Expect(error(requeueReconcileErr{cause}).Error()).To(Equal(cause.Error()))
+		})
+	})
+
+	Context("Unwrap", func() {
+		It("returns nil for a bare requeue request", func() {
+			Expect(stderrors.Unwrap(error(requeueReconcileErr{}))).To(Succeed())
+		})
+
+		It("exposes the cause to errors.As", func() {
+			var requeueErr requeueReconcileErr
+
+			err := fmt.Errorf("dra sub-controller: %w", requeueReconcileErr{cause})
+
+			Expect(stderrors.As(err, &requeueErr)).To(BeTrue())
+			Expect(requeueErr.Unwrap()).To(Equal(cause))
+		})
+	})
+
+	Context("logRequeue", func() {
+		It("handles a requeue request with and without a cause", func() {
+			Expect(func() { logRequeue(requeueReconcileErr{}, "requeueing") }).NotTo(Panic())
+			Expect(func() { logRequeue(requeueReconcileErr{cause}, "requeueing") }).NotTo(Panic())
+			Expect(func() { logRequeue(cause, "requeueing") }).NotTo(Panic())
+		})
+	})
+})