✨improve Extension status conditions (#683)

* map kapp conditions to Extension

Signed-off-by: Ankita Thomas <[email protected]>

* add Progressing condition

Signed-off-by: Ankita Thomas <[email protected]>

* remove changes to deprecation statuses

Signed-off-by: Ankita Thomas <[email protected]>

---------

Signed-off-by: Ankita Thomas <[email protected]>

Author: ankitathomas

api/v1alpha1/clusterextension_types_test.go

@@ -22,7 +22,7 @@ func TestClusterExtensionTypeRegistration(t *testing.T) {
 	}
 
 	for _, tt := range types {
-		if !slices.Contains(conditionsets.ConditionTypes, tt) {
+		if !slices.Contains(conditionsets.ConditionTypes, tt) && !slices.Contains(conditionsets.ExtensionConditionTypes, tt) {
 			t.Errorf("append Type%s to conditionsets.ConditionTypes in this package's init function", tt)
 		}
 	}
@@ -32,6 +32,11 @@ func TestClusterExtensionTypeRegistration(t *testing.T) {
 			t.Errorf("there must be a Type%[1]s string literal constant for type %[1]q (i.e. 'const Type%[1]s = %[1]q')", tt)
 		}
 	}
+	for _, tt := range conditionsets.ExtensionConditionTypes {
+		if !slices.Contains(types, tt) {
+			t.Errorf("there must be a Type%[1]s string literal constant for type %[1]q (i.e. 'const Type%[1]s = %[1]q')", tt)
+		}
+	}
 }
 
 func TestClusterExtensionReasonRegistration(t *testing.T) {
@@ -41,7 +46,7 @@ func TestClusterExtensionReasonRegistration(t *testing.T) {
 	}
 
 	for _, r := range reasons {
-		if !slices.Contains(conditionsets.ConditionReasons, r) {
+		if !slices.Contains(conditionsets.ConditionReasons, r) && !slices.Contains(conditionsets.ExtensionConditionReasons, r) {
 			t.Errorf("append Reason%s to conditionsets.ConditionReasons in this package's init function.", r)
 		}
 	}
@@ -50,6 +55,11 @@ func TestClusterExtensionReasonRegistration(t *testing.T) {
 			t.Errorf("there must be a Reason%[1]s string literal constant for reason %[1]q (i.e. 'const Reason%[1]s = %[1]q')", r)
 		}
 	}
+	for _, r := range conditionsets.ExtensionConditionReasons {
+		if !slices.Contains(reasons, r) {
+			t.Errorf("there must be a Reason%[1]s string literal constant for reason %[1]q (i.e. 'const Reason%[1]s = %[1]q')", r)
+		}
+	}
 }
 
 // parseConstants parses the values of the top-level constants in the current

api/v1alpha1/extension_types.go

@@ -18,6 +18,8 @@ package v1alpha1
 
 import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	"github.com/operator-framework/operator-controller/internal/conditionsets"
 )
 
 const (
@@ -133,6 +135,25 @@ type ExtensionList struct {
 	Items           []Extension `json:"items"`
 }
 
+const (
+	// TypeProgressing indicates whether operator-controller is
+	// reconciling, installing, updating or deleting an extension.
+	TypeProgressing = "Progressing"
+
+	ReasonProgressing                = "Progressing"
+	ReasonFailedToReachDesiredIntent = "FailedToReachDesiredIntent"
+	ReasonReachedDesiredIntent       = "ReachedDesiredIntent"
+)
+
 func init() {
 	SchemeBuilder.Register(&Extension{}, &ExtensionList{})
+
+	conditionsets.ExtensionConditionTypes = []string{
+		TypeProgressing,
+	}
+	conditionsets.ExtensionConditionReasons = []string{
+		ReasonProgressing,
+		ReasonFailedToReachDesiredIntent,
+		ReasonReachedDesiredIntent,
+	}
 }

internal/conditionsets/conditionsets.go

@@ -22,3 +22,7 @@ package conditionsets
 // NOTE: These are populated by init() in api/v1alpha1/clusterextension_types.go
 var ConditionTypes []string
 var ConditionReasons []string
+
+// ExtensionConditionTypes is the set of Extension exclusive condition Types.
+var ExtensionConditionTypes []string
+var ExtensionConditionReasons []string

internal/controllers/common_controller.go

@@ -65,17 +65,6 @@ func setResolvedStatusConditionFailed(conditions *[]metav1.Condition, message st
 	})
 }
 
