Skip to content

Commit

Permalink
Update error handling in reconciler
Browse files Browse the repository at this point in the history 
Signed-off-by: Todd Short <tshort@redhat.com>
  • Loading branch information
@tmshort
tmshort committed May 30, 2024
1 parent 0058054 commit 8d6ae96
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 88 deletions.
118 changes: 34 additions & 84 deletions internal/controllers/clusterextension_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -115,40 +124,35 @@ 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)
updateFinalizers := !equality.Semantic.DeepEqual(existingExt.Finalizers, reconciledExt.Finalizers)
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 {
panic("spec or metadata changed by reconciler")
}

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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
Expand All @@ -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))
}
Expand All @@ -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)}
}
}

Expand Down Expand Up @@ -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
Expand Down
10 changes: 6 additions & 4 deletions internal/controllers/clusterextension_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{}))
Expand Down Expand Up @@ -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{}))
Expand Down Expand Up @@ -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{}))
Expand Down

0 comments on commit 8d6ae96

Please sign in to comment.