[ws-manager-mk2] Refactor metric collection and add tests (#16585)

Author: WVerlaek

components/ws-manager-mk2/controllers/metrics.go

@@ -9,6 +9,7 @@ import (
 	"strings"
 	"time"
 
+	wsk8s "github.com/gitpod-io/gitpod/common-go/kubernetes"
 	workspacev1 "github.com/gitpod-io/gitpod/ws-manager/api/crd/v1"
 	"github.com/go-logr/logr"
 	lru "github.com/hashicorp/golang-lru"
@@ -185,22 +186,54 @@ func (m *controllerMetrics) countTotalRestoreFailures(log *logr.Logger, ws *work
 	counter.Inc()
 }
 
-func (m *controllerMetrics) rememberWorkspace(ws *workspacev1.Workspace) {
-	m.cache.Add(ws.Name, ws.Status.Phase)
+func (m *controllerMetrics) rememberWorkspace(ws *workspacev1.Workspace, state *metricState) {
+	var s metricState
+	if state != nil {
+		s = *state
+	} else {
+		s = newMetricState(ws)
+	}
+	m.cache.Add(ws.Name, s)
 }
 
 func (m *controllerMetrics) forgetWorkspace(ws *workspacev1.Workspace) {
 	m.cache.Remove(ws.Name)
 }
 
-func (m *controllerMetrics) shouldUpdate(log *logr.Logger, ws *workspacev1.Workspace) bool {
-	p, ok := m.cache.Get(ws.Name)
+// metricState is used to track which metrics have been recorded for a workspace.
+type metricState struct {
+	phase                   workspacev1.WorkspacePhase
+	recordedStartTime       bool
+	recordedInitFailure     bool
+	recordedStartFailure    bool
+	recordedContentReady    bool
+	recordedBackupFailed    bool
+	recordedBackupCompleted bool
+}
+
+func newMetricState(ws *workspacev1.Workspace) metricState {
+	return metricState{
+		phase: ws.Status.Phase,
+		// Here we assume that we've recorded metrics for the following states already if their conditions already exist.
+		// This is to prevent these from being re-recorded after the controller restarts and clears the metric state for
+		// each workspace.
+		recordedStartTime:       ws.Status.Phase == workspacev1.WorkspacePhaseRunning,
+		recordedInitFailure:     wsk8s.ConditionWithStatusAndReason(ws.Status.Conditions, string(workspacev1.WorkspaceConditionContentReady), false, "InitializationFailure"),
+		recordedStartFailure:    wsk8s.ConditionPresentAndTrue(ws.Status.Conditions, string(workspacev1.WorkspaceConditionFailed)),
+		recordedContentReady:    wsk8s.ConditionPresentAndTrue(ws.Status.Conditions, string(workspacev1.WorkspaceConditionContentReady)),
+		recordedBackupFailed:    wsk8s.ConditionPresentAndTrue(ws.Status.Conditions, string(workspacev1.WorkspaceConditionBackupFailure)),
+		recordedBackupCompleted: wsk8s.ConditionPresentAndTrue(ws.Status.Conditions, string(workspacev1.WorkspaceConditionBackupComplete)),
+	}
+}
+
+// getWorkspace returns the last recorded metric state for that workspace.
+func (m *controllerMetrics) getWorkspace(log *logr.Logger, ws *workspacev1.Workspace) (bool, metricState) {
+	s, ok := m.cache.Get(ws.Name)
 	if !ok {
-		return false
+		return false, metricState{}
 	}
 
-	phase := p.(workspacev1.WorkspacePhase)
-	return phase != ws.Status.Phase
+	return true, s.(metricState)
 }
 
 // Describe implements Collector. It will send exactly one Desc to the provided channel.

components/ws-manager-mk2/controllers/suite_test.go

@@ -50,6 +50,7 @@ var (
 	ctx        context.Context
 	cancel     context.CancelFunc
 	wsActivity *activity.WorkspaceActivity
+	wsMetrics  *controllerMetrics
 )
 
 var _ = BeforeSuite(func() {
@@ -102,6 +103,7 @@ var _ = BeforeSuite(func() {
 
 	conf := newTestConfig()
 	wsReconciler, err := NewWorkspaceReconciler(k8sManager.GetClient(), k8sManager.GetScheme(), &conf, metrics.Registry)
+	wsMetrics = wsReconciler.metrics
 	Expect(err).ToNot(HaveOccurred())
 	Expect(wsReconciler.SetupWithManager(k8sManager)).To(Succeed())
 

components/ws-manager-mk2/controllers/workspace_controller.go

@@ -159,7 +159,7 @@ func (r *WorkspaceReconciler) actOnStatus(ctx context.Context, workspace *worksp
 				//			 need to be deleted and re-created
 				workspace.Status.PodStarts++
 			}
-			r.metrics.rememberWorkspace(workspace)
+			r.metrics.rememberWorkspace(workspace, nil)
 
 		case workspace.Status.Phase == workspacev1.WorkspacePhaseStopped:
 			// Done stopping workspace - remove finalizer.
@@ -249,48 +249,58 @@ func (r *WorkspaceReconciler) actOnStatus(ctx context.Context, workspace *worksp
 func (r *WorkspaceReconciler) updateMetrics(ctx context.Context, workspace *workspacev1.Workspace) {
 	log := log.FromContext(ctx)
 
-	phase := workspace.Status.Phase
-
-	if !r.metrics.shouldUpdate(&log, workspace) {
+	ok, lastState := r.metrics.getWorkspace(&log, workspace)
+	if !ok {
 		return
 	}
 
-	switch {
-	case phase == workspacev1.WorkspacePhasePending ||
-		phase == workspacev1.WorkspacePhaseCreating ||
-		phase == workspacev1.WorkspacePhaseInitializing:
+	if !lastState.recordedInitFailure && wsk8s.ConditionWithStatusAndReason(workspace.Status.Conditions, string(workspacev1.WorkspaceConditionContentReady), false, "InitializationFailure") {
+		r.metrics.countTotalRestoreFailures(&log, workspace)
+		lastState.recordedInitFailure = true
 
-		if wsk8s.ConditionWithStatusAndReason(workspace.Status.Conditions, string(workspacev1.WorkspaceConditionContentReady), false, "InitializationFailure") {
-			r.metrics.countTotalRestoreFailures(&log, workspace)
+		if !lastState.recordedStartFailure {
 			r.metrics.countWorkspaceStartFailures(&log, workspace)
+			lastState.recordedStartFailure = true
 		}
+	}
 
-		if wsk8s.ConditionPresentAndTrue(workspace.Status.Conditions, string(workspacev1.WorkspaceConditionFailed)) {
-			r.metrics.countWorkspaceStartFailures(&log, workspace)
-		}
+	if !lastState.recordedStartFailure && wsk8s.ConditionPresentAndTrue(workspace.Status.Conditions, string(workspacev1.WorkspaceConditionFailed)) {
+		// Only record if there was no other start failure recorded yet, to ensure max one
+		// start failure gets recorded per workspace.
+		r.metrics.countWorkspaceStartFailures(&log, workspace)
+		lastState.recordedStartFailure = true
+	}
 
-	case phase == workspacev1.WorkspacePhaseRunning:
-		r.metrics.recordWorkspaceStartupTime(&log, workspace)
-		if wsk8s.ConditionPresentAndTrue(workspace.Status.Conditions, string(workspacev1.WorkspaceConditionContentReady)) {
-			r.metrics.countTotalRestores(&log, workspace)
-		}
+	if !lastState.recordedContentReady && wsk8s.ConditionPresentAndTrue(workspace.Status.Conditions, string(workspacev1.WorkspaceConditionContentReady)) {
+		r.metrics.countTotalRestores(&log, workspace)
+		lastState.recordedContentReady = true
+	}
 
-	case phase == workspacev1.WorkspacePhaseStopped:
-		if wsk8s.ConditionPresentAndTrue(workspace.Status.Conditions, string(workspacev1.WorkspaceConditionBackupFailure)) {
-			r.metrics.countTotalBackups(&log, workspace)
-			r.metrics.countTotalBackupFailures(&log, workspace)
-		}
+	if !lastState.recordedBackupFailed && wsk8s.ConditionPresentAndTrue(workspace.Status.Conditions, string(workspacev1.WorkspaceConditionBackupFailure)) {
+		r.metrics.countTotalBackups(&log, workspace)
+		r.metrics.countTotalBackupFailures(&log, workspace)
+		lastState.recordedBackupFailed = true
+	}
 
-		if wsk8s.ConditionPresentAndTrue(workspace.Status.Conditions, string(workspacev1.WorkspaceConditionBackupComplete)) {
-			r.metrics.countTotalBackups(&log, workspace)
-		}
+	if !lastState.recordedBackupCompleted && wsk8s.ConditionPresentAndTrue(workspace.Status.Conditions, string(workspacev1.WorkspaceConditionBackupComplete)) {
+		r.metrics.countTotalBackups(&log, workspace)
+		lastState.recordedBackupCompleted = true
+	}
 
+	if !lastState.recordedStartTime && workspace.Status.Phase == workspacev1.WorkspacePhaseRunning {
+		r.metrics.recordWorkspaceStartupTime(&log, workspace)
+		lastState.recordedStartTime = true
+	}
+
+	if workspace.Status.Phase == workspacev1.WorkspacePhaseStopped {
 		r.metrics.countWorkspaceStop(&log, workspace)
+
+		// Forget about this workspace, no more state updates will be recorded after this.
 		r.metrics.forgetWorkspace(workspace)
 		return
 	}
 
-	r.metrics.rememberWorkspace(workspace)
+	r.metrics.rememberWorkspace(workspace, &lastState)
 }
 
 func (r *WorkspaceReconciler) deleteWorkspacePod(ctx context.Context, pod *corev1.Pod, reason string) (ctrl.Result, error) {