Feat: Add QOS fields: MaxJobsPerUser, MaxSubmitJobsPerUser, MaxJobsPerAccount, MaxSubmitJobsPerAccount #252
Conversation
WalkthroughThe changes introduce new QoS-related job and submission limit parameters at both the user and account levels. These limits are added to command-line flags, protobuf definitions, error codes, and output formatting, enabling users to set and view maximum jobs and submissions per account and user within the system. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant CmdArgParser
participant Server
User->>CLI: Run addQos/modifyQos command with new flags
CLI->>CmdArgParser: Parse flags (including new QoS limits)
CmdArgParser->>Server: Send request with new QoS fields
Server-->>CmdArgParser: Respond with status
CmdArgParser-->>CLI: Output result
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
huerni
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
protos/PublicDefs.proto (1)
560-560: Docstring fornonexistent_jobs
The new fieldrepeated uint32 nonexistent_jobs = 6;inCranedRemoteMetawould benefit from a brief comment explaining when and why these job IDs appear in the array.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
internal/cacctmgr/CmdArgParser.go(3 hunks)internal/util/err.go(1 hunks)protos/PublicDefs.proto(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/util/err.go
- internal/cacctmgr/CmdArgParser.go
🔇 Additional comments (2)
protos/PublicDefs.proto (2)
538-540: ConfirmQosInfoschema migration and codegen
The addition of:uint32 max_jobs_per_account = 7; uint32 max_submit_jobs_per_user = 8; uint32 max_submit_jobs_per_account = 9;is consistent and preserves field ordering. Please regenerate your gRPC stubs and update API documentation or client examples to include the new QoS fields.
473-484:✅ Verification successful
Verify
ModifyFieldenum alignment with CLI parser
The new modifiable fields (MaxJobsPerUser,MaxSubmitJobsPerUser,MaxSubmitJobsPerAccount, etc.) have been appended toModifyField. Ensure that the command‐line flag parser (internal/cacctmgr/CmdArgParser.go) and any switch/case handling these enums use the correct numeric values.You can run:
🏁 Script executed:
#!/bin/bash # Verify ModifyField values in the CLI parser rg -n "ModifyField_" -A2 internal/cacctmgr/CmdArgParser.goLength of output: 4863
ModifyField enums properly referenced in CLI parser
I verified that all newly addedModifyFieldenum values (MaxJobsPerUser,MaxSubmitJobsPerUser,MaxSubmitJobsPerAccount,MaxCpusPerUser,MaxTimeLimitPerTask,MaxJobsPerAccount, etc.) are explicitly referenced by name ininternal/cacctmgr/CmdArgParser.go. Since the parser uses the generatedprotos.ModifyField_*constants rather than hard-coded integers, the numeric alignment is guaranteed by the Protocol Buffer code generator. No further changes are required.
protos/PublicDefs.proto
Outdated
| ERR_LIB_EVENT = 63 [deprecated = true]; | ||
| ERR_NO_AVAIL_NODE = 64; | ||
|
|
||
| ERR_MAX_JOB_COUNT_PER_USER = 65; | ||
| ERR_USER_NO_PRIVILEGE = 66; | ||
| ERR_NOT_IN_ALLOWED_LIST = 67; // The current account is not in the allowed account list for the partition. | ||
| ERR_IN_DENIED_LIST = 68; // The current account has been explicitly added to the deny list for the partition. | ||
| ERR_IN_DENIED_LIST = 68; // The current account has been explicitly added to the deny list for the partition. | ||
| ERR_EBPF = 69; | ||
| ERR_SUPERVISOR = 70; | ||
| ERR_MAX_JOB_COUNT_PER_ACCOUNT = 71; | ||
| ERR_USER_HAS_TASK = 72; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
ErrCode deprecation and missing submit-limit error codes
You've correctly deprecated ERR_LIB_EVENT = 63, but please ensure all references (in code and docs) are updated/replaced accordingly.
Additionally, you added limits fields for submissions per user/account (MaxSubmitJobsPerUser, MaxSubmitJobsPerAccount), but there are no corresponding ErrCode entries when those limits are exceeded. To fully support error handling for submission limits, consider adding two new codes after ERR_USER_HAS_TASK = 72:
enum ErrCode {
...
ERR_USER_HAS_TASK = 72;
+ ERR_MAX_SUBMIT_JOB_COUNT_PER_USER = 73; // Exceeded max submit jobs per user
+ ERR_MAX_SUBMIT_JOB_COUNT_PER_ACCOUNT = 74; // Exceeded max submit jobs per account
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ERR_LIB_EVENT = 63 [deprecated = true]; | |
| ERR_NO_AVAIL_NODE = 64; | |
| ERR_MAX_JOB_COUNT_PER_USER = 65; | |
| ERR_USER_NO_PRIVILEGE = 66; | |
| ERR_NOT_IN_ALLOWED_LIST = 67; // The current account is not in the allowed account list for the partition. | |
| ERR_IN_DENIED_LIST = 68; // The current account has been explicitly added to the deny list for the partition. | |
| ERR_IN_DENIED_LIST = 68; // The current account has been explicitly added to the deny list for the partition. | |
| ERR_EBPF = 69; | |
| ERR_SUPERVISOR = 70; | |
| ERR_MAX_JOB_COUNT_PER_ACCOUNT = 71; | |
| ERR_USER_HAS_TASK = 72; | |
| ERR_LIB_EVENT = 63 [deprecated = true]; | |
| ERR_NO_AVAIL_NODE = 64; | |
| ERR_MAX_JOB_COUNT_PER_USER = 65; | |
| ERR_USER_NO_PRIVILEGE = 66; | |
| ERR_NOT_IN_ALLOWED_LIST = 67; // The current account is not in the allowed account list for the partition. | |
| ERR_IN_DENIED_LIST = 68; // The current account has been explicitly added to the deny list for the partition. | |
| ERR_EBPF = 69; | |
| ERR_SUPERVISOR = 70; | |
| ERR_MAX_JOB_COUNT_PER_ACCOUNT = 71; | |
| ERR_USER_HAS_TASK = 72; | |
| ERR_MAX_SUBMIT_JOB_COUNT_PER_USER = 73; // Exceeded max submit jobs per user | |
| ERR_MAX_SUBMIT_JOB_COUNT_PER_ACCOUNT = 74; // Exceeded max submit jobs per account |
🤖 Prompt for AI Agents
In protos/PublicDefs.proto around lines 444 to 454, you have deprecated
ERR_LIB_EVENT but need to update all code and documentation references to this
deprecated code. Also, add two new error codes after ERR_USER_HAS_TASK = 72 to
represent submission limit exceed errors for user and account, ensuring proper
error handling for MaxSubmitJobsPerUser and MaxSubmitJobsPerAccount limits.
5a949b1 to
3584b2a
Compare
huerni
force-pushed
the
dev/qos_limit
branch
3 times, most recently
from
0078c17 to
8e1ebf4
Compare
Summary by CodeRabbit
New Features
Bug Fixes
Documentation