Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
21 changes: 20 additions & 1 deletion .github/workflows/tests.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ jobs:

unitTest:
runs-on: ubuntu-latest
permissions:
checks: write
steps:
- name: Check out code
uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4
Expand All @@ -38,7 +40,24 @@ jobs:
with:
go-version-file: go.mod

- name: Install gotestsum
run: go install gotest.tools/gotestsum@v1.13.0

- name: Unit Tests
id: unit_tests
continue-on-error: true
run: |
go test -v ./... -coverprofile=coverage.out
gotestsum --junitfile=test-report.xml -- ./... -coverprofile=coverage.out
go tool cover -func=coverage.out

- name: Test Report
uses: dorny/test-reporter@a43b3a5f7366b97d083190328d2c652e1a8b6aa2 # v3
if: always()
with:
name: Unit Test Results
path: test-report.xml
reporter: java-junit

- name: Fail job if tests failed
if: steps.unit_tests.outcome == 'failure'
run: exit 1
84 changes: 84 additions & 0 deletions compute/kubernetes/backend_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,20 @@ package kubernetes

import (
"context"
"fmt"
"reflect"
"testing"

"github.com/ohsu-comp-bio/funnel/config"
"github.com/ohsu-comp-bio/funnel/events"
"github.com/ohsu-comp-bio/funnel/logger"
"github.com/ohsu-comp-bio/funnel/tes"
batchv1 "k8s.io/api/batch/v1"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/kubernetes/fake"
k8stesting "k8s.io/client-go/testing"
)

// noopEventWriter implements events.Writer for testing.
Expand Down Expand Up @@ -256,3 +260,83 @@ spec:
t.Fatalf("unexpected toleration value: got=%q want=%q", gotTol.Value, "worker")
}
}

// TestCancel_SAInUse verifies that Cancel succeeds even when the task's
// ServiceAccount is still attached to a running pod (the foreground-deleted
// Job's pod hasn't terminated yet). The SA should be left in place and not
// cause Cancel to return an error.
func TestCancel_SAInUse(t *testing.T) {
const taskID = "cancel-test-task"
const ns = "test-namespace"

fakeClient := fake.NewSimpleClientset()
ctx := context.Background()

// Pre-create a worker Job so DeleteJob has something to delete.
job := &batchv1.Job{
ObjectMeta: metav1.ObjectMeta{Name: taskID, Namespace: ns},
}
if _, err := fakeClient.BatchV1().Jobs(ns).Create(ctx, job, metav1.CreateOptions{}); err != nil {
t.Fatalf("creating job: %v", err)
}

saName := fmt.Sprintf("funnel-worker-sa-%s-%s", ns, taskID)

// Pre-create the SA with the label DeleteServiceAccount queries by.
sa := &corev1.ServiceAccount{
ObjectMeta: metav1.ObjectMeta{
Name: saName,
Namespace: ns,
Labels: map[string]string{"app": "funnel", "taskId": taskID},
},
}
if _, err := fakeClient.CoreV1().ServiceAccounts(ns).Create(ctx, sa, metav1.CreateOptions{}); err != nil {
t.Fatalf("creating SA: %v", err)
}

// Simulate a still-running worker pod attached to the SA (the situation
// that arises immediately after foreground Job deletion).
pod := &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{Name: taskID + "-pod", Namespace: ns},
Spec: corev1.PodSpec{ServiceAccountName: saName},
}
if _, err := fakeClient.CoreV1().Pods(ns).Create(ctx, pod, metav1.CreateOptions{}); err != nil {
t.Fatalf("creating pod: %v", err)
}

// fake.NewSimpleClientset doesn't enforce field selectors, so we inject a
// reactor that returns the pod when isServiceAccountAttachedToPods queries
// by spec.serviceAccountName, ensuring the in-use path is exercised.
fakeClient.PrependReactor("list", "pods", func(action k8stesting.Action) (bool, runtime.Object, error) {
return true, &corev1.PodList{Items: []corev1.Pod{*pod}}, nil
})

conf := config.DefaultConfig()
conf.Kubernetes.Namespace = ns
conf.Kubernetes.JobsNamespace = ns
conf.Kubernetes.WorkerTemplate = "placeholder"

backend := &Backend{
client: fakeClient,
log: logger.NewLogger("test", logger.DefaultConfig()),
conf: conf,
event: &noopEventWriter{},
}

// Cancel must succeed even though the SA is still in use.
if err := backend.Cancel(ctx, taskID); err != nil {
t.Fatalf("Cancel returned unexpected error: %v", err)
}

// Job should be gone.
_, err := fakeClient.BatchV1().Jobs(ns).Get(ctx, taskID, metav1.GetOptions{})
if err == nil {
t.Error("expected Job to be deleted after Cancel")
}

// SA must still exist β€” it was skipped, not deleted.
_, err = fakeClient.CoreV1().ServiceAccounts(ns).Get(ctx, saName, metav1.GetOptions{})
if err != nil {
t.Errorf("expected SA to remain while pod is still running, got: %v", err)
}
}
7 changes: 4 additions & 3 deletions compute/kubernetes/resources/resources_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -420,13 +420,14 @@ func TestDeleteServiceAccountInUse(t *testing.T) {
}

err = DeleteServiceAccount(context.Background(), testTaskID, namespace, fakeClient, l, false)
if err == nil {
t.Fatal("expected DeleteServiceAccount to fail when ServiceAccount is in use")
if err != nil {
t.Fatalf("expected DeleteServiceAccount to succeed (skip) when ServiceAccount is in use, got error: %v", err)
}

// SA must still exist β€” it was skipped, not deleted.
_, err = fakeClient.CoreV1().ServiceAccounts(namespace).Get(context.Background(), sa.Name, metav1.GetOptions{})
if err != nil {
t.Fatalf("expected ServiceAccount to remain after failed delete: %v", err)
t.Fatalf("expected ServiceAccount to remain when still in use: %v", err)
}
}

Expand Down
Loading