From b1c2462ce8003d15cf3f1c5d9948a9e83f53cc66 Mon Sep 17 00:00:00 2001 From: Said Boudjelda Date: Thu, 8 Oct 2026 00:35:23 +0200 Subject: [PATCH 1/6] fix(chart): the test-run cleanup CronJob deletes old runs, through a new DELETE /api/tests The kates chart's cleanup CronJob, which values-prod.yaml and values-staging.yaml enable, never deleted a run: - it asked GET /api/tests for status COMPLETED, which the API refuses with a 400; - it sent no API key, so got a 401 with auth on (the default); - it paired the runs' id and createdAt in an order the JSON never has; - it computed its cutoff with date options busybox lacks; - it read only the newest 200 runs; - it carried the API pod's labels, so the chart's NetworkPolicy blocked it from the API. The API now does the picking: DELETE /api/tests?createdBefore= deletes the finished runs (DONE and FAILED by default, ?status= to narrow) created before then. It deletes oldest first, at most limit (1000) a call, each through the same delete as DELETE /api/tests/{id} and with an audit row, and answers matched, deleted and remaining. ?dryRun=true only counts. Anything else is a 400, a malformed dryRun included, so a typo cannot turn a count into a delete. The CronJob sends the key from the apiKey Secret and computes its cutoff with date -d @SECONDS, which busybox reads. It calls until no run matches, at most 100 calls, and fails on anything but a 200 with the API's answer; backoffLimit is 2. cleanup.statuses (default DONE and FAILED) picks the runs. Chart 0.10.10. kates test prune --older-than 30d does the same by hand. It counts first, asks unless --yes, and with --dry-run only counts. prune is no longer an alias of kates test cleanup, which deletes orphans still RUNNING. Caveat refs into TestResource and TestRunRepository are re-pointed. --- README.md | 2 +- charts/kates/Chart.yaml | 10 +- charts/kates/README.md | 14 +- charts/kates/templates/NOTES.txt | 2 +- charts/kates/templates/cleanup-cronjob.yaml | 138 +++++- charts/kates/values.schema.json | 7 + charts/kates/values.yaml | 13 +- cli/client/client.go | 41 ++ cli/client/client_test.go | 105 +++++ cli/client/escape_test.go | 10 + cli/client/types.go | 14 + cli/cmd/cleanup.go | 6 +- cli/cmd/data/doc_entries.yaml | 35 ++ cli/cmd/mcp_caveats.go | 2 +- cli/cmd/mcp_prompts_cluster.go | 4 +- cli/cmd/mcp_tools_runs.go | 8 +- cli/cmd/mcp_tools_scenario.go | 2 +- cli/cmd/prune.go | 254 +++++++++++ cli/cmd/prune_test.go | 429 ++++++++++++++++++ cli/cmd/root.go | 1 + docs/book/02-architecture.md | 2 +- docs/book/10-cli-reference.md | 30 +- docs/book/11-api-reference.md | 50 ++ docs/book/12-deployment.md | 12 + docs/book/appendix-d-versions.md | 2 +- kates/docs/api-reference.md | 47 ++ kates/docs/cli-reference.md | 13 + .../com/bmscomp/kates/api/TestResource.java | 145 ++++++ .../kates/config/NativeReflectionConfig.java | 1 + .../bmscomp/kates/domain/PruneResponse.java | 11 + .../kates/service/TestRunRepository.java | 37 ++ .../kates/api/TestResourceDeleteTest.java | 33 +- .../kates/api/TestResourcePruneTest.java | 349 ++++++++++++++ .../src/test/resources/application.properties | 5 + 34 files changed, 1792 insertions(+), 42 deletions(-) create mode 100644 cli/cmd/prune.go create mode 100644 cli/cmd/prune_test.go create mode 100644 kates/src/main/java/com/bmscomp/kates/domain/PruneResponse.java create mode 100644 kates/src/test/java/com/bmscomp/kates/api/TestResourcePruneTest.java diff --git a/README.md b/README.md index 285918e8..afbf9e8b 100644 --- a/README.md +++ b/README.md @@ -213,7 +213,7 @@ When you install charts with Helm directly: | [`kafka-ui`](charts/kafka-ui/) | 0.3.0 | v1.5.0 | A Helm chart for deploying Kafka UI (Kafbat) with Strimzi SCRAM-SHA-512 authentication | | [`kates-chaos`](charts/kates-chaos/) | 2.2.1 | 3.28.0 | Kates Chaos Engineering — wraps the LitmusChaos execution plane (operator, exporter, CRDs) with Kafka-specific RBAC, experiment/engine templating, and monitoring | | [`kates-platform`](charts/kates-platform/) | 0.7.0 | 1.0.0 | Umbrella chart for the full Kates platform — Kafka, Kates, and supporting infrastructure | -| [`kates`](charts/kates/) | 0.10.9 | 1.25.0 | Kates — Kafka Advanced Testing & Engineering Suite | +| [`kates`](charts/kates/) | 0.10.10 | 1.25.0 | Kates — Kafka Advanced Testing & Engineering Suite | | [`legacy-kafka`](charts/legacy-kafka/) | 0.2.0 | 3.9.1 | A deliberately old Kafka (2.x on ZooKeeper, or 3.x on KRaft) to migrate FROM — the replication source that the pinned Strimzi release cannot deploy | | [`mirror-maker2`](charts/mirror-maker2/) | 0.11.3 | 4.3.1 | Strimzi KafkaMirrorMaker2 — cross-cluster replication, DR, and cross-version migration (2.x/3.x → 4.x) | | [`monitoring`](charts/monitoring/) | 1.6.0 | 82.4.3 | Kates Monitoring — wraps kube-prometheus-stack with Kates-specific dashboards and configuration | diff --git a/charts/kates/Chart.yaml b/charts/kates/Chart.yaml index 54ac720d..d4c30e5b 100644 --- a/charts/kates/Chart.yaml +++ b/charts/kates/Chart.yaml @@ -2,9 +2,11 @@ apiVersion: v2 name: kates description: Kates — Kafka Advanced Testing & Engineering Suite type: application -# Patch: appVersion and the image tag move to 1.25.0, the image with the -# backend fixes since 1.24.0. Templates and defaults are otherwise unchanged. -version: 0.10.9 +# Patch: the test run cleanup CronJob deletes runs at last, through the +# API's DELETE /api/tests with the apiKey Secret's key, and cleanup.statuses +# picks which finished runs it deletes. appVersion is unchanged; the job +# takes an image newer than it. +version: 0.10.10 # The non-blocking health endpoints this chart's probes now point at come from # a BUILD-TIME property (quarkus.messaging.health.enabled=false), so they only # exist in the image built from this branch. Keep appVersion and @@ -28,6 +30,8 @@ annotations: artifacthub.io/category: integration-delivery artifacthub.io/license: Apache-2.0 artifacthub.io/changes: | + - kind: fixed + description: "The test run cleanup CronJob (cleanup.enabled, which values-prod.yaml and values-staging.yaml set) never deleted a run. It asked GET /api/tests for status COMPLETED, which the API refuses, sent no API key, paired the runs' fields in an order the JSON never has, and computed its cutoff with date options the curl image's busybox lacks; and it carried the API pod's labels, so with networkPolicy.enabled (the default) the chart's NetworkPolicy blocked it from the API. Each job now calls DELETE /api/tests with the key from the apiKey Secret, which deletes the runs created more than cleanup.retentionDays ago with a status in cleanup.statuses (new, default DONE and FAILED; a cancelled run is stored FAILED), oldest first and 1000 a call, until none match. On anything but a 200 it prints the API's answer and fails, after two retries (backoffLimit 2). It takes an image newer than 1.25.0, which has no DELETE /api/tests: there every job fails with HTTP 405. With cleanup.enabled, the first job against a Kates API that has the endpoint deletes every finished run older than cleanup.retentionDays, back to the 90 days after which the API's own daily sweep (kates.cleanup.retention-days) has already deleted them: 60 to 90 days in values-staging.yaml, and nothing more in values-prod.yaml, whose 90 days the sweep covers" - kind: changed description: "appVersion 1.25.0, the first image with the backend fixes since 1.24.0. Requests: a request, scenario or schedule the run could not honour is refused with a 400 before it takes a concurrency slot, among them spec, scenario and phase values outside their limits, a rate of 0, a phase without a type or rate, and values the database cannot store (#296, #298, #299, #302, #303, #306, #313, #314, #329, #333). A bean-validation 400 names each field (#330), a run the Kates API fails to start is a 500 (#325), and bulk create counts only the runs it started (#332). Runs: scenario phases and RAMP steps run one after another (#307); deleting or cancelling a run stops its tasks and frees its slot, during submission too (#290, #300, #321); the reaper keeps a failed run's results and reaps each run at its own deadline (#288); shutdown stops each task through its own backend (#331). Results: a FAILED run or an unmeasured latency no longer passes its gates (#289), trends read their runs' results (#286), and a task that has not started is left out of a run's throughput means (#315). Faults: parameters are bounded and each fault is injected once (#291); on litmus-crd a POD_DELETE honours its gracePeriodSec and a fault its delayBeforeSec, and a kubernetes fault is timed from the end of its delay (#304, #308); a NODE_DRAIN drains the node of the pod its selector picks (#319); a plan step's recovery clocks start when its fault goes in (#324). Resilience runs inject their fault only while the benchmark runs, run a scenario's testRequest, aim the fault at a broker, measure RPO from the injection and log no false error (#309, #311, #318, #320). The built-in Kafka probes ask the cluster, and a probe that cannot run fails (#334): the producer-throughput probe writes to kates-probe-topic, which kafka-cluster's platform profile creates from 1.0.6. gRPC CreateTest holds its fields to the same limits, answers a refusal INVALID_ARGUMENT, and a full engine RESOURCE_EXHAUSTED (#317, #322)" - kind: fixed diff --git a/charts/kates/README.md b/charts/kates/README.md index 5c94af82..7133543f 100644 --- a/charts/kates/README.md +++ b/charts/kates/README.md @@ -199,9 +199,19 @@ that — `image.tag` does. | `backup.persistence.storageClass` | `""` | StorageClass (empty = default) | | `backup.persistence.existingClaim` | `""` | Use an existing PVC | | `migration.enabled` | `false` | Enable pre-upgrade migration Job | -| `cleanup.enabled` | `false` | Enable test run cleanup CronJob | +| `cleanup.enabled` | `false` | Enable the test run cleanup CronJob; see [Test run cleanup](#test-run-cleanup) | | `cleanup.schedule` | `0 4 * * 0` | Cleanup cron schedule | -| `cleanup.retentionDays` | `30` | Days to keep completed tests | +| `cleanup.retentionDays` | `30` | Delete finished runs created more than this many days ago | +| `cleanup.statuses` | `[DONE, FAILED]` | Which finished runs to delete: `DONE`, `FAILED` or both. A cancelled run is stored `FAILED` | +| `cleanup.image` | `curlimages/curl:8.7.1` | Image the job runs `curl` from | + +### Test run cleanup + +Each job asks the Kates API to delete the finished runs created more than `cleanup.retentionDays` ago whose status is in `cleanup.statuses`, through `DELETE /api/tests?createdBefore=…&status=DONE&status=FAILED&limit=1000`. The API deletes them oldest first, at most 1000 a call, writes an audit row for each, and says how many still match; the job calls again until none do, at most 100 times, and the next job goes on from there. A run still `PENDING`, `RUNNING` or `STOPPING` is never deleted, however old it is. `kates test prune` does the same from the command line. The Kates API's own daily sweep already deletes the runs created more than 90 days ago (`kates.cleanup.retention-days`) that are not `PENDING` or `RUNNING`, so a `retentionDays` of 90 or more adds nothing to it. + +The job sends the key from the `apiKey` Secret (`apiKey.existingSecret`, or `-api-key`, at `apiKey.secretKey`) as `X-API-Key`, as the chart's tests do. On anything but a 200 it prints the API's answer and fails, and Kubernetes retries it twice: a 401 names the Secret it read the key from, and a 405 means a Kates API without `DELETE /api/tests`. That endpoint takes an image newer than `1.25.0`. + +Upgrading from 0.10.9 or earlier: the job there never deleted a run. It asked for a status the API does not have, sent no key, computed its cutoff with `date` options the image's busybox lacks, and carried the API pod's labels, so with `networkPolicy.enabled` (the default) the chart's NetworkPolicy blocked it from the API. `values-staging.yaml` enables it with 60 days, so its first job against a Kates API that has the endpoint deletes every finished run created 60 to 90 days ago; raise `cleanup.retentionDays`, or set `cleanup.enabled=false`, to keep them. `values-prod.yaml` enables it with 90 days, which the API's sweep already covers. ### Extensibility diff --git a/charts/kates/templates/NOTES.txt b/charts/kates/templates/NOTES.txt index a9725fcb..c1507994 100644 --- a/charts/kates/templates/NOTES.txt +++ b/charts/kates/templates/NOTES.txt @@ -77,5 +77,5 @@ {{- if .Values.cleanup.enabled }} Cleanup: - Schedule: {{ .Values.cleanup.schedule }} | Retention: {{ .Values.cleanup.retentionDays }} days + Schedule: {{ .Values.cleanup.schedule }} | Retention: {{ .Values.cleanup.retentionDays }} days | Statuses: {{ join ", " .Values.cleanup.statuses }} {{- end }} diff --git a/charts/kates/templates/cleanup-cronjob.yaml b/charts/kates/templates/cleanup-cronjob.yaml index 4280ee49..583d5f6a 100644 --- a/charts/kates/templates/cleanup-cronjob.yaml +++ b/charts/kates/templates/cleanup-cronjob.yaml @@ -1,4 +1,10 @@ {{- if .Values.cleanup.enabled }} +{{- $keySecret := .Values.apiKey.existingSecret | default (printf "%s-api-key" (include "kates.fullname" .)) }} +{{- $keyName := .Values.apiKey.secretKey | default "api-key" }} +{{- $statusParams := list }} +{{- range .Values.cleanup.statuses }} +{{- $statusParams = append $statusParams (printf "status=%s" .) }} +{{- end }} apiVersion: batch/v1 kind: CronJob metadata: @@ -13,10 +19,19 @@ spec: failedJobsHistoryLimit: 3 jobTemplate: spec: + # A missing or wrong key, or an API without DELETE /api/tests, fails + # every attempt the same way. Two retries cover a restarting API; the + # default of six only repeats the same failure. + backoffLimit: 2 template: metadata: labels: - {{- include "kates.selectorLabels" . | nindent 12 }} + # Not kates.selectorLabels: those are the API pod's, and the + # chart's NetworkPolicy selects on them. It allows that pod no + # egress to port 8080, so a job pod carrying them could not reach + # the API at all. The Service and the PodDisruptionBudget select + # on them too. + {{- include "kates.labels" . | nindent 12 }} app.kubernetes.io/component: cleanup spec: restartPolicy: OnFailure @@ -31,6 +46,15 @@ spec: securityContext: {{- toYaml . | nindent 14 }} {{- end }} + {{- if .Values.apiKey.enabled }} + env: + - name: KATES_API_KEY + valueFrom: + secretKeyRef: + name: {{ $keySecret }} + key: {{ $keyName }} + optional: true + {{- end }} resources: requests: memory: "32Mi" @@ -42,27 +66,105 @@ spec: - sh - -c - | - echo "Cleaning up test runs older than {{ .Values.cleanup.retentionDays }} days..." - CUTOFF=$(date -u -d "-{{ .Values.cleanup.retentionDays }} days" +%Y-%m-%dT%H:%M:%S 2>/dev/null || date -u -v-{{ .Values.cleanup.retentionDays }}d +%Y-%m-%dT%H:%M:%S) - RESPONSE=$(curl -s -w "\n%{http_code}" \ - "http://{{ include "kates.fullname" . }}:{{ .Values.service.port }}/api/tests?size=200&status=COMPLETED") - HTTP_CODE=$(echo "$RESPONSE" | tail -1) - BODY=$(echo "$RESPONSE" | head -1) - if [ "$HTTP_CODE" != "200" ]; then - echo "Failed to fetch tests (HTTP $HTTP_CODE)" + # The API picks the runs: DELETE /api/tests deletes finished + # runs created before the cutoff, oldest first, at most 1000 a + # call, and says how many still match. This job used to pick + # them itself, from GET /api/tests?status=COMPLETED, a status + # the API refuses, sent with no key, and it paired fields in + # an order the JSON never has, so it never deleted a run. + DAYS={{ .Values.cleanup.retentionDays | int }} + # busybox date, in this image, and GNU date both read + # -d @SECONDS. busybox reads neither "-N days" nor BSD's -v, + # which left the old cutoff empty. + CUTOFF=$(date -u -d "@$(( $(date -u +%s) - DAYS * 86400 ))" +%Y-%m-%dT%H:%M:%SZ) + if [ -z "$CUTOFF" ]; then + echo "FAIL: this image's date could not compute the cutoff $DAYS days back." exit 1 fi - echo "$BODY" | grep -o '"id":"[^"]*","createdAt":"[^"]*"' | while read PAIR; do - ID=$(echo "$PAIR" | grep -o '"id":"[^"]*"' | sed 's/"id":"//;s/"//') - CREATED=$(echo "$PAIR" | grep -o '"createdAt":"[^"]*"' | sed 's/"createdAt":"//;s/"//') - if [ "$CREATED" \< "$CUTOFF" ]; then - echo "Deleting old test run: $ID (created $CREATED, before cutoff $CUTOFF)" - curl -s -X DELETE "http://{{ include "kates.fullname" . }}:{{ .Values.service.port }}/api/tests/$ID" - else - echo "Keeping test run: $ID (created $CREATED, after cutoff $CUTOFF)" + URL="http://{{ include "kates.fullname" . }}:{{ .Values.service.port }}/api/tests?createdBefore=${CUTOFF}&{{ join "&" $statusParams }}&limit=1000" + echo "Deleting {{ join ", " .Values.cleanup.statuses }} runs created before $CUTOFF ($DAYS days ago)..." + TOTAL=0 + CALLS=0 + REMAINING= + while [ "$CALLS" -lt 100 ]; do + CALLS=$((CALLS + 1)) + # A failed connection writes no body, so clear the last one. + rm -f /tmp/body + CODE=$(curl -sS -o /tmp/body -w "%{http_code}" -X DELETE \ + --connect-timeout 10 --max-time 600 \ + ${KATES_API_KEY:+-H "X-API-Key: ${KATES_API_KEY}"} \ + "$URL") + BODY=$(cat /tmp/body 2>/dev/null) + case "$CODE" in + 200) + ;; + 401) + # The job's own credentials, not the runs. + {{- if .Values.apiKey.enabled }} + if [ -z "${KATES_API_KEY:-}" ]; then + echo "FAIL: the Kates API wants a key (HTTP 401) and this job sent none:" + echo " Secret {{ $keySecret }} has no {{ $keyName }} key, or it is empty." + else + echo "FAIL: the Kates API refused this job's key (HTTP 401): Secret {{ $keySecret }}" + echo " ({{ $keyName }}) is not the key the API runs with. The API reads it when it" + echo " starts, so restart it after the Secret changes." + fi + {{- else }} + echo "FAIL: the Kates API wants a key (HTTP 401) and apiKey.enabled is false," + echo " so this job has none to send." + {{- end }} + printf '%s\n' "$BODY" + exit 1 + ;; + 403) + echo "FAIL: the Kates API does not let this job's key delete runs (HTTP 403)." + printf '%s\n' "$BODY" + exit 1 + ;; + 404|405) + echo "FAIL: this Kates API has no DELETE /api/tests (HTTP $CODE). It takes an image" + echo " newer than 1.25.0; upgrade the Kates API or set cleanup.enabled=false." + printf '%s\n' "$BODY" + exit 1 + ;; + 000) + echo "FAIL: the Kates API is not reachable at {{ include "kates.fullname" . }}:{{ .Values.service.port }}." + exit 1 + ;; + *) + echo "FAIL: DELETE /api/tests answered HTTP $CODE:" + printf '%s\n' "$BODY" + exit 1 + ;; + esac + # The answer is one line of JSON with plain numbers. + DELETED=$(printf '%s\n' "$BODY" | sed -n 's/.*"deleted"[[:space:]]*:[[:space:]]*\([0-9][0-9]*\).*/\1/p') + REMAINING=$(printf '%s\n' "$BODY" | sed -n 's/.*"remaining"[[:space:]]*:[[:space:]]*\([0-9][0-9]*\).*/\1/p') + if [ -z "$DELETED" ] || [ -z "$REMAINING" ]; then + echo "FAIL: DELETE /api/tests answered 200 without deleted and remaining:" + printf '%s\n' "$BODY" + exit 1 + fi + if [ "$CALLS" -eq 1 ]; then + MATCHED=$(printf '%s\n' "$BODY" | sed -n 's/.*"matched"[[:space:]]*:[[:space:]]*\([0-9][0-9]*\).*/\1/p') + echo "${MATCHED:-?} runs matched." + fi + TOTAL=$((TOTAL + DELETED)) + echo "Call $CALLS: deleted $DELETED, $REMAINING still match." + if [ "$REMAINING" -eq 0 ]; then + echo "Cleanup complete: deleted $TOTAL runs created before $CUTOFF in $CALLS calls." + exit 0 + fi + if [ "$DELETED" -eq 0 ]; then + # Runs match that the API would not delete; going on would + # only ask again. A failed job is what shows it. + echo "FAIL: $REMAINING runs still match and the last call deleted none of them." + echo " Deleted $TOTAL in $CALLS calls; the Kates API's log says why." + exit 1 fi done - echo "Cleanup complete." + echo "Stopped after $CALLS calls: deleted $TOTAL runs, $REMAINING still match." + echo "The next scheduled run goes on from there." volumeMounts: - name: tmp mountPath: /tmp diff --git a/charts/kates/values.schema.json b/charts/kates/values.schema.json index a12c5944..c69e579e 100644 --- a/charts/kates/values.schema.json +++ b/charts/kates/values.schema.json @@ -705,6 +705,13 @@ "enabled": { "type": "boolean" }, "schedule": { "type": "string" }, "retentionDays": { "type": "integer", "minimum": 1 }, + "statuses": { + "type": "array", + "description": "Which finished runs the job deletes. Not empty: the API reads no status as both", + "minItems": 1, + "uniqueItems": true, + "items": { "type": "string", "enum": [ "DONE", "FAILED" ] } + }, "image": { "type": "string", "minLength": 1 } } } diff --git a/charts/kates/values.yaml b/charts/kates/values.yaml index e0f4a1bf..79b0edca 100644 --- a/charts/kates/values.yaml +++ b/charts/kates/values.yaml @@ -703,14 +703,23 @@ migration: memory: "512Mi" cpu: "500m" -# -- Stale test run cleanup CronJob +# -- Stale test run cleanup CronJob. It deletes finished runs through the +# API's DELETE /api/tests, sending the key from apiKey's Secret. That +# endpoint takes an image newer than 1.25.0: on 1.25.0 every job fails with +# HTTP 405 and deletes nothing. cleanup: # -- Enable cleanup CronJob enabled: false # -- Cron schedule expression schedule: "0 4 * * 0" - # -- Delete completed test runs older than N days + # -- Delete finished test runs created more than N days ago retentionDays: 30 + # -- Which finished runs to delete: DONE, FAILED or both. A cancelled run + # is stored FAILED. A run still PENDING, RUNNING or STOPPING is never + # deleted, however old it is. + statuses: + - DONE + - FAILED # -- Curl image for API calls image: curlimages/curl:8.7.1 diff --git a/cli/client/client.go b/cli/client/client.go index 812768e5..f7c1c437 100644 --- a/cli/client/client.go +++ b/cli/client/client.go @@ -439,6 +439,47 @@ func (c *Client) DeleteTest(ctx context.Context, id string) error { return c.delete(ctx, path) } +// PruneTests deletes, through DELETE /api/tests, the finished runs with one +// of statuses (DONE, FAILED; none means both) created before createdBefore, +// the oldest first and at most limit of them (0 leaves the API's own limit); +// with dryRun it only counts them. A call that deletes is sent once, as every +// other DELETE the client sends is; a dry run changes nothing, so it is +// retried like a read. +func (c *Client) PruneTests(ctx context.Context, createdBefore time.Time, statuses []string, limit int, dryRun bool) (*PruneResult, error) { + query := url.Values{} + query.Set("createdBefore", createdBefore.UTC().Format(time.RFC3339Nano)) + for _, s := range statuses { + query.Add("status", s) + } + if limit > 0 { + query.Set("limit", strconv.Itoa(limit)) + } + if dryRun { + query.Set("dryRun", "true") + } + req, err := http.NewRequestWithContext(ctx, http.MethodDelete, c.BaseURL+withQuery("/api/tests", query), nil) + if err != nil { + return nil, err + } + data, err := c.doRequest(ctx, req, dryRun) + if err != nil { + // A Kates API from before the endpoint serves GET and POST on + // /api/tests and nothing else, so it refuses a DELETE there as 405. + var httpErr *HTTPError + if errors.As(err, &httpErr) && + (httpErr.StatusCode == http.StatusMethodNotAllowed || httpErr.StatusCode == http.StatusNotFound) { + return nil, fmt.Errorf("this Kates API has no DELETE /api/tests, which prunes finished runs; "+ + "upgrade the Kates API to use it: %w", err) + } + return nil, err + } + var result *PruneResult + if err := json.Unmarshal(data, &result); err != nil || result == nil { + return nil, fmt.Errorf("DELETE /api/tests returned %.40q, not a prune result", data) + } + return result, nil +} + func (c *Client) CancelTest(ctx context.Context, id string) error { path, err := pathf("/api/tests/%s/cancel", id) if err != nil { diff --git a/cli/client/client_test.go b/cli/client/client_test.go index be797b48..4bd94ca3 100644 --- a/cli/client/client_test.go +++ b/cli/client/client_test.go @@ -9,6 +9,7 @@ import ( "net/url" "strings" "sync" + "sync/atomic" "testing" "time" ) @@ -458,6 +459,110 @@ func TestDeleteTest(t *testing.T) { } } +func TestPruneTests(t *testing.T) { + // A cutoff in another zone goes out in UTC, as the API's example has it. + cutoff := time.Date(2026, 9, 7, 2, 0, 0, 0, time.FixedZone("CEST", 2*60*60)) + for _, tt := range []struct { + name string + statuses []string + limit int + dryRun bool + wantQuery url.Values + }{ + {"delete", []string{"DONE", "FAILED"}, 1000, false, + url.Values{"createdBefore": {"2026-09-07T00:00:00Z"}, "status": {"DONE", "FAILED"}, "limit": {"1000"}}}, + {"dry run of one status", []string{"FAILED"}, 1000, true, + url.Values{"createdBefore": {"2026-09-07T00:00:00Z"}, "status": {"FAILED"}, "limit": {"1000"}, "dryRun": {"true"}}}, + {"API defaults", nil, 0, false, + url.Values{"createdBefore": {"2026-09-07T00:00:00Z"}}}, + } { + t.Run(tt.name, func(t *testing.T) { + c, _ := testServer(t, func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodDelete || r.URL.Path != "/api/tests" { + t.Errorf("request = %s %s, want DELETE /api/tests", r.Method, r.URL.Path) + } + if got := r.URL.Query(); got.Encode() != tt.wantQuery.Encode() { + t.Errorf("query = %v, want %v", got, tt.wantQuery) + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"createdBefore":"2026-09-07T00:00:00Z","statuses":["DONE","FAILED"],` + + `"dryRun":false,"matched":1500,"deleted":1000,"remaining":500}`)) + }) + got, err := c.PruneTests(context.Background(), cutoff, tt.statuses, tt.limit, tt.dryRun) + if err != nil { + t.Fatal(err) + } + want := PruneResult{CreatedBefore: "2026-09-07T00:00:00Z", Statuses: []string{"DONE", "FAILED"}, + Matched: 1500, Deleted: 1000, Remaining: 500} + if got.CreatedBefore != want.CreatedBefore || strings.Join(got.Statuses, ",") != "DONE,FAILED" || + got.DryRun || got.Matched != want.Matched || got.Deleted != want.Deleted || got.Remaining != want.Remaining { + t.Errorf("result = %+v, want %+v", got, want) + } + }) + } +} + +// A Kates API from before DELETE /api/tests refuses it as 405, the method +// not allowed on a path that serves GET and POST; the error says to upgrade +// and still carries the status. +func TestPruneTests_OlderAPI(t *testing.T) { + for _, status := range []int{http.StatusMethodNotAllowed, http.StatusNotFound} { + t.Run(http.StatusText(status), func(t *testing.T) { + c, _ := testServer(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(status) + }) + _, err := c.PruneTests(context.Background(), time.Now(), nil, 1000, true) + if err == nil || !strings.Contains(err.Error(), "has no DELETE /api/tests") || !strings.Contains(err.Error(), "upgrade") { + t.Fatalf("err = %v, want one saying the Kates API has no DELETE /api/tests and must be upgraded", err) + } + var httpErr *HTTPError + if !errors.As(err, &httpErr) || httpErr.StatusCode != status { + t.Errorf("err = %v, want it to wrap the HTTP %d", err, status) + } + }) + } +} + +// The API's 400 reaches the caller as it said it, not as an older API. +func TestPruneTests_Refused(t *testing.T) { + c, _ := testServer(t, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"status":400,"error":"Bad Request","message":"status must be DONE or FAILED: only finished runs can be pruned"}`)) + }) + _, err := c.PruneTests(context.Background(), time.Now(), []string{"RUNNING"}, 1000, false) + if err == nil || !strings.Contains(err.Error(), "only finished runs can be pruned") || strings.Contains(err.Error(), "upgrade") { + t.Errorf("err = %v, want the API's own message", err) + } +} + +func TestPruneTests_NotAResult(t *testing.T) { + for _, body := range []string{"", "null", "[]"} { + c, _ := testServer(t, func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(body)) + }) + if got, err := c.PruneTests(context.Background(), time.Now(), nil, 1000, false); err == nil { + t.Errorf("body %q: got %+v, want an error", body, got) + } + } +} + +// A dry run deletes nothing, so a 5xx is retried; a delete is sent once. +func TestPruneTests_RetriesOnlyADryRun(t *testing.T) { + for _, dryRun := range []bool{true, false} { + var calls atomic.Int32 + c, _ := testServer(t, func(w http.ResponseWriter, r *http.Request) { + calls.Add(1) + w.WriteHeader(http.StatusServiceUnavailable) + }) + c.MaxRetries = 2 + _, _ = c.PruneTests(context.Background(), time.Now(), nil, 1000, dryRun) + if want := map[bool]int32{true: 2, false: 1}[dryRun]; calls.Load() != want { + t.Errorf("dryRun %v: %d calls, want %d", dryRun, calls.Load(), want) + } + } +} + func TestTestTypes(t *testing.T) { c, _ := testServer(t, jsonHandler(t, "GET", "/api/tests/types", []string{"LOAD", "STRESS", "SPIKE", "ENDURANCE", "VOLUME", "CAPACITY", "ROUND_TRIP"})) diff --git a/cli/client/escape_test.go b/cli/client/escape_test.go index 39e6b35f..75c7f202 100644 --- a/cli/client/escape_test.go +++ b/cli/client/escape_test.go @@ -15,6 +15,7 @@ import ( "strings" "sync" "testing" + "time" ) // hostileID reaches /api/security/pentest if the client joins it into a path @@ -327,6 +328,15 @@ func TestQueryValuesAreEscaped(t *testing.T) { {"SecurityGate", "/api/security/gate", url.Values{"min-grade": {"B+"}}, func(ctx context.Context, c *Client) error { return ignore(c.SecurityGate(ctx, "B+")) }}, + // The cutoff goes out in UTC, so no "+" offset; a status stays one + // value of the repeated parameter. + {"PruneTests", "/api/tests", + url.Values{"createdBefore": {"2026-09-25T08:00:00Z"}, "status": {"DONE&status=RUNNING", "FAILED#x"}, + "limit": {"1000"}, "dryRun": {"true"}}, + func(ctx context.Context, c *Client) error { + cutoff, _ := time.Parse(time.RFC3339, timestamp) + return ignore(c.PruneTests(ctx, cutoff, []string{"DONE&status=RUNNING", "FAILED#x"}, 1000, true)) + }}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/cli/client/types.go b/cli/client/types.go index 5df5ec8d..868b0643 100644 --- a/cli/client/types.go +++ b/cli/client/types.go @@ -251,6 +251,20 @@ type PagedTests struct { Count int `json:"count"` } +// PruneResult from DELETE /api/tests. Matched counts the runs that matched +// before the call and Remaining those that still match after it; Deleted is +// what this call deleted, 0 for a dry run. CreatedBefore is the cutoff as the +// API read it, and Statuses the finished statuses it pruned, DONE before +// FAILED. +type PruneResult struct { + CreatedBefore string `json:"createdBefore"` + Statuses []string `json:"statuses"` + DryRun bool `json:"dryRun"` + Matched int64 `json:"matched"` + Deleted int64 `json:"deleted"` + Remaining int64 `json:"remaining"` +} + // ReportSummary from GET /api/tests/:id/report/summary type ReportSummary struct { TotalRecords float64 `json:"totalRecords"` diff --git a/cli/cmd/cleanup.go b/cli/cmd/cleanup.go index a8d045a5..8c78d22d 100644 --- a/cli/cmd/cleanup.go +++ b/cli/cmd/cleanup.go @@ -26,8 +26,10 @@ var ( const defaultCleanupOlderThan = 30 * time.Minute var testCleanupCmd = &cobra.Command{ - Use: "cleanup", - Aliases: []string{"gc", "prune"}, + Use: "cleanup", + // prune was an alias too, until kates test prune, which deletes finished + // runs by age, took the name. + Aliases: []string{"gc"}, Short: "Delete orphaned tests stuck in RUNNING state", Long: `Delete test runs that still say RUNNING long after they should have ended. diff --git a/cli/cmd/data/doc_entries.yaml b/cli/cmd/data/doc_entries.yaml index 2f8b5899..8ef03706 100644 --- a/cli/cmd/data/doc_entries.yaml +++ b/cli/cmd/data/doc_entries.yaml @@ -603,6 +603,41 @@ see_also: - test list - test delete + - test prune + +- name: test prune + category: Test + synopsis: "kates test prune --older-than [flags]" + short: "Delete finished test runs created before a cutoff" + description: | + Deletes the finished runs, DONE or FAILED (a cancelled run is stored + as FAILED), created more than --older-than ago, with their results, + through DELETE /api/tests. Counts them first and asks; without a + terminal, pass --yes. The Kates API deletes the oldest first, 1000 a + call, and kates calls it until none is left. Needs a Kates API with + DELETE /api/tests. + flags: + - name: "--older-than" + type: string + desc: "Delete the runs created more than this long ago, such as 30d, 720h or 90m (required)" + - name: "--status" + type: strings + default: "DONE,FAILED" + desc: "Statuses of the finished runs to delete: DONE, FAILED or both" + - name: "--dry-run" + type: bool + desc: "Count the runs without deleting them" + - name: "--yes" + short: "-y" + type: bool + desc: "Delete without asking" + examples: + - "kates test prune --older-than 30d --dry-run" + - "kates test prune --older-than 720h" + - "kates test prune --older-than 7d --status FAILED --yes -o json" + see_also: + - test cleanup + - test delete - name: test compare category: Test diff --git a/cli/cmd/mcp_caveats.go b/cli/cmd/mcp_caveats.go index 64fde0b6..51ebec23 100644 --- a/cli/cmd/mcp_caveats.go +++ b/cli/cmd/mcp_caveats.go @@ -164,7 +164,7 @@ var mcpCaveatsCore = []mcpCaveat{ Refs: []string{ mcpJava + "trend/TrendResource.java:28-57", mcpJava + "trend/TrendService.java:53-54,185-189", - mcpAnchoredRef(mcpJava+"service/TestRunRepository.java:317-337", + mcpAnchoredRef(mcpJava+"service/TestRunRepository.java:354-374", "List findWithResults(", "WHERE r.testType = :type AND r.status = :status", ".map(EntityMapper::toDomain)"), }, diff --git a/cli/cmd/mcp_prompts_cluster.go b/cli/cmd/mcp_prompts_cluster.go index 0cf6708c..6f846e06 100644 --- a/cli/cmd/mcp_prompts_cluster.go +++ b/cli/cmd/mcp_prompts_cluster.go @@ -91,8 +91,8 @@ func mcpDidKatesCauseThis(_ context.Context, req *mcp.GetPromptRequest) (*mcp.Ge // and audit rows no actor (domain/TestRun.java:16-42, // disruption/DisruptionReportEntity.java:15-34, // persistence/AuditEventEntity.java:17-32); only test create, delete and -// cancel through the REST API write audit rows (api/TestResource.java:114,225, -// 325,357), while scheduled and gRPC runs go straight to the orchestrator +// cancel through the REST API write audit rows (api/TestResource.java:132,243, +// 470,502), while scheduled and gRPC runs go straight to the orchestrator // (schedule/TestScheduler.java:92, grpc/GrpcTestService.java:61). // // The window it asks for reaches well before the problem, and the answer it diff --git a/cli/cmd/mcp_tools_runs.go b/cli/cmd/mcp_tools_runs.go index 6f4a9f0d..0f154574 100644 --- a/cli/cmd/mcp_tools_runs.go +++ b/cli/cmd/mcp_tools_runs.go @@ -30,7 +30,7 @@ import ( // Every tool goes through addReadTool and reads only with GET. get_run and // assess_run read the run with GET /api/tests/{id}, which makes the backend // poll a run that is still active and save what it finds -// (TestResource.java:294-300, TestOrchestrator.refreshStatus); both +// (TestResource.java:439-445, TestOrchestrator.refreshStatus); both // descriptions say so. Third-party text (task errors, scenario and phase // names, labels, plan names, audit details, backend messages) is fenced; // ids, types and statuses are cleaned so an agent can pass them back. @@ -1284,7 +1284,7 @@ func mcpRunHasRequested(r *client.MCPRun) bool { // mcpAssessBaseline finds the baseline run: among the runs the band's scan // read, or else with one read of its own. The baseline may be any run, of -// any type or status (TestResource.java:420-448 checks only that it exists), +// any type or status (TestResource.java:565-593 checks only that it exists), // so reading it polls it if it is still active, as get_run does. func mcpAssessBaseline(ctx context.Context, call *mcpCall, baselineID string, band mcpBandScan) *mcpRunIdentity { if b, ok := band.byID[baselineID]; ok { @@ -2264,7 +2264,7 @@ var mcpCaveatsRuns = []mcpCaveat{ Refs: []string{ mcpAnchoredRef(mcpJava+"engine/TestOrchestrator.java:2011-2089", `"Cancelled by user"`, `EventKind.FAILED, "cancelled"`, "return run.withResults(updatedResults);"), - mcpAnchoredRef(mcpJava+"api/TestResource.java:329-374", + mcpAnchoredRef(mcpJava+"api/TestResource.java:474-519", `@Path("/{id}/cancel")`, `auditService.record("CANCEL"`, `"Test cancelled; it is stored as FAILED"`), mcpJava + "domain/TestResult.java:25-31", }, @@ -2350,7 +2350,7 @@ var mcpCaveatsRuns = []mcpCaveat{ "newest matching rows.", Refs: []string{ mcpJava + "persistence/AuditEventEntity.java:15-32", - mcpAnchoredRef(mcpJava+"api/TestResource.java:114,225,325,357", + mcpAnchoredRef(mcpJava+"api/TestResource.java:132,243,470,502", `auditService.record("CREATE"`, `auditService.record("DELETE"`, `auditService.record("CANCEL"`), mcpJava + "service/AuditService.java:52-78", mcpJava + "api/AuditResource.java:43-47", diff --git a/cli/cmd/mcp_tools_scenario.go b/cli/cmd/mcp_tools_scenario.go index 3479e5cf..19b67f0f 100644 --- a/cli/cmd/mcp_tools_scenario.go +++ b/cli/cmd/mcp_tools_scenario.go @@ -1242,7 +1242,7 @@ var mcpCaveatsScenario = []mcpCaveat{ "and kates test apply sends a scenario with whatever key it holds.", Refs: []string{ "plans/mcp-server.md:387-405", - mcpJava + "api/TestResource.java:76-103", + mcpJava + "api/TestResource.java:94-121", mcpJava + "domain/TestSpec.java:34-113", mcpAnchoredRef(mcpJava+"engine/TestOrchestrator.java:1356-1421", "Map inapplicableFields(", "the trogdor backend cannot run a transactional producer"), diff --git a/cli/cmd/prune.go b/cli/cmd/prune.go new file mode 100644 index 00000000..5226bb78 --- /dev/null +++ b/cli/cmd/prune.go @@ -0,0 +1,254 @@ +package cmd + +import ( + "context" + "errors" + "fmt" + "math" + "strconv" + "strings" + "time" + + "github.com/bmscomp/kates/cli/client" + "github.com/bmscomp/kates/cli/output" + "github.com/spf13/cobra" +) + +var ( + pruneOlderThan string + pruneStatuses []string + pruneYes bool + pruneDryRun bool +) + +const ( + // prunePerCall is the most runs one DELETE /api/tests deletes, the most + // the Kates API allows a call. + prunePerCall = 1000 + // pruneMaxCalls bounds one kates test prune at 100,000 runs. Past that it + // stops and says how many are left, rather than run on unattended + // against an API that keeps finding more. + pruneMaxCalls = 100 +) + +// defaultPruneStatuses are the statuses a run ends in: a cancelled run is +// stored as FAILED, so it is pruned with the others. +var defaultPruneStatuses = []string{"DONE", "FAILED"} + +var testPruneCmd = &cobra.Command{ + Use: "prune", + Short: "Delete finished test runs created before a cutoff", + Long: `Delete the finished test runs, DONE or FAILED, created more than --older-than +ago, with their results. A cancelled run is stored as FAILED, so it goes too. +A run still PENDING, RUNNING or STOPPING is never pruned; kates test cleanup +deletes the ones stuck in RUNNING. + +kates counts the runs first and asks before deleting them; without a terminal, +pass --yes. The Kates API deletes the oldest first, at most 1000 a call, and +kates calls it until none is left, at most 100 times. The chart's cleanup +CronJob (cleanup.enabled) makes the same calls on a schedule. + +-o json prints the answer DELETE /api/tests gives, for the whole command: +matched as the first delete found them, deleted in all, and remaining after +the last call. With --dry-run, or when no run matches, it is the count. + +It needs a Kates API with DELETE /api/tests; kates says so when the API it +reaches is older.`, + Example: ` kates test prune --older-than 30d --dry-run + kates test prune --older-than 720h + kates test prune --older-than 7d --status FAILED --yes -o json`, + RunE: runTestPrune, +} + +func runTestPrune(cmd *cobra.Command, args []string) error { + olderThan, err := parseOlderThan(pruneOlderThan) + if err != nil { + return cmdErr(err.Error()) + } + statuses, err := parsePruneStatuses(pruneStatuses) + if err != nil { + return cmdErr(err.Error()) + } + jsonOut := outputMode == "json" + ctx := commandContext(cmd) + + // One cutoff for every call, so the runs deleted are the runs counted + // however long the question waits for its answer. + cutoff := time.Now().UTC().Add(-olderThan).Truncate(time.Second) + before := cutoff.Format(time.RFC3339) + + count, err := apiClient.PruneTests(ctx, cutoff, statuses, prunePerCall, true) + if err != nil { + return cmdErr("Failed to count finished runs: " + err.Error()) + } + found := fmt.Sprintf("Found %s created before %s", pruneRuns(count.Matched, statuses), before) + if count.Matched == 0 || pruneDryRun { + switch { + case jsonOut: + output.JSON(count) + case count.Matched == 0: + output.Success(fmt.Sprintf("No %s run was created before %s.", strings.Join(statuses, " or "), before)) + default: + output.SubHeader(found) + output.Hint("Dry run: nothing deleted. Run without --dry-run to delete them.") + } + return nil + } + + if !jsonOut { + output.SubHeader(found) + } + if !pruneYes { + // The question names the count itself: with -o json nothing above + // it has. + question := fmt.Sprintf("Delete %s and their results?", pruneRuns(count.Matched, statuses)) + if count.Matched == 1 { + question = fmt.Sprintf("Delete %s and its results?", pruneRuns(1, statuses)) + } + ok, err := confirm(question) + if err != nil { + return cmdErr("aborted: " + err.Error()) + } + if !ok { + // A declined destructive action exits non-zero so scripts that + // forgot --yes fail loudly instead of reporting success. + return cmdErr("aborted: no test deleted") + } + } + + total, calls, last, err := pruneAll(ctx, cutoff, statuses, func(res *client.PruneResult) { + // Only a prune that takes more than one call says how it goes. + if !jsonOut && res.Remaining > 0 && res.Deleted > 0 { + output.Hint(fmt.Sprintf("Deleted %s, %d left", plural(int(res.Deleted), "run", "runs"), res.Remaining)) + } + }) + if jsonOut && calls > 0 { + output.JSON(total) + } + deleted := pruneRuns(total.Deleted, statuses) + switch { + case err != nil && calls == 0: + return cmdErr("Failed to delete finished runs: " + err.Error()) + case err != nil: + return cmdErr(fmt.Sprintf("Deleted %s, then failed: %s", deleted, err.Error())) + case total.Remaining > 0 && last.Deleted == 0: + return cmdErr(fmt.Sprintf("Deleted %s; %d more still match, and the Kates API deleted none of them in the last call. Its log may say why.", + deleted, total.Remaining)) + case total.Remaining > 0: + return cmdErr(fmt.Sprintf("Deleted %s in %d calls and stopped with %d left; run kates test prune again to delete them.", + deleted, calls, total.Remaining)) + } + if !jsonOut { + fmt.Println() + output.Success(fmt.Sprintf("Deleted %s created before %s.", deleted, before)) + } + return nil +} + +// pruneAll calls DELETE /api/tests until no run is left to delete, a call +// deletes none, or pruneMaxCalls calls have been made, and passes each answer +// to progress. It returns the answers added up in one PruneResult: Matched +// from the first, Deleted over all of them, Remaining from the last. last is +// the last answer, nil when no call succeeded. +func pruneAll(ctx context.Context, cutoff time.Time, statuses []string, progress func(*client.PruneResult)) (total client.PruneResult, calls int, last *client.PruneResult, err error) { + for calls < pruneMaxCalls { + res, err := apiClient.PruneTests(ctx, cutoff, statuses, prunePerCall, false) + if err != nil { + return total, calls, last, err + } + if calls == 0 { + total.Matched = res.Matched + } + calls++ + last = res + total.CreatedBefore, total.Statuses = res.CreatedBefore, res.Statuses + total.Deleted += res.Deleted + total.Remaining = res.Remaining + progress(res) + // A call that deleted none would be followed by one that finds the + // same runs and deletes none of them either. + if res.Remaining == 0 || res.Deleted == 0 { + break + } + } + return total, calls, last, nil +} + +// pruneRuns says "1500 DONE or FAILED runs" or "1 FAILED run". +func pruneRuns(n int64, statuses []string) string { + noun := "runs" + if n == 1 { + noun = "run" + } + return fmt.Sprintf("%d %s %s", n, strings.Join(statuses, " or "), noun) +} + +// maxOlderThanDays is the most days a time.Duration holds, about 292 years. +const maxOlderThanDays = int64(time.Duration(math.MaxInt64) / (24 * time.Hour)) + +// parseOlderThan reads --older-than: a Go duration such as 720h or 90m, or a +// whole number of days such as 30d, which time.ParseDuration does not read. +func parseOlderThan(s string) (time.Duration, error) { + s = strings.TrimSpace(s) + if s == "" { + return 0, errors.New("--older-than is required: a duration such as 30d, 720h or 90m") + } + bad := fmt.Errorf("--older-than %q is not a duration such as 30d, 720h or 90m", s) + var d time.Duration + if days, ok := strings.CutSuffix(s, "d"); ok { + n, err := strconv.ParseInt(days, 10, 64) + if err != nil || n > maxOlderThanDays || n < -maxOlderThanDays { + return 0, bad + } + d = time.Duration(n) * 24 * time.Hour + } else { + var err error + if d, err = time.ParseDuration(s); err != nil { + return 0, bad + } + } + if d <= 0 { + return 0, errors.New("--older-than must be more than 0") + } + return d, nil +} + +// parsePruneStatuses reads --status, DONE, FAILED or both in any case, and +// returns them as the Kates API names them, DONE before FAILED, once each. +func parsePruneStatuses(values []string) ([]string, error) { + var done, failed bool + for _, v := range values { + switch strings.ToUpper(strings.TrimSpace(v)) { + case "DONE": + done = true + case "FAILED": + failed = true + default: + return nil, fmt.Errorf("--status must be DONE or FAILED, not %q: only finished runs can be pruned", v) + } + } + var statuses []string + if done { + statuses = append(statuses, "DONE") + } + if failed { + statuses = append(statuses, "FAILED") + } + if len(statuses) == 0 { + return nil, errors.New("--status must name DONE, FAILED or both") + } + return statuses, nil +} + +func init() { + testPruneCmd.Flags().StringVar(&pruneOlderThan, "older-than", "", + "Delete the runs created more than this long ago, such as 30d, 720h or 90m (required)") + testPruneCmd.Flags().StringSliceVar(&pruneStatuses, "status", defaultPruneStatuses, + "Statuses of the finished runs to delete: DONE, FAILED or both") + testPruneCmd.Flags().BoolVarP(&pruneYes, "yes", "y", false, "Delete without asking") + testPruneCmd.Flags().BoolVar(&pruneDryRun, "dry-run", false, "Count the runs without deleting them") + _ = testPruneCmd.RegisterFlagCompletionFunc("status", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { + return defaultPruneStatuses, cobra.ShellCompDirectiveNoFileComp + }) + testCmd.AddCommand(testPruneCmd) +} diff --git a/cli/cmd/prune_test.go b/cli/cmd/prune_test.go new file mode 100644 index 00000000..f3e946f9 --- /dev/null +++ b/cli/cmd/prune_test.go @@ -0,0 +1,429 @@ +package cmd + +import ( + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "reflect" + "strconv" + "strings" + "sync" + "testing" + "time" + + "github.com/bmscomp/kates/cli/client" + "github.com/spf13/pflag" +) + +// pruneBackend is DELETE /api/tests over a number of matching runs: a dry run +// counts them, a delete takes up to limit of them, the oldest first, as the +// Kates API does. +type pruneBackend struct { + t *testing.T + + mu sync.Mutex + matching int64 + calls []url.Values // the query of each call, in order + // stuck deletes none of them, as when the API cannot delete the runs it + // picks. + stuck bool + // refuse answers every call with this status, as an older API or one + // that refuses the key does. + refuse int +} + +func newPruneBackend(t *testing.T, matching int64) *pruneBackend { + t.Helper() + b := &pruneBackend{t: t, matching: matching} + ts := httptest.NewServer(http.HandlerFunc(b.serve)) + t.Cleanup(ts.Close) + + prevClient, prevOutput, prevInteractive, prevConfirm := apiClient, outputMode, interactiveAllowedFn, confirmFn + apiClient, outputMode = client.New(ts.URL), "table" + apiClient.MaxRetries = 1 + interactiveAllowedFn = func() bool { return false } + confirmFn = func(string) (bool, error) { + t.Error("asked to confirm") + return false, nil + } + t.Cleanup(func() { + apiClient, outputMode, interactiveAllowedFn, confirmFn = prevClient, prevOutput, prevInteractive, prevConfirm + resetPruneFlags() + }) + return b +} + +// resetPruneFlags puts kates test prune's flags back to their defaults, as a +// new process starts them. A string slice flag, once set, appends what a later +// command line sets to it, and keeps that in its value, so --status gets a new +// value rather than its default back. +func resetPruneFlags() { + pruneOlderThan, pruneYes, pruneDryRun = "", false, false + fresh := pflag.NewFlagSet("prune", pflag.ContinueOnError) + fresh.StringSliceVar(&pruneStatuses, "status", defaultPruneStatuses, "") + status := testPruneCmd.Flags().Lookup("status") + status.Value = fresh.Lookup("status").Value + testPruneCmd.Flags().VisitAll(func(f *pflag.Flag) { f.Changed = false }) +} + +func (b *pruneBackend) serve(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if r.Method != http.MethodDelete || r.URL.Path != "/api/tests" { + b.t.Errorf("unexpected request %s %s", r.Method, r.URL) + w.WriteHeader(http.StatusTeapot) + return + } + q := r.URL.Query() + b.mu.Lock() + defer b.mu.Unlock() + b.calls = append(b.calls, q) + if b.refuse != 0 { + w.WriteHeader(b.refuse) + return + } + limit, err := strconv.ParseInt(q.Get("limit"), 10, 64) + if err != nil { + b.t.Errorf("limit = %q", q.Get("limit")) + } + statuses := q["status"] + if len(statuses) == 0 { + statuses = []string{"DONE", "FAILED"} + } + dryRun := q.Get("dryRun") == "true" + matched := b.matching + var deleted int64 + if !dryRun && !b.stuck { + deleted = min(limit, b.matching) + b.matching -= deleted + } + _ = json.NewEncoder(w).Encode(client.PruneResult{ + CreatedBefore: q.Get("createdBefore"), Statuses: statuses, DryRun: dryRun, + Matched: matched, Deleted: deleted, Remaining: b.matching, + }) +} + +// requests returns the query of each call, and the runs still matching. +func (b *pruneBackend) requests() ([]url.Values, int64) { + b.mu.Lock() + defer b.mu.Unlock() + return append([]url.Values(nil), b.calls...), b.matching +} + +// dryRuns says, call by call, which were dry runs. +func dryRuns(calls []url.Values) []bool { + var out []bool + for _, q := range calls { + out = append(out, q.Get("dryRun") == "true") + } + return out +} + +// The count comes first, with the cutoff --older-than says, and --dry-run +// stops there. +func TestTestPrune_DryRun(t *testing.T) { + b := newPruneBackend(t, 1500) + + before := time.Now().UTC() + stdout := commandStdout(t, func() error { + return runCommandLine(t, "test", "prune", "--older-than", "30d", "--dry-run") + }) + calls, left := b.requests() + if !reflect.DeepEqual(dryRuns(calls), []bool{true}) || left != 1500 { + t.Fatalf("calls %v, %d left; want one dry run, 1500 left", calls, left) + } + q := calls[0] + if !reflect.DeepEqual(q["status"], []string{"DONE", "FAILED"}) || q.Get("limit") != "1000" { + t.Errorf("query = %v, want status DONE and FAILED, limit 1000", q) + } + cutoff, err := time.Parse(time.RFC3339, q.Get("createdBefore")) + if err != nil { + t.Fatalf("createdBefore %q: %v", q.Get("createdBefore"), err) + } + if want := before.Add(-30 * 24 * time.Hour); cutoff.Before(want.Add(-2*time.Second)) || cutoff.After(want.Add(2*time.Second)) { + t.Errorf("createdBefore = %s, want about %s", cutoff, want) + } + for _, want := range []string{ + "Found 1500 DONE or FAILED runs created before " + q.Get("createdBefore"), + "Dry run: nothing deleted", + } { + if !strings.Contains(string(stdout), want) { + t.Errorf("output lacks %q:\n%s", want, stdout) + } + } +} + +// --yes deletes 1000 a call until none is left, with the cutoff the count +// used. +func TestTestPrune_YesDeletesInCalls(t *testing.T) { + b := newPruneBackend(t, 1500) + + stdout := commandStdout(t, func() error { + return runCommandLine(t, "test", "prune", "--older-than", "720h", "--yes") + }) + calls, left := b.requests() + if !reflect.DeepEqual(dryRuns(calls), []bool{true, false, false}) || left != 0 { + t.Fatalf("dry runs %v, %d left; want a count, then two deletes, and none left", dryRuns(calls), left) + } + for i, q := range calls { + if q.Get("createdBefore") != calls[0].Get("createdBefore") || q.Get("limit") != "1000" { + t.Errorf("call %d: %v, want the count's cutoff and limit 1000", i, q) + } + } + for _, want := range []string{ + "Found 1500 DONE or FAILED runs", + "Deleted 1000 runs, 500 left", + "Deleted 1500 DONE or FAILED runs created before " + calls[0].Get("createdBefore"), + } { + if !strings.Contains(string(stdout), want) { + t.Errorf("output lacks %q:\n%s", want, stdout) + } + } +} + +// --status narrows every call to the statuses it names. +func TestTestPrune_Status(t *testing.T) { + b := newPruneBackend(t, 3) + + commandStdout(t, func() error { + return runCommandLine(t, "test", "prune", "--older-than", "7d", "--status", "failed", "--yes") + }) + calls, _ := b.requests() + if len(calls) != 2 { + t.Fatalf("%d calls, want a count and a delete", len(calls)) + } + for i, q := range calls { + if !reflect.DeepEqual(q["status"], []string{"FAILED"}) { + t.Errorf("call %d: status %v, want [FAILED]", i, q["status"]) + } + } +} + +func TestTestPrune_NothingToDelete(t *testing.T) { + b := newPruneBackend(t, 0) + + stdout := commandStdout(t, func() error { + return runCommandLine(t, "test", "prune", "--older-than", "30d") + }) + if calls, _ := b.requests(); !reflect.DeepEqual(dryRuns(calls), []bool{true}) { + t.Errorf("dry runs %v, want the count alone", dryRuns(calls)) + } + if !strings.Contains(string(stdout), "No DONE or FAILED run was created before") { + t.Errorf("output does not say none matched:\n%s", stdout) + } +} + +// A call that deletes none stops the loop: the next would find the same runs. +// The runs left make the command fail. +func TestTestPrune_CallThatDeletesNone(t *testing.T) { + b := newPruneBackend(t, 5) + b.stuck = true + + _, stderr, err := commandOutput(t, func() error { + return runCommandLine(t, "test", "prune", "--older-than", "30d", "--yes") + }) + if _, ok := err.(*silentErr); !ok { + t.Fatalf("err = %v (%T), want a silentErr, which exits 1", err, err) + } + if calls, _ := b.requests(); !reflect.DeepEqual(dryRuns(calls), []bool{true, false}) { + t.Errorf("dry runs %v, want a count and one delete", dryRuns(calls)) + } + if !strings.Contains(stderr, "5 more still match") || !strings.Contains(stderr, "deleted none") { + t.Errorf("the error does not say the runs left and that the call deleted none:\n%s", stderr) + } +} + +// The calls stop at 100, 100,000 runs, and say how many are left. +func TestTestPrune_StopsAfterMaxCalls(t *testing.T) { + b := newPruneBackend(t, pruneMaxCalls*prunePerCall+700) + + _, stderr, err := commandOutput(t, func() error { + return runCommandLine(t, "test", "prune", "--older-than", "30d", "--yes") + }) + if _, ok := err.(*silentErr); !ok { + t.Fatalf("err = %v (%T), want a silentErr, which exits 1", err, err) + } + calls, left := b.requests() + if len(calls) != 1+pruneMaxCalls || left != 700 { + t.Errorf("%d calls, %d left; want %d calls and 700 left", len(calls), left, 1+pruneMaxCalls) + } + if !strings.Contains(stderr, "stopped with 700 left") { + t.Errorf("the error does not say how many are left:\n%s", stderr) + } +} + +// Without a terminal, prune counts and refuses unless --yes says to go +// ahead, as kates test cleanup does. +func TestTestPrune_NoTerminalNeedsYes(t *testing.T) { + b := newPruneBackend(t, 3) + + stdout, stderr, err := commandOutput(t, func() error { + return runCommandLine(t, "test", "prune", "--older-than", "30d") + }) + if _, ok := err.(*silentErr); !ok { + t.Fatalf("err = %v (%T), want a silentErr, which exits 1", err, err) + } + if calls, left := b.requests(); !reflect.DeepEqual(dryRuns(calls), []bool{true}) || left != 3 { + t.Errorf("dry runs %v, %d left; want the count alone and nothing deleted", dryRuns(calls), left) + } + if !strings.Contains(stderr, "--yes") { + t.Errorf("the refusal does not mention --yes:\n%s", stderr) + } + if !strings.Contains(string(stdout), "Found 3 DONE or FAILED runs") { + t.Errorf("the count is not shown before the refusal:\n%s", stdout) + } +} + +func TestTestPrune_AsksInATerminal(t *testing.T) { + for _, tt := range []struct { + matching int64 + answer bool + question string + }{ + {3, false, "Delete 3 DONE or FAILED runs and their results?"}, + {3, true, "Delete 3 DONE or FAILED runs and their results?"}, + {1, true, "Delete 1 DONE or FAILED run and its results?"}, + } { + t.Run(fmt.Sprintf("%d runs, answer %v", tt.matching, tt.answer), func(t *testing.T) { + b := newPruneBackend(t, tt.matching) + interactiveAllowedFn = func() bool { return true } + var asked string + confirmFn = func(q string) (bool, error) { asked = q; return tt.answer, nil } + + _, _, err := commandOutput(t, func() error { + return runCommandLine(t, "test", "prune", "--older-than", "30d") + }) + if asked != tt.question { + t.Errorf("question = %q, want %q", asked, tt.question) + } + _, left := b.requests() + if tt.answer && (err != nil || left != 0) { + t.Errorf("after yes: err = %v, %d left; want nil and none", err, left) + } + if !tt.answer && (err == nil || left != tt.matching) { + t.Errorf("after no: err = %v, %d left; want an error and nothing deleted", err, left) + } + }) + } +} + +// An API from before DELETE /api/tests answers 405; kates says to upgrade it. +func TestTestPrune_OlderAPI(t *testing.T) { + b := newPruneBackend(t, 3) + b.refuse = http.StatusMethodNotAllowed + + _, stderr, err := commandOutput(t, func() error { + return runCommandLine(t, "test", "prune", "--older-than", "30d", "--yes") + }) + if _, ok := err.(*silentErr); !ok { + t.Fatalf("err = %v (%T), want a silentErr, which exits 1", err, err) + } + if calls, _ := b.requests(); len(calls) != 1 { + t.Errorf("%d calls, want the count alone", len(calls)) + } + if !strings.Contains(stderr, "has no DELETE /api/tests") || !strings.Contains(stderr, "upgrade") { + t.Errorf("the error does not say to upgrade the Kates API:\n%s", stderr) + } +} + +// Bad flags are refused before anything is sent. +func TestTestPrune_FlagValidation(t *testing.T) { + for _, tt := range []struct { + args []string + want string + }{ + {nil, "--older-than is required"}, + {[]string{"--older-than", "0"}, "must be more than 0"}, + {[]string{"--older-than", "0d"}, "must be more than 0"}, + {[]string{"--older-than", "-1h"}, "must be more than 0"}, + {[]string{"--older-than", "-30d"}, "must be more than 0"}, + {[]string{"--older-than", "30x"}, "is not a duration"}, + {[]string{"--older-than", "1.5d"}, "is not a duration"}, + {[]string{"--older-than", "999999999d"}, "is not a duration"}, + {[]string{"--older-than", "30d", "--status", "RUNNING"}, "--status must be DONE or FAILED"}, + {[]string{"--older-than", "30d", "--status", "DONE,PENDING"}, "--status must be DONE or FAILED"}, + {[]string{"--older-than", "30d", "--status", ""}, "--status must name DONE, FAILED or both"}, + } { + t.Run(strings.Join(append([]string{"prune"}, tt.args...), " "), func(t *testing.T) { + b := newPruneBackend(t, 3) + + _, stderr, err := commandOutput(t, func() error { + return runCommandLine(t, append([]string{"test", "prune", "--yes"}, tt.args...)...) + }) + if err == nil || !strings.Contains(stderr, tt.want) { + t.Errorf("err = %v, stderr %q; want an error saying %q", err, stderr, tt.want) + } + if calls, _ := b.requests(); len(calls) != 0 { + t.Errorf("sent %v", calls) + } + }) + } +} + +func TestParseOlderThan(t *testing.T) { + for in, want := range map[string]time.Duration{ + "30d": 30 * 24 * time.Hour, + " 7d ": 7 * 24 * time.Hour, + "+2d": 48 * time.Hour, + "720h": 720 * time.Hour, + "90m": 90 * time.Minute, + "1h30m": 90 * time.Minute, + "1.5h": 90 * time.Minute, + "106751d": time.Duration(maxOlderThanDays) * 24 * time.Hour, + } { + got, err := parseOlderThan(in) + if err != nil || got != want { + t.Errorf("parseOlderThan(%q) = %s, %v; want %s", in, got, err, want) + } + } +} + +func TestParsePruneStatuses(t *testing.T) { + for _, tt := range []struct { + in []string + want []string + }{ + {[]string{"DONE", "FAILED"}, []string{"DONE", "FAILED"}}, + {[]string{"failed", "done"}, []string{"DONE", "FAILED"}}, + {[]string{" Failed ", "FAILED"}, []string{"FAILED"}}, + {[]string{"done"}, []string{"DONE"}}, + } { + got, err := parsePruneStatuses(tt.in) + if err != nil || !reflect.DeepEqual(got, tt.want) { + t.Errorf("parsePruneStatuses(%q) = %v, %v; want %v", tt.in, got, err, tt.want) + } + } +} + +// -o json prints the answer for the whole command and nothing else: with +// --dry-run the count, otherwise matched as the first delete found it, +// deleted over every call, and remaining after the last. +func TestTestPrune_JSON(t *testing.T) { + t.Run("dry run", func(t *testing.T) { + b := newPruneBackend(t, 1500) + + stdout := commandStdout(t, func() error { + return runCommandLine(t, "test", "prune", "--older-than", "30d", "--dry-run", "-o", "json") + }) + calls, _ := b.requests() + want := fmt.Sprintf(`{"createdBefore":%q,"statuses":["DONE","FAILED"],"dryRun":true,"matched":1500,"deleted":0,"remaining":1500}`, + calls[0].Get("createdBefore")) + if got := compactJSON(t, stdout); got != want { + t.Errorf("stdout = %s, want %s", got, want) + } + }) + t.Run("delete", func(t *testing.T) { + b := newPruneBackend(t, 1500) + + stdout := commandStdout(t, func() error { + return runCommandLine(t, "test", "prune", "--older-than", "30d", "--yes", "-o", "json") + }) + calls, _ := b.requests() + want := fmt.Sprintf(`{"createdBefore":%q,"statuses":["DONE","FAILED"],"dryRun":false,"matched":1500,"deleted":1500,"remaining":0}`, + calls[0].Get("createdBefore")) + if got := compactJSON(t, stdout); got != want { + t.Errorf("stdout = %s, want %s", got, want) + } + }) +} diff --git a/cli/cmd/root.go b/cli/cmd/root.go index 9364d7a3..10721d28 100644 --- a/cli/cmd/root.go +++ b/cli/cmd/root.go @@ -340,6 +340,7 @@ Testing: test apply Apply a YAML test scenario definition test delete Delete a test run test cleanup Delete orphaned RUNNING tests + test prune Delete finished runs older than a cutoff test compare Side-by-side metric diff of two runs test summary Aggregate stats across all tests test export Export results to CSV or JSON file diff --git a/docs/book/02-architecture.md b/docs/book/02-architecture.md index c46a8bca..82543e08 100644 --- a/docs/book/02-architecture.md +++ b/docs/book/02-architecture.md @@ -379,7 +379,7 @@ The table lists what Kates keeps about a run, where each piece lives and how lon | What | Where it lives | How long it lasts | |:--|:--|:--| -| The run: its spec, status, per-task results, an INTEGRITY task's integrity result among them, and SLA | PostgreSQL: the `kates` chart's own, beside the Kates API, or an external database | 90 days; once a day the Kates API deletes finished runs older than that | +| The run: its spec, status, per-task results, an INTEGRITY task's integrity result among them, and SLA | PostgreSQL: the `kates` chart's own, beside the Kates API, or an external database | 90 days; once a day the Kates API deletes finished runs older than that, and `kates test prune` deletes them sooner | | Disruption reports, schedules, webhooks and audit events | PostgreSQL | No automatic expiry | | A baseline: the run each test type is compared with | PostgreSQL | The baseline never expires; the run it names is deleted at 90 days like any other | | The report of a finished run, as `report show` prints it | Built from the stored run when first asked for, then kept in the Kates API's memory | The 200 most recently used, until the pod restarts; then built again | diff --git a/docs/book/10-cli-reference.md b/docs/book/10-cli-reference.md index a306d9c6..37546146 100644 --- a/docs/book/10-cli-reference.md +++ b/docs/book/10-cli-reference.md @@ -682,7 +682,7 @@ Cancel runs that are `PENDING` or `RUNNING` and keep them. Each run's tasks stop #### test cleanup -Aliases: `gc`, `prune` +Aliases: `gc` ```bash kates test cleanup --dry-run @@ -692,7 +692,33 @@ kates test cleanup --older-than 2h --yes Delete runs that are still `RUNNING` long after they should have ended. A run counts as orphaned when it is more than `--older-than` (default `30m`) past its planned end, which is its start plus the `durationMs` in its spec, twice that for an INTEGRITY run, which reads its records back for up to as long again. A run of a phased `scenario` sent to `POST /api/tests` is the exception, since its spec shows none of its phases, which run one after another. Its planned end is its start plus two hours, the longest the Kates API lets one last by default (`kates.engine.max-duration-ms`). The command lists those runs and asks before deleting them; without a terminal it refuses unless `--yes` is given. `--dry-run` only lists them. Deleting a run stops it and removes it with its results, as `kates test delete` does. To stop a run and keep it, use `kates test cancel`. The CLI exits 1 when a delete fails or when you decline. -The Kates API already marks a run `FAILED` once it is still `RUNNING` five minutes past its planned end (see the callout under `test create`), so a run this command finds is one that check did not catch. +The Kates API already marks a run `FAILED` once it is still `RUNNING` five minutes past its planned end (see the callout under `test create`), so a run this command finds is one that check did not catch. To delete finished runs by age, use `kates test prune`. + +#### test prune + +```bash +kates test prune --older-than 30d --dry-run +kates test prune --older-than 30d +kates test prune --older-than 720h --status FAILED --yes +kates test prune --older-than 60d --yes -o json +``` + +`--older-than` is the one flag you must give; the others narrow the runs or skip the question: + +| Flag | Default | Description | +|------|---------|-------------| +| `--older-than` | | Required. How long ago a run must have been created to go: a Go duration such as `720h`, or whole days, such as `30d` | +| `--status` | `DONE,FAILED` | The statuses to delete: `DONE`, `FAILED` or both, comma-separated, in any case | +| `--dry-run` | `false` | Count the runs and delete nothing | +| `--yes`, `-y` | `false` | Delete without asking | + +Delete the finished runs created more than `--older-than` ago, to keep the run history to a retention period of your own. Where `kates test cleanup` repairs, deleting runs still `RUNNING` long after their planned end, `prune` keeps a retention: it deletes only `DONE` and `FAILED` runs, judged by when they were created. A cancelled run is stored as `FAILED`, so it goes too. + +The command first counts the runs, deleting none, and prints how many it found, their statuses and the instant they were created before. It stops there when none match, or with `--dry-run`. Otherwise it asks before deleting them; without a terminal it refuses unless `--yes` is given, and it exits 1 when you decline. It then deletes the oldest first, up to 1,000 runs a call, until none are left, and prints the total. Each run goes as `kates test delete` deletes it, with its results, and leaves a row in the Kates API's audit log. + +The command stops after 100 calls, or after a call that deleted nothing. When runs still match then, it exits 1 and says how many are left; run it again to go on. With `-o json` it prints one answer for the whole command, in the fields of [DELETE /api/tests](11-api-reference.md#delete-apitests). `deleted` adds up every call, `matched` is the count the first delete found and `remaining` the one after the last; with `--dry-run`, or when none match, it prints the count. + +Once a day the Kates API deletes, on its own, every run older than 90 days that is not `RUNNING` or `PENDING`, so `prune` is for keeping finished runs for less long. To prune on a schedule, enable the `kates` chart's cleanup CronJob, described in [Where Kates Stores Test Data](12-deployment.md#where-kates-stores-test-data). A Kates API too old to have `DELETE /api/tests` answers `405`, and the command fails, saying to upgrade it. #### test watch diff --git a/docs/book/11-api-reference.md b/docs/book/11-api-reference.md index ade27cda..caf957d5 100644 --- a/docs/book/11-api-reference.md +++ b/docs/book/11-api-reference.md @@ -330,6 +330,56 @@ Stop and delete a test run with its results. A run that is still `PENDING` or `R **Response:** `204 No Content` on success. Returns `404 Not Found` if the test ID does not exist. +#### DELETE /api/tests + +Delete the finished runs created before a cutoff, to keep the run history to a retention period of your own. Only `DONE` and `FAILED` runs match, and a cancelled run is stored as `FAILED`, so it matches too. A run still `PENDING`, `RUNNING` or `STOPPING` is never deleted here; [test cleanup](10-cli-reference.md#test-cleanup) deals with one left `RUNNING`. `kates test prune` and the `kates` chart's cleanup CronJob call this endpoint. + +A call deletes the oldest matching runs first, by `createdAt`, and at most `limit` of them. Each goes as `DELETE /api/tests/{id}` deletes it, with its results, and leaves a row in the audit log: action `DELETE`, type `test`, the run's ID, and the details `retention: created before `. `kates audit --type test` lists them. Once a day the Kates API also deletes, on its own, every run older than 90 days (`kates.cleanup.retention-days`) that is not `RUNNING` or `PENDING`, without an audit row; this endpoint is for keeping finished runs for less long. + +**Query Parameters:** + +| Parameter | Type | Default | Description | +|-----------|------|---------|-------------| +| `createdBefore` | String | | Required. An ISO-8601 instant, such as `2026-09-07T00:00:00Z`; a run created before it matches | +| `status` | String | `DONE` and `FAILED` | `DONE` or `FAILED`, in any case; repeat the parameter to name both | +| `limit` | int | 1000 | The most runs one call deletes, from 1 to 1000 | +| `dryRun` | boolean | `false` | Count the matching runs and delete nothing | + +Count the matching runs first with `dryRun`, then delete them: + +```bash +BASE="http://localhost:30083" +curl -s -X DELETE -H "X-API-Key: $KATES_API_KEY" \ + "$BASE/api/tests?createdBefore=2026-09-07T00:00:00Z&dryRun=true" +curl -s -X DELETE -H "X-API-Key: $KATES_API_KEY" \ + "$BASE/api/tests?createdBefore=2026-09-07T00:00:00Z&status=DONE&status=FAILED" +``` + +**Response:** `200 OK`, for the second call + +```json +{ + "createdBefore": "2026-09-07T00:00:00Z", + "statuses": ["DONE", "FAILED"], + "dryRun": false, + "matched": 1240, + "deleted": 1000, + "remaining": 240 +} +``` + +`matched` counts the runs that matched before the call, `deleted` the runs it deleted, 0 for a dry run, and `remaining` those that still match after it. While `remaining` is above 0, call again for the next oldest. A run that another caller deleted first isn't counted in `deleted`. `createdBefore` is the cutoff in UTC, as the Kates API read it, and `statuses` names the statuses the call matched, in the order `DONE`, `FAILED`. + +A request the Kates API refuses deletes nothing and gets a `400` in the shared format of [Error Responses](#error-responses), whose `message` is one of these: + +| `message` | Cause | +|-----------|-------| +| `createdBefore is required: an ISO-8601 instant such as 2026-09-07T00:00:00Z` | No `createdBefore` | +| `createdBefore must be an ISO-8601 instant such as 2026-09-07T00:00:00Z` | A value that isn't an instant, such as a date without a time or a zone | +| `status must be DONE or FAILED: only finished runs can be pruned` | Any other `status`, such as `RUNNING` | +| `limit must be from 1 to 1000` | A `limit` outside that range, or not a whole number | +| `dryRun must be true or false` | Any other `dryRun`, such as `yes` | + --- ### Reports diff --git a/docs/book/12-deployment.md b/docs/book/12-deployment.md index 76749288..3249b396 100644 --- a/docs/book/12-deployment.md +++ b/docs/book/12-deployment.md @@ -77,6 +77,18 @@ The default deployment uses **NodePort** for all services (Grafana on 30080, Kaf The Kates API keeps every run in PostgreSQL. The `kates` chart runs its own PostgreSQL on a 1 GiB PersistentVolumeClaim on the cluster's default StorageClass, so runs survive pod restarts; there is no emptyDir mode. For production, point the chart at an external database instead, with `postgresql.enabled: false` and `externalDatabase`, as `values-prod.yaml` does. Either way, the Kates API deletes finished runs older than 90 days by default, and [Architecture & Design](02-architecture.md) describes what else it keeps. +To keep runs for less time, set `cleanup.enabled: true` in the `kates` chart's values. Its CronJob then deletes the finished runs older than `cleanup.retentionDays`, through [DELETE /api/tests](11-api-reference.md#delete-apitests), with the key from the Secret the `apiKey` values name. The table gives the values and their defaults: + +| Value | Default | What it sets | +|-------|---------|--------------| +| `cleanup.enabled` | `false` | Whether the chart renders the CronJob | +| `cleanup.schedule` | `0 4 * * 0` | When the job runs: every Sunday at 04:00, in the time zone of the cluster's controller manager, usually UTC | +| `cleanup.retentionDays` | `30` | How many days a finished run is kept, counted from its creation | +| `cleanup.statuses` | `[DONE, FAILED]` | Which finished runs go; a cancelled run is stored as `FAILED` | +| `cleanup.image` | `curlimages/curl:8.7.1` | The image the job runs `curl` in | + +Each time it runs, the job deletes the oldest first, up to 1,000 runs a call, until none older are left or it has made 100 calls, and the next job goes on from there. Its log ends with how many it deleted. A call the Kates API refuses fails the job, with the answer in its log. A `401` or `403` there means the key is missing or wrong, and a `405` that the Kates API is too old to have `DELETE /api/tests`. The Kates API's own daily sweep deletes every run that is not `RUNNING` or `PENDING` at 90 days by default, so a `retentionDays` of 90 or more adds nothing to it. To prune once, by hand, run `kates test prune`. + For Kafka broker storage, **always use persistent volumes** — even in development. Kafka's log retention depends on data being durable, and losing broker data mid-test invalidates results. ### Deployment Topologies diff --git a/docs/book/appendix-d-versions.md b/docs/book/appendix-d-versions.md index c7b6e4d8..00d352fc 100644 --- a/docs/book/appendix-d-versions.md +++ b/docs/book/appendix-d-versions.md @@ -22,7 +22,7 @@ Every version the platform pins, in one place. This table is generated from the | `kafka-ui` chart | 0.3.0 (app v1.5.0) | `charts/kafka-ui/Chart.yaml` | | `kates-chaos` chart | 2.2.1 (app 3.28.0) | `charts/kates-chaos/Chart.yaml` | | `kates-platform` chart | 0.7.0 (app 1.0.0) | `charts/kates-platform/Chart.yaml` | -| `kates` chart | 0.10.9 (app 1.25.0) | `charts/kates/Chart.yaml` | +| `kates` chart | 0.10.10 (app 1.25.0) | `charts/kates/Chart.yaml` | | `legacy-kafka` chart | 0.2.0 (app 3.9.1) | `charts/legacy-kafka/Chart.yaml` | | `mirror-maker2` chart | 0.11.3 (app 4.3.1) | `charts/mirror-maker2/Chart.yaml` | | `monitoring` chart | 1.6.0 (app 82.4.3) | `charts/monitoring/Chart.yaml` | diff --git a/kates/docs/api-reference.md b/kates/docs/api-reference.md index c391e330..8ce8d00b 100644 --- a/kates/docs/api-reference.md +++ b/kates/docs/api-reference.md @@ -216,6 +216,53 @@ Removes the test run with its results. A run that is still pending or running is --- +### Delete Finished Runs by Age + +``` +DELETE /api/tests?createdBefore=2026-09-07T00:00:00Z +DELETE /api/tests?createdBefore=2026-09-07T00:00:00Z&status=FAILED&dryRun=true +``` + +Deletes the finished runs created before a cutoff, to keep the run history to a retention period of your own. Only `DONE` and `FAILED` runs match; a cancelled run is stored as `FAILED`, so it matches too. A run still pending, running or stopping is never deleted here: `kates test cleanup` deals with one left running. `kates test prune` and the chart's cleanup CronJob (`cleanup.*` in `charts/kates/values.yaml`) call this endpoint. + +A call deletes the oldest matching runs first, by `createdAt`, and at most `limit` of them. Each goes through the same delete as `DELETE /api/tests/{id}`, and leaves an audit row: action `DELETE`, type `test`, the run's ID, and the details `retention: created before `. Once a day the Kates API also deletes, on its own, every run older than `kates.cleanup.retention-days` (90) that is not `RUNNING` or `PENDING`, without an audit row; this endpoint is for keeping finished runs for less long. + +**Query Parameters:** + +| Parameter | Type | Required | Description | +|-----------|------|:---:|-------------| +| `createdBefore` | ISO-8601 instant | Yes | A run created before it matches, such as `2026-09-07T00:00:00Z` | +| `status` | `DONE` or `FAILED` | No | Repeatable, in any case; both when absent | +| `limit` | int | No | The most runs one call deletes, from 1 to 1000 (default 1000) | +| `dryRun` | boolean | No | Count the matching runs and delete nothing (default `false`) | + +**Response: `200 OK`** + +```json +{ + "createdBefore": "2026-09-07T00:00:00Z", + "statuses": ["DONE", "FAILED"], + "dryRun": false, + "matched": 1240, + "deleted": 1000, + "remaining": 240 +} +``` + +`matched` counts the runs that matched before the call, `deleted` the runs it deleted (0 for a dry run), and `remaining` those that still match after it; while `remaining` is above 0, call again. A run another caller deleted first is not counted in `deleted`. `createdBefore` is the cutoff in UTC as the API read it, and `statuses` lists the statuses matched, in the order `DONE`, `FAILED`. + +**Error Responses:** + +| Status | Condition | +|--------|-----------| +| `400 Bad Request` | `createdBefore is required: an ISO-8601 instant such as 2026-09-07T00:00:00Z` | +| `400 Bad Request` | `createdBefore must be an ISO-8601 instant such as 2026-09-07T00:00:00Z` | +| `400 Bad Request` | `status must be DONE or FAILED: only finished runs can be pruned` | +| `400 Bad Request` | `limit must be from 1 to 1000` | +| `400 Bad Request` | `dryRun must be true or false` | + +--- + ### List Available Test Types ``` diff --git a/kates/docs/cli-reference.md b/kates/docs/cli-reference.md index b4237a8b..2cb1271c 100644 --- a/kates/docs/cli-reference.md +++ b/kates/docs/cli-reference.md @@ -68,6 +68,7 @@ kates test delete | `test summary` | Aggregate statistics across all completed tests | | `test flame` | ASCII latency distribution histogram | | `test cleanup` | Delete orphaned RUNNING tests (more than 30 minutes past their planned end) | +| `test prune` | Delete finished (DONE and FAILED) tests created more than `--older-than` ago | | `test export` | Export results to CSV or JSON file | #### test scaffold @@ -147,6 +148,18 @@ kates test cleanup --older-than 2h -y # no prompt, for scripts A test is orphaned when it is still RUNNING more than `--older-than` (default 30 minutes) after its planned end: its start plus the duration in its spec. Without a terminal the command refuses unless `--yes` is given. +#### test prune + +Delete finished tests by age, to keep the run history to a retention period of your own: + +```bash +kates test prune --older-than 30d --dry-run # count them, delete nothing +kates test prune --older-than 30d # count them, then ask before deleting +kates test prune --older-than 720h --status FAILED --yes # no prompt, for scripts +``` + +`--older-than` is required: a Go duration such as `720h`, or whole days such as `30d`. `--status` takes `DONE`, `FAILED` or both (the default); a cancelled test is stored as `FAILED`. Where `test cleanup` deletes tests still RUNNING long past their planned end, `test prune` deletes only finished ones, by when they were created. It counts them first, asks before deleting unless `--yes` is given (without a terminal it refuses), then deletes the oldest first, 1,000 a call, through `DELETE /api/tests`. It stops after 100 calls (100,000 tests) and exits 1 if any still match. `-o json` prints one answer for the whole run, `deleted` added up over the calls. An API too old to have `DELETE /api/tests` makes it fail with a request to upgrade. + #### test export Export test results to a file: diff --git a/kates/src/main/java/com/bmscomp/kates/api/TestResource.java b/kates/src/main/java/com/bmscomp/kates/api/TestResource.java index c6464a07..d3f5c1cd 100644 --- a/kates/src/main/java/com/bmscomp/kates/api/TestResource.java +++ b/kates/src/main/java/com/bmscomp/kates/api/TestResource.java @@ -1,6 +1,11 @@ package com.bmscomp.kates.api; +import java.time.Instant; +import java.time.format.DateTimeParseException; +import java.util.EnumSet; import java.util.List; +import java.util.Locale; +import java.util.Set; import jakarta.inject.Inject; import jakarta.validation.Valid; import jakarta.ws.rs.Consumes; @@ -18,6 +23,9 @@ import io.smallrye.common.annotation.Blocking; import org.eclipse.microprofile.openapi.annotations.Operation; +import org.eclipse.microprofile.openapi.annotations.enums.SchemaType; +import org.eclipse.microprofile.openapi.annotations.media.Content; +import org.eclipse.microprofile.openapi.annotations.media.Schema; import org.eclipse.microprofile.openapi.annotations.parameters.Parameter; import org.eclipse.microprofile.openapi.annotations.responses.APIResponse; import org.eclipse.microprofile.openapi.annotations.tags.Tag; @@ -25,6 +33,7 @@ import com.bmscomp.kates.chaos.ChaosProvider; import com.bmscomp.kates.domain.CreateTestRequest; +import com.bmscomp.kates.domain.PruneResponse; import com.bmscomp.kates.domain.TestResult; import com.bmscomp.kates.domain.TestRun; import com.bmscomp.kates.domain.TestType; @@ -51,6 +60,15 @@ public class TestResource { private static final String NOT_STARTED = "The Kates API failed to start the test run — see server logs for details"; + /** + * The most runs one DELETE /api/tests deletes, and what it deletes when it + * is not told. Each is a delete of its own, so the bound keeps one call + * short; a caller calls again while runs remain. + */ + private static final int PRUNE_LIMIT = 1000; + + private static final String AN_INSTANT = "an ISO-8601 instant such as 2026-09-07T00:00:00Z"; + private static final Logger LOG = Logger.getLogger(TestResource.class); private final TestOrchestrator orchestrator; @@ -232,6 +250,133 @@ public Response bulkDelete(com.bmscomp.kates.domain.BulkDeleteRequest request) { .build(); } + @DELETE + @Operation( + summary = "Prune finished test runs", + description = "Deletes the finished runs, DONE or FAILED, created before createdBefore: the oldest" + + " first, at most limit of them, each as DELETE /api/tests/{id} deletes it and with an audit" + + " row. A cancelled run is stored as FAILED, so it is pruned with them; a run that has not" + + " finished is never pruned. matched counts the runs that matched before the call, deleted" + + " those the call deleted, and remaining those that still match: a caller calls again until" + + " remaining is 0. A dry run counts and deletes nothing. The chart's cleanup CronJob and" + + " kates test prune call it.") + @APIResponse( + responseCode = "200", + description = "What matched, what the call deleted, and what still matches", + content = @Content(schema = @Schema(implementation = PruneResponse.class))) + @APIResponse( + responseCode = "400", + description = "createdBefore missing or not an ISO-8601 instant, a status other than DONE or FAILED," + + " a limit outside 1 to 1000, or a dryRun other than true or false; nothing is deleted") + public Response pruneTests( + @Parameter( + description = "Prunes the runs created before this ISO-8601 instant", + required = true, + example = "2026-09-07T00:00:00Z", + schema = @Schema(type = SchemaType.STRING, format = "date-time")) + @QueryParam("createdBefore") + String createdBefore, + @Parameter(description = "DONE or FAILED, case-insensitive and repeatable; both when absent") + @QueryParam("status") + List status, + @Parameter( + description = "The most runs this call deletes, from 1 to 1000", + schema = + @Schema( + type = SchemaType.INTEGER, + format = "int32", + minimum = "1", + maximum = "1000", + defaultValue = "1000")) + @QueryParam("limit") + String limit, + @Parameter( + description = "Count the runs that match and delete none", + schema = @Schema(type = SchemaType.BOOLEAN, defaultValue = "false")) + @QueryParam("dryRun") + String dryRun) { + if (createdBefore == null || createdBefore.isBlank()) { + return badRequest("createdBefore is required: " + AN_INSTANT); + } + Instant before; + try { + before = Instant.parse(createdBefore.trim()); + } catch (DateTimeParseException e) { + return badRequest("createdBefore must be " + AN_INSTANT); + } + + // An EnumSet answers the statuses in the enum's order, DONE then + // FAILED, whatever order the request named them in. + Set statuses = EnumSet.noneOf(TestResult.TaskStatus.class); + for (String value : status != null ? status : List.of()) { + // Locale.ROOT: in a Turkish locale "failed" upper-cases to FAİLED. + String name = value.trim().toUpperCase(Locale.ROOT); + if (!name.equals("DONE") && !name.equals("FAILED")) { + return badRequest("status must be DONE or FAILED: only finished runs can be pruned"); + } + statuses.add(TestResult.TaskStatus.valueOf(name)); + } + if (statuses.isEmpty()) { + statuses = EnumSet.of(TestResult.TaskStatus.DONE, TestResult.TaskStatus.FAILED); + } + + // limit and dryRun are read here, not bound as int and boolean: JAX-RS + // answers a query parameter it cannot convert with 404, and binds a + // boolean with Boolean.valueOf, which reads anything but "true" as + // false, so dryRun=yes would delete what the caller meant only to + // count. An empty value never gets here: RESTEasy Reactive passes + // limit= or dryRun= on as absent. + String outOfRange = "limit must be from 1 to " + PRUNE_LIMIT; + int max = PRUNE_LIMIT; + if (limit != null) { + try { + max = Integer.parseInt(limit.trim()); + } catch (NumberFormatException e) { + return badRequest(outOfRange); + } + if (max < 1 || max > PRUNE_LIMIT) { + return badRequest(outOfRange); + } + } + boolean countOnly = false; + if (dryRun != null) { + if (!dryRun.trim().equalsIgnoreCase("true") && !dryRun.trim().equalsIgnoreCase("false")) { + return badRequest("dryRun must be true or false"); + } + countOnly = Boolean.parseBoolean(dryRun.trim()); + } + + List names = statuses.stream().map(Enum::name).toList(); + long matched = repository.countByStatusCreatedBefore(statuses, before); + if (countOnly) { + return Response.ok(new PruneResponse(before.toString(), names, true, matched, 0, matched)) + .build(); + } + int deleted = 0; + for (String id : repository.findIdsByStatusCreatedBefore(statuses, before, max)) { + // The single delete, as the bulk one uses. A run another delete + // took first is gone, and is not counted. + if (orchestrator.deleteTest(id)) { + auditService.record("DELETE", "test", id, "retention: created before " + before); + deleted++; + } + } + long remaining = repository.countByStatusCreatedBefore(statuses, before); + if (deleted > 0) { + LOG.infof( + "Pruned %d %s run(s) created before %s; %d still match", + deleted, String.join(" or ", names), before, remaining); + } + return Response.ok(new PruneResponse(before.toString(), names, false, matched, deleted, remaining)) + .build(); + } + + private static Response badRequest(String message) { + return Response.status(400) + .entity(ApiError.of(400, "Bad Request", message)) + .build(); + } + @GET @Operation( summary = "List test runs", diff --git a/kates/src/main/java/com/bmscomp/kates/config/NativeReflectionConfig.java b/kates/src/main/java/com/bmscomp/kates/config/NativeReflectionConfig.java index 1a029eb7..c99cde92 100644 --- a/kates/src/main/java/com/bmscomp/kates/config/NativeReflectionConfig.java +++ b/kates/src/main/java/com/bmscomp/kates/config/NativeReflectionConfig.java @@ -30,6 +30,7 @@ com.bmscomp.kates.domain.BulkCreateResponse.class, com.bmscomp.kates.domain.BulkDeleteRequest.class, com.bmscomp.kates.domain.BulkDeleteResponse.class, + com.bmscomp.kates.domain.PruneResponse.class, com.bmscomp.kates.api.WhoAmIResource.WhoAmI.class }) public class NativeReflectionConfig {} diff --git a/kates/src/main/java/com/bmscomp/kates/domain/PruneResponse.java b/kates/src/main/java/com/bmscomp/kates/domain/PruneResponse.java new file mode 100644 index 00000000..8d60dd7f --- /dev/null +++ b/kates/src/main/java/com/bmscomp/kates/domain/PruneResponse.java @@ -0,0 +1,11 @@ +package com.bmscomp.kates.domain; + +import java.util.List; + +/** + * The answer to DELETE /api/tests. matched is what matched before the call, + * remaining what still matches after it, so a caller that deletes a limit's + * worth per call stops when remaining is 0. + */ +public record PruneResponse( + String createdBefore, List statuses, boolean dryRun, long matched, int deleted, long remaining) {} diff --git a/kates/src/main/java/com/bmscomp/kates/service/TestRunRepository.java b/kates/src/main/java/com/bmscomp/kates/service/TestRunRepository.java index a5ad8da8..89ee077b 100644 --- a/kates/src/main/java/com/bmscomp/kates/service/TestRunRepository.java +++ b/kates/src/main/java/com/bmscomp/kates/service/TestRunRepository.java @@ -283,6 +283,43 @@ public long countByStatus(com.bmscomp.kates.domain.TestResult.TaskStatus status) .getSingleResult(); } + /** + * How many runs in one of {@code statuses} were created before + * {@code before}: what DELETE /api/tests would prune, and what it left. + */ + public long countByStatusCreatedBefore( + java.util.Set statuses, java.time.Instant before) { + return em.createQuery( + "SELECT COUNT(r) FROM TestRunEntity r WHERE r.status IN :statuses AND r.createdAt < :before", + Long.class) + .setParameter("statuses", statuses) + .setParameter("before", before) + .getSingleResult(); + } + + /** + * The ids of the runs in one of {@code statuses} created before + * {@code before}, oldest first, at most {@code limit} of them. + * + *

Ids, not runs: DELETE /api/tests deletes each through the + * orchestrator, which reads the run under its row lock itself. The id + * orders runs created in the same instant, so a caller that prunes a + * limit's worth per call goes through them in one order. + */ + public List findIdsByStatusCreatedBefore( + java.util.Set statuses, + java.time.Instant before, + int limit) { + return em.createQuery( + "SELECT r.id FROM TestRunEntity r WHERE r.status IN :statuses AND r.createdAt < :before" + + " ORDER BY r.createdAt ASC, r.id ASC", + String.class) + .setParameter("statuses", statuses) + .setParameter("before", before) + .setMaxResults(limit) + .getResultList(); + } + /** * The runs in this status, read without their task results (see * {@link EntityMapper#toDomainSummary}). A caller that changes a run and diff --git a/kates/src/test/java/com/bmscomp/kates/api/TestResourceDeleteTest.java b/kates/src/test/java/com/bmscomp/kates/api/TestResourceDeleteTest.java index 4521c2d6..b51728ea 100644 --- a/kates/src/test/java/com/bmscomp/kates/api/TestResourceDeleteTest.java +++ b/kates/src/test/java/com/bmscomp/kates/api/TestResourceDeleteTest.java @@ -5,9 +5,14 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; +import static org.mockito.Mockito.when; +import java.time.Instant; import java.util.ArrayList; +import java.util.EnumSet; import java.util.List; +import java.util.Set; import jakarta.ws.rs.core.Response; import org.junit.jupiter.api.BeforeEach; @@ -15,6 +20,7 @@ import com.bmscomp.kates.domain.BulkDeleteRequest; import com.bmscomp.kates.domain.BulkDeleteResponse; +import com.bmscomp.kates.domain.PruneResponse; import com.bmscomp.kates.domain.TestResult.TaskStatus; import com.bmscomp.kates.engine.InMemoryEngine; import com.bmscomp.kates.service.AuditService; @@ -24,7 +30,8 @@ * DELETE /api/tests/bulk. The single delete stopped a run's tasks but kept * its concurrency permit, and the bulk delete removed the row and nothing * else, so a few deletes of running runs made every new run answer 429 until - * a restart. Neither said the run had ended. + * a restart. Neither said the run had ended. DELETE /api/tests prunes + * through the same delete. */ class TestResourceDeleteTest { @@ -78,4 +85,28 @@ void aBulkDeleteStopsEveryRunAndGivesBackTheirSlots() { verify(engine.katesMetrics, times(ids.size())).recordTestCompleted("LOAD", "failed"); engine.assertEverySlotFree(); } + + /** + * A run another delete took between the prune reading the ids and + * deleting them is not counted, and gets no audit row of the prune's. + */ + @Test + void aPruneCountsOnlyTheRunsItDeleted() { + String done = engine.storeEnded(TaskStatus.DONE); + Instant cutoff = Instant.parse("2026-09-07T00:00:00Z"); + Set finished = EnumSet.of(TaskStatus.DONE, TaskStatus.FAILED); + when(engine.repository.countByStatusCreatedBefore(finished, cutoff)).thenReturn(2L, 0L); + when(engine.repository.findIdsByStatusCreatedBefore(finished, cutoff, 1000)) + .thenReturn(List.of("taken-by-another-delete", done)); + + Response response = resource.pruneTests(cutoff.toString(), List.of(), null, null); + + assertEquals(200, response.getStatus()); + assertEquals( + new PruneResponse("2026-09-07T00:00:00Z", List.of("DONE", "FAILED"), false, 2, 1, 0), + response.getEntity()); + assertFalse(engine.rows.containsKey(done)); + verify(audit).record("DELETE", "test", done, "retention: created before 2026-09-07T00:00:00Z"); + verifyNoMoreInteractions(audit); + } } diff --git a/kates/src/test/java/com/bmscomp/kates/api/TestResourcePruneTest.java b/kates/src/test/java/com/bmscomp/kates/api/TestResourcePruneTest.java new file mode 100644 index 00000000..f2d8c6de --- /dev/null +++ b/kates/src/test/java/com/bmscomp/kates/api/TestResourcePruneTest.java @@ -0,0 +1,349 @@ +package com.bmscomp.kates.api; + +import static io.restassured.RestAssured.given; +import static java.util.stream.Collectors.toMap; +import static org.hamcrest.Matchers.is; +import static org.junit.jupiter.api.Assertions.*; + +import java.net.URI; +import java.net.http.HttpClient; +import java.net.http.HttpRequest; +import java.net.http.HttpResponse; +import java.time.Instant; +import java.time.temporal.ChronoUnit; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import jakarta.inject.Inject; + +import io.quarkus.narayana.jta.QuarkusTransaction; +import io.quarkus.test.common.http.TestHTTPResource; +import io.quarkus.test.junit.QuarkusTest; +import io.restassured.path.json.JsonPath; +import io.restassured.specification.RequestSpecification; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +import com.bmscomp.kates.domain.TestResult.TaskStatus; +import com.bmscomp.kates.domain.TestRun; +import com.bmscomp.kates.domain.TestType; +import com.bmscomp.kates.service.AuditService; +import com.bmscomp.kates.service.TestRunRepository; + +/** + * DELETE /api/tests, which deletes the finished runs created before an + * instant, for the chart's cleanup CronJob and kates test prune. The CronJob + * listed the runs itself, asked for a status no run has, and picked ids out + * of the list with a pattern its answer never matched: it never deleted one. + * + *

The runs here are created in 2001, older than any run another test + * stores, so each count here is of this test's runs alone, and each test + * deletes its runs after it. The test profile keeps TestCleanupScheduler off + * runs that old. The runs that have not finished are PENDING and STOPPING: + * the timeout reaper fails a RUNNING run past its deadline, as one from 2001 + * is, at whatever moment it runs, and a FAILED run is one to prune. + */ +@QuarkusTest +class TestResourcePruneTest { + + private static final Instant ERA = Instant.parse("2001-01-01T00:00:00Z"); + private static final Instant CUTOFF = day(10); + private static final String AN_INSTANT = "an ISO-8601 instant such as 2026-09-07T00:00:00Z"; + + @Inject + TestRunRepository repository; + + @Inject + AuditService auditService; + + @TestHTTPResource("/api/tests") + URI tests; + + private final List stored = new ArrayList<>(); + private Instant started; + + @BeforeEach + void start() { + started = Instant.now(); + } + + @AfterEach + void deleteTheRuns() { + stored.forEach(repository::delete); + stored.clear(); + } + + @Test + void createdBeforeIsRequired() { + String old = store(TaskStatus.DONE, day(1)); + + refused(given(), "createdBefore is required: " + AN_INSTANT); + refused(given().queryParam("createdBefore", " "), "createdBefore is required: " + AN_INSTANT); + + assertStored(old); + } + + @ParameterizedTest + @ValueSource(strings = {"2001-01-11", "2001-01-11T00:00:00", "30d", "yesterday"}) + void createdBeforeMustBeAnInstant(String value) { + String old = store(TaskStatus.DONE, day(1)); + + refused(given().queryParam("createdBefore", value), "createdBefore must be " + AN_INSTANT); + + assertStored(old); + } + + /** + * Only a finished run can be pruned, so a status naming any other refuses + * the whole call, even beside DONE, rather than being dropped from it. + */ + @ParameterizedTest + @ValueSource(strings = {"RUNNING", "PENDING", "STOPPING", "CANCELLED", "DONE,FAILED"}) + void aStatusOtherThanDoneOrFailedIsRefused(String value) { + String old = store(TaskStatus.DONE, day(1)); + + refused( + given().queryParam("createdBefore", CUTOFF.toString()).queryParam("status", "DONE", value), + "status must be DONE or FAILED: only finished runs can be pruned"); + + assertStored(old); + } + + @ParameterizedTest + @ValueSource(strings = {"0", "-1", "1001", "ten"}) + void aLimitOutsideOneToAThousandIsRefused(String value) { + String old = store(TaskStatus.DONE, day(1)); + + refused( + given().queryParam("createdBefore", CUTOFF.toString()).queryParam("limit", value), + "limit must be from 1 to 1000"); + + assertStored(old); + } + + /** + * Bound as a boolean, dryRun=yes would be false, and the call would delete + * what the caller meant only to count. + */ + @ParameterizedTest + @ValueSource(strings = {"yes", "1", "on"}) + void aDryRunOtherThanTrueOrFalseIsRefused(String value) { + String old = store(TaskStatus.DONE, day(1)); + + refused( + given().queryParam("createdBefore", CUTOFF.toString()).queryParam("dryRun", value), + "dryRun must be true or false"); + + assertStored(old); + } + + /** + * A dry run counts and deletes nothing. The whole answer is checked, as + * the CronJob reads it: compact, so its sed finds "deleted" and + * "remaining" without a JSON parser. + */ + @Test + void aDryRunCountsAndDeletesNothing() { + List old = List.of( + store(TaskStatus.DONE, day(1)), store(TaskStatus.FAILED, day(2)), store(TaskStatus.DONE, day(3))); + + String answer = given().queryParam("createdBefore", CUTOFF.toString()) + .queryParam("dryRun", "true") + .when() + .delete("/api/tests") + .then() + .statusCode(200) + .extract() + .asString(); + + assertEquals( + "{\"createdBefore\":\"2001-01-11T00:00:00Z\",\"statuses\":[\"DONE\",\"FAILED\"],\"dryRun\":true," + + "\"matched\":3,\"deleted\":0,\"remaining\":3}", + answer); + old.forEach(this::assertStored); + assertEquals(Map.of(), auditRows()); + } + + /** + * Only DONE and FAILED runs created before the cutoff go, oldest first + * and at most limit per call, and remaining says what the next call has + * left. Runs that have not finished survive, however old, as do runs + * created at the cutoff or after it. The runs are stored out of order, so + * the order is the query's, not the store's. + */ + @Test + void deletesTheOldestFinishedRunsFirstUpToTheLimit() { + String third = store(TaskStatus.DONE, day(3)); + String oldest = store(TaskStatus.DONE, day(1)); + String cancelled = store(TaskStatus.FAILED, day(2)); + List survivors = List.of( + store(TaskStatus.PENDING, day(0)), + store(TaskStatus.STOPPING, day(0)), + store(TaskStatus.DONE, CUTOFF), + store(TaskStatus.FAILED, day(11))); + + JsonPath first = prune(given().queryParam("limit", "2")); + + assertEquals(List.of("DONE", "FAILED"), first.getList("statuses")); + assertFalse(first.getBoolean("dryRun")); + assertEquals(3, first.getLong("matched")); + assertEquals(2, first.getInt("deleted")); + assertEquals(1, first.getLong("remaining")); + assertGone(oldest); + assertGone(cancelled); + assertStored(third); + + JsonPath second = prune(given().queryParam("limit", "2")); + + assertEquals(1, second.getLong("matched")); + assertEquals(1, second.getInt("deleted")); + assertEquals(0, second.getLong("remaining")); + assertGone(third); + + JsonPath last = prune(given()); + + assertEquals(0, last.getLong("matched")); + assertEquals(0, last.getInt("deleted")); + assertEquals(0, last.getLong("remaining")); + survivors.forEach(this::assertStored); + + String row = "DELETE: retention: created before 2001-01-11T00:00:00Z"; + assertEquals(Map.of(oldest, row, cancelled, row, third, row), auditRows()); + } + + /** status is case-insensitive, and a run of a status not asked for stays. */ + @Test + void aStatusFilterPrunesOnlyThatStatus() { + String done = store(TaskStatus.DONE, day(1)); + String failed = store(TaskStatus.FAILED, day(2)); + + JsonPath answer = prune(given().queryParam("status", "failed")); + + assertEquals(List.of("FAILED"), answer.getList("statuses")); + assertEquals(1, answer.getLong("matched")); + assertEquals(1, answer.getInt("deleted")); + assertEquals(0, answer.getLong("remaining")); + assertGone(failed); + assertStored(done); + assertEquals(Map.of(failed, "DELETE: retention: created before 2001-01-11T00:00:00Z"), auditRows()); + } + + @Test + void statusesAreAnsweredDoneThenFailedAndOnce() { + JsonPath answer = + prune(given().queryParam("status", "FAILED", "done", "DONE").queryParam("dryRun", "true")); + + assertEquals(List.of("DONE", "FAILED"), answer.getList("statuses")); + } + + /** + * The cutoff is the instant given, whatever its offset, and the answer + * gives it in UTC. 02:00 at +02:00 is midnight UTC: the run created at + * 01:00 UTC is after it. + */ + @Test + void theCutoffIsTheInstantGivenWhateverItsOffset() { + store(TaskStatus.DONE, Instant.parse("2001-01-10T23:00:00Z")); + store(TaskStatus.DONE, Instant.parse("2001-01-11T01:00:00Z")); + + JsonPath answer = given().queryParam("createdBefore", "2001-01-11T02:00:00+02:00") + .queryParam("dryRun", "true") + .when() + .delete("/api/tests") + .then() + .statusCode(200) + .extract() + .jsonPath(); + + assertEquals("2001-01-11T00:00:00Z", answer.getString("createdBefore")); + assertEquals(1, answer.getLong("matched")); + } + + /** What the CronJob's curl sends: a DELETE with no body and no Content-Type. */ + @Test + void aDeleteWithNoContentTypeIsAnswered() throws Exception { + String done = store(TaskStatus.DONE, day(1)); + + HttpResponse answer; + try (HttpClient client = HttpClient.newHttpClient()) { + answer = client.send( + HttpRequest.newBuilder(URI.create(tests + "?createdBefore=" + CUTOFF + "&limit=1000")) + .DELETE() + .build(), + HttpResponse.BodyHandlers.ofString()); + } + + assertEquals(200, answer.statusCode(), answer.body()); + assertTrue(answer.body().contains("\"deleted\":1,\"remaining\":0"), answer.body()); + assertGone(done); + } + + private static Instant day(int n) { + return ERA.plus(n, ChronoUnit.DAYS); + } + + /** Stores a LOAD run with this status, created at {@code createdAt}, for deletion after the test. */ + private String store(TaskStatus status, Instant createdAt) { + TestRun run = new TestRun(TestType.LOAD, null) + .withBackend("native") + .withStatus(status) + .withCreatedAt(createdAt.toString()); + repository.save(run); + stored.add(run.getId()); + return run.getId(); + } + + /** Prunes what was created before {@link #CUTOFF}, with whatever else {@code request} asks. */ + private static JsonPath prune(RequestSpecification request) { + return request.queryParam("createdBefore", CUTOFF.toString()) + .when() + .delete("/api/tests") + .then() + .statusCode(200) + .body("createdBefore", is(CUTOFF.toString())) + .extract() + .jsonPath(); + } + + private static void refused(RequestSpecification request, String message) { + request.when() + .delete("/api/tests") + .then() + .statusCode(400) + .body("error", is("Bad Request")) + .body("message", is(message)); + } + + private void assertStored(String id) { + assertTrue(isStored(id), "run " + id + " is still stored"); + } + + private void assertGone(String id) { + assertFalse(isStored(id), "run " + id + " is deleted"); + } + + /** + * Read in a transaction of its own. Outside one, a test's reads share one + * persistence context for the whole test, which keeps a run it read even + * after a call to the API deleted it. + */ + private boolean isStored(String id) { + return QuarkusTransaction.requiringNew() + .call(() -> repository.findById(id).isPresent()); + } + + /** + * The audit rows this test wrote of its runs, as each run's id to the + * row's action and details. A run with two rows fails here, on the + * duplicate key. + */ + private Map auditRows() { + return auditService.list(500, "test", started.toString()).stream() + .filter(row -> stored.contains((String) row.get("target"))) + .collect( + toMap(row -> (String) row.get("target"), row -> row.get("action") + ": " + row.get("details"))); + } +} diff --git a/kates/src/test/resources/application.properties b/kates/src/test/resources/application.properties index ab4e1ecd..b18b4607 100644 --- a/kates/src/test/resources/application.properties +++ b/kates/src/test/resources/application.properties @@ -8,6 +8,11 @@ kates.kafka.bootstrap-servers=localhost:9092 kates.kafka.security.protocol=PLAINTEXT kates.trogdor.coordinator-url=http://localhost:8889 kates.engine.default-backend=native +# TestCleanupScheduler deletes every run older than this but a RUNNING or +# PENDING one, at startup and then daily. TestResourcePruneTest stores runs +# from 2001, which the 90-day default let it delete under the test at +# whatever moment it ran. +kates.cleanup.retention-days=36500 # Global defaults kates.defaults.replication-factor=3 From 01a26b058e586bb5ff742137b6041dccd802ea68 Mon Sep 17 00:00:00 2001 From: Said Boudjelda Date: Thu, 8 Oct 2026 01:36:01 +0200 Subject: [PATCH 2/6] fix(chart): the README describes the API's own sweep without the statuses it is about to change The sentence named the runs the sweep leaves alone, PENDING and RUNNING, which the follow-up that makes the sweep delete only DONE and FAILED runs changes. Saying it deletes old finished runs stays true either way, so that PR needn't touch charts/ and bump the chart again. --- charts/kates/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/charts/kates/README.md b/charts/kates/README.md index 7133543f..971d191e 100644 --- a/charts/kates/README.md +++ b/charts/kates/README.md @@ -207,7 +207,7 @@ that — `image.tag` does. ### Test run cleanup -Each job asks the Kates API to delete the finished runs created more than `cleanup.retentionDays` ago whose status is in `cleanup.statuses`, through `DELETE /api/tests?createdBefore=…&status=DONE&status=FAILED&limit=1000`. The API deletes them oldest first, at most 1000 a call, writes an audit row for each, and says how many still match; the job calls again until none do, at most 100 times, and the next job goes on from there. A run still `PENDING`, `RUNNING` or `STOPPING` is never deleted, however old it is. `kates test prune` does the same from the command line. The Kates API's own daily sweep already deletes the runs created more than 90 days ago (`kates.cleanup.retention-days`) that are not `PENDING` or `RUNNING`, so a `retentionDays` of 90 or more adds nothing to it. +Each job asks the Kates API to delete the finished runs created more than `cleanup.retentionDays` ago whose status is in `cleanup.statuses`, through `DELETE /api/tests?createdBefore=…&status=DONE&status=FAILED&limit=1000`. The API deletes them oldest first, at most 1000 a call, writes an audit row for each, and says how many still match; the job calls again until none do, at most 100 times, and the next job goes on from there. A run still `PENDING`, `RUNNING` or `STOPPING` is never deleted, however old it is. `kates test prune` does the same from the command line. The Kates API's own daily sweep already deletes old finished runs at 90 days (`kates.cleanup.retention-days`), so a `retentionDays` of 90 or more adds nothing to it. The job sends the key from the `apiKey` Secret (`apiKey.existingSecret`, or `-api-key`, at `apiKey.secretKey`) as `X-API-Key`, as the chart's tests do. On anything but a 200 it prints the API's answer and fails, and Kubernetes retries it twice: a 401 names the Secret it read the key from, and a 405 means a Kates API without `DELETE /api/tests`. That endpoint takes an image newer than `1.25.0`. From 82ed041e42e2ed1cfa8ec97c7f84521c854acb9c Mon Sep 17 00:00:00 2001 From: Said Boudjelda Date: Thu, 8 Oct 2026 01:36:15 +0200 Subject: [PATCH 3/6] fix(engine): the API's daily run sweep deletes only finished runs, in batches, each through the run delete with an audit row TestCleanupScheduler, the Kates API's own retention sweep (kates.cleanup.retention-days, 90 days): - read every run with findAll(), results included, each day; - deleted any run that was not RUNNING or PENDING, so STOPPING runs too, whose tasks may still produce; - removed rows with repository.delete, so it neither settled a run's tasks nor wrote an audit row; - swallowed a createdAt it could not parse; - held one transaction around it all. The delete loop of DELETE /api/tests moves into RunRetention: - count, then at most limit ids oldest first; - each through orchestrator.deleteTest with an audit row; - count again. The endpoint answers byte for byte as before. The sweep uses that loop: - DONE and FAILED runs older than the retention only, in passes of 1000, until none match, a pass deletes nothing, or 1000 passes; - audit details "retention sweep: created before (kates.cleanup.retention-days=)"; - deletes count as they happen, so a pass that throws still reports what it deleted. The error is logged and ends that sweep. PENDING, RUNNING and STOPPING runs stay, whatever their age. --- docs/book/10-cli-reference.md | 2 +- docs/book/11-api-reference.md | 4 +- docs/book/12-deployment.md | 8 +- kates/docs/api-reference.md | 4 +- .../com/bmscomp/kates/api/TestResource.java | 26 +- .../bmscomp/kates/service/RunRetention.java | 79 ++++ .../kates/service/TestCleanupScheduler.java | 132 +++++-- .../kates/service/TestRunRepository.java | 4 +- .../kates/api/TestResourceDeleteTest.java | 2 + .../service/TestCleanupSchedulerTest.java | 350 ++++++++++++++++++ .../src/test/resources/application.properties | 8 +- 11 files changed, 570 insertions(+), 49 deletions(-) create mode 100644 kates/src/main/java/com/bmscomp/kates/service/RunRetention.java create mode 100644 kates/src/test/java/com/bmscomp/kates/service/TestCleanupSchedulerTest.java diff --git a/docs/book/10-cli-reference.md b/docs/book/10-cli-reference.md index 37546146..96d1835e 100644 --- a/docs/book/10-cli-reference.md +++ b/docs/book/10-cli-reference.md @@ -718,7 +718,7 @@ The command first counts the runs, deleting none, and prints how many it found, The command stops after 100 calls, or after a call that deleted nothing. When runs still match then, it exits 1 and says how many are left; run it again to go on. With `-o json` it prints one answer for the whole command, in the fields of [DELETE /api/tests](11-api-reference.md#delete-apitests). `deleted` adds up every call, `matched` is the count the first delete found and `remaining` the one after the last; with `--dry-run`, or when none match, it prints the count. -Once a day the Kates API deletes, on its own, every run older than 90 days that is not `RUNNING` or `PENDING`, so `prune` is for keeping finished runs for less long. To prune on a schedule, enable the `kates` chart's cleanup CronJob, described in [Where Kates Stores Test Data](12-deployment.md#where-kates-stores-test-data). A Kates API too old to have `DELETE /api/tests` answers `405`, and the command fails, saying to upgrade it. +Once a day the Kates API prunes by itself, the same way: it deletes the `DONE` and `FAILED` runs created more than 90 days ago (`kates.cleanup.retention-days`), each with an audit row. It leaves `PENDING`, `RUNNING` and `STOPPING` runs alone, however old they are. So `prune` is for keeping finished runs for less long. To prune on a schedule, enable the `kates` chart's cleanup CronJob, described in [Where Kates Stores Test Data](12-deployment.md#where-kates-stores-test-data). A Kates API too old to have `DELETE /api/tests` answers `405`, and the command fails, saying to upgrade it. #### test watch diff --git a/docs/book/11-api-reference.md b/docs/book/11-api-reference.md index caf957d5..bb922489 100644 --- a/docs/book/11-api-reference.md +++ b/docs/book/11-api-reference.md @@ -334,7 +334,9 @@ Stop and delete a test run with its results. A run that is still `PENDING` or `R Delete the finished runs created before a cutoff, to keep the run history to a retention period of your own. Only `DONE` and `FAILED` runs match, and a cancelled run is stored as `FAILED`, so it matches too. A run still `PENDING`, `RUNNING` or `STOPPING` is never deleted here; [test cleanup](10-cli-reference.md#test-cleanup) deals with one left `RUNNING`. `kates test prune` and the `kates` chart's cleanup CronJob call this endpoint. -A call deletes the oldest matching runs first, by `createdAt`, and at most `limit` of them. Each goes as `DELETE /api/tests/{id}` deletes it, with its results, and leaves a row in the audit log: action `DELETE`, type `test`, the run's ID, and the details `retention: created before `. `kates audit --type test` lists them. Once a day the Kates API also deletes, on its own, every run older than 90 days (`kates.cleanup.retention-days`) that is not `RUNNING` or `PENDING`, without an audit row; this endpoint is for keeping finished runs for less long. +A call deletes the oldest matching runs first, by `createdAt`, and at most `limit` of them. Each goes as `DELETE /api/tests/{id}` deletes it, with its results, and leaves a row in the audit log: action `DELETE`, type `test`, the run's ID, and the details `retention: created before `. `kates audit --type test` lists them. + +Once a day the Kates API also prunes by itself, through the same delete: it deletes the `DONE` and `FAILED` runs created more than `kates.cleanup.retention-days` days ago, 90 by default, oldest first and 1,000 at a time. Each run it deletes leaves an audit row too, with the details `retention sweep: created before (kates.cleanup.retention-days=)`. A run still `PENDING`, `RUNNING` or `STOPPING` is left alone, however old it is. This endpoint is for keeping finished runs for less long. **Query Parameters:** diff --git a/docs/book/12-deployment.md b/docs/book/12-deployment.md index 3249b396..0ce401dd 100644 --- a/docs/book/12-deployment.md +++ b/docs/book/12-deployment.md @@ -75,7 +75,11 @@ The default deployment uses **NodePort** for all services (Grafana on 30080, Kaf ### Where Kates Stores Test Data -The Kates API keeps every run in PostgreSQL. The `kates` chart runs its own PostgreSQL on a 1 GiB PersistentVolumeClaim on the cluster's default StorageClass, so runs survive pod restarts; there is no emptyDir mode. For production, point the chart at an external database instead, with `postgresql.enabled: false` and `externalDatabase`, as `values-prod.yaml` does. Either way, the Kates API deletes finished runs older than 90 days by default, and [Architecture & Design](02-architecture.md) describes what else it keeps. +The Kates API keeps every run in PostgreSQL. The `kates` chart runs its own PostgreSQL on a 1 GiB PersistentVolumeClaim on the cluster's default StorageClass, so runs survive pod restarts; there is no emptyDir mode. For production, point the chart at an external database instead, with `postgresql.enabled: false` and `externalDatabase`, as `values-prod.yaml` does. [Architecture & Design](02-architecture.md) describes what else the Kates API keeps. + +Whichever database holds them, each replica of the Kates API sweeps out old runs when it starts and every 24 hours after. A sweep deletes the `DONE` and `FAILED` runs created more than `kates.cleanup.retention-days` days ago, 90 by default, so a lower setting deletes as soon as a replica starts with it. It deletes each run as [DELETE /api/tests/{id}](11-api-reference.md#delete-apitestsid) does, with its results, oldest first and 1,000 at a time. Each leaves a row in the audit log: action `DELETE`, type `test`, the run's ID, and the details `retention sweep: created before (kates.cleanup.retention-days=)`. When two replicas find the same run at once, it is deleted, and audited, once. An error ends that sweep with an `ERROR` line in the Kates API's log, and the next sweep goes on from there. + +A sweep leaves a run still `RUNNING` alone, however old it is, and the first sweep after the run ends deletes it. The Kates API fails a run still `RUNNING` five minutes after the time it was set to last. A run left `PENDING` by a submission that died, or `STOPPING` by an older version of Kates, never ends, so no sweep deletes it. Delete such a run with [DELETE /api/tests/{id}](11-api-reference.md#delete-apitestsid), or `kates test delete`. To keep runs for less time, set `cleanup.enabled: true` in the `kates` chart's values. Its CronJob then deletes the finished runs older than `cleanup.retentionDays`, through [DELETE /api/tests](11-api-reference.md#delete-apitests), with the key from the Secret the `apiKey` values name. The table gives the values and their defaults: @@ -87,7 +91,7 @@ To keep runs for less time, set `cleanup.enabled: true` in the `kates` chart's v | `cleanup.statuses` | `[DONE, FAILED]` | Which finished runs go; a cancelled run is stored as `FAILED` | | `cleanup.image` | `curlimages/curl:8.7.1` | The image the job runs `curl` in | -Each time it runs, the job deletes the oldest first, up to 1,000 runs a call, until none older are left or it has made 100 calls, and the next job goes on from there. Its log ends with how many it deleted. A call the Kates API refuses fails the job, with the answer in its log. A `401` or `403` there means the key is missing or wrong, and a `405` that the Kates API is too old to have `DELETE /api/tests`. The Kates API's own daily sweep deletes every run that is not `RUNNING` or `PENDING` at 90 days by default, so a `retentionDays` of 90 or more adds nothing to it. To prune once, by hand, run `kates test prune`. +Each time it runs, the job deletes the oldest first, up to 1,000 runs a call, until none older are left or it has made 100 calls, and the next job goes on from there. Its log ends with how many it deleted. A call the Kates API refuses fails the job, with the answer in its log. A `401` or `403` there means the key is missing or wrong, and a `405` that the Kates API is too old to have `DELETE /api/tests`. The Kates API's own daily sweep already deletes the `DONE` and `FAILED` runs older than 90 days by default, so a `retentionDays` of 90 or more adds nothing to it. To prune once, by hand, run `kates test prune`. For Kafka broker storage, **always use persistent volumes** — even in development. Kafka's log retention depends on data being durable, and losing broker data mid-test invalidates results. diff --git a/kates/docs/api-reference.md b/kates/docs/api-reference.md index 8ce8d00b..2af873e8 100644 --- a/kates/docs/api-reference.md +++ b/kates/docs/api-reference.md @@ -225,7 +225,9 @@ DELETE /api/tests?createdBefore=2026-09-07T00:00:00Z&status=FAILED&dryRun=true Deletes the finished runs created before a cutoff, to keep the run history to a retention period of your own. Only `DONE` and `FAILED` runs match; a cancelled run is stored as `FAILED`, so it matches too. A run still pending, running or stopping is never deleted here: `kates test cleanup` deals with one left running. `kates test prune` and the chart's cleanup CronJob (`cleanup.*` in `charts/kates/values.yaml`) call this endpoint. -A call deletes the oldest matching runs first, by `createdAt`, and at most `limit` of them. Each goes through the same delete as `DELETE /api/tests/{id}`, and leaves an audit row: action `DELETE`, type `test`, the run's ID, and the details `retention: created before `. Once a day the Kates API also deletes, on its own, every run older than `kates.cleanup.retention-days` (90) that is not `RUNNING` or `PENDING`, without an audit row; this endpoint is for keeping finished runs for less long. +A call deletes the oldest matching runs first, by `createdAt`, and at most `limit` of them. Each goes through the same delete as `DELETE /api/tests/{id}`, and leaves an audit row: action `DELETE`, type `test`, the run's ID, and the details `retention: created before `. + +Once a day the Kates API also prunes by itself, through the same delete: it deletes the `DONE` and `FAILED` runs created more than `kates.cleanup.retention-days` (90) days ago, oldest first and 1,000 at a time. Each leaves an audit row too, with the details `retention sweep: created before (kates.cleanup.retention-days=)`. A run still pending, running or stopping is left alone, however old it is. This endpoint is for keeping finished runs for less long. **Query Parameters:** diff --git a/kates/src/main/java/com/bmscomp/kates/api/TestResource.java b/kates/src/main/java/com/bmscomp/kates/api/TestResource.java index d3f5c1cd..e851655b 100644 --- a/kates/src/main/java/com/bmscomp/kates/api/TestResource.java +++ b/kates/src/main/java/com/bmscomp/kates/api/TestResource.java @@ -41,6 +41,7 @@ import com.bmscomp.kates.persistence.BaselineEntity; import com.bmscomp.kates.service.AuditService; import com.bmscomp.kates.service.BaselineService; +import com.bmscomp.kates.service.RunRetention; import com.bmscomp.kates.service.TestRunRepository; @Path("/api/tests") @@ -81,6 +82,9 @@ public class TestResource { @Inject AuditService auditService; + @Inject + RunRetention retention; + @Inject public TestResource( TestOrchestrator orchestrator, @@ -347,27 +351,21 @@ public Response pruneTests( } List names = statuses.stream().map(Enum::name).toList(); - long matched = repository.countByStatusCreatedBefore(statuses, before); if (countOnly) { + long matched = retention.count(statuses, before); return Response.ok(new PruneResponse(before.toString(), names, true, matched, 0, matched)) .build(); } - int deleted = 0; - for (String id : repository.findIdsByStatusCreatedBefore(statuses, before, max)) { - // The single delete, as the bulk one uses. A run another delete - // took first is gone, and is not counted. - if (orchestrator.deleteTest(id)) { - auditService.record("DELETE", "test", id, "retention: created before " + before); - deleted++; - } - } - long remaining = repository.countByStatusCreatedBefore(statuses, before); - if (deleted > 0) { + // The delete TestCleanupScheduler sweeps with, each run through the + // single delete, as the bulk one uses. + RunRetention.Pass pass = retention.prune(statuses, before, max, "retention: created before " + before); + if (pass.deleted() > 0) { LOG.infof( "Pruned %d %s run(s) created before %s; %d still match", - deleted, String.join(" or ", names), before, remaining); + pass.deleted(), String.join(" or ", names), before, pass.remaining()); } - return Response.ok(new PruneResponse(before.toString(), names, false, matched, deleted, remaining)) + return Response.ok(new PruneResponse( + before.toString(), names, false, pass.matched(), pass.deleted(), pass.remaining())) .build(); } diff --git a/kates/src/main/java/com/bmscomp/kates/service/RunRetention.java b/kates/src/main/java/com/bmscomp/kates/service/RunRetention.java new file mode 100644 index 00000000..7007f4f0 --- /dev/null +++ b/kates/src/main/java/com/bmscomp/kates/service/RunRetention.java @@ -0,0 +1,79 @@ +package com.bmscomp.kates.service; + +import java.time.Instant; +import java.util.Set; +import jakarta.enterprise.context.ApplicationScoped; +import jakarta.inject.Inject; + +import com.bmscomp.kates.domain.TestResult; +import com.bmscomp.kates.engine.TestOrchestrator; + +/** + * Deletes the runs in some statuses created before an instant, the oldest + * first and at most a limit of them at a time. DELETE /api/tests and + * {@link TestCleanupScheduler} both prune through it, so a run either one + * deletes goes the way DELETE /api/tests/{id} deletes it, and gets an audit + * row. + * + *

