feat: add TaskStatus Suspend #335
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughAdds suspend/resume job control: new CLI commands, parser types, handlers, and help entries; ccontrol implements SuspendJobs/ResumeJobs that call ModifyTask RPC with new Suspend/Resume attributes. Protos add Suspend/Resume attributes and a Suspended task status. Utility parsing recognizes "suspend"/"suspended"/"s". Output follows existing JSON/summary patterns. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CControl CLI
participant Parser
participant Ctl as CControl
participant RPC as gRPC Stub
participant Srv as Server
User->>CLI: ccontrol suspend <jobids>
CLI->>Parser: Parse args -> SuspendCommand(ID)
CLI->>Ctl: executeSuspendCommand(ID)
Ctl->>Ctl: Build ModifyTaskRequest{UID, TaskIds, Attribute=Suspend}
Ctl->>RPC: ModifyTask(req)
RPC->>Srv: gRPC ModifyTask
Srv-->>RPC: Reply
RPC-->>Ctl: Reply
alt FlagJson set
Ctl-->>User: Print JSON reply
else
Ctl-->>User: SummarizeReply
end
Note over Ctl: Resume flow mirrors Suspend with Attribute=Resume
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Potential focus areas:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/ccontrol/ccontrol.go (2)
690-715: Consider aligning error handling with HoldReleaseJobs.The error handling pattern differs from the similar
HoldReleaseJobsfunction:
- Parse error handling: Uses
log.Errorf+ bareCraneError(lines 693-694), whileHoldReleaseJobsreturns aCraneErrorwith a descriptive message.- JSON mode error handling: Returns
nilunconditionally (lines 709-712), whileHoldReleaseJobschecksreply.NotModifiedTasksand returns an error code when appropriate.Consider this alignment for consistency:
func SuspendJobs(jobs string) error { jobList, err := util.ParseJobIdList(jobs, ",") if err != nil { - log.Errorf("invalid job list: %s", err) - return &util.CraneError{Code: util.ErrorCmdArg} + return &util.CraneError{ + Code: util.ErrorCmdArg, + Message: fmt.Sprintf("Invalid job list specified: %s.", err), + } } req := &protos.ModifyTaskRequest{ Uid: uint32(os.Getuid()), TaskIds: jobList, Attribute: protos.ModifyTaskRequest_Suspend, } reply, err := stub.ModifyTask(context.Background(), req) if err != nil { util.GrpcErrorPrintf(err, "Failed to suspend jobs") return &util.CraneError{Code: util.ErrorNetwork} } if FlagJson { fmt.Println(util.FmtJson.FormatReply(reply)) - return nil + if len(reply.NotModifiedTasks) == 0 { + return nil + } else { + return &util.CraneError{Code: util.ErrorBackend} + } } return SummarizeReply(reply) }
717-742: Consider aligning error handling with HoldReleaseJobs.The error handling pattern differs from the similar
HoldReleaseJobsfunction:
- Parse error handling: Uses
log.Errorf+ bareCraneError(lines 720-721), whileHoldReleaseJobsreturns aCraneErrorwith a descriptive message.- JSON mode error handling: Returns
nilunconditionally (lines 736-739), whileHoldReleaseJobschecksreply.NotModifiedTasksand returns an error code when appropriate.Consider this alignment for consistency:
func ResumeJobs(jobs string) error { jobList, err := util.ParseJobIdList(jobs, ",") if err != nil { - log.Errorf("invalid job list: %s", err) - return &util.CraneError{Code: util.ErrorCmdArg} + return &util.CraneError{ + Code: util.ErrorCmdArg, + Message: fmt.Sprintf("Invalid job list specified: %s.", err), + } } req := &protos.ModifyTaskRequest{ Uid: uint32(os.Getuid()), TaskIds: jobList, Attribute: protos.ModifyTaskRequest_Resume, } reply, err := stub.ModifyTask(context.Background(), req) if err != nil { util.GrpcErrorPrintf(err, "Failed to resume jobs") return &util.CraneError{Code: util.ErrorNetwork} } if FlagJson { fmt.Println(util.FmtJson.FormatReply(reply)) - return nil + if len(reply.NotModifiedTasks) == 0 { + return nil + } else { + return &util.CraneError{Code: util.ErrorBackend} + } } return SummarizeReply(reply) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
internal/ccontrol/ccontrol.go(1 hunks)internal/ccontrol/cmd.go(2 hunks)internal/ccontrol/help.go(1 hunks)internal/ccontrol/parser.go(6 hunks)internal/util/string.go(1 hunks)protos/Crane.proto(1 hunks)protos/PublicDefs.proto(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
internal/ccontrol/cmd.go (2)
internal/util/err.go (3)
ErrorCmdArg(37-37)ErrorGeneric(36-36)ErrorSuccess(35-35)internal/ccontrol/ccontrol.go (2)
SuspendJobs(690-715)ResumeJobs(717-742)
internal/ccontrol/parser.go (1)
internal/cacctmgr/parser.go (3)
KeyValueParam(103-106)ShowCommand(75-82)DeleteCommand(43-49)
internal/ccontrol/ccontrol.go (5)
internal/util/string.go (1)
ParseJobIdList(1061-1073)internal/util/err.go (3)
CraneError(44-47)ErrorCmdArg(37-37)ErrorNetwork(38-38)internal/util/grpc.go (1)
GrpcErrorPrintf(388-399)internal/ccontrol/cmd.go (1)
FlagJson(45-45)internal/util/formatter.go (1)
FmtJson(58-58)
zhansan114514
force-pushed
the
feat/suspend
branch
from
69c59d4 to
9d2ea50
Compare
Contributor
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 (7)
internal/ccontrol/ccontrol.go(1 hunks)internal/ccontrol/cmd.go(2 hunks)internal/ccontrol/help.go(1 hunks)internal/ccontrol/parser.go(6 hunks)internal/util/string.go(1 hunks)protos/Crane.proto(1 hunks)protos/PublicDefs.proto(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- protos/Crane.proto
- protos/PublicDefs.proto
- internal/ccontrol/parser.go
- internal/util/string.go
- internal/ccontrol/help.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/ccontrol/cmd.go (2)
internal/util/err.go (3)
ErrorCmdArg(36-36)ErrorGeneric(35-35)ErrorSuccess(34-34)internal/ccontrol/ccontrol.go (2)
SuspendJobs(180-205)ResumeJobs(207-232)
internal/ccontrol/ccontrol.go (5)
internal/util/string.go (1)
ParseJobIdList(1100-1112)internal/util/err.go (3)
CraneError(43-47)ErrorCmdArg(36-36)ErrorNetwork(37-37)internal/util/grpc.go (1)
GrpcErrorPrintf(387-398)internal/ccontrol/cmd.go (1)
FlagJson(45-45)internal/util/formatter.go (1)
FmtJson(58-58)
Comment on lines
+199
to
+204
| if FlagJson { | ||
| fmt.Println(util.FmtJson.FormatReply(reply)) | ||
| return nil | ||
| } | ||
|
|
||
| return SummarizeReply(reply) |
Contributor
There was a problem hiding this comment.
Preserve JSON-mode failure signaling for suspend/resume.
When FlagJson is set, both paths return nil unconditionally. If the backend replies with NotModifiedTasks, ccontrol suspend --json or ccontrol resume --json will still exit 0, unlike other ModifyTask helpers (for example ChangeTaskTimeLimit and HoldReleaseJobs) that surface util.ErrorBackend. This silently hides partial failures from scripts relying on the exit status.
Mirror the existing pattern so JSON mode still returns an error when any task wasn’t modified.
if FlagJson {
fmt.Println(util.FmtJson.FormatReply(reply))
- return nil
+ if len(reply.NotModifiedTasks) == 0 {
+ return nil
+ }
+ return &util.CraneError{Code: util.ErrorBackend}
}Apply the same guard to both SuspendJobs and ResumeJobs.
Also applies to: 226-231
🤖 Prompt for AI Agents
In internal/ccontrol/ccontrol.go around lines 199-204 and similarly 226-231, the
FlagJson branch prints the JSON reply then unconditionally returns nil; change
it to mirror the pattern used by other ModifyTask helpers so that after printing
the JSON you check the reply for backend/partial-failure (the NotModifiedTasks /
util.ErrorBackend condition) and return the corresponding error instead of nil
when any task wasn’t modified; in short, keep the JSON output but preserve
failure signaling by returning util.ErrorBackend (or the reply error) when the
reply indicates NotModifiedTasks/partial failure.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation