feat: Crunprolog/epilog #342
Conversation
📝 WalkthroughWalkthroughAdds configurable prolog/epilog command execution: new CLI flags (global and per-task), config fields, protobuf task/step fields, StateMachineOfCrun fields, and a RunCommand method that executes external programs with timeout/signal handling; prolog runs at connect and epilog after task ack. (35 words) Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant SM as StateMachineOfCrun
participant Prolog as PrologCmd
participant Cfored as cfored
participant Epilog as EpilogCmd
CLI->>SM: start / connect
note over SM: StateConnectCfored
SM->>Prolog: RunCommand(CrunProlog)
Prolog-->>SM: exitCode / output
alt non-zero exit
SM-->>CLI: backend error (terminate)
end
SM->>Cfored: connect
Cfored-->>SM: connected
CLI->>SM: submit task (includes task_prolog/task_epilog)
SM->>Cfored: send task
Cfored-->>SM: task ack
note over SM: StateWaitAck
SM->>Epilog: RunCommand(CrunEpilog or TaskEpilog)
Epilog-->>SM: exitCode / output
alt non-zero exit
SM-->>CLI: backend error (terminate)
end
SM-->>CLI: continue / success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (4)
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 (5)
internal/crun/cmd.go (2)
63-65: Clarify prolog/epilog semantics; consider argument support.Flags hold a single string and RunCommand is invoked with Args=nil, so users can’t pass arguments or quotes. Either:
- Document these as “path to executable/script” (no args), or
- Support args/shell execution (see RunCommand refactor below).
Do you intend these to accept arguments (e.g.,
--prolog "/opt/prolog.sh --init A")?
121-123: Improve help text and user expectations.Help should state precedence (flag overrides config), local execution timing, and default timeout (300s). Example:
- RootCmd.Flags().StringVarP(&FlagProlog, "prolog", "", "", "Prolog of the job") - RootCmd.Flags().StringVarP(&FlagEpilog, "epilog", "", "", "Epilog of the job") + RootCmd.Flags().StringVarP(&FlagProlog, "prolog", "", "", "Local prolog executable/script run before connecting (overrides config CrunProlog). 300s timeout.") + RootCmd.Flags().StringVarP(&FlagEpilog, "epilog", "", "", "Local epilog executable/script run after task ack (overrides config CrunEpilog). 300s timeout.")internal/util/util.go (1)
34-43: Config fields added — add omitempty and brief docs.To avoid emitting empty fields on marshal and to clarify usage:
- CrunProlog string `yaml:"CrunProlog"` - CrunEpilog string `yaml:"CrunEpilog"` + // Path to local executable/script (arguments optional per CLI behavior). + CrunProlog string `yaml:"CrunProlog,omitempty"` + CrunEpilog string `yaml:"CrunEpilog,omitempty"`Also consider validating existence/executability at startup for clearer errors.
Please confirm whether config values may include arguments or should be absolute paths only.
internal/crun/crun.go (2)
114-116: Limit API surface: make fields private unless external use is required.
StateMachineOfCrun.CrunProlog/CrunEpiloglook internal; exporting them expands API unnecessarily. PrefercrunProlog/crunEpilog.Are these accessed by other packages within the module?
122-133: Extend RunCommandArgs to support working dir and optional shell execution.This enables honoring
--chdir/task.Cwd and (optionally) parsing full command strings.type RunCommandArgs struct { Program string Args []string Envs map[string]string TimeoutSec int + // Optional working directory for the subprocess. + Dir string + // If true, run via 'sh -c Program' to allow quoted/complex commands. + Shell bool } type RunCommandResult struct { ExitCode int Output string }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/crun/cmd.go(3 hunks)internal/crun/crun.go(5 hunks)internal/util/util.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/crun/crun.go (2)
internal/util/err.go (2)
ExitCode(30-30)ErrorBackend(38-38)internal/crun/cmd.go (2)
FlagProlog(63-63)FlagEpilog(64-64)
🔇 Additional comments (2)
internal/crun/cmd.go (1)
23-23: No functional change in imports.internal/crun/crun.go (1)
26-30: No functional change beyond added imports.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
internal/crun/crun.go (3)
210-222: Previous review suggestions not yet addressed.Earlier reviews recommended:
- Setting working directory to
task.Cwdto align with--chdir- Reducing log verbosity to avoid leaking sensitive data
- Making timeout configurable rather than hardcoded to 300 seconds
These suggestions remain valid and should be implemented.
632-644: Previous review suggestions not yet addressed.The same concerns raised for prolog execution (working directory, log verbosity, timeout configurability) apply to epilog execution as well.
1051-1110: Previous review suggestions forRunCommandnot yet addressed.A prior comprehensive review identified several robustness issues that remain unaddressed:
Environment variable merging (lines 1067-1073): Current implementation appends duplicates rather than overriding. Should build a map from
os.Environ(), apply overrides fromrunCommandArgs.Envs, then reconstruct.Working directory: No support for setting
cmd.Dir, which is needed for prolog/epilog to honortask.Cwd.Exit code conventions: Timeouts and signals should map to conventional exit codes (124 for timeout, 128+N for signal N).
Optional shell mode: Consider supporting
sh -cexecution for full command strings.Unbounded output: The
outBufcan grow without limit, risking OOM. Consider capping output (e.g., 1 MiB).These improvements would make the command execution more robust and align with standard practices.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/crun/crun.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/crun/crun.go (1)
internal/crun/cmd.go (2)
FlagProlog(63-63)FlagEpilog(64-64)
🪛 GitHub Actions: Go Code Quality Check
internal/crun/crun.go
[error] 223-223: go vet: github.com/sirupsen/logrus.Tracef call needs 1 arg but has 2 args
🪛 GitHub Check: go-check
internal/crun/crun.go
[failure] 223-223:
github.com/sirupsen/logrus.Tracef call needs 1 arg but has 2 args
🔇 Additional comments (3)
internal/crun/crun.go (3)
26-26: LGTM: Necessary imports for command execution.The
bytesandos/execimports support the newRunCommandfunctionality.Also applies to: 29-29
114-115: LGTM: Prolog/epilog fields added.The public
CrunPrologandCrunEpilogfields appropriately store the command paths for pre/post execution hooks.
645-645: Fix compilation error: incorrect number of arguments tolog.Tracef.Same issue as Line 223: the format string contains one placeholder (
%s) but two values are provided.Apply this diff:
- log.Tracef("Epilog '%s' finished successfully.", m.CrunEpilog) + log.Tracef("Epilog '%s' finished successfully.", m.CrunEpilog)Note: If the current code passes
ExitCodeas a second argument (like Line 223), remove it or add%dto the format string.Likely an incorrect or invalid review comment.
huerni
force-pushed
the
dev/Prolog
branch
2 times, most recently
from
a900f8f to
769c54f
Compare
huerni
force-pushed
the
dev/Prolog
branch
2 times, most recently
from
ea5568f to
7d5e4eb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
internal/crun/crun.go (2)
120-125: AddDirfield to support working directory control.Past reviews requested a
Dirfield to allow prolog/epilog to execute in the correct working directory (particularlytask.Cwd). Without this, commands run in the current working directory of the crun process, which may not match the user's intended context.Apply this diff:
type RunCommandArgs struct { Program string Args []string Envs map[string]string TimeoutSec int + Dir string }Based on learnings from past reviews.
1045-1101: Improve environment variable handling and exit code extraction.The current implementation has two notable issues:
Environment merge creates duplicates (lines 1061-1067): Appending to
os.Environ()can result in duplicate keys (e.g.,PATH=/aandPATH=/bboth present), leading to undefined behavior. Build a map to ensure deterministic override semantics.Exit codes not extracted in all paths:
- Timeout case (line 1080): ExitCode remains 127 instead of conventional 124
- Signal case (line 1083): ExitCode remains 127 instead of 128+signal_number
- Error case (lines 1086-1089): Exit code should be extracted from
exec.ExitErrorif availableAddress the environment merge issue:
if len(runCommandArgs.Envs) > 0 { - envs := os.Environ() + envMap := make(map[string]string) + for _, kv := range os.Environ() { + if idx := strings.IndexByte(kv, '='); idx > 0 { + envMap[kv[:idx]] = kv[idx+1:] + } + } for k, v := range runCommandArgs.Envs { - envs = append(envs, fmt.Sprintf("%s=%s", k, v)) + envMap[k] = v + } + envs := make([]string, 0, len(envMap)) + for k, v := range envMap { + envs = append(envs, fmt.Sprintf("%s=%s", k, v)) + } cmd.Env = envs }For better exit code handling, consider extracting codes from
exec.ExitErrorin the error case and using conventional codes (124 for timeout, 128+N for signals).Based on learnings from past reviews.
🧹 Nitpick comments (2)
internal/crun/crun.go (2)
208-223: Consider adding working directory and configurable timeout for prolog execution.Once the
Dirfield is added toRunCommandArgs, set it tom.task.Cwdto ensure prolog runs in the user's intended directory. Additionally, the hardcoded 300-second timeout could be made configurable via a flag or config field.Example adjustment (after Dir field is added):
ExitCode := m.RunCommand(RunCommandArgs{ Program: m.CrunProlog, Args: nil, Envs: m.task.Env, TimeoutSec: 300, + Dir: m.task.Cwd, })Based on learnings from past reviews.
626-640: Apply the same Dir and timeout improvements to epilog execution.The epilog execution has the same considerations as the prolog: add
Dir: m.task.Cwdonce the field is available, and consider making the timeout configurable.Based on learnings from past reviews.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/crun/cmd.go(2 hunks)internal/crun/crun.go(6 hunks)internal/util/util.go(1 hunks)protos/PublicDefs.proto(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/crun/cmd.go
- internal/util/util.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/crun/crun.go (2)
internal/crun/cmd.go (4)
FlagProlog(65-65)FlagEpilog(66-66)FlagTaskProlog(67-67)FlagTaskEpilog(68-68)internal/util/err.go (2)
ExitCode(30-30)ErrorBackend(38-38)
🔇 Additional comments (3)
protos/PublicDefs.proto (1)
183-185: LGTM! Clean protobuf field additions.The new
task_prologandtask_epilogfields are properly numbered and follow protobuf conventions. They align with the prolog/epilog execution support added ininternal/crun/crun.go.internal/crun/crun.go (2)
112-113: LGTM! Struct fields for prolog/epilog commands.The
CrunPrologandCrunEpilogfields are appropriately public and will be populated from config and CLI flags.
1141-1143: LGTM! Task-level prolog/epilog flags properly propagated.The
TaskPrologandTaskEpilogfields are correctly set from CLI flags and will be included in the task submission to the controller.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
internal/crun/crun.go (4)
208-223: Prolog execution lacks working directory; hardcoded timeout.The prolog is executed without setting a working directory, which may cause issues if the prolog script expects to run in
task.Cwd. The 300-second timeout is also hardcoded—consider making it configurable via flag or config.Once
Diris added toRunCommandArgs, update the call:if len(m.CrunProlog) > 0 { ExitCode := m.RunCommand(RunCommandArgs{ Program: m.CrunProlog, Args: nil, Envs: m.task.Env, TimeoutSec: 300, + Dir: m.task.Cwd, })
626-640: Epilog execution: same concerns as prolog.Apply the same adjustments as prolog—set
Dirtom.task.Cwdand consider making the timeout configurable.
120-125: AddDirfield to support working directory control.The
RunCommandArgsstruct should include aDirfield to allow callers to specify the working directory, particularly for prolog/epilog execution in the context oftask.Cwd.Apply this diff:
type RunCommandArgs struct { Program string Args []string Envs map[string]string TimeoutSec int + Dir string }
1045-1101: Process not reaped after timeout/signal; exit code may be stale.After
ctx.Done()orsigChtriggers, the code kills the process group but doesn't wait for thedonechannel. This can result in:
- Zombie processes if the process hasn't terminated yet
ProcessStatebeing nil or incomplete when checking exit statusAdditionally, the environment merge (lines 1061-1066) appends new values without overriding existing keys, leading to duplicate env vars with non-deterministic behavior.
Apply this diff to ensure the process is reaped and env vars are merged correctly:
func (m *StateMachineOfCrun) RunCommand(runCommandArgs RunCommandArgs) int { ExitCode := 127 ctx := context.Background() if runCommandArgs.TimeoutSec > 0 { var cancel context.CancelFunc ctx, cancel = context.WithTimeout(ctx, time.Duration(runCommandArgs.TimeoutSec)*time.Second) defer cancel() } sigCh := make(chan os.Signal, 1) signal.Notify(sigCh, os.Interrupt, syscall.SIGTERM) defer signal.Stop(sigCh) cmd := exec.CommandContext(ctx, runCommandArgs.Program, runCommandArgs.Args...) if len(runCommandArgs.Envs) > 0 { - envs := os.Environ() + envMap := make(map[string]string) + for _, kv := range os.Environ() { + if idx := strings.IndexByte(kv, '='); idx > 0 { + envMap[kv[:idx]] = kv[idx+1:] + } + } for k, v := range runCommandArgs.Envs { - envs = append(envs, fmt.Sprintf("%s=%s", k, v)) + envMap[k] = v + } + envs := make([]string, 0, len(envMap)) + for k, v := range envMap { + envs = append(envs, fmt.Sprintf("%s=%s", k, v)) } cmd.Env = envs } + if runCommandArgs.Dir != "" { + cmd.Dir = runCommandArgs.Dir + } + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} if err := cmd.Start(); err != nil { log.Errorf("Failed to start command: %v", err.Error()) return -1 } done := make(chan error, 1) go func() { done <- cmd.Wait() }() select { case <-ctx.Done(): _ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) + <-done // Wait for process to be reaped + ExitCode = 124 case <-sigCh: _ = syscall.Kill(-cmd.Process.Pid, syscall.SIGTERM) + <-done // Wait for process to be reaped case err := <-done: if err != nil { log.Errorf("Failed to execute command: %v", err.Error()) } } if cmd.ProcessState != nil { if status, ok := cmd.ProcessState.Sys().(syscall.WaitStatus); ok { if status.Exited() { ExitCode = status.ExitStatus() + } else if status.Signaled() { + ExitCode = 128 + int(status.Signal()) } } } return ExitCode }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/crun/cmd.go(2 hunks)internal/crun/crun.go(6 hunks)protos/PublicDefs.proto(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/crun/crun.go (2)
internal/crun/cmd.go (4)
FlagProlog(65-65)FlagEpilog(66-66)FlagTaskProlog(67-67)FlagTaskEpilog(68-68)internal/util/err.go (2)
ExitCode(30-30)ErrorBackend(38-38)
🔇 Additional comments (5)
protos/PublicDefs.proto (1)
183-185: LGTM!The new
task_prologandtask_epilogfields are correctly added with sequential tag numbers and appropriate string type for command paths.internal/crun/cmd.go (1)
65-69: LGTM!Flag variable declarations are clear and follow the existing naming conventions.
internal/crun/crun.go (3)
1141-1143: LGTM!The
TaskPrologandTaskEpilogfields are correctly wired from CLI flags to theTaskToCtldstruct for propagation to the backend.
111-113: LGTM!The
CrunPrologandCrunEpilogfields are appropriately added to the state machine struct.
198-206: LGTM on priority handling.The CLI flag correctly overrides config values, establishing proper precedence (CLI > config).
huerni
force-pushed
the
dev/Prolog
branch
2 times, most recently
from
db4bdeb to
3e3a181
Compare
| m.err = util.ErrorBackend | ||
| } | ||
|
|
||
| if len(m.CrunEpilog) > 0 { |
There was a problem hiding this comment.
只要step启动 不管怎么退出都会触发 如果crun前端就失败了，不会触发 StateWaitAck 只有任务正常退出才会触发吗？
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.