Skip to content

Add RetryPolicy CRUD service and queue attachment validation - #5049

Merged
dejanzele merged 1 commit into
armadaproject:masterfrom
dejanzele:retry-policy-api
Jul 30, 2026
Merged

Add RetryPolicy CRUD service and queue attachment validation#5049
dejanzele merged 1 commit into
armadaproject:masterfrom
dejanzele:retry-policy-api

Conversation

@dejanzele

@dejanzele dejanzele commented Jul 24, 2026

Copy link
Copy Markdown
Member

Implements the RetryPolicyService whose proto was defined in #4998: create, get, list, update, and delete for retry policies, backed by a Postgres repository. Adds create_retry_policy, update_retry_policy, and delete_retry_policy permissions and maps them in the local development server configs. Creating a policy that already exists replaces it, the same as create queue, so a pipeline that creates policies can be re-run. Updating one that does not exist still returns NotFound.

Queue attachments are stored relationally rather than inside the serialized queue definition. Migration 035 adds the retry_policy table together with a queue_retry_policy join table, where each row records the position its policy held in the submitted list so that a queue reads its policies back in the order they were given. The queue repository rewrites those rows on every queue write and composes RetryPolicies from them on every read, so the serialized definition never carries the names.

A foreign key on policy_name is what prevents a queue referencing a policy that does not exist. A queue write also looks the names up first, so the error can say which ones are missing, returned as InvalidArgument. In the narrow case where a policy is deleted between that lookup and the write, the foreign key rejects the write with a less specific message. Deleting a policy removes its attachment rows and then the policy itself, returning the queues it detached for the service to log. The migration names that constraint explicitly because the repository matches on it.

Both tables are queried through sqlc, generated into internal/server/queryapi/database, matching the pattern the scheduler already uses. The queue table's own statements are unchanged.

Independent of #5047 and #5048, any merge order works. Two notes on the seams: the RetryPolicies field on the client queue type also appears in #5048 as an identical hunk, since both PRs need it to compile on their own, and it merges cleanly from either direction. The retry policy service tests define their own gRPC status helper, which can switch to the shared one from #5047 once both are in.

Replaces #5000, which carried all of this plus the client and armadactl work in one PR.

@greptile-apps

greptile-apps Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Adds a Postgres-backed retry-policy CRUD service and relational queue-policy attachments.

  • Registers the retry-policy gRPC and REST services with create, update, and delete authorization.
  • Adds retry-policy and ordered queue-attachment tables, generated sqlc queries, repositories, validation, and tests.
  • Validates queue attachments and translates unknown policy references into InvalidArgument.

Confidence Score: 5/5

The PR appears safe to merge because no additional blocking failure eligible for this follow-up review remains.

No blocking failure remains beyond issues already covered by previous review threads.

Important Files Changed

Filename Overview
internal/server/retrypolicy/repository.go Implements transactional Postgres CRUD for serialized retry policies and attachment cleanup on deletion.
internal/server/retrypolicy/service.go Implements authorized and validated retry-policy CRUD RPC behavior with gRPC status translation.
internal/server/queue/queue_repository.go Persists ordered retry-policy attachments relationally and reconstructs them on queue reads.
internal/lookout/schema/migrations/035_create_retry_policy.sql Adds retry-policy storage and the ordered queue-policy join table with cascading foreign keys.
internal/server/server.go Wires the retry-policy repository and service into the API server.
cmd/server/main.go Registers the retry-policy REST gateway handler.
_local/server/config.yaml Maps retry-policy mutation permissions for the local unauthenticated configuration.
_local/server/config-auth.yaml Maps retry-policy mutation permissions to local administrators.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Client[Client] --> Service[RetryPolicy / Queue services]
  Service --> Repository[Postgres repositories]
  Repository --> Policy[(retry_policy)]
  Repository --> Attachment[(queue_retry_policy)]
  Attachment --> Queue[(queue)]
  Attachment --> Policy
Loading

Reviews (31): Last reviewed commit: "Add RetryPolicy CRUD service and queue a..." | Re-trigger Greptile

Comment thread internal/server/retrypolicy/service.go Outdated
Comment thread internal/server/permissions/permissions.go
Comment thread internal/server/retrypolicy/repository.go Outdated
@dejanzele
dejanzele force-pushed the retry-policy-api branch 2 times, most recently from 153861c to e500da4 Compare July 27, 2026 13:25
@datadog-armadaproject

Copy link
Copy Markdown

Pipelines

⚠️ Warnings

🚦 1 Pipeline job failed

CI | test / SQLC Generated Files Up To Date   View in Datadog   GitHub Actions

Useful? React with 👍 / 👎

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: e500da4 | Docs | Datadog PR Page | Give us feedback!

Comment thread internal/server/queue/queue_repository.go Outdated
@dejanzele

