Skip to content

fix(spurctld): reject zero partition default time - #764

Open
jamesETsmith wants to merge 2 commits into
ROCm:mainfrom
jamesETsmith:jamesETsmith/spurctld-partition-defaulttime-0-gives-an-untime
Open

fix(spurctld): reject zero partition default time#764
jamesETsmith wants to merge 2 commits into
ROCm:mainfrom
jamesETsmith:jamesETsmith/spurctld-partition-defaulttime-0-gives-an-untime

Conversation

@jamesETsmith

Copy link
Copy Markdown
Contributor

Motivation

Prevent jobs submitted without an explicit time limit from receiving a zero-length limit when their partition configures DefaultTime=0 . This matches Slurm behavior by rejecting the submission instead of accepting a job that may be terminated immediately.

Fixes #759.

Technical Details

• Changed partition time-limit defaulting to return a submission error when any selected partition explicitly defines DefaultTime=0 .
• Preserved existing behavior for positive defaults, unset defaults, cluster fallbacks, multi-partition requests, and jobs with an explicit time limit.
• Added regression coverage for both the defaulting helper and the complete job submission path.

Test Plan

• Run focused unit tests for partition time-limit defaulting.
• Run the submission-path regression test for DefaultTime=0 .
• Verify Rust formatting.

Test Result

• cargo test -p spurctld apply_default_time_limit : 11 passed.
• cargo test -p spurctld submit_rejects_zero_partition_default_time_when_time_is_unset : 1 passed.
• cargo fmt --all -- --check : passed.

Submission Checklist

[X] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.
md#pull-requests.

yansun1996
yansun1996 previously approved these changes Aug 28, 2026

@yansun1996 yansun1996 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice, focused fix — this correctly matches Slurm's _valid_job_part semantics (reject on any requested partition having an explicit DefaultTime=0, distinct from the unset/None case and the cluster-fallback-0 sentinel). Traced the call site and confirmed the new fallible path runs before job-ID allocation and the WAL/Raft append, so a rejection here is cheap and side-effect-free. Both new tests exercise the real production entry points and would fail against the pre-fix code.

Two things to handle before merge:

  1. This PR currently has a merge conflict with main — could you rebase and resolve it?
  2. While you're in there, would it be worth adding a test case where a job requests multiple partitions (e.g. "gpu,cpu") and only one has DefaultTime=0? The current tests all use a single matched partition, so the "reject if any requested partition is zero" semantics (which is the Slurm-faithful behavior here) isn't locked in against a future refactor of the .find() call.

@yansun1996

Copy link
Copy Markdown
Member

@jamesETsmith please resolve the conflicts and amend the above suggested test coverage enhancement.

@jamesETsmith

Copy link
Copy Markdown
Contributor Author

@jamesETsmith please resolve the conflicts and amend the above suggested test coverage enhancement.

Thanks for the review, I'll push the changes shortly

@jamesETsmith
jamesETsmith force-pushed the jamesETsmith/spurctld-partition-defaulttime-0-gives-an-untime branch from 4a137af to d1d1b66 Compare August 29, 2026 13:40
@codecov-commenter

codecov-commenter commented Aug 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #764      +/-   ##
==========================================
+ Coverage   79.45%   79.48%   +0.03%     
==========================================
  Files         181      181              
  Lines       84439    84504      +65     
==========================================
+ Hits        67088    67162      +74     
+ Misses      17351    17342       -9     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Prevent untimed jobs from being accepted with an immediately expired time
limit when a partition explicitly configures DefaultTime as zero.

💘 Generated with Crush

Assisted-by: Crush:gpt-5.6-sol
Ensure an untimed multi-partition job is rejected when any requested
partition explicitly sets its default time to zero.

💘 Generated with Crush

Assisted-by: Crush:gpt-5.6-sol
Signed-off-by: jamesETsmith <james.smith9113@gmail.com>
@yansun1996
yansun1996 force-pushed the jamesETsmith/spurctld-partition-defaulttime-0-gives-an-untime branch from d1d1b66 to 7f18893 Compare August 30, 2026 20:02
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.

spurctld: partition DefaultTime=0 gives an untimed job a zero-length time limit instead of refusing it

3 participants