From 2d0e624e6c4d0bfd4efa23d615827446d3d9aef3 Mon Sep 17 00:00:00 2001 From: Anik Bhattacharjee Date: Fri, 2 Feb 2024 11:40:15 -0500 Subject: [PATCH 1/2] Clear (existing) error cond from Subscription, once error resolved (#3166) The func `removeSubsCond` takes in a list of pointers to Subscription objects, modifies the objects that the pointers point to, but return a new list of those pointers. A [PR](https://github.com/operator-framework/operator-lifecycle-manager/pull/2942) included in the v0.25.0 release [changed the way the output of that function was being used](https://github.com/operator-framework/operator-lifecycle-manager/pull/2942/files#diff-a1760d9b7ac1e93734eea675d8d8938c96a50e995434b163c6f77c91bace9990R1146-R1155) leading to a regression. This PR fixes the `removeSubsCond` function, fixing the regression as a result. Closes #3162 Signed-off-by: Anik Bhattacharjee Upstream-repository: operator-lifecycle-manager Upstream-commit: 54da66a9996632315827ba37e14823de6405b4d9 --- .../controller/operators/catalog/operator.go | 18 ++---- .../operators/catalog/operator_test.go | 63 ++++++++++++++++--- .../test/e2e/subscription_e2e_test.go | 2 +- .../controller/operators/catalog/operator.go | 18 ++---- 4 files changed, 67 insertions(+), 34 deletions(-) diff --git a/staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go b/staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go index c9176d1ca1..10cb114861 100644 --- a/staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go +++ b/staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go @@ -1158,16 +1158,14 @@ func (o *Operator) syncResolvingNamespace(obj interface{}) error { } // Make sure that we no longer indicate unpacking progress - subs = o.setSubsCond(subs, v1alpha1.SubscriptionCondition{ - Type: v1alpha1.SubscriptionBundleUnpacking, - Status: corev1.ConditionFalse, - }) + o.removeSubsCond(subs, v1alpha1.SubscriptionBundleUnpacking) // Remove BundleUnpackFailed condition from subscriptions o.removeSubsCond(subs, v1alpha1.SubscriptionBundleUnpackFailed) // Remove resolutionfailed condition from subscriptions o.removeSubsCond(subs, v1alpha1.SubscriptionResolutionFailed) + newSub := true for _, updatedSub := range updatedSubs { updatedSub.Status.RemoveConditions(v1alpha1.SubscriptionResolutionFailed) @@ -1444,13 +1442,9 @@ func (o *Operator) setSubsCond(subs []*v1alpha1.Subscription, cond v1alpha1.Subs return subList } -// removeSubsCond will remove the condition to the subscription if it exists -// Only return the list of updated subscriptions -func (o *Operator) removeSubsCond(subs []*v1alpha1.Subscription, condType v1alpha1.SubscriptionConditionType) []*v1alpha1.Subscription { - var ( - lastUpdated = o.now() - ) - var subList []*v1alpha1.Subscription +// removeSubsCond removes the given condition from all of the subscriptions in the input +func (o *Operator) removeSubsCond(subs []*v1alpha1.Subscription, condType v1alpha1.SubscriptionConditionType) { + lastUpdated := o.now() for _, sub := range subs { cond := sub.Status.GetCondition(condType) // if status is ConditionUnknown, the condition doesn't exist. Just skip @@ -1459,9 +1453,7 @@ func (o *Operator) removeSubsCond(subs []*v1alpha1.Subscription, condType v1alph } sub.Status.LastUpdated = lastUpdated sub.Status.RemoveConditions(condType) - subList = append(subList, sub) } - return subList } func (o *Operator) updateSubscriptionStatuses(subs []*v1alpha1.Subscription) ([]*v1alpha1.Subscription, error) { diff --git a/staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator_test.go b/staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator_test.go index 12b96c76c2..2243729865 100644 --- a/staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator_test.go +++ b/staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator_test.go @@ -1253,13 +1253,6 @@ func TestSyncResolvingNamespace(t *testing.T) { Status: v1alpha1.SubscriptionStatus{ CurrentCSV: "", State: "", - Conditions: []v1alpha1.SubscriptionCondition{ - { - Type: v1alpha1.SubscriptionBundleUnpacking, - Status: corev1.ConditionFalse, - }, - }, - LastUpdated: now, }, }, }, @@ -1391,6 +1384,62 @@ func TestSyncResolvingNamespace(t *testing.T) { }, wantErr: fmt.Errorf("some error"), }, + { + name: "HadErrorShouldClearError", + fields: fields{ + clientOptions: []clientfake.Option{clientfake.WithSelfLinks(t)}, + existingOLMObjs: []runtime.Object{ + &v1alpha1.Subscription{ + TypeMeta: metav1.TypeMeta{ + Kind: v1alpha1.SubscriptionKind, + APIVersion: v1alpha1.SchemeGroupVersion.String(), + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "sub", + Namespace: testNamespace, + }, + Spec: &v1alpha1.SubscriptionSpec{ + CatalogSource: "src", + CatalogSourceNamespace: testNamespace, + }, + Status: v1alpha1.SubscriptionStatus{ + InstalledCSV: "sub-csv", + State: "AtLatestKnown", + Conditions: []v1alpha1.SubscriptionCondition{ + { + Type: v1alpha1.SubscriptionResolutionFailed, + Reason: "ConstraintsNotSatisfiable", + Message: "constraints not satisfiable: no operators found from catalog src in namespace testNamespace referenced by subscrition sub, subscription sub exists", + Status: corev1.ConditionTrue, + }, + }, + }, + }, + }, + resolveErr: nil, + }, + wantSubscriptions: []*v1alpha1.Subscription{ + { + TypeMeta: metav1.TypeMeta{ + Kind: v1alpha1.SubscriptionKind, + APIVersion: v1alpha1.SchemeGroupVersion.String(), + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "sub", + Namespace: testNamespace, + }, + Spec: &v1alpha1.SubscriptionSpec{ + CatalogSource: "src", + CatalogSourceNamespace: testNamespace, + }, + Status: v1alpha1.SubscriptionStatus{ + InstalledCSV: "sub-csv", + State: "AtLatestKnown", + LastUpdated: now, + }, + }, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/staging/operator-lifecycle-manager/test/e2e/subscription_e2e_test.go b/staging/operator-lifecycle-manager/test/e2e/subscription_e2e_test.go index f23e5ac303..9a00e30275 100644 --- a/staging/operator-lifecycle-manager/test/e2e/subscription_e2e_test.go +++ b/staging/operator-lifecycle-manager/test/e2e/subscription_e2e_test.go @@ -2456,7 +2456,7 @@ var _ = Describe("Subscription", func() { }, 5*time.Minute, interval, - ).Should(Equal(corev1.ConditionFalse)) + ).Should(Equal(corev1.ConditionUnknown)) By("verifying that the subscription is not reporting unpacking errors") Eventually( diff --git a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go b/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go index c9176d1ca1..10cb114861 100644 --- a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go +++ b/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go @@ -1158,16 +1158,14 @@ func (o *Operator) syncResolvingNamespace(obj interface{}) error { } // Make sure that we no longer indicate unpacking progress - subs = o.setSubsCond(subs, v1alpha1.SubscriptionCondition{ - Type: v1alpha1.SubscriptionBundleUnpacking, - Status: corev1.ConditionFalse, - }) + o.removeSubsCond(subs, v1alpha1.SubscriptionBundleUnpacking) // Remove BundleUnpackFailed condition from subscriptions o.removeSubsCond(subs, v1alpha1.SubscriptionBundleUnpackFailed) // Remove resolutionfailed condition from subscriptions o.removeSubsCond(subs, v1alpha1.SubscriptionResolutionFailed) + newSub := true for _, updatedSub := range updatedSubs { updatedSub.Status.RemoveConditions(v1alpha1.SubscriptionResolutionFailed) @@ -1444,13 +1442,9 @@ func (o *Operator) setSubsCond(subs []*v1alpha1.Subscription, cond v1alpha1.Subs return subList } -// removeSubsCond will remove the condition to the subscription if it exists -// Only return the list of updated subscriptions -func (o *Operator) removeSubsCond(subs []*v1alpha1.Subscription, condType v1alpha1.SubscriptionConditionType) []*v1alpha1.Subscription { - var ( - lastUpdated = o.now() - ) - var subList []*v1alpha1.Subscription +// removeSubsCond removes the given condition from all of the subscriptions in the input +func (o *Operator) removeSubsCond(subs []*v1alpha1.Subscription, condType v1alpha1.SubscriptionConditionType) { + lastUpdated := o.now() for _, sub := range subs { cond := sub.Status.GetCondition(condType) // if status is ConditionUnknown, the condition doesn't exist. Just skip @@ -1459,9 +1453,7 @@ func (o *Operator) removeSubsCond(subs []*v1alpha1.Subscription, condType v1alph } sub.Status.LastUpdated = lastUpdated sub.Status.RemoveConditions(condType) - subList = append(subList, sub) } - return subList } func (o *Operator) updateSubscriptionStatuses(subs []*v1alpha1.Subscription) ([]*v1alpha1.Subscription, error) { From 79fe5018539c0ec558bd8c4f8dacdcc71f513e4d Mon Sep 17 00:00:00 2001 From: Anik Bhattacharjee Date: Mon, 5 Feb 2024 14:20:27 -0500 Subject: [PATCH 2/2] Reflect BundleUnpacking cond removal in missed e2e test (#3170) PR #3166 removes the `BundleUnpacking` condition once resolution is successful. PR #3166 [modified an e2e test](https://github.com/operator-framework/operator-lifecycle-manager/commit/54da66a9996632315827ba37e14823de6405b4d9#diff-11f70fc71ac22d725767916f562789de88d06eb9ebe19f337a59fd7035a3ca2dR2448) to reflect that change. However, the test being fixed in this PR is skipped for upstream, and runs only downstream.This PR is a follow up to #3166 to reflect the `BundleUnpacking` condition removal in the remaining test. Signed-off-by: Anik Bhattacharjee Upstream-repository: operator-lifecycle-manager Upstream-commit: 6e3f051a4d9ac32222a051029570b88c8ba9e4f6 --- .../test/e2e/subscription_e2e_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/staging/operator-lifecycle-manager/test/e2e/subscription_e2e_test.go b/staging/operator-lifecycle-manager/test/e2e/subscription_e2e_test.go index 9a00e30275..0d4d6ac597 100644 --- a/staging/operator-lifecycle-manager/test/e2e/subscription_e2e_test.go +++ b/staging/operator-lifecycle-manager/test/e2e/subscription_e2e_test.go @@ -2672,7 +2672,7 @@ properties: if err != nil { return err } - if cond := fetched.Status.GetCondition(v1alpha1.SubscriptionBundleUnpacking); cond.Status != corev1.ConditionFalse { + if cond := fetched.Status.GetCondition(v1alpha1.SubscriptionBundleUnpacking); cond.Status != corev1.ConditionUnknown { return fmt.Errorf("subscription condition %s has unexpected value %s, expected %s", v1alpha1.SubscriptionBundleUnpacking, cond.Status, corev1.ConditionFalse) } if cond := fetched.Status.GetCondition(v1alpha1.SubscriptionBundleUnpackFailed); cond.Status != corev1.ConditionUnknown {