✨ Cleaner Condition Types & Reasons (operator-framework#1007)

* Replace ReasonCreateDynamicWatchFailed w/ ReasonInstallationFailed Signed-off-by: Brett Tofel <[email protected]> * Remove unused ReasonInstallationSucceeded Signed-off-by: Brett Tofel <[email protected]> * Drop usage of ConditionUnknown on Pending Install Signed-off-by: Brett Tofel <[email protected]> * Remove ReasonUnpackPending/InstallationsStatusUnk Signed-off-by: Brett Tofel <[email protected]> * Update related TODO in ClusterExtension controller Signed-off-by: Brett Tofel <[email protected]> * Rm InstalledStatus->nil on upack & add comment Signed-off-by: Brett Tofel <[email protected]> * Align CRD for added Go doc on InstalledBundle Signed-off-by: Brett Tofel <[email protected]> * Move comment on InstalledBundle field Signed-off-by: Brett Tofel <[email protected]> * Commit manifest changes for godoc change Signed-off-by: Brett Tofel <[email protected]> * Decouple bundle unpacking and installed statuses Signed-off-by: Brett Tofel <[email protected]> * Remove unneeded unpack stage helper method Signed-off-by: Brett Tofel <[email protected]> --------- Signed-off-by: Brett Tofel <[email protected]>
kevinrizza · Aug 13, 2024 · 09af010 · 09af010
1 parent ad665f8
commit 09af010
Show file tree

Hide file tree

Showing 5 changed files with 16 additions and 41 deletions.
diff --git 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,20 +154,16 @@ func init() {
 	)
 	// TODO(user): add Reasons from above
 	conditionsets.ConditionReasons = append(conditionsets.ConditionReasons,
-		ReasonInstallationSucceeded,
 		ReasonResolutionFailed,
 		ReasonInstallationFailed,
 		ReasonSuccess,
 		ReasonDeprecated,
 		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
@@ -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
@@ -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
@@ -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
@@ -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,