fix: keep shared PVC from getting stuck Terminating on workspace cleanup (CRW-13586)

adr/2026-10-26-coordinate-cleanup-job-lifecycle-with-shared-pvc-deletion.md

@@ -0,0 +1,86 @@
+# Coordinate cleanup Job lifecycle with shared PVC deletion
+
+**Status**: Proposed
+**Date**: 2026-10-26
+**Deciders**: DevWorkspace Operator maintainers
+**Related Issue**: [CRW-13586](https://redhat.atlassian.net/browse/CRW-13586)
+
+## Context
+
+When the last workspace using the common (per-user) PVC in a namespace is deleted, the per-user PVC can get stuck in a `Terminating` state indefinitely, blocking all new workspaces in that namespace (`Shared PVC is in terminating state`). The only workaround is manually deleting leftover completed `cleanup-<workspaceId>` pods.
+
+Investigation and an on-cluster reproduction (OpenShift 4.22, DWO v1.0.0) showed the failure is a chain of three independent defects that only bite together:
+
+1. **Cleanup job churn.** `FindNodeForPVC` selects pods by the presence of the `controller.devfile.io/devworkspace_id` label — which the cleanup job's *own pod* also carries once Running. The job is initially created without node affinity (no workspace pods are running yet); once its pod starts, the computed spec gains node affinity, `jobDiffFunc` reports drift, and the sync layer deletes and recreates the job mid-run.
+2. **Pod orphaning.** The sync layer deleted objects with a plain `Delete()` and no propagation policy. Since Kubernetes 1.33, deleting a Job without an explicit propagation policy *orphans* its pods (ownerReferences are stripped), so the deleted job's completed pod is never removed by anything.
+3. **Uncoordinated PVC deletion.** When `getSharedPVCWorkspaceCount` reaches zero, the shared PVC is deleted immediately, with no coordination with cleanup jobs of concurrently deleted workspaces that are still in flight.
+
+The Kubernetes `pvc-protection` finalizer then holds the PVC in `Terminating` as long as the orphaned completed pod references it — permanently, since nothing deletes that pod. On older Kubernetes versions (Dev Spaces 3.24–3.29), the recreated cleanup job fails to schedule against the terminating PVC (`FailedScheduling`), which funnels into a `FailError` and a workspace whose finalizer is never cleared.
+
+## Decision
+
+Three targeted changes, so that each defect is fixed independently of the others:
+
+1. **`FindNodeForPVC`/`getNodeNameWithPVC` ignore pods owned by Jobs** (`pkg/provision/storage/shared.go`). Cleanup and backup job pods mount the PVC themselves and say nothing about where *workspace* pods run; matching them was the source of the spec drift that triggered job churn.
+2. **All sync-layer deletions use background propagation** (`pkg/provision/sync/sync.go`). A single `deleteObject` helper applies `propagationPolicy: Background` on every delete performed by `SyncObjectWithCluster`, `SyncUnrecognizedObjectWithCluster` and `updateObjectGeneric`. For Jobs this is required to avoid orphaning pods on Kubernetes >= 1.33; for other kinds it matches the default GC behavior.
+3. **The last workspace's PVC deletion is coordinated with leftover cleanup jobs and pods** (`deleteCommonPVC` in `pkg/provision/storage/cleanup.go`, called from `CleanupWorkspaceStorage` when the shared-PVC workspace count is zero):
+   - finished jobs (completed **or failed**) and jobs whose owner workspace no longer exists are deleted with background propagation;
+   - jobs still in flight for an existing workspace are awaited (`RetryError`, 10s requeue);
+   - orphaned pods (no ownerReferences) referencing the PVC are deleted directly — this also un-wedges namespaces already stuck before an upgrade;
+   - pods still owned are awaited, as their owner chain will remove them;
+   - the PVC is only deleted on a reconcile that confirms no references remain.
+
+   The branch now also resolves the PVC name via `checkForAlternatePVC`, so the alternate PVC (`claim-che-workspace`) is deleted rather than the configured name when it is the one actually in use.
+
+## Considered Alternatives
+
+### Alternative 1: Fix only the job churn (change 1)
+
+Killing the self-referential affinity drift prevents the job from being deleted mid-run, so no pods are orphaned via that path.
+
+**Rejected because**:
+- The sync layer can still delete Jobs for other drift reasons; without an explicit propagation policy, any such deletion orphans pods on Kubernetes >= 1.33.
+- The race between PVC deletion and in-flight cleanup jobs of concurrently deleted workspaces remains, keeping the PVC in `Terminating` and blocking new workspaces for the cleanup duration.
+
+### Alternative 2: Delete all leftover cleanup jobs unconditionally before deleting the PVC
+
+Simpler than the ownership-aware rule: list jobs referencing the PVC and delete them all.
+
+**Rejected because**:
+- Deleting another terminating workspace's in-flight job makes its finalize recreate the job, causing delete/recreate ping-pong until the job happens to complete between two reconciles.
+- Awaiting owned in-flight jobs converges without churn and bounds the wait to the job's natural completion.
+
+### Alternative 3: Wait for all referencing pods without deleting anything
+
+List pods referencing the PVC and only delete the PVC once none remain.
+
+**Rejected because**:
+- Orphaned pods (the exact artifact of the bug, and what exists on already-affected clusters before an upgrade) are never removed by anything, so the last workspace would wedge in `Terminating` forever (though the PVC would survive and new workspaces could start).
+- Deleting orphaned references directly both un-wedges existing clusters and keeps the deletion path bounded.
+
+## Consequences
+
+### Positive
+
+1. The shared PVC can no longer get permanently stuck in `Terminating` due to orphaned cleanup pods; the original workaround (manually deleting completed cleanup pods) is no longer needed, and clusters already in the bad state self-heal on the next reconcile of a deleted workspace.
+2. Deleting the last workspace no longer races in-flight cleanup jobs of concurrently deleted workspaces; the PVC is removed only once its data is actually wiped.
+3. Job churn (delete/recreate mid-run) is eliminated, so `rm -rf` cleanup runs are not restarted.
+4. The last-workspace deletion path now deletes the PVC actually in use (including the alternate PVC name).
+
+### Negative
+
+1. Deleting the last workspace takes longer: it requeues every 10s while leftover jobs/pods referencing the PVC are resolved, instead of deleting the PVC immediately.
+2. A cleanup job that never finishes (e.g. its pod cannot be scheduled) while its owner workspace still exists delays the PVC deletion of another workspace's finalize until the job's owner resolves it; the namespace stays usable in the meantime since the PVC is not deleted.
+
+### Neutral
+
+1. All sync-layer deletions now carry an explicit `propagationPolicy: Background`, which is the effective default for non-Job kinds.
+2. A cleanup job whose `JobFailed` condition is set is now treated as terminal by the last-workspace deletion path and deleted; its owning workspace's finalize may then succeed on a subsequent reconcile instead of remaining in `FailError` forever.
+
+## References
+
+- Reproduction and root-cause analysis: CRW-13586
+- `pkg/provision/storage/shared.go` (`getNodeNameWithPVC`, `isOwnedByJob`)
+- `pkg/provision/sync/sync.go` (`deleteObject`)
+- `pkg/provision/storage/cleanup.go` (`deleteCommonPVC`, `jobFinished`, `jobOwnerWorkspaceExists`)
+- [Kubernetes 1.33 release notes](https://kubernetes.io/blog/2025/04/16/kubernetes-v1-33-release/) — Jobs no longer delete child pods by default on job deletion

pkg/provision/storage/cleanup.go

@@ -20,6 +20,7 @@ import (
 	"path"
 	"time"
 
+	dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
 	"github.com/devfile/devworkspace-operator/pkg/dwerrors"
 	"github.com/devfile/devworkspace-operator/pkg/library/status"
 	nsconfig "github.com/devfile/devworkspace-operator/pkg/provision/config"
@@ -29,6 +30,7 @@ import (
 	k8sErrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/types"
 	k8sclient "sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -230,3 +232,159 @@ func commonPVCExists(workspace *common.DevWorkspaceWithConfig, clusterAPI sync.C
 	}
 	return true, nil
 }
+
+// deleteCommonPVC deletes the shared PVC used for workspace storage in the workspace's
+// namespace, coordinating with leftover cleanup jobs and pods that still reference it.
+// Deleting the PVC while such references exist leaves it stuck in a Terminating state
+// (the pvc-protection finalizer holds it as long as any pod uses it), which blocks new
+// workspaces in the namespace from starting.
+//
+// Leftover references are resolved as follows:
+//   - finished cleanup jobs (completed or failed), and jobs whose owner workspace no
+//     longer exists, are deleted with background propagation so their pods are removed
+//     with them;
+//   - jobs that are still running for an existing workspace are awaited, as their
+//     owning workspace's finalize will complete them;
+//   - orphaned pods (no owner references) are deleted directly, as nothing else will
+//     ever delete them;
+//   - pods that are still owned are awaited, as their owner chain will remove them.
+//
+// If any reference is deleted, a RetryError is returned so that the PVC is only removed
+// on a subsequent reconcile that confirms no references remain.
+// Generated by Claude Code
+func deleteCommonPVC(workspace *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) error {
+	namespace := workspace.Namespace
+
+	_, pvcName, err := checkForAlternatePVC(namespace, clusterAPI)
+	if err != nil {
+		return err
+	}
+	if pvcName == "" {
+		pvcName = workspace.Config.Workspace.PVCName
+	}
+
+	labelSelector, err := labels.Parse(constants.DevWorkspaceIDLabel)
+	if err != nil {
+		return err
+	}
+
+	// Resolve leftover cleanup jobs that reference the PVC.
+	jobList := &batchv1.JobList{}
+	if err := clusterAPI.Client.List(clusterAPI.Ctx, jobList, &k8sclient.ListOptions{
+		Namespace:     namespace,
+		LabelSelector: labelSelector,
+	}); err != nil {
+		return err
+	}
+	deletedReferences := false
+	for _, job := range jobList.Items {
+		if !podSpecMountsPVC(job.Spec.Template.Spec, pvcName) {
+			continue
+		}
+		if jobFinished(&job) || !jobOwnerWorkspaceExists(&job, namespace, clusterAPI) {
+			if err := clusterAPI.Client.Delete(clusterAPI.Ctx, &job,
+				k8sclient.PropagationPolicy(metav1.DeletePropagationBackground)); err != nil && !k8sErrors.IsNotFound(err) {
+				return err
+			}
+			deletedReferences = true
+		} else {
+			return &dwerrors.RetryError{
+				Message:      fmt.Sprintf("Waiting for cleanup job %s to complete before deleting PVC %s", job.Name, pvcName),
+				RequeueAfter: 10 * time.Second,
+			}
+		}
+	}
+
+	// Resolve leftover pods that reference the PVC.
+	podList := &corev1.PodList{}
+	if err := clusterAPI.Client.List(clusterAPI.Ctx, podList, &k8sclient.ListOptions{
+		Namespace:     namespace,
+		LabelSelector: labelSelector,
+	}); err != nil {
+		return err
+	}
+	for _, pod := range podList.Items {
+		if !podSpecMountsPVC(pod.Spec, pvcName) {
+			continue
+		}
+		if len(pod.OwnerReferences) == 0 {
+			if err := clusterAPI.Client.Delete(clusterAPI.Ctx, &pod); err != nil && !k8sErrors.IsNotFound(err) {
+				return err
+			}
+			deletedReferences = true
+		} else {
+			return &dwerrors.RetryError{
+				Message:      fmt.Sprintf("Waiting for pod %s using PVC %s to be deleted before removing the PVC", pod.Name, pvcName),
+				RequeueAfter: 10 * time.Second,
+			}
+		}
+	}
+
+	if deletedReferences {
+		// Requeue to confirm on the next reconcile that no references remain before
+		// deleting the PVC.
+		return &dwerrors.RetryError{
+			Message:      fmt.Sprintf("Deleted leftover cleanup jobs/pods using PVC %s", pvcName),
+			RequeueAfter: 10 * time.Second,
+		}
+	}
+
+	sharedPVC := &corev1.PersistentVolumeClaim{}
+	namespacedName := types.NamespacedName{Name: pvcName, Namespace: namespace}
+	err = clusterAPI.Client.Get(clusterAPI.Ctx, namespacedName, sharedPVC)
+	if err != nil {
+		if k8sErrors.IsNotFound(err) {
+			return nil
+		}
+		return err
+	}
+
+	err = clusterAPI.Client.Delete(clusterAPI.Ctx, sharedPVC)
+	if err != nil && !k8sErrors.IsNotFound(err) {
+		return err
+	}
+	return nil
+}
+
+// podSpecMountsPVC returns whether the pod spec defines a volume for the given PVC.
+func podSpecMountsPVC(spec corev1.PodSpec, pvcName string) bool {
+	for _, volume := range spec.Volumes {
+		if volume.PersistentVolumeClaim != nil && volume.PersistentVolumeClaim.ClaimName == pvcName {
+			return true
+		}
+	}
+	return false
+}
+
+// jobFinished returns whether the job has reached a terminal state (JobComplete or
+// JobFailed). A failed job never completes, so waiting for it would wedge the PVC
+// deletion; it is treated as finished and deleted instead.
+func jobFinished(job *batchv1.Job) bool {
+	for _, condition := range job.Status.Conditions {
+		if (condition.Type == batchv1.JobComplete || condition.Type == batchv1.JobFailed) && condition.Status == corev1.ConditionTrue {
+			return true
+		}
+	}
+	return false
+}
+
+// jobOwnerWorkspaceExists returns whether the workspace controlling the job still
+// exists in the namespace. On list errors, true is returned so that an in-flight job
+// is never deleted when its ownership cannot be determined.
+func jobOwnerWorkspaceExists(job *batchv1.Job, namespace string, clusterAPI sync.ClusterAPI) bool {
+	controllerRef := metav1.GetControllerOf(job)
+	if controllerRef == nil || controllerRef.Kind != "DevWorkspace" {
+		// Job is not controlled by a DevWorkspace, so nothing will complete it.
+		return false
+	}
+	workspaces := &dw.DevWorkspaceList{}
+	if err := clusterAPI.Client.List(clusterAPI.Ctx, workspaces, &k8sclient.ListOptions{Namespace: namespace}); err != nil {
+		return true
+	}
+	for _, workspace := range workspaces.Items {
+		if workspace.UID == controllerRef.UID {
+			return true
+		}
+	}
+	return false
+}