diff --git a/controllers/ippool_controller.go b/controllers/ippool_controller.go index 22b71501..8f80fd3a 100644 --- a/controllers/ippool_controller.go +++ b/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) } diff --git a/ipam/ippool_manager.go b/ipam/ippool_manager.go index 916ff3f2..f4382710 100644 --- a/ipam/ippool_manager.go +++ b/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 diff --git a/ipam/ippool_manager_test.go b/ipam/ippool_manager_test.go index 0ba20ab6..7afebc4f 100644 --- a/ipam/ippool_manager_test.go +++ b/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()) diff --git a/ipam/reconcile_error.go b/ipam/reconcile_error.go new file mode 100644 index 00000000..03748c64 --- /dev/null +++ b/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} +} diff --git a/ipam/reconcile_error_test.go b/ipam/reconcile_error_test.go new file mode 100644 index 00000000..38055866 --- /dev/null +++ b/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"))) + }) +}) diff --git a/ipam/requeue_error.go b/ipam/requeue_error.go deleted file mode 100644 index bfa5622c..00000000 --- a/ipam/requeue_error.go +++ /dev/null @@ -1,49 +0,0 @@ -/* -Copyright 2018 The Kubernetes 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" -) - -// HasRequeueAfterError represents that an actuator managed object should -// be requeued for further processing after the given RequeueAfter time has -// passed. -type HasRequeueAfterError interface { - // GetRequeueAfter gets the duration to wait until the managed object is - // requeued for further processing. - GetRequeueAfter() time.Duration -} - -// RequeueAfterError represents that an actuator managed object should be -// requeued for further processing after the given RequeueAfter time has -// passed. -type RequeueAfterError struct { - RequeueAfter time.Duration -} - -// Error implements the error interface. -func (e *RequeueAfterError) Error() string { - return fmt.Sprintf("requeue in: %s", e.RequeueAfter) -} - -// GetRequeueAfter gets the duration to wait until the managed object is -// requeued for further processing. -func (e *RequeueAfterError) GetRequeueAfter() time.Duration { - return e.RequeueAfter -} diff --git a/ipam/requeue_error_test.go b/ipam/requeue_error_test.go deleted file mode 100644 index 2176b20c..00000000 --- a/ipam/requeue_error_test.go +++ /dev/null @@ -1,43 +0,0 @@ -/* -Copyright 2018 The Kubernetes 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" - - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" -) - -const ( - RequeueDuration1 = 50 - RequeueDuration2 = 40 -) - -var _ = Describe("Errors testing", func() { - It("returns the correct error", func() { - err := &RequeueAfterError{time.Second * RequeueDuration1} - Expect(err.Error()).To(Equal(fmt.Sprintf("requeue in: %vs", RequeueDuration1))) - }) - - It("Gets the correct duration", func() { - duration, _ := time.ParseDuration(fmt.Sprintf("%vs", RequeueDuration2)) - err := &RequeueAfterError{time.Second * RequeueDuration2} - Expect(err.GetRequeueAfter()).To(Equal(duration)) - }) -}) diff --git a/ipam/utils.go b/ipam/utils.go index 83df19af..07c02001 100644 --- a/ipam/utils.go +++ b/ipam/utils.go @@ -18,6 +18,8 @@ package ipam import ( "context" + "fmt" + "time" "github.com/pkg/errors" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -61,7 +63,7 @@ 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 } @@ -69,7 +71,8 @@ func updateObject(ctx context.Context, cl client.Client, obj client.Object, opts 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 } diff --git a/ipam/utils_test.go b/ipam/utils_test.go index 3272e906..f767457f 100644 --- a/ipam/utils_test.go +++ b/ipam/utils_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))