diff --git a/internal/controller/clusterpolicy_controller.go b/internal/controller/clusterpolicy_controller.go index b8052bf..8ac81ad 100644 --- a/internal/controller/clusterpolicy_controller.go +++ b/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 } diff --git a/internal/controller/clusterpolicy_controller_test.go b/internal/controller/clusterpolicy_controller_test.go index 3a91490..dc44c55 100644 --- a/internal/controller/clusterpolicy_controller_test.go +++ b/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()) + }) + }) +})