move pending release handling out of getReleaseState

Author: azych

internal/operator-controller/applier/helm.go

@@ -39,6 +39,11 @@ const (
 	maxHelmReleaseHistory        = 10
 )
 
+var (
+	errPendingRelease                = errors.New("release is in a pending state")
+	errBundleToHelmChartConverterNil = errors.New("BundleToHelmChartConverter is nil")
+)
+
 // Preflight is a check that should be run before making any changes to the cluster
 type Preflight interface {
 	// Install runs checks that should be successful prior
@@ -144,6 +149,23 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte
 	}
 
 	rel, desiredRel, state, err := h.getReleaseState(ctx, ac, ext, chrt, values, post)
+	// if a release is pending, that means that a helm action
+	// (installation/upgrade) we were attempting was likely interrupted in-flight.
+	// Pending release would leave us in reconciliation error loop because helm
+	// wouldn't be able to progress automatically from it.
+	//
+	// one of the workarounds is to try and remove helm metadata relating to
+	// that pending release which should 'reset' its state communicated to helm
+	// and the next reconciliation should be able to successfully pick up from here
+	// for context see: https://github.com/helm/helm/issues/5595 and https://github.com/helm/helm/issues/7476
+	// and the discussion in https://github.com/operator-framework/operator-controller/pull/1776
+	if errors.Is(err, errPendingRelease) {
+		if _, err := ac.Config().Releases.Delete(rel.Name, rel.Version); err != nil {
+			return nil, "", fmt.Errorf("failed removing pending release %q version %d metadata: %w", rel.Name, rel.Version, err)
+		}
+		// return an error to try to detect proper state (installation/upgrade) at next reconciliation
+		return nil, "", fmt.Errorf("removed pending release %q version %d metadata", rel.Name, rel.Version)
+	}
 	if err != nil {
 		return nil, "", fmt.Errorf("failed to get release state using server-side dry-run: %w", err)
 	}
