From 36faeb370d2b3ad59d45a42e62f6d14bc5287db2 Mon Sep 17 00:00:00 2001 From: Anatolii Bazko Date: Mon, 26 Oct 2026 15:00:13 +0100 Subject: [PATCH 01/10] fix: keep shared PVC from getting stuck Terminating on workspace cleanup Fixes a chain of defects that leaves the per-user PVC stuck in a Terminating state when the last workspace in a namespace is deleted (CRW-13586), blocking new workspaces until leftover cleanup pods are deleted manually: - FindNodeForPVC now ignores pods owned by Jobs: cleanup job pods carry the devworkspace_id label and mount the PVC themselves, so they caused spurious node affinity in the synced job spec and the job was deleted and recreated mid-run. - sync-layer deletions now use background propagation: on Kubernetes >= 1.33 deleting a Job without a propagation policy orphans its pods, and the orphaned completed pods held the PVC in Terminating forever. - deleting the last workspace now coordinates with leftover cleanup jobs and pods that reference the PVC: finished/orphaned jobs and orphaned pods are deleted, in-flight jobs and owned pods are awaited, and the PVC is only removed once no references remain. This also resolves the alternate PVC name before deletion. Adds an ADR documenting the decision and the alternatives considered. Assisted-by: Claude Code Co-Authored-By: Claude Code Signed-off-by: Anatolii Bazko --- ...-job-lifecycle-with-shared-pvc-deletion.md | 86 ++++++ pkg/provision/storage/cleanup.go | 158 +++++++++++ pkg/provision/storage/cleanup_test.go | 254 ++++++++++++++++++ pkg/provision/storage/commonStorage.go | 22 +- pkg/provision/storage/shared.go | 17 +- pkg/provision/storage/shared_test.go | 32 +++ pkg/provision/sync/sync.go | 19 +- pkg/provision/sync/sync_test.go | 99 +++++++ 8 files changed, 662 insertions(+), 25 deletions(-) create mode 100644 adr/2026-10-26-coordinate-cleanup-job-lifecycle-with-shared-pvc-deletion.md create mode 100644 pkg/provision/sync/sync_test.go diff --git a/adr/2026-10-26-coordinate-cleanup-job-lifecycle-with-shared-pvc-deletion.md b/adr/2026-10-26-coordinate-cleanup-job-lifecycle-with-shared-pvc-deletion.md new file mode 100644 index 000000000..8162549b0 --- /dev/null +++ b/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-` 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 diff --git a/pkg/provision/storage/cleanup.go b/pkg/provision/storage/cleanup.go index aaed82243..a888f0a13 100644 --- a/pkg/provision/storage/cleanup.go +++ b/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 +} diff --git a/pkg/provision/storage/cleanup_test.go b/pkg/provision/storage/cleanup_test.go index 9852cde15..2281f0ad4 100644 --- a/pkg/provision/storage/cleanup_test.go +++ b/pkg/provision/storage/cleanup_test.go @@ -21,14 +21,20 @@ import ( dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" "github.com/stretchr/testify/assert" + batchv1 "k8s.io/api/batch/v1" corev1 "k8s.io/api/core/v1" + 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/types" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/log/zap" "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" "github.com/devfile/devworkspace-operator/pkg/common" "github.com/devfile/devworkspace-operator/pkg/constants" + "github.com/devfile/devworkspace-operator/pkg/dwerrors" "github.com/devfile/devworkspace-operator/pkg/infrastructure" "github.com/devfile/devworkspace-operator/pkg/provision/sync" ) @@ -184,3 +190,251 @@ func TestGetSpecCommonPVCCleanupJobWithNilPodSecurityContext(t *testing.T) { assert.NoError(t, err) assert.Nil(t, job.Spec.Template.Spec.SecurityContext) } + +// makeTerminatingWorkspace returns a DevWorkspace that is being deleted, for use as the +// workspace whose storage cleanup is running. +func makeTerminatingWorkspace(name, namespace string) *dw.DevWorkspace { + now := metav1.Now() + return &dw.DevWorkspace{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + UID: types.UID(name + "-uid"), + DeletionTimestamp: &now, + Finalizers: []string{constants.StorageCleanupFinalizer}, + }, + Status: dw.DevWorkspaceStatus{ + DevWorkspaceId: name + "-id", + }, + } +} + +// makeCleanupJob returns a cleanup Job fixture labelled with the workspace ID label and +// mounting the given PVC. If ownerUID is set, the job is owned by the DevWorkspace with +// that UID; if complete is set, the job has a JobComplete condition. +func makeCleanupJob(name, namespace, workspaceId, pvcName, ownerUID string, complete bool) *batchv1.Job { + job := &batchv1.Job{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + Labels: map[string]string{constants.DevWorkspaceIDLabel: workspaceId}, + }, + Spec: batchv1.JobSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{{ + Name: pvcName, + VolumeSource: corev1.VolumeSource{ + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ClaimName: pvcName}, + }, + }}, + }, + }, + }, + } + if ownerUID != "" { + job.OwnerReferences = []metav1.OwnerReference{{ + APIVersion: "workspace.devfile.io/v1alpha2", + Kind: "DevWorkspace", + Name: "owner-workspace", + UID: types.UID(ownerUID), + Controller: func() *bool { b := true; return &b }(), + }} + } + if complete { + job.Status.Conditions = []batchv1.JobCondition{{ + Type: batchv1.JobComplete, + Status: corev1.ConditionTrue, + }} + } + return job +} + +// makeCleanupPod returns a pod fixture labelled with the workspace ID label and mounting +// the given PVC. If ownerUID is set, the pod is owned by the Job with that UID. +func makeCleanupPod(name, namespace, workspaceId, pvcName, ownerUID string) *corev1.Pod { + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + Labels: map[string]string{constants.DevWorkspaceIDLabel: workspaceId}, + }, + Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{{ + Name: pvcName, + VolumeSource: corev1.VolumeSource{ + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ClaimName: pvcName}, + }, + }}, + }, + Status: corev1.PodStatus{Phase: corev1.PodSucceeded}, + } + if ownerUID != "" { + pod.OwnerReferences = []metav1.OwnerReference{{ + APIVersion: "batch/v1", + Kind: "Job", + Name: "owner-job", + UID: types.UID(ownerUID), + }} + } + return pod +} + +func TestCleanupWorkspaceStorageCoordinatesSharedPVCDeletion(t *testing.T) { + const namespace = "test-ns" + const pvcName = "claim-devworkspace" + + makeClusterAPI := func(objects ...client.Object) (sync.ClusterAPI, client.Client) { + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(objects...). + Build() + return sync.ClusterAPI{ + Client: fakeClient, + Scheme: scheme, + Logger: zap.New(zap.UseDevMode(true)), + Ctx: context.Background(), + }, fakeClient + } + + makeWorkspaceWithConfig := func(workspace *dw.DevWorkspace) *common.DevWorkspaceWithConfig { + return &common.DevWorkspaceWithConfig{ + DevWorkspace: workspace, + Config: &v1alpha1.OperatorConfiguration{ + Workspace: &v1alpha1.WorkspaceConfig{PVCName: pvcName}, + }, + } + } + + getPVC := func(cli client.Client) error { + return cli.Get(context.Background(), types.NamespacedName{Name: pvcName, Namespace: namespace}, &corev1.PersistentVolumeClaim{}) + } + + makeSharedPVC := func() *corev1.PersistentVolumeClaim { + pvc, err := getPVCSpec(pvcName, namespace, nil, resource.MustParse("10Gi"), nil) + if err != nil { + t.Fatal(err) + } + return pvc + } + + t.Run("deletes PVC when no jobs or pods reference it", func(t *testing.T) { + currentWorkspace := makeTerminatingWorkspace("ws-current", namespace) + clusterAPI, fakeClient := makeClusterAPI(makeSharedPVC(), currentWorkspace) + + err := (&CommonStorageProvisioner{}).CleanupWorkspaceStorage(makeWorkspaceWithConfig(currentWorkspace), clusterAPI) + assert.NoError(t, err) + assert.True(t, k8sErrors.IsNotFound(getPVC(fakeClient)), "PVC should be deleted") + }) + + t.Run("waits for in-flight cleanup job owned by another workspace", func(t *testing.T) { + currentWorkspace := makeTerminatingWorkspace("ws-current", namespace) + otherWorkspace := makeTerminatingWorkspace("ws-other", namespace) + inFlightJob := makeCleanupJob("cleanup-other", namespace, "ws-other-id", pvcName, "ws-other-uid", false) + clusterAPI, fakeClient := makeClusterAPI(makeSharedPVC(), currentWorkspace, otherWorkspace, inFlightJob) + + err := (&CommonStorageProvisioner{}).CleanupWorkspaceStorage(makeWorkspaceWithConfig(currentWorkspace), clusterAPI) + + var retryErr *dwerrors.RetryError + assert.ErrorAs(t, err, &retryErr, "Should return RetryError while cleanup job is in flight") + assert.True(t, k8sErrors.IsNotFound(getPVC(fakeClient)) == false, "PVC should still exist") + assert.NoError(t, fakeClient.Get(context.Background(), types.NamespacedName{Name: "cleanup-other", Namespace: namespace}, &batchv1.Job{}), + "In-flight job should not be deleted") + }) + + t.Run("deletes completed cleanup job and waits for its pod before deleting PVC", func(t *testing.T) { + currentWorkspace := makeTerminatingWorkspace("ws-current", namespace) + orphanedWorkspace := makeTerminatingWorkspace("ws-gone", namespace) + completedJob := makeCleanupJob("cleanup-gone", namespace, "ws-gone-id", pvcName, "ws-gone-uid", true) + jobPod := makeCleanupPod("cleanup-gone-pod", namespace, "ws-gone-id", pvcName, "cleanup-gone-uid") + clusterAPI, fakeClient := makeClusterAPI(makeSharedPVC(), currentWorkspace, orphanedWorkspace, completedJob, jobPod) + + err := (&CommonStorageProvisioner{}).CleanupWorkspaceStorage(makeWorkspaceWithConfig(currentWorkspace), clusterAPI) + + // Completed job is deleted with background propagation, but its pod still + // references the PVC, so the PVC deletion has to wait for the pod to go away. + var retryErr *dwerrors.RetryError + assert.ErrorAs(t, err, &retryErr, "Should return RetryError while cleanup job pod still exists") + assert.True(t, k8sErrors.IsNotFound(fakeClient.Get(context.Background(), + types.NamespacedName{Name: "cleanup-gone", Namespace: namespace}, &batchv1.Job{})), "Completed job should be deleted") + assert.True(t, k8sErrors.IsNotFound(getPVC(fakeClient)) == false, "PVC should still exist") + + // Once the pod is gone (e.g. removed by GC), the PVC can be deleted. + assert.NoError(t, fakeClient.Delete(context.Background(), jobPod)) + err = (&CommonStorageProvisioner{}).CleanupWorkspaceStorage(makeWorkspaceWithConfig(currentWorkspace), clusterAPI) + assert.NoError(t, err) + assert.True(t, k8sErrors.IsNotFound(getPVC(fakeClient)), "PVC should be deleted once no pods reference it") + }) + + t.Run("deletes orphaned cleanup pods before deleting PVC", func(t *testing.T) { + currentWorkspace := makeTerminatingWorkspace("ws-current", namespace) + orphanedPod := makeCleanupPod("orphaned-cleanup-pod", namespace, "ws-gone-id", pvcName, "") + clusterAPI, fakeClient := makeClusterAPI(makeSharedPVC(), currentWorkspace, orphanedPod) + + err := (&CommonStorageProvisioner{}).CleanupWorkspaceStorage(makeWorkspaceWithConfig(currentWorkspace), clusterAPI) + + // The orphaned pod is deleted, but the PVC deletion is requeued to confirm on + // the next reconcile that no references remain. + var retryErr *dwerrors.RetryError + assert.ErrorAs(t, err, &retryErr, "Should return RetryError after deleting orphaned pod") + assert.True(t, k8sErrors.IsNotFound(fakeClient.Get(context.Background(), + types.NamespacedName{Name: "orphaned-cleanup-pod", Namespace: namespace}, &corev1.Pod{})), "Orphaned pod should be deleted") + assert.True(t, k8sErrors.IsNotFound(getPVC(fakeClient)) == false, "PVC should still exist") + + err = (&CommonStorageProvisioner{}).CleanupWorkspaceStorage(makeWorkspaceWithConfig(currentWorkspace), clusterAPI) + assert.NoError(t, err) + assert.True(t, k8sErrors.IsNotFound(getPVC(fakeClient)), "PVC should be deleted once no pods reference it") + }) + + t.Run("deletes in-flight cleanup job whose owner workspace is gone", func(t *testing.T) { + currentWorkspace := makeTerminatingWorkspace("ws-current", namespace) + // Job is incomplete and its owner workspace no longer exists, so nothing will + // ever complete it: it has to be deleted to unblock the PVC deletion. + orphanedJob := makeCleanupJob("cleanup-orphaned", namespace, "ws-gone-id", pvcName, "ws-gone-uid", false) + clusterAPI, fakeClient := makeClusterAPI(makeSharedPVC(), currentWorkspace, orphanedJob) + + err := (&CommonStorageProvisioner{}).CleanupWorkspaceStorage(makeWorkspaceWithConfig(currentWorkspace), clusterAPI) + + var retryErr *dwerrors.RetryError + assert.ErrorAs(t, err, &retryErr, "Should return RetryError until the deleted job's pods are gone") + assert.True(t, k8sErrors.IsNotFound(fakeClient.Get(context.Background(), + types.NamespacedName{Name: "cleanup-orphaned", Namespace: namespace}, &batchv1.Job{})), "Orphaned job should be deleted") + assert.True(t, k8sErrors.IsNotFound(getPVC(fakeClient)) == false, "PVC should still exist") + }) + + t.Run("deletes failed cleanup job even though its owner workspace still exists", func(t *testing.T) { + currentWorkspace := makeTerminatingWorkspace("ws-current", namespace) + otherWorkspace := makeTerminatingWorkspace("ws-other", namespace) + failedJob := makeCleanupJob("cleanup-failed", namespace, "ws-other-id", pvcName, "ws-other-uid", false) + // A failed job never completes, so waiting for it would wedge the PVC deletion. + failedJob.Status.Conditions = []batchv1.JobCondition{{ + Type: batchv1.JobFailed, + Status: corev1.ConditionTrue, + }} + clusterAPI, fakeClient := makeClusterAPI(makeSharedPVC(), currentWorkspace, otherWorkspace, failedJob) + + err := (&CommonStorageProvisioner{}).CleanupWorkspaceStorage(makeWorkspaceWithConfig(currentWorkspace), clusterAPI) + + var retryErr *dwerrors.RetryError + assert.ErrorAs(t, err, &retryErr, "Should return RetryError after deleting the failed job") + assert.True(t, k8sErrors.IsNotFound(fakeClient.Get(context.Background(), + types.NamespacedName{Name: "cleanup-failed", Namespace: namespace}, &batchv1.Job{})), "Failed job should be deleted") + assert.True(t, k8sErrors.IsNotFound(getPVC(fakeClient)) == false, "PVC should still exist") + }) + + t.Run("ignores jobs and pods that do not mount the shared PVC", func(t *testing.T) { + currentWorkspace := makeTerminatingWorkspace("ws-current", namespace) + otherJob := makeCleanupJob("async-job", namespace, "ws-other-id", "claim-async", "ws-other-uid", false) + otherPod := makeCleanupPod("async-pod", namespace, "ws-other-id", "claim-async", "") + clusterAPI, fakeClient := makeClusterAPI(makeSharedPVC(), currentWorkspace, otherJob, otherPod) + + err := (&CommonStorageProvisioner{}).CleanupWorkspaceStorage(makeWorkspaceWithConfig(currentWorkspace), clusterAPI) + + assert.NoError(t, err) + assert.True(t, k8sErrors.IsNotFound(getPVC(fakeClient)), "PVC should be deleted") + assert.NoError(t, fakeClient.Get(context.Background(), types.NamespacedName{Name: "async-job", Namespace: namespace}, &batchv1.Job{}), + "Job mounting another PVC should not be deleted") + assert.NoError(t, fakeClient.Get(context.Background(), types.NamespacedName{Name: "async-pod", Namespace: namespace}, &corev1.Pod{}), + "Pod mounting another PVC should not be deleted") + }) +} diff --git a/pkg/provision/storage/commonStorage.go b/pkg/provision/storage/commonStorage.go index c95c93bf3..3a02fe4f7 100644 --- a/pkg/provision/storage/commonStorage.go +++ b/pkg/provision/storage/commonStorage.go @@ -25,10 +25,6 @@ import ( "github.com/devfile/devworkspace-operator/pkg/library/overrides/restrictions" "github.com/devfile/devworkspace-operator/pkg/provision/sync" - corev1 "k8s.io/api/core/v1" - k8sErrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/types" - "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" ) @@ -106,24 +102,8 @@ func (p *CommonStorageProvisioner) CleanupWorkspaceStorage(workspace *common.Dev if totalWorkspaces > 0 { return runCommonPVCCleanupJob(workspace, clusterAPI) } else { - sharedPVC := &corev1.PersistentVolumeClaim{} - namespacedName := types.NamespacedName{Name: workspace.Config.Workspace.PVCName, Namespace: workspace.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 deleteCommonPVC(workspace, clusterAPI) } - - return nil } // rewriteContainerVolumeMounts rewrites the VolumeMounts in a set of PodAdditions according to the 'common' PVC strategy diff --git a/pkg/provision/storage/shared.go b/pkg/provision/storage/shared.go index 90903dfa8..528dd7153 100644 --- a/pkg/provision/storage/shared.go +++ b/pkg/provision/storage/shared.go @@ -310,9 +310,14 @@ func NodeAffinityForHostname(nodeName string) *corev1.Affinity { } } +// getNodeNameWithPVC returns the node name of the first running pod in the list that +// mounts the specified PVC. Pods owned by Jobs are ignored: cleanup and backup job pods +// mount the PVC themselves but do not indicate where workspace pods are scheduled, and +// matching them leads to spurious node affinity changes in synced Job specs. +// Generated by Claude Code func getNodeNameWithPVC(list *corev1.PodList, pvcName string) string { for _, pod := range list.Items { - if pod.Status.Phase == corev1.PodRunning { + if pod.Status.Phase == corev1.PodRunning && !isOwnedByJob(&pod) { for _, volume := range pod.Spec.Volumes { if volume.PersistentVolumeClaim != nil && volume.PersistentVolumeClaim.ClaimName == pvcName { return pod.Spec.NodeName @@ -323,6 +328,16 @@ func getNodeNameWithPVC(list *corev1.PodList, pvcName string) string { return "" } +// isOwnedByJob returns whether the pod has an ownerReference to a Job. +func isOwnedByJob(pod *corev1.Pod) bool { + for _, ownerRef := range pod.OwnerReferences { + if ownerRef.Kind == "Job" { + return true + } + } + return false +} + func checkPVCTerminating(name, namespace string, api sync.ClusterAPI) (bool, error) { if name == "" { // Should not happen diff --git a/pkg/provision/storage/shared_test.go b/pkg/provision/storage/shared_test.go index 2d2b7ef50..3bd22d513 100644 --- a/pkg/provision/storage/shared_test.go +++ b/pkg/provision/storage/shared_test.go @@ -167,6 +167,38 @@ func TestFindNodeForPVC(t *testing.T) { pvcName: "claim-devworkspace", expected: "", }, + { + name: "ignores running pods owned by Jobs", + pods: func() []*corev1.Pod { + // Cleanup job pods mount the PVC themselves, so they must not be + // used to determine where workspace storage is attached. + pod := makePod("cleanup-pod", "node-1", ns, corev1.PodRunning, pvcVolume("claim-devworkspace")) + pod.OwnerReferences = []metav1.OwnerReference{{ + APIVersion: "batch/v1", + Kind: "Job", + Name: "cleanup-workspace123", + UID: "job-uid", + }} + return []*corev1.Pod{pod} + }(), + pvcName: "claim-devworkspace", + expected: "", + }, + { + name: "considers running pods owned by ReplicaSets", + pods: func() []*corev1.Pod { + pod := makePod("workspace-pod", "node-2", ns, corev1.PodRunning, pvcVolume("claim-devworkspace")) + pod.OwnerReferences = []metav1.OwnerReference{{ + APIVersion: "apps/v1", + Kind: "ReplicaSet", + Name: "workspace123", + UID: "rs-uid", + }} + return []*corev1.Pod{pod} + }(), + pvcName: "claim-devworkspace", + expected: "node-2", + }, } for _, tt := range tests { diff --git a/pkg/provision/sync/sync.go b/pkg/provision/sync/sync.go index 25cfd9a36..2963ffafb 100644 --- a/pkg/provision/sync/sync.go +++ b/pkg/provision/sync/sync.go @@ -14,6 +14,7 @@ package sync import ( + "context" "fmt" "reflect" @@ -27,6 +28,7 @@ import ( networkingv1 "k8s.io/api/networking/v1" rbacv1 "k8s.io/api/rbac/v1" k8sErrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" crclient "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -67,7 +69,7 @@ func SyncObjectWithCluster(specObj crclient.Object, api ClusterAPI) (crclient.Ob shouldDelete, shouldUpdate := diffFunc(specObj, clusterObj) if shouldDelete { printDiff(specObj, clusterObj, api.Logger) - err := api.Client.Delete(api.Ctx, specObj) + err := deleteObject(api.Ctx, api.Client, specObj) if err != nil { return nil, err } @@ -119,7 +121,7 @@ func SyncUnrecognizedObjectWithCluster(specObj crclient.Object, api ClusterAPI) } } if delete { - if err := api.Client.Delete(api.Ctx, clusterObj); err != nil { + if err := deleteObject(api.Ctx, api.Client, clusterObj); err != nil { if k8sErrors.IsForbidden(err) { return nil, &UnrecoverableSyncError{fmt.Errorf("failed to delete object %s: %w", clusterObj.GetName(), err)} } @@ -155,7 +157,7 @@ func updateObjectGeneric(specObj, clusterObj crclient.Object, api ClusterAPI) er updateFunc := getUpdateFunc(specObj) updatedObj, err := updateFunc(specObj, clusterObj) if err != nil { - if err := api.Client.Delete(api.Ctx, specObj); err != nil { + if err := deleteObject(api.Ctx, api.Client, specObj); err != nil { return err } api.Logger.Info("Deleted object", "kind", reflect.TypeOf(specObj).Elem().String(), "name", specObj.GetName()) @@ -186,6 +188,17 @@ func isMutableObject(obj crclient.Object) bool { } } +// deleteObject deletes an object from the cluster with background propagation. On +// Kubernetes >= 1.33, deleting a Job without an explicit propagation policy orphans its +// pods: the pods are left behind with their ownerReferences stripped, and nothing ever +// deletes them (e.g. completed cleanup job pods referencing a PVC keep the PVC stuck in +// a Terminating state). Background propagation ensures dependents are removed with the +// object, and matches the default GC behavior for other object kinds. +// Generated by Claude Code +func deleteObject(ctx context.Context, c crclient.Client, obj crclient.Object) error { + return c.Delete(ctx, obj, crclient.PropagationPolicy(metav1.DeletePropagationBackground)) +} + func printDiff(specObj, clusterObj crclient.Object, log logr.Logger) { if config.IsSetUp() && config.ExperimentalFeaturesEnabled() { var diffOpts cmp.Options diff --git a/pkg/provision/sync/sync_test.go b/pkg/provision/sync/sync_test.go new file mode 100644 index 000000000..4ac69afde --- /dev/null +++ b/pkg/provision/sync/sync_test.go @@ -0,0 +1,99 @@ +// Copyright (c) 2019-2026 Red Hat, Inc. +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package sync + +import ( + "context" + "testing" + + batchv1 "k8s.io/api/batch/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + "sigs.k8s.io/controller-runtime/pkg/log/zap" + + "github.com/stretchr/testify/assert" +) + +var syncTestScheme = runtime.NewScheme() + +func init() { + utilruntime.Must(clientgoscheme.AddToScheme(syncTestScheme)) +} + +// getRecordedDeleteOptions applies captured client delete options to a DeleteOptions +// struct so they can be inspected in tests. +func getRecordedDeleteOptions(opts []client.DeleteOption) *client.DeleteOptions { + deleteOptions := &client.DeleteOptions{} + for _, opt := range opts { + opt.ApplyToDelete(deleteOptions) + } + return deleteOptions +} + +func TestSyncObjectWithClusterDeletesJobsWithBackgroundPropagation(t *testing.T) { + clusterJob := &batchv1.Job{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cleanup-workspace123", + Namespace: "test-ns", + }, + Spec: batchv1.JobSpec{}, + } + // Spec job differs from the cluster job so that jobDiffFunc reports a delete: + // the node affinity gained by the spec job is not set on the cluster job. + specJob := clusterJob.DeepCopy() + parallelism := int32(1) + specJob.Spec.Parallelism = ¶llelism + + var capturedDeleteOptions []client.DeleteOption + fakeClient := fake.NewClientBuilder(). + WithScheme(syncTestScheme). + WithObjects(clusterJob). + WithInterceptorFuncs(interceptor.Funcs{ + Delete: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { + capturedDeleteOptions = opts + return c.Delete(ctx, obj, opts...) + }, + }). + Build() + + api := ClusterAPI{ + Client: fakeClient, + Logger: zap.New(), + Ctx: context.Background(), + } + + _, err := SyncObjectWithCluster(specJob, api) + + // Sync reports the object as out of sync after deleting it (it gets recreated on a + // subsequent reconcile). + var notInSyncErr *NotInSyncError + if assert.ErrorAs(t, err, ¬InSyncErr, "Should return NotInSyncError after deleting job") { + assert.Equal(t, DeletedObjectReason, notInSyncErr.Reason, "Job should be reported as deleted") + } + + // On Kubernetes >= 1.33, deleting a Job without a propagation policy orphans its + // pods: completed pods are left behind with their ownerReferences stripped, and + // nothing ever deletes them. Background propagation ensures job pods are removed + // with the job. + deleteOptions := getRecordedDeleteOptions(capturedDeleteOptions) + if assert.NotNil(t, deleteOptions.PropagationPolicy, "Job deletion should set a propagation policy") { + assert.Equal(t, metav1.DeletePropagationBackground, *deleteOptions.PropagationPolicy, + "Job deletion should use background propagation so pods are removed with the job") + } +} From 73ce3109cfb2649fb9534038630021e4e5a56520 Mon Sep 17 00:00:00 2001 From: Anatolii Bazko Date: Tue, 27 Oct 2026 15:43:39 +0100 Subject: [PATCH 02/10] fixup Signed-off-by: Anatolii Bazko --- ...-job-lifecycle-with-shared-pvc-deletion.md | 86 ---------------- pkg/provision/storage/cleanup.go | 1 - pkg/provision/storage/shared.go | 1 - pkg/provision/sync/sync_test.go | 99 ------------------- 4 files changed, 187 deletions(-) delete mode 100644 adr/2026-10-26-coordinate-cleanup-job-lifecycle-with-shared-pvc-deletion.md delete mode 100644 pkg/provision/sync/sync_test.go diff --git a/adr/2026-10-26-coordinate-cleanup-job-lifecycle-with-shared-pvc-deletion.md b/adr/2026-10-26-coordinate-cleanup-job-lifecycle-with-shared-pvc-deletion.md deleted file mode 100644 index 8162549b0..000000000 --- a/adr/2026-10-26-coordinate-cleanup-job-lifecycle-with-shared-pvc-deletion.md +++ /dev/null @@ -1,86 +0,0 @@ -# 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-` 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 diff --git a/pkg/provision/storage/cleanup.go b/pkg/provision/storage/cleanup.go index a888f0a13..e8e3742fd 100644 --- a/pkg/provision/storage/cleanup.go +++ b/pkg/provision/storage/cleanup.go @@ -251,7 +251,6 @@ func commonPVCExists(workspace *common.DevWorkspaceWithConfig, clusterAPI sync.C // // 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 diff --git a/pkg/provision/storage/shared.go b/pkg/provision/storage/shared.go index 528dd7153..cc16acf12 100644 --- a/pkg/provision/storage/shared.go +++ b/pkg/provision/storage/shared.go @@ -314,7 +314,6 @@ func NodeAffinityForHostname(nodeName string) *corev1.Affinity { // mounts the specified PVC. Pods owned by Jobs are ignored: cleanup and backup job pods // mount the PVC themselves but do not indicate where workspace pods are scheduled, and // matching them leads to spurious node affinity changes in synced Job specs. -// Generated by Claude Code func getNodeNameWithPVC(list *corev1.PodList, pvcName string) string { for _, pod := range list.Items { if pod.Status.Phase == corev1.PodRunning && !isOwnedByJob(&pod) { diff --git a/pkg/provision/sync/sync_test.go b/pkg/provision/sync/sync_test.go deleted file mode 100644 index 4ac69afde..000000000 --- a/pkg/provision/sync/sync_test.go +++ /dev/null @@ -1,99 +0,0 @@ -// Copyright (c) 2019-2026 Red Hat, Inc. -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package sync - -import ( - "context" - "testing" - - batchv1 "k8s.io/api/batch/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - utilruntime "k8s.io/apimachinery/pkg/util/runtime" - clientgoscheme "k8s.io/client-go/kubernetes/scheme" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/client/fake" - "sigs.k8s.io/controller-runtime/pkg/client/interceptor" - "sigs.k8s.io/controller-runtime/pkg/log/zap" - - "github.com/stretchr/testify/assert" -) - -var syncTestScheme = runtime.NewScheme() - -func init() { - utilruntime.Must(clientgoscheme.AddToScheme(syncTestScheme)) -} - -// getRecordedDeleteOptions applies captured client delete options to a DeleteOptions -// struct so they can be inspected in tests. -func getRecordedDeleteOptions(opts []client.DeleteOption) *client.DeleteOptions { - deleteOptions := &client.DeleteOptions{} - for _, opt := range opts { - opt.ApplyToDelete(deleteOptions) - } - return deleteOptions -} - -func TestSyncObjectWithClusterDeletesJobsWithBackgroundPropagation(t *testing.T) { - clusterJob := &batchv1.Job{ - ObjectMeta: metav1.ObjectMeta{ - Name: "cleanup-workspace123", - Namespace: "test-ns", - }, - Spec: batchv1.JobSpec{}, - } - // Spec job differs from the cluster job so that jobDiffFunc reports a delete: - // the node affinity gained by the spec job is not set on the cluster job. - specJob := clusterJob.DeepCopy() - parallelism := int32(1) - specJob.Spec.Parallelism = ¶llelism - - var capturedDeleteOptions []client.DeleteOption - fakeClient := fake.NewClientBuilder(). - WithScheme(syncTestScheme). - WithObjects(clusterJob). - WithInterceptorFuncs(interceptor.Funcs{ - Delete: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { - capturedDeleteOptions = opts - return c.Delete(ctx, obj, opts...) - }, - }). - Build() - - api := ClusterAPI{ - Client: fakeClient, - Logger: zap.New(), - Ctx: context.Background(), - } - - _, err := SyncObjectWithCluster(specJob, api) - - // Sync reports the object as out of sync after deleting it (it gets recreated on a - // subsequent reconcile). - var notInSyncErr *NotInSyncError - if assert.ErrorAs(t, err, ¬InSyncErr, "Should return NotInSyncError after deleting job") { - assert.Equal(t, DeletedObjectReason, notInSyncErr.Reason, "Job should be reported as deleted") - } - - // On Kubernetes >= 1.33, deleting a Job without a propagation policy orphans its - // pods: completed pods are left behind with their ownerReferences stripped, and - // nothing ever deletes them. Background propagation ensures job pods are removed - // with the job. - deleteOptions := getRecordedDeleteOptions(capturedDeleteOptions) - if assert.NotNil(t, deleteOptions.PropagationPolicy, "Job deletion should set a propagation policy") { - assert.Equal(t, metav1.DeletePropagationBackground, *deleteOptions.PropagationPolicy, - "Job deletion should use background propagation so pods are removed with the job") - } -} From c8a1632709626391f39bdf8a57d3589e6a506aa8 Mon Sep 17 00:00:00 2001 From: Anatolii Bazko Date: Tue, 27 Oct 2026 15:47:11 +0100 Subject: [PATCH 03/10] fixup Signed-off-by: Anatolii Bazko --- pkg/provision/sync/sync.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/pkg/provision/sync/sync.go b/pkg/provision/sync/sync.go index 2963ffafb..6f69a80c7 100644 --- a/pkg/provision/sync/sync.go +++ b/pkg/provision/sync/sync.go @@ -189,12 +189,7 @@ func isMutableObject(obj crclient.Object) bool { } // deleteObject deletes an object from the cluster with background propagation. On -// Kubernetes >= 1.33, deleting a Job without an explicit propagation policy orphans its -// pods: the pods are left behind with their ownerReferences stripped, and nothing ever -// deletes them (e.g. completed cleanup job pods referencing a PVC keep the PVC stuck in -// a Terminating state). Background propagation ensures dependents are removed with the -// object, and matches the default GC behavior for other object kinds. -// Generated by Claude Code +// Kubernetes >= 1.33, deleting a Job without an explicit propagation policy orphans its pods. func deleteObject(ctx context.Context, c crclient.Client, obj crclient.Object) error { return c.Delete(ctx, obj, crclient.PropagationPolicy(metav1.DeletePropagationBackground)) } From b7619e0b20cd9e6bac9eed2ad71d1b0946122ce4 Mon Sep 17 00:00:00 2001 From: Anatolii Bazko Date: Tue, 27 Oct 2026 15:51:36 +0100 Subject: [PATCH 04/10] fixup Signed-off-by: Anatolii Bazko --- pkg/provision/storage/shared.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/pkg/provision/storage/shared.go b/pkg/provision/storage/shared.go index cc16acf12..cca115bae 100644 --- a/pkg/provision/storage/shared.go +++ b/pkg/provision/storage/shared.go @@ -19,6 +19,7 @@ import ( "context" "errors" "fmt" + "slices" dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" "github.com/devfile/devworkspace-operator/pkg/common" @@ -329,12 +330,12 @@ func getNodeNameWithPVC(list *corev1.PodList, pvcName string) string { // isOwnedByJob returns whether the pod has an ownerReference to a Job. func isOwnedByJob(pod *corev1.Pod) bool { - for _, ownerRef := range pod.OwnerReferences { - if ownerRef.Kind == "Job" { - return true - } - } - return false + return slices.ContainsFunc( + pod.OwnerReferences, + func(ownerRef metav1.OwnerReference) bool { + return ownerRef.Kind == "Job" + }, + ) } func checkPVCTerminating(name, namespace string, api sync.ClusterAPI) (bool, error) { From 33276138c4ea96225e29fd71ce66d52f89c1b537 Mon Sep 17 00:00:00 2001 From: Anatolii Bazko Date: Tue, 27 Oct 2026 16:04:37 +0100 Subject: [PATCH 05/10] refactor: match workspace pods by controller owner instead of excluding Job pods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getNodeNameWithPVC now only considers pods whose controller owner is a ReplicaSet or Deployment (i.e. pods created for a DevWorkspace's deployment) instead of excluding pods owned by Jobs. Ownership cannot be resolved to the DevWorkspace itself, as cleanup Jobs are also controlled by the DevWorkspace — the immediate controller kind is the only local discriminator between job pods and workload pods. Unrelated or standalone pods with the DevWorkspace ID label are ignored as well. Assisted-by: Claude Code Co-Authored-By: Claude Code Signed-off-by: Anatolii Bazko --- pkg/provision/storage/cleanup_test.go | 7 ++++++ pkg/provision/storage/shared.go | 36 ++++++++++++++++----------- pkg/provision/storage/shared_test.go | 36 +++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 14 deletions(-) diff --git a/pkg/provision/storage/cleanup_test.go b/pkg/provision/storage/cleanup_test.go index 2281f0ad4..a9af8dffa 100644 --- a/pkg/provision/storage/cleanup_test.go +++ b/pkg/provision/storage/cleanup_test.go @@ -98,6 +98,13 @@ func TestGetSpecCommonPVCCleanupJobHasNodeAffinityWhenPodMountsPVC(t *testing.T) Name: "workspace-pod", Namespace: namespace, Labels: map[string]string{constants.DevWorkspaceIDLabel: "other-workspace-id"}, + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: "apps/v1", + Kind: "ReplicaSet", + Name: "other-workspace", + UID: "rs-uid", + Controller: func() *bool { b := true; return &b }(), + }}, }, Spec: corev1.PodSpec{NodeName: "worker-node-1", Volumes: []corev1.Volume{{Name: "data", VolumeSource: corev1.VolumeSource{PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ClaimName: pvcName}}}}}, Status: corev1.PodStatus{Phase: corev1.PodRunning}, diff --git a/pkg/provision/storage/shared.go b/pkg/provision/storage/shared.go index cca115bae..00c471c90 100644 --- a/pkg/provision/storage/shared.go +++ b/pkg/provision/storage/shared.go @@ -19,7 +19,6 @@ import ( "context" "errors" "fmt" - "slices" dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" "github.com/devfile/devworkspace-operator/pkg/common" @@ -311,13 +310,21 @@ func NodeAffinityForHostname(nodeName string) *corev1.Affinity { } } -// getNodeNameWithPVC returns the node name of the first running pod in the list that -// mounts the specified PVC. Pods owned by Jobs are ignored: cleanup and backup job pods -// mount the PVC themselves but do not indicate where workspace pods are scheduled, and -// matching them leads to spurious node affinity changes in synced Job specs. +// getNodeNameWithPVC returns the node name where the shared PVC is currently in use, +// determined from the first running pod in the list that mounts it. Only pods created +// for a DevWorkspace's Deployment are considered, i.e. pods whose controller owner is a +// ReplicaSet or Deployment (workspace pods and async-storage pods). +// +// Pods with any other owner (or no owner) are ignored. This matters for the cleanup and +// backup Jobs this lookup feeds into: job pods mount the PVC and carry the DevWorkspace +// ID label themselves, so matching one would report the job's own pod as the PVC's +// location. The job would then be recreated with node affinity it did not have before +// (workspace pods are already gone at cleanup time), the sync layer would report that +// drift and resolve it by deleting and recreating the job mid-run, and on Kubernetes +// >= 1.33 that deletion would orphan the job's pods. func getNodeNameWithPVC(list *corev1.PodList, pvcName string) string { for _, pod := range list.Items { - if pod.Status.Phase == corev1.PodRunning && !isOwnedByJob(&pod) { + if pod.Status.Phase == corev1.PodRunning && isOwnedByWorkspaceDeployment(&pod) { for _, volume := range pod.Spec.Volumes { if volume.PersistentVolumeClaim != nil && volume.PersistentVolumeClaim.ClaimName == pvcName { return pod.Spec.NodeName @@ -328,14 +335,15 @@ func getNodeNameWithPVC(list *corev1.PodList, pvcName string) string { return "" } -// isOwnedByJob returns whether the pod has an ownerReference to a Job. -func isOwnedByJob(pod *corev1.Pod) bool { - return slices.ContainsFunc( - pod.OwnerReferences, - func(ownerRef metav1.OwnerReference) bool { - return ownerRef.Kind == "Job" - }, - ) +// isOwnedByWorkspaceDeployment returns whether the pod's controller owner is a +// ReplicaSet or Deployment, i.e. whether it is a workload pod created for a DevWorkspace +// rather than a cleanup/backup Job pod or an unrelated pod. +func isOwnedByWorkspaceDeployment(pod *corev1.Pod) bool { + controllerRef := metav1.GetControllerOf(pod) + if controllerRef == nil { + return false + } + return controllerRef.Kind == "ReplicaSet" || controllerRef.Kind == "Deployment" } func checkPVCTerminating(name, namespace string, api sync.ClusterAPI) (bool, error) { diff --git a/pkg/provision/storage/shared_test.go b/pkg/provision/storage/shared_test.go index 3bd22d513..5aca05f89 100644 --- a/pkg/provision/storage/shared_test.go +++ b/pkg/provision/storage/shared_test.go @@ -97,6 +97,14 @@ func TestFindNodeForPVC(t *testing.T) { Name: name, Namespace: namespace, Labels: map[string]string{constants.DevWorkspaceIDLabel: "some-id"}, + // Workspace pods are created by the workspace Deployment via a ReplicaSet + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: "apps/v1", + Kind: "ReplicaSet", + Name: "some-workspace", + UID: "rs-uid", + Controller: func() *bool { b := true; return &b }(), + }}, }, Spec: corev1.PodSpec{NodeName: nodeName, Volumes: volumes}, Status: corev1.PodStatus{Phase: phase}, @@ -178,6 +186,7 @@ func TestFindNodeForPVC(t *testing.T) { Kind: "Job", Name: "cleanup-workspace123", UID: "job-uid", + Controller: func() *bool { b := true; return &b }(), }} return []*corev1.Pod{pod} }(), @@ -193,12 +202,39 @@ func TestFindNodeForPVC(t *testing.T) { Kind: "ReplicaSet", Name: "workspace123", UID: "rs-uid", + Controller: func() *bool { b := true; return &b }(), }} return []*corev1.Pod{pod} }(), pvcName: "claim-devworkspace", expected: "node-2", }, + { + name: "ignores pods without owner references", + pods: func() []*corev1.Pod { + pod := makePod("standalone-pod", "node-1", ns, corev1.PodRunning, pvcVolume("claim-devworkspace")) + pod.OwnerReferences = nil + return []*corev1.Pod{pod} + }(), + pvcName: "claim-devworkspace", + expected: "", + }, + { + name: "ignores pods owned by StatefulSets", + pods: func() []*corev1.Pod { + pod := makePod("statefulset-pod", "node-1", ns, corev1.PodRunning, pvcVolume("claim-devworkspace")) + pod.OwnerReferences = []metav1.OwnerReference{{ + APIVersion: "apps/v1", + Kind: "StatefulSet", + Name: "not-a-workspace", + UID: "sts-uid", + Controller: func() *bool { b := true; return &b }(), + }} + return []*corev1.Pod{pod} + }(), + pvcName: "claim-devworkspace", + expected: "", + }, } for _, tt := range tests { From 02a8b01cb16f3d84320ff1cb3e09efb1d4c727a7 Mon Sep 17 00:00:00 2001 From: Anatolii Bazko Date: Tue, 27 Oct 2026 16:06:40 +0100 Subject: [PATCH 06/10] fixup Signed-off-by: Anatolii Bazko --- pkg/provision/storage/shared.go | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/pkg/provision/storage/shared.go b/pkg/provision/storage/shared.go index 00c471c90..94275a6c1 100644 --- a/pkg/provision/storage/shared.go +++ b/pkg/provision/storage/shared.go @@ -314,14 +314,6 @@ func NodeAffinityForHostname(nodeName string) *corev1.Affinity { // determined from the first running pod in the list that mounts it. Only pods created // for a DevWorkspace's Deployment are considered, i.e. pods whose controller owner is a // ReplicaSet or Deployment (workspace pods and async-storage pods). -// -// Pods with any other owner (or no owner) are ignored. This matters for the cleanup and -// backup Jobs this lookup feeds into: job pods mount the PVC and carry the DevWorkspace -// ID label themselves, so matching one would report the job's own pod as the PVC's -// location. The job would then be recreated with node affinity it did not have before -// (workspace pods are already gone at cleanup time), the sync layer would report that -// drift and resolve it by deleting and recreating the job mid-run, and on Kubernetes -// >= 1.33 that deletion would orphan the job's pods. func getNodeNameWithPVC(list *corev1.PodList, pvcName string) string { for _, pod := range list.Items { if pod.Status.Phase == corev1.PodRunning && isOwnedByWorkspaceDeployment(&pod) { @@ -335,9 +327,6 @@ func getNodeNameWithPVC(list *corev1.PodList, pvcName string) string { return "" } -// isOwnedByWorkspaceDeployment returns whether the pod's controller owner is a -// ReplicaSet or Deployment, i.e. whether it is a workload pod created for a DevWorkspace -// rather than a cleanup/backup Job pod or an unrelated pod. func isOwnedByWorkspaceDeployment(pod *corev1.Pod) bool { controllerRef := metav1.GetControllerOf(pod) if controllerRef == nil { From 10a14e109d8612c9f8d313b2f97605db248e1d01 Mon Sep 17 00:00:00 2001 From: Anatolii Bazko Date: Tue, 27 Oct 2026 16:57:44 +0100 Subject: [PATCH 07/10] fixup Signed-off-by: Anatolii Bazko --- pkg/provision/storage/cleanup.go | 98 +++++--------------------- pkg/provision/storage/commonStorage.go | 3 +- 2 files changed, 17 insertions(+), 84 deletions(-) diff --git a/pkg/provision/storage/cleanup.go b/pkg/provision/storage/cleanup.go index e8e3742fd..af0e1dc9d 100644 --- a/pkg/provision/storage/cleanup.go +++ b/pkg/provision/storage/cleanup.go @@ -18,9 +18,9 @@ package storage import ( "fmt" "path" + "slices" "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" @@ -267,33 +267,6 @@ func deleteCommonPVC(workspace *common.DevWorkspaceWithConfig, clusterAPI sync.C 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{ @@ -303,34 +276,28 @@ func deleteCommonPVC(workspace *common.DevWorkspaceWithConfig, clusterAPI sync.C return err } for _, pod := range podList.Items { - if !podSpecMountsPVC(pod.Spec, pvcName) { + if !isPVCMounted(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, + Message: fmt.Sprintf("Deleted leftover cleanup jobs/pods using PVC %s", pvcName), + RequeueAfter: 3 * 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), + Message: fmt.Sprintf("Waiting for pod %s using PVC %s to be deleted before removing the PVC", pod.Name, pvcName), RequeueAfter: 10 * time.Second, } } - sharedPVC := &corev1.PersistentVolumeClaim{} + pvc := &corev1.PersistentVolumeClaim{} namespacedName := types.NamespacedName{Name: pvcName, Namespace: namespace} - err = clusterAPI.Client.Get(clusterAPI.Ctx, namespacedName, sharedPVC) + err = clusterAPI.Client.Get(clusterAPI.Ctx, namespacedName, pvc) if err != nil { if k8sErrors.IsNotFound(err) { return nil @@ -338,52 +305,19 @@ func deleteCommonPVC(workspace *common.DevWorkspaceWithConfig, clusterAPI sync.C return err } - err = clusterAPI.Client.Delete(clusterAPI.Ctx, sharedPVC) + err = clusterAPI.Client.Delete(clusterAPI.Ctx, pvc) 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 +// isPVCMounted returns whether the pod spec defines a volume for the given PVC. +func isPVCMounted(spec corev1.PodSpec, pvcName string) bool { + return slices.ContainsFunc( + spec.Volumes, + func(volume corev1.Volume) bool { + return volume.PersistentVolumeClaim != nil && volume.PersistentVolumeClaim.ClaimName == pvcName + }, + ) } diff --git a/pkg/provision/storage/commonStorage.go b/pkg/provision/storage/commonStorage.go index 3a02fe4f7..8223ae990 100644 --- a/pkg/provision/storage/commonStorage.go +++ b/pkg/provision/storage/commonStorage.go @@ -101,9 +101,8 @@ func (p *CommonStorageProvisioner) CleanupWorkspaceStorage(workspace *common.Dev // delete common PVC instead of running cleanup job if totalWorkspaces > 0 { return runCommonPVCCleanupJob(workspace, clusterAPI) - } else { - return deleteCommonPVC(workspace, clusterAPI) } + return deleteCommonPVC(workspace, clusterAPI) } // rewriteContainerVolumeMounts rewrites the VolumeMounts in a set of PodAdditions according to the 'common' PVC strategy From 6ee556b4767991ab81b8b2fea82d7fbf1fca9e3e Mon Sep 17 00:00:00 2001 From: Anatolii Bazko Date: Tue, 27 Oct 2026 16:58:52 +0100 Subject: [PATCH 08/10] fixup Signed-off-by: Anatolii Bazko --- pkg/provision/storage/cleanup.go | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/pkg/provision/storage/cleanup.go b/pkg/provision/storage/cleanup.go index af0e1dc9d..0cb9b0864 100644 --- a/pkg/provision/storage/cleanup.go +++ b/pkg/provision/storage/cleanup.go @@ -234,23 +234,10 @@ func commonPVCExists(workspace *common.DevWorkspaceWithConfig, clusterAPI sync.C } // 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. +// namespace, coordinating with leftover cleanup 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. func deleteCommonPVC(workspace *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) error { namespace := workspace.Namespace From 3945bdd07359f763b4d46d6d4599a9873007d49e Mon Sep 17 00:00:00 2001 From: Anatolii Bazko Date: Tue, 27 Oct 2026 17:14:53 +0100 Subject: [PATCH 09/10] fixup Signed-off-by: Anatolii Bazko --- pkg/provision/storage/cleanup.go | 35 ++++++++++++++++++++++++-- pkg/provision/storage/cleanup_test.go | 36 --------------------------- 2 files changed, 33 insertions(+), 38 deletions(-) diff --git a/pkg/provision/storage/cleanup.go b/pkg/provision/storage/cleanup.go index 0cb9b0864..32a85c9a9 100644 --- a/pkg/provision/storage/cleanup.go +++ b/pkg/provision/storage/cleanup.go @@ -234,10 +234,23 @@ func commonPVCExists(workspace *common.DevWorkspaceWithConfig, clusterAPI sync.C } // deleteCommonPVC deletes the shared PVC used for workspace storage in the workspace's -// namespace, coordinating with leftover cleanup pods that still reference it. +// 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. func deleteCommonPVC(workspace *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) error { namespace := workspace.Namespace @@ -254,6 +267,24 @@ func deleteCommonPVC(workspace *common.DevWorkspaceWithConfig, clusterAPI sync.C 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 + } + for _, job := range jobList.Items { + if !isPVCMounted(job.Spec.Template.Spec, pvcName) { + continue + } + 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{ @@ -272,7 +303,7 @@ func deleteCommonPVC(workspace *common.DevWorkspaceWithConfig, clusterAPI sync.C } return &dwerrors.RetryError{ Message: fmt.Sprintf("Deleted leftover cleanup jobs/pods using PVC %s", pvcName), - RequeueAfter: 3 * time.Second, + RequeueAfter: 10 * time.Second, } } diff --git a/pkg/provision/storage/cleanup_test.go b/pkg/provision/storage/cleanup_test.go index a9af8dffa..14aa9f16f 100644 --- a/pkg/provision/storage/cleanup_test.go +++ b/pkg/provision/storage/cleanup_test.go @@ -393,42 +393,6 @@ func TestCleanupWorkspaceStorageCoordinatesSharedPVCDeletion(t *testing.T) { assert.True(t, k8sErrors.IsNotFound(getPVC(fakeClient)), "PVC should be deleted once no pods reference it") }) - t.Run("deletes in-flight cleanup job whose owner workspace is gone", func(t *testing.T) { - currentWorkspace := makeTerminatingWorkspace("ws-current", namespace) - // Job is incomplete and its owner workspace no longer exists, so nothing will - // ever complete it: it has to be deleted to unblock the PVC deletion. - orphanedJob := makeCleanupJob("cleanup-orphaned", namespace, "ws-gone-id", pvcName, "ws-gone-uid", false) - clusterAPI, fakeClient := makeClusterAPI(makeSharedPVC(), currentWorkspace, orphanedJob) - - err := (&CommonStorageProvisioner{}).CleanupWorkspaceStorage(makeWorkspaceWithConfig(currentWorkspace), clusterAPI) - - var retryErr *dwerrors.RetryError - assert.ErrorAs(t, err, &retryErr, "Should return RetryError until the deleted job's pods are gone") - assert.True(t, k8sErrors.IsNotFound(fakeClient.Get(context.Background(), - types.NamespacedName{Name: "cleanup-orphaned", Namespace: namespace}, &batchv1.Job{})), "Orphaned job should be deleted") - assert.True(t, k8sErrors.IsNotFound(getPVC(fakeClient)) == false, "PVC should still exist") - }) - - t.Run("deletes failed cleanup job even though its owner workspace still exists", func(t *testing.T) { - currentWorkspace := makeTerminatingWorkspace("ws-current", namespace) - otherWorkspace := makeTerminatingWorkspace("ws-other", namespace) - failedJob := makeCleanupJob("cleanup-failed", namespace, "ws-other-id", pvcName, "ws-other-uid", false) - // A failed job never completes, so waiting for it would wedge the PVC deletion. - failedJob.Status.Conditions = []batchv1.JobCondition{{ - Type: batchv1.JobFailed, - Status: corev1.ConditionTrue, - }} - clusterAPI, fakeClient := makeClusterAPI(makeSharedPVC(), currentWorkspace, otherWorkspace, failedJob) - - err := (&CommonStorageProvisioner{}).CleanupWorkspaceStorage(makeWorkspaceWithConfig(currentWorkspace), clusterAPI) - - var retryErr *dwerrors.RetryError - assert.ErrorAs(t, err, &retryErr, "Should return RetryError after deleting the failed job") - assert.True(t, k8sErrors.IsNotFound(fakeClient.Get(context.Background(), - types.NamespacedName{Name: "cleanup-failed", Namespace: namespace}, &batchv1.Job{})), "Failed job should be deleted") - assert.True(t, k8sErrors.IsNotFound(getPVC(fakeClient)) == false, "PVC should still exist") - }) - t.Run("ignores jobs and pods that do not mount the shared PVC", func(t *testing.T) { currentWorkspace := makeTerminatingWorkspace("ws-current", namespace) otherJob := makeCleanupJob("async-job", namespace, "ws-other-id", "claim-async", "ws-other-uid", false) From ca92565dda99df7ee678a40480cbbd6a672f82ca Mon Sep 17 00:00:00 2001 From: Anatolii Bazko Date: Tue, 27 Oct 2026 17:15:35 +0100 Subject: [PATCH 10/10] fixup Signed-off-by: Anatolii Bazko --- pkg/provision/storage/cleanup.go | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/pkg/provision/storage/cleanup.go b/pkg/provision/storage/cleanup.go index 32a85c9a9..d2da20d15 100644 --- a/pkg/provision/storage/cleanup.go +++ b/pkg/provision/storage/cleanup.go @@ -234,23 +234,10 @@ func commonPVCExists(workspace *common.DevWorkspaceWithConfig, clusterAPI sync.C } // 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. +// namespace, coordinating with cleanup jobs and leftover 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. func deleteCommonPVC(workspace *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) error { namespace := workspace.Namespace