-// setResolvedStatusConditionUnknown sets the resolved status condition to unknown.
-func setResolvedStatusConditionUnknown(conditions *[]metav1.Condition, message string, generation int64) {
-	apimeta.SetStatusCondition(conditions, metav1.Condition{
-		Type:               ocv1alpha1.TypeResolved,
-		Status:             metav1.ConditionUnknown,
-		Reason:             ocv1alpha1.ReasonResolutionUnknown,
-		Message:            message,
-		ObservedGeneration: generation,
-	})
-}
-
 // setInstalledStatusConditionSuccess sets the installed status condition to success.
 func setInstalledStatusConditionSuccess(conditions *[]metav1.Condition, message string, generation int64) {
 	apimeta.SetStatusCondition(conditions, metav1.Condition{
@@ -117,3 +106,36 @@ func setDeprecationStatusesUnknown(conditions *[]metav1.Condition, message strin
 		})
 	}
 }
+
+// setProgressingStatusConditionSuccess sets the progressing status condition to false for a successful install or upgrade.
+func setProgressingStatusConditionSuccess(conditions *[]metav1.Condition, message string, generation int64) {
+	apimeta.SetStatusCondition(conditions, metav1.Condition{
+		Type:               ocv1alpha1.TypeProgressing,
+		Status:             metav1.ConditionFalse,
+		Reason:             ocv1alpha1.ReasonReachedDesiredIntent,
+		Message:            message,
+		ObservedGeneration: generation,
+	})
+}
+
+// setProgressingStatusConditionFailed sets the progressing status condition to False for a failed install or upgrade.
+func setProgressingStatusConditionFailed(conditions *[]metav1.Condition, message string, generation int64) {
+	apimeta.SetStatusCondition(conditions, metav1.Condition{
+		Type:               ocv1alpha1.TypeProgressing,
+		Status:             metav1.ConditionFalse,
+		Reason:             ocv1alpha1.ReasonFailedToReachDesiredIntent,
+		Message:            message,
+		ObservedGeneration: generation,
+	})
+}
+
+// setProgressingStatusConditionProgressing sets the progressing status condition to true for an app being reconciled.
+func setProgressingStatusConditionProgressing(conditions *[]metav1.Condition, message string, generation int64) {
+	apimeta.SetStatusCondition(conditions, metav1.Condition{
+		Type:               ocv1alpha1.TypeProgressing,
+		Status:             metav1.ConditionTrue,
+		Reason:             ocv1alpha1.ReasonProgressing,
+		Message:            message,
+		ObservedGeneration: generation,
+	})
+}

internal/controllers/extension_controller.go

