From b536f78c5c6a4cf7768c0e3b2e13abda170a9e81 Mon Sep 17 00:00:00 2001 From: Anatolii Bazko Date: Wed, 28 Oct 2026 13:34:25 +0100 Subject: [PATCH 1/7] fix: keep shared PVC from getting stuck Terminating on workspace cleanup Signed-off-by: Anatolii Bazko --- .../backupcronjob_controller_test.go | 13 +- pkg/provision/storage/cleanup.go | 124 ++++++++ pkg/provision/storage/cleanup_test.go | 273 ++++++++++++++++++ pkg/provision/storage/commonStorage.go | 23 +- pkg/provision/storage/shared.go | 14 +- pkg/provision/storage/shared_test.go | 68 +++++ pkg/provision/sync/sync.go | 14 +- 7 files changed, 502 insertions(+), 27 deletions(-) diff --git a/controllers/backupcronjob/backupcronjob_controller_test.go b/controllers/backupcronjob/backupcronjob_controller_test.go index 2807aca0f..0d1264fa2 100644 --- a/controllers/backupcronjob/backupcronjob_controller_test.go +++ b/controllers/backupcronjob/backupcronjob_controller_test.go @@ -456,12 +456,23 @@ var _ = Describe("BackupCronJobReconciler", func() { authSecret := createAuthSecret("backup-auth", nameNamespace.Namespace, map[string][]byte{}) Expect(fakeClient.Create(ctx, authSecret)).To(Succeed()) - // Create a running pod that mounts the PVC on a specific node + // The ReplicaSet owner reference is required + // because getNodeNameWithPVC only considers deployment-owned pods -- job + // pods (e.g. cleanup jobs) and orphaned pods that mount the PVC are ignored + // so they cannot skew the node affinity. Without this owner + // reference the pod would be skipped and no node affinity would be set. pod := &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ Name: "workspace-pod", Namespace: dw.Namespace, Labels: map[string]string{constants.DevWorkspaceIDLabel: "other-workspace-id"}, + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: "apps/v1", + Kind: "ReplicaSet", + Name: "other-workspace", + UID: "other-workspace-rs-uid", + Controller: func() *bool { b := true; return &b }(), + }}, }, Spec: corev1.PodSpec{ NodeName: "worker-node-1", diff --git a/pkg/provision/storage/cleanup.go b/pkg/provision/storage/cleanup.go index aaed82243..b0981cf87 100644 --- a/pkg/provision/storage/cleanup.go +++ b/pkg/provision/storage/cleanup.go @@ -18,6 +18,7 @@ package storage import ( "fmt" "path" + "slices" "time" "github.com/devfile/devworkspace-operator/pkg/dwerrors" @@ -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,125 @@ 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 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. +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 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 !isPVCMounted(pod.Spec, pvcName) { + continue + } + // Job-owned pods are not removed when the job finishes, so a finished + // cleanup job can leave its pod holding the PVC in a Terminating state; + // delete the pod so the pvc-protection finalizer can release the PVC. + jobName := getOwningJobName(&pod) + if jobName != "" { + job := &batchv1.Job{} + if err := clusterAPI.Client.Get(clusterAPI.Ctx, types.NamespacedName{Name: jobName, Namespace: pod.Namespace}, job); err != nil { + if !k8sErrors.IsNotFound(err) { + return err + } + // The owning job is gone, so the pod will be garbage-collected via its owner reference. + } else if isJobFinished(job) { + if err := clusterAPI.Client.Delete(clusterAPI.Ctx, &pod); err != nil && !k8sErrors.IsNotFound(err) { + return err + } + return &dwerrors.RetryError{ + Message: fmt.Sprintf("Deleted pod %s of finished job %s using PVC %s", pod.Name, jobName, pvcName), + RequeueAfter: 3 * time.Second, + } + } + } + if len(pod.OwnerReferences) == 0 { + if err := clusterAPI.Client.Delete(clusterAPI.Ctx, &pod); err != nil && !k8sErrors.IsNotFound(err) { + return err + } + return &dwerrors.RetryError{ + Message: fmt.Sprintf("Deleted orphaned pod %s using PVC %s", pod.Name, pvcName), + RequeueAfter: 3 * time.Second, + } + } + + waitMsg := fmt.Sprintf("Waiting for pod %s using PVC %s to be deleted before removing the PVC", pod.Name, pvcName) + if jobName != "" { + waitMsg = fmt.Sprintf("Waiting for pod %s of job %s using PVC %s to be deleted before removing the PVC", pod.Name, jobName, pvcName) + } + return &dwerrors.RetryError{ + Message: waitMsg, + RequeueAfter: 10 * time.Second, + } + } + + pvc := &corev1.PersistentVolumeClaim{} + namespacedName := types.NamespacedName{Name: pvcName, Namespace: namespace} + err = clusterAPI.Client.Get(clusterAPI.Ctx, namespacedName, pvc) + if err != nil { + if k8sErrors.IsNotFound(err) { + return nil + } + return err + } + + err = clusterAPI.Client.Delete(clusterAPI.Ctx, pvc) + if err != nil && !k8sErrors.IsNotFound(err) { + return err + } + return nil +} + +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 + }, + ) +} + +// getOwningJobName returns the name of the Job that controls the pod, or an +// empty string if the pod is not controlled by a Job. +func getOwningJobName(pod *corev1.Pod) string { + if ref := metav1.GetControllerOf(pod); ref != nil && ref.Kind == "Job" { + return ref.Name + } + return "" +} + +func isJobFinished(job *batchv1.Job) bool { + for _, condition := range job.Status.Conditions { + if condition.Status != corev1.ConditionTrue { + continue + } + switch condition.Type { + case batchv1.JobComplete, batchv1.JobFailed: + return true + } + } + return false +} diff --git a/pkg/provision/storage/cleanup_test.go b/pkg/provision/storage/cleanup_test.go index 9852cde15..7e18d94be 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" ) @@ -92,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}, @@ -184,3 +197,263 @@ 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 ownerJobName is set, the pod is controlled by the Job with that name. +func makeCleanupPod(name, namespace, workspaceId, pvcName, ownerJobName 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 ownerJobName != "" { + pod.OwnerReferences = []metav1.OwnerReference{{ + APIVersion: "batch/v1", + Kind: "Job", + Name: ownerJobName, + UID: types.UID(ownerJobName + "-uid"), + Controller: func() *bool { b := true; return &b }(), + }} + } + 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) + jobPod := makeCleanupPod("cleanup-other-pod", namespace, "ws-other-id", pvcName, "cleanup-other") + clusterAPI, fakeClient := makeClusterAPI(makeSharedPVC(), currentWorkspace, otherWorkspace, inFlightJob, jobPod) + + 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.False(t, k8sErrors.IsNotFound(getPVC(fakeClient)), "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") + assert.NoError(t, fakeClient.Get(context.Background(), types.NamespacedName{Name: "cleanup-other-pod", Namespace: namespace}, &corev1.Pod{}), + "Pod of in-flight job should not be deleted") + }) + + t.Run("waits for running cleanup job whose owning workspace is already deleted", func(t *testing.T) { + currentWorkspace := makeTerminatingWorkspace("ws-current", namespace) + // The job's owning workspace is gone from the cluster, but the job is still + // running and its pod still references the shared PVC. + runningJob := makeCleanupJob("cleanup-orphaned-owner", namespace, "ws-gone-id", pvcName, "ws-gone-uid", false) + jobPod := makeCleanupPod("cleanup-orphaned-owner-pod", namespace, "ws-gone-id", pvcName, "cleanup-orphaned-owner") + clusterAPI, fakeClient := makeClusterAPI(makeSharedPVC(), currentWorkspace, runningJob, jobPod) + + err := (&CommonStorageProvisioner{}).CleanupWorkspaceStorage(makeWorkspaceWithConfig(currentWorkspace), clusterAPI) + + var retryErr *dwerrors.RetryError + assert.ErrorAs(t, err, &retryErr, "Should return RetryError while job of deleted workspace is still running") + assert.Contains(t, retryErr.Message, "cleanup-orphaned-owner", "Wait message should mention the owning job for debuggability") + assert.False(t, k8sErrors.IsNotFound(getPVC(fakeClient)), "PVC should still exist") + assert.NoError(t, fakeClient.Get(context.Background(), types.NamespacedName{Name: "cleanup-orphaned-owner", Namespace: namespace}, &batchv1.Job{}), + "Running job should not be deleted") + assert.NoError(t, fakeClient.Get(context.Background(), types.NamespacedName{Name: "cleanup-orphaned-owner-pod", Namespace: namespace}, &corev1.Pod{}), + "Pod of running job should not be deleted") + }) + + 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.False(t, k8sErrors.IsNotFound(getPVC(fakeClient)), "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 pods of finished cleanup jobs before deleting PVC", func(t *testing.T) { + currentWorkspace := makeTerminatingWorkspace("ws-current", namespace) + finishedJob := makeCleanupJob("cleanup-done", namespace, "ws-gone-id", pvcName, "", true) + jobPod := makeCleanupPod("cleanup-done-pod", namespace, "ws-gone-id", pvcName, "cleanup-done") + clusterAPI, fakeClient := makeClusterAPI(makeSharedPVC(), currentWorkspace, finishedJob, jobPod) + + err := (&CommonStorageProvisioner{}).CleanupWorkspaceStorage(makeWorkspaceWithConfig(currentWorkspace), clusterAPI) + + // The finished job's 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 finished job's pod") + assert.True(t, k8sErrors.IsNotFound(fakeClient.Get(context.Background(), + types.NamespacedName{Name: "cleanup-done-pod", Namespace: namespace}, &corev1.Pod{})), "Finished job's pod should be deleted") + assert.False(t, k8sErrors.IsNotFound(getPVC(fakeClient)), "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("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") + }) + + t.Run("does not wait for finished cleanup job before deleting PVC", func(t *testing.T) { + currentWorkspace := makeTerminatingWorkspace("ws-current", namespace) + completedJob := makeCleanupJob("cleanup-done", namespace, "ws-other-id", pvcName, "ws-other-uid", true) + clusterAPI, fakeClient := makeClusterAPI(makeSharedPVC(), currentWorkspace, completedJob) + + err := (&CommonStorageProvisioner{}).CleanupWorkspaceStorage(makeWorkspaceWithConfig(currentWorkspace), clusterAPI) + + assert.NoError(t, err) + assert.True(t, k8sErrors.IsNotFound(getPVC(fakeClient)), "PVC should be deleted despite finished job still referencing it in spec") + }) +} + +func TestIsJobFinished(t *testing.T) { + jobWithCondition := func(conditionType batchv1.JobConditionType, status corev1.ConditionStatus) *batchv1.Job { + return &batchv1.Job{ + Status: batchv1.JobStatus{ + Conditions: []batchv1.JobCondition{{Type: conditionType, Status: status}}, + }, + } + } + + assert.True(t, isJobFinished(jobWithCondition(batchv1.JobComplete, corev1.ConditionTrue)), "Completed job should be finished") + assert.True(t, isJobFinished(jobWithCondition(batchv1.JobFailed, corev1.ConditionTrue)), "Failed job should be finished") + assert.False(t, isJobFinished(jobWithCondition(batchv1.JobComplete, corev1.ConditionFalse)), "Job with non-true Complete condition should not be finished") + assert.False(t, isJobFinished(jobWithCondition(batchv1.JobSuspended, corev1.ConditionTrue)), "Suspended job should not be finished") + assert.False(t, isJobFinished(&batchv1.Job{}), "Job without conditions should not be finished") +} diff --git a/pkg/provision/storage/commonStorage.go b/pkg/provision/storage/commonStorage.go index c95c93bf3..8223ae990 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" ) @@ -105,25 +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 { - 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 nil + return deleteCommonPVC(workspace, clusterAPI) } // 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..94275a6c1 100644 --- a/pkg/provision/storage/shared.go +++ b/pkg/provision/storage/shared.go @@ -310,9 +310,13 @@ func NodeAffinityForHostname(nodeName string) *corev1.Affinity { } } +// 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). 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 && isOwnedByWorkspaceDeployment(&pod) { for _, volume := range pod.Spec.Volumes { if volume.PersistentVolumeClaim != nil && volume.PersistentVolumeClaim.ClaimName == pvcName { return pod.Spec.NodeName @@ -323,6 +327,14 @@ func getNodeNameWithPVC(list *corev1.PodList, pvcName string) string { return "" } +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) { if name == "" { // Should not happen diff --git a/pkg/provision/storage/shared_test.go b/pkg/provision/storage/shared_test.go index 2d2b7ef50..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}, @@ -167,6 +175,66 @@ 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", + Controller: func() *bool { b := true; return &b }(), + }} + 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", + 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 { diff --git a/pkg/provision/sync/sync.go b/pkg/provision/sync/sync.go index 25cfd9a36..6f69a80c7 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,12 @@ 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. +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 From eb544dff145da89e3534bce2f96371515fee587c Mon Sep 17 00:00:00 2001 From: Anatolii Bazko Date: Wed, 28 Oct 2026 14:31:00 +0100 Subject: [PATCH 2/7] chore: cleanuo_test.go Signed-off-by: Anatolii Bazko --- pkg/provision/storage/cleanup_test.go | 52 +++++++-------------------- 1 file changed, 12 insertions(+), 40 deletions(-) diff --git a/pkg/provision/storage/cleanup_test.go b/pkg/provision/storage/cleanup_test.go index 7e18d94be..ea92dc8e3 100644 --- a/pkg/provision/storage/cleanup_test.go +++ b/pkg/provision/storage/cleanup_test.go @@ -245,7 +245,7 @@ func makeCleanupJob(name, namespace, workspaceId, pvcName, ownerUID string, comp Kind: "DevWorkspace", Name: "owner-workspace", UID: types.UID(ownerUID), - Controller: func() *bool { b := true; return &b }(), + Controller: func() *bool { return new(true) }(), }} } if complete { @@ -282,7 +282,7 @@ func makeCleanupPod(name, namespace, workspaceId, pvcName, ownerJobName string) Kind: "Job", Name: ownerJobName, UID: types.UID(ownerJobName + "-uid"), - Controller: func() *bool { b := true; return &b }(), + Controller: func() *bool { return new(true) }(), }} } return pod @@ -338,8 +338,8 @@ func TestCleanupWorkspaceStorageCoordinatesSharedPVCDeletion(t *testing.T) { 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) - jobPod := makeCleanupPod("cleanup-other-pod", namespace, "ws-other-id", pvcName, "cleanup-other") + inFlightJob := makeCleanupJob("cleanup-other", namespace, otherWorkspace.Status.DevWorkspaceId, pvcName, string(otherWorkspace.UID), false) + jobPod := makeCleanupPod("cleanup-other-pod", namespace, otherWorkspace.Status.DevWorkspaceId, pvcName, "cleanup-other") clusterAPI, fakeClient := makeClusterAPI(makeSharedPVC(), currentWorkspace, otherWorkspace, inFlightJob, jobPod) err := (&CommonStorageProvisioner{}).CleanupWorkspaceStorage(makeWorkspaceWithConfig(currentWorkspace), clusterAPI) @@ -353,23 +353,22 @@ func TestCleanupWorkspaceStorageCoordinatesSharedPVCDeletion(t *testing.T) { "Pod of in-flight job should not be deleted") }) - t.Run("waits for running cleanup job whose owning workspace is already deleted", func(t *testing.T) { + t.Run("waits for running cleanup job before deleting PVC", func(t *testing.T) { currentWorkspace := makeTerminatingWorkspace("ws-current", namespace) - // The job's owning workspace is gone from the cluster, but the job is still - // running and its pod still references the shared PVC. - runningJob := makeCleanupJob("cleanup-orphaned-owner", namespace, "ws-gone-id", pvcName, "ws-gone-uid", false) - jobPod := makeCleanupPod("cleanup-orphaned-owner-pod", namespace, "ws-gone-id", pvcName, "cleanup-orphaned-owner") + goneWorkspace := makeTerminatingWorkspace("ws-gone", namespace) + runningJob := makeCleanupJob("cleanup-running", namespace, goneWorkspace.Status.DevWorkspaceId, pvcName, string(goneWorkspace.UID), false) + jobPod := makeCleanupPod("cleanup-running-pod", namespace, goneWorkspace.Status.DevWorkspaceId, pvcName, "cleanup-running") clusterAPI, fakeClient := makeClusterAPI(makeSharedPVC(), currentWorkspace, runningJob, jobPod) err := (&CommonStorageProvisioner{}).CleanupWorkspaceStorage(makeWorkspaceWithConfig(currentWorkspace), clusterAPI) var retryErr *dwerrors.RetryError - assert.ErrorAs(t, err, &retryErr, "Should return RetryError while job of deleted workspace is still running") - assert.Contains(t, retryErr.Message, "cleanup-orphaned-owner", "Wait message should mention the owning job for debuggability") + assert.ErrorAs(t, err, &retryErr, "Should return RetryError while a cleanup job is still running") + assert.Contains(t, retryErr.Message, "cleanup-running", "Wait message should mention the owning job for debuggability") assert.False(t, k8sErrors.IsNotFound(getPVC(fakeClient)), "PVC should still exist") - assert.NoError(t, fakeClient.Get(context.Background(), types.NamespacedName{Name: "cleanup-orphaned-owner", Namespace: namespace}, &batchv1.Job{}), + assert.NoError(t, fakeClient.Get(context.Background(), types.NamespacedName{Name: "cleanup-running", Namespace: namespace}, &batchv1.Job{}), "Running job should not be deleted") - assert.NoError(t, fakeClient.Get(context.Background(), types.NamespacedName{Name: "cleanup-orphaned-owner-pod", Namespace: namespace}, &corev1.Pod{}), + assert.NoError(t, fakeClient.Get(context.Background(), types.NamespacedName{Name: "cleanup-running-pod", Namespace: namespace}, &corev1.Pod{}), "Pod of running job should not be deleted") }) @@ -429,31 +428,4 @@ func TestCleanupWorkspaceStorageCoordinatesSharedPVCDeletion(t *testing.T) { assert.NoError(t, fakeClient.Get(context.Background(), types.NamespacedName{Name: "async-pod", Namespace: namespace}, &corev1.Pod{}), "Pod mounting another PVC should not be deleted") }) - - t.Run("does not wait for finished cleanup job before deleting PVC", func(t *testing.T) { - currentWorkspace := makeTerminatingWorkspace("ws-current", namespace) - completedJob := makeCleanupJob("cleanup-done", namespace, "ws-other-id", pvcName, "ws-other-uid", true) - clusterAPI, fakeClient := makeClusterAPI(makeSharedPVC(), currentWorkspace, completedJob) - - err := (&CommonStorageProvisioner{}).CleanupWorkspaceStorage(makeWorkspaceWithConfig(currentWorkspace), clusterAPI) - - assert.NoError(t, err) - assert.True(t, k8sErrors.IsNotFound(getPVC(fakeClient)), "PVC should be deleted despite finished job still referencing it in spec") - }) -} - -func TestIsJobFinished(t *testing.T) { - jobWithCondition := func(conditionType batchv1.JobConditionType, status corev1.ConditionStatus) *batchv1.Job { - return &batchv1.Job{ - Status: batchv1.JobStatus{ - Conditions: []batchv1.JobCondition{{Type: conditionType, Status: status}}, - }, - } - } - - assert.True(t, isJobFinished(jobWithCondition(batchv1.JobComplete, corev1.ConditionTrue)), "Completed job should be finished") - assert.True(t, isJobFinished(jobWithCondition(batchv1.JobFailed, corev1.ConditionTrue)), "Failed job should be finished") - assert.False(t, isJobFinished(jobWithCondition(batchv1.JobComplete, corev1.ConditionFalse)), "Job with non-true Complete condition should not be finished") - assert.False(t, isJobFinished(jobWithCondition(batchv1.JobSuspended, corev1.ConditionTrue)), "Suspended job should not be finished") - assert.False(t, isJobFinished(&batchv1.Job{}), "Job without conditions should not be finished") } From 1f111cb71b8b7ebbe721ccf019044558462527ee Mon Sep 17 00:00:00 2001 From: Anatolii Bazko Date: Thu, 29 Oct 2026 10:08:46 +0100 Subject: [PATCH 3/7] fixup Signed-off-by: Anatolii Bazko --- pkg/provision/sync/sync.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/provision/sync/sync.go b/pkg/provision/sync/sync.go index 6f69a80c7..f8e78b448 100644 --- a/pkg/provision/sync/sync.go +++ b/pkg/provision/sync/sync.go @@ -189,7 +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. +// Kubernetes, 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 7a7a0007d2a3a4b7799005da0b6053097326d3f3 Mon Sep 17 00:00:00 2001 From: Anatolii Bazko Date: Tue, 3 Nov 2026 11:42:35 +0100 Subject: [PATCH 4/7] fix: Don't delete alternative using PVC Signed-off-by: Anatolii Bazko --- pkg/provision/storage/cleanup.go | 39 ++++++++++++++++++-------------- pkg/provision/sync/diff.go | 1 - 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/pkg/provision/storage/cleanup.go b/pkg/provision/storage/cleanup.go index b0981cf87..7560e8520 100644 --- a/pkg/provision/storage/cleanup.go +++ b/pkg/provision/storage/cleanup.go @@ -241,7 +241,7 @@ func commonPVCExists(workspace *common.DevWorkspaceWithConfig, clusterAPI sync.C func deleteCommonPVC(workspace *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) error { namespace := workspace.Namespace - _, pvcName, err := checkForAlternatePVC(namespace, clusterAPI) + usingAlternatePVC, pvcName, err := checkForAlternatePVC(namespace, clusterAPI) if err != nil { return err } @@ -277,6 +277,12 @@ func deleteCommonPVC(workspace *common.DevWorkspaceWithConfig, clusterAPI sync.C return err } // The owning job is gone, so the pod will be garbage-collected via its owner reference. + return &dwerrors.RetryError{ + Message: fmt.Sprintf( + "Waiting for pod %s/%s (orphaned by deleted job %s) to be garbage-collected, as it is still using PVC %s and blocks its deletion", + pod.Namespace, pod.Name, jobName, pvcName), + RequeueAfter: 3 * time.Second, + } } else if isJobFinished(job) { if err := clusterAPI.Client.Delete(clusterAPI.Ctx, &pod); err != nil && !k8sErrors.IsNotFound(err) { return err @@ -297,30 +303,29 @@ func deleteCommonPVC(workspace *common.DevWorkspaceWithConfig, clusterAPI sync.C } } - waitMsg := fmt.Sprintf("Waiting for pod %s using PVC %s to be deleted before removing the PVC", pod.Name, pvcName) - if jobName != "" { - waitMsg = fmt.Sprintf("Waiting for pod %s of job %s using PVC %s to be deleted before removing the PVC", pod.Name, jobName, pvcName) - } return &dwerrors.RetryError{ - Message: waitMsg, + Message: fmt.Sprintf("Waiting for pod %s using PVC %s to be deleted before removing the PVC", pod.Name, pvcName), RequeueAfter: 10 * time.Second, } } - pvc := &corev1.PersistentVolumeClaim{} - namespacedName := types.NamespacedName{Name: pvcName, Namespace: namespace} - err = clusterAPI.Client.Get(clusterAPI.Ctx, namespacedName, pvc) - if err != nil { - if k8sErrors.IsNotFound(err) { - return nil + if !usingAlternatePVC { + pvc := &corev1.PersistentVolumeClaim{} + namespacedName := types.NamespacedName{Name: pvcName, Namespace: namespace} + err = clusterAPI.Client.Get(clusterAPI.Ctx, namespacedName, pvc) + if err != nil { + if k8sErrors.IsNotFound(err) { + return nil + } + return err } - return err - } - err = clusterAPI.Client.Delete(clusterAPI.Ctx, pvc) - if err != nil && !k8sErrors.IsNotFound(err) { - return err + err = clusterAPI.Client.Delete(clusterAPI.Ctx, pvc) + if err != nil && !k8sErrors.IsNotFound(err) { + return err + } } + return nil } diff --git a/pkg/provision/sync/diff.go b/pkg/provision/sync/diff.go index fbf9f01de..7b6f68287 100644 --- a/pkg/provision/sync/diff.go +++ b/pkg/provision/sync/diff.go @@ -250,7 +250,6 @@ func routingDiffFunc(spec, cluster crclient.Object) (delete, update bool) { func jobDiffFunc(spec, cluster crclient.Object) (delete, update bool) { specJob := spec.(*batchv1.Job) clusterJob := cluster.(*batchv1.Job) - // TODO: previously, this delete was specified with a background deletion policy, which is currently unsupported. return !equality.Semantic.DeepDerivative(specJob.Spec, clusterJob.Spec), false } From e326c95bd5a57aeac0ff765d78e70530b8a2d40b Mon Sep 17 00:00:00 2001 From: Anatolii Bazko Date: Tue, 3 Nov 2026 14:49:52 +0100 Subject: [PATCH 5/7] shore: some fixes Signed-off-by: Anatolii Bazko --- pkg/provision/storage/cleanup.go | 13 +++++++++++-- pkg/provision/storage/shared.go | 2 +- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/pkg/provision/storage/cleanup.go b/pkg/provision/storage/cleanup.go index 7560e8520..b2baab8a9 100644 --- a/pkg/provision/storage/cleanup.go +++ b/pkg/provision/storage/cleanup.go @@ -279,7 +279,7 @@ func deleteCommonPVC(workspace *common.DevWorkspaceWithConfig, clusterAPI sync.C // The owning job is gone, so the pod will be garbage-collected via its owner reference. return &dwerrors.RetryError{ Message: fmt.Sprintf( - "Waiting for pod %s/%s (orphaned by deleted job %s) to be garbage-collected, as it is still using PVC %s and blocks its deletion", + "Waiting for pod %s/%s of deleted job %s to be garbage-collected; it still uses PVC %s", pod.Namespace, pod.Name, jobName, pvcName), RequeueAfter: 3 * time.Second, } @@ -303,8 +303,12 @@ func deleteCommonPVC(workspace *common.DevWorkspaceWithConfig, clusterAPI sync.C } } + jobInfo := "" + if jobName != "" { + jobInfo = fmt.Sprintf(" of job %s", jobName) + } return &dwerrors.RetryError{ - Message: fmt.Sprintf("Waiting for pod %s using PVC %s to be deleted before removing the PVC", pod.Name, pvcName), + Message: fmt.Sprintf("Waiting for pod %s%s using PVC %s to be deleted", pod.Name, jobInfo, pvcName), RequeueAfter: 10 * time.Second, } } @@ -324,6 +328,11 @@ func deleteCommonPVC(workspace *common.DevWorkspaceWithConfig, clusterAPI sync.C if err != nil && !k8sErrors.IsNotFound(err) { return err } + } else { + clusterAPI.Logger.Info( + fmt.Sprintf("Skipping deletion of PVC %s as it is not provisioned by the DevWorkspace Operator", pvcName), + "namespace", namespace, + ) } return nil diff --git a/pkg/provision/storage/shared.go b/pkg/provision/storage/shared.go index 94275a6c1..4c1de8b71 100644 --- a/pkg/provision/storage/shared.go +++ b/pkg/provision/storage/shared.go @@ -332,7 +332,7 @@ func isOwnedByWorkspaceDeployment(pod *corev1.Pod) bool { if controllerRef == nil { return false } - return controllerRef.Kind == "ReplicaSet" || controllerRef.Kind == "Deployment" + return controllerRef.Kind == "ReplicaSet" } func checkPVCTerminating(name, namespace string, api sync.ClusterAPI) (bool, error) { From dc2740e10894b450228e0d33377c87ff43fea14d Mon Sep 17 00:00:00 2001 From: Anatolii Bazko Date: Tue, 3 Nov 2026 16:28:09 +0100 Subject: [PATCH 6/7] fix: only exclude cleanup job's own pods when computing PVC node affinity Excluding all Job-owned pods from getNodeNameForPVC meant that a running backup job or a concurrent cleanup job holding the shared PVC was invisible when computing node affinity, so a new cleanup job could be scheduled on another node and hit Multi-Attach errors with ReadWriteOnce PVCs. It also silently changed backup job affinity behavior, which uses the same FindNodeForPVC helper. Now only pods controlled by the cleanup job itself are excluded; all other running pods mounting the PVC are considered. Co-Authored-By: Claude Code Signed-off-by: Anatolii Bazko --- .../backupcronjob/backupcronjob_controller.go | 4 +- pkg/provision/storage/cleanup.go | 5 +- pkg/provision/storage/shared.go | 39 +++++++-------- pkg/provision/storage/shared_test.go | 49 +++++++++++++------ 4 files changed, 61 insertions(+), 36 deletions(-) diff --git a/controllers/backupcronjob/backupcronjob_controller.go b/controllers/backupcronjob/backupcronjob_controller.go index 7a64ff346..4a780c6cd 100644 --- a/controllers/backupcronjob/backupcronjob_controller.go +++ b/controllers/backupcronjob/backupcronjob_controller.go @@ -459,7 +459,9 @@ func (r *BackupCronJobReconciler) createBackupJob( } // Pin backup Job to the node where the PVC is currently mounted to avoid // Multi-Attach errors with ReadWriteOnce PVCs on multi-node clusters. - targetNode, err := provstorage.FindNodeForPVC(ctx, r.Client, workspace.Namespace, pvc.Name) + // The backup job name is generated at creation time, so it cannot be excluded here; + // all running pods mounting the PVC are considered when computing the affinity. + targetNode, err := provstorage.FindNodeForPVC(ctx, r.Client, workspace.Namespace, pvc.Name, "") if err != nil { log.Error(err, "Failed to find node with PVC, backup Job will not have node affinity", "pvc", pvc.Name) } else if targetNode == "" { diff --git a/pkg/provision/storage/cleanup.go b/pkg/provision/storage/cleanup.go index b2baab8a9..81bee8986 100644 --- a/pkg/provision/storage/cleanup.go +++ b/pkg/provision/storage/cleanup.go @@ -121,7 +121,10 @@ func getSpecCommonPVCCleanupJob(workspace *common.DevWorkspaceWithConfig, cluste pvcName = workspace.Config.Workspace.PVCName } - targetNode, err := FindNodeForPVC(clusterAPI.Ctx, clusterAPI.Client, workspace.Namespace, pvcName) + // Exclude the cleanup job's own pods so that recomputing the job spec does not + // pin it to the node of its own (possibly leftover) pods; other Job pods using + // the PVC (e.g. backup or concurrent cleanup jobs) are still followed. + targetNode, err := FindNodeForPVC(clusterAPI.Ctx, clusterAPI.Client, workspace.Namespace, pvcName, common.PVCCleanupJobName(workspaceId)) if err != nil { clusterAPI.Logger.Error(err, "Error getting target node for cleanup job") } else if targetNode == "" { diff --git a/pkg/provision/storage/shared.go b/pkg/provision/storage/shared.go index 4c1de8b71..59b880a10 100644 --- a/pkg/provision/storage/shared.go +++ b/pkg/provision/storage/shared.go @@ -271,8 +271,9 @@ func getSharedPVCWorkspaceCount(namespace string, api sync.ClusterAPI) (total in // FindNodeForPVC lists DevWorkspace pods in the given namespace and returns the // node name where a running pod mounts the specified PVC. Returns an empty -// string (and nil error) when no such pod is found. -func FindNodeForPVC(ctx context.Context, k8sClient client.Client, namespace, pvcName string) (string, error) { +// string (and nil error) when no such pod is found. Pods controlled by the Job +// named excludeJob are skipped; pass an empty string to consider all pods. +func FindNodeForPVC(ctx context.Context, k8sClient client.Client, namespace, pvcName, excludeJob string) (string, error) { labelSelector, err := labels.Parse(constants.DevWorkspaceIDLabel) if err != nil { return "", err @@ -284,7 +285,7 @@ func FindNodeForPVC(ctx context.Context, k8sClient client.Client, namespace, pvc }); err != nil { return "", err } - return getNodeNameWithPVC(podList, pvcName), nil + return getNodeNameWithPVC(podList, pvcName, excludeJob), nil } // NodeAffinityForHostname returns an Affinity that pins a pod to the given node @@ -311,30 +312,28 @@ func NodeAffinityForHostname(nodeName string) *corev1.Affinity { } // 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). -func getNodeNameWithPVC(list *corev1.PodList, pvcName string) string { +// determined from the first running pod in the list that mounts it. All running pods +// are considered (workspace pods, async-storage pods, and Job pods such as backup or +// concurrent cleanup Jobs), since any pod mounting a ReadWriteOnce PVC forces the same +// node. The only exception is pods controlled by the Job named excludeJob: a Job +// computing its own node affinity must not follow its own leftover pods. +func getNodeNameWithPVC(list *corev1.PodList, pvcName, excludeJob string) string { for _, pod := range list.Items { - 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 - } + if pod.Status.Phase != corev1.PodRunning { + continue + } + if excludeJob != "" && getOwningJobName(&pod) == excludeJob { + continue + } + for _, volume := range pod.Spec.Volumes { + if volume.PersistentVolumeClaim != nil && volume.PersistentVolumeClaim.ClaimName == pvcName { + return pod.Spec.NodeName } } } return "" } -func isOwnedByWorkspaceDeployment(pod *corev1.Pod) bool { - controllerRef := metav1.GetControllerOf(pod) - if controllerRef == nil { - return false - } - return controllerRef.Kind == "ReplicaSet" -} - 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 5aca05f89..90058d280 100644 --- a/pkg/provision/storage/shared_test.go +++ b/pkg/provision/storage/shared_test.go @@ -114,10 +114,11 @@ func TestFindNodeForPVC(t *testing.T) { const ns = "test-ns" tests := []struct { - name string - pods []*corev1.Pod - pvcName string - expected string + name string + pods []*corev1.Pod + pvcName string + excludeJob string + expected string }{ { name: "returns node for running pod with matching PVC", @@ -176,10 +177,10 @@ func TestFindNodeForPVC(t *testing.T) { expected: "", }, { - name: "ignores running pods owned by Jobs", + name: "ignores running pods owned by the excluded Job", pods: func() []*corev1.Pod { - // Cleanup job pods mount the PVC themselves, so they must not be - // used to determine where workspace storage is attached. + // A cleanup job computing its own affinity must not follow its own + // (possibly leftover) pods. pod := makePod("cleanup-pod", "node-1", ns, corev1.PodRunning, pvcVolume("claim-devworkspace")) pod.OwnerReferences = []metav1.OwnerReference{{ APIVersion: "batch/v1", @@ -190,8 +191,28 @@ func TestFindNodeForPVC(t *testing.T) { }} return []*corev1.Pod{pod} }(), - pvcName: "claim-devworkspace", - expected: "", + pvcName: "claim-devworkspace", + excludeJob: "cleanup-workspace123", + expected: "", + }, + { + name: "considers running pods owned by other Jobs", + pods: func() []*corev1.Pod { + // E.g. a backup job or a concurrent cleanup job still holds the PVC; + // following its node avoids Multi-Attach errors. + pod := makePod("backup-pod", "node-2", ns, corev1.PodRunning, pvcVolume("claim-devworkspace")) + pod.OwnerReferences = []metav1.OwnerReference{{ + APIVersion: "batch/v1", + Kind: "Job", + Name: "backup-workspace123", + UID: "job-uid", + Controller: func() *bool { b := true; return &b }(), + }} + return []*corev1.Pod{pod} + }(), + pvcName: "claim-devworkspace", + excludeJob: "cleanup-workspace123", + expected: "node-2", }, { name: "considers running pods owned by ReplicaSets", @@ -210,17 +231,17 @@ func TestFindNodeForPVC(t *testing.T) { expected: "node-2", }, { - name: "ignores pods without owner references", + name: "considers 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: "", + expected: "node-1", }, { - name: "ignores pods owned by StatefulSets", + name: "considers pods owned by StatefulSets", pods: func() []*corev1.Pod { pod := makePod("statefulset-pod", "node-1", ns, corev1.PodRunning, pvcVolume("claim-devworkspace")) pod.OwnerReferences = []metav1.OwnerReference{{ @@ -233,7 +254,7 @@ func TestFindNodeForPVC(t *testing.T) { return []*corev1.Pod{pod} }(), pvcName: "claim-devworkspace", - expected: "", + expected: "node-1", }, } @@ -245,7 +266,7 @@ func TestFindNodeForPVC(t *testing.T) { } fakeClient := builder.Build() - result, err := FindNodeForPVC(context.Background(), fakeClient, ns, tt.pvcName) + result, err := FindNodeForPVC(context.Background(), fakeClient, ns, tt.pvcName, tt.excludeJob) assert.NoError(t, err) assert.Equal(t, tt.expected, result) }) From ea588c50e5ce24c3f1a4983f4b8f0d31397fc972 Mon Sep 17 00:00:00 2001 From: Anatolii Bazko Date: Tue, 3 Nov 2026 16:44:09 +0100 Subject: [PATCH 7/7] fix: label backup job pod template so PVC cleanup sees backup pods Job metadata labels are not propagated to pods, so backup pods lacked the DevWorkspace ID label and were missed by the label-selector check in deleteCommonPVC. This could leave the shared PVC stuck in Terminating when the last workspace was deleted while a backup pod still referenced it. Addresses CodeRabbit finding on PR #1725. Assisted-by: Claude Code Co-Authored-By: Claude Code Signed-off-by: Anatolii Bazko --- controllers/backupcronjob/backupcronjob_controller.go | 5 +++++ controllers/backupcronjob/backupcronjob_controller_test.go | 4 ++++ 2 files changed, 9 insertions(+) diff --git a/controllers/backupcronjob/backupcronjob_controller.go b/controllers/backupcronjob/backupcronjob_controller.go index 4a780c6cd..8bfb4908d 100644 --- a/controllers/backupcronjob/backupcronjob_controller.go +++ b/controllers/backupcronjob/backupcronjob_controller.go @@ -394,6 +394,11 @@ func (r *BackupCronJobReconciler) createBackupJob( BackoffLimit: backUpConfig.BackoffLimit, Template: corev1.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + constants.DevWorkspaceIDLabel: dwID, + constants.DevWorkspaceNameLabel: workspace.Name, + constants.DevWorkspaceBackupJobLabel: "true", + }, Annotations: map[string]string{ "io.kubernetes.cri-o.Devices": "/dev/fuse", }, diff --git a/controllers/backupcronjob/backupcronjob_controller_test.go b/controllers/backupcronjob/backupcronjob_controller_test.go index 0d1264fa2..e55464b99 100644 --- a/controllers/backupcronjob/backupcronjob_controller_test.go +++ b/controllers/backupcronjob/backupcronjob_controller_test.go @@ -365,6 +365,10 @@ var _ = Describe("BackupCronJobReconciler", func() { Expect(jobList.Items).To(HaveLen(1)) job := jobList.Items[0] Expect(job.Labels[constants.DevWorkspaceIDLabel]).To(Equal("id-recent")) + Expect(job.Spec.Template.Labels).To(HaveKeyWithValue(constants.DevWorkspaceIDLabel, "id-recent"), + "backup job pod template must carry the workspace ID label so deleteCommonPVC can see pods still using the PVC") + Expect(job.Spec.Template.Labels).To(HaveKeyWithValue(constants.DevWorkspaceNameLabel, "dw-recent")) + Expect(job.Spec.Template.Labels).To(HaveKeyWithValue(constants.DevWorkspaceBackupJobLabel, "true")) Expect(job.Spec.Template.Spec.ServiceAccountName).To(Equal("devworkspace-job-runner-id-recent")) container := job.Spec.Template.Spec.Containers[0] expectedEnvs := []corev1.EnvVar{