WIP maxRetries test

Author: geropl

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

@@ -242,13 +242,6 @@ func (r *WorkspaceReconciler) actOnStatus(ctx context.Context, workspace *worksp
 			}
 			log.Info("trigger pod recreation")
 
-			// Must persist the modification pod starts, and ensure we retry on conflict.
-			// If we fail to persist this value, it's possible that the Pod gets recreated endlessly
-			// when the workspace stops, due to PodStarts still being 0 when the original Pod
-			// disappears.
-			// Use a Patch instead of an Update, to prevent conflicts.
-			patch := client.MergeFrom(workspace.DeepCopy())
-
 			// Reset status
 			sc := workspace.Status.DeepCopy()
 			workspace.Status = workspacev1.WorkspaceStatus{}
@@ -258,8 +251,8 @@ func (r *WorkspaceReconciler) actOnStatus(ctx context.Context, workspace *worksp
 			workspace.Status.PodRecreated = sc.PodRecreated + 1
 			workspace.Status.SetCondition(workspacev1.NewWorkspaceConditionPodRejected(fmt.Sprintf("Recreating pod... (%d retry)", workspace.Status.PodRecreated), metav1.ConditionFalse))
 
-			if err := r.Status().Patch(ctx, workspace, patch); err != nil {
-				log.Error(err, "Failed to patch workspace status-reset")
+			if err := r.Status().Update(ctx, workspace); err != nil {
+				log.Error(err, "Failed to update workspace status-reset")
 				return ctrl.Result{}, err
 			}
 

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

@@ -538,6 +538,154 @@ var _ = Describe("WorkspaceController", func() {
 			expectPhaseTransitions(su, []workspacev1.WorkspacePhase{workspacev1.WorkspacePhasePending, workspacev1.WorkspacePhaseCreating, workspacev1.WorkspacePhaseInitializing, workspacev1.WorkspacePhaseRunning})
 			// ### validate end
 		})
+
+		It("pod recreation is only tried until limit", func() {
+			ws := newWorkspace(uuid.NewString(), "default")
+			m := collectMetricCounts(wsMetrics, ws)
+			su := collectSubscriberUpdates()
+
+			// ### prepare block start
+			By("creating workspace")
+			// Simulate pod getting scheduled to a node.
+			var node corev1.Node
+			node.Name = uuid.NewString()
+			Expect(k8sClient.Create(ctx, &node)).To(Succeed())
+			// Manually create the workspace pod with the node name.
+			// We can't update the pod with the node name, as this operation
+			// is only allowed for the scheduler. So as a hack, we manually
+			// create the workspace's pod.
+			pod := &corev1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       fmt.Sprintf("ws-%s", ws.Name),
+					Namespace:  ws.Namespace,
+					Finalizers: []string{workspacev1.GitpodFinalizerName},
+					Labels: map[string]string{
+						wsk8s.WorkspaceManagedByLabel: constants.ManagedBy,
+					},
+				},
+				Spec: corev1.PodSpec{
+					NodeName: node.Name,
+					Containers: []corev1.Container{{
+						Name:  "workspace",
+						Image: "someimage",
+					}},
+				},
+			}
+
+			Expect(k8sClient.Create(ctx, pod)).To(Succeed())
+			pod = createWorkspaceExpectPod(ws)
+			updateObjWithRetries(k8sClient, pod, false, func(pod *corev1.Pod) {
+				Expect(ctrl.SetControllerReference(ws, pod, k8sClient.Scheme())).To(Succeed())
+			})
+
+			podStarts := 1
+			podRecreated := 0
+			// mimic the regular "start" phase
+			updateObjWithRetries(k8sClient, ws, true, func(ws *workspacev1.Workspace) {
+				ws.Status.PodStarts = podStarts
+				ws.Status.PodRecreated = podRecreated
+			})
+
+			startAndRejectWorkspace := func() {
+
+				// Wait until controller has reconciled at least once (by waiting for the runtime status to get updated).
+				// This is necessary for the metrics to get recorded correctly. If we don't wait, the first reconciliation
+				// might be once the Pod is already in a running state, and hence the metric state might not record e.g. content
+				// restore.
+				// This is only necessary because we manually created the pod, normally the Pod creation is the controller's
+				// first reconciliation which ensures the metrics are recorded from the workspace's initial state.
+
+				Eventually(func(g Gomega) {
+					g.Expect(k8sClient.Get(ctx, types.NamespacedName{Name: ws.Name, Namespace: ws.Namespace}, ws)).To(Succeed())
+					g.Expect(ws.Status.Runtime).ToNot(BeNil())
+					g.Expect(ws.Status.Runtime.PodName).To(Equal(pod.Name))
+				}, timeout, interval).Should(Succeed())
+
+				// Await "deployed" condition, and check we are good
+				expectConditionEventually(ws, string(workspacev1.WorkspaceConditionDeployed), metav1.ConditionTrue, "")
+				Eventually(func(g Gomega) {
+					g.Expect(k8sClient.Get(ctx, types.NamespacedName{Name: ws.Name, Namespace: ws.Namespace}, ws)).To(Succeed())
+					g.Expect(ws.Status.PodStarts).To(Equal(podStarts))
+					g.Expect(ws.Status.PodRecreated).To(Equal(podRecreated))
+				}, timeout, interval).Should(Succeed())
+
+				// ### prepare block end
+
+				// ### trigger block start
+				// Make pod be rejected 🪄
+				By("rejecting pod")
+				rejectPod(pod)
+
+				By("await pod being in stopping")
+				Eventually(func(g Gomega) {
+					g.Expect(k8sClient.Get(ctx, types.NamespacedName{Name: ws.Name, Namespace: ws.Namespace}, ws)).To(Succeed())
+					g.Expect(ws.Status.Phase).To(Equal(workspacev1.WorkspacePhaseStopping))
+				}, timeout, interval).Should(Succeed())
+
+				// when a rejected workspace pod is in stopping, ws-daemon wipes the state before it's moved to "stopped"
+				// mimic this ws-daemon behavior
+				updateObjWithRetries(k8sClient, ws, true, func(ws *workspacev1.Workspace) {
+					ws.Status.SetCondition(workspacev1.NewWorkspaceConditionStateWiped("", metav1.ConditionTrue))
+				})
+
+				podStarts++
+				podRecreated++
+
+				By("await pod recreation")
+				Eventually(func(g Gomega) {
+					g.Expect(k8sClient.Get(ctx, types.NamespacedName{Name: ws.Name, Namespace: ws.Namespace}, ws)).To(Succeed())
+					g.Expect(ws.Status.PodRecreated).To(Equal(podRecreated))
+					g.Expect(ws.Status.Phase).To(Equal(workspacev1.WorkspacePhasePending))
+				}, timeout, interval).Should(Succeed())
+				// ### trigger block end
+
+				// ### retry block start
+				// Transition Pod to pending, and expect workspace to reach Creating phase.
+				// This should also cause create time metrics to be recorded.
+				updateObjWithRetries(k8sClient, pod, true, func(pod *corev1.Pod) {
+					pod.Status.Phase = corev1.PodPending
+					pod.Status.ContainerStatuses = []corev1.ContainerStatus{{
+						State: corev1.ContainerState{
+							Waiting: &corev1.ContainerStateWaiting{
+								Reason: "ContainerCreating",
+							},
+						},
+						Name: "workspace",
+					}}
+				})
+
+				expectPhaseEventually(ws, workspacev1.WorkspacePhaseCreating)
+				// ### retry block end
+			}
+
+			for j := 0; j < 4; j++ {
+				startAndRejectWorkspace()
+			}
+
+			expectWorkspaceCleanup(ws, pod)
+
+			// ### validate result
+			Eventually(func(g Gomega) {
+				g.Expect(k8sClient.Get(ctx, types.NamespacedName{Name: ws.Name, Namespace: ws.Namespace}, ws)).To(Succeed())
+				g.Expect(ws.Status.PodStarts).To(Equal(4))
+				g.Expect(ws.Status.PodRecreated).To(Equal(3))
+			}, timeout, interval).Should(Succeed())
+			expectConditionEventually(ws, string(workspacev1.WorkspaceConditionFailed), metav1.ConditionTrue, "")
+
+			expectMetricsDelta(m, collectMetricCounts(wsMetrics, ws), metricCounts{
+				restores:       1,
+				backups:        0,
+				backupFailures: 0,
+				failures:       1,
+				creatingCounts: 1,
+				stops:          map[StopReason]int{StopReasonStartFailure: 1},
+				starts:         1, // this is NOT PodStarts, but merely an artifact of how we count it in the tests
+				recreations:    map[int]int{1: 1},
+			})
+
+			expectPhaseTransitions(su, []workspacev1.WorkspacePhase{workspacev1.WorkspacePhasePending, workspacev1.WorkspacePhaseCreating, workspacev1.WorkspacePhaseInitializing, workspacev1.WorkspacePhaseStopping, workspacev1.WorkspacePhaseStopped})
+			// ### validate end
+		})
 	})
 
 	Context("with headless workspaces", func() {