Skip to content

fix: Resolve ServiceAccount deletion race condition (thanks @nss10!) #1421

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lbeckman314 merged 7 commits into from
Jun 3, 2026
Merged

fix: Resolve ServiceAccount deletion race condition (thanks @nss10!) #1421

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d2e910b
fix: Resolve ServiceAccount deletion race condition (thanks @nss10!)
lbeckman314 May 20, 2026
0a8117a
fix: K8s Unit Tests
lbeckman314 May 20, 2026
7a5616f
chore: Revert function name in serviceaccount.go
lbeckman314 May 21, 2026
fdc9b71
chore: Update comment
lbeckman314 May 21, 2026
7c134e7
chore: Remove unused parameter
lbeckman314 May 21, 2026
d413aa4
chore: Restrict fix/service-account-deletion to SA race condition only
lbeckman314 May 29, 2026
d496921
Merge branch 'develop' into fix/service-account-deletion
lbeckman314 Jun 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 2 additions & 30 deletions compute/kubernetes/backend.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,29 +296,6 @@ func (b *Backend) cleanResources(ctx context.Context, taskId string) error {
b.log.Error("deleting Job", "error", err)
}

// Delete PVC
err = resources.DeletePVC(ctx, taskId, b.conf.Kubernetes.JobsNamespace, b.client, b.log)

@nss10 nss10 May 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lbeckman314 — Deferring the deletion of these resources to Kubernetes using ownerReferences sounds good. However, we should handle that transition in a separate PR.

It also just occurred to me: what happens to cleanOrphanedResources? If there are any pending resources left behind by older server runs or due to previous deletion errors, our logic might track them but never actually clean them up.

Kubernetes garbage collection will continuously retry deleting these resources until they are gone, so we don't need to worry about transient API errors or rate limiting.

Given this, we should do one of two things:

  1. Make these changes in a separate PR, and mark this change as a breaking change.
  2. Update the cleanOrphanedResources method so it no longer looks for these specific resources as "orphaned," since Kubernetes will handle the cleanup natively.

@lbeckman314 lbeckman314 May 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in PR #1423!

@nss10 nss10 May 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @lbeckman314 , I was suggesting #1423 to be a PR pointed to develop, such that it includes all changes related to resource cleanup. Keep #1421 only about handling Service Account deletion race condition. All logic regarding resource cleanup should be handled in #1423 only.

if err != nil {
errs = multierror.Append(errs, err)
b.log.Error("deleting Worker PVC", "error", err)
}

// Delete per-task ConfigMap only if ConfigMapTemplate was configured
if b.conf.Kubernetes.ConfigMapTemplate != "" {
err = resources.DeleteConfigMap(ctx, taskId, b.conf.Kubernetes.JobsNamespace, b.client, b.log)
if err != nil {
errs = multierror.Append(errs, err)
b.log.Error("deleting Worker ConfigMap", "error", err)
}
}

// Delete RoleBinding
err = resources.DeleteRoleBinding(ctx, taskId, b.conf.Kubernetes.JobsNamespace, b.client, b.log)
if err != nil {
errs = multierror.Append(errs, err)
b.log.Error("deleting Job", "error", err)
}

// Determine the ServiceAccount for this task.
// Default to the conventional task-scoped name; override if the task
// specifies an externally-managed SA via the _WORKER_SA tag.
Expand All @@ -337,13 +314,6 @@ func (b *Backend) cleanResources(ctx context.Context, taskId string) error {
b.log.Error("deleting Worker ServiceAccount", "taskID", taskId, "error", err)
}

// Delete Role
err = resources.DeleteRole(ctx, taskId, b.conf.Kubernetes.JobsNamespace, b.client, b.log)
if err != nil {
errs = multierror.Append(errs, err)
b.log.Error("deleting Worker Role", "error", err)
}

// Delete PV
err = resources.DeletePV(ctx, taskId, b.conf.Kubernetes.JobsNamespace, b.client, b.log)
if err != nil {
Expand Down Expand Up @@ -701,6 +671,8 @@ func (b *Backend) CleanOrphanedResources(ctx context.Context) {
}
}

// TODO: Add Executor Jobs here beacause orphaned tasks can result in orphaned jobs

