Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
104b56d
hotfix: Revert ConfigMap creation update
lbeckman314 Apr 15, 2026
92edb9e
fix: Skip deletion of ServiceAccounts still in use
lbeckman314 Apr 17, 2026
dc02e7b
feat: Move orphaned resource cleanup to backend startup
lbeckman314 Apr 21, 2026
9665964
fix: Skip ServiceAccount deletion if in use by other pods
lbeckman314 Apr 22, 2026
f967150
fix: Skip ServiceAccount deletion if in use by other pods
lbeckman314 Apr 22, 2026
2021d2e
feat: update cleanOrphanedResources to non-blocking goroutine
lbeckman314 Apr 22, 2026
2bc183b
fix: Address PR review comments on ServiceAccount pod-in-use check
lbeckman314 Apr 27, 2026
6f07339
feat: Add owner references to task resources for automatic K8s GC cle…
lbeckman314 Apr 28, 2026
4a96b7a
fix: Add support for strong read-write consistency (S3 CSI Driver)
lbeckman314 Apr 29, 2026
10bde50
feat: Add support for exponential backoff calls to K8s
lbeckman314 Apr 29, 2026
a7fc01a
Merge pull request #1401 from calypr/fix/s3-output-files
lbeckman314 Apr 29, 2026
2bc8c53
fix: Add missing test fixture for quote handling
lbeckman314 Apr 29, 2026
775c24f
Merge branch 'fix/resource-clean' into fix/serviceaccount-deletion
lbeckman314 Apr 29, 2026
6946c9c
feat: Move cleanOrphanedResources to Helm Cron Job (background task)
lbeckman314 May 1, 2026
99b4bf6
Merge remote-tracking branch 'origin/develop' into fix/serviceaccount…
lbeckman314 May 1, 2026
5a567a2
fix: Update K8s tests
lbeckman314 May 1, 2026
206f414
fix: Add kubernetes package for managing Kubernetes resources
lbeckman314 May 2, 2026
957d200
fix: K8s unit test
lbeckman314 May 2, 2026
ec64e00
fix: Single element quote test (TODO: check if OK)
lbeckman314 May 4, 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
12 changes: 11 additions & 1 deletion compute/kubernetes/backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,16 @@ func (b *Backend) createResources(ctx context.Context, task *tes.Task, config *c
func (b *Backend) cleanResources(ctx context.Context, taskId string) error {
var errs error

// Check whether this task used an externally-managed ServiceAccount (e.g.
// Gen3Workflow per-user SA supplied via _WORKER_SA tag). If so, skip SA
// deletion β€” the SA is shared across tasks and must not be torn down here.
externalSA := false
if task, err := b.database.GetTask(ctx, &tes.GetTaskRequest{Id: taskId, View: tes.View_FULL.String()}); err == nil {

Copilot AI Apr 17, 2026

Copy link

Choose a reason for hiding this comment

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

cleanResources fetches the task with View_FULL just to inspect tags. In BoltDB (and likely other DB implementations), FULL also loads executor stdout/stderr and system logs, which is significantly more work than needed here. Use tes.View_BASIC (or another minimal view that still includes Tags) to avoid unnecessary IO/CPU during cleanup.

Suggested change
if task, err := b.database.GetTask(ctx, &tes.GetTaskRequest{Id: taskId, View: tes.View_FULL.String()}); err == nil {
if task, err := b.database.GetTask(ctx, &tes.GetTaskRequest{Id: taskId, View: tes.View_BASIC.String()}); err == nil {

Copilot uses AI. Check for mistakes.
if saName, exists := task.Tags["_WORKER_SA"]; exists && saName != "" {
externalSA = true
}
Comment on lines +292 to +301

Copilot AI Apr 17, 2026

Copy link

Choose a reason for hiding this comment

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

The PR description says SA deletion is skipped when the ServiceAccount is "being used by any other task", but the implementation only checks whether this task has a non-empty _WORKER_SA tag. If the intent really is "any other task", consider querying the DB (e.g., ListTasks filtered on _WORKER_SA=<name> and non-terminal states) to confirm no other active tasks reference the same SA; otherwise, update the PR description/comment to match the narrower behavior ("skip when _WORKER_SA is set").

Copilot uses AI. Check for mistakes.
}

Copilot AI Apr 17, 2026

Copy link

Choose a reason for hiding this comment

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

If b.database.GetTask fails, externalSA remains false and cleanup will proceed to delete ServiceAccounts. That undermines the intent of protecting shared/external SAs under transient DB errors or when the task record has been pruned. Consider handling the error explicitly (at least log it) and choosing a safer behavior (e.g., skip SA deletion when SA ownership can't be determined, or return an error so cleanup can be retried).

Copilot uses AI. Check for mistakes.

// Delete Job
b.log.Debug("deleting Job", "taskID", taskId)
err := resources.DeleteJob(ctx, b.conf, taskId, b.client, b.log)
Comment on lines +295 to 306

Copilot AI Apr 17, 2026

Copy link

Choose a reason for hiding this comment

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

Errors from GetTask are silently ignored here; if the DB read fails transiently, externalSA stays false and Funnel may delete a shared externally-managed ServiceAccount, reintroducing the authorization failure this PR is trying to prevent. Consider logging the error and choosing a safer fallback (e.g., skip SA deletion when tag lookup fails, or derive SA ownership from the Job/serviceAccountName before deleting).

Suggested change
externalSA := false
if task, err := b.database.GetTask(ctx, &tes.GetTaskRequest{Id: taskId, View: tes.View_FULL.String()}); err == nil {
if saName, exists := task.Tags["_WORKER_SA"]; exists && saName != "" {
externalSA = true
}
}
// Delete Job
b.log.Debug("deleting Job", "taskID", taskId)
err := resources.DeleteJob(ctx, b.conf, taskId, b.client, b.log)
// If task lookup fails, conservatively skip SA deletion rather than risk
// deleting a shared externally-managed ServiceAccount.
externalSA := false
task, err := b.database.GetTask(ctx, &tes.GetTaskRequest{Id: taskId, View: tes.View_FULL.String()})
if err != nil {
externalSA = true
b.log.Error("getting task for ServiceAccount ownership check", "taskID", taskId, "error", err)
} else if saName, exists := task.Tags["_WORKER_SA"]; exists && saName != "" {
externalSA = true
}
// Delete Job
b.log.Debug("deleting Job", "taskID", taskId)
err = resources.DeleteJob(ctx, b.conf, taskId, b.client, b.log)

Copilot uses AI. Check for mistakes.
Expand Down Expand Up @@ -303,7 +313,7 @@ func (b *Backend) cleanResources(ctx context.Context, taskId string) error {
}

// Delete ServiceAccount
err = resources.DeleteServiceAccount(ctx, taskId, b.conf.Kubernetes.JobsNamespace, b.client, b.log)
err = resources.DeleteServiceAccount(ctx, taskId, b.conf.Kubernetes.JobsNamespace, b.client, b.log, externalSA)
if err != nil {
errs = multierror.Append(errs, err)
b.log.Error("deleting Worker ServiceAccount", "error", err)
Expand Down
52 changes: 13 additions & 39 deletions compute/kubernetes/resources/configmap.go
Original file line number Diff line number Diff line change
@@ -1,18 +1,14 @@
package resources

import (
"bytes"
"context"
"fmt"
"strings"
"text/template"

"github.com/ohsu-comp-bio/funnel/config"
"github.com/ohsu-comp-bio/funnel/logger"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/kubernetes/scheme"
)

func CreateConfigMap(ctx context.Context, taskId string, conf *config.Config, client kubernetes.Interface, log *logger.Logger) error {
Expand All @@ -21,41 +17,19 @@ func CreateConfigMap(ctx context.Context, taskId string, conf *config.Config, cl
return fmt.Errorf("marshaling config to ConfigMap: %v", err)
}

indentFn := func(spaces int, s string) string {
pad := strings.Repeat(" ", spaces)
lines := strings.Split(s, "\n")
for i, line := range lines {
if line != "" {
lines[i] = pad + line
}
}
return strings.Join(lines, "\n")
}

t, err := template.New(taskId).Funcs(template.FuncMap{"indent": indentFn}).Parse(conf.Kubernetes.ConfigMapTemplate)
if err != nil {
return fmt.Errorf("parsing template: %v", err)
}

var buf bytes.Buffer
err = t.Execute(&buf, map[string]interface{}{
"TaskId": taskId,
"Namespace": conf.Kubernetes.JobsNamespace,
"Data": string(configBytes),
})
if err != nil {
return fmt.Errorf("%v", err)
}

decode := scheme.Codecs.UniversalDeserializer().Decode
obj, _, err := decode(buf.Bytes(), nil, nil)
if err != nil {
return fmt.Errorf("decoding ConfigMap spec: %v", err)
}

cm, ok := obj.(*corev1.ConfigMap)
if !ok {
return fmt.Errorf("failed to decode ConfigMap spec")
// Create the ConfigMap that will contain the Funnel Worker Config (`funnel-worker.yaml`)
cm := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: fmt.Sprintf("funnel-worker-config-%s", taskId),
Namespace: conf.Kubernetes.JobsNamespace,
Labels: map[string]string{
"app": "funnel",
"taskId": taskId,
},
},
Data: map[string]string{
"funnel-worker.yaml": string(configBytes),
},
}

_, err = client.CoreV1().ConfigMaps(conf.Kubernetes.JobsNamespace).Create(ctx, cm, metav1.CreateOptions{})
Expand Down
2 changes: 1 addition & 1 deletion compute/kubernetes/resources/resources_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ func TestDeleteServiceAccount(t *testing.T) {
t.Fatalf("Failed to create test ServiceAccount: %v", err)
}

err = DeleteServiceAccount(context.Background(), testTaskID, namespace, fakeClient, l)
err = DeleteServiceAccount(context.Background(), testTaskID, namespace, fakeClient, l, false)

Copilot AI Apr 17, 2026

Copy link

Choose a reason for hiding this comment

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

The new externalSA branch in DeleteServiceAccount isn't covered by tests. Add a test case that calls DeleteServiceAccount(..., externalSA=true) and asserts the ServiceAccount still exists afterward (and that no error is returned).

Copilot uses AI. Check for mistakes.
if err != nil {
t.Errorf("DeleteServiceAccount failed: %v", err)
}
Expand Down
9 changes: 8 additions & 1 deletion compute/kubernetes/resources/serviceaccount.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,14 @@ func CreateServiceAccount(ctx context.Context, task *tes.Task, conf *config.Conf
}

// DeleteServiceAccount deletes the ServiceAccount created for a task.
func DeleteServiceAccount(ctx context.Context, taskID string, namespace string, client kubernetes.Interface, log *logger.Logger) error {
// If externalSA is true the ServiceAccount is externally managed (e.g. a
// Gen3Workflow per-user SA supplied via the _WORKER_SA task tag) and must not
// be deleted by Funnel.
func DeleteServiceAccount(ctx context.Context, taskID string, namespace string, client kubernetes.Interface, log *logger.Logger, externalSA bool) error {
if externalSA {
log.Debug("skipping deletion of externally-managed ServiceAccount", "taskID", taskID)
return nil
}
// ServiceAccount names are not available here without config, so we list by label.
sas, err := client.CoreV1().ServiceAccounts(namespace).List(ctx, metav1.ListOptions{
LabelSelector: fmt.Sprintf("app=funnel,taskId=%s", taskID),
Expand Down
Loading