Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

✨improve Extension status conditions #683

Merged
merged 3 commits into from
Apr 22, 2024
Merged

✨improve Extension status conditions #683

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
be9f4a3
map kapp conditions to Extension
ankitathomas Mar 7, 2024
ce3a005
add Progressing condition
ankitathomas Mar 25, 2024
27a6d32
remove changes to deprecation statuses
ankitathomas Apr 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions api/v1alpha1/clusterextension_types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Expand All @@ -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) {
Expand All @@ -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)
}
}
Expand All @@ -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
Expand Down
21 changes: 21 additions & 0 deletions api/v1alpha1/extension_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ package v1alpha1

import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"github.com/operator-framework/operator-controller/internal/conditionsets"
)

const (
Expand Down Expand Up @@ -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,
}
}
4 changes: 4 additions & 0 deletions internal/conditionsets/conditionsets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
44 changes: 33 additions & 11 deletions internal/controllers/common_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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,
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We could also pass in the the reason to the method signature, such that these helpers are used to just indicate if the particular Status is set to false or true. Its a personal preference, so whatever you think makes it easier to read the code.

}

// 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,
})
}
99 changes: 64 additions & 35 deletions internal/controllers/extension_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you didn't touch this part of the code, but we should be able to determine deprecation status:

  • for package and channel if we have a catalog
  • for bundle if we have an installed bundle

Can you make a follow-up issue to improve this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#671 should address it

Expand All @@ -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())
ankitathomas marked this conversation as resolved.
Show resolved Hide resolved
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())
ankitathomas marked this conversation as resolved.
Show resolved Hide resolved
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
}

Expand All @@ -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
}

Expand All @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iirc, also Status.Inspect appears if the knob to inspect resources is set to true in the spec. We would have to probably cover this aspect better with the health checks.

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)
Comment on lines +262 to +281
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a huge fan of looping through all the status conditions 5 times but since we are moving away from building on top of the App CR and this is an optimization that could be made in the future I'm fine with it for now

}

// setDeprecationStatus will set the appropriate deprecation statuses for a Extension
Expand Down
Loading