chore(v2): refactor defers (#1806)

* chore(v2): refactor defers

* f

* f

* f

Author: emosbaugh

operator/pkg/cli/migratev2/installation.go

@@ -6,6 +6,7 @@ import (
 	"log/slog"
 
 	ecv1beta1 "github.com/replicatedhq/embedded-cluster/kinds/apis/v1beta1"
+	"github.com/replicatedhq/embedded-cluster/pkg/helpers"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/util/retry"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -44,7 +45,8 @@ func setV2MigrationComplete(ctx context.Context, cli client.Client, in *ecv1beta
 func setV2MigrationFailed(ctx context.Context, cli client.Client, in *ecv1beta1.Installation, failure error) error {
 	slog.Info("Setting v2 migration failed")
 
-	err := setV2MigrationInProgressCondition(ctx, cli, in, metav1.ConditionFalse, "MigrationFailed", failure.Error())
+	message := helpers.CleanErrorMessage(failure)
+	err := setV2MigrationInProgressCondition(ctx, cli, in, metav1.ConditionFalse, "MigrationFailed", message)
 	if err != nil {
 		return fmt.Errorf("set v2 migration in progress condition: %w", err)
 	}

operator/pkg/cli/migratev2/migrate.go

@@ -15,7 +15,7 @@ type LogFunc func(string, ...any)
 // Run runs the v1 to v2 migration. It installs the manager service on all nodes, copies the
 // installations to configmaps, enables the v2 admin console, and finally removes the operator
 // chart.
-func Run(ctx context.Context, cli client.Client, in *ecv1beta1.Installation) (err error) {
+func Run(ctx context.Context, cli client.Client, in *ecv1beta1.Installation) error {
 	ok, err := needsMigration(ctx, cli)
 	if err != nil {
 		return fmt.Errorf("check if migration is needed: %w", err)
@@ -31,17 +31,38 @@ func Run(ctx context.Context, cli client.Client, in *ecv1beta1.Installation) (er
 	if err != nil {
 		return fmt.Errorf("set v2 migration in progress: %w", err)
 	}
+
 	defer func() {
-		if err == nil {
-			return
+		if r := recover(); r != nil {
+			err := setV2MigrationFailed(ctx, cli, in, fmt.Errorf("panic: %v", err))
+			if err != nil {
+				slog.Error("Failed to set v2 migration failed", "error", err)
+			}
+			panic(r)
 		}
+	}()
+
+	err = runMigration(ctx, cli)
+	if err != nil {
 		if err := setV2MigrationFailed(ctx, cli, in, err); err != nil {
 			slog.Error("Failed to set v2 migration failed", "error", err)
 		}
-	}()
+		return err
+	}
 
+	err = setV2MigrationComplete(ctx, cli, in)
+	if err != nil {
+		return fmt.Errorf("set v2 migration complete: %w", err)
+	}
+
+	slog.Info("Successfully migrated from v2")
+
+	return nil
+}
+
+func runMigration(ctx context.Context, cli client.Client) error {
 	// scale down the operator to ensure that it does not reconcile and revert our changes.
-	err = scaleDownOperator(ctx, cli)
+	err := scaleDownOperator(ctx, cli)
 	if err != nil {
 		return fmt.Errorf("disable operator: %w", err)
 	}
@@ -51,13 +72,6 @@ func Run(ctx context.Context, cli client.Client, in *ecv1beta1.Installation) (er
 		return fmt.Errorf("cleanup k0s: %w", err)
 	}
 
-	err = setV2MigrationComplete(ctx, cli, in)
-	if err != nil {
-		return fmt.Errorf("set v2 migration complete: %w", err)
-	}
-
-	slog.Info("Successfully migrated from v2")
-
 	return nil
 }
 

operator/pkg/upgrade/upgrade.go

@@ -208,7 +208,7 @@ func updateClusterConfig(ctx context.Context, cli client.Client) error {
 	return nil
 }
 
-func upgradeAddons(ctx context.Context, cli client.Client, hcli helm.Client, in *ecv1beta1.Installation) (finalErr error) {
+func upgradeAddons(ctx context.Context, cli client.Client, hcli helm.Client, in *ecv1beta1.Installation) error {
 	err := k8sutil.SetInstallationState(ctx, cli, in.Name, v1beta1.InstallationStateAddonsInstalling, "Upgrading addons")
 	if err != nil {
 		return fmt.Errorf("set installation state: %w", err)

pkg/addons2/upgrade.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"fmt"
 	"log/slog"
-	"runtime/debug"
 
 	"github.com/pkg/errors"
 	ecv1beta1 "github.com/replicatedhq/embedded-cluster/kinds/apis/v1beta1"
@@ -36,7 +35,7 @@ func Upgrade(ctx context.Context, hcli helm.Client, in *ecv1beta1.Installation,
 	}
 	for _, addon := range addons {
 		if err := upgradeAddOn(ctx, hcli, kcli, in, addon); err != nil {
-			return err
+			return errors.Wrapf(err, "addon %s", addon.Name())
 		}
 	}
 
@@ -105,7 +104,7 @@ func getAddOnsForUpgrade(in *ecv1beta1.Installation, meta *ectypes.ReleaseMetada
 	return addOns, nil
 }
 
-func upgradeAddOn(ctx context.Context, hcli helm.Client, kcli client.Client, in *ecv1beta1.Installation, addon types.AddOn) (finalErr error) {
+func upgradeAddOn(ctx context.Context, hcli helm.Client, kcli client.Client, in *ecv1beta1.Installation, addon types.AddOn) error {
 	// check if we already processed this addon
 	conditionStatus, err := k8sutil.GetConditionStatus(ctx, kcli, in.Name, conditionName(addon))
 	if err != nil {
@@ -123,31 +122,21 @@ func upgradeAddOn(ctx context.Context, hcli helm.Client, kcli client.Client, in
 		return errors.Wrap(err, "failed to set condition status")
 	}
 
-	defer func() {
-		if r := recover(); r != nil {
-			finalErr = fmt.Errorf("upgrading %s recovered from panic: %v: %s", addon.Name(), r, string(debug.Stack()))
-		}
-
-		status := metav1.ConditionTrue
-		reason := "Upgraded"
-		message := ""
-
-		if finalErr != nil {
-			status = metav1.ConditionFalse
-			reason = "UpgradeFailed"
-			message = helpers.CleanErrorMessage(finalErr)
-		}
-
-		if err := setCondition(ctx, kcli, in, conditionName(addon), status, reason, message); err != nil {
-			slog.Error("Failed to set condition status", "error", err)
-		}
-	}()
-
 	// TODO (@salah): add support for end user overrides
 	overrides := addOnOverrides(addon, in.Spec.Config, nil)
 
-	if err := addon.Upgrade(ctx, kcli, hcli, overrides); err != nil {
-		return errors.Wrap(err, addon.Name())
+	err = addon.Upgrade(ctx, kcli, hcli, overrides)
+	if err != nil {
+		message := helpers.CleanErrorMessage(err)
+		if err := setCondition(ctx, kcli, in, conditionName(addon), metav1.ConditionFalse, "UpgradeFailed", message); err != nil {
+			slog.Error("Failed to set condition upgrade failed", "error", err)
+		}
+		return errors.Wrap(err, "upgrade addon")
+	}
+
+	err = setCondition(ctx, kcli, in, conditionName(addon), metav1.ConditionTrue, "Upgraded", "")
+	if err != nil {
+		return errors.Wrap(err, "set condition upgrade succeeded")
 	}
 
 	slog.Info(addon.Name() + " is ready!")

pkg/extensions/upgrade.go

@@ -2,9 +2,7 @@ package extensions
 
 import (
 	"context"
-	"fmt"
 	"log/slog"
-	"runtime/debug"
 
 	"github.com/pkg/errors"
 	ecv1beta1 "github.com/replicatedhq/embedded-cluster/kinds/apis/v1beta1"
@@ -76,7 +74,7 @@ func Upgrade(ctx context.Context, kcli client.Client, hcli helm.Client, prev *ec
 	return nil
 }
 
-func handleExtensionInstall(ctx context.Context, kcli client.Client, hcli helm.Client, in *ecv1beta1.Installation, ext ecv1beta1.Chart) (finalErr error) {
+func handleExtensionInstall(ctx context.Context, kcli client.Client, hcli helm.Client, in *ecv1beta1.Installation, ext ecv1beta1.Chart) error {
 	return handleExtension(ctx, kcli, in, ext, actionInstall, func() error {
 		exists, err := hcli.ReleaseExists(ctx, ext.TargetNS, ext.Name)
 		if err != nil {
@@ -93,7 +91,7 @@ func handleExtensionInstall(ctx context.Context, kcli client.Client, hcli helm.C
 	})
 }
 
-func handleExtensionUpgrade(ctx context.Context, kcli client.Client, hcli helm.Client, in *ecv1beta1.Installation, ext ecv1beta1.Chart) (finalErr error) {
+func handleExtensionUpgrade(ctx context.Context, kcli client.Client, hcli helm.Client, in *ecv1beta1.Installation, ext ecv1beta1.Chart) error {
 	return handleExtension(ctx, kcli, in, ext, actionUpgrade, func() error {
 		if err := upgrade(ctx, hcli, ext); err != nil {
 			return errors.Wrap(err, "upgrade")
@@ -109,7 +107,7 @@ func handleExtensionNoop(ctx context.Context, kcli client.Client, in *ecv1beta1.
 	})
 }
 
-func handleExtensionUninstall(ctx context.Context, kcli client.Client, hcli helm.Client, in *ecv1beta1.Installation, ext ecv1beta1.Chart) (finalErr error) {
+func handleExtensionUninstall(ctx context.Context, kcli client.Client, hcli helm.Client, in *ecv1beta1.Installation, ext ecv1beta1.Chart) error {
 	return handleExtension(ctx, kcli, in, ext, actionUninstall, func() error {
 		exists, err := hcli.ReleaseExists(ctx, ext.TargetNS, ext.Name)
 		if err != nil {
@@ -126,7 +124,7 @@ func handleExtensionUninstall(ctx context.Context, kcli client.Client, hcli helm
 	})
 }
 
-func handleExtension(ctx context.Context, kcli client.Client, in *ecv1beta1.Installation, ext ecv1beta1.Chart, action helmAction, processFn func() error) (finalErr error) {
+func handleExtension(ctx context.Context, kcli client.Client, in *ecv1beta1.Installation, ext ecv1beta1.Chart, action helmAction, processFn func() error) error {
 	slogArgs := slogArgs(ext, action)
 
 	processed, err := extensionAlreadyProcessed(ctx, kcli, in, ext)
@@ -146,26 +144,19 @@ func handleExtension(ctx context.Context, kcli client.Client, in *ecv1beta1.Inst
 		}
 	}
 
-	defer func() {
-		if r := recover(); r != nil {
-			actionIng, _ := formatAction(action)
-			finalErr = fmt.Errorf("%s %s recovered from panic: %v: %s", actionIng, ext.Name, r, string(debug.Stack()))
-		}
-
-		err := markExtensionProcessed(ctx, kcli, in, ext, action, finalErr)
-		if err != nil {
-			if finalErr == nil {
-				finalErr = errors.Wrap(err, "mark extension as processed")
-			} else {
-				slog.Error("Failed to mark extension as processed", append(slogArgs, "error", err)...)
-			}
+	err = processFn()
+	if err != nil {
+		if err := markExtensionAsFailed(ctx, kcli, in, ext, action, err); err != nil {
+			slog.Error("Failed to mark extension as failed", append(slogArgs, "error", err)...)
 		}
-	}()
-
-	if err := processFn(); err != nil {
 		return errors.Wrap(err, "process extension")
 	}
 
+	err = markExtensionAsProcessed(ctx, kcli, in, ext, action)
+	if err != nil {
+		return errors.Wrap(err, "mark extension as processed")
+	}
+
 	slog.Info("Extension is ready!", slogArgs...)
 
 	return nil
@@ -187,23 +178,20 @@ func markExtensionAsProcessing(ctx context.Context, kcli client.Client, in *ecv1
 	return nil
 }
 
-func markExtensionProcessed(ctx context.Context, kcli client.Client, in *ecv1beta1.Installation, ext ecv1beta1.Chart, action helmAction, finalErr error) error {
+func markExtensionAsProcessed(ctx context.Context, kcli client.Client, in *ecv1beta1.Installation, ext ecv1beta1.Chart, action helmAction) error {
 	_, actionEd := formatAction(action)
-
-	status := metav1.ConditionTrue
-	reason := actionEd
-	message := ""
-
-	if finalErr != nil {
-		status = metav1.ConditionFalse
-		reason = string(action) + "Failed"
-		message = helpers.CleanErrorMessage(finalErr)
+	if err := setCondition(ctx, kcli, in, conditionName(ext), metav1.ConditionTrue, actionEd, ""); err != nil {
+		return errors.Wrap(err, "failed to set condition status")
 	}
+	return nil
+}
 
-	if err := setCondition(ctx, kcli, in, conditionName(ext), status, reason, message); err != nil {
+func markExtensionAsFailed(ctx context.Context, kcli client.Client, in *ecv1beta1.Installation, ext ecv1beta1.Chart, action helmAction, finalErr error) error {
+	reason := string(action) + "Failed"
+	message := helpers.CleanErrorMessage(finalErr)
+	if err := setCondition(ctx, kcli, in, conditionName(ext), metav1.ConditionFalse, reason, message); err != nil {
 		return errors.Wrap(err, "failed to set condition status")
 	}
-
 	return nil
 }