(feat): Communicate "Ready" Status of CatalogSource CR
#17
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,14 +27,17 @@ import ( | |
| batchv1 "k8s.io/api/batch/v1" | ||
| v1 "k8s.io/api/core/v1" | ||
| "k8s.io/apimachinery/pkg/api/errors" | ||
| "k8s.io/apimachinery/pkg/api/meta" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/runtime" | ||
| "k8s.io/client-go/kubernetes" | ||
| "k8s.io/client-go/rest" | ||
| ctrl "sigs.k8s.io/controller-runtime" | ||
| "sigs.k8s.io/controller-runtime/pkg/builder" | ||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| ctrlutil "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" | ||
| "sigs.k8s.io/controller-runtime/pkg/log" | ||
| "sigs.k8s.io/controller-runtime/pkg/predicate" | ||
|
|
||
| corev1beta1 "github.com/operator-framework/catalogd/pkg/apis/core/v1beta1" | ||
| ) | ||
|
|
@@ -72,11 +75,19 @@ func (r *CatalogSourceReconciler) Reconcile(ctx context.Context, req ctrl.Reques | |
| if err != nil { | ||
| if errors.IsNotFound(err) { | ||
| if err = r.createUnpackJob(ctx, catalogSource); err != nil { | ||
| updateStatusError(&catalogSource, err) | ||
| if err := r.Client.Status().Update(ctx, &catalogSource); err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("updating catalogsource status: %v", err) | ||
| } | ||
| return ctrl.Result{}, err | ||
| } | ||
| // after creating the job requeue | ||
| return ctrl.Result{Requeue: true}, nil | ||
| } | ||
| updateStatusError(&catalogSource, err) | ||
| if err := r.Client.Status().Update(ctx, &catalogSource); err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("updating catalogsource status: %v", err) | ||
| } | ||
| return ctrl.Result{}, err | ||
| } | ||
|
|
||
|
|
@@ -86,24 +97,68 @@ func (r *CatalogSourceReconciler) Reconcile(ctx context.Context, req ctrl.Reques | |
| if corev1beta1.IsUnpackPhaseError(err) { | ||
| return ctrl.Result{RequeueAfter: 10 * time.Second}, nil | ||
| } | ||
| updateStatusError(&catalogSource, err) | ||
| if err := r.Client.Status().Update(ctx, &catalogSource); err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("updating catalogsource status: %v", err) | ||
| } | ||
| return ctrl.Result{}, err | ||
| } | ||
|
|
||
| // TODO: Can we create these resources in parallel using goroutines? | ||
| if err := r.buildPackages(ctx, declCfg, catalogSource); err != nil { | ||
| updateStatusError(&catalogSource, err) | ||
| if err := r.Client.Status().Update(ctx, &catalogSource); err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("updating catalogsource status: %v", err) | ||
| } | ||
| return ctrl.Result{}, err | ||
| } | ||
|
|
||
| if err := r.buildBundleMetadata(ctx, declCfg, catalogSource); err != nil { | ||
|
everettraven marked this conversation as resolved.
|
||
| updateStatusError(&catalogSource, err) | ||
| if err := r.Client.Status().Update(ctx, &catalogSource); err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("updating catalogsource status: %v", err) | ||
| } | ||
| return ctrl.Result{}, err | ||
| } | ||
|
|
||
| // update CatalogSource status as "Ready" since at this point | ||
| // all catalog content should be available on cluster | ||
| updateStatusReady(&catalogSource) | ||
| if err := r.Client.Status().Update(ctx, &catalogSource); err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("updating catalogsource status: %v", err) | ||
| } | ||
|
Comment on lines
+127
to
+129
Member
There was a problem hiding this comment. For follow-up: we should check to see if the status changed to avoid unecessary update calls. I think this may be covered by #5 |
||
| return ctrl.Result{}, nil | ||
| } | ||
|
|
||
| func updateStatusReady(catalogSource *corev1beta1.CatalogSource) { | ||
| meta.SetStatusCondition(&catalogSource.Status.Conditions, metav1.Condition{ | ||
| Type: corev1beta1.TypeReady, | ||
| Reason: corev1beta1.ReasonContentsAvailable, | ||
| Status: metav1.ConditionTrue, | ||
| Message: "catalog contents have been unpacked and are available on cluster", | ||
| }) | ||
| } | ||
|
Comment on lines
+133
to
+140
Member
There was a problem hiding this comment. In all of the other scenarios when we return an error from But eventually we'll need to consider what happens when an old version of the catalog is available but we had a problem unpacking/syncing the new version.
Collaborator
Author
There was a problem hiding this comment. That is partially the goal of #6 but if we want it loosely implemented in this PR I am happy to update it
Member
There was a problem hiding this comment. Yeah, I'd propose we at least do the big picture invariant of "all conditions are updated somehow in every Reconcile pass" |
||
|
|
||
| func updateStatusError(catalogSource *corev1beta1.CatalogSource, err error) { | ||
| meta.SetStatusCondition(&catalogSource.Status.Conditions, metav1.Condition{ | ||
| Type: corev1beta1.TypeReady, | ||
| Status: metav1.ConditionFalse, | ||
| Reason: corev1beta1.ReasonUnpackError, | ||
| Message: err.Error(), | ||
| }) | ||
| } | ||
|
|
||
| // SetupWithManager sets up the controller with the Manager. | ||
| func (r *CatalogSourceReconciler) SetupWithManager(mgr ctrl.Manager) error { | ||
| return ctrl.NewControllerManagedBy(mgr). | ||
| // TODO: Due to us not having proper error handling, | ||
| // not having this results in the controller getting into | ||
| // an error state because once we update the status it requeues | ||
| // and then errors out when trying to create all the Packages again | ||
| // even though they already exist. This should be resolved by the fix | ||
| // for https://github.com/operator-framework/catalogd/issues/6. The fix for | ||
| // #6 should also remove the usage of `builder.WithPredicates(predicate.GenerationChangedPredicate{})` | ||
| For(&corev1beta1.CatalogSource{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})). | ||
|
Member
There was a problem hiding this comment. Don't do this. If someone/something changes the status out from under us, we should go back and re-reconcile.
Collaborator
Author
There was a problem hiding this comment. Currently since we don't have good error handling, not having this results in the controller getting into an error state because once we update the status it requeues and then errors out when trying to create all the Would you be okay with this going in for now and fixing it along with the changes needed for #6 ?
Member
There was a problem hiding this comment. Sure, can you add a TODO comment, summarizing that in the code?
Member
There was a problem hiding this comment. And maybe it makes sense to have a separate issue about just that particular problem? |
||
| Complete(r) | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do in a follow up too. But having all the conditions and reasons added to an array eventually would help in testing condition invariants as in here in future.