Graceful shutdown -- handle termination signals #14
Conversation
Implement graceful shutdown of all the runners on termination signals received by the orchestrator. In order to avoid ctrl-c from being sent down the entire tree, a separate process group is created by the runner loop: this prevents abrupt termination of the microVM. When termination is received, a termination file (with the same secret as for the break file) is created. The implementation will react to the termination file and kill and unregister the runner. In order for all termination to be handled in the background, all processes are started in the background and waited for. The library is enhanced with a number of functions to handle processes, these should be portable to macOS.
When the token file is present, remove through creating a termination file. Otherwise, kill the entire process tree directly, as the runner has not (yet) been registered at GitHub.
WalkthroughThe recent updates enhance process management and script cleanup across several shell scripts. Key changes include reordering command-line arguments, introducing new process management functions, refining cleanup procedures, and improving job control for asynchronous execution. These modifications aim to boost the efficiency and reliability of the scripts, focusing on better handling of command-line arguments, process tracking, and termination logic. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (5)
- demo/demo.sh (1 hunks)
- lib/common.sh (1 hunks)
- orchestrator.sh (3 hunks)
- runner.sh (3 hunks)
- runner/runner.sh (3 hunks)
Additional comments: 15
demo/demo.sh (1)
- 18-18: The adjustment of command-line arguments passed to
orchestrator.shseems appropriate. Ensure thatorchestrator.shis updated to correctly process these arguments in their new order.lib/common.sh (6)
- 78-80: The
tacfunction implementation usingawkis correct and efficiently reverses input lines. This is a good cross-platform solution.- 84-86: The
pgrep_Pfunction correctly mimics the behavior ofpgrep -Pusingpsandawk, providing a cross-platform way to find child processes. Ensure that thepsoptions used are compatible across different UNIX-like systems.- 88-95: The
ps_treefunction recursively lists a process tree starting from a given PID. This implementation is logically sound and uses recursion effectively. However, consider adding error handling for invalid PIDs.- 97-105: The
runningfunction constructs a list of running PIDs from a given list. It's important to ensure that thepscommand's output format is consistent across different systems for this function to work correctly.- 108-118: The
waitpidfunction uses a polling mechanism to wait for a list of PIDs to terminate. While this approach is functional, consider the performance implications of polling with a fixed sleep interval. An adaptive sleep interval might improve performance in some scenarios.- 121-126: The
kill_treefunction effectively kills a process tree starting from a given PID. This function is well-implemented, but ensure that the signal specified in the second argument is handled correctly by all targeted processes.orchestrator.sh (3)
- 92-100: The modifications to the cleanup function, including the adjustment of the trap behavior for EXIT signals and the enhanced process termination logic, are well-implemented. Ensure that the
waitpidfunction is robust and handles all edge cases correctly.- 130-130: Setting up the trap for the cleanup function on EXIT signals is a good practice for ensuring resources are properly released upon script termination. Verify that all necessary cleanup actions are included in the
cleanupfunction.- 171-173: The explicit call to
waitpidat the end of the script, followed by a call tocleanup, ensures that all child processes are waited on before proceeding with cleanup. This is a good practice for ensuring a graceful shutdown.runner.sh (3)
- 265-272: The modifications to
vm_start()to include PID tracking and wait logic are well-implemented. The use of job control settings and the restoration of the original option state are good practices. Ensure that the PID tracking mechanism is robust and handles all edge cases correctly.- 287-311: The
vm_terminate()function introduces a comprehensive termination logic that handles different scenarios effectively. Ensure that the signal used inkill_treeis appropriate for the processes being terminated and that the waiting logic correctly handles all child processes.- 314-322: The
cleanup()function is well-designed to manage cleanup tasks and utilizes thevm_terminate()function effectively. The use oftrapto ensure cleanup on script exit is a good practice. Verify that all necessary cleanup actions are included and correctly implemented.runner/runner.sh (2)
- 420-431: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [423-439]
The implementation of asynchronous execution and PID tracking for the runner process is correctly done using
&to run the command in the background and capturing the PID with$!. However, ensure that the user permissions and environment are correctly set up for the runner process when running as a non-root user, especially when changing ownership of the$RUNNER_WORKDIR. This is crucial for avoiding permission issues that could affect the runner's operation.
- 447-466: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-466]
Overall, the script modifications align with the PR objectives of enhancing graceful shutdown capabilities. Ensure that all external commands and utilities used in the script are available in the target environments, especially considering the script's potential to run in diverse environments. Additionally, thorough testing in different scenarios (e.g., different user permissions, unexpected termination signals) is recommended to ensure the changes work as intended across various configurations.
Summary by CodeRabbit