feat: Add [account/user-partition] resource limit#287
Open
feat: Add [account/user-partition] resource limit#287
Conversation
Contributor
📝 WalkthroughWalkthroughPartition-scoped resource limits were added: CLI flags and parsing, ModifyAccount RPC gained a partition field/parameter, protobufs extended with partition resource limit types and error codes, output rendering now shows per-partition limits, and tests and error messages were updated. Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI (user)
participant Parser as CLI Parser
participant Client as gRPC Client
participant Server as Crane Server
participant Store as Backend/DB
User->>Parser: parse flags/where/set (--partition-limit, Partition=...)
Parser->>Client: build ModifyAccountRequest{Uid, Name, Partition, Params}
Client->>Server: gRPC ModifyAccount(request)
Server->>Store: validate & persist partition-scoped limits
Store-->>Server: OK / ErrCode
Server-->>Client: response (error or success)
Client-->>User: render output (PrintAccountPartitionTable / PrintUserPartitionLimit)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
huerni
force-pushed
the
dev/acc_partition_limit
branch
from
1288e46 to
bd9494d
Compare
huerni
force-pushed
the
dev/acc_partition_limit
branch
from
45ddb53 to
2a54f64
Compare
huerni
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cacctmgr/cacctmgr.go (1)
553-656:⚠️ Potential issue | 🟠 MajorPreserve comma-delimited TRES values as a single operation value.
The new partition-scoped
maxtres/maxtresperjobvalues are valid comma-separated TRES strings, butModifyAccountandModifyUsersplit everyparam.NewValueon commas before sending the RPC. For example,cpu:2,mem:4Gbecomes twoValueListentries instead of one TRES expression, unlike the QoS path that sends[]string{param.NewValue}.🐛 Proposed fix
+func modifyParamValueList(param ModifyParam) ([]string, error) { + switch param.ModifyField { + case protos.ModifyField_MaxTres, protos.ModifyField_MaxTresPerJob: + return []string{param.NewValue}, nil + default: + return util.ParseStringParamList(param.NewValue, ",") + } +} + func ModifyAccount(params []ModifyParam, partition string, name string) error { var valueList []string var err error @@ for _, param := range params { - valueList, err = util.ParseStringParamList(param.NewValue, ",") + valueList, err = modifyParamValueList(param) if err != nil { switch param.ModifyField { @@ for _, param := range params { @@ - valueList, err = util.ParseStringParamList(param.NewValue, ",") + valueList, err = modifyParamValueList(param) if err != nil { return util.WrapCraneErr(util.ErrorCmdArg, "Invalid value list specified: %v.\n", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cacctmgr/cacctmgr.go` around lines 553 - 656, The RPC currently splits every param.NewValue on commas, which incorrectly breaks comma-delimited TRES strings (maxtres/maxtresperjob); update ModifyAccount and ModifyUser so that when param.ModifyField is the TRES fields (protos.ModifyField_MaxTres or protos.ModifyField_MaxTresPerJob) you do not call util.ParseStringParamList and instead set valueList = []string{param.NewValue} (same approach used for QoS path), then continue to append the ModifyFieldOperation; keep existing single-value checks (DefaultQos, Description, AdminLevel, DefaultAccount) intact.
🧹 Nitpick comments (2)
internal/cacctmgr/output.go (1)
185-187: Sort map keys before rendering partition-limit rows.Go map iteration order is randomized, so account/user partition-limit rows can appear in a different order between runs. Sort account and partition keys before appending table rows to keep CLI output and tests stable.
♻️ Proposed direction
- for partition, partitionResourceLimit := range accountInfo.PartitionResourceLimit { + partitions := make([]string, 0, len(accountInfo.PartitionResourceLimit)) + for partition := range accountInfo.PartitionResourceLimit { + partitions = append(partitions, partition) + } + sort.Strings(partitions) + for _, partition := range partitions { + partitionResourceLimit := accountInfo.PartitionResourceLimit[partition]Apply the same pattern for
AccountToPartitionLimitand each nestedPartitionResourceLimitmap.Also applies to: 405-408
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cacctmgr/output.go` around lines 185 - 187, The loops over maps (e.g., the top-level AccountToPartitionLimit map and the nested accountInfo.PartitionResourceLimit map used when building table rows) rely on Go's random map iteration; collect the map keys into a slice, sort them with sort.Strings, and then iterate over the sorted key slice when appending partition-limit rows (apply this pattern for AccountToPartitionLimit and for each accountInfo.PartitionResourceLimit) so output order is stable for CLI and tests.internal/cacctmgr/cmd.go (1)
824-895: Consider centralizing partition-limit parsing.The account and user branches duplicate the same six partition-scoped limit cases. A small helper returning
ModifyParamwould reduce drift and make future limit fields safer to add.Also applies to: 1026-1097
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cacctmgr/cmd.go` around lines 824 - 895, The switch cases for partition-scoped limits (flags "maxSubmitJobs","maxJobs","maxTres","maxTresPerJob","maxWall","maxWallPerJob") are duplicated; create a helper (e.g., buildPartitionLimitParam(value string, flagName string) (ModifyParam, error)) that checks FlagPartition != "", validates/parses the value (use validateUintValue for numeric flags and util.ParseTres for Tres flags), maps flagName to the correct protos.ModifyField (protos.ModifyField_MaxSubmitJobs, _MaxJobs, _MaxTres, _MaxTresPerJob, _MaxWall, _MaxWallDurationPerJob), and returns the constructed ModifyParam with RequestType protos.OperationType_Overwrite or an error; then replace the duplicated case blocks in the account and user branches to call this helper and append the returned ModifyParam to params.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@protos/PublicDefs.proto`:
- Around line 948-955: The PartitionResourceLimit message's scalar fields
max_jobs and max_submit_jobs are currently non-optional and will default to 0 in
proto3, causing producers who omit them to be interpreted as "0 jobs allowed";
update the proto so these two fields are declared optional (i.e., optional
uint32 max_jobs = 3; optional uint32 max_submit_jobs = 4;) so callers can
distinguish unset vs explicitly zero, and regenerate stubs; alternatively, if
you prefer sentinel semantics, add explicit validation in the backend producer
that initializes PartitionResourceLimit.max_jobs and max_submit_jobs to
math.MaxUint32 before sending (ensure any code that constructs
PartitionResourceLimit uses the sentinel), but prefer the optional-field change
for safety.
---
Outside diff comments:
In `@internal/cacctmgr/cacctmgr.go`:
- Around line 553-656: The RPC currently splits every param.NewValue on commas,
which incorrectly breaks comma-delimited TRES strings (maxtres/maxtresperjob);
update ModifyAccount and ModifyUser so that when param.ModifyField is the TRES
fields (protos.ModifyField_MaxTres or protos.ModifyField_MaxTresPerJob) you do
not call util.ParseStringParamList and instead set valueList =
[]string{param.NewValue} (same approach used for QoS path), then continue to
append the ModifyFieldOperation; keep existing single-value checks (DefaultQos,
Description, AdminLevel, DefaultAccount) intact.
---
Nitpick comments:
In `@internal/cacctmgr/cmd.go`:
- Around line 824-895: The switch cases for partition-scoped limits (flags
"maxSubmitJobs","maxJobs","maxTres","maxTresPerJob","maxWall","maxWallPerJob")
are duplicated; create a helper (e.g., buildPartitionLimitParam(value string,
flagName string) (ModifyParam, error)) that checks FlagPartition != "",
validates/parses the value (use validateUintValue for numeric flags and
util.ParseTres for Tres flags), maps flagName to the correct protos.ModifyField
(protos.ModifyField_MaxSubmitJobs, _MaxJobs, _MaxTres, _MaxTresPerJob, _MaxWall,
_MaxWallDurationPerJob), and returns the constructed ModifyParam with
RequestType protos.OperationType_Overwrite or an error; then replace the
duplicated case blocks in the account and user branches to call this helper and
append the returned ModifyParam to params.
In `@internal/cacctmgr/output.go`:
- Around line 185-187: The loops over maps (e.g., the top-level
AccountToPartitionLimit map and the nested accountInfo.PartitionResourceLimit
map used when building table rows) rely on Go's random map iteration; collect
the map keys into a slice, sort them with sort.Strings, and then iterate over
the sorted key slice when appending partition-limit rows (apply this pattern for
AccountToPartitionLimit and for each accountInfo.PartitionResourceLimit) so
output order is stable for CLI and tests.
🪄 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: 93014fcb-3b01-4cb6-9dde-3714a55b5438
📒 Files selected for processing (6)
internal/cacctmgr/cacctmgr.gointernal/cacctmgr/cmd.gointernal/cacctmgr/output.gointernal/cacctmgr/parser.goprotos/Crane.protoprotos/PublicDefs.proto
Comment on lines
+948
to
+955
| message PartitionResourceLimit { | ||
| ResourceView max_tres = 1; | ||
| ResourceView max_tres_per_job = 2; | ||
| uint32 max_jobs = 3; | ||
| uint32 max_submit_jobs = 4; | ||
| uint64 max_wall = 5; | ||
| uint64 max_wall_duration_per_job = 6; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether all PartitionResourceLimit producers initialize job-count sentinels.
set -euo pipefail
echo "Find PartitionResourceLimit construction/update sites:"
rg -n -C 5 'PartitionResourceLimit|partition_resource_limit|PartitionResource' .
echo "Find MaxJobs/MaxSubmitJobs initialization around partition-limit code:"
rg -n -C 5 'MaxJobs|MaxSubmitJobs|max_jobs|max_submit_jobs' .Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 38956
🏁 Script executed:
# Find where PartitionResourceLimit is being populated/constructed in Go code
rg -n 'PartitionResourceLimit\s*{|PartitionResourceLimit\s*\[|partition_resource_limit\s*=' . --type go -A 3
# Look for proto message construction or assignment patterns
rg -n 'partition_resource_limit\s*:=|partition_resource_limit\s*=' . --type go -B 2 -A 2
# Search for AccountInfo initialization with partition limits
rg -n 'AccountInfo\s*{' . --type go -A 10 | head -100Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 355
🏁 Script executed:
# Find where PartitionResourceLimit is being populated/constructed
rg -n 'PartitionResourceLimit' . --type go -A 3
# Look for partition_resource_limit assignments
rg -n 'partition_resource_limit' . --type go -B 1 -A 1 | grep -E '(=|:=|\[)' -A 1 -B 1
# Search for AccountInfo initialization patterns
rg -n 'AccountInfo' . --type go -B 1 -A 5 | head -150
# Check for map assignments to partition_resource_limit
rg -n 'map.*string.*PartitionResourceLimit' . --type goRepository: PKUHPC/CraneSched-FrontEnd
Length of output: 7549
🏁 Script executed:
# Search for C++ code that might handle PartitionResourceLimit
find . -type f \( -name "*.cpp" -o -name "*.h" -o -name "*.cc" \) | head -20
# Check if there are any non-Go files that might be backend
find . -type f -name "*.pb.go" | head -5
# Look for proto syntax version and optional field usage
rg -n 'optional\s+(uint32|uint64)' protos/PublicDefs.proto
# Check the full PartitionResourceLimit definition including any comments
rg -B 10 -A 10 'message PartitionResourceLimit' protos/PublicDefs.proto
# Look for initialization or default value patterns in proto comments
rg -n 'default|initialize|sentinel|MaxUint32|unlimited' protos/PublicDefs.proto | head -30Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 1486
Use optional for PartitionResourceLimit job-count fields or enforce backend sentinel initialization.
In proto3, non-optional scalar fields default to 0 when unset. The output path treats math.MaxUint32 as unlimited for max_jobs and max_submit_jobs (lines 189, 195, 410, 416 in output.go), so omitted fields will be interpreted or rendered as "0 jobs allowed" unless every producer explicitly initializes them to the sentinel. The codebase already uses optional for similar fields elsewhere in PublicDefs.proto (e.g., optional uint64 mem_per_cpu). Either mark max_jobs and max_submit_jobs as optional, or document and enforce that all backend code must initialize these fields to math.MaxUint32.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@protos/PublicDefs.proto` around lines 948 - 955, The PartitionResourceLimit
message's scalar fields max_jobs and max_submit_jobs are currently non-optional
and will default to 0 in proto3, causing producers who omit them to be
interpreted as "0 jobs allowed"; update the proto so these two fields are
declared optional (i.e., optional uint32 max_jobs = 3; optional uint32
max_submit_jobs = 4;) so callers can distinguish unset vs explicitly zero, and
regenerate stubs; alternatively, if you prefer sentinel semantics, add explicit
validation in the backend producer that initializes
PartitionResourceLimit.max_jobs and max_submit_jobs to math.MaxUint32 before
sending (ensure any code that constructs PartitionResourceLimit uses the
sentinel), but prefer the optional-field change for safety.
- Update help.go to document new partition resource limit commands:
- Add --partition-limit/-P flag description in show account/user
- Add account partition resource limit options in modify section
- Add user partition resource limit options in modify section
- Add --partition-limit/-P to GLOBAL OPTIONS
- Add scripts/test_partition_limit.sh:
- Test setting account partition limits (maxJobs, maxSubmitJobs,
maxTres, maxTresPerJob, maxWall, maxWallPerJob)
- Test setting user partition limits (same fields)
- Test show partition limits with --partition-limit and -P flags
- Test error cases (missing partition in where clause)
- Test job submission enforcement of partition limits
- Test resetting partition limits
…limit clarification
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
internal/cacctmgr/output.go (4)
393-395: Inconsistent guard betweenPrintUserPartitionLimitandPrintAccountPartitionTable.This function early-returns on
len(userList) == 0, whilePrintAccountPartitionTabledoesn't have the equivalentlen(accountList) == 0guard. Either make both consistent (preferred: guard on emptytableDataafter the loop in both, since a non-empty list with noPartitionResourceLimitentries still produces an empty table here too).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cacctmgr/output.go` around lines 393 - 395, Both functions are inconsistent: PrintUserPartitionLimit early-returns on len(userList)==0 but PrintAccountPartitionTable does not; instead make the behavior consistent by removing the pre-loop len(...) guard and adding a guard after you build tableData in both PrintUserPartitionLimit and PrintAccountPartitionTable (check if tableData is empty after iterating users/accounts and the PartitionResourceLimit entries) so you only early-return when the rendered table would be empty; reference variables: userList, accountList, PartitionResourceLimit, and tableData to locate the logic to change.
392-460: Sort account and partition keys for deterministic rows.Same concern as
PrintAccountPartitionTable:userInfo.AccountToPartitionLimitand the innerpartitionToLimitMap.PartitionResourceLimitare Go maps, so the (account, partition) pairs for a given user appear in random order on each run. Users are sorted byUidhere but rows within a user are not. Sort both keysets before iterating to produce stable output (and stable test expectations).♻️ Proposed fix sketch
- if userInfo.AccountToPartitionLimit != nil { - for account, partitionToLimitMap := range userInfo.AccountToPartitionLimit { - if partitionToLimitMap != nil { - for partition, partitionResourceLimit := range partitionToLimitMap.PartitionResourceLimit { + if userInfo.AccountToPartitionLimit != nil { + accounts := make([]string, 0, len(userInfo.AccountToPartitionLimit)) + for a := range userInfo.AccountToPartitionLimit { + accounts = append(accounts, a) + } + sort.Strings(accounts) + for _, account := range accounts { + partitionToLimitMap := userInfo.AccountToPartitionLimit[account] + if partitionToLimitMap != nil { + partitions := make([]string, 0, len(partitionToLimitMap.PartitionResourceLimit)) + for p := range partitionToLimitMap.PartitionResourceLimit { + partitions = append(partitions, p) + } + sort.Strings(partitions) + for _, partition := range partitions { + partitionResourceLimit := partitionToLimitMap.PartitionResourceLimit[partition]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cacctmgr/output.go` around lines 392 - 460, PrintUserPartitionLimit produces non-deterministic row order because it iterates maps directly; fix by iterating sorted keys: for each userInfo in PrintUserPartitionLimit, collect and sort the accounts from userInfo.AccountToPartitionLimit, then for each account collect and sort the partitions from partitionToLimitMap.PartitionResourceLimit before iterating and appending rows; keep all existing formatting logic (MaxJobs/MaxSubmitJobs/MaxWall handling, util.ResourceViewToTres, util.TrimTable, table.AppendBulk/table.Render) unchanged.
180-233: Map iteration order is non-deterministic — sort partition keys for stable output.Ranging directly over
accountInfo.PartitionResourceLimitproduces rows in random order across invocations, which makes diffing the CLI output (and the integration tests inscripts/test_partition_limit.sh) flakier and degrades UX when an account has multiple partitions. Consider sorting partition keys before iteration, similar to howaccountListitself is sorted by name inPrintAccountList.♻️ Proposed fix
for _, accountInfo := range accountList { if accountInfo.PartitionResourceLimit != nil { - for partition, partitionResourceLimit := range accountInfo.PartitionResourceLimit { + partitions := make([]string, 0, len(accountInfo.PartitionResourceLimit)) + for p := range accountInfo.PartitionResourceLimit { + partitions = append(partitions, p) + } + sort.Strings(partitions) + for _, partition := range partitions { + partitionResourceLimit := accountInfo.PartitionResourceLimit[partition] var maxJobsStr string🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cacctmgr/output.go` around lines 180 - 233, The loop in PrintAccountPartitionTable iterates directly over the map accountInfo.PartitionResourceLimit causing non-deterministic output; fix it by collecting the partition keys into a slice, sort them with sort.Strings, and then iterate over the sorted slice using the key to retrieve partitionResourceLimit (keep the existing logic building tableData and the later util.TrimTable/table.AppendBulk calls unchanged).
180-233: Consider extracting the per-partition row formatting into a helper to remove duplication withPrintUserPartitionLimit.The unlimited-sentinel handling for
MaxJobs,MaxSubmitJobs,MaxWall, andMaxWallDurationPerJob, plus theResourceViewToTrescalls, is duplicated almost verbatim inPrintUserPartitionLimit(lines 410–434). A small helper such asformatPartitionResourceLimit(prl *protos.PartitionResourceLimit) (maxJobs, maxSubmit, maxWall, maxWallPerJob string)would keep both tables in sync if the sentinel rules ever change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cacctmgr/output.go` around lines 180 - 233, PrintAccountPartitionTable duplicates sentinel-formatting logic also present in PrintUserPartitionLimit; extract that logic into a single helper (e.g. formatPartitionResourceLimit(prl *protos.PartitionResourceLimit) (maxJobs, maxSubmitJobs, maxWall, maxWallPerJob string)) and call it from both PrintAccountPartitionTable and PrintUserPartitionLimit, leaving ResourceViewToTres calls in place for MaxTres/MaxTresPerJob and preserving the current sentinel rules (MaxJobs/MaxSubmitJobs == math.MaxUint32 -> "unlimited", MaxWall == 0 -> "unlimited", MaxWallDurationPerJob >= util.MaxJobTimeLimit -> "unlimited"); update both functions to append the returned formatted strings instead of duplicating the formatting code.scripts/test_partition_limit.sh (4)
32-32:set -ecombined withcacctmgr ... | grep -oP '\d+' | head -1andcqueue ... | head -1is fragile.Multiple pipelines later in the script (lines 84, 324, 334, 348) can produce empty output or non-zero intermediate exit codes that, under
set -euo pipefail, will abort the script mid-test rather than recording alog_fail. Consider relaxing toset -uo pipefail(drop-e) for an integration test where you want all assertions to run, and rely on the explicitFAILcounter and the finalexit 1for the overall result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test_partition_limit.sh` at line 32, The script currently uses "set -euo pipefail" which causes early exit on non-zero pipeline commands (e.g., the pipelines that call cacctmgr ... | grep -oP '\d+' | head -1 and cqueue ... | head -1), making the integration test abort before recording failures; change the shell flags to "set -uo pipefail" (remove -e) in the top-level invocation so pipeline commands that return empty or non-zero intermediate codes don't terminate the script, rely on the existing FAIL counter/log_fail calls and final exit status to report failures, and keep the rest of the script (including the identified pipeline usages) unchanged.
367-367: Use a named sentinel rather than the literal4294967295.The script comment calls this "unlimited", which is exactly
math.MaxUint32perPrintAccountPartitionTable/PrintUserPartitionLimit("unlimited" mapping). Embedding the magic number works today but couples the test to the wire encoding; if the project ever exposes a CLI keyword (e.g.maxSubmitJobs=unlimited) prefer that, otherwise add a brief shell variable likeUNLIMITED_U32=4294967295with a comment for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test_partition_limit.sh` at line 367, Replace the magic literal 4294967295 with a named sentinel variable to show intent and avoid coupling to wire encoding: define e.g. UNLIMITED_U32=4294967295 (with a brief comment “math.MaxUint32 — represents 'unlimited' per PrintAccountPartitionTable/PrintUserPartitionLimit”) near the top of the script or before use, then update the cacctmgr invocation (the line containing cacctmgr modify user where name="$TEST_USER" account="$TEST_ACCOUNT" partition="$TEST_PARTITION" set maxSubmitJobs=4294967295) to use maxSubmitJobs="$UNLIMITED_U32"; if the CLI later supports a keyword like “unlimited”, prefer that keyword instead of the numeric sentinel.
64-64: Help output extraction is fragile against future header edits.
sed -n '2,30p' "$0" | grep '^#' | sed 's/^# \?//'hard-codes the line range of the header block. Any addition/removal in the comment header silently truncates-houtput. A small range comment (e.g., delimit with# === HELP START/END ===markers andsed -n '/HELP START/,/HELP END/p') keeps it self-maintaining.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test_partition_limit.sh` at line 64, The hard-coded header extraction using sed -n '2,30p' "$0" | grep '^#' | sed 's/^# \?//' should be replaced with a marker-driven extraction: add explicit header delimiters (e.g., lines reading "# === HELP START ===" and "# === HELP END ===") in the script comment block and change the extraction to select between those markers (use sed -n '/HELP START/,/HELP END/p' on "$0") and then strip leading '#' and optional space as before; update the code that currently invokes the sed pipeline (the line containing sed -n '2,30p' "$0") and ensure the header contains the corresponding marker lines so future edits won’t break -h output.
53-53:log_skipis defined but never invoked (shellcheck SC2329). Either remove it, or add a real skip path (e.g. whencqueuereturns no partitions, when not run as an admin, etc.) and call it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test_partition_limit.sh` at line 53, The helper function log_skip is defined but never used; either remove the unused log_skip function or add a real skip condition and call it (e.g., detect when cqueue reports no partitions or when the script isn't run as root). For a minimal fix, after you run the cqueue/partition discovery (the variable or command that yields partitions in this script), check if the resulting partitions list is empty or if a required privilege check (UID/EUID) fails, then call log_skip "<reason>" and exit or continue as appropriate; keep using the existing SKIP counter in log_skip so the skip is recorded. Locate and update the partition-checking block (the code that invokes cqueue or assigns the partitions variable) or the privilege check and insert the conditional call to log_skip there, or simply delete the unused log_skip function if you prefer to not implement skip behavior.
🤖 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/output.go`:
- Around line 184-232: The code builds tableData from
accountList.PartitionResourceLimit but still renders the table header when
tableData is empty; after the loop that populates tableData (and before
util.TrimTable / table.AppendBulk / table.Render) add a guard: if len(tableData)
== 0 { return } so the function exits early and does not call table.AppendBulk
or table.Render (mirroring the guard in PrintUserPartitionLimit); reference the
variables/functions tableData, accountInfo.PartitionResourceLimit,
util.TrimTable, table.AppendBulk, and table.Render when making the change.
In `@scripts/test_partition_limit.sh`:
- Around line 322-339: The success check for cbatch is too loose and can match
error lines; update the submission logic around JOB1_OUTPUT, JOB2_OUTPUT and
JOB_IDS to first test cbatch's exit code (e.g. run cbatch and capture its exit
status) and only on a zero exit status proceed to parse the ID, or alternatively
match a much more specific success token that cbatch actually prints (for
example an anchored/case-correct literal like "Job id" or "Submitted batch job"
rather than "job|submitted|id"); ensure you only append to JOB_IDS and call
log_pass when the exit status is zero or the strict success pattern matches, and
call log_fail with the raw output on non-zero exit or no strict match.
- Line 324: The pipeline used to extract JOB IDs (e.g., JOB1_ID=$(echo
"$JOB1_OUTPUT" | grep -oP '\d+' | head -1')) can fail under set -o pipefail if
grep finds no match; change these extractions to a non-pipeline regex using
Bash's built-in regex: test the variable with [[ $JOB1_OUTPUT =~ ([0-9]+) ]]
and, if true, assign JOB1_ID=${BASH_REMATCH[1]} (do the same for the other two
occurrences that extract JOB IDs from JOB2_OUTPUT and similar), or alternatively
append "|| true" to the command substitution to avoid pipeline failure—update
the code paths that set JOB1_ID, JOB2_ID, etc., accordingly.
- Around line 273-301: The negative tests (Tests 4.1–4.4) are broken by set -euo
pipefail because the pipeline `cacctmgr ... 2>&1 | grep -qi "partition"` returns
the non-zero status of cacctmgr even when grep succeeds; fix each test by
capturing cacctmgr output into a variable (e.g. out="$(cacctmgr modify account
where name=\"$TEST_ACCOUNT\" set maxJobs=5 2>&1")"), then run the grep against
that captured output (`echo "$out" | grep -qi "partition"`) and branch on grep's
exit to call log_pass/log_fail; apply the same pattern for the other commands:
`cacctmgr modify user ... set maxSubmitJobs=5`, `cacctmgr modify account ... set
maxTresPerJob=cpu:2`, and `cacctmgr modify user ... set maxWallPerJob=900` so
pipefail no longer causes false failures.
---
Nitpick comments:
In `@internal/cacctmgr/output.go`:
- Around line 393-395: Both functions are inconsistent: PrintUserPartitionLimit
early-returns on len(userList)==0 but PrintAccountPartitionTable does not;
instead make the behavior consistent by removing the pre-loop len(...) guard and
adding a guard after you build tableData in both PrintUserPartitionLimit and
PrintAccountPartitionTable (check if tableData is empty after iterating
users/accounts and the PartitionResourceLimit entries) so you only early-return
when the rendered table would be empty; reference variables: userList,
accountList, PartitionResourceLimit, and tableData to locate the logic to
change.
- Around line 392-460: PrintUserPartitionLimit produces non-deterministic row
order because it iterates maps directly; fix by iterating sorted keys: for each
userInfo in PrintUserPartitionLimit, collect and sort the accounts from
userInfo.AccountToPartitionLimit, then for each account collect and sort the
partitions from partitionToLimitMap.PartitionResourceLimit before iterating and
appending rows; keep all existing formatting logic
(MaxJobs/MaxSubmitJobs/MaxWall handling, util.ResourceViewToTres,
util.TrimTable, table.AppendBulk/table.Render) unchanged.
- Around line 180-233: The loop in PrintAccountPartitionTable iterates directly
over the map accountInfo.PartitionResourceLimit causing non-deterministic
output; fix it by collecting the partition keys into a slice, sort them with
sort.Strings, and then iterate over the sorted slice using the key to retrieve
partitionResourceLimit (keep the existing logic building tableData and the later
util.TrimTable/table.AppendBulk calls unchanged).
- Around line 180-233: PrintAccountPartitionTable duplicates sentinel-formatting
logic also present in PrintUserPartitionLimit; extract that logic into a single
helper (e.g. formatPartitionResourceLimit(prl *protos.PartitionResourceLimit)
(maxJobs, maxSubmitJobs, maxWall, maxWallPerJob string)) and call it from both
PrintAccountPartitionTable and PrintUserPartitionLimit, leaving
ResourceViewToTres calls in place for MaxTres/MaxTresPerJob and preserving the
current sentinel rules (MaxJobs/MaxSubmitJobs == math.MaxUint32 -> "unlimited",
MaxWall == 0 -> "unlimited", MaxWallDurationPerJob >= util.MaxJobTimeLimit ->
"unlimited"); update both functions to append the returned formatted strings
instead of duplicating the formatting code.
In `@scripts/test_partition_limit.sh`:
- Line 32: The script currently uses "set -euo pipefail" which causes early exit
on non-zero pipeline commands (e.g., the pipelines that call cacctmgr ... | grep
-oP '\d+' | head -1 and cqueue ... | head -1), making the integration test abort
before recording failures; change the shell flags to "set -uo pipefail" (remove
-e) in the top-level invocation so pipeline commands that return empty or
non-zero intermediate codes don't terminate the script, rely on the existing
FAIL counter/log_fail calls and final exit status to report failures, and keep
the rest of the script (including the identified pipeline usages) unchanged.
- Line 367: Replace the magic literal 4294967295 with a named sentinel variable
to show intent and avoid coupling to wire encoding: define e.g.
UNLIMITED_U32=4294967295 (with a brief comment “math.MaxUint32 — represents
'unlimited' per PrintAccountPartitionTable/PrintUserPartitionLimit”) near the
top of the script or before use, then update the cacctmgr invocation (the line
containing cacctmgr modify user where name="$TEST_USER" account="$TEST_ACCOUNT"
partition="$TEST_PARTITION" set maxSubmitJobs=4294967295) to use
maxSubmitJobs="$UNLIMITED_U32"; if the CLI later supports a keyword like
“unlimited”, prefer that keyword instead of the numeric sentinel.
- Line 64: The hard-coded header extraction using sed -n '2,30p' "$0" | grep
'^#' | sed 's/^# \?//' should be replaced with a marker-driven extraction: add
explicit header delimiters (e.g., lines reading "# === HELP START ===" and "#
=== HELP END ===") in the script comment block and change the extraction to
select between those markers (use sed -n '/HELP START/,/HELP END/p' on "$0") and
then strip leading '#' and optional space as before; update the code that
currently invokes the sed pipeline (the line containing sed -n '2,30p' "$0") and
ensure the header contains the corresponding marker lines so future edits won’t
break -h output.
- Line 53: The helper function log_skip is defined but never used; either remove
the unused log_skip function or add a real skip condition and call it (e.g.,
detect when cqueue reports no partitions or when the script isn't run as root).
For a minimal fix, after you run the cqueue/partition discovery (the variable or
command that yields partitions in this script), check if the resulting
partitions list is empty or if a required privilege check (UID/EUID) fails, then
call log_skip "<reason>" and exit or continue as appropriate; keep using the
existing SKIP counter in log_skip so the skip is recorded. Locate and update the
partition-checking block (the code that invokes cqueue or assigns the partitions
variable) or the privilege check and insert the conditional call to log_skip
there, or simply delete the unused log_skip function if you prefer to not
implement skip behavior.
🪄 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: 316025c4-a584-4e7d-8097-35436dd0b80b
📒 Files selected for processing (4)
internal/cacctmgr/cmd.gointernal/cacctmgr/help.gointernal/cacctmgr/output.goscripts/test_partition_limit.sh
✅ Files skipped from review due to trivial changes (1)
- internal/cacctmgr/help.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/cacctmgr/cmd.go
Comment on lines
+184
to
+232
| var tableData [][]string | ||
| for _, accountInfo := range accountList { | ||
| if accountInfo.PartitionResourceLimit != nil { | ||
| for partition, partitionResourceLimit := range accountInfo.PartitionResourceLimit { | ||
| var maxJobsStr string | ||
| if partitionResourceLimit.MaxJobs == math.MaxUint32 { | ||
| maxJobsStr = "unlimited" | ||
| } else { | ||
| maxJobsStr = strconv.FormatUint(uint64(partitionResourceLimit.MaxJobs), 10) | ||
| } | ||
| var maxSubmitJobsStr string | ||
| if partitionResourceLimit.MaxSubmitJobs == math.MaxUint32 { | ||
| maxSubmitJobsStr = "unlimited" | ||
| } else { | ||
| maxSubmitJobsStr = strconv.FormatUint(uint64(partitionResourceLimit.MaxSubmitJobs), 10) | ||
| } | ||
| var maxWallStr string | ||
| if partitionResourceLimit.MaxWall == 0 { | ||
| // MaxWall == 0 means no total wall time limit (ZeroDuration on backend) | ||
| maxWallStr = "unlimited" | ||
| } else { | ||
| maxWallStr = util.SecondTimeFormat(int64(partitionResourceLimit.MaxWall)) | ||
| } | ||
| var maxWallPerJobStr string | ||
| if partitionResourceLimit.MaxWallDurationPerJob >= util.MaxJobTimeLimit { | ||
| maxWallPerJobStr = "unlimited" | ||
| } else { | ||
| maxWallPerJobStr = util.SecondTimeFormat(int64(partitionResourceLimit.MaxWallDurationPerJob)) | ||
| } | ||
|
|
||
| tableData = append(tableData, []string{ | ||
| accountInfo.Name, | ||
| partition, | ||
| util.ResourceViewToTres(partitionResourceLimit.MaxTres), | ||
| util.ResourceViewToTres(partitionResourceLimit.MaxTresPerJob), | ||
| maxJobsStr, | ||
| maxSubmitJobsStr, | ||
| maxWallStr, | ||
| maxWallPerJobStr, | ||
| }) | ||
| } | ||
| } | ||
| } | ||
| if !FlagFull && FlagFormat == "" { | ||
| util.TrimTable(&tableData) | ||
| } | ||
|
|
||
| table.AppendBulk(tableData) | ||
| table.Render() |
Contributor
There was a problem hiding this comment.
Empty table is still rendered when no account has partition limits.
If every accountInfo.PartitionResourceLimit is nil/empty, tableData stays empty but the header is still printed, producing an empty table above the regular account table. Consider an early return when len(tableData) == 0 (after the loop) to avoid noisy output, mirroring the len(...) == 0 guard you already have in PrintUserPartitionLimit.
🔧 Proposed fix
}
+ if len(tableData) == 0 {
+ return
+ }
if !FlagFull && FlagFormat == "" {
util.TrimTable(&tableData)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cacctmgr/output.go` around lines 184 - 232, The code builds
tableData from accountList.PartitionResourceLimit but still renders the table
header when tableData is empty; after the loop that populates tableData (and
before util.TrimTable / table.AppendBulk / table.Render) add a guard: if
len(tableData) == 0 { return } so the function exits early and does not call
table.AppendBulk or table.Render (mirroring the guard in
PrintUserPartitionLimit); reference the variables/functions tableData,
accountInfo.PartitionResourceLimit, util.TrimTable, table.AppendBulk, and
table.Render when making the change.
Comment on lines
+273
to
+301
| if cacctmgr modify account where name="$TEST_ACCOUNT" set maxJobs=5 2>&1 | grep -qi "partition"; then | ||
| log_pass "Correctly rejected: maxJobs requires partition in where clause" | ||
| else | ||
| log_fail "Should have rejected maxJobs without partition specification" | ||
| fi | ||
|
|
||
| # Test 4.2: Set maxSubmitJobs for user without partition (should fail) | ||
| log_info "Test 4.2: Set maxSubmitJobs for user without partition (should fail)" | ||
| if cacctmgr modify user where name="$TEST_USER" set maxSubmitJobs=5 2>&1 | grep -qi "partition"; then | ||
| log_pass "Correctly rejected: maxSubmitJobs requires partition in where clause" | ||
| else | ||
| log_fail "Should have rejected maxSubmitJobs without partition specification" | ||
| fi | ||
|
|
||
| # Test 4.3: Set maxTresPerJob without partition (should fail) | ||
| log_info "Test 4.3: Set maxTresPerJob without partition (should fail)" | ||
| if cacctmgr modify account where name="$TEST_ACCOUNT" set maxTresPerJob=cpu:2 2>&1 | grep -qi "partition"; then | ||
| log_pass "Correctly rejected: maxTresPerJob requires partition in where clause" | ||
| else | ||
| log_fail "Should have rejected maxTresPerJob without partition specification" | ||
| fi | ||
|
|
||
| # Test 4.4: Set maxWallPerJob without partition (should fail) | ||
| log_info "Test 4.4: Set maxWallPerJob without partition (should fail)" | ||
| if cacctmgr modify user where name="$TEST_USER" set maxWallPerJob=900 2>&1 | grep -qi "partition"; then | ||
| log_pass "Correctly rejected: maxWallPerJob requires partition in where clause" | ||
| else | ||
| log_fail "Should have rejected maxWallPerJob without partition specification" | ||
| fi |
Contributor
There was a problem hiding this comment.
set -o pipefail makes the negative-test pipelines invert the result — these tests will fail even when cacctmgr correctly rejects the command.
With set -euo pipefail enabled at line 32, the pipeline cacctmgr modify ... 2>&1 | grep -qi "partition" exits with the last non-zero status in the pipeline. When cacctmgr correctly rejects the request and exits non-zero (the success scenario for these tests), grep -qi matches the word "partition" and exits 0, but pipefail propagates cacctmgr's non-zero exit. The if then takes the false branch and log_fail fires for what is actually a passing test.
Capture the output first, then grep, so the negative-path exit code doesn't poison the assertion.
🐛 Proposed fix (apply to Tests 4.1–4.4)
-# Test 4.1: Set maxJobs without specifying partition (should fail)
-log_info "Test 4.1: Set maxJobs without partition in where clause (should fail)"
-if cacctmgr modify account where name="$TEST_ACCOUNT" set maxJobs=5 2>&1 | grep -qi "partition"; then
- log_pass "Correctly rejected: maxJobs requires partition in where clause"
-else
- log_fail "Should have rejected maxJobs without partition specification"
-fi
+# Test 4.1: Set maxJobs without specifying partition (should fail)
+log_info "Test 4.1: Set maxJobs without partition in where clause (should fail)"
+OUTPUT=$(cacctmgr modify account where name="$TEST_ACCOUNT" set maxJobs=5 2>&1 || true)
+if echo "$OUTPUT" | grep -qi "partition"; then
+ log_pass "Correctly rejected: maxJobs requires partition in where clause"
+else
+ log_fail "Should have rejected maxJobs without partition specification: $OUTPUT"
+fi(Apply analogous changes to Tests 4.2, 4.3, and 4.4.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/test_partition_limit.sh` around lines 273 - 301, The negative tests
(Tests 4.1–4.4) are broken by set -euo pipefail because the pipeline `cacctmgr
... 2>&1 | grep -qi "partition"` returns the non-zero status of cacctmgr even
when grep succeeds; fix each test by capturing cacctmgr output into a variable
(e.g. out="$(cacctmgr modify account where name=\"$TEST_ACCOUNT\" set maxJobs=5
2>&1")"), then run the grep against that captured output (`echo "$out" | grep
-qi "partition"`) and branch on grep's exit to call log_pass/log_fail; apply the
same pattern for the other commands: `cacctmgr modify user ... set
maxSubmitJobs=5`, `cacctmgr modify account ... set maxTresPerJob=cpu:2`, and
`cacctmgr modify user ... set maxWallPerJob=900` so pipefail no longer causes
false failures.
Comment on lines
+322
to
+339
| JOB1_OUTPUT=$(cbatch --partition="$TEST_PARTITION" --account="$TEST_ACCOUNT" "$TMPJOB" 2>&1) | ||
| if echo "$JOB1_OUTPUT" | grep -qi "job\|submitted\|id"; then | ||
| JOB1_ID=$(echo "$JOB1_OUTPUT" | grep -oP '\d+' | head -1) | ||
| JOB_IDS+=("$JOB1_ID") | ||
| log_pass "First job submitted successfully (ID: $JOB1_ID)" | ||
| else | ||
| log_fail "First job submission failed: $JOB1_OUTPUT" | ||
| fi | ||
|
|
||
| # Submit second job (should succeed) | ||
| JOB2_OUTPUT=$(cbatch --partition="$TEST_PARTITION" --account="$TEST_ACCOUNT" "$TMPJOB" 2>&1) | ||
| if echo "$JOB2_OUTPUT" | grep -qi "job\|submitted\|id"; then | ||
| JOB2_ID=$(echo "$JOB2_OUTPUT" | grep -oP '\d+' | head -1) | ||
| JOB_IDS+=("$JOB2_ID") | ||
| log_pass "Second job submitted successfully (ID: $JOB2_ID)" | ||
| else | ||
| log_fail "Second job submission failed: $JOB2_OUTPUT" | ||
| fi |
Contributor
There was a problem hiding this comment.
Success heuristic for cbatch is too permissive and may also match error output.
grep -qi "job\|submitted\|id" matches the literal substrings job, submitted, or id in the output. Many cbatch failure messages will also contain job or id (e.g. "failed to submit job", "invalid id"), so a failed submission can be classified as a pass and an JOB?_ID extracted from an unrelated number in the error. Prefer asserting on cbatch's exit code, or matching a more specific success token actually emitted by cbatch (e.g. the literal "Job id" / "submitted" prefix it prints on success).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/test_partition_limit.sh` around lines 322 - 339, The success check
for cbatch is too loose and can match error lines; update the submission logic
around JOB1_OUTPUT, JOB2_OUTPUT and JOB_IDS to first test cbatch's exit code
(e.g. run cbatch and capture its exit status) and only on a zero exit status
proceed to parse the ID, or alternatively match a much more specific success
token that cbatch actually prints (for example an anchored/case-correct literal
like "Job id" or "Submitted batch job" rather than "job|submitted|id"); ensure
you only append to JOB_IDS and call log_pass when the exit status is zero or the
strict success pattern matches, and call log_fail with the raw output on
non-zero exit or no strict match.
| # Submit first job (should succeed) | ||
| JOB1_OUTPUT=$(cbatch --partition="$TEST_PARTITION" --account="$TEST_ACCOUNT" "$TMPJOB" 2>&1) | ||
| if echo "$JOB1_OUTPUT" | grep -qi "job\|submitted\|id"; then | ||
| JOB1_ID=$(echo "$JOB1_OUTPUT" | grep -oP '\d+' | head -1) |
Contributor
There was a problem hiding this comment.
grep -oP | head -1 inside $(...) can trip set -o pipefail when no match is found.
If cbatch's output contains no digits, grep -oP '\d+' exits 1 and head -1 exits 0 with empty input, so the pipeline returns non-zero under pipefail. Combined with set -e, the assignment JOB1_ID=$(...) propagates the failure and aborts the script before log_fail can run. Either suffix the substitution with || true, or extract via a non-pipeline regex (e.g. [[ $JOB1_OUTPUT =~ ([0-9]+) ]] && JOB1_ID=${BASH_REMATCH[1]}). Same applies to lines 334 and 348.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/test_partition_limit.sh` at line 324, The pipeline used to extract
JOB IDs (e.g., JOB1_ID=$(echo "$JOB1_OUTPUT" | grep -oP '\d+' | head -1')) can
fail under set -o pipefail if grep finds no match; change these extractions to a
non-pipeline regex using Bash's built-in regex: test the variable with [[
$JOB1_OUTPUT =~ ([0-9]+) ]] and, if true, assign JOB1_ID=${BASH_REMATCH[1]} (do
the same for the other two occurrences that extract JOB IDs from JOB2_OUTPUT and
similar), or alternatively append "|| true" to the command substitution to avoid
pipeline failure—update the code paths that set JOB1_ID, JOB2_ID, etc.,
accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests
Other