From c0a1193218c66b20b471fd5d7b98a267acff57d8 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Mon, 3 Jun 2024 14:36:25 -0400 Subject: [PATCH] Update status update functions Remove references to rukpak k8s API where possible. With the removal of Extension, the status update functions no longer need to be generic. Simplify the function signatures Signed-off-by: Todd Short --- api/v1alpha1/clusterextension_types.go | 14 ++- .../clusterextension_controller.go | 59 +++++----- internal/controllers/common_controller.go | 109 +++++++++--------- test/e2e/cluster_extension_install_test.go | 5 +- 4 files changed, 100 insertions(+), 87 deletions(-) diff --git a/api/v1alpha1/clusterextension_types.go b/api/v1alpha1/clusterextension_types.go index c69803614..557c0570d 100644 --- a/api/v1alpha1/clusterextension_types.go +++ b/api/v1alpha1/clusterextension_types.go @@ -106,7 +106,14 @@ const ( ReasonDeprecated = "Deprecated" ReasonUpgradeFailed = "UpgradeFailed" ReasonHasValidBundleUnknown = "HasValidBundleUnknown" - ReasonUnpackPending = "UnpackPending" + + ReasonUnpackPending = "UnpackPending" + ReasonUnpackSuccess = "UnpackSuccess" + ReasonUnpackFailed = "UnpackFailed" + ReasonUnpacking = "Unpacking" + + ReasonErrorGettingReleaseState = "ErrorGettingReleaseState" + ReasonCreateDynamicWatchFailed = "CreateDynamicWatchFailed" ) func init() { @@ -134,6 +141,11 @@ func init() { ReasonInstallationStatusUnknown, ReasonHasValidBundleUnknown, ReasonUnpackPending, + ReasonUnpackSuccess, + ReasonUnpacking, + ReasonUnpackFailed, + ReasonErrorGettingReleaseState, + ReasonCreateDynamicWatchFailed, ) } diff --git a/internal/controllers/clusterextension_controller.go b/internal/controllers/clusterextension_controller.go index ae75bcc59..7adb6f151 100644 --- a/internal/controllers/clusterextension_controller.go +++ b/internal/controllers/clusterextension_controller.go @@ -199,7 +199,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp // Note: We don't distinguish between resolution-specific errors and generic errors ext.Status.ResolvedBundle = nil ext.Status.InstalledBundle = nil - setResolvedStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration()) + setResolvedStatusConditionFailed(ext, err.Error()) ensureAllConditionsWithReason(ext, ocv1alpha1.ReasonResolutionFailed, err.Error()) return ctrl.Result{}, err } @@ -207,9 +207,9 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp if err := r.validateBundle(bundle); err != nil { ext.Status.ResolvedBundle = nil ext.Status.InstalledBundle = nil - setResolvedStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration()) - setInstalledStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration()) - setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as installation has failed", ext.GetGeneration()) + setResolvedStatusConditionFailed(ext, err.Error()) + setInstalledStatusConditionFailed(ext, err.Error()) + setDeprecationStatusesUnknown(ext, "deprecation checks have not been attempted as installation has failed") return ctrl.Result{}, err } // set deprecation status after _successful_ resolution @@ -219,13 +219,13 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp if err != nil { ext.Status.ResolvedBundle = nil ext.Status.InstalledBundle = nil - setResolvedStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration()) - setInstalledStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.Generation) + setResolvedStatusConditionFailed(ext, err.Error()) + setInstalledStatusConditionFailed(ext, err.Error()) return ctrl.Result{}, err } ext.Status.ResolvedBundle = bundleMetadataFor(bundle) - setResolvedStatusConditionSuccess(&ext.Status.Conditions, fmt.Sprintf("resolved to %q", bundle.Image), ext.GetGeneration()) + setResolvedStatusConditionSuccess(ext, fmt.Sprintf("resolved to %q", bundle.Image)) // Generate a BundleDeployment from the ClusterExtension to Unpack. // Note: The BundleDeployment here is not a k8s API, its a simple Go struct which @@ -233,50 +233,53 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp bd := r.generateBundleDeploymentForUnpack(bundle.Image, ext) unpackResult, err := r.Unpacker.Unpack(ctx, bd) if err != nil { - return ctrl.Result{}, updateStatusUnpackFailing(&ext.Status, err) + setStatusUnpackFailed(ext, err.Error()) + return ctrl.Result{}, err } switch unpackResult.State { case rukpaksource.StatePending: - updateStatusUnpackPending(&ext.Status, unpackResult, ext.GetGeneration()) + setStatusUnpackPending(ext, unpackResult.Message) // There must be a limit to number of entries if status is stuck at // unpack pending. - setHasValidBundleUnknown(&ext.Status.Conditions, "unpack pending", ext.GetGeneration()) - setInstalledStatusConditionUnknown(&ext.Status.Conditions, "installation has not been attempted as unpack is pending", ext.GetGeneration()) + setHasValidBundleUnknown(ext, "unpack pending") + setInstalledStatusConditionUnknown(ext, "installation has not been attempted as unpack is pending") return ctrl.Result{}, nil case rukpaksource.StateUnpacking: - updateStatusUnpacking(&ext.Status, unpackResult) - setHasValidBundleUnknown(&ext.Status.Conditions, "unpack pending", ext.GetGeneration()) - setInstalledStatusConditionUnknown(&ext.Status.Conditions, "installation has not been attempted as unpack is pending", ext.GetGeneration()) + setStatusUnpacking(ext, unpackResult.Message) + setHasValidBundleUnknown(ext, "unpack pending") + setInstalledStatusConditionUnknown(ext, "installation has not been attempted as unpack is pending") return ctrl.Result{}, nil case rukpaksource.StateUnpacked: // TODO: Add finalizer to clean the stored bundles, after https://github.com/operator-framework/rukpak/pull/897 // merges. if err := r.Storage.Store(ctx, ext, unpackResult.Bundle); err != nil { - return ctrl.Result{}, updateStatusUnpackFailing(&ext.Status, err) + setStatusUnpackFailed(ext, err.Error()) + return ctrl.Result{}, err } - updateStatusUnpacked(&ext.Status, unpackResult) + setStatusUnpacked(ext, unpackResult.Message) default: - return ctrl.Result{}, updateStatusUnpackFailing(&ext.Status, err) + setStatusUnpackFailed(ext, err.Error()) + return ctrl.Result{}, err } bundleFS, err := r.Storage.Load(ctx, ext) if err != nil { - setHasValidBundleFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration()) + setHasValidBundleFailed(ext, err.Error()) return ctrl.Result{}, err } chrt, values, err := r.Handler.Handle(ctx, bundleFS, ext) if err != nil { - setInstalledStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration()) + setInstalledStatusConditionFailed(ext, err.Error()) return ctrl.Result{}, err } ac, err := r.ActionClientGetter.ActionClientFor(ctx, ext) if err != nil { ext.Status.InstalledBundle = nil - setInstalledStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonErrorGettingClient, err), ext.Generation) + setInstalledStatusConditionFailed(ext, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonErrorGettingClient, err)) return ctrl.Result{}, err } @@ -292,7 +295,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp rel, state, err := r.getReleaseState(ac, ext, chrt, values, post) if err != nil { - setInstalledStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("%s:%v", rukpakv1alpha2.ReasonErrorGettingReleaseState, err), ext.Generation) + setInstalledStatusConditionFailed(ext, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonErrorGettingReleaseState, err)) return ctrl.Result{}, err } @@ -304,18 +307,18 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp return nil }, helmclient.AppendInstallPostRenderer(post)) if err != nil { - setInstalledStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonInstallationFailed, err), ext.Generation) + setInstalledStatusConditionFailed(ext, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonInstallationFailed, err)) return ctrl.Result{}, err } case stateNeedsUpgrade: rel, err = ac.Upgrade(ext.GetName(), r.ReleaseNamespace, chrt, values, helmclient.AppendUpgradePostRenderer(post)) if err != nil { - setInstalledStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonUpgradeFailed, err), ext.Generation) + setInstalledStatusConditionFailed(ext, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonUpgradeFailed, err)) return ctrl.Result{}, err } case stateUnchanged: if err := ac.Reconcile(rel); err != nil { - setInstalledStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonResolutionFailed, err), ext.Generation) + setInstalledStatusConditionFailed(ext, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonResolutionFailed, err)) return ctrl.Result{}, err } default: @@ -324,14 +327,14 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp relObjects, err := util.ManifestObjects(strings.NewReader(rel.Manifest), fmt.Sprintf("%s-release-manifest", rel.Name)) if err != nil { - setInstalledStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("%s:%v", rukpakv1alpha2.ReasonCreateDynamicWatchFailed, err), ext.Generation) + setInstalledStatusConditionFailed(ext, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonCreateDynamicWatchFailed, err)) return ctrl.Result{}, err } for _, obj := range relObjects { uMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj) if err != nil { - setInstalledStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("%s:%v", rukpakv1alpha2.ReasonCreateDynamicWatchFailed, err), ext.Generation) + setInstalledStatusConditionFailed(ext, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonCreateDynamicWatchFailed, err)) return ctrl.Result{}, err } @@ -353,12 +356,12 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp return nil }(); err != nil { ext.Status.InstalledBundle = nil - setInstalledStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("%s:%v", rukpakv1alpha2.ReasonCreateDynamicWatchFailed, err), ext.Generation) + setInstalledStatusConditionFailed(ext, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonCreateDynamicWatchFailed, err)) return ctrl.Result{}, err } } ext.Status.InstalledBundle = bundleMetadataFor(bundle) - setInstalledStatusConditionSuccess(&ext.Status.Conditions, fmt.Sprintf("Instantiated bundle %s successfully", ext.GetName()), ext.Generation) + setInstalledStatusConditionSuccess(ext, fmt.Sprintf("Instantiated bundle %s successfully", ext.GetName())) return ctrl.Result{}, nil } diff --git a/internal/controllers/common_controller.go b/internal/controllers/common_controller.go index 047f1176b..ab99574da 100644 --- a/internal/controllers/common_controller.go +++ b/internal/controllers/common_controller.go @@ -22,9 +22,6 @@ import ( apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - rukpakv1alpha2 "github.com/operator-framework/rukpak/api/v1alpha2" - "github.com/operator-framework/rukpak/pkg/source" - ocv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1" "github.com/operator-framework/operator-controller/internal/catalogmetadata" ) @@ -36,84 +33,84 @@ type BundleProvider interface { } // setResolvedStatusConditionSuccess sets the resolved status condition to success. -func setResolvedStatusConditionSuccess(conditions *[]metav1.Condition, message string, generation int64) { - apimeta.SetStatusCondition(conditions, metav1.Condition{ +func setResolvedStatusConditionSuccess(ext *ocv1alpha1.ClusterExtension, message string) { + apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ Type: ocv1alpha1.TypeResolved, Status: metav1.ConditionTrue, Reason: ocv1alpha1.ReasonSuccess, Message: message, - ObservedGeneration: generation, + ObservedGeneration: ext.GetGeneration(), }) } // setInstalledStatusConditionUnknown sets the installed status condition to unknown. -func setInstalledStatusConditionUnknown(conditions *[]metav1.Condition, message string, generation int64) { - apimeta.SetStatusCondition(conditions, metav1.Condition{ +func setInstalledStatusConditionUnknown(ext *ocv1alpha1.ClusterExtension, message string) { + apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ Type: ocv1alpha1.TypeInstalled, Status: metav1.ConditionUnknown, Reason: ocv1alpha1.ReasonInstallationStatusUnknown, Message: message, - ObservedGeneration: generation, + ObservedGeneration: ext.GetGeneration(), }) } // setHasValidBundleUnknown sets the valid bundle condition to unknown. -func setHasValidBundleUnknown(conditions *[]metav1.Condition, message string, generation int64) { - apimeta.SetStatusCondition(conditions, metav1.Condition{ +func setHasValidBundleUnknown(ext *ocv1alpha1.ClusterExtension, message string) { + apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ Type: ocv1alpha1.TypeHasValidBundle, Status: metav1.ConditionUnknown, Reason: ocv1alpha1.ReasonHasValidBundleUnknown, Message: message, - ObservedGeneration: generation, + ObservedGeneration: ext.GetGeneration(), }) } // setHasValidBundleFalse sets the ivalid bundle condition to false -func setHasValidBundleFailed(conditions *[]metav1.Condition, message string, generation int64) { - apimeta.SetStatusCondition(conditions, metav1.Condition{ +func setHasValidBundleFailed(ext *ocv1alpha1.ClusterExtension, message string) { + apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ Type: ocv1alpha1.TypeHasValidBundle, Status: metav1.ConditionFalse, Reason: ocv1alpha1.ReasonBundleLoadFailed, Message: message, - ObservedGeneration: generation, + ObservedGeneration: ext.GetGeneration(), }) } // setResolvedStatusConditionFailed sets the resolved status condition to failed. -func setResolvedStatusConditionFailed(conditions *[]metav1.Condition, message string, generation int64) { - apimeta.SetStatusCondition(conditions, metav1.Condition{ +func setResolvedStatusConditionFailed(ext *ocv1alpha1.ClusterExtension, message string) { + apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ Type: ocv1alpha1.TypeResolved, Status: metav1.ConditionFalse, Reason: ocv1alpha1.ReasonResolutionFailed, Message: message, - ObservedGeneration: generation, + ObservedGeneration: ext.GetGeneration(), }) } // setInstalledStatusConditionSuccess sets the installed status condition to success. -func setInstalledStatusConditionSuccess(conditions *[]metav1.Condition, message string, generation int64) { - apimeta.SetStatusCondition(conditions, metav1.Condition{ +func setInstalledStatusConditionSuccess(ext *ocv1alpha1.ClusterExtension, message string) { + apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ Type: ocv1alpha1.TypeInstalled, Status: metav1.ConditionTrue, Reason: ocv1alpha1.ReasonSuccess, Message: message, - ObservedGeneration: generation, + ObservedGeneration: ext.GetGeneration(), }) } // setInstalledStatusConditionFailed sets the installed status condition to failed. -func setInstalledStatusConditionFailed(conditions *[]metav1.Condition, message string, generation int64) { - apimeta.SetStatusCondition(conditions, metav1.Condition{ +func setInstalledStatusConditionFailed(ext *ocv1alpha1.ClusterExtension, message string) { + apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ Type: ocv1alpha1.TypeInstalled, Status: metav1.ConditionFalse, Reason: ocv1alpha1.ReasonInstallationFailed, Message: message, - ObservedGeneration: generation, + ObservedGeneration: ext.GetGeneration(), }) } // setDeprecationStatusesUnknown sets the deprecation status conditions to unknown. -func setDeprecationStatusesUnknown(conditions *[]metav1.Condition, message string, generation int64) { +func setDeprecationStatusesUnknown(ext *ocv1alpha1.ClusterExtension, message string) { conditionTypes := []string{ ocv1alpha1.TypeDeprecated, ocv1alpha1.TypePackageDeprecated, @@ -122,55 +119,57 @@ func setDeprecationStatusesUnknown(conditions *[]metav1.Condition, message strin } for _, conditionType := range conditionTypes { - apimeta.SetStatusCondition(conditions, metav1.Condition{ + apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ Type: conditionType, Reason: ocv1alpha1.ReasonDeprecated, Status: metav1.ConditionUnknown, Message: message, - ObservedGeneration: generation, + ObservedGeneration: ext.GetGeneration(), }) } } -func updateStatusUnpackFailing(status *ocv1alpha1.ClusterExtensionStatus, err error) error { - status.InstalledBundle = nil - apimeta.SetStatusCondition(&status.Conditions, metav1.Condition{ - Type: rukpakv1alpha2.TypeUnpacked, - Status: metav1.ConditionFalse, - Reason: rukpakv1alpha2.ReasonUnpackFailed, - Message: err.Error(), +func setStatusUnpackFailed(ext *ocv1alpha1.ClusterExtension, message string) { + ext.Status.InstalledBundle = nil + apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ + Type: ocv1alpha1.TypeUnpacked, + Status: metav1.ConditionFalse, + Reason: ocv1alpha1.ReasonUnpackFailed, + Message: message, + ObservedGeneration: ext.GetGeneration(), }) - return err } // TODO: verify if we need to update the installBundle status or leave it as is. -func updateStatusUnpackPending(status *ocv1alpha1.ClusterExtensionStatus, result *source.Result, generation int64) { - status.InstalledBundle = nil - apimeta.SetStatusCondition(&status.Conditions, metav1.Condition{ - Type: rukpakv1alpha2.TypeUnpacked, +func setStatusUnpackPending(ext *ocv1alpha1.ClusterExtension, message string) { + ext.Status.InstalledBundle = nil + apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ + Type: ocv1alpha1.TypeUnpacked, Status: metav1.ConditionFalse, - Reason: rukpakv1alpha2.ReasonUnpackPending, - Message: result.Message, - ObservedGeneration: generation, + Reason: ocv1alpha1.ReasonUnpackPending, + Message: message, + ObservedGeneration: ext.GetGeneration(), }) } // TODO: verify if we need to update the installBundle status or leave it as is. -func updateStatusUnpacking(status *ocv1alpha1.ClusterExtensionStatus, result *source.Result) { - status.InstalledBundle = nil - apimeta.SetStatusCondition(&status.Conditions, metav1.Condition{ - Type: rukpakv1alpha2.TypeUnpacked, - Status: metav1.ConditionFalse, - Reason: rukpakv1alpha2.ReasonUnpacking, - Message: result.Message, +func setStatusUnpacking(ext *ocv1alpha1.ClusterExtension, message string) { + ext.Status.InstalledBundle = nil + apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ + Type: ocv1alpha1.TypeUnpacked, + Status: metav1.ConditionFalse, + Reason: ocv1alpha1.ReasonUnpacking, + Message: message, + ObservedGeneration: ext.GetGeneration(), }) } -func updateStatusUnpacked(status *ocv1alpha1.ClusterExtensionStatus, result *source.Result) { - apimeta.SetStatusCondition(&status.Conditions, metav1.Condition{ - Type: rukpakv1alpha2.TypeUnpacked, - Status: metav1.ConditionTrue, - Reason: rukpakv1alpha2.ReasonUnpackSuccessful, - Message: result.Message, +func setStatusUnpacked(ext *ocv1alpha1.ClusterExtension, message string) { + apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ + Type: ocv1alpha1.TypeUnpacked, + Status: metav1.ConditionTrue, + Reason: ocv1alpha1.ReasonUnpackSuccess, + Message: message, + ObservedGeneration: ext.GetGeneration(), }) } diff --git a/test/e2e/cluster_extension_install_test.go b/test/e2e/cluster_extension_install_test.go index 721a0f716..5eca7747c 100644 --- a/test/e2e/cluster_extension_install_test.go +++ b/test/e2e/cluster_extension_install_test.go @@ -25,7 +25,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" catalogd "github.com/operator-framework/catalogd/api/core/v1alpha1" - rukpakv1alpha2 "github.com/operator-framework/rukpak/api/v1alpha2" ocv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1" ) @@ -97,12 +96,12 @@ func TestClusterExtensionInstallRegistry(t *testing.T) { t.Log("By eventually reporting a successful unpacked") require.EventuallyWithT(t, func(ct *assert.CollectT) { assert.NoError(ct, c.Get(context.Background(), types.NamespacedName{Name: clusterExtension.Name}, clusterExtension)) - cond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, rukpakv1alpha2.TypeUnpacked) + cond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeUnpacked) if !assert.NotNil(ct, cond) { return } assert.Equal(ct, metav1.ConditionTrue, cond.Status) - assert.Equal(ct, rukpakv1alpha2.ReasonUnpackSuccessful, cond.Reason) + assert.Equal(ct, ocv1alpha1.ReasonUnpackSuccess, cond.Reason) assert.Contains(ct, cond.Message, "Successfully unpacked") }, pollDuration, pollInterval)