diff --git a/pkg/provision/storage/cleanup.go b/pkg/provision/storage/cleanup.go index aaed82243..d2da20d15 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,97 @@ 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 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{ + Namespace: namespace, + LabelSelector: labelSelector, + }); err != nil { + return err + } + for _, pod := range podList.Items { + 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 + } + return &dwerrors.RetryError{ + Message: fmt.Sprintf("Deleted leftover cleanup jobs/pods using PVC %s", pvcName), + RequeueAfter: 10 * time.Second, + } + } + + 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, + } + } + + 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 +} + +// 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/cleanup_test.go b/pkg/provision/storage/cleanup_test.go index 9852cde15..14aa9f16f 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,215 @@ 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("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..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