Merge pull request #787 from Nordix/replace-requeafterError-type/sunnat

🌱 Introduce ReconcileError with Transient and Terminal Error type

Author: metal3-io-bot

controllers/ippool_controller.go

@@ -34,6 +34,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 )
 
 const (
@@ -147,7 +148,7 @@ func (r *IPPoolReconciler) reconcileNormal(ctx context.Context,
 
 	_, err := ipPoolMgr.UpdateAddresses(ctx)
 	if err != nil {
-		return checkRequeueError(err, "Failed to create the missing data")
+		return checkReconcileError(err, "Failed to create the missing data")
 	}
 
 	return ctrl.Result{}, nil
@@ -158,7 +159,7 @@ func (r *IPPoolReconciler) reconcileDelete(ctx context.Context,
 ) (ctrl.Result, error) {
 	allocationsNb, err := ipPoolMgr.UpdateAddresses(ctx)
 	if err != nil {
-		return checkRequeueError(err, "Failed to delete the old secrets")
+		return checkReconcileError(err, "Failed to delete the old secrets")
 	}
 
 	if allocationsNb == 0 {
@@ -206,13 +207,21 @@ func (r *IPPoolReconciler) IPClaimToIPPool(_ context.Context, obj client.Object)
 	return []ctrl.Request{}
 }
 
-func checkRequeueError(err error, errMessage string) (ctrl.Result, error) {
+// checkReconcileError checks if the error is a transient or terminal error.
+// If it is transient, it returns a Result with Requeue set to true.
+// Non-reconcile errors are returned as-is.
+func checkReconcileError(err error, errMessage string) (ctrl.Result, error) {
 	if err == nil {
 		return ctrl.Result{}, nil
 	}
-	var requeueErr ipam.HasRequeueAfterError
-	if ok := errors.As(err, &requeueErr); ok {
-		return ctrl.Result{Requeue: true, RequeueAfter: requeueErr.GetRequeueAfter()}, nil
+	var reconcileError ipam.ReconcileError
+	if errors.As(err, &reconcileError) {
+		if reconcileError.IsTransient() {
+			return reconcile.Result{Requeue: true, RequeueAfter: reconcileError.GetRequeueAfter()}, nil
+		}
+		if reconcileError.IsTerminal() {
+			return reconcile.Result{}, nil
+		}
 	}
 	return ctrl.Result{}, errors.Wrap(err, errMessage)
 }

ipam/ippool_manager.go

@@ -397,11 +397,11 @@ func (m *IPPoolManager) createAddress(ctx context.Context,
 	}
 
 	// Create the IPAddress object. If we get a conflict (that will set
-	// HasRequeueAfterError), then requeue to retrigger the reconciliation with
+	// Transient error), then requeue to retrigger the reconciliation with
 	// the new state
 	if err := createObject(ctx, m.client, addressObject); err != nil {
-		var reqAfter *RequeueAfterError
-		if ok := errors.As(err, &reqAfter); !ok {
+		var reconcileError ReconcileError
+		if !errors.As(err, &reconcileError) {
 			addressClaim.Status.ErrorMessage = ptr.To("Failed to create associated IPAddress object")
 		}
 		return addresses, err

ipam/ippool_manager_test.go

@@ -334,9 +334,9 @@ var _ = Describe("IPPool manager", func() {
 			if tc.expectRequeue || tc.expectError {
 				Expect(err).To(HaveOccurred())
 				if tc.expectRequeue {
-					Expect(err).To(BeAssignableToTypeOf(&RequeueAfterError{}))
+					Expect(err).To(BeAssignableToTypeOf(ReconcileError{}))
 				} else {
-					Expect(err).NotTo(BeAssignableToTypeOf(&RequeueAfterError{}))
+					Expect(err).NotTo(BeAssignableToTypeOf(ReconcileError{}))
 				}
 			} else {
 				Expect(err).NotTo(HaveOccurred())
@@ -596,9 +596,9 @@ var _ = Describe("IPPool manager", func() {
 			if tc.expectRequeue || tc.expectError {
 				Expect(err).To(HaveOccurred())
 				if tc.expectRequeue {
-					Expect(err).To(BeAssignableToTypeOf(&RequeueAfterError{}))
+					Expect(err).To(BeAssignableToTypeOf(ReconcileError{}))
 				} else {
-					Expect(err).NotTo(BeAssignableToTypeOf(&RequeueAfterError{}))
+					Expect(err).NotTo(BeAssignableToTypeOf(ReconcileError{}))
 				}
 			} else {
 				Expect(err).NotTo(HaveOccurred())

ipam/reconcile_error.go

@@ -0,0 +1,85 @@
+/*
+Copyright 2024 The Metal3 Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package ipam
+
+import (
+	"fmt"
+	"time"
+)
+
+// ReconcileError represents an generic error of Reconcile loop. ErrorType indicates what type
+// of action is required to recover. It can take two values:
+// 1. `Transient` - Can be recovered , will be requeued after.
+// 2. `Terminal` - Cannot be recovered, will not be requeued.
+
+type ReconcileError struct {
+	error
+	errorType    ReconcileErrorType
+	RequeueAfter time.Duration
+}
+
+// ReconcileErrorType represents the type of a ReconcileError.
+type ReconcileErrorType string
+
+const (
+	// TransientErrorType can be recovered, will be requeued after a configured time interval.
+	TransientErrorType ReconcileErrorType = "Transient"
+	// TerminalErrorType cannot be recovered, will not be requeued.
+	TerminalErrorType ReconcileErrorType = "Terminal"
+)
+
+// Error returns the error message for a ReconcileError.
+func (e ReconcileError) Error() string {
+	var errStr string
+	if e.error != nil {
+		errStr = e.error.Error()
+	}
+	switch e.errorType {
+	case TransientErrorType:
+		return fmt.Sprintf("%s. Object will be requeued after %s", errStr, e.GetRequeueAfter())
+	case TerminalErrorType:
+		return fmt.Sprintf("reconcile error that cannot be recovered occurred: %s. Object will not be requeued", errStr)
+	default:
+		return fmt.Sprintf("reconcile error occurred with unknown recovery type. The actual error is: %s", errStr)
+	}
+}
+
+// GetRequeueAfter gets the duration to wait until the managed object is
+// requeued for further processing.
+func (e ReconcileError) GetRequeueAfter() time.Duration {
+	return e.RequeueAfter
+}
+
+// IsTransient returns if the ReconcileError is recoverable.
+func (e ReconcileError) IsTransient() bool {
+	return e.errorType == TransientErrorType
+}
+
+// IsTerminal returns if the ReconcileError is non recoverable.
+func (e ReconcileError) IsTerminal() bool {
+	return e.errorType == TerminalErrorType
+}
+
+// WithTransientError wraps the error in a ReconcileError with errorType as `Transient`.
+func WithTransientError(err error, requeueAfter time.Duration) ReconcileError {
+	return ReconcileError{error: err, errorType: TransientErrorType, RequeueAfter: requeueAfter}
+}
+
+// WithTerminalError wraps the error in a ReconcileError with errorType as `Terminal`.
+func WithTerminalError(err error) ReconcileError {
+	return ReconcileError{error: err, errorType: TerminalErrorType}
+}

ipam/reconcile_error_test.go

@@ -0,0 +1,56 @@
+/*
+Copyright 2024 The Metal3 Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package ipam
+
+import (
+	"errors"
+	"fmt"
+	"time"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+const (
+	duration = 50 * time.Second
+)
+
+var _ = Describe("Reconcile Error testing", func() {
+
+	It("Returns correct values for Transient Error", func() {
+
+		err := WithTransientError(errors.New("Transient Error"), duration)
+		Expect(err.GetRequeueAfter()).To(Equal(duration))
+		Expect(err.IsTransient()).To(BeTrue())
+		Expect(err.IsTerminal()).To(BeFalse())
+		Expect(err.Error()).To(Equal(fmt.Sprintf("%s. Object will be requeued after %s", "Transient Error", duration)))
+	})
+
+	It("Returns correct values for Terminal Error", func() {
+		err := WithTerminalError(errors.New("Terminal Error"))
+		Expect(err.IsTransient()).To(BeFalse())
+		Expect(err.IsTerminal()).To(BeTrue())
+		Expect(err.Error()).To(Equal(fmt.Sprintf("reconcile error that cannot be recovered occurred: %s. Object will not be requeued", "Terminal Error")))
+	})
+
+	It("Returns correct values for Unknown ReconcileError type", func() {
+		err := ReconcileError{errors.New("Unknown Error"), "unknownErrorType", 0 * time.Second}
+		Expect(err.IsTerminal()).To(BeFalse())
+		Expect(err.IsTransient()).To(BeFalse())
+		Expect(err.Error()).To(Equal(fmt.Sprintf("reconcile error occurred with unknown recovery type. The actual error is: %s", "Unknown Error")))
+	})
+})

ipam/requeue_error.go

@@ -18,6 +18,8 @@ package ipam
 
 import (
 	"context"
+	"fmt"
+	"time"
 
 	"github.com/pkg/errors"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -61,15 +63,16 @@ func (e *NotFoundError) Error() string {
 func updateObject(ctx context.Context, cl client.Client, obj client.Object, opts ...client.UpdateOption) error {
 	err := cl.Update(ctx, obj.DeepCopyObject().(client.Object), opts...)
 	if apierrors.IsConflict(err) {
-		return &RequeueAfterError{}
+		return WithTransientError(errors.New("Updating object failed"), 0*time.Second)
 	}
 	return err
 }
 
 func createObject(ctx context.Context, cl client.Client, obj client.Object, opts ...client.CreateOption) error {
 	err := cl.Create(ctx, obj.DeepCopyObject().(client.Object), opts...)
 	if apierrors.IsAlreadyExists(err) {
-		return &RequeueAfterError{}
+		fmt.Printf("I am inside IsAlreadyExists")
+		return WithTransientError(errors.New("Object already exists"), 0*time.Second)
 	}
 	return err
 }

ipam/requeue_error_test.go

@@ -130,7 +130,7 @@ var _ = Describe("Metal3 manager utils", func() {
 			err := updateObject(context.TODO(), c, obj)
 			if tc.ExpectedError {
 				Expect(err).To(HaveOccurred())
-				Expect(err).NotTo(BeAssignableToTypeOf(&RequeueAfterError{}))
+				Expect(err).NotTo(BeAssignableToTypeOf(ReconcileError{}))
 			} else {
 				Expect(err).NotTo(HaveOccurred())
 				Expect(obj.Spec).To(Equal(tc.TestObject.Spec))
@@ -148,7 +148,7 @@ var _ = Describe("Metal3 manager utils", func() {
 				Expect(savedObject.ResourceVersion).NotTo(Equal(tc.TestObject.ResourceVersion))
 				err := updateObject(context.TODO(), c, obj)
 				Expect(err).To(HaveOccurred())
-				Expect(err).To(BeAssignableToTypeOf(&RequeueAfterError{}))
+				Expect(err).To(BeAssignableToTypeOf(ReconcileError{}))
 			}
 			err = c.Delete(context.TODO(), tc.TestObject)
 			if err != nil {
@@ -211,7 +211,7 @@ var _ = Describe("Metal3 manager utils", func() {
 			err := createObject(context.TODO(), c, obj)
 			if tc.ExpectedError {
 				Expect(err).To(HaveOccurred())
-				Expect(err).To(BeAssignableToTypeOf(&RequeueAfterError{}))
+				Expect(err).To(BeAssignableToTypeOf(ReconcileError{}))
 			} else {
 				Expect(err).NotTo(HaveOccurred())
 				Expect(obj.Spec).To(Equal(tc.TestObject.Spec))