Set conditions more carefully in start/stop to improve metric accuracy

* Move setting "Started" condition to the point in the reconcile loop
  where we know the workspace is started to avoid marking new stopped
  workspaces as started

* Set "Started" condition to false when a workspace is stopped

* Only update metrics if the API call to sync workspace status succeeds
  to avoid accidentally incrementing metrics when a request fails

Signed-off-by: Angel Misevski <[email protected]>

Author: amisevsk

controllers/workspace/devworkspace_controller.go

@@ -120,18 +120,7 @@ func (r *DevWorkspaceReconciler) Reconcile(req ctrl.Request) (reconcileResult ct
 			return reconcile.Result{}, err
 		}
 		workspace.Status.DevWorkspaceId = workspaceId
-		workspace.Status.Phase = dw.DevWorkspaceStatusStarting
-		workspace.Status.Message = "Initializing DevWorkspace"
-		workspace.Status.Conditions = []dw.DevWorkspaceCondition{
-			{
-				Type:               conditions.Started,
-				Status:             corev1.ConditionTrue,
-				LastTransitionTime: metav1.Time{Time: clock.Now()},
-				Message:            "DevWorkspace is starting",
-			},
-		}
 		err = r.Status().Update(ctx, workspace)
-		metrics.WorkspaceStarted(workspace, reqLogger)
 		return reconcile.Result{Requeue: true}, err
 	}
 
@@ -160,6 +149,27 @@ func (r *DevWorkspaceReconciler) Reconcile(req ctrl.Request) (reconcileResult ct
 		return r.stopWorkspace(workspace, reqLogger)
 	}
 
+	// If this is the first reconcile for a starting workspace, mark it as starting now. This is done outside the regular
+	// updateWorkspaceStatus function to ensure it gets set immediately
+	if workspace.Status.Phase != dw.DevWorkspaceStatusStarting && workspace.Status.Phase != dw.DevWorkspaceStatusRunning {
+		// Set 'Started' condition as early as possible to get accurate timing metrics
+		workspace.Status.Phase = dw.DevWorkspaceStatusStarting
+		workspace.Status.Message = "Initializing DevWorkspace"
+		workspace.Status.Conditions = []dw.DevWorkspaceCondition{
+			{
+				Type:               conditions.Started,
+				Status:             corev1.ConditionTrue,
+				LastTransitionTime: metav1.Time{Time: clock.Now()},
+				Message:            "DevWorkspace is starting",
+			},
+		}
+		err = r.Status().Update(ctx, workspace)
+		if err == nil {
+			metrics.WorkspaceStarted(workspace, reqLogger)
+		}
+		return reconcile.Result{}, err
+	}
+
 	// Prepare handling workspace status and condition
 	reconcileStatus := currentStatus{phase: dw.DevWorkspaceStatusStarting}
 	reconcileStatus.setConditionTrue(conditions.Started, "DevWorkspace is starting")
@@ -370,8 +380,10 @@ func (r *DevWorkspaceReconciler) stopWorkspace(workspace *dw.DevWorkspace, logge
 		switch status.phase {
 		case devworkspacePhaseFailing, dw.DevWorkspaceStatusFailed:
 			status.phase = dw.DevWorkspaceStatusFailed
+			status.setConditionFalse(conditions.Started, "Workspace stopped due to error")
 		default:
 			status.phase = dw.DevWorkspaceStatusStopped
+			status.setConditionFalse(conditions.Started, "Workspace is stopped")
 		}
 	}
 	return r.updateWorkspaceStatus(workspace, logger, &status, reconcile.Result{}, nil)

controllers/workspace/status.go

@@ -68,7 +68,7 @@ var healthHttpClient = &http.Client{
 // updating the status.
 func (r *DevWorkspaceReconciler) updateWorkspaceStatus(workspace *dw.DevWorkspace, logger logr.Logger, status *currentStatus, reconcileResult reconcile.Result, reconcileError error) (reconcile.Result, error) {
 	syncConditions(&workspace.Status, status)
-	updateMetricsForPhase(workspace, status.phase, logger)
+	oldPhase := workspace.Status.Phase
 	workspace.Status.Phase = status.phase
 
 	infoMessage := getInfoMessage(workspace, status)
@@ -85,7 +85,10 @@ func (r *DevWorkspaceReconciler) updateWorkspaceStatus(workspace *dw.DevWorkspac
 		if reconcileError == nil {
 			reconcileError = err
 		}
+	} else {
+		updateMetricsForPhase(workspace, oldPhase, status.phase, logger)
 	}
+
 	return reconcileResult, reconcileError
 }
 
@@ -221,8 +224,8 @@ func getInfoMessage(workspace *dw.DevWorkspace, status *currentStatus) string {
 // updateMetricsForPhase increments DevWorkspace startup metrics based on phase transitions in a DevWorkspace. It avoids
 // incrementing the underlying metrics where possible (e.g. reconciling an already running workspace) by only incrementing
 // counters when the new phase is different from the current on in the DevWorkspace.
-func updateMetricsForPhase(workspace *dw.DevWorkspace, newPhase dw.DevWorkspacePhase, logger logr.Logger) {
-	if workspace.Status.Phase == newPhase {
+func updateMetricsForPhase(workspace *dw.DevWorkspace, oldPhase, newPhase dw.DevWorkspacePhase, logger logr.Logger) {
+	if oldPhase == newPhase {
 		return
 	}
 	switch newPhase {