Add cbatch/crun/calloc add --exclusive #256
Conversation
1daidai1
force-pushed
the
dev/exclusivenew
branch
7 times, most recently
from
793126e to
7ccde9d
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for an "--exclusive" flag across multiple components (cbatch, crun, calloc, etc.) to indicate exclusive node resource usage. The changes update proto messages, command-line argument parsing, and output formatting to include the new exclusive field.
- Revised proto definitions and message fields to propagate exclusive attributes.
- Updated command-line parsers and flag definitions for cbatch, crun, calloc, and related commands.
- Adjusted output formatting in components such as cqueue, ceff, ccontrol, and cacct to display the exclusive status.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| protos/PublicDefs.proto | Added new proto messages and renamed fields; introduced "exclusive". |
| internal/util/string.go | Updated field reference from Resources to ReqResources. |
| internal/crun/crun.go | Switched resource field references from Resources to ReqResources. |
| internal/crun/CmdArgParser.go | Added --exclusive flag support in crun command parser. |
| internal/cqueue/cqueue.go | Updated output processors and format specifiers to include exclusive. |
| internal/cqueue/CmdArgParser.go | Updated help text to include --exclusive flag. |
| internal/ceff/ceff.go | Adjusted calculations to use new aggregated resource fields. |
| internal/ccontrol/ccontrol.go | Changed memory formatting function calls to util.FormatMemToMB. |
| internal/cbatch/cbatch.go | Updated to use ReqResources and added exclusive flag processing. |
| internal/cbatch/CmdArgParser.go | Added --exclusive flag support in cbatch command parser. |
| internal/calloc/calloc.go | Updated resource references from Resources to ReqResources and added exclusive. |
| internal/calloc/CmdArgParser.go | Included --exclusive flag support in calloc command parser. |
| internal/cacct/cacct.go | Adjusted table output and field processors to include exclusive status. |
| internal/cacct/CmdArgParser.go | Updated help text for new exclusive flag in cacct command parser. |
Comments suppressed due to low confidence (1)
protos/PublicDefs.proto:299
- [nitpick] Consider adding spaces around the '=' operator for consistency (e.g., 'string alloc_device_total = 42;').
string alloc_device_total= 42;
|
""" WalkthroughThe changes update resource formatting and processing across multiple components to distinguish requested and allocated resources more clearly. New format specifiers and processing functions are introduced for CPU, memory, and exclusivity attributes. Command-line interfaces in several modules gain a new boolean Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLIParser
participant TaskConfigurator
participant Scheduler
User->>CLIParser: Submit job with --exclusive flag
CLIParser->>TaskConfigurator: Parse CLI flags (FlagExclusive)
TaskConfigurator->>TaskConfigurator: Rename Resources to ReqResources
TaskConfigurator->>TaskConfigurator: Set task.Exclusive = true (if FlagExclusive)
TaskConfigurator->>Scheduler: Submit task with updated resource fields
Scheduler-->>User: Acknowledge job with exclusive configuration
Suggested labels
Suggested reviewers
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (14)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (12)
🧰 Additional context used🧬 Code Graph Analysis (1)internal/ccontrol/ccontrol.go (1)
🔇 Additional comments (6)
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
internal/cacct/cacct.go (1)
324-327: Consider reducing code duplication across files.The functions
ProcessReqCPUs,ProcessReqMemPerNode,ProcessAllocMemPerNode, andProcessExclusivehave very similar implementations in both this file andcqueue.go. Consider moving these to a common utility package to avoid duplication and ensure consistency.Also applies to: 408-411, 413-419, 500-503
internal/cqueue/cqueue.go (2)
181-184: Data processing functions updated for resource values.The implementation correctly uses the new processing functions for resource data, but note that line 192 directly uses
strconv.FormatBool(taskInfo.Exclusive)instead of callingProcessExclusive(taskInfo)which is inconsistent with how other fields are processed.Consider using the dedicated processing function for consistency:
-strconv.FormatBool(taskInfo.Exclusive), +ProcessExclusive(taskInfo),Also applies to: 192-192
385-388: Consider reducing code duplication across files.The functions
ProcessReqCPUs,ProcessReqMemPerNode,ProcessAllocMemPerNode, andProcessExclusivehave very similar implementations in both this file andcacct.go. Consider moving these to a common utility package to avoid duplication and ensure consistency.Also applies to: 357-360, 350-355, 442-445
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
internal/cacct/CmdArgParser.go(1 hunks)internal/cacct/cacct.go(10 hunks)internal/calloc/CmdArgParser.go(2 hunks)internal/calloc/calloc.go(5 hunks)internal/cbatch/CmdArgParser.go(2 hunks)internal/cbatch/cbatch.go(8 hunks)internal/ccontrol/ccontrol.go(4 hunks)internal/ceff/ceff.go(5 hunks)internal/cqueue/CmdArgParser.go(1 hunks)internal/cqueue/cqueue.go(11 hunks)internal/crun/CmdArgParser.go(2 hunks)internal/crun/crun.go(4 hunks)internal/util/string.go(2 hunks)protos/PublicDefs.proto(5 hunks)
🧰 Additional context used
🧬 Code Definitions (5)
internal/cbatch/CmdArgParser.go (9)
internal/calloc/CmdArgParser.go (1)
RootCmd(54-67)internal/crun/CmdArgParser.go (1)
RootCmd(58-70)internal/cqueue/CmdArgParser.go (1)
RootCmd(47-72)internal/cacctmgr/CmdArgParser.go (1)
RootCmd(69-82)internal/cinfo/CmdArgParser.go (1)
RootCmd(42-62)internal/cacct/CmdArgParser.go (1)
RootCmd(47-70)internal/ccontrol/CmdArgParser.go (1)
RootCmd(46-57)internal/ccancel/CmdArgParser.go (1)
RootCmd(41-84)internal/cplugind/cplugind.go (1)
RootCmd(36-123)
internal/crun/CmdArgParser.go (9)
internal/calloc/CmdArgParser.go (1)
RootCmd(54-67)internal/cbatch/CmdArgParser.go (1)
RootCmd(63-125)internal/cqueue/CmdArgParser.go (1)
RootCmd(47-72)internal/cacct/CmdArgParser.go (1)
RootCmd(47-70)internal/cinfo/CmdArgParser.go (1)
RootCmd(42-62)internal/cacctmgr/CmdArgParser.go (1)
RootCmd(69-82)internal/ccontrol/CmdArgParser.go (1)
RootCmd(46-57)internal/ccancel/CmdArgParser.go (1)
RootCmd(41-84)internal/cplugind/cplugind.go (1)
RootCmd(36-123)
internal/cbatch/cbatch.go (4)
internal/util/string.go (1)
ParseGres(642-687)internal/cbatch/CmdArgParser.go (2)
FlagGres(46-46)FlagExclusive(51-51)internal/calloc/CmdArgParser.go (2)
FlagGres(39-39)FlagExclusive(44-44)internal/crun/CmdArgParser.go (2)
FlagGres(43-43)FlagExclusive(45-45)
internal/cqueue/cqueue.go (1)
internal/cacct/cacct.go (4)
ProcessReqCPUs(325-327)ProcessReqMemPerNode(409-411)ProcessAllocMemPerNode(414-419)ProcessExclusive(501-503)
internal/cacct/cacct.go (1)
internal/cqueue/cqueue.go (4)
ProcessReqCPUs(386-388)ProcessReqMemPerNode(358-360)ProcessAllocMemPerNode(350-355)ProcessExclusive(443-445)
🔇 Additional comments (47)
internal/crun/CmdArgParser.go (2)
45-45: Clean flag declaration added.The
FlagExclusiveboolean flag is properly declared alongside other configuration flags, following the naming convention established in the file.
111-111: Flag properly initialized with cobra.The
FlagExclusiveflag is correctly initialized with the Cobra library, including an appropriate default value (false) and a clear description ("Exclusive node resources").internal/cbatch/CmdArgParser.go (2)
51-51: Flag declaration consistent with other commands.The
FlagExclusiveboolean flag is properly declared, maintaining consistency with its implementation in thecruncommand.
162-162: Flag properly initialized with consistent parameters.The
FlagExclusiveflag is correctly initialized with the same parameters as in thecruncommand, ensuring a uniform interface across different commands in the CraneSched-FrontEnd project.internal/calloc/calloc.go (3)
373-379: Field renamed from Resources to ReqResources.The task structure initialization now uses
ReqResourcesinstead ofResources, which more precisely indicates that these are requested resources rather than allocated ones. This renaming appears to be part of a larger refactoring across the codebase.
402-402: References to Resources consistently updated to ReqResources.All references to the former
Resourcesfield have been systematically updated to useReqResources, maintaining consistency throughout the file:
- Device map assignment (line 402)
- Memory limit settings (lines 418-419)
- CPU core limit calculation (line 472)
Also applies to: 418-419, 472-472
461-463: Exclusive flag implementation properly integrated.The implementation correctly sets the
task.Exclusivefield totruewhen theFlagExclusiveflag is enabled. This placement is logical, coming after other flag processing and before the extra attributes are marshaled.internal/crun/crun.go (3)
804-810: Field renamed consistently with other files.The task structure initialization uses
ReqResourcesinstead ofResources, maintaining consistency with the same renaming incalloc.goand other files in the codebase.
847-849: References to Resources consistently updated throughout the file.All references to the former
Resourcesfield have been systematically updated to useReqResources:
- Memory limit settings (lines 847-848)
- Device map assignment (line 852)
- CPU core limit calculation (line 905)
Also applies to: 852-852, 905-905
894-896: Exclusive flag handling consistent with other commands.The implementation for setting the
task.Exclusivefield matches the approach used incalloc.go, ensuring uniform behavior across different commands when the--exclusiveflag is used.internal/calloc/CmdArgParser.go (2)
44-44: New flag added for exclusive resource allocation.The
FlagExclusiveboolean variable has been added to support exclusive node resource allocation. This aligns with similar changes in other command-line utilities (cbatch, crun) in the codebase.
100-100: Flag registration for exclusive resource allocation.The
--exclusiveflag is properly registered with a descriptive help message and a default value offalse. This implementation enables users to request exclusive node resources when needed.internal/util/string.go (1)
305-307: Field access updated to use ReqResources instead of Resources.The code has been updated to access CPU core limits from
task.ReqResourcesinstead oftask.Resources. This change is part of a broader refactoring across the codebase where the field has been renamed fromResourcestoReqResources.internal/ceff/ceff.go (4)
336-339: Added validation for node count and updated CPU calculation.The code now checks if
taskInfo.NodeNumis 0 before performing calculations that depend on it, which prevents potential division by zero errors. The CPU total calculation has also been updated to usetaskInfo.AllocCpusTotaldirectly.
367-369: Updated memory efficiency calculation to use AllocMemTotal.The calculation of memory allocation per node and total memory allocation now uses
taskInfo.AllocMemTotaldirectly instead of computing it from other fields. This change maintains consistency with the CPU calculation update.
404-409: Added node count validation and updated CPU calculation in JSON output.Similar to the changes in
PrintTaskInfo, this code adds validation fortaskInfo.NodeNumand updates the CPU calculation to usetaskInfo.AllocCpusTotaldirectly in the JSON output function. This ensures consistent error handling and resource calculation between the two output methods.
440-444: Updated memory calculation in JSON output.The memory allocation calculation in the JSON output function has been updated to use
taskInfo.AllocMemTotaldirectly, matching the changes made in thePrintTaskInfofunction. This ensures consistency between the two output methods.internal/cbatch/cbatch.go (4)
45-51: Updated field name from Resources to ReqResources.The field name has been changed from
ResourcestoReqResourcesto better reflect that it represents requested resources rather than allocated resources. This naming convention is clearer and more descriptive of the field's purpose.
84-84: Consistent field renaming throughout resource handling code.All references to
Resourceshave been updated to useReqResourcesthroughout the file, ensuring consistency with the field renaming. This includes device map access, memory limits, and CPU core limit calculations.Also applies to: 105-106, 179-180, 196-198, 258-258
146-153: Added support for exclusive flag in batch script arguments.Support for the
--exclusiveflag has been added to the argument processing loop, allowing users to specify exclusive node allocation in their batch scripts. The implementation correctly parses the boolean value and assigns it totask.Exclusive.
245-247: Added command-line override for exclusive flag.This code allows the command-line
--exclusiveflag to override the script setting, maintaining consistency with how other flags are processed. Command-line arguments take precedence over script arguments, which is clearly documented in the code comments.internal/ccontrol/ccontrol.go (4)
177-179: Improved code reuse by utilizing a centralized utility function.The code now uses
util.FormatMemToMBinstead of a local function, which promotes better code organization and reuse across the codebase.
230-232: LGTM! Consistent use of memory formatting utility.This change maintains consistency with the changes on lines 177-179 by using the same utility function.
345-348: Good handling of allocDeviceTotal display.The code properly initializes allocDeviceTotal to "None" and conditionally updates it if actual data is available.
358-373: Enhanced job information display with clear requested vs. allocated resources.The output format has been improved to clearly differentiate between requested resources (ReqRes) and allocated resources (AllocRes), making it easier for users to understand resource utilization. The addition of the exclusive flag display is also helpful.
internal/cqueue/CmdArgParser.go (1)
125-141: Well-documented new format specifiers for command-line output.The new format specifiers clearly explain the additional information that can be displayed:
%M/%ReqMemPerNode- For requested memory per node%m/%AllocMemPerNode- For allocated memory per node%Q/%ReqCpus- For requested CPUs%X/%Exclusive- For the exclusive statusThese additions align with the resource request/allocation distinction being implemented throughout the codebase.
protos/PublicDefs.proto (5)
119-119: Clear field renaming for better semantics.Renaming
resourcestoreq_resourcesin the TaskToCtld message clarifies that this field represents requested resources rather than allocated resources.
149-149: Added exclusive resource allocation support.The new
exclusiveboolean field allows tasks to request exclusive access to resources, which is a valuable feature for workloads that require dedicated access to hardware.
176-178: Added fields to track allocated resources.These new fields in RuntimeAttrOfTask provide a way to track the total CPU cores, memory, and devices allocated to a task, which is essential for resource monitoring and reporting.
269-269: Consistent field renaming for resource view.Renaming
res_viewtoreq_res_viewmaintains consistency with the earlier change fromresourcestoreq_resources.
292-295: Complete implementation of resource tracking in TaskInfo.These additions mirror the fields added to RuntimeAttrOfTask, ensuring that all necessary information about exclusive execution and allocated resources is available in the TaskInfo message.
internal/cacct/CmdArgParser.go (1)
119-148: Added consistent format specifiers across command-line tools.The format specifiers added here match those added to cqueue/CmdArgParser.go, ensuring a consistent user experience across different tools in the suite. The documentation is clear and well-formatted.
internal/cacct/cacct.go (8)
164-165: Table header updated to reflect the distinction between requested and allocated resources.The header now includes "ReqCPUs", "ReqMemPerNode", "AllocCPUs", "AllocMemPerNode", and "Exclusive", which aligns with the PR objective of adding exclusive resource allocation support.
213-217: Data processing functions updated for resource values.The implementation now properly uses the new processing functions to format resource data, including
ProcessReqCPUs,ProcessReqMemPerNode,ProcessAllocCPUs,ProcessAllocMemPerNode, andProcessExclusive.Also applies to: 223-223
324-327: NewProcessReqCPUsfunction implemented.This function correctly calculates the total requested CPUs by multiplying the per-node CPU core limit by the number of nodes.
408-411: NewProcessReqMemPerNodefunction implemented.The function properly calculates the requested memory per node in megabytes by converting from bytes.
413-419: NewProcessAllocMemPerNodefunction implemented with zero handling.The function includes proper handling for the case where
NodeNumis 0 to avoid division by zero errors.
500-503: AddedProcessExclusivefunction.This function returns the exclusivity status as a boolean string, which is used for the new exclusive flag in the table output.
515-518: Updated field processor mappings for new resource fields.The field processor map has been correctly updated to include mappings for the new resource processing functions.
Also applies to: 555-562, 615-618
676-678: Updated format specifier help text.The help text now includes the new format specifiers, which is helpful for users.
internal/cqueue/cqueue.go (7)
135-136: Table header updated to reflect the distinction between requested and allocated resources.The header now includes "ReqCPUs", "ReqMemPerNode", "AllocCPUs", "AllocMemPerNode", and "Exclusive", which aligns with the PR objective of adding exclusive resource allocation support.
301-304: RenamedProcessCpuPerNodetoProcessReqCpuPerNode.The function has been appropriately renamed to clarify that it processes requested (not allocated) CPU per node, and now uses
ReqResViewinstead ofResView.
350-355: RenamedProcessMemPerNodetoProcessAllocMemPerNodewith zero handling.The function has been renamed for clarity and includes proper handling for when
NodeNumis 0 to avoid division by zero errors.
357-360: AddedProcessReqMemPerNodefunction.The function properly calculates the requested memory per node in megabytes.
385-388: AddedProcessReqCPUsfunction.This function correctly calculates the total requested CPUs across all nodes.
442-445: AddedProcessExclusivefunction.This function returns the exclusivity status as a boolean string, matching the implementation in
cacct.go.
597-599: Updated format specifier help text.The help text now includes the new format specifiers, which is helpful for users. However, the mapping inconsistencies noted above should be addressed to ensure the help text is accurate.
Also applies to: 603-605
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
internal/calloc/CmdArgParser.go(2 hunks)internal/cbatch/CmdArgParser.go(2 hunks)internal/cbatch/cbatch.go(8 hunks)internal/cqueue/CmdArgParser.go(1 hunks)internal/cqueue/cqueue.go(11 hunks)internal/crun/CmdArgParser.go(2 hunks)internal/util/string.go(2 hunks)protos/PublicDefs.proto(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/cbatch/CmdArgParser.go
- internal/crun/CmdArgParser.go
- internal/calloc/CmdArgParser.go
- protos/PublicDefs.proto
- internal/cqueue/cqueue.go
- internal/cqueue/CmdArgParser.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/cbatch/cbatch.go (4)
internal/util/string.go (1)
ParseGres(643-688)internal/calloc/CmdArgParser.go (2)
FlagGres(39-39)FlagExclusive(44-44)internal/cbatch/CmdArgParser.go (2)
FlagGres(46-46)FlagExclusive(51-51)internal/crun/CmdArgParser.go (2)
FlagGres(43-43)FlagExclusive(45-45)
🔇 Additional comments (9)
internal/util/string.go (2)
306-307: Field renamed from Resources to ReqResources.The change modifies the CPU core limit check to validate against
ReqResourcesinstead ofResources, which aligns with the broader renaming pattern across the codebase.
883-890: New utility function for memory formatting.This function converts memory sizes from bytes to MB for display. The function properly handles the edge case when data is 0 and correctly applies the B2MB conversion ratio.
internal/cbatch/cbatch.go (7)
46-52: Field renamed from Resources to ReqResources.The resource initialization has been updated to use
ReqResourcesinstead ofResources, maintaining consistent naming across the codebase.
85-85: Field renamed from Resources to ReqResources.The device map assignment now uses
ReqResourcesfor consistency.
106-107: Field renamed from Resources to ReqResources.Memory limits are now assigned to
ReqResourcesfor consistency.
147-153: Added support for parsing the --exclusive flag from job scripts.New option allows users to specify exclusive resource allocation in job scripts. The implementation correctly parses boolean values and handles error cases.
189-189: Field renamed from Resources to ReqResources.Command-line gres argument processing now uses
ReqResourcesfor consistency.
206-207: Field renamed from Resources to ReqResources.Memory limits from command-line arguments now use
ReqResourcesfor consistency.
277-277: Field renamed from Resources to ReqResources.CPU core limit calculation now uses
ReqResourcesfor consistency.
1daidai1
force-pushed
the
dev/exclusivenew
branch
from
ecae063 to
adc5f7d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
internal/cqueue/cqueue.go (2)
458-461: CPU field mapping reminder
Past reviews noted inconsistent CPU field mappings (“c” vs. “C”). This change sets"C" → ReqCpusand"c" → AllocCpus. Please confirm that the corresponding mappings incacct.goare aligned.
504-508: 'Q' → 'ReqCpuPerNode'
Ensure the usage of'Q'for “ReqCpuPerNode” is reflected consistently in other files (e.g.,cacct.go) to avoid confusion in format specifiers.
🧹 Nitpick comments (2)
internal/cqueue/cqueue.go (2)
385-389: Minor doc mismatch for 'Q' group
The comment at line 385 merges “group” with “ProcessReqCpus” and may be missing a space. Also verify that mapping “Q” → total requested CPUs is clearly documented elsewhere for consistency.
597-599: Doc comment mapping mismatch
The doc comment here states'c' → 'ReqCpuPerNode'and'C' → 'AllocCpus', which contradicts the actualfieldMap(lines 458-461). Please update the reference documentation for accuracy.Also applies to: 603-605
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/cbatch/cbatch.go(8 hunks)internal/cqueue/CmdArgParser.go(1 hunks)internal/cqueue/cqueue.go(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/cbatch/cbatch.go
- internal/cqueue/CmdArgParser.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/cqueue/cqueue.go (1)
internal/cacct/cacct.go (4)
ProcessReqCPUs(325-327)ProcessReqMemPerNode(409-411)ProcessAllocMemPerNode(414-419)ProcessExclusive(501-503)
🔇 Additional comments (7)
internal/cqueue/cqueue.go (7)
135-136: Columns for resource info added
The newly introduced columns—“ReqCPUs”, “ReqMemPerNode”, “AllocCPUs”, “AllocMemPerNode”, and “Exclusive”—look correct. Ensure the naming remains consistent across the codebase (e.g., withcacct.go).
181-184: New resource processing function calls
UsingProcessReqCPUs,ProcessReqMemPerNode,ProcessAllocCpus, andProcessAllocMemPerNodein these table columns appears correct and consistent with their definitions.
192-192: Exclusive column output
Converting the booleantaskInfo.Exclusiveinto a string is straightforward and likely suitable for display.
302-303: Confirm CPU-per-node logic
This function returns onlytask.ReqResView.AllocatableRes.CpuCoreLimit, without multiplying bytask.NodeNum. If “per node” is truly the intention, then it’s correct. Otherwise, verify you aren’t missing a factor oftask.NodeNum.
350-355: AllocMemPerNode calculation
Dividing the total allocated memory by the number of nodes, then converting to MB makes sense. Returning "0" ifNodeNum == 0is a safe fallback.
357-359: ReqMemPerNode calculation
This function’s logic—takingMemoryLimitBytesand converting to MB—matches the “per node” naming, assuming the limit is already per node data.
442-445: Exclusive function
Implementation is straightforward—just returns the string form of theExclusiveboolean.
1daidai1
force-pushed
the
dev/exclusivenew
branch
4 times, most recently
from
9372506 to
bd27a26
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/cqueue/cqueue.go (1)
385-388: Incorrect comment for 'Q' group function.The comment for the 'Q' group has "ProcessReqCpus" appended to it, which appears to be a typo or incomplete edit.
-// 'Q' groupProcessReqCpus +// 'Q' group
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/cacct/CmdArgParser.go(1 hunks)internal/cbatch/cbatch.go(9 hunks)internal/cqueue/CmdArgParser.go(1 hunks)internal/cqueue/cqueue.go(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/cbatch/cbatch.go
- internal/cqueue/CmdArgParser.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/cqueue/cqueue.go (1)
internal/cacct/cacct.go (4)
ProcessReqCPUs(325-327)ProcessReqMemPerNode(409-411)ProcessAllocMemPerNode(414-419)ProcessExclusive(501-503)
🔇 Additional comments (11)
internal/cacct/CmdArgParser.go (2)
119-133: Improved output format documentation with new resource specifiers.The added format specifiers clearly differentiate between requested and allocated resources, making it easier for users to understand resource utilization.
The changes properly follow the existing documentation pattern and provide sufficient explanations for each new specifier.
148-148: Added support for displaying job exclusivity.The new %X/%Exclusive format specifier allows users to see if jobs are running in exclusive mode, which is important for resource accounting and scheduling optimization.
internal/cqueue/cqueue.go (9)
135-136: Enhanced table header with requested and allocated resource fields.The table headers now properly distinguish between requested and allocated resources, and includes the new exclusive flag. This provides users with more comprehensive information about resource requests vs. actual allocations.
181-184: Updated table data to use new processor functions.The implementation correctly uses the new processor functions to display requested and allocated resources.
192-192: Added support for displaying job exclusivity.The exclusive flag is now properly included in the full output format, allowing users to see whether jobs are running in exclusive mode.
302-309: Renamed and updated CPU processing functions.The functions are properly renamed to reflect the distinction between allocated and requested resources. The implementation of
ProcessReqCpuPerNodenow correctly usesReqResViewto get the requested CPU limit per node.
350-360: Added distinct memory processing functions.The implementation now correctly handles both allocated and requested memory with separate functions. The null check in
ProcessAllocMemPerNodeis a good practice to prevent division by zero.
487-490: Memory format specifiers are properly mapped.The memory format specifiers are consistently mapped with 'm' for allocated memory and 'M' for requested memory, which aligns with the documentation in
CmdArgParser.go.
442-445: Added exclusive status processing and mapping.The implementation correctly processes and maps the exclusive flag, using 'X' as the format specifier which is consistent with the documentation in
CmdArgParser.go.Also applies to: 537-538
598-601: Updated format specifier reference.The reference list in the error message has been updated to include all the new format specifiers, which helps users understand the available options.
458-462:❓ Verification inconclusive
Inconsistent mapping between CPU-related format specifiers.
There's an inconsistency in how CPU-related format specifiers are mapped:
- 'C' maps to
ProcessReqCPUs(total CPUs across all nodes)- 'Q' maps to
ProcessReqCpuPerNode(CPUs per node)This could be confusing for users, especially since the previous review noted a similar inconsistency with
cacct.go.Also applies to: 505-509
🏁 Script executed:
#!/bin/bash # Check mapping in other files echo "Checking CPU-related format specifiers in cacct.go:" rg "ProcessReqCPUs|ProcessAllocCpus|ProcessReqCpuPerNode" internal/cacct/ -A 2 -B 2 echo -e "\nChecking format specifier documentation in other command parsers:" rg "ReqCpus|AllocCpus|ReqCpuPerNode" internal/ --type go -A 1 -B 1Length of output: 6306
Attention: Revisit CPU Format Specifier Mapping
The current implementation consistently maps the specifiers across the codebase:
- In both
internal/cqueue/cqueue.goandinternal/cacct/cacct.go, the lowercase"c"(and"alloccpus") maps to"AllocCpus"viaProcessAllocCpus, while the uppercase"C"(and"reqcpus") maps to"ReqCpus"viaProcessReqCPUs—which calculates the total requested CPUs across all nodes.- Separately, the specifier
"Q"(and"reqcpupernode") maps to"ReqCpuPerNode"viaProcessReqCpuPerNode.While this pattern is consistent across modules, the divergence between using
"C"for total requested CPUs and"Q"for per-node requested CPUs may still be nonintuitive for users. This mirrors the mapping noted earlier incacct.go.Points to Verify:
- Confirm that the choice of specifiers (using
"C"for total and"Q"for per-node) is an intentional design decision.- Consider whether renaming or adding clearer documentation for these specifiers might help avoid potential user confusion.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/cacct/cacct.go (2)
408-420: Consider extracting memory conversion to a utility function.The conversion from bytes to MB (dividing by 1024*1024) is duplicated in both memory-related processing functions. Consider extracting this to a utility function for better maintainability.
// In ProcessReqMemPerNode - return strconv.FormatUint(task.ReqResView.AllocatableRes.MemoryLimitBytes/(1024*1024), 10) + return strconv.FormatUint(util.BytesToMB(task.ReqResView.AllocatableRes.MemoryLimitBytes), 10) // In ProcessAllocMemPerNode - return strconv.FormatUint( - task.AllocatedResView.AllocatableRes.MemoryLimitBytes /uint64(task.NodeNum) /(1024*1024), 10) + return strconv.FormatUint( + util.BytesToMB(task.AllocatedResView.AllocatableRes.MemoryLimitBytes /uint64(task.NodeNum)), 10)This would require adding a utility function like:
// In util package func BytesToMB(bytes uint64) uint64 { return bytes / (1024 * 1024) }
324-332: Add nil checks to prevent potential panics.The processing functions don't check if
task.ReqResViewortask.AllocatedResViewis nil before accessing their fields. This could lead to panics if these fields are not set.// In ProcessReqCPUs func ProcessReqCPUs(task *protos.TaskInfo) string { + if task.ReqResView == nil || task.ReqResView.AllocatableRes == nil { + return "0.00" + } return strconv.FormatFloat(task.ReqResView.AllocatableRes.CpuCoreLimit*float64(task.NodeNum), 'f', 2, 64) } // Similar changes for other processing functionsAlso applies to: 408-420
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/cacct/cacct.go(10 hunks)internal/ccontrol/ccontrol.go(4 hunks)internal/ceff/ceff.go(5 hunks)internal/cqueue/cqueue.go(11 hunks)protos/PublicDefs.proto(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- protos/PublicDefs.proto
- internal/ccontrol/ccontrol.go
- internal/ceff/ceff.go
- internal/cqueue/cqueue.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/cacct/cacct.go (1)
internal/cqueue/cqueue.go (4)
ProcessReqCPUs(387-389)ProcessReqMemPerNode(359-361)ProcessAllocMemPerNode(350-356)ProcessExclusive(444-446)
🔇 Additional comments (13)
internal/cacct/cacct.go (13)
164-165: Added comprehensive resource fields to output header.Nice enhancement to the job output header to now include both requested and allocated resource metrics, as well as the exclusive flag. This change provides users with a clearer understanding of both what was requested and what was actually allocated.
213-216: Updated table data output to use new resource metrics.The implementation correctly updates the table output to include both requested and allocated resources, along with the exclusive flag.
Also applies to: 223-223
246-246: Updated CPU allocation display to use AllocatedResView.Correct implementation to use the new
AllocatedResViewstructure instead of the previous Resources field.
324-327: Added ProcessReqCPUs function to display requested CPU cores.The function correctly calculates total requested CPU cores by multiplying per-node CPU limit by the number of nodes.
331-331: Updated ProcessAllocCPUs to use AllocatedResView.This change appropriately updates the function to use the new resource view structure.
408-411: Added ProcessReqMemPerNode function.The implementation correctly returns the requested memory per node in MB.
413-420: Added ProcessAllocMemPerNode with zero division protection.Good implementation with proper error handling to prevent division by zero when node count is zero.
501-504: Added ProcessExclusive function.The function correctly returns the exclusivity flag value.
516-518: Added ReqCpus field processor entries.Correctly maps both the short form
Cand long formreqcpusto the ProcessReqCPUs function.
557-558: Added ReqMemPerNode field processor entries.Correctly maps both the short form
Mand long formreqmempernodeto the ProcessReqMemPerNode function.
561-562: Updated AllocMemPerNode field processor entries.The mapping is correctly updated to use the new ProcessAllocMemPerNode function.
617-618: Added Exclusive field processor entries.Correctly maps both the short form
Xand long formexclusiveto the ProcessExclusive function.
677-679: Updated format specifier documentation.The error message has been updated to include all the new format specifiers, making it easier for users to understand the available options.
1daidai1
force-pushed
the
dev/exclusivenew
branch
from
0776d23 to
4c5dc70
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/cqueue/cqueue.go (1)
508-509: Inconsistent field mapping for CPU fields still present.The field mapping for "Q" to
ProcessReqCpuPerNoderemains inconsistent with the mapping incacct.go. In a previous review comment, it was suggested to use "C" for requested CPU fields to match across files.#!/bin/bash # Search for CPU-related field mappings in cacct.go to verify consistency rg -A 2 "reqcpupernode|ReqCpuPerNode" internal/cacct
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
internal/cacct/CmdArgParser.go(1 hunks)internal/cacct/cacct.go(10 hunks)internal/calloc/CmdArgParser.go(2 hunks)internal/calloc/calloc.go(5 hunks)internal/cbatch/CmdArgParser.go(2 hunks)internal/cbatch/cbatch.go(9 hunks)internal/ccontrol/ccontrol.go(4 hunks)internal/ceff/ceff.go(5 hunks)internal/cqueue/CmdArgParser.go(1 hunks)internal/cqueue/cqueue.go(11 hunks)internal/crun/CmdArgParser.go(2 hunks)internal/crun/crun.go(4 hunks)internal/util/string.go(3 hunks)protos/PublicDefs.proto(5 hunks)
✅ Files skipped from review due to trivial changes (3)
- internal/cbatch/CmdArgParser.go
- internal/cacct/CmdArgParser.go
- internal/cqueue/CmdArgParser.go
🚧 Files skipped from review as they are similar to previous changes (10)
- internal/calloc/CmdArgParser.go
- internal/crun/CmdArgParser.go
- internal/crun/crun.go
- internal/calloc/calloc.go
- internal/ccontrol/ccontrol.go
- protos/PublicDefs.proto
- internal/cbatch/cbatch.go
- internal/util/string.go
- internal/cacct/cacct.go
- internal/ceff/ceff.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/cqueue/cqueue.go (1)
internal/cacct/cacct.go (4)
ProcessReqCPUs(325-327)ProcessReqMemPerNode(409-411)ProcessAllocMemPerNode(414-420)ProcessExclusive(502-504)
🔇 Additional comments (8)
internal/cqueue/cqueue.go (8)
135-136: New columns added to full output.The addition of "ReqCPUs", "ReqMemPerNode", and "Exclusive" columns to the full output properly implements the requested feature for distinguishing between requested and allocated resources and adding support for exclusive node allocation.
302-304: LGTM: Process allocated CPUs per node.The refactored function correctly processes allocated CPUs.
358-360: LGTM: ProcessReqMemPerNode implementation.The function correctly returns the requested memory per node in MB.
386-389: LGTM: ProcessReqCPUs now correctly calculates total requested CPUs.The function correctly multiplies per-node CPU limit by the number of nodes to get total requested CPUs.
443-446: LGTM: New ProcessExclusive function.Properly formats the exclusive flag as a boolean string.
459-463: LGTM: Updated CPU field mappings.The mapping has been updated to correctly associate lowercase 'c' with allocated CPUs and uppercase 'C' with requested CPUs, addressing a previous inconsistency between files.
538-539: LGTM: New exclusive field mapping.Properly maps "X"/"exclusive" to the ProcessExclusive function.
599-607: LGTM: Updated format specifier help text.The help text has been updated to include all the new field specifiers, providing clear guidance for users.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/cqueue/cqueue.go (1)
458-463: Inconsistent naming conventions for CPU resource specifiersThere's an inconsistency in how CPU-related format specifiers are mapped. In this file, 'C' maps to total requested CPUs, while 'Q' maps to CPU per node. However, this naming might be confusing as it doesn't follow a clear pattern (e.g., capital letters for total resources and lowercase for per-node resources).
Consider adopting a more consistent naming convention, such as:
- "C": {"ReqCpus", ProcessReqCPUs}, - "reqcpus": {"ReqCpus", ProcessReqCPUs}, - "Q": {"ReqCpuPerNode", ProcessReqCpuPerNode}, - "reqcpupernode": {"ReqCpuPerNode", ProcessReqCpuPerNode}, + // Requested resources + "C": {"ReqCpusTotal", ProcessReqCPUs}, + "reqcpustotal": {"ReqCpusTotal", ProcessReqCPUs}, + "c": {"ReqCpuPerNode", ProcessReqCpuPerNode}, + "reqcpupernode": {"ReqCpuPerNode", ProcessReqCpuPerNode}, + + // Allocated resources + "A": {"AllocCpusTotal", ProcessAllocCpus}, + "alloccpustotal":{"AllocCpusTotal", ProcessAllocCpus},Also applies to: 505-510
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/cacct/cacct.go(10 hunks)internal/ccontrol/ccontrol.go(4 hunks)internal/ceff/ceff.go(5 hunks)internal/cqueue/cqueue.go(11 hunks)internal/util/string.go(2 hunks)protos/PublicDefs.proto(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- protos/PublicDefs.proto
- internal/util/string.go
- internal/ceff/ceff.go
🧰 Additional context used
🧠 Learnings (2)
internal/cqueue/cqueue.go (1)
Learnt from: NamelessOIer
PR: PKUHPC/CraneSched-FrontEnd#256
File: internal/cqueue/cqueue.go:350-356
Timestamp: 2025-04-24T16:05:47.127Z
Learning: In cqueue.go, the function ProcessAllocMemPerNode should use task.AllocatedResView.AllocatableRes.MemoryLimitBytes (allocated resources) rather than task.ReqResView.AllocatableRes.MemoryLimitBytes (requested resources) to correctly display allocated memory per node.
internal/cacct/cacct.go (1)
Learnt from: NamelessOIer
PR: PKUHPC/CraneSched-FrontEnd#256
File: internal/cqueue/cqueue.go:350-356
Timestamp: 2025-04-24T16:05:47.127Z
Learning: In cqueue.go, the function ProcessAllocMemPerNode should use task.AllocatedResView.AllocatableRes.MemoryLimitBytes (allocated resources) rather than task.ReqResView.AllocatableRes.MemoryLimitBytes (requested resources) to correctly display allocated memory per node.
🧬 Code Graph Analysis (2)
internal/cqueue/cqueue.go (1)
internal/cacct/cacct.go (4)
ProcessReqCPUs(325-327)ProcessReqMemPerNode(409-411)ProcessAllocMemPerNode(414-420)ProcessExclusive(502-504)
internal/cacct/cacct.go (1)
internal/cqueue/cqueue.go (4)
ProcessReqCPUs(387-389)ProcessReqMemPerNode(359-361)ProcessAllocMemPerNode(350-356)ProcessExclusive(444-446)
🪛 golangci-lint (1.64.8)
internal/cacct/cacct.go
246-246: undefined: task
(typecheck)
🔇 Additional comments (13)
internal/ccontrol/ccontrol.go (3)
178-180: Good improvement: Standardized memory formattingThe change to use
util.FormatMemToMBinstead of a local formatting function provides better consistency across the codebase for memory formatting.
417-419: Enhanced resource reporting with clear separation of requested vs allocated resourcesThe output now clearly distinguishes between requested resources (
ReqRes) and allocated resources (AllocRes), making it easier for users to understand what they asked for versus what they actually got.
420-436: Added exclusive flag support in job displayThe job display now includes the exclusive flag status, which aligns with the new
--exclusiveflag support added throughout the system.internal/cacct/cacct.go (5)
164-165: Good enhancement: Expanded output header with resource distinctionThe output header now clearly separates requested resources (ReqCPUs, ReqMemPerNode) from allocated resources (AllocCPUs, AllocMemPerNode) and adds the Exclusive flag, providing users with more detailed information.
213-216: Enhanced resource reporting with relevant processing functionsThe implementation correctly uses the new processing functions to display requested and allocated resource information.
324-327: Good implementation: ProcessReqCPUs functionThe function correctly calculates total requested CPUs as the product of CPU per node and node count.
414-420: Well-implemented allocation memory formatting with zero protectionThe implementation correctly handles the edge case where node count is zero to avoid division by zero errors, and properly uses the allocated resources view.
501-504: Good implementation: ProcessExclusive functionSimple and correct implementation to display the exclusive flag as a boolean string.
internal/cqueue/cqueue.go (5)
135-136: Good enhancement: Updated output format with resource distinctionThe output header has been updated to include requested and allocated resource columns along with the exclusive flag.
181-185: Enhanced resource reporting with separated resource viewsThe implementation correctly displays both requested and allocated resources.
302-304: Properly updated ProcessAllocCpus to use allocated resourcesThe function now correctly uses AllocatedResView instead of ReqResView to display actually allocated CPUs.
350-356: Correctly implemented memory per node with zero protectionThe implementation handles the edge case where node count is zero to avoid division by zero errors, and properly uses allocated resources view as noted in your previous feedback.
599-602: Updated format specifier documentationThe documentation for format specifiers has been properly updated to include the new resource view specifiers and exclusive flag.
1daidai1
force-pushed
the
dev/exclusivenew
branch
4 times, most recently
from
f556a8a to
5cd246c
Compare
1daidai1
force-pushed
the
dev/exclusivenew
branch
2 times, most recently
from
619c191 to
9c1b9fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/cqueue/cqueue.go (3)
596-598: Inconsistent capitalization in format specifier documentation.There's inconsistent capitalization in the format specifier documentation:
- Line 597: "AllocCPUs" vs "ReqCpus" (upper vs mixed case for "CPU")
- Line 598: "ReqCpuPerNode" (mixed case)
This could be confusing to users. Consider standardizing to either all uppercase ("CPU") or all mixed case ("Cpu").
- //a-Account, c-AllocCPUs, C-ReqCpus, e-ElapsedTime, h-Held, j-JobID, l-TimeLimit, L-NodeList, k-Comment, - //m-AllocMemPerNode, M-ReqMemPerNode, n-Name, N-NodeNum, p-Priority, P-Partition, q-Qos, Q-ReqCpuPerNode, r-ReqNodes, + //a-Account, c-AllocCpus, C-ReqCpus, e-ElapsedTime, h-Held, j-JobID, l-TimeLimit, L-NodeList, k-Comment, + //m-AllocMemPerNode, M-ReqMemPerNode, n-Name, N-NodeNum, p-Priority, P-Partition, q-Qos, Q-ReqCpuPerNode, r-ReqNodes,
456-457: Inconsistency between function name and field name capitalization.The fieldMap maps "c" to
{"AllocCpus", ProcessAllocCpus}, but the function is namedProcessAllocCpus(with mixed case "Cpus") while the field is named "AllocCpus" (with uppercase "C"). This inconsistency appears in several places throughout the code.Consider standardizing the capitalization pattern across function names and field names.
601-604: Use hyphen for connecting format specifiers in error message comment.Based on the previous review comment from NamelessOIer, the format for connecting format specifiers in the error message should use a hyphen ("-") rather than a slash ("/").
- "a/Account, c/AllocCPUs, C/ReqCpus, e/ElapsedTime, h/Held, j/JobID, l/TimeLimit, L/NodeList, k/Comment,\n"+ - "m/AllocMemPerNode, M/ReqMemPerNode, n/Name, N/NodeNum, p/Priority, P/Partition, q/Qos, Q/ReqCpuPerNode, r/ReqNodes,\n"+ - "R/Reason, s/SubmitTime, S/StartTime, t/State, T/JobType, u/User, U/Uid, x/ExcludeNodes, X/Exclusive.", field) + "a-Account, c-AllocCpus, C-ReqCpus, e-ElapsedTime, h-Held, j-JobID, l-TimeLimit, L-NodeList, k-Comment,\n"+ + "m-AllocMemPerNode, M-ReqMemPerNode, n-Name, N-NodeNum, p-Priority, P-Partition, q-Qos, Q-ReqCpuPerNode, r-ReqNodes,\n"+ + "R-Reason, s-SubmitTime, S-StartTime, t-State, T-JobType, u-User, U-Uid, x-ExcludeNodes, X-Exclusive.", field)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/cacct/cacct.go(10 hunks)internal/ccontrol/ccontrol.go(4 hunks)internal/ceff/ceff.go(5 hunks)internal/cqueue/cqueue.go(11 hunks)internal/util/string.go(2 hunks)protos/PublicDefs.proto(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/ceff/ceff.go
- protos/PublicDefs.proto
- internal/ccontrol/ccontrol.go
- internal/util/string.go
- internal/cacct/cacct.go
🧰 Additional context used
🧠 Learnings (1)
internal/cqueue/cqueue.go (1)
Learnt from: NamelessOIer
PR: PKUHPC/CraneSched-FrontEnd#256
File: internal/cqueue/cqueue.go:350-356
Timestamp: 2025-04-24T16:05:47.127Z
Learning: In cqueue.go, the function ProcessAllocMemPerNode should use task.AllocatedResView.AllocatableRes.MemoryLimitBytes (allocated resources) rather than task.ReqResView.AllocatableRes.MemoryLimitBytes (requested resources) to correctly display allocated memory per node.
🔇 Additional comments (3)
internal/cqueue/cqueue.go (3)
350-353: Confirm memory calculation logic is correct.I see that
ProcessAllocMemPerNodenow correctly usesAllocatedResViewinstead ofReqResView, which is consistent with the function's purpose. This addresses the issue identified in previous reviews.However, unlike
ProcessAllocCpuswhich multiplies bytask.NodeNum, this function doesn't divide bytask.NodeNumto get the memory per node. Please verify that this is indeed the expected behavior.
134-137: Changes look good - clear separation of requested vs allocated resources.The header changes appropriately add columns for "ReqCPUs", "ReqMemPerNode", and "Exclusive" which aligns with the PR objectives of distinguishing requested and allocated resources more clearly and adding the exclusive flag.
440-443: Properly implemented exclusive flag handling.The
ProcessExclusivefunction correctly returns the string representation of the task's exclusive flag, which is consistent with the PR objective.
1daidai1
force-pushed
the
dev/exclusivenew
branch
2 times, most recently
from
e13ee35 to
cd6e89f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
internal/cqueue/cqueue.go (3)
459-463: Inconsistency in CPU field mappings.There remains an inconsistency in the mapping of format specifiers between this file and
cacct.go. In this file:
- 'c' maps to
ProcessAllocCpus(allocated CPUs)- 'C' maps to
ProcessReqCPUs(total requested CPUs)- 'Q' maps to
ProcessReqCpuPerNode(requested CPUs per node)While in
cacct.go, 'C' maps toProcessReqCPUsand 'c' maps toProcessAllocCPUs. This difference could confuse users when switching between commands.Also applies to: 506-509
599-601: Missing hyphen in comment formatting.The comments for format specifiers are missing a hyphen between character and description in some places, making the format inconsistent with the standard used elsewhere in the codebase.
-//a-Account, c-AllocCPUs, C-ReqCpus, e-ElapsedTime, h-Held, j-JobID, l-TimeLimit, L-NodeList, k-Comment, -//m-AllocMemPerNode, M-ReqMemPerNode, n-Name, N-NodeNum, p-Priority, P-Partition, q-Qos, Q-ReqCpuPerNode, r-ReqNodes, -//R-Reason, s-SubmitTime, S-StartTime, t-State, T-JobType, u-User, U-Uid, x-ExcludeNodes, X-Exclusive. +//a-Account, c-AllocCPUs, C-ReqCpus, e-ElapsedTime, h-Held, j-JobID, l-TimeLimit, L-NodeList, k-Comment, +//m-AllocMemPerNode, M-ReqMemPerNode, n-Name, N-NodeNum, p-Priority, P-Partition, q-Qos, Q-ReqCpuPerNode, r-ReqNodes, +//R-Reason, s-SubmitTime, S-StartTime, t-State, T-JobType, u-User, U-Uid, x-ExcludeNodes, X-Exclusive.
606-607: Missing hyphen in help text.Similar to the above comment, the help text in the error message is missing hyphens between format specifiers and their descriptions.
- "a/Account, c/AllocCPUs, C/ReqCpus, e/ElapsedTime, h/Held, j/JobID, l/TimeLimit, L/NodeList, k/Comment,\n"+ - "m/AllocMemPerNode, M/ReqMemPerNode, n/Name, N/NodeNum, p/Priority, P/Partition, q/Qos, Q/ReqCpuPerNode, r/ReqNodes,\n"+ - "R/Reason, s/SubmitTime, S/StartTime, t/State, T/JobType, u/User, U/Uid, x/ExcludeNodes, X/Exclusive.", field) + "a-Account, c-AllocCPUs, C-ReqCpus, e-ElapsedTime, h-Held, j-JobID, l-TimeLimit, L-NodeList, k-Comment,\n"+ + "m-AllocMemPerNode, M-ReqMemPerNode, n-Name, N-NodeNum, p-Priority, P-Partition, q-Qos, Q-ReqCpuPerNode, r-ReqNodes,\n"+ + "R-Reason, s-SubmitTime, S-StartTime, t-State, T-JobType, u-User, U-Uid, x-ExcludeNodes, X-Exclusive.", field)
🧹 Nitpick comments (1)
internal/cqueue/cqueue.go (1)
302-309: Consider adding function comments.The CPU processing functions lack descriptive comments explaining what they do. Adding comments would improve code readability and make it clearer that
ProcessAllocCpusreturns allocated CPU cores whileProcessReqCpuPerNodereturns requested CPU cores per node.// 'c' group +// ProcessAllocCpus returns the allocated CPU cores for a task func ProcessAllocCpus(task *protos.TaskInfo) string { return strconv.FormatFloat(task.AllocatedResView.AllocatableRes.CpuCoreLimit, 'f', 2, 64) } // 'C' group +// ProcessReqCpuPerNode returns the requested CPU cores per node for a task func ProcessReqCpuPerNode(task *protos.TaskInfo) string { return strconv.FormatFloat(task.ReqResView.AllocatableRes.CpuCoreLimit, 'f', 2, 64) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/cacct/cacct.go(10 hunks)internal/ccontrol/ccontrol.go(4 hunks)internal/ceff/ceff.go(5 hunks)internal/cqueue/cqueue.go(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/ccontrol/ccontrol.go
- internal/ceff/ceff.go
- internal/cacct/cacct.go
🧰 Additional context used
🧠 Learnings (1)
internal/cqueue/cqueue.go (1)
Learnt from: NamelessOIer
PR: PKUHPC/CraneSched-FrontEnd#256
File: internal/cqueue/cqueue.go:350-356
Timestamp: 2025-04-24T16:05:47.127Z
Learning: In cqueue.go, the function ProcessAllocMemPerNode should use task.AllocatedResView.AllocatableRes.MemoryLimitBytes (allocated resources) rather than task.ReqResView.AllocatableRes.MemoryLimitBytes (requested resources) to correctly display allocated memory per node.
🧬 Code Graph Analysis (1)
internal/cqueue/cqueue.go (2)
internal/cacct/cacct.go (4)
ProcessReqCPUs(325-327)ProcessReqMemPerNode(409-411)ProcessAllocMemPerNode(414-420)ProcessExclusive(502-504)internal/util/string.go (1)
FormatMemToMB(940-947)
🔇 Additional comments (5)
internal/cqueue/cqueue.go (5)
350-356: Correct implementation ofProcessAllocMemPerNode.Your change to use
AllocatedResView.AllocatableRes.MemoryLimitBytesis correct, ensuring the function returns allocated rather than requested memory per node. This aligns with the function name and ensures accurate display of allocated resources.I notice you're returning "0M" for the zero node case, which is good for avoiding division by zero.
358-360: Memory formatting implementation looks good.The
ProcessReqMemPerNodefunction correctly uses the utility functionFormatMemToMBto format the requested memory per node, which ensures consistent memory formatting throughout the application.
386-389: Proper implementation of requested CPUs calculation.The function correctly calculates total requested CPUs by multiplying the per-node CPU core limit by the number of nodes, which is essential for displaying the total CPU resources requested by a task.
443-446: Added exclusive flag processing.The new
ProcessExclusivefunction correctly formats the task's exclusive flag as a string, which is needed for displaying whether a task requests exclusive node access.
135-136: Correctly implemented new columns in table output.The implementation correctly adds new columns for "ReqCPUs", "ReqMemPerNode", and "Exclusive" to distinguish between requested and allocated resources, and to display the new exclusive flag. The table data is also properly populated with the corresponding values.
Also applies to: 181-184, 192-192
1daidai1
force-pushed
the
dev/exclusivenew
branch
from
fe53fa9 to
e4d7efd
Compare
1daidai1
force-pushed
the
dev/exclusivenew
branch
from
e4d7efd to
be15514
Compare
Summary by CodeRabbit
New Features
Refactor