Copy link
Copy Markdown
Member Author

@greptileai

@dejanzele
dejanzele force-pushed the retry-policy-api branch 3 times, most recently from 5101c32 to f540179 Compare July 28, 2026 12:11
Comment thread internal/server/queue/queue_repository.go Outdated
Comment thread internal/server/retrypolicy/repository.go
@dejanzele
dejanzele force-pushed the retry-policy-api branch 5 times, most recently from 7426f76 to 260fcbe Compare July 29, 2026 10:02
Comment thread internal/server/retrypolicy/repository.go
@dejanzele
dejanzele force-pushed the retry-policy-api branch 2 times, most recently from ca76f62 to d3dd663 Compare July 29, 2026 11:00
@dejanzele

Copy link
Copy Markdown
Member Author

@greptileai please analyze now with the new database approach

Comment thread internal/server/queue/queue_repository.go
@dejanzele

Copy link
Copy Markdown
Member Author

@greptileai

@dejanzele
dejanzele force-pushed the retry-policy-api branch 2 times, most recently from 0ce1f01 to 83e8737 Compare July 29, 2026 12:54
Comment thread internal/server/queue/queue_repository.go
@dejanzele
dejanzele force-pushed the retry-policy-api branch 8 times, most recently from b7f6894 to d235df5 Compare July 30, 2026 10:44
Comment thread internal/server/retrypolicy/repository.go
@dejanzele

Copy link
Copy Markdown
Member Author

@greptileai

@mergify

mergify Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

Signed-off-by: Dejan Zele Pejchev <pejcev.dejan@gmail.com>
@dejanzele
dejanzele merged commit 0c97541 into armadaproject:master Jul 30, 2026
18 checks passed
dejanzele added a commit that referenced this pull request Aug 5, 2026
…#5001)

## Problem

A failed run is retried today only when a failed pod check matches it.
The checks answer yes or no, and a retry returns the lease silently: no
failure category on the event, no per-queue control, no budget per
failure kind, and no way to change the job before it runs again.
Operators cannot express "retry OOM kills with more memory" or "give
this queue three retries for infrastructure failures, away from the node
that failed".

## Earlier work this builds on

- #4998 added the retry engine and the RetryPolicy API resource.
- #4999 made the executor delete a failed pod in a Delete-action
category and report the failure only after the pod is gone, so a retry
can reuse the pod name.
- #5049 and #5048 added the RetryPolicy CRUD service, client, and
armadactl commands.
- #5080 and #5081 made pod-level failure messages and Kubernetes events
matchable by category rules.
- #5012 added the mutate block on retry rules and the pipeline that
delivers memory growth to the pod.

## This PR

This PR connects those pieces into scheduler behaviour, behind the
scheduling.retryPolicy feature flag. With the flag off, the scheduler
produces the same event sequences as before, byte for byte, and identity
tests pin this. With the flag on, a failed run consults the policy
attached to its queue. The policy decides whether the categorized
failure retries, and its verdict replaces the attempt counting for those
runs. A retryable failure emits an intermediate event with
retryable=true. The wiring also applies the matched rule's mutations at
requeue: avoidSameNode steers the retry away from every node the job
failed on, and a memory bump grows the job so placement, accounting, and
the retried pod all see the new size. Gangs and fail-fast jobs keep
their existing behaviour.

One operator requirement: avoidSameNode expresses the avoidance through
the scheduler's nodeIdLabel, so that label must be in the executor's
trackedNodeLabels.

Two small event-stream changes apply with the flag off as well, both
additive. A terminal lease expiry now emits a JobFailedEvent with the
reason "Lease expired" instead of an empty reason. Failure category
fields now appear on all failure reason kinds, not only pod errors.
Consumers that match on empty reason strings will see the new values.

## Validating the full loop

The OOM-with-memory-bump loop is the best single check. Enable the flag
and give a category for OOMKilled conditions action Delete, then:

```yaml
# armadactl create retry-policy -f policy.yaml
apiVersion: armadaproject.io/v1beta1
kind: RetryPolicy
name: oom-grow
retryLimit: 2
defaultAction: Fail
rules:
  - action: Retry
    onCategory: oom
    mutate:
      resources:
        memory:
          factor: 2
# armadactl create queue oom-grow-queue --retry-policies oom-grow
```

Submit a job with a 64Mi memory limit that writes 100Mi into a
memory-backed emptyDir. Expect: the run fails with category oom and a
retryable=true event, the pod disappears before that event arrives, and
the same pod name comes back with a 128Mi limit and succeeds. Lookout
shows two runs: one failed with the category, one succeeded. This
sequence was validated on a live cluster, together with avoidSameNode
relocating a retry to a second node and failing the job cleanly once
every node had been tried.

---------

Signed-off-by: Dejan Zele Pejchev <pejcev.dejan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants