Include all priority classes option for cancellation and preemption

[nikola-jokic]

What type of PR is this?

Enhancement

What this PR does / why we need it

Allow users to preempt/cancel jobs on all priority classes

[dejanzele]

I am thinking should we extract all-priority-classes into a const as it is repeated 30 times

[dejanzele]

Actually, the block from L77-L84 is repeated, might be worthwhile to extract it into a function and reuse in 12 places

[greptile-apps]

Greptile Summary

This PR enhances the armadactl CLI by adding an --all-priority-classes (-a) boolean flag to all cancel and preempt subcommands (executor, node, queue), allowing users to target jobs across every priority class without enumerating them explicitly. When the flag is set, priority-classes is no longer required, and the scheduler DB layer is updated to skip priority-class filtering when the list is empty.

Key changes:

• cmd/armadactl/cmd/cancel.go / preempt.go: New --all-priority-classes flag added to all six cancel/preempt subcommands; PreRunE logic conditionally makes priority-classes required only when the flag is absent.
• internal/armadactl/cancel.go: All three cancel functions updated to log "all" for priority classes when the slice is empty.
• internal/armadactl/preempt.go: PreemptOnExecutor updated to log "all", but PreemptOnNode and PreemptOnQueues are missing the same update — log output will show an empty string instead of "all" for those two code paths.
• internal/scheduleringester/schedulerdb.go: Priority-class filtering is now conditional (len > 0 guard), so an empty slice correctly passes all jobs through.
• Tests added for all new flag combinations across both commands and all subcommand types.
• The --all-priority-classes flag description in all three cancel subcommands incorrectly reads "Preempt jobs…" (copy-paste from preempt commands) rather than "Cancel jobs…".

Confidence Score: 4/5

• Safe to merge with minor fixes — no functional regressions, only cosmetic/logging inconsistencies.
• The core logic change (conditional priority-class filtering in the scheduler DB) is correct and well-tested. The CLI flag wiring is sound. Issues are limited to (1) wrong verb in help-text descriptions for cancel commands, (2) missing priorityClassesMsg = "all" in two preempt functions causing misleading log output, and (3) an unenforced "Cannot be used with" constraint in one flag description. None of these affect actual job cancellation or preemption behaviour.
• internal/armadactl/preempt.go (missing log message update for PreemptOnNode and PreemptOnQueues) and cmd/armadactl/cmd/cancel.go (wrong verb in flag descriptions).

Important Files Changed

Filename	Overview
cmd/armadactl/cmd/cancel.go	Adds --all-priority-classes / -a bool flag to all three cancel subcommands; makes priority-classes optional when --all-priority-classes is set. Flag descriptions for the new flag incorrectly say "Preempt" instead of "Cancel".
cmd/armadactl/cmd/preempt.go	Adds --all-priority-classes / -a bool flag to all four preempt subcommands; logic is correct. preemptQueuesCmd description claims the flag cannot be used with --priority-classes but there is no enforcement.
internal/armadactl/cancel.go	All three cancel functions (CancelOnExecutor, CancelOnNode, CancelOnQueues) correctly set priorityClassesMsg = "all" when priorityClasses is empty, for accurate log output.
internal/armadactl/preempt.go	PreemptOnExecutor correctly sets priorityClassesMsg = "all" when empty, but PreemptOnNode and PreemptOnQueues are missing this update, causing the log message to display an empty string instead of "all" when all priority classes are targeted.
internal/scheduleringester/schedulerdb.go	Replaces the always-applied jobInPriorityClasses filter with a conditional filterJobsByPriorityClasses call; when PriorityClasses is empty, all jobs pass through. Change is correct and consistent across all four cases (cancel/preempt × executor/queue).
internal/scheduleringester/schedulerdb_test.go	Adds six new integration test cases confirming that empty PriorityClasses matches all jobs for each combination of executor/node/queue × cancel/preempt; coverage is comprehensive.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[User runs cancel/preempt command] --> B{--all-priority-classes set?}
    B -- Yes --> C[priorityClasses = nil]
    B -- No --> D{--priority-classes provided?}
    D -- No --> E[Error: priority-classes required]
    D -- Yes --> F[priorityClasses = provided values]
    C --> G[Call App.CancelOn* / PreemptOn*]
    F --> G
    G --> H{len priorityClasses > 0?}
    H -- No --> I[Log: priority-classes = 'all'\nSkip DB filter → all jobs selected]
    H -- Yes --> J[Log: priority-classes = values\nfilterJobsByPriorityClasses]
    I --> K[MarkJobsCancelRequested / MarkJobRunsPreemptRequested]
    J --> K

Loading

Comments Outside Diff (1)

1. internal/armadactl/preempt.go, line 67-84 (link)

   Missing "all" log message for empty priority classes in PreemptOnNode and PreemptOnQueues

   PreemptOnExecutor was updated to set priorityClassesMsg = "all" when priorityClasses is empty, but PreemptOnNode and PreemptOnQueues were not updated consistently. When a user passes --all-priority-classes, the log output will print an empty string for priority classes instead of "all", while PreemptOnExecutor and all three cancel variants correctly print "all".

   For PreemptOnNode (line 68), add after priorityClassesMsg := strings.Join(priorityClasses, ","):

   if len(priorityClasses) == 0 {
       priorityClassesMsg = "all"
   }

   For PreemptOnQueues (line 93), add after priorityClassesMsg := strings.Join(priorityClasses, ","):

   if len(priorityClasses) == 0 {
       priorityClassesMsg = "all"
   }

_{Reviews (1): Last reviewed commit: "Merge branch 'master' into nikola-jokic/..." | Re-trigger Greptile}

[greptile-apps]

Wrong verb in flag description for cancel commands

The --all-priority-classes flag description says "Preempt jobs on executor for all priority classes." but this is a cancel command — the description was copied from the preempt command. This appears in all three cancel subcommands: cancelExecutorCmd (here), cancelNodeCmd (line ~195), and cancelQueueCmd (in the queue block). The description should say "Cancel" rather than "Preempt", and the mention of "executor" is also wrong for cancelNodeCmd and cancelQueueCmd.

Suggested change

	"Preempt jobs on executor for all priority classes.",
	"Cancel jobs on executor for all priority classes.",

[greptile-apps]

Flag description claims mutual exclusion that is not enforced

The description says "Cannot be used with the priority-classes flag." but there is no enforcement of this constraint in PreRunE or RunE. If a user provides both --all-priority-classes and --priority-classes, all-priority-classes silently wins (sets priorityClasses = nil). Either add a validation check like:

if allPriorityClasses && len(priorityClasses) > 0 {
    return fmt.Errorf("--all-priority-classes and --priority-classes are mutually exclusive")
}

Or update the description to reflect the actual behaviour (e.g., "When set, overrides the --priority-classes flag and targets all priority classes.").