Skip to content

Commit

Permalink
OSSM-6153 Simplify error handling around updateStatus() (#1742)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@luksa
luksa committed Mar 27, 2024
1 parent cc22504 commit 2ee8636
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 67 deletions.
36 changes: 15 additions & 21 deletions controllers/istio/istio_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package istio

import (
"context"
"errors"
"fmt"
"path"
"reflect"
Expand All @@ -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"
Expand Down Expand Up @@ -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
}
Expand All @@ -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
Expand Down Expand Up @@ -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{
Expand All @@ -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
}
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion controllers/istio/istio_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
27 changes: 8 additions & 19 deletions controllers/istiocni/istiocni_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package istiocni

import (
"context"
"errors"
"fmt"
"path"
"reflect"
Expand All @@ -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"
Expand Down Expand Up @@ -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
}
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
36 changes: 10 additions & 26 deletions controllers/istiorevision/istiorevision_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -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
}
Expand All @@ -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 {
Expand Down Expand Up @@ -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)

Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down

0 comments on commit 2ee8636

Please sign in to comment.