Improved handshaking #10
Merged
Conversation
This file contains 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
A .tkn file, containing the runner registration token is created inside the microVM. The main orchestrator will wait for the first such token file for a given runner loop before continuing iterating to the next runner loop. This slows down startup time slightly, but provides improved determinism. Also, when an exit signal is caught, a file with the extension .brk is created with a secret. This file and the content of the secret are verified in the runner loop, and, when present and matching, the loop is stopped instead of a new runner being created.
Add an initial description of the internals of the project's implementation.
WalkthroughThe updates introduce a new GitHub Actions workflow for testing, establish guidelines for process signaling in isolated environments, enhance script functionality through reordering and new features, and improve runner management with secure registration and termination. These changes streamline operations, enhance security, and facilitate better control and integration of runners in the orchestrator setup, ensuring a more efficient and secure development workflow. 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: 3
Configuration used: CodeRabbit UI
Files selected for processing (9)
- .github/workflows/test.yml (1 hunks)
- CONTRIBUTING.md (1 hunks)
- base/base.sh (2 hunks)
- lib/common.sh (2 hunks)
- orchestrator.sh (5 hunks)
- runner.sh (6 hunks)
- runner/install.sh (2 hunks)
- runner/runner.sh (7 hunks)
- runner/token.sh (1 hunks)
Additional comments: 18
.github/workflows/test.yml (1)
- 1-24: The workflow file is well-structured and follows best practices for GitHub Actions. It correctly specifies triggers, jobs, steps, and uses appropriate actions like
actions/checkout@v4. The use of a self-hosted runner is noted, and the commands in the run step provide useful debugging information. Ensure the self-hosted runner has the necessary permissions and environment setup for these commands to execute successfully.CONTRIBUTING.md (1)
- 1-35: The
CONTRIBUTING.mdfile clearly outlines the signaling mechanism between processes in an isolated environment, including runner identification, registration, and termination. The use of a.tknfile for runner registration and a.brkfile with a secret for termination signals is a secure and effective method. Ensure that the random string used for the secret is sufficiently unpredictable and securely generated to prevent unauthorized termination signals.runner/token.sh (1)
- 112-112: Adding a verbose message before requesting a runner token enhances the script's usability and debuggability by making the action explicit. This is a good practice, especially for operations that involve network requests. Ensure that the
TOKEN_PATused for authorization is securely handled and not exposed in logs or error messages.lib/common.sh (2)
- 79-85: The
find_patternfunction is a useful addition for searching file patterns within a directory. It correctly usesfindwith appropriate options to limit the search depth and match the specified pattern. Ensure that the function is used in a secure context, especially when handling user-supplied input to prevent potential path traversal or injection vulnerabilities.- 97-98: Modifying the
wait_pathfunction to usefind_patternfor path existence checking is a logical improvement. It makes the function more versatile and efficient by leveraging the newly addedfind_pattern. Ensure that the timeout mechanism is tested thoroughly to prevent infinite loops or premature timeouts in scenarios with high system load or slow file system operations.runner/install.sh (1)
- 32-33: Moving the inclusion of
common.shto after settingINSTALL_ROOTDIRinrunner/install.shis appropriate, as it ensures thatINSTALL_ROOTDIRis available to any functions or variables defined withincommon.shthat might depend on it. This change improves the script's modularity and initialization sequence. Ensure that all functions and variables fromcommon.share indeed not needed before this point in the script.base/base.sh (1)
- 33-34: Moving the inclusion of
common.shto after resolvingBASE_ROOTDIRinbase/base.shensures thatBASE_ROOTDIRis defined before any functions or variables fromcommon.share accessed. This change is consistent with best practices for script initialization and modularity. Verify that there are no dependencies oncommon.shbefore this point in the script.runner.sh (4)
- 32-33: Sourcing
lib/common.shat the beginning ofrunner.shensures that shared functions and variables are available throughout the script. This is a good practice for maintaining modularity and reusability of code. Confirm thatlib/common.shdoes not depend on any variables or settings defined later inrunner.sh.- 88-89: Setting a default value for
RUNNER_SECRETusingrandom_stringis a secure and effective way to generate a unique secret for each runner instance. Ensure that therandom_stringfunction generates sufficiently unpredictable and secure strings to prevent unauthorized access or termination signals.- 119-120: Handling the
-Soption to allow specifyingRUNNER_SECRETprovides flexibility for use cases where a predefined secret is necessary. Ensure that this option is documented and that users are aware of the security implications of using predictable secrets.- 205-214: The logic for checking a break condition based on
RUNNER_SECRETis well-implemented. It ensures that the runner loop can be securely terminated by verifying the content of the.brkfile against the secret. This approach adds an additional layer of security by preventing unauthorized termination. Ensure that the handling of the.brkfile and the secret is secure and does not expose them to unauthorized access.orchestrator.sh (3)
- 32-34: Sourcing
lib/common.shearlier in the script is a good practice for ensuring that all functions and variables defined incommon.share available to the rest of the script. This change should improve the maintainability and modularity of the script by centralizing common functionality.- 168-174: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [171-193]
Updating the
trapcommand in thecleanupfunction to reset the trap (trap - INT TERM EXIT) before performing cleanup actions is a good practice. It prevents the trap from being triggered multiple times in case of multiple signals. However, ensure that all resources that need to be cleaned up are correctly handled in thecleanupfunction to avoid resource leaks.
- 259-266: The logic modification for waiting before starting the next runner, which includes waiting for the runner token to be ready or sleeping for a specified time, is a thoughtful addition. It helps in managing the pace of runner creation and ensures that the system is not overwhelmed. The use of
wait_pathandfind_patternfromlib/common.shfor waiting on the token file is a good example of leveraging common functionality. However, ensure that error handling is in place for cases where the token file does not become available within the expected timeframe.runner/runner.sh (4)
- 33-35: Sourcing
../lib/common.shcorrectly ensures that the script can use the common functions and variables. It's important to maintain the relative path accuracy to avoid sourcing errors, especially when the script is executed from different directories.- 217-222: Storing the runner registration token in a file if
RUNNER_TOKENFILEis set is a secure way to handle sensitive information. It reduces the risk of token exposure compared to passing tokens directly through command-line arguments or environment variables. Ensure that the file permissions are set appropriately to restrict access to the token file.- 228-267: The modifications in the
runner_unregisterfunction, including handling storing and reading the runner token from a file and creating a break file under specific conditions, are well thought out. These changes improve the script's robustness in managing runner lifecycle events. However, ensure that error handling is robust, especially for file operations like reading from or deleting the token file, to avoid potential script failures.- 405-409: Updating trap signals in the script to handle termination signals more gracefully is a good practice. It ensures that the runner is unregistered cleanly from GitHub before the script exits, which helps in maintaining a clean state. The distinction between breaking the microVM creation loop on INT or TERM signals versus regular EXIT signals is a thoughtful detail.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Close #7
Summary by CodeRabbit
New Features
CONTRIBUTING.mdfile for process signaling within isolated environments.Refactor
Documentation