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

✨ Cleaner Condition Types & Reasons #1007

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

✨ Cleaner Condition Types & Reasons #1007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6867932
Replace ReasonCreateDynamicWatchFailed w/ ReasonInstallationFailed
bentito Jul 2, 2024
3b2b39d
Remove unused ReasonInstallationSucceeded
bentito Jul 2, 2024
135cf59
Drop usage of ConditionUnknown on Pending Install
bentito Jul 2, 2024
b6a1109
Remove ReasonUnpackPending/InstallationsStatusUnk
bentito Jul 2, 2024
e0ff96d
Update related TODO in ClusterExtension controller
bentito Jul 3, 2024
5758f10
Rm InstalledStatus->nil on upack & add comment
bentito Jul 3, 2024
1a0cc12
Align CRD for added Go doc on InstalledBundle
bentito Jul 3, 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
15 changes: 4 additions & 11 deletions api/v1alpha1/clusterextension_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,21 +115,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() {
Expand All @@ -145,20 +141,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,
)
}

Expand All @@ -167,7 +159,8 @@ type BundleMetadata struct {
Version string `json:"version"`
}

// ClusterExtensionStatus defines the observed state of ClusterExtension
// ClusterExtensionStatus defines the observed state of ClusterExtension.
// InstalledBundle should only be modified when a new bundle is successfully installed. This ensures that if you've previously successfully installed a bundle before, and an upgrade fails, it is still appropriately communicated to you that there is still a bundle that is currently installed and owned by the ClusterExtension.
Copy link
Member

Choose a reason for hiding this comment

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

Two comments:

  1. I don't think the Status object is the right place for details of the InstalledBundle field. Move this comment to that field?
  2. We should be able to assess a "current installation" independent of a bunch of other reconcile tasks. Our reconcile behavior probably needs to go and actually lookup the currently installed bundle early on and affirmatively set status.conditions[type=Installed] and status.installedBundle based on what we find. If we fail trying to lookup the installed bundle, we should set the condition to Unknown, probably unset the installedBundle field, and then continue on with other unrelated reconcile tasks.

Copy link

Choose a reason for hiding this comment

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

I don't think the Status object is the right place for details of the InstalledBundle field. Move this comment to that field?

@joelanford Not sure if I understand your comments fully. Currently InstalledBundle is part of ClusterExtensionStatus , I guess you are suggesting it should not?

type ClusterExtensionStatus struct {
	// +optional
	InstalledBundle *BundleMetadata `json:"installedBundle,omitempty"`
	// +optional
	ResolvedBundle *BundleMetadata `json:"resolvedBundle,omitempty"`
	// +patchMergeKey=type
	// +patchStrategy=merge
	// +listType=map
	// +listMapKey=type
	Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"`
}

Copy link

Choose a reason for hiding this comment

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

Also I do not see any suggested change for the InstalledBundle in the RFC document.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing (1) above
on point (2), I'd llke to add #1012 and keep this PR focused on implementing the RFC.

type ClusterExtensionStatus struct {
// +optional
InstalledBundle *BundleMetadata `json:"installedBundle,omitempty"`
Expand Down
4 changes: 3 additions & 1 deletion config/base/crd/bases/olm.operatorframework.io_clusterextensions.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,9 @@ spec:
- packageName
type: object
status:
description: ClusterExtensionStatus defines the observed state of ClusterExtension
description: |-
ClusterExtensionStatus defines the observed state of ClusterExtension.
InstalledBundle should only be modified when a new bundle is successfully installed. This ensures that if you've previously successfully installed a bundle before, and an upgrade fails, it is still appropriately communicated to you that there is still a bundle that is currently installed and owned by the ClusterExtension.
properties:
conditions:
items:
Expand Down
11 changes: 5 additions & 6 deletions internal/controllers/clusterextension_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,13 +273,12 @@ 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")
setStatusInstallFalseUnpackFailed(ext, unpackResult.Message)
Copy link
Member

Choose a reason for hiding this comment

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

Unpack state is pending, but we're setting Unpacked=False, Failed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well this was changed to acknowledge that we've switched to direct image registry and that as far as opr-ctrl is concerned we can't really be in an Unknown state, we're either failed or not. Perhaps we should just not process StatePending here?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we should write this code with specific knowledge of the underlying Source implementation. In theory, the source could be switched out to a different implementation that can return StatePending. I've been tinkering with an async direct image client, so this is not entirely hypothetical.

More broadly, I wonder if we even need an Unpacked condition type? I don't see it in the diagram in the RFC. The only reason we are unpacking a bundle is so that we can install it. So can we rollup unpacking status into Progressing and/or Installed conditions?

setInstalledStatusConditionInstalledFalse(ext, "installation has not been attempted as unpack is pending")
Copy link
Member

Choose a reason for hiding this comment

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

Installed=False is not always correct here. If we have previously installed a bundle, and now the unpack of the next bundle is pending, we still have an installed bundle.

The installation status and the bundle unpack status are not related to each other.


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.
// TODO: https://github.com/operator-framework/rukpak/pull/897 merged, add finalizer to clean the stored bundles
if err := r.Storage.Store(ctx, ext, unpackResult.Bundle); err != nil {
setStatusUnpackFailed(ext, err.Error())
return ctrl.Result{}, err
Expand Down Expand Up @@ -378,7 +377,7 @@ 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, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonCreateDynamicWatchFailed, err))
setInstalledStatusConditionFailed(ext, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonInstallationFailed, err))
return ctrl.Result{}, err
}

Expand All @@ -402,7 +401,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
}
}
Expand Down
4 changes: 2 additions & 2 deletions internal/controllers/clusterextension_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ func TestClusterExtensionChannelVersionExists(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{}))
}
Expand Down Expand Up @@ -242,7 +242,7 @@ func TestClusterExtensionChannelExistsNoVersion(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)

verifyInvariants(ctx, t, reconciler.Client, clusterExtension)
require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{}))
Expand Down
14 changes: 6 additions & 8 deletions internal/controllers/common_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,12 @@ func setResolvedStatusConditionSuccess(ext *ocv1alpha1.ClusterExtension, message
})
}

// setInstalledStatusConditionUnknown sets the installed status condition to unknown.
func setInstalledStatusConditionUnknown(ext *ocv1alpha1.ClusterExtension, message string) {
// setInstalledStatusConditionInstalledFalse sets the installed status condition to unknown.
func setInstalledStatusConditionInstalledFalse(ext *ocv1alpha1.ClusterExtension, message string) {
apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{
Type: ocv1alpha1.TypeInstalled,
Status: metav1.ConditionUnknown,
Reason: ocv1alpha1.ReasonInstallationStatusUnknown,
Status: metav1.ConditionFalse,
Reason: ocv1alpha1.ReasonInstallationFailed,
Message: message,
ObservedGeneration: ext.GetGeneration(),
})
Expand Down Expand Up @@ -118,13 +118,11 @@ 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
func setStatusInstallFalseUnpackFailed(ext *ocv1alpha1.ClusterExtension, message string) {
apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{
Type: ocv1alpha1.TypeUnpacked,
Status: metav1.ConditionFalse,
Reason: ocv1alpha1.ReasonUnpackPending,
Reason: ocv1alpha1.ReasonUnpackFailed,
Message: message,
ObservedGeneration: ext.GetGeneration(),
})
Expand Down
Loading