ci: support v2 data engine for hal cluster #2147
Conversation
Signed-off-by: Yang Chiu <[email protected]>
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request includes modifications to several scripts and configuration files. Key changes involve the addition of the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
pipelines/e2e/scripts/longhorn-setup.sh (1)
Line range hint
1-78: Consider enhancing error handling throughout the script.While the
longhornctl_checkfunction has error handling, other critical operations in the script might benefit from additional checks. For example, consider adding error checks after key operations like creating secrets, installing backupstores, or installing the CSI snapshotter.Here's an example of how you could enhance error handling:
install_backupstores if [ $? -ne 0 ]; then echo "Failed to install backupstores" exit 1 fiAdding similar checks throughout the script would improve its robustness and make troubleshooting easier.
e2e/keywords/common.resource (1)
35-37: Approve with suggestion: Dynamic disk path assignmentThe dynamic assignment of
disk_pathbased on thehost_provideris a good improvement. It allows for different disk configurations depending on the environment, which is crucial for supporting various setups including the HAL cluster mentioned in the PR objectives.However, there's room for improvement:
- The current implementation assumes '/dev/xvdh' for all non-Harvester environments, which might not be universally correct.
- Consider making the disk path more configurable for non-Harvester environments.
Consider refactoring the disk path assignment to be more flexible:
${default_disk_path}= Get Environment Variable DEFAULT_DISK_PATH /dev/xvdh ${disk_path}= Set Variable If "${host_provider}" == "harvester" /dev/vdc ${default_disk_path}This approach allows setting a default disk path via an environment variable, falling back to '/dev/xvdh' if not specified. This provides more flexibility for different environments while maintaining the current behavior as a default.
test_framework/scripts/longhorn-setup.sh (2)
Line range hint
1-562: Consider modularizing the script for better maintainabilityWhile the current change is minimal and correct, the overall script is quite long and complex. Consider breaking it down into smaller, more manageable modules or separate scripts for different functionalities (e.g., installation, testing, upgrade). This would improve readability and maintainability in the long run.
Additionally, adding more inline comments explaining the purpose and functionality of each major section would greatly enhance the script's readability and make it easier for other developers to understand and maintain.
Line range hint
1-562: Enhance error handling and loggingTo improve the robustness and debuggability of the script, consider implementing the following enhancements:
- Add a logging function to standardize log output format and severity levels.
- Implement more comprehensive error handling, especially for critical operations.
- Consider adding a cleanup function to handle interruptions or failures gracefully.
These improvements would make the script more resilient and easier to troubleshoot in case of issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- e2e/keywords/common.resource (2 hunks)
- pipelines/e2e/scripts/longhorn-setup.sh (1 hunks)
- test_framework/scripts/longhorn-setup.sh (1 hunks)
- test_framework/terraform/harvester/ubuntu/main.tf (2 hunks)
🧰 Additional context used
🔇 Additional comments (11)
pipelines/e2e/scripts/longhorn-setup.sh (2)
29-29: Verify if the longhornctl version update aligns with PR objectives.The update to longhornctl v1.7.2 seems appropriate, but we should verify if this change is explicitly mentioned in the PR objectives or if it's part of the broader goal of supporting the v2 data engine for the HAL cluster.
To ensure this change aligns with the PR objectives:
- Check if the version update is mentioned in the PR description or linked issues.
- Verify if v1.7.2 of longhornctl is compatible with or required for the v2 data engine support.
Could you please clarify if this version update is a necessary part of implementing v2 data engine support for the HAL cluster?
29-29: Approve the update to longhornctl version.The change from v1.7.1-rc2 to v1.7.2 for
longhornctlis appropriate. This update moves from a release candidate to a stable version, which is generally a good practice for production environments.To ensure consistency across the codebase, let's verify if this version update has been applied uniformly:
This will help us confirm that all references to
longhornctlhave been updated to v1.7.2 throughout the project.✅ Verification successful
Approve the update to longhornctl version.
The update to
longhornctlfrom v1.7.1-rc2 to v1.7.2 is consistent across the codebase and does not introduce any discrepancies.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent longhornctl version across the codebase # Test: Search for longhornctl version references rg -i 'longhornctl.*v1\.7\.' --type shLength of output: 40
Script:
#!/bin/bash # Description: Verify consistent longhornctl version references across the entire codebase # Test: Search for longhornctl version references in all file types with a flexible pattern rg -i 'longhornctl.*v1\.7\.\d+'Length of output: 33
e2e/keywords/common.resource (3)
4-4: LGTM: Addition of OperatingSystem libraryThe inclusion of the OperatingSystem library is a good addition. It provides useful keywords for OS-related tasks, which can be beneficial for file system operations and environment variable handling in the test automation process.
34-34: LGTM: Dynamic host provider configurationThe introduction of the
host_providervariable using theGet Environment Variablekeyword is a good practice. It allows for dynamic configuration based on the host environment, which enhances the flexibility and reusability of the test suite across different setups, including the HAL cluster mentioned in the PR objectives.
Line range hint
4-37: Summary: Good improvements with minor suggestionOverall, the changes in this file are well-aligned with the PR objectives of supporting the v2 data engine for the HAL cluster. The additions and modifications enhance the flexibility and configurability of the test environment, which is crucial for supporting various setups.
Key improvements:
- Addition of the OperatingSystem library for enhanced OS-related operations.
- Introduction of dynamic host provider configuration.
- Implementation of conditional disk path assignment.
The only suggestion is to consider making the disk path assignment more flexible for non-Harvester environments, as detailed in the previous comment.
These changes contribute positively to the test suite's ability to handle different environments, including the HAL cluster, and support the v2 data engine as intended.
test_framework/scripts/longhorn-setup.sh (1)
Line range hint
356-362: Longhorn CLI version updated to v1.7.2The Longhorn CLI version has been updated from v1.7.1-rc2 to v1.7.2. This change ensures that the script uses the latest stable version of the Longhorn CLI.
To ensure this change is consistent across the project, please run the following command:
This will help identify any other scripts that might need to be updated to use the new version.
test_framework/terraform/harvester/ubuntu/main.tf (5)
82-83: Confirm the necessity of added packagescryptsetupanddmsetupThe packages
cryptsetupanddmsetuphave been added to the installation list. These are typically used for disk encryption and device mapping.Ensure that these packages are required for your setup. If disk encryption or device management features are needed, verify that subsequent configurations utilize these packages appropriately.
89-94: Review the disabling of themultipathdserviceThe following commands have been added to stop and disable the
multipathdservice and socket:- systemctl stop multipathd.service - systemctl stop multipathd.socket - systemctl disable multipathd.service - systemctl disable multipathd.socketConfirm that disabling
multipathdwill not affect storage devices that rely on multipath I/O in your environment.
95-105: Load necessary kernel modules and ensure persistenceKernel modules are being loaded and set to persist across reboots:
Modules loaded:
uiouio_pci_genericvfio_pcinvme-tcpdm_cryptThese modules are added to
/etc/modules-load.d/modules.conffor persistence.Ensure that all these modules are required for your application, and that they are compatible with the system's kernel version.
106-107: Set up hugepages configuration correctlyHugepages are configured with:
- Runtime setting:
echo 1024 > /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages- Persistent setting:
echo "vm.nr_hugepages=1024" >> /etc/sysctl.confMake sure that allocating 1024 hugepages (2 MiB each) aligns with your application's memory requirements and that the system has sufficient resources.
51-58: Verify the updated disk configuration and image referencesThe
disk_infosection has been modified:
- The
imageNameis updated to"longhorn-qa/image-kkkjv". Ensure that this image exists and is accessible in thelonghorn-qanamespace.- A new disk configuration is added with
storageClassNameset to"harvester-longhorn", asizeof100, and abootOrderof2.Make sure that the new storage class is correctly set up in your environment and that the boot order is intended.
Run the following script to confirm the existence of the image and storage class:
Which issue(s) this PR fixes:
Issue longhorn/longhorn#9671
What this PR does / why we need it:
support v2 data engine for hal cluster
Special notes for your reviewer:
Additional documentation or context
Summary by CodeRabbit
New Features
Bug Fixes
Documentation