Skip to content

Commit 9f32ace

Browse files
tmshorttmshort
committed
Update error handling in reconciler
Separate out resolution errors from generic errors. Signed-off-by: Todd Short <[email protected]>
1 parent 9f0c6a9 commit 9f32ace

File tree

3 files changed

+61
-110
lines changed

3 files changed

+61
-110
lines changed

‎internal/controllers/clusterextension_controller.go

Lines changed: 42 additions & 105 deletions
Original file line numberDiff line numberDiff line change
@@ -38,14 +38,12 @@ import (
3838
"helm.sh/helm/v3/pkg/storage/driver"
3939
corev1 "k8s.io/api/core/v1"
4040
"k8s.io/apimachinery/pkg/api/equality"
41-
apierrors "k8s.io/apimachinery/pkg/api/errors"
4241
apimeta "k8s.io/apimachinery/pkg/api/meta"
4342
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
4443
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
4544
"k8s.io/apimachinery/pkg/runtime"
4645
"k8s.io/apimachinery/pkg/runtime/schema"
4746
"k8s.io/apimachinery/pkg/types"
48-
utilerrors "k8s.io/apimachinery/pkg/util/errors"
4947
"k8s.io/apimachinery/pkg/util/sets"
5048
apimachyaml "k8s.io/apimachinery/pkg/util/yaml"
5149
ctrl "sigs.k8s.io/controller-runtime"
@@ -80,6 +78,17 @@ import (
8078
"github.com/operator-framework/operator-controller/internal/labels"
8179
)
8280

81+
// ResolutionError type
82+
// If a more specific error type needs to be distinguished,
83+
// add another type here
84+
type ResolutionError struct {
85+
message string
86+
}
87+
88+
func (e ResolutionError) Error() string {
89+
return e.message
90+
}
91+
8392
// ClusterExtensionReconciler reconciles a ClusterExtension object
8493
type ClusterExtensionReconciler struct {
8594
client.Client
@@ -120,40 +129,35 @@ func (r *ClusterExtensionReconciler) Reconcile(ctx context.Context, req ctrl.Req
120129

121130
var existingExt = &ocv1alpha1.ClusterExtension{}
122131
if err := r.Client.Get(ctx, req.NamespacedName, existingExt); err != nil {
123-
return ctrl.Result{}, utilerrors.NewAggregate([]error{client.IgnoreNotFound(err), nil})
132+
return ctrl.Result{}, client.IgnoreNotFound(err)
124133
}
125134

126-
reconciledExt := existingExt.DeepCopy()
127-
res, reconcileErr := r.reconcile(ctx, reconciledExt)
135+
var updateError error
128136

129-
var updateErrors []error
137+
reconciledExt := existingExt.DeepCopy()
138+
res, err := r.reconcile(ctx, reconciledExt)
139+
updateError = errors.Join(updateError, err)
130140

131141
// Do checks before any Update()s, as Update() may modify the resource structure!
132142
updateStatus := !equality.Semantic.DeepEqual(existingExt.Status, reconciledExt.Status)
133143
updateFinalizers := !equality.Semantic.DeepEqual(existingExt.Finalizers, reconciledExt.Finalizers)
134144
unexpectedFieldsChanged := checkForUnexpectedFieldChange(*existingExt, *reconciledExt)
135145

136146
if updateStatus {
137-
if updateErr := r.Client.Status().Update(ctx, reconciledExt); updateErr != nil {
138-
updateErrors = append(updateErrors, updateErr)
139-
}
147+
err = r.Client.Status().Update(ctx, reconciledExt)
148+
updateError = errors.Join(updateError, err)
140149
}
141150

142151
if unexpectedFieldsChanged {
143152
panic("spec or metadata changed by reconciler")
144153
}
145154

146155
if updateFinalizers {
147-
if updateErr := r.Client.Update(ctx, reconciledExt); updateErr != nil {
148-
updateErrors = append(updateErrors, updateErr)
149-
}
156+
err = r.Client.Update(ctx, reconciledExt)
157+
updateError = errors.Join(updateError, err)
150158
}
151159

152-
if reconcileErr != nil {
153-
updateErrors = append(updateErrors, reconcileErr)
154-
}
155-
156-
return res, utilerrors.NewAggregate(updateErrors)
160+
return res, updateError
157161
}
158162

159163
// ensureAllConditionsWithReason checks that all defined condition types exist in the given ClusterExtension,
@@ -183,35 +187,6 @@ func checkForUnexpectedFieldChange(a, b ocv1alpha1.ClusterExtension) bool {
183187
return !equality.Semantic.DeepEqual(a, b)
184188
}
185189

186-
func (r *ClusterExtensionReconciler) handleResolutionErrors(ext *ocv1alpha1.ClusterExtension, err error) (ctrl.Result, error) {
187-
var aggErrs utilerrors.Aggregate
188-
if errors.As(err, &aggErrs) {
189-
for _, err := range aggErrs.Errors() {
190-
errorMessage := err.Error()
191-
if strings.Contains(errorMessage, "no package") {
192-
// Handle no package found errors, potentially setting status conditions
193-
setResolvedStatusConditionFailed(&ext.Status.Conditions, errorMessage, ext.Generation)
194-
ensureAllConditionsWithReason(ext, "ResolutionFailed", errorMessage)
195-
} else if strings.Contains(errorMessage, "invalid version range") {
196-
// Handle invalid version range errors, potentially setting status conditions
197-
setResolvedStatusConditionFailed(&ext.Status.Conditions, errorMessage, ext.Generation)
198-
ensureAllConditionsWithReason(ext, "ResolutionFailed", errorMessage)
199-
} else {
200-
// General error handling
201-
setResolvedStatusConditionFailed(&ext.Status.Conditions, errorMessage, ext.Generation)
202-
ensureAllConditionsWithReason(ext, "InstallationStatusUnknown", "")
203-
}
204-
}
205-
} else {
206-
// If the error is not an aggregate, handle it as a general error
207-
errorMessage := err.Error()
208-
setResolvedStatusConditionFailed(&ext.Status.Conditions, errorMessage, ext.Generation)
209-
ensureAllConditionsWithReason(ext, "InstallationStatusUnknown", "")
210-
}
211-
ext.Status.ResolvedBundle = nil
212-
return ctrl.Result{}, err
213-
}
214-
215190
// Helper function to do the actual reconcile
216191
//
217192
// Today we always return ctrl.Result{} and an error.
@@ -232,10 +207,22 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
232207
// run resolution
233208
bundle, err := r.resolve(ctx, *ext)
234209
if err != nil {
235-
return r.handleResolutionErrors(ext, err)
210+
ext.Status.ResolvedBundle = nil
211+
ext.Status.InstalledBundle = nil
212+
setResolvedStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration())
213+
// TODO: indicate Progressing state based on whether error is ResolutionError or not
214+
if errors.As(err, &ResolutionError{}) {
215+
ensureAllConditionsWithReason(ext, ocv1alpha1.ReasonResolutionFailed, err.Error())
216+
} else {
217+
ensureAllConditionsWithReason(ext, ocv1alpha1.ReasonInstallationStatusUnknown, err.Error())
218+
}
219+
return ctrl.Result{}, err
236220
}
237221

238222
if err := r.validateBundle(bundle); err != nil {
223+
ext.Status.ResolvedBundle = nil
224+
ext.Status.InstalledBundle = nil
225+
setResolvedStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration())
239226
setInstalledStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration())
240227
setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as installation has failed", ext.GetGeneration())
241228
return ctrl.Result{}, err
@@ -291,23 +278,13 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
291278

