Skip to content

Commit 58c5776

Browse files
bentitobentito
authored
✨ Cleaner Condition Types & Reasons (#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]>
1 parent a0fca0d commit 58c5776

File tree

5 files changed

+16
-41
lines changed

5 files changed

+16
-41
lines changed

Diff for: ‎api/v1alpha1/clusterextension_types.go

+6-11
Original file line numberDiff line numberDiff line change
@@ -128,21 +128,17 @@ const (
128128
ReasonErrorGettingClient = "ErrorGettingClient"
129129
ReasonBundleLoadFailed = "BundleLoadFailed"
130130

131-
ReasonInstallationFailed = "InstallationFailed"
132-
ReasonInstallationStatusUnknown = "InstallationStatusUnknown"
133-
ReasonInstallationSucceeded = "InstallationSucceeded"
134-
ReasonResolutionFailed = "ResolutionFailed"
131+
ReasonInstallationFailed = "InstallationFailed"
132+
ReasonResolutionFailed = "ResolutionFailed"
135133

136134
ReasonSuccess = "Success"
137135
ReasonDeprecated = "Deprecated"
138136
ReasonUpgradeFailed = "UpgradeFailed"
139137

140-
ReasonUnpackPending = "UnpackPending"
141138
ReasonUnpackSuccess = "UnpackSuccess"
142139
ReasonUnpackFailed = "UnpackFailed"
143140

144141
ReasonErrorGettingReleaseState = "ErrorGettingReleaseState"
145-
ReasonCreateDynamicWatchFailed = "CreateDynamicWatchFailed"
146142
)
147143

148144
func init() {
@@ -158,20 +154,16 @@ func init() {
158154
)
159155
// TODO(user): add Reasons from above
160156
conditionsets.ConditionReasons = append(conditionsets.ConditionReasons,
161-
ReasonInstallationSucceeded,
162157
ReasonResolutionFailed,
163158
ReasonInstallationFailed,
164159
ReasonSuccess,
165160
ReasonDeprecated,
166161
ReasonUpgradeFailed,
167162
ReasonBundleLoadFailed,
168163
ReasonErrorGettingClient,
169-
ReasonInstallationStatusUnknown,
170-
ReasonUnpackPending,
171164
ReasonUnpackSuccess,
172165
ReasonUnpackFailed,
173166
ReasonErrorGettingReleaseState,
174-
ReasonCreateDynamicWatchFailed,
175167
)
176168
}
177169

@@ -180,8 +172,11 @@ type BundleMetadata struct {
180172
Version string `json:"version"`
181173
}
182174

183-
// ClusterExtensionStatus defines the observed state of ClusterExtension
175+
// ClusterExtensionStatus defines the observed state of ClusterExtension.
184176
type ClusterExtensionStatus struct {
177+
// InstalledBundle should only be modified when a new bundle is successfully installed. This ensures that if there
178+
// is a previously successfully installed a bundle, and an upgrade fails, it is still communicated that there is
179+
// still a bundle that is currently installed and owned by the ClusterExtension.
185180
// +optional
186181
InstalledBundle *BundleMetadata `json:"installedBundle,omitempty"`
187182
// +optional

Diff for: ‎config/base/crd/bases/olm.operatorframework.io_clusterextensions.yaml

+5-1
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ spec:
113113
- serviceAccount
114114
type: object
115115
status:
116-
description: ClusterExtensionStatus defines the observed state of ClusterExtension
116+
description: ClusterExtensionStatus defines the observed state of ClusterExtension.
117117
properties:
118118
conditions:
119119
items:
@@ -188,6 +188,10 @@ spec:
188188
- type
189189
x-kubernetes-list-type: map
190190
installedBundle:
191+
description: |-
192+
InstalledBundle should only be modified when a new bundle is successfully installed. This ensures that if there
193+
is a previously successfully installed a bundle, and an upgrade fails, it is still communicated that there is
194+
still a bundle that is currently installed and owned by the ClusterExtension.
191195
properties:
192196
name:
193197
type: string

Diff for: ‎internal/controllers/clusterextension_controller.go

+4-5
Original file line numberDiff line numberDiff line change
@@ -296,9 +296,8 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
296296

297297
switch unpackResult.State {
298298
case rukpaksource.StatePending:
299-
setStatusUnpackPending(ext, unpackResult.Message)
300-
setInstalledStatusConditionUnknown(ext, "installation has not been attempted as unpack is pending")
301-
299+
setStatusUnpackFailed(ext, unpackResult.Message)
300+
ensureAllConditionsWithReason(ext, ocv1alpha1.ReasonUnpackFailed, "unpack pending")
302301
return ctrl.Result{}, nil
303302
case rukpaksource.StateUnpacked:
304303
setStatusUnpacked(ext, fmt.Sprintf("unpack successful: %v", unpackResult.Message))
@@ -400,7 +399,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
400399
l.V(1).Info("configuring watches for release objects")
401400
relObjects, err := util.ManifestObjects(strings.NewReader(rel.Manifest), fmt.Sprintf("%s-release-manifest", rel.Name))
402401
if err != nil {
403-
setInstalledStatusConditionFailed(ext, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonCreateDynamicWatchFailed, err))
402+
setInstalledStatusConditionFailed(ext, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonInstallationFailed, err))
404403
return ctrl.Result{}, err
405404
}
406405

@@ -430,7 +429,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
430429
return nil
431430
}(); err != nil {
432431
ext.Status.InstalledBundle = nil
433-
setInstalledStatusConditionFailed(ext, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonCreateDynamicWatchFailed, err))
432+
setInstalledStatusConditionFailed(ext, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonInstallationFailed, err))
434433
return ctrl.Result{}, err
435434
}
436435
}

