feat: cbatch --deadline #341
Conversation
WalkthroughAdds end-to-end deadline support: protobuf fields and enums, RPC renames for time-constraint APIs, CLI flags across clients, parsing/wiring of deadline timestamps into task messages, display processors/columns, and control-plane ModifyTask deadline update. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI
participant Ctld as Ctld(gRPC)
participant Supervisor
rect rgb(245,250,255)
Note over User,CLI: User supplies --deadline or update param deadline=<t>
end
User->>CLI: run submit/update with deadline
CLI->>CLI: parse deadline string (util.ParseTime)
CLI->>CLI: create google.protobuf.Timestamp (timestamppb.New)
CLI->>Ctld: SubmitTask / ModifyTask (include deadline_time)
Ctld->>Supervisor: forward deadline_time in internal RPC
Ctld-->>CLI: Reply (ok / error)
alt success
CLI->>User: print success
else parse/RPC error
CLI->>User: print error (invalid deadline / ERR_INVALID_DEADLINE)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
internal/cbatch/cbatch.go (2)
125-131: Remove redundant error logging.Line 128 logs an error that's immediately returned with proper formatting on line 129. This redundant logging is unnecessary since the error message already provides sufficient context.
Apply this diff to remove redundant logging:
case "--deadline": deadlineTime, err := util.ParseTime(arg.val) if err != nil { - log.Errorf("Invalid argument: %v in script: %v", arg.name, err) return nil, fmt.Errorf("invalid argument: %s value '%s' in script: %w", arg.name, arg.val, err) } task.DeadlineTime = timestamppb.New(deadlineTime)
313-320: Remove redundant error logging.Line 316 logs an error that's immediately returned with proper formatting on line 317. This redundant logging is unnecessary since the error message already provides sufficient context.
Apply this diff to remove redundant logging:
if FlagDeadlineTime != "" { deadlineTime, err := util.ParseTime(FlagDeadlineTime) if err != nil { - log.Errorf("Invalid argument: invalid --deadline: %v", err) return nil, fmt.Errorf("invalid argument: invalid --deadline value '%s': %w", FlagDeadlineTime, err) } task.DeadlineTime = timestamppb.New(deadlineTime) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
internal/cacct/cacct.go(7 hunks)internal/cacct/cmd.go(3 hunks)internal/cbatch/cbatch.go(2 hunks)internal/cbatch/cmd.go(2 hunks)internal/cbatch/line.go(1 hunks)internal/ccontrol/ccontrol.go(6 hunks)internal/ccontrol/cmd.go(3 hunks)internal/ccontrol/help.go(1 hunks)internal/cqueue/cmd.go(3 hunks)internal/cqueue/format.go(2 hunks)internal/cqueue/output.go(3 hunks)protos/Crane.proto(4 hunks)protos/PublicDefs.proto(5 hunks)protos/Supervisor.proto(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
internal/ccontrol/cmd.go (5)
internal/ccontrol/ccontrol.go (2)
DeadlineTypeFlag(55-55)ChangeDeadlineTime(638-680)internal/cbatch/cmd.go (1)
FlagDeadlineTime(70-70)internal/cacct/cmd.go (1)
FlagDeadlineTime(44-44)internal/cqueue/cmd.go (1)
FlagDeadlineTime(32-32)internal/util/err.go (1)
ErrorGeneric(35-35)
internal/cbatch/cmd.go (3)
internal/cacct/cmd.go (1)
FlagDeadlineTime(44-44)internal/cqueue/cmd.go (1)
FlagDeadlineTime(32-32)internal/ccontrol/cmd.go (1)
FlagDeadlineTime(53-53)
internal/cacct/cmd.go (3)
internal/cbatch/cmd.go (1)
FlagDeadlineTime(70-70)internal/cqueue/cmd.go (1)
FlagDeadlineTime(32-32)internal/ccontrol/cmd.go (1)
FlagDeadlineTime(53-53)
internal/cbatch/cbatch.go (2)
internal/util/string.go (1)
ParseTime(377-436)internal/cbatch/cmd.go (1)
FlagDeadlineTime(70-70)
internal/cqueue/output.go (4)
internal/cacct/cacct.go (1)
ProcessDeadline(388-394)internal/cqueue/format.go (1)
ProcessDeadline(33-39)internal/cacct/cmd.go (1)
FlagDeadlineTime(44-44)internal/cqueue/cmd.go (1)
FlagDeadlineTime(32-32)
internal/cqueue/format.go (1)
internal/cacct/cacct.go (1)
ProcessDeadline(388-394)
internal/cqueue/cmd.go (3)
internal/cbatch/cmd.go (1)
FlagDeadlineTime(70-70)internal/cacct/cmd.go (1)
FlagDeadlineTime(44-44)internal/ccontrol/cmd.go (1)
FlagDeadlineTime(53-53)
internal/cacct/cacct.go (3)
internal/cacct/cmd.go (1)
FlagDeadlineTime(44-44)internal/cqueue/cmd.go (1)
FlagDeadlineTime(32-32)internal/cqueue/format.go (1)
ProcessDeadline(33-39)
internal/ccontrol/ccontrol.go (5)
internal/util/string.go (2)
ParseTime(377-436)ParseJobIdList(1066-1078)internal/util/err.go (4)
CraneError(43-47)ErrorCmdArg(36-36)ErrorNetwork(37-37)ErrorBackend(38-38)internal/util/grpc.go (1)
GrpcErrorPrintf(387-398)internal/ccontrol/cmd.go (1)
FlagJson(45-45)internal/util/formatter.go (1)
FmtJson(58-58)
🔇 Additional comments (12)
internal/ccontrol/help.go (1)
67-75: LGTM! Clear documentation for the new deadline parameter.The help text additions are well-formatted and consistent with the existing parameter documentation style.
protos/PublicDefs.proto (2)
118-118: LGTM! New task status enum value added correctly.The
Deadline = 7status value fits logically with the existing task termination states (Completed, Failed, ExceedTimeLimit, Cancelled, OutOfMemory).
174-174: LGTM! Deadline fields added consistently across protocol messages.The
deadline_timefields are added to all relevant task-related messages (TaskToCtld, TaskToD, TaskInfo) with appropriate field numbers and consistent use ofgoogle.protobuf.Timestamptype.Also applies to: 221-221, 336-336
internal/cbatch/line.go (1)
67-67: LGTM! Deadline option properly registered for Slurm compatibility.The
--deadlineflag is correctly added to the supported options map, enabling batch scripts to specify deadlines via#SBATCH --deadlinedirectives.internal/cqueue/cmd.go (2)
32-32: LGTM! Deadline display flag added appropriately.The boolean
FlagDeadlineTimeis correctly used as a display toggle for cqueue (to show/hide the deadline column), which differs appropriately from cbatch/ccontrol where it's a string value to set.Also applies to: 99-99
116-116: LGTM! Format specifier documentation added clearly.The
%deadline/%Deadlineformat tokens are documented consistently with the existing format specifier help text.internal/cqueue/format.go (1)
199-200: LGTM! Deadline field processor registered correctly.The deadline field is properly added to the field map with appropriate header and processor function.
internal/cqueue/output.go (2)
55-55: LGTM! Deadline column integrated into full table mode.The deadline column is correctly added to both the header and row data for full-mode display.
Also applies to: 84-84
129-134: LGTM! Dynamic deadline column properly gated by flag.The conditional deadline column addition follows the established pattern for dynamic columns (like StartTime and QoS).
internal/cacct/cmd.go (2)
44-44: LGTM! Deadline display flag added consistently.The boolean flag for toggling deadline visibility is implemented consistently with the cqueue module.
Also applies to: 111-111
126-126: LGTM! Format specifier documentation added.The deadline format token is documented clearly in the help text.
internal/cbatch/cmd.go (1)
69-70: LGTM! Deadline flag added appropriately for batch submission.The string-typed
FlagDeadlineTimeis correctly used to accept a deadline value for job submission, which appropriately differs from the boolean display flags in cqueue/cacct.Also applies to: 160-160
L-Xiafeng
left a comment
L-Xiafeng
left a comment
There was a problem hiding this comment.
crun/calloc没加deadline选项
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/cacct/cacct.go(7 hunks)internal/calloc/calloc.go(2 hunks)internal/calloc/cmd.go(3 hunks)internal/cqueue/format.go(2 hunks)internal/crun/cmd.go(3 hunks)internal/crun/crun.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/cqueue/format.go
🧰 Additional context used
🧬 Code graph analysis (5)
internal/calloc/cmd.go (5)
internal/crun/cmd.go (1)
FlagDeadlineTime(62-62)internal/cbatch/cmd.go (1)
FlagDeadlineTime(70-70)internal/cacct/cmd.go (1)
FlagDeadlineTime(44-44)internal/cqueue/cmd.go (1)
FlagDeadlineTime(32-32)internal/ccontrol/cmd.go (1)
FlagDeadlineTime(53-53)
internal/crun/crun.go (2)
internal/crun/cmd.go (1)
FlagDeadlineTime(62-62)internal/util/string.go (1)
ParseTime(377-436)
internal/cacct/cacct.go (5)
internal/cqueue/format.go (2)
ProcessDeadline(33-40)InfiniteFuture(35-35)internal/calloc/cmd.go (1)
FlagDeadlineTime(56-56)internal/crun/cmd.go (1)
FlagDeadlineTime(62-62)internal/cacct/cmd.go (1)
FlagDeadlineTime(44-44)internal/cqueue/cmd.go (1)
FlagDeadlineTime(32-32)
internal/calloc/calloc.go (2)
internal/calloc/cmd.go (1)
FlagDeadlineTime(56-56)internal/util/string.go (1)
ParseTime(377-436)
internal/crun/cmd.go (3)
internal/calloc/cmd.go (2)
RootCmd(58-69)FlagDeadlineTime(56-56)internal/cbatch/cmd.go (2)
RootCmd(72-117)FlagDeadlineTime(70-70)internal/ccontrol/cmd.go (1)
FlagDeadlineTime(53-53)
🔇 Additional comments (5)
internal/calloc/cmd.go (1)
55-56: LGTM!The new
FlagDeadlineTimevariable and--deadlineflag are properly declared and registered.internal/crun/cmd.go (1)
61-62: LGTM!The new
FlagDeadlineTimevariable and--deadlineflag are properly declared and registered.internal/cacct/cacct.go (3)
185-250: LGTM!The deadline column is correctly integrated into the full output display, with proper formatting via
ProcessDeadline.
305-310: LGTM!The conditional append of the Deadline column when
FlagDeadlineTimeis set provides users control over display verbosity in non-full mode.
567-567: LGTM!The deadline field processor is correctly registered, enabling format string support.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/crun/crun.go (1)
1126-1135: Fix error message format for consistency.The error handling correctly addresses the previous review concern by properly returning errors instead of silently ignoring them. However, the error message format is inconsistent with similar error messages in this file.
Compare:
- Line 1057:
"Invalid argument: invalid --time: %s."- Line 1131:
"Invalid argument: --deadline: %s"Apply this diff for consistency:
return &util.CraneError{ Code: util.ErrorCmdArg, - Message: fmt.Sprintf("Invalid argument: --deadline: %s", err), + Message: fmt.Sprintf("Invalid argument: invalid --deadline: %s.", err), }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/calloc/calloc.go(2 hunks)internal/cbatch/cbatch.go(2 hunks)internal/crun/crun.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/calloc/calloc.go
- internal/cbatch/cbatch.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/crun/crun.go (3)
internal/crun/cmd.go (1)
FlagDeadlineTime(62-62)internal/util/string.go (1)
ParseTime(377-436)internal/util/err.go (2)
CraneError(43-47)ErrorCmdArg(36-36)
🔇 Additional comments (1)
internal/crun/crun.go (1)
48-48: LGTM!The
timestamppbimport is correctly added and used for converting the parsed deadline time to a protobuf timestamp.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
internal/cqueue/format.go (1)
32-39: Inconsistency: "unknown" vs "InfiniteFuture".The
ProcessDeadlinefunction in this file returns"unknown"for infinite deadlines (line 38), but the identical function ininternal/cacct/cacct.go(line 389) returns"InfiniteFuture"for the same condition. This creates inconsistent UX where users see different representations depending on which tool they use.As noted in previous reviews, consider standardizing on a single return value across both implementations.
internal/cacct/cacct.go (1)
383-390: Code duplication with cqueue/format.go.The
ProcessDeadlinefunction duplicates nearly identical logic found ininternal/cqueue/format.go(lines 32-39). The only difference is the return value for the infinite deadline case: this file returns"InfiniteFuture"while cqueue returns"unknown".As noted in previous reviews, extract the shared logic into a common utility function (e.g., in
internal/util) to eliminate duplication and ensure consistency.
🧹 Nitpick comments (1)
internal/util/util.go (1)
110-110: Add parentheses for clarity.While the expression
1<<63-1is technically correct due to operator precedence (shift has higher precedence than subtraction), it may be misread at a glance.Apply this diff to make the precedence explicit:
- InfiniteFuture = time.Unix(1<<63-1, 0) + InfiniteFuture = time.Unix((1<<63)-1, 0)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/cacct/cacct.go(7 hunks)internal/cqueue/format.go(2 hunks)internal/util/util.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/cacct/cacct.go (4)
internal/cqueue/format.go (1)
ProcessDeadline(33-39)internal/cacct/cmd.go (1)
FlagDeadlineTime(44-44)internal/cqueue/cmd.go (1)
FlagDeadlineTime(32-32)internal/util/util.go (1)
InfiniteFuture(110-110)
internal/cqueue/format.go (2)
internal/cacct/cacct.go (1)
ProcessDeadline(384-390)internal/util/util.go (1)
InfiniteFuture(110-110)
🔇 Additional comments (7)
internal/cqueue/format.go (1)
199-200: LGTM—deadline field registered correctly.The "deadline" specifier is properly registered in the field map with the correct header and processor function.
internal/cacct/cacct.go (5)
185-185: LGTM—Deadline column added to full header.The "Deadline" column is correctly appended to the full-mode table header, providing visibility into task deadlines.
226-227: LGTM—Deadline properly integrated into full-mode rows.The deadline is correctly formatted via
ProcessDeadlineand appended to each table row in full mode.Also applies to: 249-250
305-310: LGTM—Conditional deadline column logic is correct.The deadline column is conditionally appended when
FlagDeadlineTimeis set, allowing users to opt into viewing deadline information in non-full mode.
566-566: LGTM—Deadline processor registered correctly.The "deadline" field is properly registered in the
fieldProcessorsmap, enabling custom format string support for deadline fields.
736-736: LGTM—Help text updated to include deadline specifier.The format specifier help text correctly documents the new "deadline/Deadline" option.
internal/util/util.go (1)
110-110: Sentinel value change needs backend verification.The change from
time.Date(9999, 12, 31, 23, 59, 59, 999999999, time.UTC)totime.Unix(1<<63-1, 0)represents a significant difference:
- Old value: ~253 billion seconds (year 9999)
- New value: ~9.2 quintillion seconds (year 292277026596)
The constant
MaxJobTimeStamp = 253402300799remains defined and is used for validation ininternal/ccontrol/ccontrol.go:413. TheInfiniteFutureconstant is used only for read-side comparisons in display formatting (internal/cqueue/format.go:35andinternal/cacct/cacct.go:386).Risk: If the backend initializes or returns
deadline_timeusing the historical sentinel value (9999-12-31 23:59:59), the equality checks will fail, breaking deadline display logic. No backend initialization code or migration logic was found in the codebase to confirm this behavior.Action: Verify with backend integration or API documentation whether:
- Backend ever sets deadline_time to the old sentinel (253402300799)
- Existing tasks in production use this value
- Migration or compatibility handling is required
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/cacct/cacct.go (1)
384-390: Code duplication already flagged.This implementation duplicates logic in
internal/cqueue/format.go(lines 32-38). A previous review comment already suggested extracting this to a shared utility function ininternal/util/format.go.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/cacct/cacct.go(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/cacct/cacct.go (3)
internal/cqueue/format.go (1)
ProcessDeadline(33-39)internal/cacct/cmd.go (1)
FlagDeadlineTime(44-44)internal/util/util.go (1)
InfiniteFuture(110-110)
🔇 Additional comments (4)
internal/cacct/cacct.go (4)
185-185: LGTM: Deadline column integrated into full output.The deadline field is correctly added to the full table header and data, following the same pattern as other time-based fields.
Also applies to: 226-227, 249-250
305-310: LGTM: Conditional deadline column logic is correct.The implementation correctly mirrors the pattern used for other conditional time fields (StartTime, EndTime, SubmitTime).
566-566: LGTM: Field processor entry is correct.The "deadline" mapping is properly registered and follows the established pattern.
736-736: LGTM: Help text updated.The format specifier documentation correctly includes the new deadline option.
internal/ccontrol/ccontrol.go
Outdated
| } | ||
|
|
||
| timeDeadline := taskInfo.DeadlineTime.AsTime() | ||
| if !timeDeadline.Before(timeStart) { |
calloc --deadline
edragain2nd
force-pushed
the
deadline
branch
from
e12ad78 to
ff98b06
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/cqueue/format.go (1)
32-39: Duplication with internal/cacct/cacct.go resolved from past review.The implementation now correctly returns "unknown" instead of an empty string, which addresses the previous inconsistency. However, this function remains duplicated with an identical implementation in
internal/cacct/cacct.go(lines 394-400).Consider extracting this common logic to a shared utility package to eliminate duplication and improve maintainability. Based on learnings.
internal/cacct/cacct.go (1)
394-401: Fix misleading comment.The comment
// Deadline (D)is incorrect. The format specifier "D" is already used for ElapsedTime (line 375). The actual format specifier for deadline is "deadline" (full word), as registered on line 577.Apply this diff:
-// Deadline (D) +// Deadline (deadline) func ProcessDeadline(task *protos.TaskInfo) string {Additionally, this function is duplicated with an identical implementation in
internal/cqueue/format.go(lines 32-39). Consider extracting to a shared utility package to eliminate duplication. Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
internal/cacct/cacct.go(7 hunks)internal/cacct/cmd.go(3 hunks)internal/calloc/calloc.go(2 hunks)internal/calloc/cmd.go(3 hunks)internal/cbatch/cbatch.go(2 hunks)internal/cbatch/cmd.go(2 hunks)internal/cbatch/line.go(1 hunks)internal/ccontrol/ccontrol.go(3 hunks)internal/ccontrol/cmd.go(3 hunks)internal/ccontrol/help.go(1 hunks)internal/cqueue/cmd.go(3 hunks)internal/cqueue/format.go(2 hunks)internal/cqueue/output.go(3 hunks)internal/crun/cmd.go(3 hunks)internal/crun/crun.go(2 hunks)internal/util/err.go(1 hunks)internal/util/util.go(2 hunks)protos/Crane.proto(4 hunks)protos/PublicDefs.proto(14 hunks)protos/Supervisor.proto(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- internal/cacct/cmd.go
- internal/ccontrol/cmd.go
- internal/calloc/cmd.go
- internal/crun/crun.go
- protos/Supervisor.proto
- internal/crun/cmd.go
- internal/cbatch/cmd.go
- internal/util/util.go
- internal/calloc/calloc.go
- internal/cbatch/cbatch.go
🧰 Additional context used
🧬 Code graph analysis (5)
internal/cqueue/format.go (2)
internal/cacct/cacct.go (1)
ProcessDeadline(395-401)internal/util/util.go (1)
InfiniteFuture(112-112)
internal/cacct/cacct.go (3)
internal/cqueue/format.go (1)
ProcessDeadline(33-39)internal/cacct/cmd.go (1)
FlagDeadlineTime(45-45)internal/util/util.go (1)
InfiniteFuture(112-112)
internal/cqueue/cmd.go (5)
internal/calloc/cmd.go (1)
FlagDeadlineTime(56-56)internal/crun/cmd.go (1)
FlagDeadlineTime(62-62)internal/cbatch/cmd.go (1)
FlagDeadlineTime(68-68)internal/cacct/cmd.go (1)
FlagDeadlineTime(45-45)internal/ccontrol/cmd.go (1)
FlagDeadlineTime(53-53)
internal/cqueue/output.go (2)
internal/cqueue/format.go (1)
ProcessDeadline(33-39)internal/cqueue/cmd.go (1)
FlagDeadlineTime(32-32)
internal/ccontrol/ccontrol.go (5)
internal/util/string.go (2)
ParseTime(377-436)ParseJobIdList(1098-1110)internal/util/err.go (4)
CraneError(43-47)ErrorCmdArg(36-36)ErrorNetwork(37-37)ErrorBackend(38-38)internal/util/grpc.go (1)
GrpcErrorPrintf(387-398)internal/ccontrol/cmd.go (1)
FlagJson(45-45)internal/util/formatter.go (1)
FmtJson(58-58)
🔇 Additional comments (27)
internal/cbatch/line.go (1)
67-67: LGTM!The addition of
"--deadline": trueto the supported Slurm options map is correctly placed and follows the existing pattern for other Slurm flags.internal/cqueue/output.go (3)
55-55: LGTM!The "Deadline" column is correctly added to the full header list, maintaining consistency with the full row data mapping.
84-84: LGTM!The deadline value is correctly appended to the full row data using
ProcessDeadline(task), which properly formats the deadline timestamp.
129-134: LGTM!The conditional Deadline column addition in non-full mode follows the same pattern as other dynamic columns (StartTime, QoS) and correctly uses the
FlagDeadlineTimeflag to control visibility.internal/cqueue/cmd.go (3)
32-32: LGTM!The
FlagDeadlineTimeboolean flag is correctly declared alongside other flag variables.
102-102: LGTM!The
--deadlineflag is properly registered with appropriate description and follows the same pattern as other boolean flags in the command.
119-119: LGTM!The format documentation correctly describes the new
%deadline/%Deadlineplaceholder and its purpose for displaying job deadline times.internal/ccontrol/help.go (2)
67-67: LGTM!The help text correctly documents the new
deadline=<deadline>parameter in the update command syntax, following the same pattern as other optional parameters.
75-75: LGTM!The parameter description clearly explains the purpose of the deadline parameter.
internal/cqueue/format.go (1)
199-200: LGTM!The "deadline" field mapping is correctly registered with header "Deadline" and the ProcessDeadline function.
internal/ccontrol/ccontrol.go (3)
33-33: LGTM!The import of
timestamppbis necessary for setting the deadline timestamp in the ModifyTaskRequest.
49-49: LGTM!The
DeadlineTypeFlagconstant is correctly added to theUpdateJobParamFlagsenum using the bitwise shift pattern.
132-174: LGTM!The
ChangeDeadlineTimefunction correctly implements deadline modification:
- Parses the deadline string using
util.ParseTime- Validates task IDs with proper error handling
- Constructs the ModifyTaskRequest with the Deadline attribute
- Uses
timestamppb.New(time)to convert the parsed time- Handles JSON output and error responses consistently with other modify operations
The implementation follows the same pattern as
ChangeTaskTimeLimitand integrates properly with the existing error handling framework.internal/cacct/cacct.go (5)
196-196: LGTM!The "Deadline" column is correctly added to the full header array in the appropriate position.
237-261: LGTM!The deadline processing is correctly integrated:
- Line 237 calls
ProcessDeadline(taskInfo)to format the deadline- Line 261 includes the deadline string in the table data row
The implementation properly handles deadline formatting for the full display mode.
316-321: LGTM!The conditional deadline column addition in non-full mode correctly uses the
FlagDeadlineTimeflag and appliesProcessDeadlineto each task, following the same pattern as other conditional columns.
577-577: LGTM!The "deadline" field mapping is correctly registered in the fieldProcessors map with header "Deadline" and the ProcessDeadline function.
747-747: LGTM!The format specifier error message is properly updated to include "deadline/Deadline" in the list of supported identifiers.
protos/Crane.proto (5)
151-155: LGTM!The message rename from
ChangeJobTimeLimitRequesttoChangeJobTimeConstraintRequestappropriately reflects the expanded scope. The changes are backward-compatible:
time_limit_secondsis now optional (line 153)- New
optional int64 deadline_timefield added (line 154)- Field numbering is correct and non-conflicting
157-159: LGTM!The reply message is correctly renamed to match the request message.
234-234: LGTM!The
Deadline = 3enum value is correctly added toTargetAttributeswithout conflicting with existing values.
250-250: LGTM!The
google.protobuf.Timestamp deadline_time = 8field is correctly added to the oneof value with proper field numbering that doesn't conflict with existing fields.
1074-1074: LGTM!The RPC rename from
ChangeJobTimeLimittoChangeJobTimeConstraintcorrectly reflects the expanded functionality to handle both time limits and deadlines.protos/PublicDefs.proto (4)
121-121: TaskStatus enum correctly extended.The addition of
Deadline = 10supports explicit deadline-related status tracking, complementing the new deadline_time fields across message types.
178-178: Deadline timestamp fields properly integrated.The
deadline_timefields are consistently added across TaskToCtld, StepToCtld, StepToD, TaskInfo, and StepInfo messages. Field numbers are unique and non-conflicting, and all use the standardgoogle.protobuf.Timestamptype aligned with existing time fields.Also applies to: 243-243, 336-336, 469-469, 486-486
460-464: Documentation comments enhance clarity.The added comments explain elapsed time calculation handling and UserInfo protobuf-to-DB mapping semantics, which will aid developers maintaining clock skew resilience and understanding message semantics.
Also applies to: 511-515, 755-765
632-636: Error codes properly structured for deadline and QoS support.New error codes are added with appropriate semantics: ERR_INVALID_DEADLINE (78) for deadline validation, QoS-related codes (25–29) for constraint management, and CRI error codes (80–82) for container runtime integration. All values are unique and maintain room for future expansion.
Also applies to: 705-705, 707-711
| protos.ErrCode_ERR_CRI_GENERIC: "CRI runtime returns error. Check logs for details.", | ||
| protos.ErrCode_ERR_CRI_DISABLED: "CRI support is disabled in the cluster.", | ||
| protos.ErrCode_ERR_CRI_CONTAINER_NOT_READY: "Task is pending or container is not ready.", | ||
| protos.ErrCode_ERR_INVALID_DEADLINE: "Invalid deadline", |
There was a problem hiding this comment.
🧩 Analysis chain
LGTM! Clean addition of deadline error mapping.
The error code mapping is correctly formatted and the message is clear and consistent with the existing error messages in the map.
Optionally verify that ERR_INVALID_DEADLINE is properly utilized in the deadline validation logic:
🏁 Script executed:
#!/bin/bash
# Description: Find usage of ERR_INVALID_DEADLINE to confirm it's wired into validation logic
# Search for where this error code is returned or used
rg -nP --type=go -C3 'ERR_INVALID_DEADLINE'Length of output: 566
I need to verify whether the error code is actually being used or if this is an incomplete implementation. Let me search for deadline-related validation logic and usage patterns.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for deadline-related logic and error handling patterns
echo "=== Searching for deadline validation logic ==="
rg -nP --type=go -C2 'deadline|Deadline' | head -50
echo -e "\n=== Checking if error code is referenced by other means ==="
rg -nP --type=go -C2 'ERR_CRI_CONTAINER_NOT_READY|ERR_CRI_DISABLED' | head -30
Length of output: 3937
Let me verify if the proto error code constant is properly defined and check for any related context.
🏁 Script executed:
#!/bin/bash
# Search for proto definitions and any TODO/FIXME related to deadline errors
echo "=== Searching for ERR_INVALID_DEADLINE in proto files ==="
rg -nP --type=proto 'ERR_INVALID_DEADLINE'
echo -e "\n=== Searching for deadline-related TODOs or FIXMEs ==="
rg -nP --type=go -C2 'TODO|FIXME.*deadline|deadline.*TODO|deadline.*FIXME'
echo -e "\n=== Check where util.ErrorCmdArg deadline error is used ==="
rg -nP --type=go -C3 'Invalid argument.*deadline'Length of output: 3374
Error code added to map but not wired into deadline validation logic.
Verification found that ERR_INVALID_DEADLINE is defined in the error message map but is never actually used. Deadline validation in both internal/crun/crun.go (lines 1123-1127) and internal/calloc/calloc.go (lines 498-502) returns util.ErrorCmdArg instead of the new error code.
The error code mapping needs to be connected to the actual deadline validation error handling in those two locations for consistency.
2 participants
Summary by CodeRabbit
New Features
Documentation