292279
bundleFS, err := r.Storage.Load(ctx, ext)
293280
if err != nil {
294-
apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{
295-
Type: ocv1alpha1.TypeHasValidBundle,
296-
Status: metav1.ConditionFalse,
297-
Reason: ocv1alpha1.ReasonBundleLoadFailed,
298-
Message: err.Error(),
299-
})
281+
setHasValidBundleFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration())
300282
return ctrl.Result{}, err
301283
}
302284

303285
chrt, values, err := r.Handler.Handle(ctx, bundleFS, ext)
304286
if err != nil {
305-
apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{
306-
Type: ocv1alpha1.TypeInstalled,
307-
Status: metav1.ConditionFalse,
308-
Reason: ocv1alpha1.ReasonInstallationFailed,
309-
Message: err.Error(),
310-
})
287+
setInstalledStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration())
311288
return ctrl.Result{}, err
312289
}
313290

@@ -342,26 +319,17 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
342319
return nil
343320
}, helmclient.AppendInstallPostRenderer(post))
344321
if err != nil {
345-
if isResourceNotFoundErr(err) {
346-
err = errRequiredResourceNotFound{err}
347-
}
348322
setInstalledStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonInstallationFailed, err), ext.Generation)
349323
return ctrl.Result{}, err
350324
}
351325
case stateNeedsUpgrade:
352326
rel, err = ac.Upgrade(ext.GetName(), r.ReleaseNamespace, chrt, values, helmclient.AppendUpgradePostRenderer(post))
353327
if err != nil {
354-
if isResourceNotFoundErr(err) {
355-
err = errRequiredResourceNotFound{err}
356-
}
357328
setInstalledStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonUpgradeFailed, err), ext.Generation)
358329
return ctrl.Result{}, err
359330
}
360331
case stateUnchanged:
361332
if err := ac.Reconcile(rel); err != nil {
362-
if isResourceNotFoundErr(err) {
363-
err = errRequiredResourceNotFound{err}
364-
}
365333
setInstalledStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonResolutionFailed, err), ext.Generation)
366334
return ctrl.Result{}, err
367335
}
@@ -414,7 +382,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
414382
func (r *ClusterExtensionReconciler) resolve(ctx context.Context, ext ocv1alpha1.ClusterExtension) (*catalogmetadata.Bundle, error) {
415383
allBundles, err := r.BundleProvider.Bundles(ctx)
416384
if err != nil {
417-
return nil, utilerrors.NewAggregate([]error{fmt.Errorf("error fetching bundles: %w", err)})
385+
return nil, fmt.Errorf("error fetching bundles: %w", err)
418386
}
419387

420388
packageName := ext.Spec.PackageName
@@ -437,7 +405,7 @@ func (r *ClusterExtensionReconciler) resolve(ctx context.Context, ext ocv1alpha1
437405
if versionRange != "" {
438406
vr, err := mmsemver.NewConstraint(versionRange)
439407
if err != nil {
440-
return nil, utilerrors.NewAggregate([]error{fmt.Errorf("invalid version range '%s': %w", versionRange, err)})
408+
return nil, ResolutionError{fmt.Sprintf("invalid version range %q: %v", versionRange, err)}
441409
}
442410
predicates = append(predicates, catalogfilter.InMastermindsSemverRange(vr))
443411
}
@@ -464,13 +432,13 @@ func (r *ClusterExtensionReconciler) resolve(ctx context.Context, ext ocv1alpha1
464432
if len(resultSet) == 0 {
465433
switch {
466434
case versionRange != "" && channelName != "":
467-
return nil, fmt.Errorf("%sno package %q matching version %q in channel %q found", upgradeErrorPrefix, packageName, versionRange, channelName)
435+
return nil, ResolutionError{fmt.Sprintf("%sno package %q matching version %q in channel %q found", upgradeErrorPrefix, packageName, versionRange, channelName)}
468436
case versionRange != "":
469-
return nil, fmt.Errorf("%sno package %q matching version %q found", upgradeErrorPrefix, packageName, versionRange)
437+
return nil, ResolutionError{fmt.Sprintf("%sno package %q matching version %q found", upgradeErrorPrefix, packageName, versionRange)}
470438
case channelName != "":
471-
return nil, fmt.Errorf("%sno package %q in channel %q found", upgradeErrorPrefix, packageName, channelName)
439+
return nil, ResolutionError{fmt.Sprintf("%sno package %q in channel %q found", upgradeErrorPrefix, packageName, channelName)}
472440
default:
473-
return nil, fmt.Errorf("%sno package %q found", upgradeErrorPrefix, packageName)
441+
return nil, ResolutionError{fmt.Sprintf("%sno package %q found", upgradeErrorPrefix, packageName)}
474442
}
475443
}
476444

@@ -753,37 +721,6 @@ func (d *DefaultInstalledBundleGetter) GetInstalledBundle(ctx context.Context, a
753721
return resultSet[0], nil
754722
}
755723

756-
type errRequiredResourceNotFound struct {
757-
error
758-
}
759-
760-
func (err errRequiredResourceNotFound) Error() string {
761-
return fmt.Sprintf("required resource not found: %v", err.error)
762-
}
763-
764-
func isResourceNotFoundErr(err error) bool {
765-
var agg utilerrors.Aggregate
766-
if errors.As(err, &agg) {
767-
for _, err := range agg.Errors() {
768-
return isResourceNotFoundErr(err)
769-
}
770-
}
771-
772-
nkme := &apimeta.NoKindMatchError{}
773-
if errors.As(err, &nkme) {
774-
return true
775-
}
776-
if apierrors.IsNotFound(err) {
777-
return true
778-
}
779-
780-
// TODO: improve NoKindMatchError matching
781-
// An error that is bubbled up from the k8s.io/cli-runtime library
782-
// does not wrap meta.NoKindMatchError, so we need to fallback to
783-
// the use of string comparisons for now.
784-
return strings.Contains(err.Error(), "no matches for kind")
785-
}
786-
787724
type postrenderer struct {
788725
labels map[string]string
789726
cascade postrender.PostRenderer

‎internal/controllers/clusterextension_controller_test.go

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,7 @@ func TestClusterExtensionNonExistentVersion(t *testing.T) {
120120
require.Equal(t, metav1.ConditionFalse, cond.Status)
121121
require.Equal(t, ocv1alpha1.ReasonResolutionFailed, cond.Reason)
122122
require.Equal(t, fmt.Sprintf(`no package %q matching version "0.50.0" found`, pkgName), cond.Message)
123+
123124
cond = apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled)
124125
require.NotNil(t, cond)
125126
require.Equal(t, metav1.ConditionFalse, cond.Status)
@@ -286,11 +287,11 @@ func TestClusterExtensionVersionNoChannel(t *testing.T) {
286287
require.Equal(t, metav1.ConditionFalse, cond.Status)
287288
require.Equal(t, ocv1alpha1.ReasonResolutionFailed, cond.Reason)
288289
require.Equal(t, fmt.Sprintf("no package %q matching version %q in channel %q found", pkgName, pkgVer, pkgChan), cond.Message)
289-
cond = apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled)
290290

291+
cond = apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled)
291292
require.NotNil(t, cond)
292293
require.Equal(t, metav1.ConditionFalse, cond.Status)
293-
require.Equal(t, ocv1alpha1.ReasonInstallationStatusUnknown, cond.Reason)
294+
require.Equal(t, ocv1alpha1.ReasonResolutionFailed, cond.Reason)
294295

295296
verifyInvariants(ctx, t, reconciler.Client, clusterExtension)
296297
require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{}))
@@ -334,10 +335,11 @@ func TestClusterExtensionNoChannel(t *testing.T) {
334335
require.Equal(t, metav1.ConditionFalse, cond.Status)
335336
require.Equal(t, ocv1alpha1.ReasonResolutionFailed, cond.Reason)
336337
require.Equal(t, fmt.Sprintf("no package %q in channel %q found", pkgName, pkgChan), cond.Message)
338+
337339
cond = apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled)
338340
require.NotNil(t, cond)
339341
require.Equal(t, metav1.ConditionFalse, cond.Status)
340-
require.Equal(t, ocv1alpha1.ReasonInstallationStatusUnknown, cond.Reason)
342+
require.Equal(t, ocv1alpha1.ReasonResolutionFailed, cond.Reason)
341343

342344
verifyInvariants(ctx, t, reconciler.Client, clusterExtension)
343345
require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{}))
@@ -383,10 +385,11 @@ func TestClusterExtensionNoVersion(t *testing.T) {
383385
require.Equal(t, metav1.ConditionFalse, cond.Status)
384386
require.Equal(t, ocv1alpha1.ReasonResolutionFailed, cond.Reason)
385387
require.Equal(t, fmt.Sprintf("no package %q matching version %q in channel %q found", pkgName, pkgVer, pkgChan), cond.Message)
388+
386389
cond = apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled)
387390
require.NotNil(t, cond)
388391
require.Equal(t, metav1.ConditionFalse, cond.Status)
389-
require.Equal(t, ocv1alpha1.ReasonInstallationStatusUnknown, cond.Reason)
392+
require.Equal(t, ocv1alpha1.ReasonResolutionFailed, cond.Reason)
390393

