Add retry policy client package and armadactl commands - #5048
Merged
dejanzele merged 1 commit intoAug 2, 2026
Conversation
This was referenced Jul 24, 2026
dejanzele
force-pushed
the
retry-policy-client-armadactl
branch
from
July 24, 2026 20:17
12e8aec to
9d97762
Compare
Contributor
Greptile SummaryAdds retry-policy client and armadactl CRUD commands, strict policy-file decoding, and ordered retry-policy attachments for queue create/update operations. Confidence Score: 5/5The PR appears safe to merge. No blocking failures remain within the scope of the previous review threads.
|
| Filename | Overview |
|---|---|
| cmd/armadactl/cmd/queue.go | Adds ordered retry-policy string-slice flags to queue creation and full-replacement updates. |
| cmd/armadactl/cmd/retrypolicy.go | Registers file-based retry-policy create/update commands and name-based get/delete commands. |
| internal/armadactl/retrypolicy.go | Implements strict policy-file parsing, API delegation, and reusable YAML output. |
| pkg/client/retrypolicy/create.go | Adds the retry-policy creation gRPC client wrapper. |
| pkg/client/retrypolicy/delete.go | Adds the retry-policy deletion gRPC client wrapper. |
| pkg/client/retrypolicy/get.go | Adds single-policy and policy-list gRPC client wrappers. |
| pkg/client/retrypolicy/update.go | Adds the retry-policy update gRPC client wrapper. |
Reviews (13): Last reviewed commit: "Add retry policy client package and arma..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
dejanzele
force-pushed
the
retry-policy-client-armadactl
branch
from
July 26, 2026 21:43
9d97762 to
7ba9401
Compare
Member
Author
dejanzele
force-pushed
the
retry-policy-client-armadactl
branch
3 times, most recently
from
July 29, 2026 12:47
1af01c7 to
fecb089
Compare
dejanzele
force-pushed
the
retry-policy-client-armadactl
branch
from
July 30, 2026 15:37
fecb089 to
b58a8f1
Compare
dejanzele
added a commit
that referenced
this pull request
Jul 30, 2026
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. Signed-off-by: Dejan Zele Pejchev <pejcev.dejan@gmail.com>
dejanzele
force-pushed
the
retry-policy-client-armadactl
branch
5 times, most recently
from
July 31, 2026 10:34
83df9fe to
55a34eb
Compare
Signed-off-by: Dejan Zele Pejchev <pejcev.dejan@gmail.com>
dejanzele
force-pushed
the
retry-policy-client-armadactl
branch
from
July 31, 2026 11:10
55a34eb to
6eae508
Compare
williamvega
approved these changes
Jul 31, 2026
Contributor
|
Tick the box to add this pull request to the merge queue (same as
|
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
pkg/client/retrypolicyand armadactl commands for managing retry policies:armadactl create/get/update/delete retry-policy, plus a--retry-policiesflag onarmadactl create queueandarmadactl update queuefor attaching policies to a queue, in evaluation order. Since queue update is a full replace, the flag has to be passed on every update or the attachment is cleared. The update help text spells this out.Policy files are decoded strictly and must carry
apiVersionandkind, so the same file works with eitherarmadactl create -forarmadactl create retry-policy -f. Strict decoding matters here because the lenient path silently drops a mistyped field, and a droppedretryLimitleaves it at zero, which the API defines as never retrying. Requiring the kind stops a queue definition being accepted as a policy on the strength of it having a name. The YAML thatgetprints is valid input tocreate.