chore(v3): add upgrade reporting (#3046)

emosbaugh · web-flow · commit 057c007679c3 · 2025-10-14T07:31:07.000-07:00
* chore(v3): add upgrade reporting

* tests
diff --git a/api/controllers/linux/upgrade/controller.go b/api/controllers/linux/upgrade/controller.go
@@ -53,6 +53,8 @@ type UpgradeController struct {
 	stateMachine         statemachine.Interface
 	requiresInfraUpgrade bool
 	logger               logrus.FieldLogger
+	targetVersion        string
+	initialVersion       string
 	// App controller composition
 	*appcontroller.AppController
 }
@@ -167,6 +169,18 @@ func WithEndUserConfig(endUserConfig *ecv1beta1.Config) UpgradeControllerOption
 	}
 }
 
+func WithTargetVersion(targetVersion string) UpgradeControllerOption {
+	return func(c *UpgradeController) {
+		c.targetVersion = targetVersion
+	}
+}
+
+func WithInitialVersion(initialVersion string) UpgradeControllerOption {
+	return func(c *UpgradeController) {
+		c.initialVersion = initialVersion
+	}
+}
+
 func NewUpgradeController(opts ...UpgradeControllerOption) (*UpgradeController, error) {
 	controller := &UpgradeController{
 		store:  store.NewMemoryStore(),
@@ -252,6 +266,8 @@ func NewUpgradeController(opts ...UpgradeControllerOption) (*UpgradeController,
 		controller.AppController = appController
 	}
 
+	controller.registerReportingHandlers()
+
 	return controller, nil
 }
 
diff --git a/api/controllers/linux/upgrade/controller_test.go b/api/controllers/linux/upgrade/controller_test.go
@@ -14,6 +14,7 @@ import (
 	"github.com/replicatedhq/embedded-cluster/api/internal/store"
 	"github.com/replicatedhq/embedded-cluster/api/types"
 	ecv1beta1 "github.com/replicatedhq/embedded-cluster/kinds/apis/v1beta1"
+	"github.com/replicatedhq/embedded-cluster/pkg/metrics"
 	"github.com/replicatedhq/embedded-cluster/pkg/release"
 	"github.com/replicatedhq/embedded-cluster/pkg/runtimeconfig"
 	kotsv1beta1 "github.com/replicatedhq/kotskinds/apis/kots/v1beta1"
@@ -271,6 +272,171 @@ func TestGetInfra(t *testing.T) {
 	}
 }
 
+func TestReportingHandlers(t *testing.T) {
+	tests := []struct {
+		name                 string
+		currentState         statemachine.State
+		targetState          statemachine.State
+		requiresInfraUpgrade bool
+		targetVersion        string
+		initialVersion       string
+		setupMocks           func(*metrics.MockReporter, *store.MockStore)
+	}{
+		{
+			name:                 "report upgrade succeeded",
+			currentState:         states.StateAppUpgrading,
+			targetState:          states.StateSucceeded,
+			requiresInfraUpgrade: false,
+			targetVersion:        "1.0.0",
+			initialVersion:       "0.9.0",
+			setupMocks: func(mr *metrics.MockReporter, st *store.MockStore) {
+				mr.On("ReportUpgradeSucceeded", mock.Anything, "1.0.0", "0.9.0")
+			},
+		},
+		{
+			name:                 "report infrastructure upgrade failed",
+			currentState:         states.StateInfrastructureUpgrading,
+			targetState:          states.StateInfrastructureUpgradeFailed,
+			requiresInfraUpgrade: true,
+			targetVersion:        "1.0.0",
+			initialVersion:       "0.9.0",
+			setupMocks: func(mr *metrics.MockReporter, st *store.MockStore) {
+				st.LinuxInfraMockStore.On("GetStatus").Return(types.Status{
+					State:       types.StateFailed,
+					Description: "infrastructure upgrade failed",
+				}, nil)
+				mr.On("ReportUpgradeFailed", mock.Anything, mock.MatchedBy(func(err error) bool {
+					return err.Error() == "infrastructure upgrade failed"
+				}), "1.0.0", "0.9.0")
+			},
+		},
+		{
+			name:                 "report app upgrade failed",
+			currentState:         states.StateAppUpgrading,
+			targetState:          states.StateAppUpgradeFailed,
+			requiresInfraUpgrade: false,
+			targetVersion:        "1.0.0",
+			initialVersion:       "0.9.0",
+			setupMocks: func(mr *metrics.MockReporter, st *store.MockStore) {
+				st.AppUpgradeMockStore.On("GetStatus").Return(types.Status{
+					State:       types.StateFailed,
+					Description: "app upgrade failed",
+				}, nil)
+				mr.On("ReportUpgradeFailed", mock.Anything, mock.MatchedBy(func(err error) bool {
+					return err.Error() == "app upgrade failed"
+				}), "1.0.0", "0.9.0")
+			},
+		},
+		{
+			name:                 "report app preflights succeeded",
+			currentState:         states.StateAppPreflightsRunning,
+			targetState:          states.StateAppPreflightsSucceeded,
+			requiresInfraUpgrade: false,
+			setupMocks: func(mr *metrics.MockReporter, st *store.MockStore) {
+				mr.On("ReportAppPreflightsSucceeded", mock.Anything)
+			},
+		},
+		{
+			name:                 "report app preflights failed",
+			currentState:         states.StateAppPreflightsRunning,
+			targetState:          states.StateAppPreflightsFailed,
+			requiresInfraUpgrade: false,
+			setupMocks: func(mr *metrics.MockReporter, st *store.MockStore) {
+				output := &types.PreflightsOutput{
+					Fail: []types.PreflightsRecord{
+						{
+							Title:   "Test Check",
+							Message: "Test check failed",
+							Strict:  true,
+						},
+					},
+				}
+				st.AppPreflightMockStore.On("GetOutput").Return(output, nil)
+				mr.On("ReportAppPreflightsFailed", mock.Anything, output)
+			},
+		},
+		{
+			name:                 "report app preflights bypassed",
+			currentState:         states.StateAppPreflightsFailed,
+			targetState:          states.StateAppPreflightsFailedBypassed,
+			requiresInfraUpgrade: false,
+			setupMocks: func(mr *metrics.MockReporter, st *store.MockStore) {
+				output := &types.PreflightsOutput{
+					Fail: []types.PreflightsRecord{
+						{
+							Title:   "Non-strict Check",
+							Message: "Test check failed but can be bypassed",
+							Strict:  false,
+						},
+					},
+				}
+				st.AppPreflightMockStore.On("GetOutput").Return(output, nil)
+				mr.On("ReportAppPreflightsBypassed", mock.Anything, output)
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			mockMetricsReporter := &metrics.MockReporter{}
+			mockStore := &store.MockStore{}
+			mockInfraManager := &infra.MockInfraManager{}
+
+			tt.setupMocks(mockMetricsReporter, mockStore)
+
+			// Mock RequiresUpgrade which is called during controller initialization
+			mockInfraManager.On("RequiresUpgrade", mock.Anything, mock.Anything).Return(tt.requiresInfraUpgrade, nil)
+
+			// Create state machine starting in the current state
+			sm := NewStateMachine(
+				WithCurrentState(tt.currentState),
+				WithRequiresInfraUpgrade(tt.requiresInfraUpgrade),
+			)
+
+			// Create app controller (required for upgrade controller)
+			appController, err := appcontroller.NewAppController(
+				appcontroller.WithStateMachine(sm),
+				appcontroller.WithStore(mockStore),
+				appcontroller.WithReleaseData(getTestReleaseData(&kotsv1beta1.Config{})),
+			)
+			require.NoError(t, err)
+
+			// Create upgrade controller with metrics reporter
+			controller, err := NewUpgradeController(
+				WithStateMachine(sm),
+				WithAppController(appController),
+				WithMetricsReporter(mockMetricsReporter),
+				WithStore(mockStore),
+				WithInfraManager(mockInfraManager),
+				WithTargetVersion(tt.targetVersion),
+				WithInitialVersion(tt.initialVersion),
+				WithReleaseData(getTestReleaseData(&kotsv1beta1.Config{})),
+			)
+			require.NoError(t, err)
+
+			// Trigger the state transition
+			lock, err := sm.AcquireLock()
+			require.NoError(t, err)
+			defer lock.Release()
+
+			err = sm.Transition(lock, tt.targetState)
+			require.NoError(t, err)
+
+			// Wait for the event handler goroutine to complete
+			time.Sleep(1 * time.Second)
+
+			// Verify that the metrics reporter was called as expected
+			mockMetricsReporter.AssertExpectations(t)
+			mockStore.LinuxInfraMockStore.AssertExpectations(t)
+			mockStore.AppUpgradeMockStore.AssertExpectations(t)
+			mockStore.AppPreflightMockStore.AssertExpectations(t)
+
+			// Avoid unused variable error
+			_ = controller
+		})
+	}
+}
+
 func getTestReleaseData(appConfig *kotsv1beta1.Config) *release.ReleaseData {
 	return &release.ReleaseData{
 		EmbeddedClusterConfig: &ecv1beta1.Config{},
diff --git a/api/controllers/linux/upgrade/reporting_handlers.go b/api/controllers/linux/upgrade/reporting_handlers.go
@@ -0,0 +1,79 @@
+package upgrade
+
+import (
+	"context"
+	"errors"
+	"fmt"
+
+	"github.com/replicatedhq/embedded-cluster/api/internal/statemachine"
+	"github.com/replicatedhq/embedded-cluster/api/internal/states"
+	"github.com/replicatedhq/embedded-cluster/api/types"
+)
+
+func (c *UpgradeController) registerReportingHandlers() {
+	c.stateMachine.RegisterEventHandler(states.StateInfrastructureUpgradeFailed, c.reportUpgradeFailed)
+	c.stateMachine.RegisterEventHandler(states.StateAppUpgradeFailed, c.reportUpgradeFailed)
+	c.stateMachine.RegisterEventHandler(states.StateSucceeded, c.reportUpgradeSucceeded)
+
+	// report preflight failures and bypassed
+	c.stateMachine.RegisterEventHandler(states.StateAppPreflightsFailed, c.reportAppPreflightsFailed)
+	c.stateMachine.RegisterEventHandler(states.StateAppPreflightsFailedBypassed, c.reportAppPreflightsBypassed)
+	c.stateMachine.RegisterEventHandler(states.StateAppPreflightsSucceeded, c.reportAppPreflightsSucceeded)
+}
+
+func (c *UpgradeController) reportUpgradeFailed(ctx context.Context, _, toState statemachine.State) {
+	var status types.Status
+	var err error
+
+	switch toState {
+	case states.StateInfrastructureUpgradeFailed:
+		status, err = c.store.LinuxInfraStore().GetStatus()
+		if err != nil {
+			err = fmt.Errorf("get status from infra store: %w", err)
+		}
+	case states.StateAppUpgradeFailed:
+		status, err = c.store.AppUpgradeStore().GetStatus()
+		if err != nil {
+			err = fmt.Errorf("get status from app upgrade store: %w", err)
+		}
+	}
+	if err != nil {
+		c.logger.WithError(err).Error("failed to report failed upgrade")
+		return
+	}
+
+	c.logger.Info("Reporting metrics event upgrade failed")
+	c.metricsReporter.ReportUpgradeFailed(ctx, errors.New(status.Description), c.targetVersion, c.initialVersion)
+}
+
+func (c *UpgradeController) reportUpgradeSucceeded(ctx context.Context, _, _ statemachine.State) {
+	c.logger.Info("Reporting metrics event upgrade succeeded")
+	c.metricsReporter.ReportUpgradeSucceeded(ctx, c.targetVersion, c.initialVersion)
+}
+
+func (c *UpgradeController) reportAppPreflightsFailed(ctx context.Context, _, _ statemachine.State) {
+	output, err := c.store.AppPreflightStore().GetOutput()
+	if err != nil {
+		err = fmt.Errorf("get output from app preflight store: %w", err)
+		c.logger.WithError(err).Error("failed to report app preflights failed")
+		return
+	}
+	c.logger.Info("Reporting metrics event app preflights failed")
+	c.metricsReporter.ReportAppPreflightsFailed(ctx, output)
+}
+
+func (c *UpgradeController) reportAppPreflightsBypassed(ctx context.Context, _, _ statemachine.State) {
+	output, err := c.store.AppPreflightStore().GetOutput()
+	if err != nil {
+		err = fmt.Errorf("get output from app preflight store: %w", err)
+		c.logger.WithError(err).Error("failed to report app preflights bypassed")
+		return
+	}
+	c.logger.Info("Reporting metrics event app preflights bypassed")
+	c.metricsReporter.ReportAppPreflightsBypassed(ctx, output)
+}
+
+func (c *UpgradeController) reportAppPreflightsSucceeded(ctx context.Context, _, _ statemachine.State) {
+	c.logger.Info("Reporting metrics event app preflights succeeded")
+	c.metricsReporter.ReportAppPreflightsSucceeded(ctx)
+}
diff --git a/api/controllers/linux/upgrade/statemachine.go b/api/controllers/linux/upgrade/statemachine.go
@@ -28,8 +28,9 @@ var baseStateTransitions = map[statemachine.State][]statemachine.State{
 
 // Infrastructure-specific state transitions
 var infraStateTransitions = map[statemachine.State][]statemachine.State{
-	states.StateInfrastructureUpgrading:     {states.StateInfrastructureUpgraded, states.StateInfrastructureUpgradeFailed},
-	states.StateInfrastructureUpgraded:      {states.StateAppPreflightsRunning},
+	states.StateInfrastructureUpgrading: {states.StateInfrastructureUpgraded, states.StateInfrastructureUpgradeFailed},
+	states.StateInfrastructureUpgraded:  {states.StateAppPreflightsRunning},
+	// final states
 	states.StateInfrastructureUpgradeFailed: {},
 }
 
diff --git a/api/internal/handlers/linux/linux.go b/api/internal/handlers/linux/linux.go
@@ -127,6 +127,8 @@ func New(cfg types.APIConfig, opts ...Option) (*Handler, error) {
 				upgrade.WithConfigValues(h.cfg.ConfigValues),
 				upgrade.WithEndUserConfig(h.cfg.EndUserConfig),
 				upgrade.WithClusterID(h.cfg.ClusterID),
+				upgrade.WithTargetVersion(h.cfg.TargetVersion),
+				upgrade.WithInitialVersion(h.cfg.InitialVersion),
 			)
 			if err != nil {
 				return nil, fmt.Errorf("new upgrade controller: %w", err)
diff --git a/api/types/api.go b/api/types/api.go
@@ -24,6 +24,8 @@ type APIConfig struct {
 	ClusterID          string
 	Target             Target
 	Mode               Mode
+	TargetVersion      string // Used for upgrade metrics reporting
+	InitialVersion     string // Used for upgrade metrics reporting
 
 	LinuxConfig
 	KubernetesConfig
diff --git a/cmd/installer/cli/upgrade.go b/cmd/installer/cli/upgrade.go
@@ -10,7 +10,6 @@ import (
 	"strings"
 	"syscall"
 
-	"github.com/AlecAivazis/survey/v2/terminal"
 	apitypes "github.com/replicatedhq/embedded-cluster/api/types"
 	"github.com/replicatedhq/embedded-cluster/cmd/installer/goods"
 	"github.com/replicatedhq/embedded-cluster/cmd/installer/kotscli"
@@ -129,21 +128,14 @@ func UpgradeCmd(ctx context.Context, appSlug, appTitle string) *cobra.Command {
 			)
 			metricsReporter.ReportUpgradeStarted(ctx)
 
-			// Setup signal handler with the metrics reporter cleanup function
-			signalHandler(ctx, cancel, func(ctx context.Context, sig os.Signal) {
-				metricsReporter.ReportSignalAborted(ctx, sig)
-			})
-
-			if err := runManagerExperienceUpgrade(ctx, flags, upgradeConfig, existingRC, metricsReporter.reporter, appTitle); err != nil {
-				// Check if this is an interrupt error from the terminal
-				if errors.Is(err, terminal.InterruptErr) {
-					metricsReporter.ReportSignalAborted(ctx, syscall.SIGINT)
-				} else {
-					metricsReporter.ReportUpgradeFailed(ctx, err)
-				}
+			// Run the manager experience upgrade - the upgrade controller will handle
+			// reporting success/failure events through its event handlers
+			if err := runManagerExperienceUpgrade(
+				ctx, flags, upgradeConfig, existingRC, metricsReporter.reporter, appTitle,
+				targetVersion, initialVersion,
+			); err != nil {
 				return err
 			}
-			metricsReporter.ReportUpgradeSucceeded(ctx)
 
 			return nil
 		},
@@ -433,7 +425,7 @@ func readPasswordHash(ctx context.Context, kcli client.Client) ([]byte, error) {
 
 func runManagerExperienceUpgrade(
 	ctx context.Context, flags UpgradeCmdFlags, upgradeConfig upgradeConfig, rc runtimeconfig.RuntimeConfig,
-	metricsReporter metrics.ReporterInterface, appTitle string,
+	metricsReporter metrics.ReporterInterface, appTitle string, targetVersion string, initialVersion string,
 ) (finalErr error) {
 	apiConfig := apiOptions{
 		APIConfig: apitypes.APIConfig{
@@ -450,6 +442,8 @@ func runManagerExperienceUpgrade(
 			ClusterID:          upgradeConfig.clusterID,
 			Target:             apitypes.Target(flags.target),
 			Mode:               apitypes.ModeUpgrade,
+			TargetVersion:      targetVersion,
+			InitialVersion:     initialVersion,
 
 			LinuxConfig: apitypes.LinuxConfig{
 				RuntimeConfig: rc,

Original file line number	Diff line number	Diff line change
`@@ -127,6 +127,8 @@ func New(cfg types.APIConfig, opts ...Option) (*Handler, error) {`
`127`	`127`	`upgrade.WithConfigValues(h.cfg.ConfigValues),`
`128`	`128`	`upgrade.WithEndUserConfig(h.cfg.EndUserConfig),`
`129`	`129`	`upgrade.WithClusterID(h.cfg.ClusterID),`
	`130`	`+ upgrade.WithTargetVersion(h.cfg.TargetVersion),`
	`131`	`+ upgrade.WithInitialVersion(h.cfg.InitialVersion),`
`130`	`132`	`)`
`131`	`133`	`if err != nil {`
`132`	`134`	`return nil, fmt.Errorf("new upgrade controller: %w", err)`