@@ -127,14 +127,14 @@ func (r *ExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alpha1.Ext
 	if !features.OperatorControllerFeatureGate.Enabled(features.EnableExtensionAPI) {
 		l.Info("extension feature is gated", "name", ext.GetName(), "namespace", ext.GetNamespace())
 
-		// Set the TypeInstalled condition to Unknown to indicate that the resolution
+		// Set the TypeInstalled condition to Failed to indicate that the resolution
 		// hasn't been attempted yet, due to the spec being invalid.
 		ext.Status.InstalledBundle = nil
-		setInstalledStatusConditionUnknown(&ext.Status.Conditions, "extension feature is disabled", ext.GetGeneration())
-		// Set the TypeResolved condition to Unknown to indicate that the resolution
+		setInstalledStatusConditionFailed(&ext.Status.Conditions, "extension feature is disabled", ext.GetGeneration())
+		// Set the TypeResolved condition to Failed to indicate that the resolution
 		// hasn't been attempted yet, due to the spec being invalid.
 		ext.Status.ResolvedBundle = nil
-		setResolvedStatusConditionUnknown(&ext.Status.Conditions, "extension feature is disabled", ext.GetGeneration())
+		setResolvedStatusConditionFailed(&ext.Status.Conditions, "extension feature is disabled", ext.GetGeneration())
 
 		setDeprecationStatusesUnknown(&ext.Status.Conditions, "extension feature is disabled", ext.GetGeneration())
 		return ctrl.Result{}, nil
@@ -150,8 +150,10 @@ func (r *ExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alpha1.Ext
 	// TODO: Improve the resolution logic.
 	bundle, err := r.resolve(ctx, *ext)
 	if err != nil {
-		ext.Status.InstalledBundle = nil
-		setInstalledStatusConditionUnknown(&ext.Status.Conditions, "installation has not been attempted as resolution failed", ext.GetGeneration())
+		if c := apimeta.FindStatusCondition(ext.Status.Conditions, ocv1alpha1.TypeInstalled); c == nil {
+			ext.Status.InstalledBundle = nil
+			setInstalledStatusConditionFailed(&ext.Status.Conditions, "installation has not been attempted as resolution failed", ext.GetGeneration())
+		}
 		ext.Status.ResolvedBundle = nil
 		setResolvedStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration())
 		setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as resolution failed", ext.GetGeneration())
@@ -164,33 +166,46 @@ func (r *ExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alpha1.Ext
 
 	mediaType, err := bundle.MediaType()
 	if err != nil {
-		setInstalledStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration())
-		setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as installation has failed", ext.GetGeneration())
+		if c := apimeta.FindStatusCondition(ext.Status.Conditions, ocv1alpha1.TypeInstalled); c == nil {
+			ext.Status.InstalledBundle = nil
+			setInstalledStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("failed to read bundle mediaType: %v", err), ext.GetGeneration())
+			setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as installation has failed", ext.GetGeneration())
+		}
+		setProgressingStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("failed to read bundle mediaType: %v", err), ext.GetGeneration())
 		return ctrl.Result{}, err
 	}
 
 	// TODO: this needs to include the registryV1 bundle option. As of this PR, this only supports direct
 	// installation of a set of manifests.
 	if mediaType != catalogmetadata.MediaTypePlain {
-		// Set the TypeInstalled condition to Unknown to indicate that the resolution
-		// hasn't been attempted yet, due to the spec being invalid.
-		ext.Status.InstalledBundle = nil
-		setInstalledStatusConditionUnknown(&ext.Status.Conditions, fmt.Sprintf("bundle type %s not supported currently", mediaType), ext.GetGeneration())
-		setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as installation has failed", ext.GetGeneration())
+		if c := apimeta.FindStatusCondition(ext.Status.Conditions, ocv1alpha1.TypeInstalled); c == nil {
+			// Set the TypeInstalled condition to Failed to indicate that the resolution
+			// hasn't been attempted yet, due to the spec being invalid.
+			ext.Status.InstalledBundle = nil
+			setInstalledStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("bundle type %s not supported currently", mediaType), ext.GetGeneration())
+			setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as installation has failed", ext.GetGeneration())
+		}
+		setProgressingStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("bundle type %s not supported currently", mediaType), ext.GetGeneration())
 		return ctrl.Result{}, nil
 	}
 
 	app, err := r.GenerateExpectedApp(*ext, bundle)
 	if err != nil {
-		setInstalledStatusConditionUnknown(&ext.Status.Conditions, err.Error(), ext.GetGeneration())
-		setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as installation has failed", ext.GetGeneration())
+		if c := apimeta.FindStatusCondition(ext.Status.Conditions, ocv1alpha1.TypeInstalled); c == nil {
+			ext.Status.InstalledBundle = nil
+			setInstalledStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration())
+			setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as installation has failed", ext.GetGeneration())
+		}
+		setProgressingStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration())
 		return ctrl.Result{}, err
 	}
 
 	if err := r.ensureApp(ctx, app); err != nil {
 		// originally Reason: ocv1alpha1.ReasonInstallationFailed
 		ext.Status.InstalledBundle = nil
 		setInstalledStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration())
+		setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as installation has failed", ext.GetGeneration())
+		setProgressingStatusConditionProgressing(&ext.Status.Conditions, "installation failed", ext.GetGeneration())
 		return ctrl.Result{}, err
 	}
 
