From 09af010667f286b412cc89e9a65181ea3b1624d0 Mon Sep 17 00:00:00 2001 From: Brett Tofel Date: Mon, 22 Jul 2024 10:03:59 -0400 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20Cleaner=20Condition=20Types=20&=20R?= =?UTF-8?q?easons=20(#1007)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Replace ReasonCreateDynamicWatchFailed w/ ReasonInstallationFailed Signed-off-by: Brett Tofel * Remove unused ReasonInstallationSucceeded Signed-off-by: Brett Tofel * Drop usage of ConditionUnknown on Pending Install Signed-off-by: Brett Tofel * Remove ReasonUnpackPending/InstallationsStatusUnk Signed-off-by: Brett Tofel * Update related TODO in ClusterExtension controller Signed-off-by: Brett Tofel * Rm InstalledStatus->nil on upack & add comment Signed-off-by: Brett Tofel * Align CRD for added Go doc on InstalledBundle Signed-off-by: Brett Tofel * Move comment on InstalledBundle field Signed-off-by: Brett Tofel * Commit manifest changes for godoc change Signed-off-by: Brett Tofel * Decouple bundle unpacking and installed statuses Signed-off-by: Brett Tofel * Remove unneeded unpack stage helper method Signed-off-by: Brett Tofel --------- Signed-off-by: Brett Tofel --- api/v1alpha1/clusterextension_types.go | 17 +++++--------- ...peratorframework.io_clusterextensions.yaml | 6 ++++- .../clusterextension_controller.go | 9 ++++---- .../clusterextension_controller_test.go | 2 +- internal/controllers/common_controller.go | 23 ------------------- 5 files changed, 16 insertions(+), 41 deletions(-) diff --git a/api/v1alpha1/clusterextension_types.go b/api/v1alpha1/clusterextension_types.go index a784ddd4a..c88cb7add 100644 --- a/api/v1alpha1/clusterextension_types.go +++ b/api/v1alpha1/clusterextension_types.go @@ -128,21 +128,17 @@ const ( ReasonErrorGettingClient = "ErrorGettingClient" ReasonBundleLoadFailed = "BundleLoadFailed" - ReasonInstallationFailed = "InstallationFailed" - ReasonInstallationStatusUnknown = "InstallationStatusUnknown" - ReasonInstallationSucceeded = "InstallationSucceeded" - ReasonResolutionFailed = "ResolutionFailed" + ReasonInstallationFailed = "InstallationFailed" + ReasonResolutionFailed = "ResolutionFailed" ReasonSuccess = "Success" ReasonDeprecated = "Deprecated" ReasonUpgradeFailed = "UpgradeFailed" - ReasonUnpackPending = "UnpackPending" ReasonUnpackSuccess = "UnpackSuccess" ReasonUnpackFailed = "UnpackFailed" ReasonErrorGettingReleaseState = "ErrorGettingReleaseState" - ReasonCreateDynamicWatchFailed = "CreateDynamicWatchFailed" ) func init() { @@ -158,7 +154,6 @@ func init() { ) // TODO(user): add Reasons from above conditionsets.ConditionReasons = append(conditionsets.ConditionReasons, - ReasonInstallationSucceeded, ReasonResolutionFailed, ReasonInstallationFailed, ReasonSuccess, @@ -166,12 +161,9 @@ func init() { ReasonUpgradeFailed, ReasonBundleLoadFailed, ReasonErrorGettingClient, - ReasonInstallationStatusUnknown, - ReasonUnpackPending, ReasonUnpackSuccess, ReasonUnpackFailed, ReasonErrorGettingReleaseState, - ReasonCreateDynamicWatchFailed, ) } @@ -180,8 +172,11 @@ type BundleMetadata struct { Version string `json:"version"` } -// ClusterExtensionStatus defines the observed state of ClusterExtension +// ClusterExtensionStatus defines the observed state of ClusterExtension. type ClusterExtensionStatus struct { + // InstalledBundle should only be modified when a new bundle is successfully installed. This ensures that if there + // is a previously successfully installed a bundle, and an upgrade fails, it is still communicated that there is + // still a bundle that is currently installed and owned by the ClusterExtension. // +optional InstalledBundle *BundleMetadata `json:"installedBundle,omitempty"` // +optional diff --git a/config/base/crd/bases/olm.operatorframework.io_clusterextensions.yaml b/config/base/crd/bases/olm.operatorframework.io_clusterextensions.yaml index 9b92a3045..24cdf64af 100644 --- a/config/base/crd/bases/olm.operatorframework.io_clusterextensions.yaml +++ b/config/base/crd/bases/olm.operatorframework.io_clusterextensions.yaml @@ -113,7 +113,7 @@ spec: - serviceAccount type: object status: - description: ClusterExtensionStatus defines the observed state of ClusterExtension + description: ClusterExtensionStatus defines the observed state of ClusterExtension. properties: conditions: items: @@ -188,6 +188,10 @@ spec: - type x-kubernetes-list-type: map installedBundle: + description: |- + InstalledBundle should only be modified when a new bundle is successfully installed. This ensures that if there + is a previously successfully installed a bundle, and an upgrade fails, it is still communicated that there is + still a bundle that is currently installed and owned by the ClusterExtension. properties: name: type: string diff --git a/internal/controllers/clusterextension_controller.go b/internal/controllers/clusterextension_controller.go index b042d4de6..ef5939867 100644 --- a/internal/controllers/clusterextension_controller.go +++ b/internal/controllers/clusterextension_controller.go @@ -296,9 +296,8 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp switch unpackResult.State { case rukpaksource.StatePending: - setStatusUnpackPending(ext, unpackResult.Message) - setInstalledStatusConditionUnknown(ext, "installation has not been attempted as unpack is pending") - + setStatusUnpackFailed(ext, unpackResult.Message) + ensureAllConditionsWithReason(ext, ocv1alpha1.ReasonUnpackFailed, "unpack pending") return ctrl.Result{}, nil case rukpaksource.StateUnpacked: setStatusUnpacked(ext, fmt.Sprintf("unpack successful: %v", unpackResult.Message)) @@ -400,7 +399,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp l.V(1).Info("configuring watches for release objects") relObjects, err := util.ManifestObjects(strings.NewReader(rel.Manifest), fmt.Sprintf("%s-release-manifest", rel.Name)) if err != nil { - setInstalledStatusConditionFailed(ext, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonCreateDynamicWatchFailed, err)) + setInstalledStatusConditionFailed(ext, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonInstallationFailed, err)) return ctrl.Result{}, err } @@ -430,7 +429,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp return nil }(); err != nil { ext.Status.InstalledBundle = nil - setInstalledStatusConditionFailed(ext, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonCreateDynamicWatchFailed, err)) + setInstalledStatusConditionFailed(ext, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonInstallationFailed, err)) return ctrl.Result{}, err } } diff --git a/internal/controllers/clusterextension_controller_test.go b/internal/controllers/clusterextension_controller_test.go index bcf652633..f3a54bb0d 100644 --- a/internal/controllers/clusterextension_controller_test.go +++ b/internal/controllers/clusterextension_controller_test.go @@ -151,7 +151,7 @@ func TestClusterExtensionResolutionSucceeds(t *testing.T) { unpackedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeUnpacked) require.NotNil(t, unpackedCond) require.Equal(t, metav1.ConditionFalse, unpackedCond.Status) - require.Equal(t, ocv1alpha1.ReasonUnpackPending, unpackedCond.Reason) + require.Equal(t, ocv1alpha1.ReasonUnpackFailed, unpackedCond.Reason) require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{})) } diff --git a/internal/controllers/common_controller.go b/internal/controllers/common_controller.go index 5900ced30..b69c76774 100644 --- a/internal/controllers/common_controller.go +++ b/internal/controllers/common_controller.go @@ -34,17 +34,6 @@ func setResolvedStatusConditionSuccess(ext *ocv1alpha1.ClusterExtension, message }) } -// setInstalledStatusConditionUnknown sets the installed status condition to unknown. -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: ext.GetGeneration(), - }) -} - // setResolvedStatusConditionFailed sets the resolved status condition to failed. func setResolvedStatusConditionFailed(ext *ocv1alpha1.ClusterExtension, message string) { apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ @@ -109,18 +98,6 @@ func setStatusUnpackFailed(ext *ocv1alpha1.ClusterExtension, message string) { }) } -// TODO: verify if we need to update the installBundle status or leave it as is. -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: ocv1alpha1.ReasonUnpackPending, - Message: message, - ObservedGeneration: ext.GetGeneration(), - }) -} - func setStatusUnpacked(ext *ocv1alpha1.ClusterExtension, message string) { apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ Type: ocv1alpha1.TypeUnpacked,