From 91d2b1491ded9693150bca780fac653976c61b53 Mon Sep 17 00:00:00 2001 From: everettraven Date: Fri, 11 Oct 2024 11:36:39 -0400 Subject: [PATCH 1/3] (bugfix): don't set installed status unless it is successful Signed-off-by: everettraven --- .../clusterextension_controller.go | 8 -- .../clusterextension_controller_test.go | 104 ------------------ test/e2e/cluster_extension_install_test.go | 10 -- 3 files changed, 122 deletions(-) diff --git a/internal/controllers/clusterextension_controller.go b/internal/controllers/clusterextension_controller.go index bb3a1494b..ec025f669 100644 --- a/internal/controllers/clusterextension_controller.go +++ b/internal/controllers/clusterextension_controller.go @@ -205,9 +205,6 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp l.Info("getting installed bundle") installedBundle, err := r.InstalledBundleGetter.GetInstalledBundle(ctx, ext) if err != nil { - setInstallStatus(ext, nil) - // TODO: use Installed=Unknown - setInstalledStatusConditionFailed(ext, err.Error()) setStatusProgressing(ext, err) return ctrl.Result{}, err } @@ -217,7 +214,6 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp resolvedBundle, resolvedBundleVersion, resolvedDeprecation, err := r.Resolver.Resolve(ctx, ext, installedBundle) if err != nil { // Note: We don't distinguish between resolution-specific errors and generic errors - setInstallStatus(ext, nil) setStatusProgressing(ext, err) ensureAllConditionsWithReason(ext, ocv1alpha1.ReasonFailed, err.Error()) return ctrl.Result{}, err @@ -286,10 +282,6 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp managedObjs, _, err := r.Applier.Apply(ctx, unpackResult.Bundle, ext, objLbls, storeLbls) if err != nil { setStatusProgressing(ext, wrapErrorWithResolutionInfo(resolvedBundleMetadata, err)) - // If bundle is not already installed, set Installed status condition to False - if installedBundle == nil { - setInstalledStatusConditionFailed(ext, err.Error()) - } return ctrl.Result{}, err } diff --git a/internal/controllers/clusterextension_controller_test.go b/internal/controllers/clusterextension_controller_test.go index 8f7331384..379492e40 100644 --- a/internal/controllers/clusterextension_controller_test.go +++ b/internal/controllers/clusterextension_controller_test.go @@ -319,110 +319,6 @@ func TestClusterExtensionResolutionAndUnpackSuccessfulApplierFails(t *testing.T) expectedBundleMetadata := ocv1alpha1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"} require.Empty(t, clusterExtension.Status.Install) - t.Log("By checking the expected installed conditions") - installedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled) - require.NotNil(t, installedCond) - require.Equal(t, metav1.ConditionFalse, installedCond.Status) - require.Equal(t, ocv1alpha1.ReasonFailed, installedCond.Reason) - - t.Log("By checking the expected progressing conditions") - progressingCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeProgressing) - require.NotNil(t, progressingCond) - require.Equal(t, metav1.ConditionTrue, progressingCond.Status) - require.Equal(t, ocv1alpha1.ReasonRetrying, progressingCond.Reason) - require.Contains(t, progressingCond.Message, fmt.Sprintf("for resolved bundle %q with version %q", expectedBundleMetadata.Name, expectedBundleMetadata.Version)) - - require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{})) -} - -func TestClusterExtensionApplierFailsWithBundleInstalled(t *testing.T) { - cl, reconciler := newClientAndReconciler(t) - reconciler.Unpacker = &MockUnpacker{ - result: &source.Result{ - State: source.StateUnpacked, - Bundle: fstest.MapFS{}, - }, - } - - ctx := context.Background() - extKey := types.NamespacedName{Name: fmt.Sprintf("cluster-extension-test-%s", rand.String(8))} - - t.Log("When the cluster extension specifies a channel with version that exist") - t.Log("By initializing cluster state") - pkgName := "prometheus" - pkgVer := "1.0.0" - pkgChan := "beta" - namespace := fmt.Sprintf("test-ns-%s", rand.String(8)) - serviceAccount := fmt.Sprintf("test-sa-%s", rand.String(8)) - - clusterExtension := &ocv1alpha1.ClusterExtension{ - ObjectMeta: metav1.ObjectMeta{Name: extKey.Name}, - Spec: ocv1alpha1.ClusterExtensionSpec{ - Source: ocv1alpha1.SourceConfig{ - SourceType: "Catalog", - Catalog: &ocv1alpha1.CatalogSource{ - PackageName: pkgName, - Version: pkgVer, - Channels: []string{pkgChan}, - }, - }, - Install: ocv1alpha1.ClusterExtensionInstallConfig{ - Namespace: namespace, - ServiceAccount: ocv1alpha1.ServiceAccountReference{ - Name: serviceAccount, - }, - }, - }, - } - err := cl.Create(ctx, clusterExtension) - require.NoError(t, err) - - t.Log("It sets resolution success status") - t.Log("By running reconcile") - reconciler.Resolver = resolve.Func(func(_ context.Context, _ *ocv1alpha1.ClusterExtension, _ *ocv1alpha1.BundleMetadata) (*declcfg.Bundle, *bsemver.Version, *declcfg.Deprecation, error) { - v := bsemver.MustParse("1.0.0") - return &declcfg.Bundle{ - Name: "prometheus.v1.0.0", - Package: "prometheus", - Image: "quay.io/operatorhubio/prometheus@fake1.0.0", - }, &v, nil, nil - }) - - reconciler.Manager = &MockManagedContentCacheManager{ - cache: &MockManagedContentCache{}, - } - reconciler.InstalledBundleGetter = &MockInstalledBundleGetter{ - bundle: &ocv1alpha1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"}, - } - reconciler.Applier = &MockApplier{ - objs: []client.Object{}, - } - - res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) - require.Equal(t, ctrl.Result{}, res) - require.NoError(t, err) - - reconciler.Applier = &MockApplier{ - err: errors.New("apply failure"), - } - - res, err = reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) - require.Equal(t, ctrl.Result{}, res) - require.Error(t, err) - - t.Log("By fetching updated cluster extension after reconcile") - require.NoError(t, cl.Get(ctx, extKey, clusterExtension)) - - t.Log("By checking the status fields") - expectedBundleMetadata := ocv1alpha1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"} - require.Equal(t, expectedBundleMetadata, clusterExtension.Status.Install.Bundle) - - t.Log("By checking the expected installed conditions") - installedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled) - require.NotNil(t, installedCond) - require.Equal(t, metav1.ConditionTrue, installedCond.Status) - require.Equal(t, ocv1alpha1.ReasonSucceeded, installedCond.Reason) - t.Log("By checking the expected progressing conditions") progressingCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeProgressing) require.NotNil(t, progressingCond) diff --git a/test/e2e/cluster_extension_install_test.go b/test/e2e/cluster_extension_install_test.go index ed514a70b..8cf09938a 100644 --- a/test/e2e/cluster_extension_install_test.go +++ b/test/e2e/cluster_extension_install_test.go @@ -759,16 +759,6 @@ func TestClusterExtensionRecoversFromInitialInstallFailedWhenFailureFixed(t *tes if assert.NotNil(ct, cond) { assert.Equal(ct, metav1.ConditionTrue, cond.Status) assert.Equal(ct, ocv1alpha1.ReasonRetrying, cond.Reason) - } - }, pollDuration, pollInterval) - - t.Log("By eventually failing to install the package successfully due to insufficient ServiceAccount permissions") - require.EventuallyWithT(t, func(ct *assert.CollectT) { - assert.NoError(ct, c.Get(context.Background(), types.NamespacedName{Name: clusterExtension.Name}, clusterExtension)) - cond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled) - if assert.NotNil(ct, cond) { - assert.Equal(ct, metav1.ConditionFalse, cond.Status) - assert.Equal(ct, ocv1alpha1.ReasonFailed, cond.Reason) assert.Contains(ct, cond.Message, "forbidden") } }, pollDuration, pollInterval) From 3963157fcab83acfb9269f1f3236c9c178755494 Mon Sep 17 00:00:00 2001 From: everettraven Date: Fri, 11 Oct 2024 11:47:29 -0400 Subject: [PATCH 2/3] remove unused code Signed-off-by: everettraven --- internal/controllers/common_controller.go | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/internal/controllers/common_controller.go b/internal/controllers/common_controller.go index d04cd0a01..ed8912853 100644 --- a/internal/controllers/common_controller.go +++ b/internal/controllers/common_controller.go @@ -37,17 +37,6 @@ func setInstalledStatusConditionSuccess(ext *ocv1alpha1.ClusterExtension, messag }) } -// setInstalledStatusConditionFailed sets the installed status condition to failed. -func setInstalledStatusConditionFailed(ext *ocv1alpha1.ClusterExtension, message string) { - apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ - Type: ocv1alpha1.TypeInstalled, - Status: metav1.ConditionFalse, - Reason: ocv1alpha1.ReasonFailed, - Message: message, - ObservedGeneration: ext.GetGeneration(), - }) -} - func setInstallStatus(ext *ocv1alpha1.ClusterExtension, installStatus *ocv1alpha1.ClusterExtensionInstallStatus) { ext.Status.Install = installStatus } From 21b2777f15c626cd88eb2b53cefbf809c2c53c57 Mon Sep 17 00:00:00 2001 From: everettraven Date: Fri, 11 Oct 2024 12:18:19 -0400 Subject: [PATCH 3/3] add unit test for testing all components failing individually during subsequent reconcile Signed-off-by: everettraven --- .../clusterextension_controller_test.go | 176 ++++++++++++++++++ internal/controllers/suite_test.go | 3 +- 2 files changed, 178 insertions(+), 1 deletion(-) diff --git a/internal/controllers/clusterextension_controller_test.go b/internal/controllers/clusterextension_controller_test.go index 379492e40..1532d7e80 100644 --- a/internal/controllers/clusterextension_controller_test.go +++ b/internal/controllers/clusterextension_controller_test.go @@ -682,6 +682,182 @@ func TestClusterExtensionDeleteFinalizerFails(t *testing.T) { require.Contains(t, cond.Message, finalizersMessage) } +func TestClusterExtensionUpgradeFails(t *testing.T) { + cl, reconciler := newClientAndReconciler(t) + reconciler.Unpacker = &MockUnpacker{ + result: &source.Result{ + State: source.StateUnpacked, + Bundle: fstest.MapFS{}, + }, + } + + ctx := context.Background() + extKey := types.NamespacedName{Name: fmt.Sprintf("cluster-extension-test-%s", rand.String(8))} + + t.Log("When the cluster extension specifies a channel with version that exist") + t.Log("By initializing cluster state") + pkgName := "prometheus" + pkgVer := "1.0.0" + pkgChan := "beta" + namespace := fmt.Sprintf("test-ns-%s", rand.String(8)) + serviceAccount := fmt.Sprintf("test-sa-%s", rand.String(8)) + + clusterExtension := &ocv1alpha1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{Name: extKey.Name}, + Spec: ocv1alpha1.ClusterExtensionSpec{ + Source: ocv1alpha1.SourceConfig{ + SourceType: "Catalog", + Catalog: &ocv1alpha1.CatalogSource{ + PackageName: pkgName, + Version: pkgVer, + Channels: []string{pkgChan}, + }, + }, + Install: ocv1alpha1.ClusterExtensionInstallConfig{ + Namespace: namespace, + ServiceAccount: ocv1alpha1.ServiceAccountReference{ + Name: serviceAccount, + }, + }, + }, + } + err := cl.Create(ctx, clusterExtension) + require.NoError(t, err) + + t.Log("It sets resolution success status") + t.Log("By running reconcile") + reconciler.Resolver = resolve.Func(func(_ context.Context, _ *ocv1alpha1.ClusterExtension, _ *ocv1alpha1.BundleMetadata) (*declcfg.Bundle, *bsemver.Version, *declcfg.Deprecation, error) { + v := bsemver.MustParse("1.0.0") + return &declcfg.Bundle{ + Name: "prometheus.v1.0.0", + Package: "prometheus", + Image: "quay.io/operatorhubio/prometheus@fake1.0.0", + }, &v, nil, nil + }) + reconciler.Applier = &MockApplier{ + objs: []client.Object{}, + } + reconciler.Manager = &MockManagedContentCacheManager{ + cache: &MockManagedContentCache{}, + } + res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) + require.Equal(t, ctrl.Result{}, res) + require.NoError(t, err) + + t.Log("By fetching updated cluster extension after reconcile") + require.NoError(t, cl.Get(ctx, extKey, clusterExtension)) + + t.Log("By checking the status fields") + require.Equal(t, ocv1alpha1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"}, clusterExtension.Status.Install.Bundle) + + t.Log("By checking the expected installed conditions") + installedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled) + require.NotNil(t, installedCond) + require.Equal(t, metav1.ConditionTrue, installedCond.Status) + require.Equal(t, ocv1alpha1.ReasonSucceeded, installedCond.Reason) + + t.Log("By checking the expected progressing conditions") + progressingCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeProgressing) + require.NotNil(t, progressingCond) + require.Equal(t, metav1.ConditionFalse, progressingCond.Status) + require.Equal(t, ocv1alpha1.ReasonSucceeded, progressingCond.Reason) + + tcs := []struct { + name string + modification func(*controllers.ClusterExtensionReconciler) + }{ + { + name: "getting installed bundle fails", + modification: func(cer *controllers.ClusterExtensionReconciler) { + cer.InstalledBundleGetter = &MockInstalledBundleGetter{ + bundle: nil, + err: errors.New("boom"), + } + }, + }, + { + name: "resolution fails", + modification: func(cer *controllers.ClusterExtensionReconciler) { + cer.Resolver = resolve.Func(func(_ context.Context, _ *ocv1alpha1.ClusterExtension, _ *ocv1alpha1.BundleMetadata) (*declcfg.Bundle, *bsemver.Version, *declcfg.Deprecation, error) { + return nil, nil, nil, errors.New("boom") + }) + }, + }, + { + name: "unpack fails", + modification: func(cer *controllers.ClusterExtensionReconciler) { + cer.Unpacker = &MockUnpacker{ + err: errors.New("boom"), + } + }, + }, + { + name: "applier fails", + modification: func(cer *controllers.ClusterExtensionReconciler) { + cer.Applier = &MockApplier{ + err: errors.New("boom"), + } + }, + }, + } + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + t.Log("By resetting the reconciler") + reconciler.InstalledBundleGetter = &MockInstalledBundleGetter{ + bundle: &ocv1alpha1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"}, + } + reconciler.Unpacker = &MockUnpacker{ + result: &source.Result{ + State: source.StateUnpacked, + Bundle: fstest.MapFS{}, + }, + } + reconciler.Resolver = resolve.Func(func(_ context.Context, _ *ocv1alpha1.ClusterExtension, _ *ocv1alpha1.BundleMetadata) (*declcfg.Bundle, *bsemver.Version, *declcfg.Deprecation, error) { + v := bsemver.MustParse("1.0.0") + return &declcfg.Bundle{ + Name: "prometheus.v1.0.0", + Package: "prometheus", + Image: "quay.io/operatorhubio/prometheus@fake1.0.0", + }, &v, nil, nil + }) + reconciler.Applier = &MockApplier{ + objs: []client.Object{}, + } + // Here we modify the reconciler to test that a failure of each + // step up until the installation is successful results in no modification + // of the existing Installed condition and associated fields, but does + // result in modification of the Progressing condition to communicate the + // failure. + t.Log("By modifying the reconciler to fail on next reconcile") + tc.modification(reconciler) + + t.Log("By running reconciliation again") + res, err = reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) + require.Equal(t, ctrl.Result{}, res) + require.Error(t, err) + + t.Log("By fetching updated cluster extension after reconcile") + require.NoError(t, cl.Get(ctx, extKey, clusterExtension)) + + t.Log("By checking the status fields") + require.Equal(t, ocv1alpha1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"}, clusterExtension.Status.Install.Bundle) + + t.Log("By checking the expected installed conditions") + installedCond = apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled) + require.NotNil(t, installedCond) + require.Equal(t, metav1.ConditionTrue, installedCond.Status) + require.Equal(t, ocv1alpha1.ReasonSucceeded, installedCond.Reason) + + t.Log("By checking the expected progressing conditions") + progressingCond = apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeProgressing) + require.NotNil(t, progressingCond) + require.Equal(t, metav1.ConditionTrue, progressingCond.Status) + require.Equal(t, ocv1alpha1.ReasonRetrying, progressingCond.Reason) + }) + } + require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{})) +} + func verifyInvariants(ctx context.Context, t *testing.T, c client.Client, ext *ocv1alpha1.ClusterExtension) { key := client.ObjectKeyFromObject(ext) require.NoError(t, c.Get(ctx, key, ext)) diff --git a/internal/controllers/suite_test.go b/internal/controllers/suite_test.go index 9fa70e457..1c6d143e6 100644 --- a/internal/controllers/suite_test.go +++ b/internal/controllers/suite_test.go @@ -74,6 +74,7 @@ func newClient(t *testing.T) client.Client { type MockInstalledBundleGetter struct { bundle *ocv1alpha1.BundleMetadata + err error } func (m *MockInstalledBundleGetter) SetBundle(bundle *ocv1alpha1.BundleMetadata) { @@ -81,7 +82,7 @@ func (m *MockInstalledBundleGetter) SetBundle(bundle *ocv1alpha1.BundleMetadata) } func (m *MockInstalledBundleGetter) GetInstalledBundle(ctx context.Context, ext *ocv1alpha1.ClusterExtension) (*ocv1alpha1.BundleMetadata, error) { - return m.bundle, nil + return m.bundle, m.err } var _ controllers.Applier = (*MockApplier)(nil)