@@ -199,14 +214,19 @@ func (r *ExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alpha1.Ext
 	if err := runtime.DefaultUnstructuredConverter.FromUnstructured(app.UnstructuredContent(), existingTypedApp); err != nil {
 		// originally Reason: ocv1alpha1.ReasonInstallationStatusUnknown
 		ext.Status.InstalledBundle = nil
-		setInstalledStatusConditionUnknown(&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())
+		setProgressingStatusConditionProgressing(&ext.Status.Conditions, "installation failed", ext.GetGeneration())
 		return ctrl.Result{}, err
 	}
 
-	mapAppStatusToInstalledCondition(existingTypedApp, ext, bundle)
+	ext.Status.InstalledBundle = bundleMetadataFor(bundle)
+	setInstalledStatusConditionSuccess(&ext.Status.Conditions, fmt.Sprintf("successfully installed %v", ext.Status.InstalledBundle), ext.GetGeneration())
 	SetDeprecationStatusInExtension(ext, bundle)
 
+	// TODO: add conditions to determine extension health
+	mapAppStatusToCondition(existingTypedApp, ext)
+
 	return ctrl.Result{}, nil
 }
 
@@ -228,28 +248,37 @@ func (r *ExtensionReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		Complete(r)
 }
 
-// TODO: follow up with mapping of all the available App statuses: https://github.com/carvel-dev/kapp-controller/blob/855063edee53315811a13ee8d5df1431ba258ede/pkg/apis/kappctrl/v1alpha1/status.go#L28-L35
-// mapAppStatusToInstalledCondition currently maps only the installed condition.
-func mapAppStatusToInstalledCondition(existingApp *kappctrlv1alpha1.App, ext *ocv1alpha1.Extension, bundle *catalogmetadata.Bundle) {
-	appReady := findStatusCondition(existingApp.Status.GenericStatus.Conditions, kappctrlv1alpha1.ReconcileSucceeded)
-	if appReady == nil {
-		ext.Status.InstalledBundle = nil
-		setInstalledStatusConditionUnknown(&ext.Status.Conditions, "install status unknown", ext.Generation)
+// mapAppStatusToCondition maps the reconciling/deleting App conditions to the installed/deleting conditions on the Extension.
+func mapAppStatusToCondition(existingApp *kappctrlv1alpha1.App, ext *ocv1alpha1.Extension) {
+	// Note: App.Status.Inspect errors are never surfaced to App conditions, so are currently ignored when determining App status.
+	if ext == nil || existingApp == nil {
 		return
 	}
-
-	if appReady.Status != corev1.ConditionTrue {
-		ext.Status.InstalledBundle = nil
-		setInstalledStatusConditionFailed(
-			&ext.Status.Conditions,
-			appReady.Message,
-			ext.GetGeneration(),
-		)
-		return
+	message := existingApp.Status.FriendlyDescription
+	if len(message) == 0 || strings.Contains(message, "Error (see .status.usefulErrorMessage for details)") {
+		message = existingApp.Status.UsefulErrorMessage
 	}
 
-	ext.Status.InstalledBundle = bundleMetadataFor(bundle)
-	setInstalledStatusConditionSuccess(&ext.Status.Conditions, appReady.Message, ext.Generation)
+	appStatusMapFn := map[kappctrlv1alpha1.ConditionType]func(*[]metav1.Condition, string, int64){
+		kappctrlv1alpha1.Deleting:           setProgressingStatusConditionProgressing,
+		kappctrlv1alpha1.Reconciling:        setProgressingStatusConditionProgressing,
+		kappctrlv1alpha1.DeleteFailed:       setProgressingStatusConditionFailed,
+		kappctrlv1alpha1.ReconcileFailed:    setProgressingStatusConditionFailed,
+		kappctrlv1alpha1.ReconcileSucceeded: setProgressingStatusConditionSuccess,
+	}
+	for cond := range appStatusMapFn {
+		if c := findStatusCondition(existingApp.Status.GenericStatus.Conditions, cond); c != nil && c.Status == corev1.ConditionTrue {
+			if len(message) == 0 {
+				message = c.Message
+			}
+			appStatusMapFn[cond](&ext.Status.Conditions, fmt.Sprintf("App %s: %s", c.Type, message), ext.Generation)
+			return
+		}
+	}
+	if len(message) == 0 {
+		message = "waiting for app"
+	}
+	setProgressingStatusConditionProgressing(&ext.Status.Conditions, message, ext.Generation)
 }
 
 // setDeprecationStatus will set the appropriate deprecation statuses for a Extension