diff --git a/components/ws-manager-api/go/crd/v1/workspace_types.go b/components/ws-manager-api/go/crd/v1/workspace_types.go index bbc3a969e6e028..1cc0dcff8de080 100644 --- a/components/ws-manager-api/go/crd/v1/workspace_types.go +++ b/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 diff --git a/components/ws-manager-mk2/controllers/create.go b/components/ws-manager-mk2/controllers/create.go index 7574ecb69963bd..fb360be9ae714d 100644 --- a/components/ws-manager-mk2/controllers/create.go +++ b/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 @@ -407,7 +404,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, diff --git a/components/ws-manager-mk2/controllers/status.go b/components/ws-manager-mk2/controllers/status.go index 3fdb47634c04fa..63f9ace7d072cf 100644 --- a/components/ws-manager-mk2/controllers/status.go +++ b/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)) || diff --git a/components/ws-manager-mk2/controllers/workspace_controller.go b/components/ws-manager-mk2/controllers/workspace_controller.go index d52a9b539250da..a3a83e669c1273 100644 --- a/components/ws-manager-mk2/controllers/workspace_controller.go +++ b/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) } diff --git a/components/ws-manager-mk2/controllers/workspace_controller_test.go b/components/ws-manager-mk2/controllers/workspace_controller_test.go index 48007087fce91c..12e6fd90a6da34 100644 --- a/components/ws-manager-mk2/controllers/workspace_controller_test.go +++ b/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{ diff --git a/components/ws-manager-mk2/service/manager.go b/components/ws-manager-mk2/service/manager.go index 0e47d5fd9081eb..84c23fac98cb5c 100644 --- a/components/ws-manager-mk2/service/manager.go +++ b/components/ws-manager-mk2/service/manager.go @@ -41,6 +41,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 ( @@ -208,6 +209,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)