391394
verifyInvariants(ctx, t, reconciler.Client, clusterExtension)
392395
require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{}))

‎internal/controllers/common_controller.go

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ func setInstalledStatusConditionUnknown(conditions *[]metav1.Condition, message
5757
})
5858
}
5959

60-
// setHasValidBundleUnknown sets the installed status condition to unknown.
60+
// setHasValidBundleUnknown sets the valid bundle condition to unknown.
6161
func setHasValidBundleUnknown(conditions *[]metav1.Condition, message string, generation int64) {
6262
apimeta.SetStatusCondition(conditions, metav1.Condition{
6363
Type: ocv1alpha1.TypeHasValidBundle,
@@ -68,6 +68,17 @@ func setHasValidBundleUnknown(conditions *[]metav1.Condition, message string, ge
6868
})
6969
}
7070

71+
// setHasValidBundleFalse sets the ivalid bundle condition to false
72+
func setHasValidBundleFailed(conditions *[]metav1.Condition, message string, generation int64) {
73+
apimeta.SetStatusCondition(conditions, metav1.Condition{
74+
Type: ocv1alpha1.TypeHasValidBundle,
75+
Status: metav1.ConditionFalse,
76+
Reason: ocv1alpha1.ReasonBundleLoadFailed,
77+
Message: message,
78+
ObservedGeneration: generation,
79+
})
80+
}
81+
7182
// setResolvedStatusConditionFailed sets the resolved status condition to failed.
7283
func setResolvedStatusConditionFailed(conditions *[]metav1.Condition, message string, generation int64) {
7384
apimeta.SetStatusCondition(conditions, metav1.Condition{

0 commit comments

Comments
 (0)