From 2ee8636d967987cf61a1c204b227c95211e32127 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Luk=C5=A1a?= Date: Wed, 27 Mar 2024 12:59:09 +0100 Subject: [PATCH] OSSM-6153 Simplify error handling around updateStatus() (#1742) Previously, when both a reconcile and a status update error occurred, we logged the update error and returned the reconcile error. Now, we simply return a composite of both errors. This commit also removes unneeded conflict handling, since status is always updated via PATCH, which does not return 409 Conflict. --- controllers/istio/istio_controller.go | 36 ++++++++----------- controllers/istio/istio_controller_test.go | 2 +- controllers/istiocni/istiocni_controller.go | 27 +++++--------- .../istiorevision/istiorevision_controller.go | 36 ++++++------------- 4 files changed, 34 insertions(+), 67 deletions(-) diff --git a/controllers/istio/istio_controller.go b/controllers/istio/istio_controller.go index f05a5c8d32..2e33aaf160 100644 --- a/controllers/istio/istio_controller.go +++ b/controllers/istio/istio_controller.go @@ -16,6 +16,7 @@ package istio import ( "context" + "errors" "fmt" "path" "reflect" @@ -27,7 +28,7 @@ import ( "github.com/istio-ecosystem/sail-operator/pkg/common" "github.com/istio-ecosystem/sail-operator/pkg/kube" "github.com/istio-ecosystem/sail-operator/pkg/profiles" - "k8s.io/apimachinery/pkg/api/errors" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -71,7 +72,7 @@ func (r *IstioReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl var istio v1alpha1.Istio if err := r.Client.Get(ctx, req.NamespacedName, &istio); err != nil { - if errors.IsNotFound(err) { + if apierrors.IsNotFound(err) { log.V(2).Info("Istio not found. Skipping reconciliation") return ctrl.Result{}, nil } @@ -83,15 +84,12 @@ func (r *IstioReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl } log.Info("Reconciling") - result, err := r.doReconcile(ctx, istio) + result, reconcileErr := r.doReconcile(ctx, istio) log.Info("Reconciliation done. Updating status.") - err = r.updateStatus(ctx, &istio, err) - if errors.IsConflict(err) { - log.Info("Status update failed. Requeuing reconciliation") - return ctrl.Result{RequeueAfter: 2 * time.Second}, nil - } - return result, err + statusErr := r.updateStatus(ctx, &istio, reconcileErr) + + return result, errors.Join(reconcileErr, statusErr) } // doReconcile is the function that actually reconciles the Istio object. Any error reported by this @@ -129,11 +127,11 @@ func (r *IstioReconciler) reconcileActiveRevision(ctx context.Context, istio *v1 rev.Spec.Version = istio.Spec.Version rev.Spec.Values = values log.Info("Updating IstioRevision") - if err = r.Client.Update(ctx, &rev); errors.IsConflict(err) { + if err = r.Client.Update(ctx, &rev); apierrors.IsConflict(err) { log.Info("IstioRevision update failed. Requeuing reconciliation") return ctrl.Result{RequeueAfter: 2 * time.Second}, nil } - } else if errors.IsNotFound(err) { + } else if apierrors.IsNotFound(err) { // create new rev = v1alpha1.IstioRevision{ ObjectMeta: metav1.ObjectMeta{ @@ -156,7 +154,7 @@ func (r *IstioReconciler) reconcileActiveRevision(ctx context.Context, istio *v1 }, } log.Info("Creating IstioRevision") - if err = r.Client.Create(ctx, &rev); errors.IsForbidden(err) && strings.Contains(err.Error(), "RESTMapping") { + if err = r.Client.Create(ctx, &rev); apierrors.IsForbidden(err) && strings.Contains(err.Error(), "RESTMapping") { log.Info("APIServer seems to be not ready - RESTMapper of gc admission plugin is not up to date. Trying again in 5 seconds", "error", err) return ctrl.Result{RequeueAfter: 5 * time.Second}, nil } @@ -398,17 +396,17 @@ func (r *IstioReconciler) SetupWithManager(mgr ctrl.Manager) error { Complete(r) } -func (r *IstioReconciler) updateStatus(ctx context.Context, istio *v1alpha1.Istio, reconciliationErr error) error { +func (r *IstioReconciler) updateStatus(ctx context.Context, istio *v1alpha1.Istio, reconcileErr error) error { status := istio.Status.DeepCopy() status.ObservedGeneration = istio.Generation // set Reconciled and Ready conditions - if reconciliationErr != nil { + if reconcileErr != nil { status.SetCondition(v1alpha1.IstioCondition{ Type: v1alpha1.IstioConditionReconciled, Status: metav1.ConditionFalse, Reason: v1alpha1.IstioReasonReconcileError, - Message: reconciliationErr.Error(), + Message: reconcileErr.Error(), }) status.SetCondition(v1alpha1.IstioCondition{ Type: v1alpha1.IstioConditionReady, @@ -419,7 +417,7 @@ func (r *IstioReconciler) updateStatus(ctx context.Context, istio *v1alpha1.Isti status.State = v1alpha1.IstioReasonReconcileError } else { rev, err := r.getActiveRevision(ctx, istio) - if errors.IsNotFound(err) { + if apierrors.IsNotFound(err) { revisionNotFound := func(conditionType v1alpha1.IstioConditionType) v1alpha1.IstioCondition { return v1alpha1.IstioCondition{ Type: conditionType, @@ -462,11 +460,7 @@ func (r *IstioReconciler) updateStatus(ctx context.Context, istio *v1alpha1.Isti return nil } - statusErr := r.Client.Status().Patch(ctx, istio, kube.NewStatusPatch(*status)) - if statusErr != nil { - return statusErr - } - return reconciliationErr + return r.Client.Status().Patch(ctx, istio, kube.NewStatusPatch(*status)) } func convertCondition(condition v1alpha1.IstioRevisionCondition) v1alpha1.IstioCondition { diff --git a/controllers/istio/istio_controller_test.go b/controllers/istio/istio_controller_test.go index 972029e6e0..b8caf28741 100644 --- a/controllers/istio/istio_controller_test.go +++ b/controllers/istio/istio_controller_test.go @@ -271,7 +271,7 @@ func TestUpdateStatus(t *testing.T) { { name: "reconciliation error", reconciliationErr: fmt.Errorf("reconciliation error"), - wantErr: true, + wantErr: false, expectedStatus: v1alpha1.IstioStatus{ State: v1alpha1.IstioReasonReconcileError, ObservedGeneration: generation, diff --git a/controllers/istiocni/istiocni_controller.go b/controllers/istiocni/istiocni_controller.go index 9bccda17c4..b9c23cc753 100644 --- a/controllers/istiocni/istiocni_controller.go +++ b/controllers/istiocni/istiocni_controller.go @@ -16,6 +16,7 @@ package istiocni import ( "context" + "errors" "fmt" "path" "reflect" @@ -29,7 +30,7 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" - "k8s.io/apimachinery/pkg/api/errors" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/rest" @@ -93,7 +94,7 @@ func (r *IstioCNIReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c log := logf.FromContext(ctx) var cni v1alpha1.IstioCNI if err := r.Client.Get(ctx, req.NamespacedName, &cni); err != nil { - if errors.IsNotFound(err) { + if apierrors.IsNotFound(err) { log.V(2).Info("IstioCNI not found. Skipping reconciliation") return ctrl.Result{}, nil } @@ -116,12 +117,12 @@ func (r *IstioCNIReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c } log.Info("Installing components") - err := r.installHelmChart(ctx, &cni) + reconcileErr := r.installHelmChart(ctx, &cni) log.Info("Reconciliation done. Updating status.") - err = r.updateStatus(ctx, &cni, err) + statusErr := r.updateStatus(ctx, &cni, reconcileErr) - return ctrl.Result{}, err + return ctrl.Result{}, errors.Join(reconcileErr, statusErr) } func validateIstioCNI(cni v1alpha1.IstioCNI) error { @@ -252,19 +253,7 @@ func (r *IstioCNIReconciler) updateStatus(ctx context.Context, cni *v1alpha1.Ist if reflect.DeepEqual(cni.Status, status) { return nil } - - statusErr := r.Client.Status().Patch(ctx, cni, kube.NewStatusPatch(status)) - if statusErr != nil { - log := logf.FromContext(ctx) - log.Error(statusErr, "failed to patch status") - - // ensure that we retry the reconcile by returning the status error - // (but without overriding the original error) - if reconcileErr == nil { - return statusErr - } - } - return reconcileErr + return r.Client.Status().Patch(ctx, cni, kube.NewStatusPatch(status)) } func deriveState(reconciledCondition, readyCondition v1alpha1.IstioCNICondition) v1alpha1.IstioCNIConditionReason { @@ -306,7 +295,7 @@ func (r *IstioCNIReconciler) determineReadyCondition(ctx context.Context, cni *v } else { c.Status = metav1.ConditionTrue } - } else if errors.IsNotFound(err) { + } else if apierrors.IsNotFound(err) { c.Reason = v1alpha1.IstioCNIDaemonSetNotReady c.Message = "istio-cni-node DaemonSet not found" } else { diff --git a/controllers/istiorevision/istiorevision_controller.go b/controllers/istiorevision/istiorevision_controller.go index 98d0ba5220..dbc13d6b0e 100644 --- a/controllers/istiorevision/istiorevision_controller.go +++ b/controllers/istiorevision/istiorevision_controller.go @@ -16,11 +16,11 @@ package istiorevision import ( "context" + "errors" "fmt" "path" "reflect" "regexp" - "time" "github.com/go-logr/logr" "github.com/istio-ecosystem/sail-operator/api/v1alpha1" @@ -33,7 +33,7 @@ import ( corev1 "k8s.io/api/core/v1" policyv1 "k8s.io/api/policy/v1" rbacv1 "k8s.io/api/rbac/v1" - "k8s.io/apimachinery/pkg/api/errors" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -101,7 +101,7 @@ func (r *IstioRevisionReconciler) Reconcile(ctx context.Context, req ctrl.Reques log := logf.FromContext(ctx) var rev v1alpha1.IstioRevision if err := r.Client.Get(ctx, req.NamespacedName, &rev); err != nil { - if errors.IsNotFound(err) { + if apierrors.IsNotFound(err) { log.V(2).Info("IstioRevision not found. Skipping reconciliation") return ctrl.Result{}, nil } @@ -124,16 +124,12 @@ func (r *IstioRevisionReconciler) Reconcile(ctx context.Context, req ctrl.Reques } log.Info("Installing components") - err := r.installHelmCharts(ctx, &rev) + reconcileErr := r.installHelmCharts(ctx, &rev) log.Info("Reconciliation done. Updating status.") - err = r.updateStatus(ctx, &rev, err) - if errors.IsConflict(err) { - log.Info("Status update failed. Requeuing reconciliation") - return ctrl.Result{RequeueAfter: 2 * time.Second}, nil - } + statusErr := r.updateStatus(ctx, &rev, reconcileErr) - return ctrl.Result{}, err + return ctrl.Result{}, errors.Join(reconcileErr, statusErr) } func validateIstioRevision(rev v1alpha1.IstioRevision) error { @@ -243,9 +239,8 @@ func (r *IstioRevisionReconciler) SetupWithManager(mgr ctrl.Manager) error { Complete(r) } -func (r *IstioRevisionReconciler) updateStatus(ctx context.Context, rev *v1alpha1.IstioRevision, err error) error { - log := logf.FromContext(ctx) - reconciledCondition := r.determineReconciledCondition(err) +func (r *IstioRevisionReconciler) updateStatus(ctx context.Context, rev *v1alpha1.IstioRevision, reconcileErr error) error { + reconciledCondition := r.determineReconciledCondition(reconcileErr) readyCondition := r.determineReadyCondition(ctx, rev) inUseCondition := r.determineInUseCondition(ctx, rev) @@ -259,18 +254,7 @@ func (r *IstioRevisionReconciler) updateStatus(ctx context.Context, rev *v1alpha if reflect.DeepEqual(rev.Status, *status) { return nil } - - statusErr := r.Client.Status().Patch(ctx, rev, kube.NewStatusPatch(*status)) - if statusErr != nil { - log.Error(statusErr, "failed to patch status") - - // ensure that we retry the reconcile by returning the status error - // (but without overriding the original error) - if err == nil { - return statusErr - } - } - return err + return r.Client.Status().Patch(ctx, rev, kube.NewStatusPatch(*status)) } func deriveState(reconciledCondition, readyCondition v1alpha1.IstioRevisionCondition) v1alpha1.IstioRevisionConditionReason { @@ -312,7 +296,7 @@ func (r *IstioRevisionReconciler) determineReadyCondition(ctx context.Context, r } else { c.Status = metav1.ConditionTrue } - } else if errors.IsNotFound(err) { + } else if apierrors.IsNotFound(err) { c.Reason = v1alpha1.IstioRevisionReasonIstiodNotReady c.Message = "istiod Deployment not found" } else {