[ws-manager-mk2] Add finalizer on workspace, handle deletion

Author: WVerlaek

components/ws-manager-api/go/crd/v1/workspace_types.go

@@ -9,6 +9,11 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
+const (
+	// GitpodFinalizerName is the name of the finalizer we use on workspaces and their pods.
+	GitpodFinalizerName = "gitpod.io/finalizer"
+)
+
 // WorkspaceSpec defines the desired state of Workspace
 type WorkspaceSpec struct {
 	// +kubebuilder:validation:Required

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

@@ -45,9 +45,6 @@ const (
 	// TODO(furisto): remove this label once we have moved ws-daemon to a controller setup
 	instanceIDLabel = "gitpod.io/instanceID"
 
-	// gitpodPodFinalizerName is the name of the finalizer we use on pods
-	gitpodPodFinalizerName = "gitpod.io/finalizer"
-
 	// Grace time until the process in the workspace is properly completed
 	// e.g. dockerd in the workspace may take some time to clean up the overlay directory.
 	gracePeriod = 3 * time.Minute
@@ -411,7 +408,7 @@ func createDefiniteWorkspacePod(sctx *startWorkspaceContext) (*corev1.Pod, error
 			Namespace:   sctx.Config.Namespace,
 			Labels:      labels,
 			Annotations: annotations,
-			Finalizers:  []string{gitpodPodFinalizerName},
+			Finalizers:  []string{workspacev1.GitpodFinalizerName},
 		},
 		Spec: corev1.PodSpec{
 			Hostname:                     sctx.Workspace.Spec.Ownership.WorkspaceID,

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

@@ -121,7 +121,7 @@ func updateWorkspaceStatus(ctx context.Context, workspace *workspacev1.Workspace
 	case isPodBeingDeleted(pod):
 		workspace.Status.Phase = workspacev1.WorkspacePhaseStopping
 
-		if controllerutil.ContainsFinalizer(pod, gitpodPodFinalizerName) {
+		if controllerutil.ContainsFinalizer(pod, workspacev1.GitpodFinalizerName) {
 			if wsk8s.ConditionPresentAndTrue(workspace.Status.Conditions, string(workspacev1.WorkspaceConditionBackupComplete)) ||
 				wsk8s.ConditionPresentAndTrue(workspace.Status.Conditions, string(workspacev1.WorkspaceConditionBackupFailure)) ||
 				wsk8s.ConditionPresentAndTrue(workspace.Status.Conditions, string(workspacev1.WorkspaceConditionAborted)) ||

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

@@ -162,10 +162,18 @@ func (r *WorkspaceReconciler) actOnStatus(ctx context.Context, workspace *worksp
 			r.metrics.rememberWorkspace(workspace)
 
 		case workspace.Status.Phase == workspacev1.WorkspacePhaseStopped:
-			err := r.Client.Delete(ctx, workspace)
-			if err != nil {
-				return ctrl.Result{Requeue: true}, err
+			// Done stopping workspace - remove finalizer.
+			if controllerutil.ContainsFinalizer(workspace, workspacev1.GitpodFinalizerName) {
+				controllerutil.RemoveFinalizer(workspace, workspacev1.GitpodFinalizerName)
+				if err := r.Update(ctx, workspace); err != nil {
+					return ctrl.Result{}, client.IgnoreNotFound(err)
+				}
 			}
+
+			// Workspace might have already been in a deleting state,
+			// but not guaranteed, so try deleting anyway.
+			err := r.Client.Delete(ctx, workspace)
+			return ctrl.Result{}, client.IgnoreNotFound(err)
 		}
 
 		return ctrl.Result{}, nil
@@ -223,10 +231,20 @@ func (r *WorkspaceReconciler) actOnStatus(ctx context.Context, workspace *worksp
 			return ctrl.Result{Requeue: true}, err
 		}
 
+	case isWorkspaceBeingDeleted(workspace) && !isPodBeingDeleted(pod):
+		// Workspace was requested to be deleted, propagate by deleting the Pod.
+		// The Pod deletion will then trigger workspace disposal steps.
+		err := r.Client.Delete(ctx, pod)
+		if errors.IsNotFound(err) {
+			// pod is gone - nothing to do here
+		} else {
+			return ctrl.Result{Requeue: true}, err
+		}
+
 	// we've disposed already - try to remove the finalizer and call it a day
 	case workspace.Status.Phase == workspacev1.WorkspacePhaseStopped:
-		hadFinalizer := controllerutil.ContainsFinalizer(pod, gitpodPodFinalizerName)
-		controllerutil.RemoveFinalizer(pod, gitpodPodFinalizerName)
+		hadFinalizer := controllerutil.ContainsFinalizer(pod, workspacev1.GitpodFinalizerName)
+		controllerutil.RemoveFinalizer(pod, workspacev1.GitpodFinalizerName)
 		if err := r.Client.Update(ctx, pod); err != nil {
 			return ctrl.Result{}, fmt.Errorf("failed to remove gitpod finalizer: %w", err)
 		}

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

@@ -39,7 +39,7 @@ var _ = Describe("WorkspaceController", func() {
 			ws := newWorkspace(uuid.NewString(), "default")
 			pod := createWorkspaceExpectPod(ws)
 
-			Expect(controllerutil.ContainsFinalizer(pod, gitpodPodFinalizerName)).To(BeTrue())
+			Expect(controllerutil.ContainsFinalizer(pod, workspacev1.GitpodFinalizerName)).To(BeTrue())
 
 			By("controller updating the pod starts value")
 			Eventually(func() (int, error) {
@@ -156,6 +156,19 @@ var _ = Describe("WorkspaceController", func() {
 			// Expect cleanup without a backup.
 			expectWorkspaceCleanup(ws, pod)
 		})
+
+		It("deleting workspace resource should gracefully clean up", func() {
+			ws := newWorkspace(uuid.NewString(), "default")
+			pod := createWorkspaceExpectPod(ws)
+
+			Expect(k8sClient.Delete(ctx, ws)).To(Succeed())
+
+			expectPhaseEventually(ws, workspacev1.WorkspacePhaseStopping)
+
+			expectFinalizerAndMarkBackupCompleted(ws, pod)
+
+			expectWorkspaceCleanup(ws, pod)
+		})
 	})
 
 	Context("with headless workspaces", func() {
@@ -250,6 +263,14 @@ func createWorkspaceExpectPod(ws *workspacev1.Workspace) *corev1.Pod {
 	return pod
 }
 
+func expectPhaseEventually(ws *workspacev1.Workspace, phase workspacev1.WorkspacePhase) {
+	By(fmt.Sprintf("controller transition workspace phase to %s", phase))
+	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(phase))
+	}, timeout, interval).Should(Succeed())
+}
+
 func expectConditionEventually(ws *workspacev1.Workspace, tpe string, status string, reason string) {
 	By(fmt.Sprintf("controller setting workspace condition %s to %s", tpe, status))
 	Eventually(func(g Gomega) {
@@ -306,7 +327,7 @@ func expectFinalizerAndMarkBackupCompleted(ws *workspacev1.Workspace, pod *corev
 		if err := k8sClient.Get(ctx, types.NamespacedName{Name: pod.GetName(), Namespace: pod.GetNamespace()}, pod); err != nil {
 			return false, err
 		}
-		return controllerutil.ContainsFinalizer(pod, gitpodPodFinalizerName), nil
+		return controllerutil.ContainsFinalizer(pod, workspacev1.GitpodFinalizerName), nil
 	}, duration, interval).Should(BeTrue(), "missing gitpod finalizer on pod, expected one to wait for backup to succeed")
 
 	By("signalling backup completed")
@@ -328,7 +349,7 @@ func expectFinalizerAndMarkBackupFailed(ws *workspacev1.Workspace, pod *corev1.P
 		if err := k8sClient.Get(ctx, types.NamespacedName{Name: pod.GetName(), Namespace: pod.GetNamespace()}, pod); err != nil {
 			return false, err
 		}
-		return controllerutil.ContainsFinalizer(pod, gitpodPodFinalizerName), nil
+		return controllerutil.ContainsFinalizer(pod, workspacev1.GitpodFinalizerName), nil
 	}, duration, interval).Should(BeTrue(), "missing gitpod finalizer on pod, expected one to wait for backup to succeed")
 
 	By("signalling backup completed")
@@ -347,7 +368,8 @@ func expectWorkspaceCleanup(ws *workspacev1.Workspace, pod *corev1.Pod) {
 	Eventually(func() (int, error) {
 		if err := k8sClient.Get(ctx, types.NamespacedName{Name: pod.GetName(), Namespace: pod.GetNamespace()}, pod); err != nil {
 			if errors.IsNotFound(err) {
-				// Pod got deleted, because finalizers got removed.
+				// Race: finalizers got removed causing pod to get deleted before we could check.
+				// This is what we want though.
 				return 0, nil
 			}
 			return 0, err
@@ -361,6 +383,20 @@ func expectWorkspaceCleanup(ws *workspacev1.Workspace, pod *corev1.Pod) {
 		return checkNotFound(pod)
 	}, timeout, interval).Should(Succeed(), "pod did not go away")
 
+	By("controller removing workspace finalizers")
+	Eventually(func() (int, error) {
+		if err := k8sClient.Get(ctx, types.NamespacedName{Name: ws.GetName(), Namespace: ws.GetNamespace()}, ws); err != nil {
+			if errors.IsNotFound(err) {
+				// Race: finalizers got removed causing workspace to get deleted before we could check.
+				// This is what we want though.
+				return 0, nil
+			}
+			return 0, err
+		}
+		return len(ws.ObjectMeta.Finalizers), nil
+
+	}, timeout, interval).Should(Equal(0), "workspace finalizers did not go away")
+
 	By("cleaning up the workspace resource")
 	Eventually(func() error {
 		return checkNotFound(ws)
@@ -395,8 +431,9 @@ func newWorkspace(name, namespace string) *workspacev1.Workspace {
 			Kind:       "Workspace",
 		},
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      name,
-			Namespace: namespace,
+			Name:       name,
+			Namespace:  namespace,
+			Finalizers: []string{workspacev1.GitpodFinalizerName},
 		},
 		Spec: workspacev1.WorkspaceSpec{
 			Ownership: workspacev1.Ownership{

components/ws-manager-mk2/service/manager.go

@@ -43,6 +43,7 @@ import (
 	"k8s.io/client-go/util/retry"
 	"k8s.io/utils/pointer"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
 
 const (
@@ -232,6 +233,7 @@ func (wsm *WorkspaceManagerServer) StartWorkspace(ctx context.Context, req *wsma
 			Ports: ports,
 		},
 	}
+	controllerutil.AddFinalizer(&ws, workspacev1.GitpodFinalizerName)
 
 	wsm.metrics.recordWorkspaceStart(&ws)
 	err = wsm.Client.Create(ctx, &ws)