Diff for: ‎internal/controllers/clusterextension_controller_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ func TestClusterExtensionResolutionSucceeds(t *testing.T) {
151151
unpackedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeUnpacked)
152152
require.NotNil(t, unpackedCond)
153153
require.Equal(t, metav1.ConditionFalse, unpackedCond.Status)
154-
require.Equal(t, ocv1alpha1.ReasonUnpackPending, unpackedCond.Reason)
154+
require.Equal(t, ocv1alpha1.ReasonUnpackFailed, unpackedCond.Reason)
155155

156156
require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{}))
157157
}

Diff for: ‎internal/controllers/common_controller.go

-23
Original file line numberDiff line numberDiff line change
@@ -34,17 +34,6 @@ func setResolvedStatusConditionSuccess(ext *ocv1alpha1.ClusterExtension, message
3434
})
3535
}
3636

37-
// setInstalledStatusConditionUnknown sets the installed status condition to unknown.
38-
func setInstalledStatusConditionUnknown(ext *ocv1alpha1.ClusterExtension, message string) {
39-
apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{
40-
Type: ocv1alpha1.TypeInstalled,
41-
Status: metav1.ConditionUnknown,
42-
Reason: ocv1alpha1.ReasonInstallationStatusUnknown,
43-
Message: message,
44-
ObservedGeneration: ext.GetGeneration(),
45-
})
46-
}
47-
4837
// setResolvedStatusConditionFailed sets the resolved status condition to failed.
4938
func setResolvedStatusConditionFailed(ext *ocv1alpha1.ClusterExtension, message string) {
5039
apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{
@@ -109,18 +98,6 @@ func setStatusUnpackFailed(ext *ocv1alpha1.ClusterExtension, message string) {
10998
})
11099
}
111100

112-
// TODO: verify if we need to update the installBundle status or leave it as is.
113-
func setStatusUnpackPending(ext *ocv1alpha1.ClusterExtension, message string) {
114-
ext.Status.InstalledBundle = nil
115-
apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{
116-
Type: ocv1alpha1.TypeUnpacked,
117-
Status: metav1.ConditionFalse,
118-
Reason: ocv1alpha1.ReasonUnpackPending,
119-
Message: message,
120-
ObservedGeneration: ext.GetGeneration(),
121-
})
122-
}
123-
124101
func setStatusUnpacked(ext *ocv1alpha1.ClusterExtension, message string) {
125102
apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{
126103
Type: ocv1alpha1.TypeUnpacked,

0 commit comments

Comments
 (0)