Create, start and delete microVMs in runner.sh #13
Conversation
Change the logic to adapt to the semantics of the microVMs lifecycle. MicroVMs are created, started and deleted by the runner loops instead. The orchestrator is only responsible for creating the loops. As a result, many ORCHESTRATOR_ variables (and their options) have been moved to the runner.sh loop script, and renamed to RUNNER_. The orchestrator now accepts much fewer options. Instead, options should be passed after a -- at the command-line, and these are blindly passed to each runner loop. This means that the number of loops to create is not an argument anymore, but rather an option to the orchestrator (defaults to 1). This is a breaking change. Also the tgz binary distribution is now removed during installation, and, instead the runner-specific installation is (recursively) copied to the target directory. This is to avoid the need to have the tgz distribution in the runner's directory, but also because copying is actually quicker than moving files (hierarchies).
WalkthroughThe overall changes focus on enhancing the project's infrastructure for managing microVMs, specifically by refining script functionalities for runner creation, installation, and management. These alterations aim to improve usability, security, and performance through adjustments in command-line options, environment variable usage, and process handling. Additionally, the introduction of new validation functions and modifications in the VM handling scripts align with efforts to streamline operations and reduce initialization times for runners. Changes
Assessment against linked issues
The changes align with the objective outlined in issue #12, focusing on optimizing the runner initialization process by adjusting how the runner implementation is handled during installation and startup. Although the summary does not explicitly mention moving the unpacked installation as described in the issue, the modifications in the installation scripts and the emphasis on directory handling suggest an approach that would support quicker initializations, indirectly addressing the objective. The lack of specific details about the implementation of 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: 2
Configuration used: CodeRabbit UI
Files selected for processing (7)
- CONTRIBUTING.md (2 hunks)
- README.md (4 hunks)
- lib/common.sh (2 hunks)
- orchestrator.sh (3 hunks)
- runner.sh (6 hunks)
- runner/install.sh (1 hunks)
- runner/runner.sh (4 hunks)
Additional comments: 17
CONTRIBUTING.md (2)
- 5-8: The addition of a tip regarding the usage of the orchestrator and runner scripts is helpful for contributors. It clarifies how options are passed between these scripts, enhancing the documentation's usability.
- 42-58: The section on changes to the installation scripts provides valuable information on how to handle modifications to
base.sh,install.sh, anddocker.sh. It also guides contributors on how to use the results of thedev.ymlworkflow for testing. This is a useful addition for contributors looking to understand the CI/CD process and image publication.lib/common.sh (3)
- 36-36: Reducing the default output of the
random_stringfunction from 12 to 7 characters is a notable change. This adjustment likely aims to balance uniqueness with efficiency. However, it's important to ensure that this shorter length does not compromise the security or functionality in contexts where the random string is used.- 107-115: The addition of the
check_numberfunction is a good practice for validating numeric inputs. It enhances the script's robustness by ensuring that inputs expected to be numbers are correctly validated. This is a valuable addition for error handling and input validation.- 117-126: Similarly, the
check_positive_numberfunction further refines input validation by ensuring numbers are not only valid but also positive. This function is particularly useful for contexts where negative numbers or zero are not acceptable inputs, improving the script's utility and error handling.runner/install.sh (1)
- 107-114: Creating a version-specific subdirectory for runner installations (
"${INSTALL_DIR}/runner-${INSTALL_VERSION}") is a significant improvement in managing different runner versions. This change enhances the script's flexibility and maintainability by isolating installations by version. Ensure that all references to the runner installation directory elsewhere in the project are updated to reflect this new structure.orchestrator.sh (4)
- 42-56: Introducing the
ORCHESTRATOR_RUNNERSandORCHESTRATOR_PREFIXvariables significantly enhances the script's flexibility in managing runner loops. These changes allow for the creation of multiple runner loops with customizable VM name prefixes, improving the script's usability for different scenarios. Ensure that the logic for handling these variables is consistently applied throughout the script.- 63-79: The modifications to the option parsing logic to accommodate the new
ORCHESTRATOR_RUNNERSandORCHESTRATOR_PREFIXvariables are correctly implemented. However, it's important to ensure that the documentation and help messages are updated to reflect these new options, providing clear guidance to users on how to use them.- 93-102: The cleanup function's logic to remove microVMs based on the
ORCHESTRATOR_PREFIXis a crucial addition for resource management and cleanup. This ensures that resources are properly cleaned up upon script termination, preventing potential resource leaks. Verify that thegreppattern correctly matches the intended VM names and that there are no edge cases where legitimate VMs might be unintentionally deleted.- 112-165: The logic for creating and managing multiple runner loops based on the
ORCHESTRATOR_RUNNERScount is well-implemented. The use of a loop to create each runner and the conditional logic for synchronization between runner creations are particularly noteworthy. This significantly enhances the script's capability to manage multiple runners efficiently. Ensure that the synchronization mechanism (wait_path) is robust and correctly handles edge cases, such as timeouts or missing token files.README.md (3)
- 20-22: Updating the command-line example to use the
-noption for specifying the number of runner loops is a significant improvement in clarity. This change makes it easier for users to understand how to create multiple runner loops, enhancing the documentation's usability.- 108-114: The clarification that the
orchestrator.shscript creates loops of ephemeral runners and the detailed explanation of therunner.shscript's functionality provide valuable insights into the project's architecture and design. This enhances the documentation by giving users a clear understanding of how the project manages runners and microVMs.- 146-150: The detailed explanation of how runner tokens are handled, including their synchronization role and security measures, is an important addition to the documentation. This information enhances the project's transparency regarding security practices and operational details, contributing to a better understanding of the project's inner workings.
runner.sh (4)
- 91-91: The addition of
RUNNER_PREFIXfor VM naming is a good practice for identifying and managing VMs related to this runner. It enhances the maintainability and organization of VMs.- 202-226: The
vm_createfunction is well-structured and covers the essential parameters for creating a microVM. However, it's important to ensure that therun_krunvmcommand, which seems to be a critical external call, has proper error handling mechanisms in place. Ifrun_krunvmfails for any reason (e.g., due to incorrect parameters or system issues), the script should handle such errors gracefully to avoid leaving the system in an inconsistent state.Please ensure that
run_krunvmincludes robust error handling and consider adding error checks after its invocation in the script.
- 270-279: The
vm_deletefunction provides a straightforward mechanism for cleaning up VMs, including the removal of environment files. This is a good practice for maintaining a clean and efficient system state. However, ensure that the deletion process is robust against partial failures, such as when a VM or its associated files might not exist or be accessible due to permissions or other issues.Please verify the robustness of the VM deletion process, especially in error scenarios.
- 262-290: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [283-308]
The main loop for VM creation, execution, and deletion is well-structured and includes mechanisms for limiting the number of iterations (
RUNNER_REPEAT) and handling break conditions via a secret (RUNNER_SECRET). This provides a flexible and controlled execution flow. However, it's crucial to ensure that the loop correctly handles errors and exceptions, particularly when creating, starting, or deleting VMs fails. Consider adding error handling or recovery mechanisms within the loop to address such scenarios.Please review the error handling within the main loop to ensure robustness in the face of failures.
Change the logic to adapt to the semantics of the microVMs lifecycle.
MicroVMs are created, started and deleted by the runner loops instead.
The orchestrator is only responsible for creating the loops. As a
result, many ORCHESTRATOR_ variables (and their options) have been moved
to the runner.sh loop script, and renamed to RUNNER_.
The orchestrator now accepts much fewer options. Instead, options should
be passed after a -- at the command-line, and these are blindly passed
to each runner loop. This means that the number of loops to create is
not an argument anymore, but rather an option to the orchestrator
(defaults to 1). This is a breaking change.
Also the tgz binary distribution is now removed during installation,
and, instead the runner-specific installation is (recursively) copied to
the target directory. This is to avoid the need to have the tgz
distribution in the runner's directory, but also because copying is
actually quicker than moving files (hierarchies).
Closes #12 (was mostly a red herring)
Summary by CodeRabbit
random_stringfunction to generate shorter strings by default.