From 8d6ae965e450c535bd3d8cd2dbcd530c9e3173c6 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Tue, 21 May 2024 11:52:07 -0400 Subject: [PATCH] Update error handling in reconciler Signed-off-by: Todd Short --- .../clusterextension_controller.go | 118 +++++------------- .../clusterextension_controller_test.go | 10 +- 2 files changed, 40 insertions(+), 88 deletions(-) diff --git a/internal/controllers/clusterextension_controller.go b/internal/controllers/clusterextension_controller.go index f9243000..3f6fcb81 100644 --- a/internal/controllers/clusterextension_controller.go +++ b/internal/controllers/clusterextension_controller.go @@ -38,14 +38,12 @@ import ( "helm.sh/helm/v3/pkg/storage/driver" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" - apierrors "k8s.io/apimachinery/pkg/api/errors" apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" - utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/sets" apimachyaml "k8s.io/apimachinery/pkg/util/yaml" ctrl "sigs.k8s.io/controller-runtime" @@ -80,6 +78,17 @@ import ( "github.com/operator-framework/operator-controller/internal/labels" ) +// ResolutionError type +// If a more specific error type needs to be distinguished, +// add another type here +type ResolutionError struct { + message string +} + +func (e ResolutionError) Error() string { + return e.message +} + // ClusterExtensionReconciler reconciles a ClusterExtension object type ClusterExtensionReconciler struct { client.Client @@ -115,13 +124,14 @@ func (r *ClusterExtensionReconciler) Reconcile(ctx context.Context, req ctrl.Req var existingExt = &ocv1alpha1.ClusterExtension{} if err := r.Client.Get(ctx, req.NamespacedName, existingExt); err != nil { - return ctrl.Result{}, utilerrors.NewAggregate([]error{client.IgnoreNotFound(err), nil}) + return ctrl.Result{}, client.IgnoreNotFound(err) } - reconciledExt := existingExt.DeepCopy() - res, reconcileErr := r.reconcile(ctx, reconciledExt) + var updateError error - var updateErrors []error + reconciledExt := existingExt.DeepCopy() + res, err := r.reconcile(ctx, reconciledExt) + updateError = errors.Join(updateError, err) // Do checks before any Update()s, as Update() may modify the resource structure! updateStatus := !equality.Semantic.DeepEqual(existingExt.Status, reconciledExt.Status) @@ -129,9 +139,8 @@ func (r *ClusterExtensionReconciler) Reconcile(ctx context.Context, req ctrl.Req unexpectedFieldsChanged := checkForUnexpectedFieldChange(*existingExt, *reconciledExt) if updateStatus { - if updateErr := r.Client.Status().Update(ctx, reconciledExt); updateErr != nil { - updateErrors = append(updateErrors, updateErr) - } + err = r.Client.Status().Update(ctx, reconciledExt) + updateError = errors.Join(updateError, err) } if unexpectedFieldsChanged { @@ -139,16 +148,11 @@ func (r *ClusterExtensionReconciler) Reconcile(ctx context.Context, req ctrl.Req } if updateFinalizers { - if updateErr := r.Client.Update(ctx, reconciledExt); updateErr != nil { - updateErrors = append(updateErrors, updateErr) - } - } - - if reconcileErr != nil { - updateErrors = append(updateErrors, reconcileErr) + err = r.Client.Update(ctx, reconciledExt) + updateError = errors.Join(updateError, err) } - return res, utilerrors.NewAggregate(updateErrors) + return res, updateError } // ensureAllConditionsWithReason checks that all defined condition types exist in the given ClusterExtension, @@ -179,29 +183,15 @@ func checkForUnexpectedFieldChange(a, b ocv1alpha1.ClusterExtension) bool { } func (r *ClusterExtensionReconciler) handleResolutionErrors(ext *ocv1alpha1.ClusterExtension, err error) (ctrl.Result, error) { - var aggErrs utilerrors.Aggregate - if errors.As(err, &aggErrs) { - for _, err := range aggErrs.Errors() { - errorMessage := err.Error() - if strings.Contains(errorMessage, "no package") { - // Handle no package found errors, potentially setting status conditions - setResolvedStatusConditionFailed(&ext.Status.Conditions, errorMessage, ext.Generation) - ensureAllConditionsWithReason(ext, "ResolutionFailed", errorMessage) - } else if strings.Contains(errorMessage, "invalid version range") { - // Handle invalid version range errors, potentially setting status conditions - setResolvedStatusConditionFailed(&ext.Status.Conditions, errorMessage, ext.Generation) - ensureAllConditionsWithReason(ext, "ResolutionFailed", errorMessage) - } else { - // General error handling - setResolvedStatusConditionFailed(&ext.Status.Conditions, errorMessage, ext.Generation) - ensureAllConditionsWithReason(ext, "InstallationStatusUnknown", "") - } - } + errorMessage := err.Error() + if errors.As(err, &ResolutionError{}) { + // Handle resolution errors + setResolvedStatusConditionFailed(&ext.Status.Conditions, errorMessage, ext.Generation) + ensureAllConditionsWithReason(ext, ocv1alpha1.ReasonResolutionFailed, errorMessage) } else { - // If the error is not an aggregate, handle it as a general error - errorMessage := err.Error() + // General error handling setResolvedStatusConditionFailed(&ext.Status.Conditions, errorMessage, ext.Generation) - ensureAllConditionsWithReason(ext, "InstallationStatusUnknown", "") + ensureAllConditionsWithReason(ext, ocv1alpha1.ReasonInstallationStatusUnknown, errorMessage) } ext.Status.ResolvedBundle = nil return ctrl.Result{}, err @@ -337,26 +327,17 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp return nil }, helmclient.AppendInstallPostRenderer(post)) if err != nil { - if isResourceNotFoundErr(err) { - err = errRequiredResourceNotFound{err} - } setInstalledStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonInstallationFailed, err), ext.Generation) return ctrl.Result{}, err } case stateNeedsUpgrade: rel, err = ac.Upgrade(ext.GetName(), r.ReleaseNamespace, chrt, values, helmclient.AppendUpgradePostRenderer(post)) if err != nil { - if isResourceNotFoundErr(err) { - err = errRequiredResourceNotFound{err} - } setInstalledStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonUpgradeFailed, err), ext.Generation) return ctrl.Result{}, err } case stateUnchanged: if err := ac.Reconcile(rel); err != nil { - if isResourceNotFoundErr(err) { - err = errRequiredResourceNotFound{err} - } setInstalledStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonResolutionFailed, err), ext.Generation) return ctrl.Result{}, err } @@ -409,7 +390,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp func (r *ClusterExtensionReconciler) resolve(ctx context.Context, ext ocv1alpha1.ClusterExtension) (*catalogmetadata.Bundle, error) { allBundles, err := r.BundleProvider.Bundles(ctx) if err != nil { - return nil, utilerrors.NewAggregate([]error{fmt.Errorf("error fetching bundles: %w", err)}) + return nil, fmt.Errorf("error fetching bundles: %w", err) } packageName := ext.Spec.PackageName @@ -432,7 +413,7 @@ func (r *ClusterExtensionReconciler) resolve(ctx context.Context, ext ocv1alpha1 if versionRange != "" { vr, err := mmsemver.NewConstraint(versionRange) if err != nil { - return nil, utilerrors.NewAggregate([]error{fmt.Errorf("invalid version range '%s': %w", versionRange, err)}) + return nil, ResolutionError{fmt.Sprintf("invalid version range %q: %v", versionRange, err)} } predicates = append(predicates, catalogfilter.InMastermindsSemverRange(vr)) } @@ -459,13 +440,13 @@ func (r *ClusterExtensionReconciler) resolve(ctx context.Context, ext ocv1alpha1 if len(resultSet) == 0 { switch { case versionRange != "" && channelName != "": - return nil, fmt.Errorf("%sno package %q matching version %q in channel %q found", upgradeErrorPrefix, packageName, versionRange, channelName) + return nil, ResolutionError{fmt.Sprintf("%sno package %q matching version %q in channel %q found", upgradeErrorPrefix, packageName, versionRange, channelName)} case versionRange != "": - return nil, fmt.Errorf("%sno package %q matching version %q found", upgradeErrorPrefix, packageName, versionRange) + return nil, ResolutionError{fmt.Sprintf("%sno package %q matching version %q found", upgradeErrorPrefix, packageName, versionRange)} case channelName != "": - return nil, fmt.Errorf("%sno package %q in channel %q found", upgradeErrorPrefix, packageName, channelName) + return nil, ResolutionError{fmt.Sprintf("%sno package %q in channel %q found", upgradeErrorPrefix, packageName, channelName)} default: - return nil, fmt.Errorf("%sno package %q found", upgradeErrorPrefix, packageName) + return nil, ResolutionError{fmt.Sprintf("%sno package %q found", upgradeErrorPrefix, packageName)} } } @@ -746,37 +727,6 @@ var GetInstalledbundle = func(ctx context.Context, acg helmclient.ActionClientGe return resultSet[0], nil } -type errRequiredResourceNotFound struct { - error -} - -func (err errRequiredResourceNotFound) Error() string { - return fmt.Sprintf("required resource not found: %v", err.error) -} - -func isResourceNotFoundErr(err error) bool { - var agg utilerrors.Aggregate - if errors.As(err, &agg) { - for _, err := range agg.Errors() { - return isResourceNotFoundErr(err) - } - } - - nkme := &apimeta.NoKindMatchError{} - if errors.As(err, &nkme) { - return true - } - if apierrors.IsNotFound(err) { - return true - } - - // TODO: improve NoKindMatchError matching - // An error that is bubbled up from the k8s.io/cli-runtime library - // does not wrap meta.NoKindMatchError, so we need to fallback to - // the use of string comparisons for now. - return strings.Contains(err.Error(), "no matches for kind") -} - type postrenderer struct { labels map[string]string cascade postrender.PostRenderer diff --git a/internal/controllers/clusterextension_controller_test.go b/internal/controllers/clusterextension_controller_test.go index 08fd0da2..383cb85b 100644 --- a/internal/controllers/clusterextension_controller_test.go +++ b/internal/controllers/clusterextension_controller_test.go @@ -287,11 +287,11 @@ func TestClusterExtensionVersionNoChannel(t *testing.T) { require.Equal(t, metav1.ConditionFalse, cond.Status) require.Equal(t, ocv1alpha1.ReasonResolutionFailed, cond.Reason) require.Equal(t, fmt.Sprintf("no package %q matching version %q in channel %q found", pkgName, pkgVer, pkgChan), cond.Message) - cond = apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled) + cond = apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled) require.NotNil(t, cond) require.Equal(t, metav1.ConditionFalse, cond.Status) - require.Equal(t, ocv1alpha1.ReasonInstallationStatusUnknown, cond.Reason) + require.Equal(t, ocv1alpha1.ReasonResolutionFailed, cond.Reason) verifyInvariants(ctx, t, reconciler.Client, clusterExtension) require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{})) @@ -335,10 +335,11 @@ func TestClusterExtensionNoChannel(t *testing.T) { require.Equal(t, metav1.ConditionFalse, cond.Status) require.Equal(t, ocv1alpha1.ReasonResolutionFailed, cond.Reason) require.Equal(t, fmt.Sprintf("no package %q in channel %q found", pkgName, pkgChan), cond.Message) + cond = apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled) require.NotNil(t, cond) require.Equal(t, metav1.ConditionFalse, cond.Status) - require.Equal(t, ocv1alpha1.ReasonInstallationStatusUnknown, cond.Reason) + require.Equal(t, ocv1alpha1.ReasonResolutionFailed, cond.Reason) verifyInvariants(ctx, t, reconciler.Client, clusterExtension) require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{})) @@ -384,10 +385,11 @@ func TestClusterExtensionNoVersion(t *testing.T) { require.Equal(t, metav1.ConditionFalse, cond.Status) require.Equal(t, ocv1alpha1.ReasonResolutionFailed, cond.Reason) require.Equal(t, fmt.Sprintf("no package %q matching version %q in channel %q found", pkgName, pkgVer, pkgChan), cond.Message) + cond = apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled) require.NotNil(t, cond) require.Equal(t, metav1.ConditionFalse, cond.Status) - require.Equal(t, ocv1alpha1.ReasonInstallationStatusUnknown, cond.Reason) + require.Equal(t, ocv1alpha1.ReasonResolutionFailed, cond.Reason) verifyInvariants(ctx, t, reconciler.Client, clusterExtension) require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{}))