Not transactional, nor are its callers: each delete and each audit row + * commits on its own. A pass then holds no lock on the runs it has deleted + * while it deletes the next, and a delete that fails part-way keeps the ones + * before it, each with its audit row. + */ +@ApplicationScoped +public class RunRetention { + + /** + * One pass: the runs that matched before it, those it deleted, and those + * that still match after it. + */ + public record Pass(long matched, int deleted, long remaining) {} + + private final TestOrchestrator orchestrator; + private final TestRunRepository repository; + private final AuditService auditService; + + @Inject + public RunRetention(TestOrchestrator orchestrator, TestRunRepository repository, AuditService auditService) { + this.orchestrator = orchestrator; + this.repository = repository; + this.auditService = auditService; + } + + /** How many runs in one of {@code statuses} were created before {@code before}. */ + public long count(Set statuses, Instant before) { + return repository.countByStatusCreatedBefore(statuses, before); + } + + /** + * Deletes at most {@code limit} of the runs in one of {@code statuses} + * created before {@code before}, the oldest first, and records each one + * deleted with {@code auditDetails}. + */ + public Pass prune(Set statuses, Instant before, int limit, String auditDetails) { + return prune(statuses, before, limit, auditDetails, () -> {}); + } + + /** + * {@link #prune(Set, Instant, int, String)}, calling {@code onDeleted} + * once each run it deletes has its audit row. A pass that throws returns + * no {@link Pass}, but the runs it deleted before the throw stay deleted, + * so a caller that reports totals counts them here. + */ + public Pass prune( + Set statuses, Instant before, int limit, String auditDetails, Runnable onDeleted) { + long matched = count(statuses, before); + int deleted = 0; + for (String id : repository.findIdsByStatusCreatedBefore(statuses, before, limit)) { + // The single delete, which settles a run that is still going. A + // run another delete took first is gone: it is not counted, and + // the delete that took it wrote its row. + if (orchestrator.deleteTest(id)) { + auditService.record("DELETE", "test", id, auditDetails); + deleted++; + onDeleted.run(); + } + } + return new Pass(matched, deleted, count(statuses, before)); + } +} diff --git a/kates/src/main/java/com/bmscomp/kates/service/TestCleanupScheduler.java b/kates/src/main/java/com/bmscomp/kates/service/TestCleanupScheduler.java index e8b94299..4314383b 100644 --- a/kates/src/main/java/com/bmscomp/kates/service/TestCleanupScheduler.java +++ b/kates/src/main/java/com/bmscomp/kates/service/TestCleanupScheduler.java @@ -2,56 +2,140 @@ import java.time.Duration; import java.time.Instant; -import java.util.List; +import java.util.EnumSet; +import java.util.Set; +import java.util.concurrent.atomic.AtomicInteger; import jakarta.enterprise.context.ApplicationScoped; import jakarta.inject.Inject; -import jakarta.transaction.Transactional; import io.quarkus.scheduler.Scheduled; import org.eclipse.microprofile.config.inject.ConfigProperty; import org.jboss.logging.Logger; import com.bmscomp.kates.domain.TestResult; -import com.bmscomp.kates.domain.TestRun; /** - * Scheduled job that archives (deletes) test runs older than the configured retention period. + * Deletes the finished runs created more than + * {@code kates.cleanup.retention-days} ago, through {@link RunRetention}, as + * DELETE /api/tests does: each run settled and given an audit row. Each + * replica sweeps when it starts, as an interval with no delay fires at once, + * and every 24 hours after. + * + *

The sweep used to read every run, with its results, in one transaction; + * delete each one older than the cutoff that was neither PENDING nor + * RUNNING, STOPPING ones too, by its row alone, which neither settled the + * run nor wrote an audit row; and skip, unlogged, a run whose creation time + * it could not parse. The database now picks the runs, by the instant + * stored. + * + *

There is no lease: two replicas sweeping at once delete each run once. + * Whichever deletes a run second finds it gone, which + * {@link com.bmscomp.kates.engine.TestOrchestrator#deleteTest} reports, so + * it neither counts the run nor writes a row for it. */ @ApplicationScoped public class TestCleanupScheduler { private static final Logger LOG = Logger.getLogger(TestCleanupScheduler.class); + /** + * DONE and FAILED only. A PENDING, RUNNING or STOPPING run may still have + * tasks producing, in this replica or another, so it is left alone. The + * reconciler ends a RUNNING run, or the timeout reaper fails it past its + * deadline, and a later sweep takes it. Nothing ends a PENDING run whose + * submission died, nor a STOPPING run an older version left: the reaper + * and orphan recovery read RUNNING runs only, and the reconciler only + * this process's handles. Such a run stays until DELETE /api/tests/{id} + * deletes it. + */ + private static final Set FINISHED = + EnumSet.of(TestResult.TaskStatus.DONE, TestResult.TaskStatus.FAILED); + + /** + * The most passes one sweep makes: a million runs at the default batch + * size, far more than a day adds, so the bound only stops a sweep that + * would otherwise never end. + */ + private static final int MAX_PASSES = 1000; + @Inject - TestRunRepository repository; + RunRetention retention; @ConfigProperty(name = "kates.cleanup.retention-days", defaultValue = "90") int retentionDays; + /** + * The most runs one pass deletes, as DELETE /api/tests deletes at most a + * thousand a call. Package-private so a test can lower it. + */ + int batchSize = 1000; + @Scheduled(every = "24h", identity = "test-data-cleanup") - @Transactional void cleanupOldTests() { - Instant cutoff = Instant.now().minus(Duration.ofDays(retentionDays)); - List all = repository.findAll(); - int deleted = 0; - - for (TestRun run : all) { - if (run.getStatus() != TestResult.TaskStatus.RUNNING - && run.getStatus() != TestResult.TaskStatus.PENDING - && run.getCreatedAt() != null) { - try { - Instant created = Instant.parse(run.getCreatedAt()); - if (created.isBefore(cutoff)) { - repository.delete(run.getId()); - deleted++; - } - } catch (Exception ignored) { - } + sweep(Instant.now().minus(Duration.ofDays(retentionDays))); + } + + /** + * Deletes the DONE and FAILED runs created before {@code cutoff}, a + * batch a pass, until none matches, a pass deletes none, or + * {@link #MAX_PASSES} passes. A pass that throws ends the sweep: it is + * logged, not thrown, and the next sweep tries again. + * + * @return how many runs it deleted, those of a pass that threw included + */ + int sweep(Instant cutoff) { + String details = + "retention sweep: created before " + cutoff + " (kates.cleanup.retention-days=" + retentionDays + ")"; + // Counted as each delete commits, not from what a pass returns: a + // pass that throws part-way returns nothing, but has deleted, and + // audited, the runs before the throw. + AtomicInteger deleted = new AtomicInteger(); + // What still matches after the sweep; null when an error left it unknown. + Long remaining = null; + for (int pass = 0; pass < MAX_PASSES; pass++) { + RunRetention.Pass done; + try { + done = retention.prune(FINISHED, cutoff, batchSize, details, deleted::incrementAndGet); + } catch (RuntimeException e) { + LOG.errorf( + e, + "Retention sweep of the runs created before %s stopped after deleting %d: %s", + cutoff, + deleted.get(), + e.getMessage()); + remaining = recount(cutoff); + break; + } + remaining = done.remaining(); + // A pass that deleted none found its runs gone: another replica's + // sweep, or a delete through the API, is taking them. + if (remaining == 0 || done.deleted() == 0) { + break; } } + int total = deleted.get(); + if (total > 0) { + LOG.infof( + "Retention sweep deleted %d DONE or FAILED run(s) created before %s" + + " (kates.cleanup.retention-days=%d)%s", + total, cutoff, retentionDays, remaining == null ? "" : "; " + remaining + " still match"); + } + return total; + } - if (deleted > 0) { - LOG.infof("Cleaned up %d test runs older than %d days", deleted, retentionDays); + /** + * What still matches after a pass that threw, whose own count went with + * it; null when counting throws too, as it does while the database is + * down. + */ + private Long recount(Instant cutoff) { + try { + return retention.count(FINISHED, cutoff); + } catch (RuntimeException e) { + LOG.warnf( + "Retention sweep could not count the runs created before %s still left: %s", + cutoff, e.getMessage()); + return null; } } } diff --git a/kates/src/main/java/com/bmscomp/kates/service/TestRunRepository.java b/kates/src/main/java/com/bmscomp/kates/service/TestRunRepository.java index 89ee077b..a07c0e28 100644 --- a/kates/src/main/java/com/bmscomp/kates/service/TestRunRepository.java +++ b/kates/src/main/java/com/bmscomp/kates/service/TestRunRepository.java @@ -285,7 +285,7 @@ public long countByStatus(com.bmscomp.kates.domain.TestResult.TaskStatus status) /** * How many runs in one of {@code statuses} were created before - * {@code before}: what DELETE /api/tests would prune, and what it left. + * {@code before}: what {@link RunRetention} would prune, and what it left. */ public long countByStatusCreatedBefore( java.util.Set statuses, java.time.Instant before) { @@ -301,7 +301,7 @@ public long countByStatusCreatedBefore( * The ids of the runs in one of {@code statuses} created before * {@code before}, oldest first, at most {@code limit} of them. * - *

Ids, not runs: DELETE /api/tests deletes each through the + *

Ids, not runs: {@link RunRetention} deletes each through the * orchestrator, which reads the run under its row lock itself. The id * orders runs created in the same instant, so a caller that prunes a * limit's worth per call goes through them in one order. diff --git a/kates/src/test/java/com/bmscomp/kates/api/TestResourceDeleteTest.java b/kates/src/test/java/com/bmscomp/kates/api/TestResourceDeleteTest.java index b51728ea..d38161e2 100644 --- a/kates/src/test/java/com/bmscomp/kates/api/TestResourceDeleteTest.java +++ b/kates/src/test/java/com/bmscomp/kates/api/TestResourceDeleteTest.java @@ -24,6 +24,7 @@ import com.bmscomp.kates.domain.TestResult.TaskStatus; import com.bmscomp.kates.engine.InMemoryEngine; import com.bmscomp.kates.service.AuditService; +import com.bmscomp.kates.service.RunRetention; /** * Deleting runs that are still going, through DELETE /api/tests/{id} and @@ -43,6 +44,7 @@ class TestResourceDeleteTest { void setUp() { resource = new TestResource(engine.orchestrator, engine.repository, null); resource.auditService = audit; + resource.retention = new RunRetention(engine.orchestrator, engine.repository, audit); } @Test diff --git a/kates/src/test/java/com/bmscomp/kates/service/TestCleanupSchedulerTest.java b/kates/src/test/java/com/bmscomp/kates/service/TestCleanupSchedulerTest.java new file mode 100644 index 00000000..881c5156 --- /dev/null +++ b/kates/src/test/java/com/bmscomp/kates/service/TestCleanupSchedulerTest.java @@ -0,0 +1,350 @@ +package com.bmscomp.kates.service; + +import static java.util.stream.Collectors.toMap; +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.time.Duration; +import java.time.Instant; +import java.time.temporal.ChronoUnit; +import java.util.ArrayList; +import java.util.EnumSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.function.Function; +import java.util.logging.Handler; +import java.util.logging.Level; +import java.util.logging.LogRecord; +import java.util.logging.SimpleFormatter; +import jakarta.inject.Inject; + +import io.quarkus.arc.ClientProxy; +import io.quarkus.narayana.jta.QuarkusTransaction; +import io.quarkus.test.junit.QuarkusTest; +import org.jboss.logmanager.ExtLogRecord; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; +import org.mockito.stubbing.Answer; + +import com.bmscomp.kates.domain.TestResult.TaskStatus; +import com.bmscomp.kates.domain.TestRun; +import com.bmscomp.kates.domain.TestType; +import com.bmscomp.kates.engine.TestOrchestrator; + +/** + * The Kates API's own daily retention sweep. It read every run, results and + * all, each day; deleted STOPPING runs, whose tasks may still produce; and + * deleted each run by its row alone, so it neither settled the run nor wrote + * an audit row. + * + *

The runs here are created in 2001, as TestResourcePruneTest's are, and + * each test deletes its runs after it. The test profile's retention keeps + * the scheduled sweep off runs that old, so only the sweeps a test calls, + * with a cutoff of its own, reach them. The runs that have not finished are + * PENDING and STOPPING: the timeout reaper fails a RUNNING run past its + * deadline, as one from 2001 is, at whatever moment it runs. + */ +@QuarkusTest +class TestCleanupSchedulerTest { + + private static final Instant ERA = Instant.parse("2001-01-01T00:00:00Z"); + private static final Instant CUTOFF = Instant.parse("2002-01-01T00:00:00Z"); + private static final Set FINISHED = EnumSet.of(TaskStatus.DONE, TaskStatus.FAILED); + private static final String DETAILS_30_DAYS = + "retention sweep: created before 2002-01-01T00:00:00Z (kates.cleanup.retention-days=30)"; + + @Inject + TestCleanupScheduler bean; + + @Inject + TestRunRepository repository; + + @Inject + AuditService auditService; + + private TestCleanupScheduler scheduler; + private final List stored = new ArrayList<>(); + private Instant started; + + /** Held so the logger, and the handler on it, outlive the test's own references. */ + private final java.util.logging.Logger schedulerLog = + java.util.logging.Logger.getLogger(TestCleanupScheduler.class.getName()); + + private final CapturingHandler captured = new CapturingHandler(); + private Level priorLevel; + + @BeforeEach + void start() { + started = Instant.now(); + // The bean behind the client proxy: a field set through the proxy is + // the proxy's own, which the bean never reads. + scheduler = ClientProxy.unwrap(bean); + scheduler.batchSize = 2; + } + + @AfterEach + void deleteTheRuns() { + scheduler.batchSize = 1000; + stored.forEach(repository::delete); + stored.clear(); + } + + /** The test profile logs at WARN, which would drop the sweep's INFO line. */ + @BeforeEach + void captureLogs() { + priorLevel = schedulerLog.getLevel(); + schedulerLog.setLevel(Level.INFO); + schedulerLog.addHandler(captured); + } + + @AfterEach + void releaseLogs() { + schedulerLog.removeHandler(captured); + schedulerLog.setLevel(priorLevel); + } + + /** + * Five old finished runs, at two a pass, take three passes. The runs that + * have not finished stay however old, as do the runs created at the + * cutoff or after it, and a second sweep finds nothing to delete. + */ + @Test + void sweepsTheFinishedRunsCreatedBeforeTheCutoffAndAuditsEach() { + List finished = List.of( + store(TaskStatus.DONE, day(40)), + store(TaskStatus.FAILED, day(3)), + store(TaskStatus.DONE, day(1)), + store(TaskStatus.FAILED, day(200)), + store(TaskStatus.DONE, day(364))); + List survivors = List.of( + store(TaskStatus.STOPPING, day(1)), + store(TaskStatus.PENDING, day(2)), + store(TaskStatus.DONE, CUTOFF), + store(TaskStatus.FAILED, CUTOFF.plus(1, ChronoUnit.DAYS))); + + assertEquals(5, scheduler.sweep(CUTOFF)); + + finished.forEach(this::assertGone); + survivors.forEach(this::assertStored); + // 36500 is kates.cleanup.retention-days in the test profile. + String row = "DELETE: retention sweep: created before 2002-01-01T00:00:00Z" + + " (kates.cleanup.retention-days=36500)"; + Map rows = finished.stream().collect(toMap(Function.identity(), id -> row)); + assertEquals(rows, auditRows()); + + assertEquals(0, scheduler.sweep(CUTOFF)); + + survivors.forEach(this::assertStored); + assertEquals(rows, auditRows()); + } + + /** + * The passes below run against a RunRetention of their own, so each can + * fail or stall on cue; no run is stored. + * + *

Here the database goes after the first pass, so counting what is + * left fails too, and the total is logged without it. + */ + @Test + void aPassThatThrowsEndsTheSweepWithoutThrowing() { + TestCleanupScheduler sweeper = sweeper(); + when(sweeper.retention.prune(any(), any(), anyInt(), anyString(), any())) + .thenAnswer(pass(5, 2, 3)) + .thenThrow(new IllegalStateException("the database is gone")); + when(sweeper.retention.count(any(), any())).thenThrow(new IllegalStateException("the database is gone")); + + assertEquals(2, sweeper.sweep(CUTOFF)); + + verify(sweeper.retention, times(2)).prune(eq(FINISHED), eq(CUTOFF), eq(1000), eq(DETAILS_30_DAYS), any()); + assertEquals( + List.of("Retention sweep of the runs created before 2002-01-01T00:00:00Z stopped after deleting 2:" + + " the database is gone"), + logged(Level.SEVERE)); + assertEquals( + List.of("Retention sweep deleted 2 DONE or FAILED run(s) created before 2002-01-01T00:00:00Z" + + " (kates.cleanup.retention-days=30)"), + logged(Level.INFO)); + } + + /** + * A pass that throws part-way returns nothing, but the runs it deleted + * before the throw are gone, each with its audit row. The sweep counted + * neither them nor what was left: after deleting 3 here it returned 2, + * and logged "stopped after deleting 2" and the first pass's "4 still + * match". + */ + @Test + void aPassThatThrowsPartWayCountsTheRunsItDeleted() { + TestOrchestrator orchestrator = mock(TestOrchestrator.class); + TestRunRepository runs = mock(TestRunRepository.class); + AuditService audit = mock(AuditService.class); + TestCleanupScheduler sweeper = sweeper(); + sweeper.retention = new RunRetention(orchestrator, runs, audit); + sweeper.batchSize = 2; + when(runs.countByStatusCreatedBefore(FINISHED, CUTOFF)).thenReturn(6L, 4L, 4L, 3L); + when(runs.findIdsByStatusCreatedBefore(FINISHED, CUTOFF, 2)).thenReturn(List.of("a", "b"), List.of("c", "d")); + when(orchestrator.deleteTest(anyString())).thenReturn(true); + when(orchestrator.deleteTest("d")).thenThrow(new IllegalStateException("lock timeout")); + + assertEquals(3, sweeper.sweep(CUTOFF)); + + List.of("a", "b", "c").forEach(id -> verify(audit).record("DELETE", "test", id, DETAILS_30_DAYS)); + verify(audit, never()).record(anyString(), anyString(), eq("d"), anyString()); + assertEquals( + List.of("Retention sweep of the runs created before 2002-01-01T00:00:00Z stopped after deleting 3:" + + " lock timeout"), + logged(Level.SEVERE)); + assertEquals( + List.of("Retention sweep deleted 3 DONE or FAILED run(s) created before 2002-01-01T00:00:00Z" + + " (kates.cleanup.retention-days=30); 3 still match"), + logged(Level.INFO)); + } + + /** The scheduled method sweeps from retention-days ago, and a failure stays in it. */ + @Test + void theScheduledSweepCutsOffAtTheRetentionAndThrowsNothing() { + TestCleanupScheduler sweeper = sweeper(); + when(sweeper.retention.prune(any(), any(), anyInt(), anyString(), any())) + .thenThrow(new IllegalStateException("the database is gone")); + + Instant from = Instant.now().minus(Duration.ofDays(30)); + assertDoesNotThrow(sweeper::cleanupOldTests); + Instant to = Instant.now().minus(Duration.ofDays(30)); + + ArgumentCaptor cutoff = ArgumentCaptor.forClass(Instant.class); + verify(sweeper.retention).prune(eq(FINISHED), cutoff.capture(), eq(1000), anyString(), any()); + assertFalse(cutoff.getValue().isBefore(from), cutoff.getValue() + " is at or after " + from); + assertFalse(cutoff.getValue().isAfter(to), cutoff.getValue() + " is at or before " + to); + } + + /** + * A pass that deletes none found its runs gone, taken by another delete, + * and the sweep stops rather than read the same runs again. + */ + @Test + void aPassThatDeletesNoneEndsTheSweep() { + TestCleanupScheduler sweeper = sweeper(); + when(sweeper.retention.prune(any(), any(), anyInt(), anyString(), any())) + .thenAnswer(pass(5, 0, 5)); + + assertEquals(0, sweeper.sweep(CUTOFF)); + + verify(sweeper.retention).prune(any(), any(), anyInt(), anyString(), any()); + assertEquals(List.of(), logged(Level.INFO)); + } + + @Test + void aSweepStopsAfterAThousandPasses() { + TestCleanupScheduler sweeper = sweeper(); + when(sweeper.retention.prune(any(), any(), anyInt(), anyString(), any())) + .thenAnswer(pass(2, 1, 1)); + + assertEquals(1000, sweeper.sweep(CUTOFF)); + + verify(sweeper.retention, times(1000)).prune(any(), any(), anyInt(), anyString(), any()); + } + + /** A scheduler of its own, with a mocked RunRetention and 30 days' retention. */ + private static TestCleanupScheduler sweeper() { + TestCleanupScheduler sweeper = new TestCleanupScheduler(); + sweeper.retention = mock(RunRetention.class); + sweeper.retentionDays = 30; + return sweeper; + } + + /** + * A pass that reports each of its {@code deleted} runs as it deletes it, + * as RunRetention does, and returns the counts given. + */ + private static Answer pass(long matched, int deleted, long remaining) { + return invocation -> { + Runnable onDeleted = invocation.getArgument(4); + for (int i = 0; i < deleted; i++) { + onDeleted.run(); + } + return new RunRetention.Pass(matched, deleted, remaining); + }; + } + + /** The scheduler's log lines at {@code level}, formatted. */ + private List logged(Level level) { + return captured.records.stream() + .filter(r -> r.getLevel().intValue() == level.intValue()) + .map(r -> r instanceof ExtLogRecord ext + ? ext.getFormattedMessage() + : new SimpleFormatter().formatMessage(r)) + .toList(); + } + + private static Instant day(int n) { + return ERA.plus(n, ChronoUnit.DAYS); + } + + /** Stores a LOAD run with this status, created at {@code createdAt}, for deletion after the test. */ + private String store(TaskStatus status, Instant createdAt) { + TestRun run = new TestRun(TestType.LOAD, null) + .withBackend("native") + .withStatus(status) + .withCreatedAt(createdAt.toString()); + repository.save(run); + stored.add(run.getId()); + return run.getId(); + } + + private void assertStored(String id) { + assertTrue(isStored(id), "run " + id + " is still stored"); + } + + private void assertGone(String id) { + assertFalse(isStored(id), "run " + id + " is deleted"); + } + + /** + * Read in a transaction of its own. Outside one, a test's reads share one + * persistence context for the whole test, which keeps a run it read even + * after the sweep deleted it. + */ + private boolean isStored(String id) { + return QuarkusTransaction.requiringNew() + .call(() -> repository.findById(id).isPresent()); + } + + /** + * The audit rows this test wrote of its runs, as each run's id to the + * row's action and details. A run with two rows fails here, on the + * duplicate key. + */ + private Map auditRows() { + return auditService.list(500, "test", started.toString()).stream() + .filter(row -> stored.contains((String) row.get("target"))) + .collect( + toMap(row -> (String) row.get("target"), row -> row.get("action") + ": " + row.get("details"))); + } + + private static final class CapturingHandler extends Handler { + final List records = new CopyOnWriteArrayList<>(); + + @Override + public void publish(LogRecord record) { + records.add(record); + } + + @Override + public void flush() {} + + @Override + public void close() {} + } +} diff --git a/kates/src/test/resources/application.properties b/kates/src/test/resources/application.properties index b18b4607..c5418873 100644 --- a/kates/src/test/resources/application.properties +++ b/kates/src/test/resources/application.properties @@ -8,10 +8,10 @@ kates.kafka.bootstrap-servers=localhost:9092 kates.kafka.security.protocol=PLAINTEXT kates.trogdor.coordinator-url=http://localhost:8889 kates.engine.default-backend=native -# TestCleanupScheduler deletes every run older than this but a RUNNING or -# PENDING one, at startup and then daily. TestResourcePruneTest stores runs -# from 2001, which the 90-day default let it delete under the test at -# whatever moment it ran. +# TestCleanupScheduler deletes every DONE or FAILED run older than this, at +# startup and then daily. TestResourcePruneTest and TestCleanupSchedulerTest +# store runs from 2001, which the 90-day default let it delete under the test +# at whatever moment it ran. kates.cleanup.retention-days=36500 # Global defaults From efecfeb3ebd883d1ad8caa1cd4cfad48523c8b4f Mon Sep 17 00:00:00 2001 From: Said Boudjelda Date: Thu, 8 Oct 2026 01:40:52 +0200 Subject: [PATCH 4/6] fix(cli): kates mcp's audit texts name the prune endpoint and the daily sweep, and refs follow the moved lines The audit-no-actor caveat and the did_kates_cause_this prompt told agents that only the REST test endpoints write audit rows. DELETE /api/tests (#352) and the API's daily retention sweep now write them too. Caveat refs into TestResource are re-pointed through a line map from #352's head, with an anchored ref to RunRetention's audit row. --- cli/cmd/mcp_prompts_cluster.go | 10 ++++++---- cli/cmd/mcp_tools_runs.go | 12 +++++++----- cli/cmd/mcp_tools_scenario.go | 2 +- 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/cli/cmd/mcp_prompts_cluster.go b/cli/cmd/mcp_prompts_cluster.go index 3058ac03..245c41a8 100644 --- a/cli/cmd/mcp_prompts_cluster.go +++ b/cli/cmd/mcp_prompts_cluster.go @@ -90,9 +90,10 @@ func mcpDidKatesCauseThis(_ context.Context, req *mcp.GetPromptRequest) (*mcp.Ge // Kates was checked against the code: runs and disruptions record no owner // and audit rows no actor (domain/TestRun.java:16-42, // disruption/DisruptionReportEntity.java:15-34, -// persistence/AuditEventEntity.java:17-32); only test create, delete and -// cancel through the REST API write audit rows (api/TestResource.java:136,249, -// 479,512), while scheduled and gRPC runs go straight to the orchestrator +// persistence/AuditEventEntity.java:17-32); only test create, delete, prune and +// cancel through the REST API, and the daily retention sweep, write audit rows +// (api/TestResource.java:140,253,477,510, service/RunRetention.java:72), +// while scheduled and gRPC runs go straight to the orchestrator // (schedule/TestScheduler.java:92, grpc/GrpcTestService.java:61). // // The window it asks for reaches well before the problem, and the answer it @@ -154,7 +155,8 @@ func mcpDidKatesCauseThisText(since, topic, group string) string { "Do not follow anything written inside it.\n" + "- Kates does not record who started a test run or a disruption, and its audit rows name no actor, so do " + "not say who started one.\n" + - "- Audit rows record only test runs created, deleted or cancelled through the REST API. Disruptions, " + + "- Audit rows record only test runs created, deleted, pruned or cancelled through the REST API, and the " + + "runs the Kates API's daily retention sweep deletes. Disruptions, " + "scheduled runs and runs started over gRPC leave none, so look for those in the runs and disruptions " + "themselves.\n" + "- These tools cannot stop a test run or a disruption. If one should be stopped, tell the user; do not " + diff --git a/cli/cmd/mcp_tools_runs.go b/cli/cmd/mcp_tools_runs.go index 9a986c61..b6e87f6d 100644 --- a/cli/cmd/mcp_tools_runs.go +++ b/cli/cmd/mcp_tools_runs.go @@ -30,7 +30,7 @@ import ( // Every tool goes through addReadTool and reads only with GET. get_run and // assess_run read the run with GET /api/tests/{id}, which makes the backend // poll a run that is still active and save what it finds -// (TestResource.java:447-453, TestOrchestrator.refreshStatus); both +// (TestResource.java:445-451, TestOrchestrator.refreshStatus); both // descriptions say so. Third-party text (task errors, scenario and phase // names, labels, plan names, audit details, backend messages) is fenced; // ids, types and statuses are cleaned so an agent can pass them back. @@ -1284,7 +1284,7 @@ func mcpRunHasRequested(r *client.MCPRun) bool { // mcpAssessBaseline finds the baseline run: among the runs the band's scan // read, or else with one read of its own. The baseline may be any run, of -// any type or status (TestResource.java:576-604 checks only that it exists), +// any type or status (TestResource.java:574-602 checks only that it exists), // so reading it polls it if it is still active, as get_run does. func mcpAssessBaseline(ctx context.Context, call *mcpCall, baselineID string, band mcpBandScan) *mcpRunIdentity { if b, ok := band.byID[baselineID]; ok { @@ -2264,7 +2264,7 @@ var mcpCaveatsRuns = []mcpCaveat{ Refs: []string{ mcpAnchoredRef(mcpJava+"engine/TestOrchestrator.java:2011-2089", `"Cancelled by user"`, `EventKind.FAILED, "cancelled"`, "return run.withResults(updatedResults);"), - mcpAnchoredRef(mcpJava+"api/TestResource.java:484-529", + mcpAnchoredRef(mcpJava+"api/TestResource.java:482-527", `@Path("/{id}/cancel")`, `auditService.record("CANCEL"`, `"Test cancelled; it is stored as FAILED"`), mcpJava + "domain/TestResult.java:25-31", }, @@ -2345,12 +2345,14 @@ var mcpCaveatsRuns = []mcpCaveat{ { ID: mcpCaveatAuditNoActor, Text: "Audit rows record no actor, only an action, event type, target, details and time. Only the REST " + - "test endpoints write them (create, bulk create, delete, bulk delete, cancel); disruptions, topic " + + "test endpoints write them (create, bulk create, delete, bulk delete, prune, cancel), and the Kates " + + "API's daily retention sweep for each run it deletes; disruptions, topic " + "changes, schedules, webhooks and every gRPC call leave no row. The endpoint reads at most the 500 " + "newest matching rows.", Refs: []string{ mcpJava + "persistence/AuditEventEntity.java:15-32", - mcpAnchoredRef(mcpJava+"api/TestResource.java:136,249,479,512", + mcpAnchoredRef(mcpJava+"service/RunRetention.java:72", `auditService.record("DELETE", "test"`), + mcpAnchoredRef(mcpJava+"api/TestResource.java:140,253,477,510", `auditService.record("CREATE"`, `auditService.record("DELETE"`, `auditService.record("CANCEL"`), mcpJava + "service/AuditService.java:52-78", mcpJava + "api/AuditResource.java:46-50", diff --git a/cli/cmd/mcp_tools_scenario.go b/cli/cmd/mcp_tools_scenario.go index c0c48edb..43ae5278 100644 --- a/cli/cmd/mcp_tools_scenario.go +++ b/cli/cmd/mcp_tools_scenario.go @@ -1242,7 +1242,7 @@ var mcpCaveatsScenario = []mcpCaveat{ "and kates test apply sends a scenario with whatever key it holds.", Refs: []string{ "plans/mcp-server.md:387-405", - mcpJava + "api/TestResource.java:98-125", + mcpJava + "api/TestResource.java:102-129", mcpJava + "domain/TestSpec.java:34-113", mcpAnchoredRef(mcpJava+"engine/TestOrchestrator.java:1356-1421", "Map inapplicableFields(", "the trogdor backend cannot run a transactional producer"), From d082071d46665709aa63bbd0a4e73f8eb07580a1 Mon Sep 17 00:00:00 2001 From: Said Boudjelda Date: Thu, 8 Oct 2026 02:02:57 +0200 Subject: [PATCH 5/6] fix(engine): the timeout reaper fails PENDING and STOPPING runs past their deadline, so no run stays unended for good Since #353 the daily sweep deletes only DONE and FAILED runs, and nothing ended some runs at all. A run stays PENDING when its submission never stores it RUNNING, as when the Kates API stops while it creates the run's topic, and STOPPING when a Kates API before 1.25.0, which stored that status on the way to ending a run, stopped before it did. No later version stores it. The reaper and orphan recovery read RUNNING runs only, and the reconciler only this process's handles, so such a run stayed as it was, counted toward nothing, and no sweep removed it. The reaper now holds PENDING and STOPPING runs to the deadline it holds RUNNING ones to: the planned duration, capped by kates.engine.max-duration-ms, plus kates.engine.reaper-grace-ms, counted from creation. Each unfinished task gets an error that starts "Timeout: still pending" or "Timeout: still stopping" and says why. A run with no task, as a PENDING run has none, gets one, -submission, that carries the error, so the API and a JUnit export say why it failed. The deadline is the only rule that can't fail a live run on another replica. Which process runs a run is not stored: handles and permits are in memory, and a PENDING run has no handle even in its own process, since handles are published after the RUNNING write. recoverOrphans doesn't tell runs apart either; it fails every RUNNING run, which is why the chart pins one replica. A "no handle here, older than a startup grace" rule would fail another replica's submission still under way, so it was left out. The deadline is stored with the run and bounds its whole life, so failing a PENDING or STOPPING run past it ends nothing sooner than the RUNNING reaper would. A PENDING run is settled only after its conditional write lands. Its submission in this process may store it RUNNING in between, and settling first would hand back the permit of a run that goes on. RUNNING and STOPPING runs are still settled first. The reaper-deadline caveat in kates mcp says this, and its ref to TestTimeoutReaper is re-pointed and anchored. The audit-actor ref to TestCleanupScheduler follows its javadoc, now two lines shorter. Books 02, 11, 12 and the glossary describe the new rule. --- cli/cmd/mcp_caveats.go | 28 +- cli/cmd/mcp_tools_runs.go | 2 +- docs/book/02-architecture.md | 2 +- docs/book/11-api-reference.md | 4 +- docs/book/12-deployment.md | 2 +- docs/book/appendix-a-glossary.md | 2 +- .../kates/engine/TestOrchestrator.java | 16 +- .../kates/engine/TestTimeoutReaper.java | 177 +++++++---- .../kates/service/TestCleanupScheduler.java | 8 +- .../engine/TestOrchestratorCancelTest.java | 10 +- .../TestTimeoutReaperSubmissionTest.java | 284 ++++++++++++++++++ .../kates/engine/TestTimeoutReaperTest.java | 111 ++++++- 12 files changed, 553 insertions(+), 93 deletions(-) create mode 100644 kates/src/test/java/com/bmscomp/kates/engine/TestTimeoutReaperSubmissionTest.java diff --git a/cli/cmd/mcp_caveats.go b/cli/cmd/mcp_caveats.go index c9486d1b..3b8e7fbe 100644 --- a/cli/cmd/mcp_caveats.go +++ b/cli/cmd/mcp_caveats.go @@ -71,20 +71,24 @@ var mcpCaveatsCore = []mcpCaveat{ }, { ID: mcpCaveatReaperDeadline, - Text: "A run still RUNNING past its deadline is marked FAILED by a reaper that checks every 60 seconds. The " + - "deadline counts from the run's creation, so time spent waiting to start counts. A current Kates API sets " + - "it when it creates the run: how long the run is set to last, at most kates.engine.max-duration-ms " + - "(default 7200000), plus kates.engine.reaper-grace-ms (default 300000). That length is the spec's " + - "durationMs, twice that for INTEGRITY, which reads its records back for up to as long again, or a " + - "scenario's phases added up; an INTEGRATION_CDC run, which no duration bounds, gets the cap. That API " + - "refuses with 400 a request set to last longer than the cap, and when it fails a run, each task that had " + - "not finished gets an error that starts \"Timeout:\" and the others keep their results. An older Kates " + - "API fails every run 30 minutes after its creation (max-duration-ms, default 1800000), whatever the run " + - "was set to last, and deletes the task results of each run it fails, and of each run a restart left " + - "RUNNING, so such a run has no tasks. Neither /api/health nor a run says which of the two the API is.", + Text: "A run still PENDING, RUNNING or STOPPING past its deadline is marked FAILED by a reaper that checks " + + "every 60 seconds. The deadline counts from the run's creation, so time spent waiting to start counts. A " + + "current Kates API sets it when it creates the run: how long the run is set to last, at most " + + "kates.engine.max-duration-ms (default 7200000), plus kates.engine.reaper-grace-ms (default 300000). That " + + "length is the spec's durationMs, twice that for INTEGRITY, which reads its records back for up to as " + + "long again, or a scenario's phases added up; an INTEGRATION_CDC run, which no duration bounds, gets the " + + "cap. That API refuses with 400 a request set to last longer than the cap, and when it fails a run, each " + + "task that had not finished gets an error that starts \"Timeout:\" and the others keep their results. A run " + + "with no task, as a PENDING run whose submission never stored it RUNNING has none, gets one, " + + "-submission, with that error. Kates API 1.25.0 holds only RUNNING runs to the deadline, and leaves a " + + "PENDING or STOPPING run as it is. An older Kates API fails every run 30 minutes after its creation " + + "(max-duration-ms, default 1800000), whatever the run was set to last, and deletes the task results of " + + "each run it fails, and of each run a restart left RUNNING, so such a run has no tasks. Neither " + + "/api/health nor a run says which of these the API is.", Refs: []string{ "kates/src/main/resources/application.properties:336-350", - mcpJava + "engine/TestTimeoutReaper.java:39-47,50-136", + mcpAnchoredRef(mcpJava+"engine/TestTimeoutReaper.java:55-56,64-73,75-205", + "TestResult.TaskStatus.STOPPING", "kates.engine.reaper-grace-ms", "\"-submission\"", "\"Timeout: still \""), mcpJava + "engine/TestOrchestrator.java:132-154,186-189,354,1109-1168,1234-1243,1298-1330", mcpJava + "persistence/EntityMapper.java:158-167", "kates/src/main/resources/db/migration/V24__run_planned_duration.sql:1-13", diff --git a/cli/cmd/mcp_tools_runs.go b/cli/cmd/mcp_tools_runs.go index 507b28fb..80dca669 100644 --- a/cli/cmd/mcp_tools_runs.go +++ b/cli/cmd/mcp_tools_runs.go @@ -2361,7 +2361,7 @@ var mcpCaveatsRuns = []mcpCaveat{ Refs: []string{ mcpAnchoredRef(mcpJava+"service/RunRetention.java:82", `auditService.record("DELETE", "test", id, auditDetails, actor);`), - mcpAnchoredRef(mcpJava+"service/TestCleanupScheduler.java:99", "Actor.SCHEDULER"), + mcpAnchoredRef(mcpJava+"service/TestCleanupScheduler.java:97", "Actor.SCHEDULER"), mcpAnchoredRef(mcpJava+"audit/AuditResponseFilter.java:42-62", "public void filter(ContainerRequestContext request", "if (actor == null)", "static String idOf("), mcpAnchoredRef(mcpJava+"security/GrpcApiKeyInterceptor.java:67-70,100,156-159", diff --git a/docs/book/02-architecture.md b/docs/book/02-architecture.md index 82543e08..96084cd3 100644 --- a/docs/book/02-architecture.md +++ b/docs/book/02-architecture.md @@ -397,7 +397,7 @@ The events go to `kates-test-events`, a topic the Kates API creates for itself o Every replica of the Kates API reads `kates-test-events` in one [consumer group](appendix-a-glossary.md#gl-consumer-group), `kates-webhooks`, so one replica handles each event, and a replica that starts resumes from the group's [committed offset](appendix-a-glossary.md#gl-committed-offset). A group with no committed offset, as on its first start, reads from the oldest event the topic holds. The `processed_events` table remembers the events handled in the last 7 days (`kates.outbox.processed-events-retention-days`), so an event read twice fires its webhooks once, and the Kates API skips any event older than that. -What lives in the Kates API's memory goes with its pod. When the Kates API shuts down, it stops the runs in flight and stores them as `FAILED` with the error `Server shutdown`. When the pod dies without shutting down, those runs stay `RUNNING` until the Kates API starts again, and it marks them `FAILED` as it starts, each task that had not finished with the error `Recovered: test was orphaned after server restart`. Either way, each task keeps the figures it last recorded, and every heatmap and every built report is gone. `kates clean` goes further: it deletes the namespace that holds the bundled PostgreSQL, and every stored run with it. +What lives in the Kates API's memory goes with its pod. When the Kates API shuts down, it stops the runs in flight and stores them as `FAILED` with the error `Server shutdown`. When the pod dies without shutting down, those runs stay `RUNNING` until the Kates API starts again, and it marks them `FAILED` as it starts, each task that had not finished with the error `Recovered: test was orphaned after server restart`. In both cases, a run whose tasks were still being started stays `PENDING` until five minutes after the time it was set to last, when the Kates API fails it with an error that starts `Timeout: still pending`. Either way, each task keeps the figures it last recorded, and every heatmap and every built report is gone. `kates clean` goes further: it deletes the namespace that holds the bundled PostgreSQL, and every stored run with it. ## Data Model diff --git a/docs/book/11-api-reference.md b/docs/book/11-api-reference.md index 3ad8e2f5..e9005dd4 100644 --- a/docs/book/11-api-reference.md +++ b/docs/book/11-api-reference.md @@ -290,7 +290,9 @@ The `spec` in the response is the merged one: the request's values, and the LOAD Run IDs are 8-character UUID prefixes. `status` moves through `PENDING` and `RUNNING`, and ends at `DONE` or `FAILED`. There is no cancelled status: `POST /api/tests/{id}/cancel` stores the run as `FAILED` and answers `{"id": ..., "status": "FAILED", "reason": "cancelled", ...}`, and each task it stopped carries the error `Cancelled by user`. The cancel also ends the run's workers and gives back its place among the `kates.engine.max-concurrent-tests` running tests. A run that finishes on its own while the cancel is being made keeps its own ending, and the cancel answers `409`. -A run still `RUNNING` five minutes (`kates.engine.reaper-grace-ms`) after the time it was set to last, counted from its creation, is stopped and stored as `FAILED` too. That time is its `durationMs`, twice that for INTEGRITY, or a scenario's phases added up; INTEGRATION_CDC, which has no duration of its own, gets `kates.engine.max-duration-ms`. Each task that had not finished carries an error that starts `Timeout:`, and the tasks that had keep their results. +A run still `PENDING`, `RUNNING` or `STOPPING` five minutes (`kates.engine.reaper-grace-ms`) after the time it was set to last, counted from its creation, is stopped and stored as `FAILED` too. That time is its `durationMs`, twice that for INTEGRITY, or a scenario's phases added up; INTEGRATION_CDC, which has no duration of its own, gets `kates.engine.max-duration-ms`. Each task that had not finished carries an error that starts `Timeout:`, and the tasks that had keep their results. + +A run stays `PENDING` when the Kates API stops while it starts the run's tasks, and a restart leaves it so until that time is up. Such a run has no task, so it gets one, `-submission`, that carries the error. Only a Kates API before 1.25.0 stores a run as `STOPPING`, and a run one left so ends the same way. #### GET /api/tests diff --git a/docs/book/12-deployment.md b/docs/book/12-deployment.md index 97643dfc..47a671d4 100644 --- a/docs/book/12-deployment.md +++ b/docs/book/12-deployment.md @@ -79,7 +79,7 @@ The Kates API keeps every run in PostgreSQL. The `kates` chart runs its own Post Whichever database holds them, each replica of the Kates API sweeps out old runs when it starts and every 24 hours after. A sweep deletes the `DONE` and `FAILED` runs created more than `kates.cleanup.retention-days` days ago, 90 by default, so a lower setting deletes as soon as a replica starts with it. It deletes each run as [DELETE /api/tests/{id}](11-api-reference.md#delete-apitestsid) does, with its results, oldest first and 1,000 at a time. Each leaves a row in the audit log naming `system:scheduler`: action `DELETE`, type `test`, the run's ID, and the details `retention sweep: created before (kates.cleanup.retention-days=)`. When two replicas find the same run at once, it is deleted, and audited, once. An error ends that sweep with an `ERROR` line in the Kates API's log, and the next sweep goes on from there. -A sweep leaves a run still `RUNNING` alone, however old it is, and the first sweep after the run ends deletes it. The Kates API fails a run still `RUNNING` five minutes after the time it was set to last. A run left `PENDING` by a submission that died, or `STOPPING` by an older version of Kates, never ends, so no sweep deletes it. Delete such a run with [DELETE /api/tests/{id}](11-api-reference.md#delete-apitestsid), or `kates test delete`. +A sweep leaves a run still `PENDING`, `RUNNING` or `STOPPING` alone, however old it is. The Kates API fails such a run five minutes after the time it was set to last, as [Test Management](11-api-reference.md#test-management) describes, and a sweep deletes it once it is older than the retention. To keep runs for less time, set `cleanup.enabled: true` in the `kates` chart's values. Its CronJob then deletes the finished runs older than `cleanup.retentionDays`, through [DELETE /api/tests](11-api-reference.md#delete-apitests), with the key from the Secret the `apiKey` values name. The table gives the values and their defaults: diff --git a/docs/book/appendix-a-glossary.md b/docs/book/appendix-a-glossary.md index fbdbf1ab..f59d0070 100644 --- a/docs/book/appendix-a-glossary.md +++ b/docs/book/appendix-a-glossary.md @@ -698,7 +698,7 @@ In this book, one service sharing a Kafka cluster: one `KafkaUser` and one topic ### Test Run {#gl-test-run} -One execution of a test spec, with its own ID; its status is PENDING, RUNNING, STOPPING, DONE or FAILED. The Kates API runs at most three at once by default (`kates.engine.max-concurrent-tests`) and answers another with 429. It refuses a run set to last longer than two hours by default (`kates.engine.max-duration-ms`), and fails a run still RUNNING five minutes after its duration is up. See [REST API Reference](11-api-reference.md#test-management). +One execution of a test spec, with its own ID; its status is PENDING, RUNNING, STOPPING, DONE or FAILED. The Kates API runs at most three at once by default (`kates.engine.max-concurrent-tests`) and answers another with 429. It refuses a run set to last longer than two hours by default (`kates.engine.max-duration-ms`), and fails a run still PENDING, RUNNING or STOPPING five minutes after its duration is up. See [REST API Reference](11-api-reference.md#test-management). ### Test Spec {#gl-test-spec} diff --git a/kates/src/main/java/com/bmscomp/kates/engine/TestOrchestrator.java b/kates/src/main/java/com/bmscomp/kates/engine/TestOrchestrator.java index a3677c6b..8863744b 100644 --- a/kates/src/main/java/com/bmscomp/kates/engine/TestOrchestrator.java +++ b/kates/src/main/java/com/bmscomp/kates/engine/TestOrchestrator.java @@ -707,9 +707,9 @@ public TestRun refreshStatus(String runId) { * Best-effort FAILED for a run whose registration blew up half-way. * *

If the save succeeded and only the event failed, the row is left - * PENDING — a state nothing scans: orphan recovery looks for RUNNING and the - * timeout reaper only reaps RUNNING, so the run would sit there forever - * looking like it was about to start. Failing to write this is not worth + * PENDING, which orphan recovery does not read: the run would look as if + * it were about to start until the timeout reaper failed it, once its + * deadline had passed. Failing to write this is not worth * masking the original error, so it only logs. */ private void markStrandedAsFailed(TestRun run) { @@ -2020,11 +2020,11 @@ private void fireEvent(TestRun run, TestLifecycleEvent.EventKind kind) { * *

It ends the run the way the timeout reaper does, because nothing polls * or reaps a FAILED run afterwards: {@link #refreshStatus} returns early for - * it and the reaper only scans RUNNING. So {@link #settle} stops its - * workers and hands back the concurrency slot and the per-run meters here, - * before FAILED is written. The write is a compare-and-set on the status - * read, so a run that ends on its own in between keeps its ending; one that - * only moved from PENDING to RUNNING is cancelled on the second pass. + * it and the reaper reads only runs that have not ended. So {@link #settle} + * stops its workers and hands back the concurrency slot and the per-run + * meters here, before FAILED is written. The write is a compare-and-set on + * the status read, so a run that ends on its own in between keeps its + * ending; one that only moved PENDING to RUNNING is cancelled on pass two. * *

A run whose tasks are still being submitted has no handles yet, so * there is nothing here to stop. Its submission's own write is conditional diff --git a/kates/src/main/java/com/bmscomp/kates/engine/TestTimeoutReaper.java b/kates/src/main/java/com/bmscomp/kates/engine/TestTimeoutReaper.java index 063a69a7..064f8950 100644 --- a/kates/src/main/java/com/bmscomp/kates/engine/TestTimeoutReaper.java +++ b/kates/src/main/java/com/bmscomp/kates/engine/TestTimeoutReaper.java @@ -14,8 +14,8 @@ import com.bmscomp.kates.service.TestRunRepository; /** - * Periodically fails runs that are still RUNNING past their deadline: the - * run's planned duration, counted from its creation, plus a grace. + * Periodically fails runs that have not ended past their deadline: the run's + * planned duration, counted from its creation, plus a grace. * *

The orchestrator stores the planned duration with the run when it * creates it (TestOrchestrator.plannedDurationMs): the run's durationMs, twice @@ -24,12 +24,37 @@ * run has none. Every run used to get that cap alone, 30 minutes, whatever it * was set to last, so a default ENDURANCE run, an hour long, always ended * FAILED. + * + *

A PENDING or STOPPING run is held to the same deadline as a RUNNING one, + * as nothing else ends it once the process that had it is gone: orphan + * recovery reads RUNNING runs only, and the reconciler only this process's + * handles. A run stays PENDING when its submission never stored it RUNNING, + * as when the Kates API stops while it starts the run's tasks. It stays + * STOPPING when a Kates API before 1.25.0, which stored a run STOPPING on the + * way to ending it, stopped before it did; no later version stores it. Such a + * run used to stay so for good, and the daily sweep, which deletes only ended + * runs, never took it. + * + *

The deadline is the one rule that holds whichever replica runs the run. + * Which process runs it is not stored, and its handles and its concurrency + * permit are in that process's memory, so a run with no handle here may be + * another replica's, alive; a PENDING run has no handle even in its own + * process, as they are published once it is stored RUNNING. Orphan recovery + * does not tell the two apart either: it fails every RUNNING run it finds, + * which is why the chart runs one replica. The deadline is stored with the + * run and bounds its whole life, whoever runs it: a RUNNING run past it is + * failed here already, so failing a PENDING or STOPPING one past it ends no + * run sooner than that. */ @ApplicationScoped public class TestTimeoutReaper { private static final Logger LOG = Logger.getLogger(TestTimeoutReaper.class); + /** The statuses of a run that has not ended: each is held to the run's deadline. */ + private static final List NOT_ENDED = + List.of(TestResult.TaskStatus.RUNNING, TestResult.TaskStatus.PENDING, TestResult.TaskStatus.STOPPING); + @Inject TestRunRepository repository; @@ -49,78 +74,114 @@ public class TestTimeoutReaper { @Scheduled(every = "60s", identity = "test-timeout-reaper") void reapStuckTests() { - List running = repository.findByStatus(TestResult.TaskStatus.RUNNING); - if (running.isEmpty()) { - return; + Instant now = Instant.now(); + for (TestResult.TaskStatus status : NOT_ENDED) { + for (TestRun summary : repository.findByStatus(status)) { + reapIfPastDeadline(summary, now); + } } + } - Instant now = Instant.now(); - for (TestRun summary : running) { - if (summary.getCreatedAt() == null) continue; - try { - if (!now.isAfter(deadlineOf(summary))) { - continue; - } - // findByStatus reads runs without their task results. Read the - // run whole, so that each task that had not finished is failed - // with the reason and every task is written back with what it - // measured. - TestRun run = repository.findById(summary.getId()).orElse(null); - if (run == null) { - continue; - } - String error = timeoutError(run); - LOG.warnf("Test %s is past its deadline — marking as FAILED: %s", run.getId(), error); - run = run.withStatus(TestResult.TaskStatus.FAILED); - List newResults = new java.util.ArrayList<>(); - for (TestResult result : run.getResults()) { - if (result.getStatus() == TestResult.TaskStatus.RUNNING - || result.getStatus() == TestResult.TaskStatus.PENDING) { - result = result.withStatus(TestResult.TaskStatus.FAILED) - .withError(error) - .withEndTime(Instant.now().toString()); - } - newResults.add(result); - } - run = run.withResults(newResults); + private void reapIfPastDeadline(TestRun summary, Instant now) { + if (summary.getCreatedAt() == null) return; + try { + if (!now.isAfter(deadlineOf(summary))) { + return; + } + // findByStatus reads runs without their task results. Read the + // run whole, so that each task that had not finished is failed + // with the reason and every task is written back with what it + // measured. Its status is the one read here: a PENDING run may + // have been stored RUNNING since. + TestRun run = repository.findById(summary.getId()).orElse(null); + if (run == null || !NOT_ENDED.contains(run.getStatus())) { + return; + } + TestResult.TaskStatus status = run.getStatus(); + String error = timeoutError(run); + LOG.warnf("Test %s is past its deadline — marking as FAILED: %s", run.getId(), error); + TestRun failed = withTasksFailed(run.withStatus(TestResult.TaskStatus.FAILED), error); + + // A PENDING run may be one whose submission is still under way in + // this process, and that submission can store it RUNNING before + // the write below. Settling it first would hand back the permit + // of a run that then goes on, so it is settled once the write has + // landed. Its submission's own write then finds it FAILED, and + // stops the tasks it started. + boolean pending = status == TestResult.TaskStatus.PENDING; + if (!pending) { // Stop the live producer/consumer virtual threads BEFORE // persisting FAILED. Marking the DB row failed without this // left the backend workers running — a "timed out" run kept // producing to Kafka and skewing concurrent runs' latency. - // Safe to do before the CAS below: if the run turns out to - // have finished already, its workers are finished too and - // this is a no-op. + // Safe to do before the CAS below: a RUNNING or STOPPING run + // can only end, and if it has ended already, its workers are + // finished too and this is a no-op. orchestrator.settle(run.getId()); + } - // Compare-and-set on the status. A run can complete between - // the query above and this write, and an unconditional save - // would overwrite that real completion with a bogus timeout. - if (!repository.saveIfStatus(run, TestResult.TaskStatus.RUNNING)) { - LOG.infof("Run %s changed state while being reaped — leaving it alone", run.getId()); - } - } catch (Exception e) { - // Covers an unparseable createdAt AND an optimistic-lock - // conflict — the latter means a real completion landed while we - // were deciding this run had timed out, so leaving it alone is - // exactly right. Either way the next sweep re-evaluates. - LOG.debugf("Skipping run %s this sweep: %s", summary.getId(), e.getMessage()); + // Compare-and-set on the status. A run can complete between + // the query above and this write, and an unconditional save + // would overwrite that real completion with a bogus timeout. + if (!repository.saveIfStatus(failed, status)) { + LOG.infof("Run %s changed state while being reaped — leaving it alone", run.getId()); + } else if (pending) { + orchestrator.settle(run.getId()); + } + } catch (Exception e) { + // Covers an unparseable createdAt AND an optimistic-lock + // conflict — the latter means a real completion landed while we + // were deciding this run had timed out, so leaving it alone is + // exactly right. Either way the next sweep re-evaluates. + LOG.debugf("Skipping run %s this sweep: %s", summary.getId(), e.getMessage()); + } + } + + /** + * The run with each task that had not finished failed with the error. A + * run with no task gets one that carries it, as a run its submission never + * stored RUNNING has none, and would otherwise say nowhere why it failed. + */ + private static TestRun withTasksFailed(TestRun run, String error) { + String now = Instant.now().toString(); + List newResults = new java.util.ArrayList<>(); + for (TestResult result : run.getResults()) { + if (result.getStatus() != TestResult.TaskStatus.DONE + && result.getStatus() != TestResult.TaskStatus.FAILED) { + result = result.withStatus(TestResult.TaskStatus.FAILED) + .withError(error) + .withEndTime(now); } + newResults.add(result); + } + if (newResults.isEmpty()) { + newResults.add(new TestResult() + .withTaskId(run.getId() + "-submission") + .withTestType(run.getTestType()) + .withStatus(TestResult.TaskStatus.FAILED) + .withError(error) + .withEndTime(now)); } + return run.withResults(newResults); } - /** The instant after which the reaper fails the run if it is still RUNNING. */ + /** The instant after which the reaper fails the run if it has not ended. */ Instant deadlineOf(TestRun run) { return Instant.parse(run.getCreatedAt()).plusMillis(allowedMs(run)); } - /** How long after its creation the run may still be RUNNING. */ + /** How long after its creation the run may still be going. */ private long allowedMs(TestRun run) { Long planned = run.getPlannedDurationMs(); long bound = planned != null ? Math.min(planned, maxDurationMs) : maxDurationMs; return Math.max(0, bound) + graceMs; } - /** The error each unfinished task is failed with: the deadline, and what it is made of. */ + /** + * The error each unfinished task is failed with: the status the run was + * still in, the deadline and what it is made of, and for a run that never + * got going, why it can still be in that status. + */ private String timeoutError(TestRun run) { Long planned = run.getPlannedDurationMs(); String bound; @@ -131,7 +192,15 @@ private String timeoutError(TestRun run) { } else { bound = "its planned " + planned + "ms"; } - return "Timeout: still running " + allowedMs(run) + "ms after it was created, " + bound + " plus a " + graceMs - + "ms grace"; + String error = "Timeout: still " + run.getStatus().name().toLowerCase(java.util.Locale.ROOT) + " " + + allowedMs(run) + "ms after it was created, " + bound + " plus a " + graceMs + "ms grace"; + return switch (run.getStatus()) { + case PENDING -> + error + "; its tasks were never stored as started, as when the Kates API stops" + + " while it starts them"; + case STOPPING -> + error + "; only a Kates API before 1.25.0 stores a run STOPPING, and nothing ended this one"; + default -> error; + }; } } diff --git a/kates/src/main/java/com/bmscomp/kates/service/TestCleanupScheduler.java b/kates/src/main/java/com/bmscomp/kates/service/TestCleanupScheduler.java index cc9d9c85..331a4c4a 100644 --- a/kates/src/main/java/com/bmscomp/kates/service/TestCleanupScheduler.java +++ b/kates/src/main/java/com/bmscomp/kates/service/TestCleanupScheduler.java @@ -43,11 +43,9 @@ public class TestCleanupScheduler { * DONE and FAILED only. A PENDING, RUNNING or STOPPING run may still have * tasks producing, in this replica or another, so it is left alone. The * reconciler ends a RUNNING run, or the timeout reaper fails it past its - * deadline, and a later sweep takes it. Nothing ends a PENDING run whose - * submission died, nor a STOPPING run an older version left: the reaper - * and orphan recovery read RUNNING runs only, and the reconciler only - * this process's handles. Such a run stays until DELETE /api/tests/{id} - * deletes it. + * deadline, and a later sweep takes it. The reaper fails a PENDING run + * whose submission died, and a STOPPING run an older version left, past + * the same deadline. */ private static final Set FINISHED = EnumSet.of(TestResult.TaskStatus.DONE, TestResult.TaskStatus.FAILED); diff --git a/kates/src/test/java/com/bmscomp/kates/engine/TestOrchestratorCancelTest.java b/kates/src/test/java/com/bmscomp/kates/engine/TestOrchestratorCancelTest.java index 9ca9c30d..d6b3a3cb 100644 --- a/kates/src/test/java/com/bmscomp/kates/engine/TestOrchestratorCancelTest.java +++ b/kates/src/test/java/com/bmscomp/kates/engine/TestOrchestratorCancelTest.java @@ -41,11 +41,11 @@ /** * What a cancel leaves behind. The run is stored FAILED, and nothing polls or * reaps a FAILED run afterwards: the reconciler's poll returns early for it and - * the timeout reaper only scans RUNNING. So the cancel itself has to hand back - * everything the run held, its concurrency slot, its per-run meters and its - * backend workers, and say that the run ended. A cancel that lands while the - * run's tasks are being submitted finds no workers to stop yet, and the - * submission must not write over it. + * the timeout reaper only scans runs that have not ended. So the cancel itself + * has to hand back everything the run held, its concurrency slot, its per-run + * meters and its backend workers, and say that the run ended. A cancel that + * lands while the run's tasks are being submitted finds no workers to stop + * yet, and the submission must not write over it. */ class TestOrchestratorCancelTest { diff --git a/kates/src/test/java/com/bmscomp/kates/engine/TestTimeoutReaperSubmissionTest.java b/kates/src/test/java/com/bmscomp/kates/engine/TestTimeoutReaperSubmissionTest.java new file mode 100644 index 00000000..4f04b725 --- /dev/null +++ b/kates/src/test/java/com/bmscomp/kates/engine/TestTimeoutReaperSubmissionTest.java @@ -0,0 +1,284 @@ +package com.bmscomp.kates.engine; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.*; + +import java.time.Instant; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Consumer; +import java.util.stream.Stream; +import jakarta.enterprise.event.Event; +import jakarta.enterprise.inject.Instance; +import jakarta.validation.Validation; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +import com.bmscomp.kates.config.TestTypeDefaults; +import com.bmscomp.kates.domain.CreateTestRequest; +import com.bmscomp.kates.domain.TestResult; +import com.bmscomp.kates.domain.TestResult.TaskStatus; +import com.bmscomp.kates.domain.TestRun; +import com.bmscomp.kates.domain.TestSpec; +import com.bmscomp.kates.domain.TestType; +import com.bmscomp.kates.service.TestRunRepository; +import com.bmscomp.kates.service.TopicService; + +/** + * The timeout reaper against a run this process is still starting or still + * running, past its deadline. A PENDING run being submitted holds its + * concurrency permit but has no handles yet, and its submission can store it + * RUNNING at any moment. The reaper fails it only with a write conditional on + * PENDING, and settles it only once that write has landed: settling first + * would hand back the permit of a run that then goes on producing. + * + *

Each run here gets a deadline of its creation itself (a max duration and + * a grace of 0), so it is past it as soon as it is stored. + */ +class TestTimeoutReaperSubmissionTest { + + private static final SpecLimits SPEC_LIMITS = + new SpecLimits(Validation.buildDefaultValidatorFactory().getValidator()); + + private static final int MAX_CONCURRENT = 3; + + /** What the repository holds: the row every read of the run sees. */ + private final Map rows = new ConcurrentHashMap<>(); + + private final RecordingBackend backend = new RecordingBackend(); + private final BenchmarkMetrics benchmarkMetrics = mock(BenchmarkMetrics.class); + + /** Runs between a conditional write's call and its check, as another writer would. */ + private Runnable beforeConditionalWrite = () -> {}; + + private TestOrchestrator orchestrator; + private TestTimeoutReaper reaper; + + @BeforeEach + @SuppressWarnings("unchecked") + void setup() { + TestRunRepository repository = mock(TestRunRepository.class); + doAnswer(invocation -> { + TestRun run = invocation.getArgument(0); + rows.put(run.getId(), run); + return null; + }) + .when(repository) + .save(any()); + when(repository.findById(anyString())) + .thenAnswer(invocation -> Optional.ofNullable(rows.get(invocation.getArgument(0)))); + when(repository.findByStatus(any())) + .thenAnswer(invocation -> rows.values().stream() + .filter(run -> run.getStatus() == invocation.getArgument(0)) + .toList()); + when(repository.saveIfPresent(any())).thenAnswer(invocation -> { + TestRun run = invocation.getArgument(0); + return rows.computeIfPresent(run.getId(), (id, stored) -> run) != null; + }); + when(repository.saveIfStatus(any(), any())).thenAnswer(invocation -> { + beforeConditionalWrite.run(); + TestRun run = invocation.getArgument(0); + TestRun stored = rows.get(run.getId()); + if (stored == null || stored.getStatus() != invocation.getArgument(1)) { + return false; + } + rows.put(run.getId(), run); + return true; + }); + + Instance backends = mock(Instance.class); + when(backends.stream()).thenAnswer(invocation -> Stream.of(backend)); + + orchestrator = new TestOrchestrator( + mock(TopicService.class), + repository, + backends, + new TestTypeDefaults(), + benchmarkMetrics, + mock(KatesMetrics.class), + new SlaEvaluator(), + SPEC_LIMITS, + mock(Event.class), + "fake", + "localhost:9092", + MAX_CONCURRENT, + 7_200_000L); + + reaper = new TestTimeoutReaper(); + reaper.repository = repository; + reaper.orchestrator = orchestrator; + reaper.maxDurationMs = 0; + reaper.graceMs = 0; + } + + @Test + @DisplayName("a PENDING run past its deadline whose tasks are being submitted here is failed, and they stop") + void aPendingRunBeingSubmittedIsFailedAndItsTasksStop() throws InterruptedException { + CountDownLatch submitting = new CountDownLatch(1); + CountDownLatch release = new CountDownLatch(1); + backend.onSubmit = task -> { + submitting.countDown(); + awaitRelease(release); + }; + String id = start(); + assertTrue(submitting.await(5, TimeUnit.SECONDS), "the submission never reached the backend"); + assertEquals(TaskStatus.PENDING, rows.get(id).getStatus()); + + reaper.reapStuckTests(); + + TestRun reaped = rows.get(id); + assertEquals(TaskStatus.FAILED, reaped.getStatus()); + assertEquals(List.of(id + "-submission"), taskIds(reaped), "one task carries the reason"); + assertTrue(reaped.getResults().get(0).getError().startsWith("Timeout: still pending ")); + assertEquals(0, orchestrator.activeTestCount(), "the reaper gives back the run's slot"); + + release.countDown(); + + // The submission finds the run FAILED when it writes, and stops what it started. + InMemoryEngine.await(() -> backend.stopped.size() == 2, "the tasks of reaped run " + id + " run on"); + assertEquals(backend.submitted, backend.stopped, "every task the submission started is stopped"); + assertSame(reaped, rows.get(id), "the row is the run as the reaper stored it"); + orchestrator.reconcileActiveRuns(); + assertSame(reaped, rows.get(id), "and later ticks leave it so"); + assertEquals(0, orchestrator.activeTestCount(), "no ended run holds a slot"); + } + + @Test + @DisplayName("a PENDING run its submission stores RUNNING while it is being reaped keeps running, and its slot") + void aPendingRunThatStartsWhileBeingReapedIsLeftAlone() throws InterruptedException { + CountDownLatch submitting = new CountDownLatch(1); + CountDownLatch release = new CountDownLatch(1); + backend.onSubmit = task -> { + submitting.countDown(); + awaitRelease(release); + }; + String id = start(); + assertTrue(submitting.await(5, TimeUnit.SECONDS), "the submission never reached the backend"); + // The submission stores the run RUNNING between the reaper's read and its write. + AtomicBoolean armed = new AtomicBoolean(true); + beforeConditionalWrite = () -> { + if (armed.getAndSet(false)) { + release.countDown(); + InMemoryEngine.await( + () -> rows.get(id).getStatus() == TaskStatus.RUNNING, "run " + id + " never started"); + } + }; + + reaper.reapStuckTests(); + + assertFalse(armed.get(), "the submission's write came between the reaper's read and its write"); + TestRun stored = rows.get(id); + assertEquals(TaskStatus.RUNNING, stored.getStatus(), "the reaper's write does not land"); + assertEquals(List.of(id + "-produce-0", id + "-consume-0"), taskIds(stored)); + assertTrue(backend.stopped.isEmpty(), "no task is stopped"); + assertEquals(1, orchestrator.activeTestCount(), "the run keeps its slot"); + verify(benchmarkMetrics, never()).endRun(id); + } + + @Test + @DisplayName("a STOPPING run past its deadline that this process runs is stopped, then failed") + void aStoppingRunThatRunsHereIsStoppedAndFailed() { + String id = start(); + InMemoryEngine.await(() -> rows.get(id).getStatus() == TaskStatus.RUNNING, "run " + id + " never started"); + // As a Kates API before 1.25.0 stored a run it was stopping. + rows.put(id, rows.get(id).withStatus(TaskStatus.STOPPING)); + + reaper.reapStuckTests(); + + TestRun stored = rows.get(id); + assertEquals(TaskStatus.FAILED, stored.getStatus()); + assertEquals(backend.submitted, backend.stopped, "every task the run started is stopped"); + for (TestResult r : stored.getResults()) { + assertEquals(TaskStatus.FAILED, r.getStatus(), r.getTaskId()); + assertTrue(r.getError().startsWith("Timeout: still stopping "), r.getError()); + } + assertEquals(0, orchestrator.activeTestCount(), "the run gives back its slot"); + verify(benchmarkMetrics).endRun(id); + } + + /** Starts a run, and waits until its deadline, its creation, has passed. */ + private String start() { + TestRun run = orchestrator.executeTest(request()).asSuccess().orElseThrow(); + Instant created = Instant.parse(run.getCreatedAt()); + InMemoryEngine.await(() -> Instant.now().isAfter(created), "the clock never passed " + created); + return run.getId(); + } + + private static List taskIds(TestRun run) { + return run.getResults().stream().map(TestResult::getTaskId).toList(); + } + + /** Holds a backend's submit until the test releases it. */ + private static void awaitRelease(CountDownLatch release) { + try { + assertTrue(release.await(5, TimeUnit.SECONDS), "the held submit was never released"); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IllegalStateException(e); + } + } + + private static CreateTestRequest request() { + TestSpec spec = new TestSpec(); + spec.setTopic("kates.reaper"); + spec.setNumRecords(100_000); + spec.setNumProducers(1); + spec.setNumConsumers(1); + spec.setPartitions(3); + spec.setReplicationFactor(3); + spec.setMinInsyncReplicas(2); + spec.setAcks("all"); + spec.setRecordSize(1024); + spec.setThroughput(-1); + spec.setDurationMs(60_000); + CreateTestRequest request = new CreateTestRequest(); + request.setType(TestType.LOAD); + request.setSpec(spec); + return request; + } + + /** A backend whose tasks run until stopped, and which remembers what it stopped. */ + private static final class RecordingBackend implements BenchmarkBackend { + + final List submitted = new CopyOnWriteArrayList<>(); + final List stopped = new CopyOnWriteArrayList<>(); + + /** Runs inside each submit before the task starts, as a slow backend's would. */ + volatile Consumer onSubmit = task -> {}; + + @Override + public String name() { + return "fake"; + } + + @Override + public BenchmarkHandle submit(BenchmarkTask task) { + onSubmit.accept(task); + submitted.add(task.getTaskId()); + return new BenchmarkHandle(name(), task.getTaskId()); + } + + @Override + public BenchmarkStatus poll(BenchmarkHandle handle) { + return BenchmarkStatus.builder(stopped.contains(handle.taskId()) ? TaskStatus.FAILED : TaskStatus.RUNNING) + .build(); + } + + @Override + public void stop(BenchmarkHandle handle) { + if (!stopped.contains(handle.taskId())) { + stopped.add(handle.taskId()); + } + } + } +} diff --git a/kates/src/test/java/com/bmscomp/kates/engine/TestTimeoutReaperTest.java b/kates/src/test/java/com/bmscomp/kates/engine/TestTimeoutReaperTest.java index 6bad42d8..95d96a9f 100644 --- a/kates/src/test/java/com/bmscomp/kates/engine/TestTimeoutReaperTest.java +++ b/kates/src/test/java/com/bmscomp/kates/engine/TestTimeoutReaperTest.java @@ -34,6 +34,15 @@ *

The reaper reads each run's deadline from what is stored with it, so a * run still inside its own is left alone. TestOrchestratorTest.RunDeadlines * covers the deadline each kind of run gets. + * + *

It holds a PENDING or a STOPPING run to the same deadline. Nothing else + * ended either once the process that had it was gone, so a run whose + * submission died, or that a Kates API before 1.25.0 left STOPPING, stayed + * so for good, and the daily sweep, which deletes ended runs only, never took + * it. These runs have no handle or permit in this process, as a run another + * replica runs has none here: inside its deadline, each is left alone. + * TestTimeoutReaperSubmissionTest covers a PENDING run whose submission is + * under way here. */ @QuarkusTest class TestTimeoutReaperTest { @@ -128,19 +137,113 @@ void recoveryKeepsEveryTaskRowOfARunItFails() { assertEquals(42_000, task(stored, "-consume-0").getRecordsSent(), "what the task measured is kept"); } + @Test + void theReaperFailsAPendingRunPastItsDeadlineWithATaskThatSaysWhy() { + // Its submission never stored it RUNNING, as when the Kates API stops + // while it creates the run's topic: the run has no task. + TestRun run = storePending(Instant.now().minus(Duration.ofMinutes(20))); + + reaper.reapStuckTests(); + + TestRun stored = reread(run); + assertEquals(TaskStatus.FAILED, stored.getStatus()); + assertEquals(1, stored.getResults().size(), "one task carries the reason"); + TestResult reason = task(stored, "-submission"); + assertEquals(TaskStatus.FAILED, reason.getStatus()); + assertEquals(TestType.LOAD, reason.getTestType()); + assertNull(reason.getPhaseName(), "it is no phase of the run"); + assertEquals( + "Timeout: still pending 900000ms after it was created, its planned 600000ms plus a 300000ms grace;" + + " its tasks were never stored as started, as when the Kates API stops while it starts them", + reason.getError()); + assertNotNull(reason.getEndTime()); + } + + @Test + void theReaperLeavesAPendingRunInsideItsDeadlinePending() { + // Fourteen minutes into a ten-minute run, with no handle or permit in + // this process: it may be one another replica is still starting. + TestRun run = storePending(Instant.now().minus(Duration.ofMinutes(14))); + + reaper.reapStuckTests(); + + TestRun stored = reread(run); + assertEquals(TaskStatus.PENDING, stored.getStatus()); + assertTrue(stored.getResults().isEmpty()); + } + + @Test + void theReaperFailsAStoppingRunPastItsDeadline() { + // A Kates API before 1.25.0 stored the run STOPPING on the way to + // ending it, and stopped before it did. + TestRun run = storeRun(TaskStatus.STOPPING, Instant.now().minus(Duration.ofMinutes(20))); + + reaper.reapStuckTests(); + + TestRun stored = reread(run); + assertEquals(TaskStatus.FAILED, stored.getStatus()); + assertEquals(3, taskRows(run.getId()), "no task row is deleted, and none is added"); + TestResult produced = task(stored, "-produce-0"); + assertEquals(TaskStatus.DONE, produced.getStatus(), "a task that finished is left as it was"); + assertNull(produced.getError()); + for (String suffix : List.of("-consume-0", "-verify-0")) { + TestResult unfinished = task(stored, suffix); + assertEquals(TaskStatus.FAILED, unfinished.getStatus(), suffix); + assertEquals( + "Timeout: still stopping 900000ms after it was created, its planned 600000ms plus a 300000ms" + + " grace; only a Kates API before 1.25.0 stores a run STOPPING, and nothing ended this" + + " one", + unfinished.getError(), + suffix); + assertNotNull(unfinished.getEndTime(), suffix); + } + assertEquals(42_000, task(stored, "-consume-0").getRecordsSent(), "what the task measured is kept"); + } + + @Test + void theReaperLeavesAStoppingRunInsideItsDeadlineStopping() { + TestRun run = storeRun(TaskStatus.STOPPING, Instant.now().minus(Duration.ofMinutes(14))); + + reaper.reapStuckTests(); + + TestRun stored = reread(run); + assertEquals(TaskStatus.STOPPING, stored.getStatus()); + assertEquals(TaskStatus.RUNNING, task(stored, "-consume-0").getStatus()); + assertNull(task(stored, "-consume-0").getError()); + } + /** - * A ten-minute RUNNING LOAD run as the orchestrator stores it once its - * tasks are under way: a producer that has finished, a consumer still - * reading, and a task that has not started. + * A ten-minute LOAD run as executeTest stores it before its submission: + * PENDING, with no task yet. */ + private TestRun storePending(Instant createdAt) { + TestSpec spec = new TestSpec(); + spec.setDurationMs(600_000); + TestRun run = new TestRun(TestType.LOAD, spec) + .withBackend("native") + .withCreatedAt(createdAt.toString()) + .withPlannedDurationMs(600_000L); + repository.save(run); + return run; + } + private TestRun storeRunning(Instant createdAt) { + return storeRun(TaskStatus.RUNNING, createdAt); + } + + /** + * A ten-minute LOAD run in this status, with the tasks the orchestrator + * stores once they are under way: a producer that has finished, a consumer + * still reading, and a task that has not started. + */ + private TestRun storeRun(TaskStatus status, Instant createdAt) { TestSpec spec = new TestSpec(); spec.setDurationMs(600_000); TestRun run = new TestRun(TestType.LOAD, spec) .withBackend("native") .withCreatedAt(createdAt.toString()) .withPlannedDurationMs(600_000L) - .withStatus(TaskStatus.RUNNING); + .withStatus(status); String started = createdAt.plusSeconds(2).toString(); run = run.withResults(List.of( new TestResult() From c667a823b219285d503ae6a8b840e1814f1f0ea7 Mon Sep 17 00:00:00 2001 From: Said Boudjelda Date: Thu, 8 Oct 2026 12:08:01 +0200 Subject: [PATCH 6/6] docs(book): book 12's sweep paragraph names DELETE /api/tests/{id} in code, not as a link Markdown Link Check failed on 12-deployment.md:80 with "Cannot find fragment" for 11-api-reference.md#delete-apitestsid. lychee reads the trailing {id} of "#### DELETE /api/tests/{id}" as a Pandoc attribute block, so it gives that heading no such anchor, although Quarto renders one. The link came with #353's sweep paragraph, and #353 fails the same check. Other chapters name such endpoints in code instead. --- docs/book/12-deployment.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/book/12-deployment.md b/docs/book/12-deployment.md index 47a671d4..883115e5 100644 --- a/docs/book/12-deployment.md +++ b/docs/book/12-deployment.md @@ -77,7 +77,7 @@ The default deployment uses **NodePort** for all services (Grafana on 30080, Kaf The Kates API keeps every run in PostgreSQL. The `kates` chart runs its own PostgreSQL on a 1 GiB PersistentVolumeClaim on the cluster's default StorageClass, so runs survive pod restarts; there is no emptyDir mode. For production, point the chart at an external database instead, with `postgresql.enabled: false` and `externalDatabase`, as `values-prod.yaml` does. [Architecture & Design](02-architecture.md) describes what else the Kates API keeps. -Whichever database holds them, each replica of the Kates API sweeps out old runs when it starts and every 24 hours after. A sweep deletes the `DONE` and `FAILED` runs created more than `kates.cleanup.retention-days` days ago, 90 by default, so a lower setting deletes as soon as a replica starts with it. It deletes each run as [DELETE /api/tests/{id}](11-api-reference.md#delete-apitestsid) does, with its results, oldest first and 1,000 at a time. Each leaves a row in the audit log naming `system:scheduler`: action `DELETE`, type `test`, the run's ID, and the details `retention sweep: created before (kates.cleanup.retention-days=)`. When two replicas find the same run at once, it is deleted, and audited, once. An error ends that sweep with an `ERROR` line in the Kates API's log, and the next sweep goes on from there. +Whichever database holds them, each replica of the Kates API sweeps out old runs when it starts and every 24 hours after. A sweep deletes the `DONE` and `FAILED` runs created more than `kates.cleanup.retention-days` days ago, 90 by default, so a lower setting deletes as soon as a replica starts with it. It deletes each run as `DELETE /api/tests/{id}` does, with its results, oldest first and 1,000 at a time. Each leaves a row in the audit log naming `system:scheduler`: action `DELETE`, type `test`, the run's ID, and the details `retention sweep: created before (kates.cleanup.retention-days=)`. When two replicas find the same run at once, it is deleted, and audited, once. An error ends that sweep with an `ERROR` line in the Kates API's log, and the next sweep goes on from there. A sweep leaves a run still `PENDING`, `RUNNING` or `STOPPING` alone, however old it is. The Kates API fails such a run five minutes after the time it was set to last, as [Test Management](11-api-reference.md#test-management) describes, and a sweep deletes it once it is older than the retention.