@@ -203,7 +225,7 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte
 
 func (h *Helm) buildHelmChart(bundleFS fs.FS, ext *ocv1.ClusterExtension) (*chart.Chart, error) {
 	if h.BundleToHelmChartConverter == nil {
-		return nil, errors.New("BundleToHelmChartConverter is nil")
+		return nil, errBundleToHelmChartConverterNil
 	}
 	watchNamespace, err := GetWatchNamespace(ext)
 	if err != nil {
@@ -244,24 +266,6 @@ func (h *Helm) getReleaseState(ctx context.Context, cl helmclient.ActionInterfac
 	logger := log.FromContext(ctx)
 	currentRelease, err := cl.Get(ext.GetName())
 
-	// if a release is pending at this point, that means that a helm action
-	// (installation/upgrade) we were attempting was likely interrupted in-flight.
-	// Pending release would leave us in reconciliation error loop because helm
-	// wouldn't be able to progress automatically from it.
-	//
-	// one of the workarounds is to try and remove helm metadata relating to
-	// that pending release which should 'reset' its state communicated to helm
-	// and the next reconciliation should be able to successfully pick up from here
-	// for context see: https://github.com/helm/helm/issues/5595 and https://github.com/helm/helm/issues/7476
-	// and the discussion in https://github.com/operator-framework/operator-controller/pull/1776
-	if err == nil && currentRelease.Info.Status.IsPending() {
-		if _, err = cl.Config().Releases.Delete(currentRelease.Name, currentRelease.Version); err != nil {
-			return nil, nil, StateError, fmt.Errorf("failed removing interrupted release %q version %d metadata: %w", currentRelease.Name, currentRelease.Version, err)
-		}
-		// return error to try to detect proper state (installation/upgrade) at next reconciliation
-		return nil, nil, StateError, fmt.Errorf("removed interrupted release %q version %d metadata", currentRelease.Name, currentRelease.Version)
-	}
-
 	if errors.Is(err, driver.ErrReleaseNotFound) {
 		logger.V(4).Info("ClusterExtension dry-run install", "extension", ext.GetName())
 		desiredRelease, err := cl.Install(ext.GetName(), ext.Spec.Namespace, chrt, values, func(i *action.Install) error {
@@ -277,6 +281,9 @@ func (h *Helm) getReleaseState(ctx context.Context, cl helmclient.ActionInterfac
 	if err != nil {
 		return nil, nil, StateError, err
 	}
+	if currentRelease.Info.Status.IsPending() {
+		return currentRelease, nil, StateError, errPendingRelease
+	}
 
 	desiredRelease, err := cl.Upgrade(ext.GetName(), ext.Spec.Namespace, chrt, values, func(upgrade *action.Upgrade) error {
 		logger.V(4).Info("ClusterExtension dry-run upgrade", "extension", ext.GetName())

internal/operator-controller/applier/helm_test.go

@@ -229,8 +229,8 @@ func TestApply_Base(t *testing.T) {
 	})
 }
 
-func TestApply_InterruptedRelease(t *testing.T) {
-	t.Run("fails removing an interrupted install release", func(t *testing.T) {
+func TestApply_PendingRelease(t *testing.T) {
+	t.Run("fails removing a pending install release", func(t *testing.T) {
 		testRel := &release.Release{Name: "testrel", Version: 0, Info: &release.Info{Status: release.StatusPendingInstall}}
 		testStorage := storage.Init(driver.NewMemory())
 
@@ -242,12 +242,12 @@ func TestApply_InterruptedRelease(t *testing.T) {
 
 		objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
 		require.Error(t, err)
-		require.ErrorContains(t, err, "removing interrupted release")
+		require.ErrorContains(t, err, "removing pending release")
 		require.Nil(t, objs)
 		require.Empty(t, state)
 	})
 
-	t.Run("fails removing an interrupted upgrade release", func(t *testing.T) {
+	t.Run("fails removing a pending upgrade release", func(t *testing.T) {
 		testRel := &release.Release{Name: "testrel", Version: 0, Info: &release.Info{Status: release.StatusPendingUpgrade}}
 		testStorage := storage.Init(driver.NewMemory())
 
@@ -259,12 +259,12 @@ func TestApply_InterruptedRelease(t *testing.T) {
 
 		objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
 		require.Error(t, err)
-		require.ErrorContains(t, err, "removing interrupted release")
+		require.ErrorContains(t, err, "removing pending release")
 		require.Nil(t, objs)
 		require.Empty(t, state)
 	})
 
-	t.Run("successfully removes an interrupted install release", func(t *testing.T) {
+	t.Run("successfully removes a pending install release", func(t *testing.T) {
 		testRel := &release.Release{Name: "testrel", Version: 0, Info: &release.Info{Status: release.StatusPendingInstall}}
 		testStorage := storage.Init(driver.NewMemory())
 		err := testStorage.Create(testRel)
@@ -278,12 +278,12 @@ func TestApply_InterruptedRelease(t *testing.T) {
 
 		objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
 		require.Error(t, err)
-		require.ErrorContains(t, err, "removed interrupted release")
+		require.ErrorContains(t, err, "removed pending release")
 		require.Nil(t, objs)
 		require.Empty(t, state)
 	})
 
-	t.Run("successfully removes an interrupted upgrade release", func(t *testing.T) {
+	t.Run("successfully removes an pending upgrade release", func(t *testing.T) {
 		testRel := &release.Release{Name: "testrel", Version: 0, Info: &release.Info{Status: release.StatusPendingUpgrade}}
 		testStorage := storage.Init(driver.NewMemory())
 		err := testStorage.Create(testRel)
@@ -297,7 +297,7 @@ func TestApply_InterruptedRelease(t *testing.T) {
 
 		objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
 		require.Error(t, err)
-		require.ErrorContains(t, err, "removed interrupted release")
+		require.ErrorContains(t, err, "removed pending release")
 		require.Nil(t, objs)
 		require.Empty(t, state)
 	})