AKS/EKS using Longhorn repo Manifests: Deploy MinIO/NFS for Backup Store Creation #2188
Conversation
…ore Creation longhorn/longhorn#9955 Signed-off-by: Roger Yao <[email protected]>
WalkthroughThe pull request modifies the Changes
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: 1
🧹 Outside diff range and nitpick comments (1)
test_framework/scripts/longhorn-setup.sh (1)
572-576: Improve code readability and documentation.The conditional logic for AKS/EKS needs better documentation and consistent indentation.
- if [[ "${TF_VAR_k8s_distro_name}" == "eks" || "${TF_VAR_k8s_distro_name}" == "aks" ]]; then - install_backupstores_from_lh_repo - else - install_backupstores - fi + # AKS/EKS require backup store manifests from the main Longhorn repo + # due to differences in storage provisioning and networking + if [[ "${TF_VAR_k8s_distro_name}" == "eks" || "${TF_VAR_k8s_distro_name}" == "aks" ]]; then + install_backupstores_from_lh_repo + else + install_backupstores + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
test_framework/scripts/longhorn-setup.sh(2 hunks)
🔇 Additional comments (1)
test_framework/scripts/longhorn-setup.sh (1)
572-576: Verify changes don't impact other distributions.
Let's verify that these changes only affect AKS/EKS and don't impact other distributions.
✅ Verification successful
Changes are safe and properly handle distribution-specific requirements
The verification shows that:
- The only difference between
install_backupstores()andinstall_backupstores_from_lh_repo()is the source URL path (longhorn-tests vs longhorn repo), but they install the same components. - The distribution-specific logic is consistently applied across multiple files, showing AKS/EKS are handled specially in various scenarios.
- Other distributions (rke, rke2, k3s) continue to use their existing paths unaffected by this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the backup store installation changes affect other distributions
# Test 1: Check for any direct references to the old backup store URLs in other files
echo "Checking for references to old backup store URLs..."
rg -l "longhorn-tests/master/manager/integration/deploy/backupstores" --type bash
# Test 2: Check for any other conditional logic based on k8s distribution
echo "Checking for other k8s distribution-specific logic..."
rg "TF_VAR_k8s_distro_name.*==" --type bash
Length of output: 415
Script:
#!/bin/bash
# Let's try without the --type bash flag since it's not recognized
# and use more specific search patterns
# Check for references to old backup store URLs
echo "Checking for references to old backup store URLs..."
rg "longhorn-tests/master/manager/integration/deploy/backupstores"
# Check for k8s distribution-specific logic
echo "Checking for k8s distribution-specific logic..."
rg "TF_VAR_k8s_distro_name.*=="
# Let's also check the implementation of both backup store installation functions
echo "Checking backup store installation functions..."
rg -A 10 "install_backupstores\(\)"
rg -A 10 "install_backupstores_from_lh_repo\(\)"
Length of output: 8989
| install_backupstores_from_lh_repo(){ | ||
| MINIO_BACKUPSTORE_URL="https://raw.githubusercontent.com/longhorn/longhorn/master/deploy/backupstores/minio-backupstore.yaml" | ||
| NFS_BACKUPSTORE_URL="https://raw.githubusercontent.com/longhorn/longhorn/master/deploy/backupstores/nfs-backupstore.yaml" | ||
| CIFS_BACKUPSTORE_URL="https://raw.githubusercontent.com/longhorn/longhorn/master/deploy/backupstores/cifs-backupstore.yaml" | ||
| AZURITE_BACKUPSTORE_URL="https://raw.githubusercontent.com/longhorn/longhorn/master/deploy/backupstores/azurite-backupstore.yaml" | ||
| kubectl create -f ${MINIO_BACKUPSTORE_URL} \ | ||
| -f ${NFS_BACKUPSTORE_URL} \ | ||
| -f ${CIFS_BACKUPSTORE_URL} \ | ||
| -f ${AZURITE_BACKUPSTORE_URL} | ||
| setup_azuitize_backup_store | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to eliminate code duplication.
The new function is almost identical to install_backupstores. Consider refactoring both functions to use a common implementation with configurable base URLs.
-install_backupstores_from_lh_repo(){
- MINIO_BACKUPSTORE_URL="https://raw.githubusercontent.com/longhorn/longhorn/master/deploy/backupstores/minio-backupstore.yaml"
- NFS_BACKUPSTORE_URL="https://raw.githubusercontent.com/longhorn/longhorn/master/deploy/backupstores/nfs-backupstore.yaml"
- CIFS_BACKUPSTORE_URL="https://raw.githubusercontent.com/longhorn/longhorn/master/deploy/backupstores/cifs-backupstore.yaml"
- AZURITE_BACKUPSTORE_URL="https://raw.githubusercontent.com/longhorn/longhorn/master/deploy/backupstores/azurite-backupstore.yaml"
- kubectl create -f ${MINIO_BACKUPSTORE_URL} \
- -f ${NFS_BACKUPSTORE_URL} \
- -f ${CIFS_BACKUPSTORE_URL} \
- -f ${AZURITE_BACKUPSTORE_URL}
- setup_azuitize_backup_store
-}
+install_backupstores() {
+ local base_url="${1:-https://raw.githubusercontent.com/longhorn/longhorn-tests/master/manager/integration/deploy/backupstores}"
+
+ # Use arrays for better maintainability
+ local manifests=(
+ "minio-backupstore.yaml"
+ "nfs-backupstore.yaml"
+ "cifs-backupstore.yaml"
+ "azurite-backupstore.yaml"
+ )
+
+ local kubectl_args=""
+ for manifest in "${manifests[@]}"; do
+ kubectl_args="${kubectl_args} -f ${base_url}/${manifest}"
+ done
+
+ if ! kubectl create ${kubectl_args}; then
+ echo "Error: Failed to create backup stores"
+ return 1
+ fi
+
+ setup_azuitize_backup_store
+}
+
+# Wrapper for backward compatibility
+install_backupstores_from_lh_repo() {
+ install_backupstores "https://raw.githubusercontent.com/longhorn/longhorn/master/deploy/backupstores"
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| install_backupstores_from_lh_repo(){ | |
| MINIO_BACKUPSTORE_URL="https://raw.githubusercontent.com/longhorn/longhorn/master/deploy/backupstores/minio-backupstore.yaml" | |
| NFS_BACKUPSTORE_URL="https://raw.githubusercontent.com/longhorn/longhorn/master/deploy/backupstores/nfs-backupstore.yaml" | |
| CIFS_BACKUPSTORE_URL="https://raw.githubusercontent.com/longhorn/longhorn/master/deploy/backupstores/cifs-backupstore.yaml" | |
| AZURITE_BACKUPSTORE_URL="https://raw.githubusercontent.com/longhorn/longhorn/master/deploy/backupstores/azurite-backupstore.yaml" | |
| kubectl create -f ${MINIO_BACKUPSTORE_URL} \ | |
| -f ${NFS_BACKUPSTORE_URL} \ | |
| -f ${CIFS_BACKUPSTORE_URL} \ | |
| -f ${AZURITE_BACKUPSTORE_URL} | |
| setup_azuitize_backup_store | |
| } | |
| install_backupstores() { | |
| local base_url="${1:-https://raw.githubusercontent.com/longhorn/longhorn-tests/master/manager/integration/deploy/backupstores}" | |
| # Use arrays for better maintainability | |
| local manifests=( | |
| "minio-backupstore.yaml" | |
| "nfs-backupstore.yaml" | |
| "cifs-backupstore.yaml" | |
| "azurite-backupstore.yaml" | |
| ) | |
| local kubectl_args="" | |
| for manifest in "${manifests[@]}"; do | |
| kubectl_args="${kubectl_args} -f ${base_url}/${manifest}" | |
| done | |
| if ! kubectl create ${kubectl_args}; then | |
| echo "Error: Failed to create backup stores" | |
| return 1 | |
| fi | |
| setup_azuitize_backup_store | |
| } | |
| # Wrapper for backward compatibility | |
| install_backupstores_from_lh_repo() { | |
| install_backupstores "https://raw.githubusercontent.com/longhorn/longhorn/master/deploy/backupstores" | |
| } |
longhorn/longhorn#9955 Signed-off-by: Roger Yao <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
test_framework/scripts/longhorn-setup.sh (1)
572-576: Fix indentation for consistency.The indentation of this conditional block is inconsistent with the surrounding code. Consider adjusting the indentation to match the parent
ifblock's style.- if [[ "${TF_VAR_k8s_distro_name}" == "eks" || "${TF_VAR_k8s_distro_name}" == "aks" ]]; then - install_backupstores_from_lh_repo - else - install_backupstores - fi + if [[ "${TF_VAR_k8s_distro_name}" == "eks" || "${TF_VAR_k8s_distro_name}" == "aks" ]]; then + install_backupstores_from_lh_repo + else + install_backupstores + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pipelines/utilities/install_backupstores.sh(1 hunks)test_framework/scripts/longhorn-setup.sh(2 hunks)
🔇 Additional comments (3)
pipelines/utilities/install_backupstores.sh (2)
10-10: LGTM: Function name corrected.
The typo in the function name has been fixed from setup_azuitize_backup_store to setup_azurite_backup_store.
13-13: LGTM: Function declaration updated.
The function declaration has been correctly renamed from setup_azuitize_backup_store to setup_azurite_backup_store.
test_framework/scripts/longhorn-setup.sh (1)
347-357: 🛠️ Refactor suggestion
Consider refactoring to eliminate code duplication.
The new function install_backupstores_from_lh_repo is nearly identical to install_backupstores, with the only difference being the source URLs. Consider refactoring both functions to use a common implementation with configurable base URLs.
|
Which issue(s) this PR fixes:
longhorn/longhorn#9955
What this PR does / why we need it:
This is to address the issues affecting the AKS and EKS Jenkins jobs after the merge of 39e61f5.
Special notes for your reviewer:
@yangchiu
Additional documentation or context
#2173
Summary by CodeRabbit