From bda06aab8c129e1aa1483a0539db1d4ac7a5c660 Mon Sep 17 00:00:00 2001 From: Brett Tofel Date: Tue, 2 Jul 2024 06:20:30 -0400 Subject: [PATCH 01/11] Replace ReasonCreateDynamicWatchFailed w/ ReasonInstallationFailed Signed-off-by: Brett Tofel --- api/v1alpha1/clusterextension_types.go | 2 -- internal/controllers/clusterextension_controller.go | 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/api/v1alpha1/clusterextension_types.go b/api/v1alpha1/clusterextension_types.go index a784ddd4a..37b440d10 100644 --- a/api/v1alpha1/clusterextension_types.go +++ b/api/v1alpha1/clusterextension_types.go @@ -142,7 +142,6 @@ const ( ReasonUnpackFailed = "UnpackFailed" ReasonErrorGettingReleaseState = "ErrorGettingReleaseState" - ReasonCreateDynamicWatchFailed = "CreateDynamicWatchFailed" ) func init() { @@ -171,7 +170,6 @@ func init() { ReasonUnpackSuccess, ReasonUnpackFailed, ReasonErrorGettingReleaseState, - ReasonCreateDynamicWatchFailed, ) } diff --git a/internal/controllers/clusterextension_controller.go b/internal/controllers/clusterextension_controller.go index b042d4de6..eaf4c4e9d 100644 --- a/internal/controllers/clusterextension_controller.go +++ b/internal/controllers/clusterextension_controller.go @@ -400,7 +400,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 +430,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 } } From f3a024ea62af8153e22489cfdd338520f21bc4c8 Mon Sep 17 00:00:00 2001 From: Brett Tofel Date: Tue, 2 Jul 2024 06:23:14 -0400 Subject: [PATCH 02/11] Remove unused ReasonInstallationSucceeded Signed-off-by: Brett Tofel --- api/v1alpha1/clusterextension_types.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/api/v1alpha1/clusterextension_types.go b/api/v1alpha1/clusterextension_types.go index 37b440d10..ecd5ae23c 100644 --- a/api/v1alpha1/clusterextension_types.go +++ b/api/v1alpha1/clusterextension_types.go @@ -130,7 +130,6 @@ const ( ReasonInstallationFailed = "InstallationFailed" ReasonInstallationStatusUnknown = "InstallationStatusUnknown" - ReasonInstallationSucceeded = "InstallationSucceeded" ReasonResolutionFailed = "ResolutionFailed" ReasonSuccess = "Success" @@ -157,7 +156,6 @@ func init() { ) // TODO(user): add Reasons from above conditionsets.ConditionReasons = append(conditionsets.ConditionReasons, - ReasonInstallationSucceeded, ReasonResolutionFailed, ReasonInstallationFailed, ReasonSuccess, From e0ba7187b37800bbdaa7666c3a94169956d1ebe8 Mon Sep 17 00:00:00 2001 From: Brett Tofel Date: Tue, 2 Jul 2024 08:56:11 -0400 Subject: [PATCH 03/11] Drop usage of ConditionUnknown on Pending Install Signed-off-by: Brett Tofel --- internal/controllers/clusterextension_controller.go | 2 +- internal/controllers/common_controller.go | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/controllers/clusterextension_controller.go b/internal/controllers/clusterextension_controller.go index eaf4c4e9d..d618171bd 100644 --- a/internal/controllers/clusterextension_controller.go +++ b/internal/controllers/clusterextension_controller.go @@ -297,7 +297,7 @@ 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") + setInstalledStatusConditionInstalledFalse(ext, "installation has not been attempted as unpack is pending") return ctrl.Result{}, nil case rukpaksource.StateUnpacked: diff --git a/internal/controllers/common_controller.go b/internal/controllers/common_controller.go index 5900ced30..4d0fc0785 100644 --- a/internal/controllers/common_controller.go +++ b/internal/controllers/common_controller.go @@ -34,12 +34,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(), }) From b032582ee2ebc91fd7d4f155143ea1b9adc73953 Mon Sep 17 00:00:00 2001 From: Brett Tofel Date: Tue, 2 Jul 2024 09:56:54 -0400 Subject: [PATCH 04/11] Remove ReasonUnpackPending/InstallationsStatusUnk Signed-off-by: Brett Tofel --- api/v1alpha1/clusterextension_types.go | 8 ++------ internal/controllers/clusterextension_controller.go | 2 +- internal/controllers/clusterextension_controller_test.go | 2 +- internal/controllers/common_controller.go | 4 ++-- 4 files changed, 6 insertions(+), 10 deletions(-) diff --git a/api/v1alpha1/clusterextension_types.go b/api/v1alpha1/clusterextension_types.go index ecd5ae23c..7c1684493 100644 --- a/api/v1alpha1/clusterextension_types.go +++ b/api/v1alpha1/clusterextension_types.go @@ -128,15 +128,13 @@ const ( ReasonErrorGettingClient = "ErrorGettingClient" ReasonBundleLoadFailed = "BundleLoadFailed" - ReasonInstallationFailed = "InstallationFailed" - ReasonInstallationStatusUnknown = "InstallationStatusUnknown" - ReasonResolutionFailed = "ResolutionFailed" + ReasonInstallationFailed = "InstallationFailed" + ReasonResolutionFailed = "ResolutionFailed" ReasonSuccess = "Success" ReasonDeprecated = "Deprecated" ReasonUpgradeFailed = "UpgradeFailed" - ReasonUnpackPending = "UnpackPending" ReasonUnpackSuccess = "UnpackSuccess" ReasonUnpackFailed = "UnpackFailed" @@ -163,8 +161,6 @@ func init() { ReasonUpgradeFailed, ReasonBundleLoadFailed, ReasonErrorGettingClient, - ReasonInstallationStatusUnknown, - ReasonUnpackPending, ReasonUnpackSuccess, ReasonUnpackFailed, ReasonErrorGettingReleaseState, diff --git a/internal/controllers/clusterextension_controller.go b/internal/controllers/clusterextension_controller.go index d618171bd..4cb589c0a 100644 --- a/internal/controllers/clusterextension_controller.go +++ b/internal/controllers/clusterextension_controller.go @@ -296,7 +296,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp switch unpackResult.State { case rukpaksource.StatePending: - setStatusUnpackPending(ext, unpackResult.Message) + setStatusInstallFalseUnpackFailed(ext, unpackResult.Message) setInstalledStatusConditionInstalledFalse(ext, "installation has not been attempted as unpack is pending") return ctrl.Result{}, nil diff --git a/internal/controllers/clusterextension_controller_test.go b/internal/controllers/clusterextension_controller_test.go index bcf652633..f3a54bb0d 100644 --- 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 index 4d0fc0785..26c90a4c5 100644 --- a/internal/controllers/common_controller.go +++ b/internal/controllers/common_controller.go @@ -110,12 +110,12 @@ 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) { +func setStatusInstallFalseUnpackFailed(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, + Reason: ocv1alpha1.ReasonUnpackFailed, Message: message, ObservedGeneration: ext.GetGeneration(), }) From 8997c0c5d96e971717bbe47adc2b07c07054578c Mon Sep 17 00:00:00 2001 From: Brett Tofel Date: Wed, 3 Jul 2024 07:30:03 -0400 Subject: [PATCH 05/11] Update related TODO in ClusterExtension controller Signed-off-by: Brett Tofel --- internal/controllers/common_controller.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/controllers/common_controller.go b/internal/controllers/common_controller.go index 26c90a4c5..7217ca3c3 100644 --- a/internal/controllers/common_controller.go +++ b/internal/controllers/common_controller.go @@ -109,7 +109,6 @@ func setStatusUnpackFailed(ext *ocv1alpha1.ClusterExtension, message string) { }) } -// TODO: verify if we need to update the installBundle status or leave it as is. func setStatusInstallFalseUnpackFailed(ext *ocv1alpha1.ClusterExtension, message string) { ext.Status.InstalledBundle = nil apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ From 2a20dfac0629af22ab5ac4b252c2488f8bcd0296 Mon Sep 17 00:00:00 2001 From: Brett Tofel Date: Wed, 3 Jul 2024 10:20:28 -0400 Subject: [PATCH 06/11] Rm InstalledStatus->nil on upack & add comment Signed-off-by: Brett Tofel --- api/v1alpha1/clusterextension_types.go | 2 ++ internal/controllers/common_controller.go | 1 - 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/api/v1alpha1/clusterextension_types.go b/api/v1alpha1/clusterextension_types.go index 7c1684493..0096ad101 100644 --- a/api/v1alpha1/clusterextension_types.go +++ b/api/v1alpha1/clusterextension_types.go @@ -173,6 +173,8 @@ type BundleMetadata struct { } // ClusterExtensionStatus defines the observed state of ClusterExtension +// InstalledBundle should only be modified when a new bundle is successfully installed, that way if you've previously successfully installed a bundle before, +// if 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 type ClusterExtensionStatus struct { // +optional InstalledBundle *BundleMetadata `json:"installedBundle,omitempty"` diff --git a/internal/controllers/common_controller.go b/internal/controllers/common_controller.go index 7217ca3c3..4d33f9603 100644 --- a/internal/controllers/common_controller.go +++ b/internal/controllers/common_controller.go @@ -110,7 +110,6 @@ func setStatusUnpackFailed(ext *ocv1alpha1.ClusterExtension, message string) { } func setStatusInstallFalseUnpackFailed(ext *ocv1alpha1.ClusterExtension, message string) { - ext.Status.InstalledBundle = nil apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ Type: ocv1alpha1.TypeUnpacked, Status: metav1.ConditionFalse, From 0cff94c9b680ea47fa67c5bbbaec15f737947113 Mon Sep 17 00:00:00 2001 From: Brett Tofel Date: Wed, 3 Jul 2024 10:50:14 -0400 Subject: [PATCH 07/11] Align CRD for added Go doc on InstalledBundle Signed-off-by: Brett Tofel --- api/v1alpha1/clusterextension_types.go | 5 ++--- .../bases/olm.operatorframework.io_clusterextensions.yaml | 4 +++- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/api/v1alpha1/clusterextension_types.go b/api/v1alpha1/clusterextension_types.go index 0096ad101..46fe48822 100644 --- a/api/v1alpha1/clusterextension_types.go +++ b/api/v1alpha1/clusterextension_types.go @@ -172,9 +172,8 @@ type BundleMetadata struct { Version string `json:"version"` } -// ClusterExtensionStatus defines the observed state of ClusterExtension -// InstalledBundle should only be modified when a new bundle is successfully installed, that way if you've previously successfully installed a bundle before, -// if 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 +// 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. type ClusterExtensionStatus struct { // +optional InstalledBundle *BundleMetadata `json:"installedBundle,omitempty"` diff --git a/config/base/crd/bases/olm.operatorframework.io_clusterextensions.yaml b/config/base/crd/bases/olm.operatorframework.io_clusterextensions.yaml index 9b92a3045..2509f4ff3 100644 --- a/config/base/crd/bases/olm.operatorframework.io_clusterextensions.yaml +++ b/config/base/crd/bases/olm.operatorframework.io_clusterextensions.yaml @@ -113,7 +113,9 @@ spec: - serviceAccount 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: From 938e8658ce7d08922477350cb65130809ad0ae69 Mon Sep 17 00:00:00 2001 From: Brett Tofel Date: Mon, 8 Jul 2024 07:50:33 -0400 Subject: [PATCH 08/11] Move comment on InstalledBundle field Signed-off-by: Brett Tofel --- api/v1alpha1/clusterextension_types.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/api/v1alpha1/clusterextension_types.go b/api/v1alpha1/clusterextension_types.go index 46fe48822..c88cb7add 100644 --- a/api/v1alpha1/clusterextension_types.go +++ b/api/v1alpha1/clusterextension_types.go @@ -173,8 +173,10 @@ type BundleMetadata struct { } // 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. 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 From 4ec2c976e65605e39280d3366b792d0360e9e2de Mon Sep 17 00:00:00 2001 From: Brett Tofel Date: Mon, 8 Jul 2024 08:07:49 -0400 Subject: [PATCH 09/11] Commit manifest changes for godoc change Signed-off-by: Brett Tofel --- .../bases/olm.operatorframework.io_clusterextensions.yaml | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/config/base/crd/bases/olm.operatorframework.io_clusterextensions.yaml b/config/base/crd/bases/olm.operatorframework.io_clusterextensions.yaml index 2509f4ff3..24cdf64af 100644 --- a/config/base/crd/bases/olm.operatorframework.io_clusterextensions.yaml +++ b/config/base/crd/bases/olm.operatorframework.io_clusterextensions.yaml @@ -113,9 +113,7 @@ spec: - serviceAccount type: object status: - 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. + description: ClusterExtensionStatus defines the observed state of ClusterExtension. properties: conditions: items: @@ -190,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 From b7607dff16406936a4cd9abc585b394b9d5c1516 Mon Sep 17 00:00:00 2001 From: Brett Tofel Date: Mon, 8 Jul 2024 14:30:25 -0400 Subject: [PATCH 10/11] Decouple bundle unpacking and installed statuses Signed-off-by: Brett Tofel --- internal/controllers/clusterextension_controller.go | 3 +-- internal/controllers/common_controller.go | 11 ----------- 2 files changed, 1 insertion(+), 13 deletions(-) diff --git a/internal/controllers/clusterextension_controller.go b/internal/controllers/clusterextension_controller.go index 4cb589c0a..13e514b37 100644 --- a/internal/controllers/clusterextension_controller.go +++ b/internal/controllers/clusterextension_controller.go @@ -297,8 +297,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp switch unpackResult.State { case rukpaksource.StatePending: setStatusInstallFalseUnpackFailed(ext, unpackResult.Message) - setInstalledStatusConditionInstalledFalse(ext, "installation has not been attempted as unpack is pending") - + ensureAllConditionsWithReason(ext, ocv1alpha1.ReasonUnpackFailed, "unpack pending") return ctrl.Result{}, nil case rukpaksource.StateUnpacked: setStatusUnpacked(ext, fmt.Sprintf("unpack successful: %v", unpackResult.Message)) diff --git a/internal/controllers/common_controller.go b/internal/controllers/common_controller.go index 4d33f9603..ade40d48a 100644 --- a/internal/controllers/common_controller.go +++ b/internal/controllers/common_controller.go @@ -34,17 +34,6 @@ func setResolvedStatusConditionSuccess(ext *ocv1alpha1.ClusterExtension, message }) } -// 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.ConditionFalse, - Reason: ocv1alpha1.ReasonInstallationFailed, - 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{ From f678a7c04bab72d075cf50bf197a52284ce9ca99 Mon Sep 17 00:00:00 2001 From: Brett Tofel Date: Tue, 9 Jul 2024 04:52:49 -0400 Subject: [PATCH 11/11] Remove unneeded unpack stage helper method Signed-off-by: Brett Tofel --- internal/controllers/clusterextension_controller.go | 2 +- internal/controllers/common_controller.go | 10 ---------- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/internal/controllers/clusterextension_controller.go b/internal/controllers/clusterextension_controller.go index 13e514b37..ef5939867 100644 --- a/internal/controllers/clusterextension_controller.go +++ b/internal/controllers/clusterextension_controller.go @@ -296,7 +296,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp switch unpackResult.State { case rukpaksource.StatePending: - setStatusInstallFalseUnpackFailed(ext, unpackResult.Message) + setStatusUnpackFailed(ext, unpackResult.Message) ensureAllConditionsWithReason(ext, ocv1alpha1.ReasonUnpackFailed, "unpack pending") return ctrl.Result{}, nil case rukpaksource.StateUnpacked: diff --git a/internal/controllers/common_controller.go b/internal/controllers/common_controller.go index ade40d48a..b69c76774 100644 --- a/internal/controllers/common_controller.go +++ b/internal/controllers/common_controller.go @@ -98,16 +98,6 @@ func setStatusUnpackFailed(ext *ocv1alpha1.ClusterExtension, message string) { }) } -func setStatusInstallFalseUnpackFailed(ext *ocv1alpha1.ClusterExtension, message string) { - apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ - Type: ocv1alpha1.TypeUnpacked, - Status: metav1.ConditionFalse, - Reason: ocv1alpha1.ReasonUnpackFailed, - Message: message, - ObservedGeneration: ext.GetGeneration(), - }) -} - func setStatusUnpacked(ext *ocv1alpha1.ClusterExtension, message string) { apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ Type: ocv1alpha1.TypeUnpacked,