From e7a0bc42abe839c89e6226d4229174a78aaae1dc Mon Sep 17 00:00:00 2001 From: Tomba Leishangthem <10569680+tomba7@users.noreply.github.com> Date: Fri, 7 Aug 2026 14:20:12 -0700 Subject: [PATCH 1/3] fix(controller): delete k8s worker Deployments during Worker Deployment cleanup ## Summary handleDeletion never tore down the child worker k8s Deployments, so their pods kept polling. The server rejects `DeleteVersion` (even with `SkipDrainage=true`) while a version still has active pollers, so version cleanup failed on every retry and the finalizer was never removed, which is the deadlock. ownerRef GC could not break it either, since GC is itself blocked on the finalizer. Add an explicit k8s Deployment teardown step before WD version deletion. Pollers linger in the server cache for a few minutes after pods die, so the later steps requeue until they age out. ## Testing - `go test ./internal/controller -count=1` - `go vet ./internal/controller` --- internal/controller/worker_controller.go | 48 ++++++++++++++++++++---- 1 file changed, 41 insertions(+), 7 deletions(-) diff --git a/internal/controller/worker_controller.go b/internal/controller/worker_controller.go index a657fc36..361e95a5 100644 --- a/internal/controller/worker_controller.go +++ b/internal/controller/worker_controller.go @@ -534,8 +534,15 @@ func (r *WorkerDeploymentReconciler) markWRTsWDNotFound(ctx context.Context, wd // The cleanup sequence: // 1. Clear the ramping version (must happen first to avoid a split-traffic window) // 2. Set the current version to "unversioned" (empty BuildID) so new tasks route to unversioned workers -// 3. Delete all registered versions (with SkipDrainage since the WD is being removed entirely) -// 4. Delete the deployment record itself once all versions are gone +// 3. Delete the worker Deployments so their pods stop polling. This is required +// before a version can be deleted: the server rejects DeleteVersion (even with +// SkipDrainage) while a version still has active pollers. Normally this teardown +// happens in executePlan, but that path never runs during deletion, so without +// it the pods keep polling and cleanup deadlocks. Pollers linger in the server's +// cache for a few minutes after the pods die, so steps 4-5 requeue until they +// age out. +// 4. Delete all registered versions (with SkipDrainage since the WD is being removed entirely) +// 5. Delete the deployment record itself once all versions are gone func (r *WorkerDeploymentReconciler) handleDeletion( ctx context.Context, l logr.Logger, @@ -643,11 +650,38 @@ func (r *WorkerDeploymentReconciler) handleDeletion( l.Info("No current version set, skipping unversioned redirect") } - // Step 3: Delete versions that are eligible. Versions that are still draining + // Step 3: Tear down the worker Deployments backing these versions so their pods + // terminate and stop polling. This is required before a version can be deleted: + // the server rejects DeleteVersion (even with SkipDrainage=true) while a version + // still has active pollers. During normal reconciliation this teardown happens in + // executePlan, but that path is downstream of the deletion bail-out and never runs + // here. Without this step the pods keep polling, DeleteVersion keeps failing, and + // the finalizer is never removed (deadlock). Deleting them explicitly breaks it, + // rather than waiting for ownerRef GC, which is itself blocked on the finalizer. + k8sState, err := k8s.GetDeploymentState( + ctx, + r.Client, + workerDeploy.Namespace, + workerDeploy.Name, + workerDeploymentName) + if err != nil { + return fmt.Errorf("unable to list child deployments during cleanup: %w", err) + } + // Deletion order is irrelevant, but iterating over the DeploymentsByTime slice instead + // of the Deployments map gives deterministic ordering, so the log lines below stay + // stable across the 10s retries. + for _, d := range k8sState.DeploymentsByTime { + l.Info("Deleting worker deployment during cleanup", "deployment", d.Name) + if err := r.Delete(ctx, d); err != nil && !apierrors.IsNotFound(err) { + return fmt.Errorf("unable to delete worker deployment %s during cleanup (will retry): %w", d.Name, err) + } + } + + // Step 4: Delete versions that are eligible. Versions that are still draining // are force-deleted with SkipDrainage since the WD is being removed entirely. - // If any version fails to delete (e.g. active pollers), return an error so the - // reconciler requeues. Pollers disappear once pods terminate and the next - // reconciliation will succeed. + // If any version fails to delete (e.g. active pollers still draining after the + // Deployment delete above), return an error so the reconciler requeues. Pollers + // disappear once pods terminate and a subsequent reconciliation will succeed. for _, version := range resp.Info.VersionSummaries { buildID := version.Version.BuildID l.Info("Deleting worker deployment version", "buildID", buildID) @@ -660,7 +694,7 @@ func (r *WorkerDeploymentReconciler) handleDeletion( } } - // Step 4: Delete the deployment itself. This only succeeds if all versions are gone. + // Step 5: Delete the deployment itself. This only succeeds if all versions are gone. l.Info("Attempting to delete worker deployment from Temporal server", "name", workerDeploymentName) if _, err := temporalClient.WorkerDeploymentClient().Delete(ctx, sdkclient.WorkerDeploymentDeleteOptions{ Name: workerDeploymentName, From 36b282d8ef88ec2a0de3b2abdeaa1078a017d4d7 Mon Sep 17 00:00:00 2001 From: Tomba Leishangthem <10569680+tomba7@users.noreply.github.com> Date: Sun, 16 Aug 2026 22:48:33 -0700 Subject: [PATCH 2/3] test(controller): cover k8s deployment teardown during WD cleanup ## Summary Addresses review feedback on #508. Add an assertion to the WD deletion integration test that both versioned k8s deployments are gone once the finalizer completes, covering the teardown step this PR introduced. Also correct the handleDeletion comments, which mischaracterized the SkipDrainage flag as allowing force-deletion. It only waives the requirement that a version should not be in draining. The no-active-pollers precondition is evaluated separately and still applies. The remaining comment and err msg edits disambiguate k8s deployments from Temporal worker deployments. No behavior change. ## Testing - `go test -v -tags test_dep ./internal/tests/internal -run 'TestIntegration/deletion-sets-current-to-unversioned' -timeout 30m` - `go test -v -tags test_dep ./internal/tests/internal -run TestIntegration -timeout 30m` - `go vet ./internal/controller ./internal/tests/internal` --- internal/controller/worker_controller.go | 21 ++++++++++--------- .../internal/deletion_integration_test.go | 17 +++++++++++++++ 2 files changed, 28 insertions(+), 10 deletions(-) diff --git a/internal/controller/worker_controller.go b/internal/controller/worker_controller.go index 361e95a5..8c5fd6c9 100644 --- a/internal/controller/worker_controller.go +++ b/internal/controller/worker_controller.go @@ -650,7 +650,7 @@ func (r *WorkerDeploymentReconciler) handleDeletion( l.Info("No current version set, skipping unversioned redirect") } - // Step 3: Tear down the worker Deployments backing these versions so their pods + // Step 3: Tear down the k8s deployments backing these versions so their pods // terminate and stop polling. This is required before a version can be deleted: // the server rejects DeleteVersion (even with SkipDrainage=true) while a version // still has active pollers. During normal reconciliation this teardown happens in @@ -665,23 +665,23 @@ func (r *WorkerDeploymentReconciler) handleDeletion( workerDeploy.Name, workerDeploymentName) if err != nil { - return fmt.Errorf("unable to list child deployments during cleanup: %w", err) + return fmt.Errorf("unable to list k8s deployments during deletion of worker deployment: %w", err) } // Deletion order is irrelevant, but iterating over the DeploymentsByTime slice instead // of the Deployments map gives deterministic ordering, so the log lines below stay // stable across the 10s retries. for _, d := range k8sState.DeploymentsByTime { - l.Info("Deleting worker deployment during cleanup", "deployment", d.Name) + l.Info("Deleting k8s worker deployment during cleanup", "deployment", d.Name) if err := r.Delete(ctx, d); err != nil && !apierrors.IsNotFound(err) { - return fmt.Errorf("unable to delete worker deployment %s during cleanup (will retry): %w", d.Name, err) + return fmt.Errorf("unable to delete k8s deployment %s during deletion of worker deployment (will retry): %w", d.Name, err) } } - // Step 4: Delete versions that are eligible. Versions that are still draining - // are force-deleted with SkipDrainage since the WD is being removed entirely. - // If any version fails to delete (e.g. active pollers still draining after the - // Deployment delete above), return an error so the reconciler requeues. Pollers - // disappear once pods terminate and a subsequent reconciliation will succeed. + // Step 4: Delete every registered version. SkipDrainage lets DeleteVersion succeed on + // versions that are still draining, which is acceptable here since the whole WD is + // going away. If any version fails to delete (e.g. it still has recent pollers after the + // k8s Deployment delete above), return an error so the reconciler requeues. Pollers age + // out on the server minutes after the pods terminate, so a later attempt succeeds. for _, version := range resp.Info.VersionSummaries { buildID := version.Version.BuildID l.Info("Deleting worker deployment version", "buildID", buildID) @@ -694,7 +694,8 @@ func (r *WorkerDeploymentReconciler) handleDeletion( } } - // Step 5: Delete the deployment itself. This only succeeds if all versions are gone. + // Step 5: Delete the worker deployment itself. This only succeeds if all versions and + // their associated k8s deployments are gone. l.Info("Attempting to delete worker deployment from Temporal server", "name", workerDeploymentName) if _, err := temporalClient.WorkerDeploymentClient().Delete(ctx, sdkclient.WorkerDeploymentDeleteOptions{ Name: workerDeploymentName, diff --git a/internal/tests/internal/deletion_integration_test.go b/internal/tests/internal/deletion_integration_test.go index 7686fc96..d7c8e088 100644 --- a/internal/tests/internal/deletion_integration_test.go +++ b/internal/tests/internal/deletion_integration_test.go @@ -6,6 +6,7 @@ package internal // // Covered: // - WD deletion sets current version to unversioned on Temporal server +// - WD deletion tears down the k8s deployments // - WD deletion removes finalizer from Connection when no other WDs reference it // - WD is fully deleted from K8s after cleanup (finalizer removed) // - WD deletion with Connection deleted simultaneously (Helm race condition) still succeeds @@ -186,6 +187,22 @@ func testDeletionSetsCurrentToUnversioned( }) t.Log("WD deleted successfully (finalizer completed)") + // Verify the WD's k8s deployments were deleted. + // In a real cluster active pollers linger for matching.PollerHistoryTTL (5m) after the pods + // die, delaying the finalizer; this test uses a 1s TTL, so cleanup completes quickly. The + // Get below proves the k8s deployments are gone. + for _, name := range []string{expectedDeploymentName, deploymentNameV2} { + eventually(t, 30*time.Second, time.Second, func() error { + var dep appsv1.Deployment + err := k8sClient.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, &dep) + if err != nil { + return nil + } + return fmt.Errorf("k8s deployment %s still exists after WD cleanup", name) + }) + } + t.Log("Verified: both k8s deployments were deleted during cleanup") + // Verify Temporal server-side state: current version should be unversioned resp, err := deploymentHandle.Describe(ctx, sdkclient.WorkerDeploymentDescribeOptions{}) if err != nil { From 89a65d9b1a3d7e39f7b1697044ce1150cc78f03b Mon Sep 17 00:00:00 2001 From: Tomba Leishangthem <10569680+tomba7@users.noreply.github.com> Date: Mon, 17 Aug 2026 16:29:57 -0700 Subject: [PATCH 3/3] docs(controller): fix verbose and unnecessary code comments ## Summary Addresses review feedback on #508. - Trimmed verbose comments in the handleDeletion path - Remove comments that don't add value - Named the dynamic config knob behind the retry window matching.PollerHistoryTTL versus just saying "minutes" - Verified the existing deletion test fails without the teardown block and passes with it. Comment-only change. No behavior change. ## Testing - `go build ./...` - `gofmt -l internal/controller/worker_controller.go` - `go test -v -tags test_dep ./internal/tests/internal -run 'TestIntegration/deletion-sets-current-to-unvers ioned' -timeout 30m` - `go test -v -tags test_dep ./internal/tests/internal -run TestIntegration -timeout 30m` - `go vet ./internal/controller ./internal/tests/internal` --- internal/controller/worker_controller.go | 30 ++++++------------------ 1 file changed, 7 insertions(+), 23 deletions(-) diff --git a/internal/controller/worker_controller.go b/internal/controller/worker_controller.go index 8c5fd6c9..a1cf1112 100644 --- a/internal/controller/worker_controller.go +++ b/internal/controller/worker_controller.go @@ -534,13 +534,7 @@ func (r *WorkerDeploymentReconciler) markWRTsWDNotFound(ctx context.Context, wd // The cleanup sequence: // 1. Clear the ramping version (must happen first to avoid a split-traffic window) // 2. Set the current version to "unversioned" (empty BuildID) so new tasks route to unversioned workers -// 3. Delete the worker Deployments so their pods stop polling. This is required -// before a version can be deleted: the server rejects DeleteVersion (even with -// SkipDrainage) while a version still has active pollers. Normally this teardown -// happens in executePlan, but that path never runs during deletion, so without -// it the pods keep polling and cleanup deadlocks. Pollers linger in the server's -// cache for a few minutes after the pods die, so steps 4-5 requeue until they -// age out. +// 3. Delete the k8s deployments so their pods stop polling // 4. Delete all registered versions (with SkipDrainage since the WD is being removed entirely) // 5. Delete the deployment record itself once all versions are gone func (r *WorkerDeploymentReconciler) handleDeletion( @@ -650,14 +644,8 @@ func (r *WorkerDeploymentReconciler) handleDeletion( l.Info("No current version set, skipping unversioned redirect") } - // Step 3: Tear down the k8s deployments backing these versions so their pods - // terminate and stop polling. This is required before a version can be deleted: - // the server rejects DeleteVersion (even with SkipDrainage=true) while a version - // still has active pollers. During normal reconciliation this teardown happens in - // executePlan, but that path is downstream of the deletion bail-out and never runs - // here. Without this step the pods keep polling, DeleteVersion keeps failing, and - // the finalizer is never removed (deadlock). Deleting them explicitly breaks it, - // rather than waiting for ownerRef GC, which is itself blocked on the finalizer. + // Step 3: Delete the k8s deployments so their pods stop polling. DeleteVersion is + // rejected while a version still has active pollers. k8sState, err := k8s.GetDeploymentState( ctx, r.Client, @@ -667,9 +655,6 @@ func (r *WorkerDeploymentReconciler) handleDeletion( if err != nil { return fmt.Errorf("unable to list k8s deployments during deletion of worker deployment: %w", err) } - // Deletion order is irrelevant, but iterating over the DeploymentsByTime slice instead - // of the Deployments map gives deterministic ordering, so the log lines below stay - // stable across the 10s retries. for _, d := range k8sState.DeploymentsByTime { l.Info("Deleting k8s worker deployment during cleanup", "deployment", d.Name) if err := r.Delete(ctx, d); err != nil && !apierrors.IsNotFound(err) { @@ -679,9 +664,9 @@ func (r *WorkerDeploymentReconciler) handleDeletion( // Step 4: Delete every registered version. SkipDrainage lets DeleteVersion succeed on // versions that are still draining, which is acceptable here since the whole WD is - // going away. If any version fails to delete (e.g. it still has recent pollers after the - // k8s Deployment delete above), return an error so the reconciler requeues. Pollers age - // out on the server minutes after the pods terminate, so a later attempt succeeds. + // going away. If any version fails to delete, return an error so the reconciler requeues. + // Pollers linger in the server's cache for matching.PollerHistoryTTL (dynamic config, + // 5m by default) after the pods terminate, so a later attempt succeeds. for _, version := range resp.Info.VersionSummaries { buildID := version.Version.BuildID l.Info("Deleting worker deployment version", "buildID", buildID) @@ -694,8 +679,7 @@ func (r *WorkerDeploymentReconciler) handleDeletion( } } - // Step 5: Delete the worker deployment itself. This only succeeds if all versions and - // their associated k8s deployments are gone. + // Step 5: Delete the worker deployment itself. This only succeeds if all versions are gone. l.Info("Attempting to delete worker deployment from Temporal server", "name", workerDeploymentName) if _, err := temporalClient.WorkerDeploymentClient().Delete(ctx, sdkclient.WorkerDeploymentDeleteOptions{ Name: workerDeploymentName,