Skip to content

Add RetryPolicy CRUD service and queue attachment validation - #5000

Closed
dejanzele wants to merge 2 commits into
armadaproject:masterfrom
dejanzele:retry-policy-api
Closed

Add RetryPolicy CRUD service and queue attachment validation#5000
dejanzele wants to merge 2 commits into
armadaproject:masterfrom
dejanzele:retry-policy-api

Conversation

@dejanzele

@dejanzele dejanzele commented Jul 8, 2026

Copy link
Copy Markdown
Member

Implements the RetryPolicyService whose proto was defined in #4998: create, get, update, and delete for retry policies, backed by a Postgres repository and a lookout schema migration for the retry_policy table. Referential integrity is enforced in both directions. Deleting a policy is rejected while any queue still references it, and creating or updating a queue is rejected if it references a policy that does not exist. Adds create_retry_policy, update_retry_policy, and delete_retry_policy permissions.

Stacked on #5047, which moves a shared gRPC test helper into servertest. Its commit shows in this diff until it merges. The client package and armadactl commands are split into #5048.

@datadog-armadaproject

Copy link
Copy Markdown

Pipelines

⚠️ Warnings

🚦 1 Pipeline job failed

CI | test / Proto Up To Date   View in Datadog   GitHub Actions

Useful? React with 👍 / 👎

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

@greptile-apps

greptile-apps Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR implements the RetryPolicyService (CRUD) defined in #4998, backed by a new retry_policy Postgres table (migration 035) and a PostgresRetryPolicyRepository. It also adds validateRetryPolicy guards on CreateQueue / UpdateQueue so a queue cannot reference a non-existent policy, and consolidates the previously triply-duplicated requireGrpcCode test helper into the shared servertest package.

  • New retrypolicy package: repository.go (Postgres CRUD + RetryPolicyExists), service.go (gRPC handlers with auth), validation.go (RFC-1123 name + rule checks), all covered by unit and integration tests.
  • Queue service changes: RetryPolicyExistenceChecker interface injected into queue.Server; validateRetryPolicy called before the repository write on both create and update paths.
  • pkg/client/queue: Queue.RetryPolicies []string added with correct NewQueue / ToAPI round-trip.

Confidence Score: 5/5

Safe to merge; the new service is well-structured, auth-gated, and has solid unit and integration test coverage.

The implementation follows established patterns in the codebase, the migration is consistent with existing schema, auth checks are in place for all mutating operations, and read paths are intentionally unauthenticated in line with GetQueue. The two findings are minor efficiency/consistency nits that do not affect correctness or security.

internal/server/retrypolicy/repository.go — nil vs empty slice on GetAllRetryPolicies and the full-fetch in RetryPolicyExists are worth a second look before broader rollout.

Important Files Changed

Filename Overview
internal/server/retrypolicy/repository.go New Postgres-backed CRUD repository for retry policies; mirrors the queue repository pattern but GetAllRetryPolicies returns nil (not empty slice) and RetryPolicyExists fetches the full definition rather than using a lightweight existence query.
internal/server/retrypolicy/service.go New gRPC service wiring CRUD operations with auth and queue-reference checks; the acknowledged TOCTOU race on delete and the post-auth empty-name check are noted in prior threads.
internal/server/retrypolicy/validation.go Clean RFC-1123 name validation and rule structural checks; well-covered by validation_test.go.
internal/server/queue/queue_service.go Adds validateRetryPolicy guard on CreateQueue and UpdateQueue; wires the new RetryPolicyExistenceChecker interface correctly.
internal/lookout/schema/migrations/035_create_retry_policy.sql New retry_policy table that exactly mirrors the existing queue table schema; format is consistent with all other migrations in this directory.
internal/server/server.go Correctly wires retryPolicyRepo as both the RetryPolicyExistenceChecker for the queue server and the repository for the new retryPolicyServer; queueRepository satisfies QueueLister via GetAllQueues.
internal/server/servertest/grpc.go Consolidates the previously duplicated requireGrpcCode helper into a shared package; uses require.Equal instead of assert.Equal for stricter test failure behaviour.
internal/server/queue/queue_service_test.go Adds tests for unknown-policy rejection and multi-policy acceptance; PriorityFactor: 1 is correctly set to avoid early-exit from NewQueue; UpdateQueue unknown-policy path remains untested (noted in prior threads).
pkg/client/queue/queue.go Adds RetryPolicies []string field to the Queue struct with correct round-trip through NewQueue and ToAPI.

Sequence Diagram

sequenceDiagram
    participant Client
    participant QueueServer
    participant RetryPolicyServer
    participant PostgresRetryPolicyRepo
    participant PostgresQueueRepo

    Note over Client,PostgresQueueRepo: CreateQueue / UpdateQueue flow
    Client->>QueueServer: CreateQueue(req)
    QueueServer->>QueueServer: authorize(CreateQueue)
    QueueServer->>QueueServer: queue.NewQueue(req)
    loop for each RetryPolicy name
        QueueServer->>PostgresRetryPolicyRepo: RetryPolicyExists(name)
        PostgresRetryPolicyRepo-->>QueueServer: (bool, err)
    end
    QueueServer->>PostgresQueueRepo: CreateQueue(queue)
    PostgresQueueRepo-->>QueueServer: ok
    QueueServer-->>Client: "Empty{}"

    Note over Client,PostgresQueueRepo: DeleteRetryPolicy flow
    Client->>RetryPolicyServer: DeleteRetryPolicy(req)
    RetryPolicyServer->>RetryPolicyServer: authorize(DeleteRetryPolicy)
    RetryPolicyServer->>PostgresQueueRepo: GetAllQueues()
    PostgresQueueRepo-->>RetryPolicyServer: []Queue
    RetryPolicyServer->>RetryPolicyServer: check referencing queues
    alt queues still reference policy
        RetryPolicyServer-->>Client: FailedPrecondition
    else no references
        RetryPolicyServer->>PostgresRetryPolicyRepo: DeleteRetryPolicy(name)
        PostgresRetryPolicyRepo-->>RetryPolicyServer: ok
        RetryPolicyServer-->>Client: "Empty{}"
    end
Loading

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

Comment thread internal/armadactl/queue.go Outdated
Comment thread internal/server/retrypolicy/service.go
Comment thread internal/server/retrypolicy/service.go
@dejanzele
dejanzele force-pushed the retry-policy-api branch 5 times, most recently from f796a82 to 77bdd4d Compare July 9, 2026 15:17
Comment thread internal/armadactl/queue.go Outdated
@dejanzele
dejanzele force-pushed the retry-policy-api branch 2 times, most recently from 37e060a to 3b1f6f2 Compare July 9, 2026 16:12
@dejanzele

Copy link
Copy Markdown
Member Author

@greptileai

@dejanzele
dejanzele force-pushed the retry-policy-api branch 4 times, most recently from b1b9ef3 to 85bf359 Compare July 10, 2026 13:16
@dejanzele

Copy link
Copy Markdown
Member Author

@greptileai

@dejanzele
dejanzele force-pushed the retry-policy-api branch 8 times, most recently from dc56181 to 93322e2 Compare July 13, 2026 12:48
@dejanzele

Copy link
Copy Markdown
Member Author

@greptileai

Comment thread internal/server/queue/queue_service_test.go Outdated
@dejanzele
dejanzele force-pushed the retry-policy-api branch 2 times, most recently from f5fefac to 7bfc8f3 Compare July 17, 2026 09:55
@dejanzele
dejanzele force-pushed the retry-policy-api branch 12 times, most recently from 8b63ef2 to 85bef9c Compare July 23, 2026 12:56
Signed-off-by: Dejan Zele Pejchev <pejcev.dejan@gmail.com>
Signed-off-by: Dejan Zele Pejchev <pejcev.dejan@gmail.com>
@dejanzele dejanzele changed the title Add RetryPolicy CRUD service, client, and armadactl support Add RetryPolicy CRUD service and queue attachment validation Jul 24, 2026
@dejanzele

dejanzele commented Jul 24, 2026

Copy link
Copy Markdown
Member Author

Closing in favour of the split: #5047 (shared gRPC test helper), #5049 (server-side service), and #5048 (client package and armadactl commands).

@dejanzele dejanzele closed this Jul 24, 2026
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>
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.

1 participant