Repository: operator-framework/operator-lifecycle-manager
Pull request #3404 (2024-09-26 12:20:51)

Improve `InstallPlan` error handling
**Description of the change:**

With this change we no set condition on subscriptions about errors during `InstallPlan` creation.

Previously we were only logging these errors.

**Motivation for the change:**

We recently seen failures on `InstallPlan` status update because the size of status exceeded request limit (`InstallPlan` was over 2.1M). A failure like this is currently not visible to the end user and it only possible to see from the OLM debug logs. 

**Architectural changes:**

<!--
If necessary, briefly describe any architectural changes, other options considered, and/or link to any EPs or design docs
-->

**Testing remarks:**

<!--
Call out any information around how you've tested the code change that may be useful for reviewers. For instance:
 * any edge-cases you have (dis)covered
 * how you have reproduced and tested for regressions in bug fixes
 * how you've tested for flakes in e2e tests or flake fixes
-->

**Reviewer Checklist**
- [ ] Implementation matches the proposed design, or proposal is updated to match implementation
- [ ] Sufficient unit test coverage
- [ ] Sufficient end-to-end test coverage
- [ ] Bug fixes are accompanied by regression test(s)
- [ ] e2e tests and flake fixes are accompanied evidence of flake testing, e.g. executing the test 100(0) times
- [ ] tech debt/todo is accompanied by issue link(s) in comments in the surrounding code
- [ ] Tests are comprehensible, e.g. Ginkgo DSL is being used appropriately
- [ ] Docs updated or added to `/doc`
- [ ] Commit messages sensible and descriptive
- [ ] Tests marked as `[FLAKE]` are truly flaky and have an issue
- [ ] Code is properly formatted


<!--

Note: If this PR is fixing an issue make sure to add a note saying:
Closes #<ISSUE_NUMBER>

-->


openshift-ci[bot]: Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with `/test all`
m1kola: Looks like a flake in a merge queue

Patch:
diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go
index e8a3da648..cc0252e4c 100644
--- a/pkg/controller/operators/catalog/operator.go
+++ b/pkg/controller/operators/catalog/operator.go
@@ -1442,7 +1442,20 @@ func (o *Operator) syncResolvingNamespace(obj interface{}) error {
 
 		installPlanReference, err := o.ensureInstallPlan(logger, namespace, maxGeneration+1, subs, installPlanApproval, steps, bundleLookups)
 		if err != nil {
-			logger.WithError(err).Debug("error ensuring installplan")
+			err := fmt.Errorf("error ensuring InstallPlan: %s", err)
+			logger.Infof("%v", err)
+
+			_, updateErr := o.updateSubscriptionStatuses(
+				o.setSubsCond(subs, v1alpha1.SubscriptionCondition{
+					Type:    v1alpha1.SubscriptionBundleUnpackFailed,
+					Reason:  "EnsureInstallPlanFailed",
+					Message: err.Error(),
+					Status:  corev1.ConditionTrue,
+				}))
+			if updateErr != nil {
+				logger.WithError(updateErr).Debug("failed to update subs conditions")
+				return updateErr
+			}
 			return err
 		}
 		updatedSubs = o.setIPReference(updatedSubs, maxGeneration+1, installPlanReference)


Test patch:
diff --git a/pkg/controller/operators/catalog/subscriptions_test.go b/pkg/controller/operators/catalog/subscriptions_test.go
index 079f1cb48..c21dc1a01 100644
--- a/pkg/controller/operators/catalog/subscriptions_test.go
+++ b/pkg/controller/operators/catalog/subscriptions_test.go
@@ -13,6 +13,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
+	clitesting "k8s.io/client-go/testing"
 	utilclocktesting "k8s.io/utils/clock/testing"
 
 	operatorsv1 "github.com/operator-framework/api/pkg/operators/v1"
@@ -570,6 +571,117 @@ func TestSyncSubscriptions(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "NoStatus/NoCurrentCSV/EnsureInstallPlanFailed",
+			fields: fields{
+				clientOptions: []clientfake.Option{
+					clientfake.WithSelfLinks(t),
+					func(c clientfake.ClientsetDecorator) {
+						c.PrependReactor("create", "installplans", func(a clitesting.Action) (bool, runtime.Object, error) {
+							return true, nil, errors.New("fake install plan create error")
+						})
+					},
+				},
+				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{
+							CurrentCSV: "",
+							State:      "",
+						},
+					},
+				},
+				resolveSteps: []*v1alpha1.Step{
+					{
+						Resolving: "csv.v.2",
+						Resource: v1alpha1.StepResource{
+							CatalogSource:          "src",
+							CatalogSourceNamespace: testNamespace,
+							Group:                  v1alpha1.GroupName,
+							Version:                v1alpha1.GroupVersion,
+							Kind:                   v1alpha1.ClusterServiceVersionKind,
+							Name:                   "csv.v.2",
+							Manifest:               "{}",
+						},
+					},
+				},
+				resolveSubs: []*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,
+						},
+					},
+				},
+			},
+			args: args{
+				obj: &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{
+						CurrentCSV: "",
+						State:      "",
+					},
+				},
+			},
+			wantSubscriptions: []*v1alpha1.Subscription{
+				{
+					TypeMeta: metav1.TypeMeta{
+						Kind:       v1alpha1.SubscriptionKind,
+						APIVersion: v1alpha1.SubscriptionCRDAPIVersion,
+					},
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "sub",
+						Namespace: testNamespace,
+					},
+					Spec: &v1alpha1.SubscriptionSpec{
+						CatalogSource:          "src",
+						CatalogSourceNamespace: testNamespace,
+					},
+					Status: v1alpha1.SubscriptionStatus{
+						LastUpdated: now,
+						Conditions: []v1alpha1.SubscriptionCondition{
+							{
+								Type:    v1alpha1.SubscriptionBundleUnpackFailed,
+								Reason:  "EnsureInstallPlanFailed",
+								Message: "error ensuring InstallPlan: fake install plan create error",
+								Status:  corev1.ConditionTrue,
+							},
+						},
+					},
+				},
+			},
+			wantErr: errors.New("error ensuring InstallPlan: fake install plan create error"),
+		},
 		{
 			name: "NoStatus/NoCurrentCSV/BundleLookupError",
 			fields: fields{