feat: preempt_v2#429
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds QoS preemption: new protobuf enums/fields, CLI flags for Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
internal/cacctmgr/output.go (1)
911-927:formatPreemptModedefault returning"OFF"can hide unknown values.Falling back to a real, valid mode name for unknown enum values silently masks any future additions to
PreemptMode(or values from a newer backend). A surfaced sentinel is safer for diagnostics:🔍 Suggested fallback
default: - return "OFF" + return fmt.Sprintf("UNKNOWN(%d)", int32(m)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cacctmgr/output.go` around lines 911 - 927, formatPreemptMode currently returns "OFF" for unknown protos.PreemptMode values which can silently hide mismatches; change it to return a clear sentinel string containing the unknown numeric value (e.g., "UNKNOWN_PREEMPT_MODE(<numeric>)" or "UNKNOWN:<numeric>") so diagnostics show unexpected enum values; update formatPreemptMode to detect the default case and format the sentinel using the numeric value of m (from protos.PreemptMode) instead of returning "OFF".internal/cacctmgr/cmd.go (2)
1235-1245: Avoid parsing the preempt list twice.Here you call
ParseStringParamListAllowEmptypurely for validation, then pass the rawvaluestring intoNewValue;ModifyQosincacctmgr.goparses it again. The validation round-trip is harmless but redundant — and the two parses must stay in agreement forever. Either:
- drop the local parse and rely on
ModifyQosto surface the error (it already does, with a similar message), or- thread the parsed slice through
ModifyParam(e.g., add aValueList []stringfield) so the wire path is single-source.♻️ Lightest cleanup (option 1)
case "preempt": - // Validate the list format early. ModifyQos will split NewValue - // again so the backend receives the actual list. Empty value - // means "clear the preempt set" and is accepted. - if _, err := util.ParseStringParamListAllowEmpty(value, ","); err != nil { - return util.NewCraneErr(util.ErrorCmdArg, fmt.Sprintf("invalid preempt list %q: %v\n", value, err)) - } + // Empty value means "clear the preempt set" and is accepted; + // ModifyQos validates and splits the list before sending. params = append(params, ModifyParam{ ModifyField: protos.ModifyField_Preempt, NewValue: value, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cacctmgr/cmd.go` around lines 1235 - 1245, Remove the redundant local parse: drop the call to util.ParseStringParamListAllowEmpty in the "preempt" case and stop validating twice; simply append ModifyParam with ModifyField: protos.ModifyField_Preempt and NewValue: value (leave ModifyParam.NewValue as the raw string) and let ModifyQos in cacctmgr.go perform parsing/validation. Update any comment to note that ModifyQos is the single source of truth for parsing the preempt list and remove that unused import/variable if it becomes unused.
131-141: Consider more defensive error handling inparsePreemptModefor clarity and maintainability.Proto declares 4
PreemptModevalues (OFF, CANCEL, REQUEUE, SUSPEND), but this parser only accepts OFF and CANCEL—the TODO comment correctly acknowledges this is intentional until the scheduler supports the others. Two minor points:
Error message maintenance: The error lists "valid values are OFF, CANCEL" as a hard-coded string. If the backend ever gains support for REQUEUE/SUSPEND ahead of the frontend, this message will become stale. Either keep it in sync manually or consider sourcing the allowed set from a shared constant.
Error return value: On the error path, the function returns
PREEMPT_MODE_OFF(a valid enum value) alongside the error. While both current call sites correctly checkerrand ignore the enum, returning the zero value on error is a safer defensive pattern—returning a meaningful value alongside an error can mislead callers who skip error checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cacctmgr/cmd.go` around lines 131 - 141, Update parsePreemptMode to be more defensive: when returning an error, return the enum zero/unspecified value instead of PREEMPT_MODE_OFF (use protos.PreemptMode(0) or the generated UNSPECIFIED value) and build the error message from a single source of truth (e.g., a local constant or slice of allowed strings defined near parsePreemptMode) rather than a hard-coded literal; keep the switch accepting only "OFF" and "CANCEL" for now but derive the "valid values" text from that constant so it stays in sync with the accepted cases in parsePreemptMode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cacctmgr/help.go`:
- Around line 197-198: The help claims "Self-reference is rejected" for the
Preempt field but neither executeAddQosCommand nor executeModifyQosCommand
enforce it (ParseStringParamListAllowEmpty only checks emptiness); add a
validation step in both executeAddQosCommand and executeModifyQosCommand that
iterates the parsed Preempt list and returns an error if any entry equals the
QoS name being added/modified (use the local qos name variable used in those
functions), or alternatively update the help text to remove the self-reference
claim if backend does not enforce it; ensure the error message clearly
references the Preempt field and the offending name.
In `@protos/PublicDefs.proto`:
- Around line 915-919: The PreemptType enum is unused and should be removed or
documented: locate the enum declaration named PreemptType and either delete it
from PublicDefs.proto (then regenerate protos/code via protoc and remove any
stray references) or replace it with a clear comment explaining it is
intentionally reserved for the backend/C++ if that is required; also verify
related types (e.g., QosInfo which uses PreemptMode) remain correct and update
any proto consumers after regeneration.
---
Nitpick comments:
In `@internal/cacctmgr/cmd.go`:
- Around line 1235-1245: Remove the redundant local parse: drop the call to
util.ParseStringParamListAllowEmpty in the "preempt" case and stop validating
twice; simply append ModifyParam with ModifyField: protos.ModifyField_Preempt
and NewValue: value (leave ModifyParam.NewValue as the raw string) and let
ModifyQos in cacctmgr.go perform parsing/validation. Update any comment to note
that ModifyQos is the single source of truth for parsing the preempt list and
remove that unused import/variable if it becomes unused.
- Around line 131-141: Update parsePreemptMode to be more defensive: when
returning an error, return the enum zero/unspecified value instead of
PREEMPT_MODE_OFF (use protos.PreemptMode(0) or the generated UNSPECIFIED value)
and build the error message from a single source of truth (e.g., a local
constant or slice of allowed strings defined near parsePreemptMode) rather than
a hard-coded literal; keep the switch accepting only "OFF" and "CANCEL" for now
but derive the "valid values" text from that constant so it stays in sync with
the accepted cases in parsePreemptMode.
In `@internal/cacctmgr/output.go`:
- Around line 911-927: formatPreemptMode currently returns "OFF" for unknown
protos.PreemptMode values which can silently hide mismatches; change it to
return a clear sentinel string containing the unknown numeric value (e.g.,
"UNKNOWN_PREEMPT_MODE(<numeric>)" or "UNKNOWN:<numeric>") so diagnostics show
unexpected enum values; update formatPreemptMode to detect the default case and
format the sentinel using the numeric value of m (from protos.PreemptMode)
instead of returning "OFF".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 26a6d1be-c255-42a1-8d6e-ecbd1629e8d8
📒 Files selected for processing (6)
internal/cacctmgr/cacctmgr.gointernal/cacctmgr/cmd.gointernal/cacctmgr/help.gointernal/cacctmgr/output.gointernal/util/string.goprotos/PublicDefs.proto
| Preempt=<qos1,qos2,...> Comma-separated QoS names this QoS may preempt. Self-reference is rejected. | ||
| PreemptMode=<mode> Preemption handling (OFF, CANCEL). Default OFF. REQUEUE/SUSPEND are reserved for future versions. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for any frontend self-reference check tied to the preempt list.
rg -nP '(?i)\bpreempt\b.*\b(self|same.?name|name\s*==)' -C3Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 777
🏁 Script executed:
fd -t f '\.go$' | xargs rg -l 'executeAddQosCommand|executeModifyQosCommand'Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 95
🏁 Script executed:
rg -n 'func.*executeAddQosCommand|func.*executeModifyQosCommand' -A 30Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 3932
🏁 Script executed:
rg -n 'Preempt' internal/cacctmgr/ --type go -B 2 -A 2Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 9335
🏁 Script executed:
rg -n 'ParseStringParamList' internal/cacctmgr/ -B 2 -A 2Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 7245
🏁 Script executed:
fd -t f -name '*.go' | xargs rg -l 'func ParseStringParamList'Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 343
🏁 Script executed:
cat -n internal/util/string.go | head -100Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 3805
🏁 Script executed:
rg -n 'func ParseStringParamList' internal/util/string.go -A 20Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 1349
Frontend does not validate self-reference rejection for Preempt field, but help advertises it.
The help text at line 197 claims Self-reference is rejected., but neither executeAddQosCommand nor executeModifyQosCommand compare the supplied preempt names against the QoS name being added/modified. ParseStringParamListAllowEmpty only validates that values are non-empty; it performs no self-reference checking. If the backend enforces this constraint, the help is misleading about the validation boundary. If the backend does not enforce it, the help is simply incorrect.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cacctmgr/help.go` around lines 197 - 198, The help claims
"Self-reference is rejected" for the Preempt field but neither
executeAddQosCommand nor executeModifyQosCommand enforce it
(ParseStringParamListAllowEmpty only checks emptiness); add a validation step in
both executeAddQosCommand and executeModifyQosCommand that iterates the parsed
Preempt list and returns an error if any entry equals the QoS name being
added/modified (use the local qos name variable used in those functions), or
alternatively update the help text to remove the self-reference claim if backend
does not enforce it; ensure the error message clearly references the Preempt
field and the offending name.
| enum PreemptType { | ||
| PREEMPT_NONE = 0; | ||
| PREEMPT_QOS = 1; | ||
| PREEMPT_PARTITION = 2; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for any usage of PreemptType across the repo (this PR is FrontEnd-only,
# but the proto is shared, so the enum may be referenced via the generated Go bindings).
rg -nP '\bPreemptType\b' --type=go --type=proto -C2Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 100
🏁 Script executed:
#!/bin/bash
# Search for PreemptType across the repo without using unsupported file types
rg -i '\bPreemptType\b' -C2Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 251
Remove PreemptType enum or clarify its intended use.
PreemptType is defined here (lines 915–919) but is not referenced by any message field in this proto file. Notably, QosInfo uses PreemptMode instead. A search of the codebase confirms this enum has no usage in the FrontEnd repository. If this enum is intended for the C++ backend or shared infrastructure, document that; otherwise, remove it to avoid dead code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@protos/PublicDefs.proto` around lines 915 - 919, The PreemptType enum is
unused and should be removed or documented: locate the enum declaration named
PreemptType and either delete it from PublicDefs.proto (then regenerate
protos/code via protoc and remove any stray references) or replace it with a
clear comment explaining it is intentionally reserved for the backend/C++ if
that is required; also verify related types (e.g., QosInfo which uses
PreemptMode) remain correct and update any proto consumers after regeneration.
NamelessOIer
force-pushed
the
dev/preempt_v2
branch
from
57158ce to
f7dd64d
Compare
1 participant
Summary by CodeRabbit
add/modifyQoS commands accept preemption parameters; empty preempt value clears the list.