Skip to content

Simplfy how requestor is set in scheduler - #5128

Open
JamesMurkin wants to merge 1 commit into
masterfrom
simplify_requestor_in_scheduler
Open

Simplfy how requestor is set in scheduler#5128
JamesMurkin wants to merge 1 commit into
masterfrom
simplify_requestor_in_scheduler

Conversation

@JamesMurkin

Copy link
Copy Markdown
Contributor
  • Don't set UserId on sequence generated by the scheduler, as this can have the wrong user in the case multiple events happen at once that could set the user
    • Probably we should make all sequences generated by scheduler have user id "armada-scheduler" or just left blank as now
  • Use Deref as it is neater

 - Don't set UserId on sequence generated by the scheduler, as this can have the wrong user in the case multiple events happen at once that could set the user
   - Probably we should make all sequences generated by scheduler have user id "armada-scheduler" or just left blank as now
 - Use Deref as it is neater

Signed-off-by: JamesMurkin <jamesmurkin@hotmail.com>
@JamesMurkin
JamesMurkin marked this pull request as ready for review August 21, 2026 15:48
@greptile-apps

greptile-apps Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR moves requester attribution from scheduler-generated sequence metadata to individual reprioritisation, cancellation, and preemption events, and replaces manual pointer handling with ptr.Deref. The sequence-level removal overlooks CancelJob, which has no event-level requester field.

  • Uses ptr.Deref for optional reprioritisation, cancellation, and preemption users.
  • Removes the accumulated EventSequence.UserId from scheduler update messages.
  • Preserves requesters on event types that expose dedicated Requestor fields.

Confidence Score: 4/5

The requester loss for cancel-by-job-set events should be fixed before merging.

CancelJob lacks an event-level requester, so removing EventSequence.UserId causes downstream persisted and public cancellation attribution to become blank and breaks existing scheduler expectations.

Files Needing Attention: internal/scheduler/scheduler.go

Important Files Changed

Filename Overview
internal/scheduler/scheduler.go Simplifies optional requester dereferences and removes sequence-level attribution, but loses requester data for the cancel-by-job-set CancelJob path.

Reviews (1): Last reviewed commit: "Simplfy how requestor is set in schedule..." | Re-trigger Greptile

Comment on lines 1496 to 1499
return &armadaevents.EventSequence{
Queue: job.Queue(),
JobSetName: job.Jobset(),
UserId: sequenceUserId,
Events: events,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 CancelJob requester attribution is lost

When a job has a cancel-by-job-set request, the scheduler emits a CancelJob without an event-level requester and now leaves EventSequence.UserId empty, causing downstream ingestion and public event conversion to record a blank cancellation requester.

Knowledge Base Used: Event processing and projections

@datadog-armadaproject

datadog-armadaproject Bot commented Aug 21, 2026

Copy link
Copy Markdown

Pipelines

⚠️ Warnings

🚦 2 Pipeline jobs failed

CI | All jobs succeeded

View in Datadog · View in GitHub Actions

CI | test / Golang Unit Tests

View in Datadog · View in GitHub Actions

8 failed tests including unexpected empty requestor on job state and assertion failures related to event sequences.

Useful? React with 👍 / 👎

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 02eb3ea | Docs | View more details | Give us feedback!

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