for taskID := range taskIDs {
clean, err := b.isResourceCleanupNeeded(ctx, taskID)
if err != nil {
Expand Down
26 changes: 19 additions & 7 deletions compute/kubernetes/backend_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,14 @@ func TestTaskSubmission(t *testing.T) {
// Create a fake Kubernetes client
fakeClient := fake.NewSimpleClientset()

// Inject a deterministic UID on every Job create so ownerRef propagation can be verified.
const testJobUID = "test-job-uid-1234"
fakeClient.PrependReactor("create", "jobs", func(action k8stesting.Action) (bool, runtime.Object, error) {
obj := action.(k8stesting.CreateAction).GetObject().(*batchv1.Job)
obj.UID = testJobUID
return false, obj, nil
})

// Create a mock configuration
conf := config.DefaultConfig()
conf.Kubernetes.Namespace = "test-namespace"
Expand Down Expand Up @@ -155,12 +163,18 @@ spec:
t.Errorf("expected Job name '%s', got '%s'", task.Id, job.Name)
}

// Verify that the ConfigMap was created
// Verify that the ConfigMap was created with the Job's UID in its ownerRef.
configMapName := "funnel-worker-config-" + task.Id
_, err = fakeClient.CoreV1().ConfigMaps(conf.Kubernetes.JobsNamespace).Get(context.Background(), configMapName, metav1.GetOptions{})
cm, err := fakeClient.CoreV1().ConfigMaps(conf.Kubernetes.JobsNamespace).Get(context.Background(), configMapName, metav1.GetOptions{})
if err != nil {
t.Fatalf("failed to get ConfigMap: %v", err)
}
if len(cm.OwnerReferences) == 0 {
t.Fatal("expected ConfigMap to have an ownerReference, but got none")
}
if got := cm.OwnerReferences[0].UID; got != testJobUID {
t.Errorf("expected ConfigMap ownerRef UID %q, got %q", testJobUID, got)
}

// Clean up resources
err = backend.cleanResources(context.Background(), task.Id)
Expand All @@ -174,11 +188,9 @@ spec:
t.Error("expected Job to be deleted, but it still exists")
}

// Verify that the ConfigMap was deleted
_, err = fakeClient.CoreV1().ConfigMaps(conf.Kubernetes.JobsNamespace).Get(context.Background(), configMapName, metav1.GetOptions{})
if err == nil {
t.Error("expected ConfigMap to be deleted, but it still exists")
}
// ConfigMap deletion is handled by Kubernetes garbage collection via ownerReferences,

@nss10 nss10 May 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of Configmap, look for a resource which is actually being cleaned up by cleanResources -- say PV. But as I mentioned above, it has to be in a PR of its own.

@lbeckman314 lbeckman314 May 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in PR #1423!

// not explicitly by cleanResources. The fake clientset does not simulate cascading GC,
// so we only verify the ownerRef is set correctly (asserted above).

}

Expand Down
14 changes: 9 additions & 5 deletions compute/kubernetes/resources/serviceaccount.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"context"
"fmt"
"text/template"
"time"

"github.com/ohsu-comp-bio/funnel/config"
"github.com/ohsu-comp-bio/funnel/logger"
Expand Down Expand Up @@ -73,9 +74,11 @@ func CreateServiceAccount(ctx context.Context, task *tes.Task, conf *config.Conf
return nil
}

// isServiceAccountAttachedToPods returns true as soon as it finds one active
// (non-terminating) pod using the given ServiceAccount.
func isServiceAccountAttachedToPods(ctx context.Context, saName, namespace string, client kubernetes.Interface) (bool, error) {
// isServiceAccountAttachedToPods returns true if any active pod for any active pod
// is still using the given ServiceAccount. Pods are skipped if they are terminating (DeletionTimestamp set)
func isServiceAccountAttachedToPods(ctx context.Context, saName, namespace string, client kubernetes.Interface, log *logger.Logger) (bool, error) {
log.Debug("ServiceAccount", "Sleeping for 5s before ServiceAccount deletion to avoid Race Conditions...")
time.Sleep(5 * time.Second)
pods, err := client.CoreV1().Pods(namespace).List(ctx, metav1.ListOptions{
FieldSelector: fmt.Sprintf("spec.serviceAccountName=%s", saName),
})
Expand All @@ -84,9 +87,10 @@ func isServiceAccountAttachedToPods(ctx context.Context, saName, namespace strin
}
for _, pod := range pods.Items {
if pod.DeletionTimestamp == nil {
return true, nil // early return on first active pod
return true, nil
}
}

return false, nil
}

Expand Down Expand Up @@ -120,7 +124,7 @@ func DeleteServiceAccount(ctx context.Context, taskID, namespace string, client
}

if sharedSA {
inUse, err := isServiceAccountAttachedToPods(ctx, saName, namespace, client)
inUse, err := isServiceAccountAttachedToPods(ctx, saName, namespace, client, log)
if err != nil {
return fmt.Errorf("checking pod attachment for ServiceAccount %s: %v", saName, err)
}
Expand Down
Loading