Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test(robot): pull backup from another longhorn #2151

Merged
merged 2 commits into from
Nov 26, 2024
Merged

test(robot): pull backup from another longhorn #2151

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
14d697f
test(robot): pull backup from another longhorn
chriscchien Oct 23, 2024
909e3c4
Merge branch 'master' into backup_in_other_cluster
yangchiu Nov 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions e2e/keywords/longhorn.resource
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,3 +82,11 @@ Delete instance-manager of deployment ${deployment_id} volume

Wait for Longhorn components all running
wait_for_namespace_pods_running longhorn-system

Install Longhorn stable version
install_longhorn_system is_stable_version=True

Uninstall Longhorn stable version
${backups_before_uninstall} = list_all_backups
uninstall_longhorn_system is_stable_version=True
Set Test Variable ${backups_before_uninstall}
9 changes: 9 additions & 0 deletions e2e/keywords/volume.resource
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,10 @@ Write data ${data_id} to volume ${volume_id}
${volume_name} = generate_name_with_suffix volume ${volume_id}
write_volume_random_data ${volume_name} 2048 ${data_id}

Write data ${data_id} ${size} MB to volume ${volume_id}
${volume_name} = generate_name_with_suffix volume ${volume_id}
write_volume_random_data ${volume_name} ${size} ${data_id}

Keep writing data to volume ${volume_id}
${volume_name} = generate_name_with_suffix volume ${volume_id}
keep_writing_data ${volume_name}
Expand Down Expand Up @@ -292,6 +296,11 @@ Check volume ${volume_id} data is backup ${backup_id} created in another cluster
${backup_data} = get_backup_data_from_backup_list ${backups_before_uninstall} ${backup_id}
Should Be Equal ${current_checksum} ${backup_data}

Create volume ${volume_id} from backup ${backup_id} in another cluster
${volume_name} = generate_name_with_suffix volume ${volume_id}
${backup_url} = get_backup_url_from_backup_list ${backups_before_uninstall} ${backup_id}
create_volume ${volume_name} fromBackup=${backup_url}
Comment on lines +308 to +309
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add validation for backup existence

The keyword assumes the backup exists in the backups_before_uninstall list. Consider adding validation to handle cases where the backup is not found.

 Create volume ${volume_id} from backup ${backup_id} in another cluster
     ${volume_name} =    generate_name_with_suffix    volume    ${volume_id}
     ${backup_url} =    get_backup_url_from_backup_list    ${backups_before_uninstall}    ${backup_id}
+    Run Keyword If    '${backup_url}' == '${None}'    Fail    Backup ${backup_id} not found in backups_before_uninstall
     create_volume   ${volume_name}    fromBackup=${backup_url}

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +306 to +309
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Missing backup URL format validation

No backup URL format validation found. Consider implementing validation to ensure compatibility across different Longhorn versions.

🔗 Analysis chain

Verify backup URL format

The backup URL format might differ between Longhorn versions. Consider adding validation for the backup URL format to ensure compatibility between different Longhorn versions.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check backup URL format across different Longhorn versions

# Search for backup URL format validation or transformation in the codebase
rg -A 5 'backup.*url.*format|backup.*url.*transform'

# Search for backup URL related tests
rg -A 5 'test.*backup.*url'

Length of output: 84

Script:

#!/bin/bash
# Description: Expanded search for backup URL format validation or related functions

# Search for various patterns related to backup URL handling
rg -i -A 5 'backup.*URL.*validate|backupURL|validateBackupURL|parse_backup|backup_url_format'

# Search for backup URL related tests with broader patterns
rg -i -A 5 'test.*backup.*URL|backup.*URL.*test'

Length of output: 9336


Create DR volume ${volume_id} from backup ${backup_id} in another cluster
${volume_name} = generate_name_with_suffix volume ${volume_id}
${backup_url} = get_backup_url_from_backup_list ${backups_before_uninstall} ${backup_id}
Expand Down
9 changes: 7 additions & 2 deletions e2e/libs/backup/rest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,13 @@ def get(self, backup_id, volume_name):

def get_from_list(self, backup_list, backup_id):
for backup in backup_list["items"]:
if backup['metadata']['annotations']['test.longhorn.io/backup-id'] == backup_id:
return backup
try:
if backup['metadata']['annotations']['test.longhorn.io/backup-id'] == backup_id:
return backup
except KeyError as e:
logging(f"Missing key in backup metadata: {str(e)} for backup {backup['metadata']['name']}")
except Exception as e:
logging(f"Unexpected error accessing backup {backup['metadata']['name']}: {str(e)}")
return None

def get_by_snapshot(self, volume_name, snapshot_name):
Expand Down
8 changes: 4 additions & 4 deletions e2e/libs/keywords/longhorn_deploy_keywords.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@ class longhorn_deploy_keywords:
def __init__(self):
self.longhorn = LonghornDeploy()

def uninstall_longhorn_system(self):
self.longhorn.uninstall()
def uninstall_longhorn_system(self, is_stable_version=False):
self.longhorn.uninstall(is_stable_version)

def check_longhorn_crd_removed(self):
self.longhorn.check_longhorn_crd_removed()

def install_longhorn_system(self):
self.longhorn.install()
def install_longhorn_system(self, is_stable_version=False):
self.longhorn.install(is_stable_version)
22 changes: 16 additions & 6 deletions e2e/libs/longhorn_deploy/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ def install(self):
return NotImplemented

@abstractmethod
def uninstall(self, longhorn_branch=None):
def uninstall(self, is_stable_version=False):
return NotImplemented

def check_longhorn_crd_removed(self):
Expand All @@ -29,17 +29,27 @@ def check_longhorn_crd_removed(self):

def check_longhorn_uninstall_pod_log(self):
logs = k8s.get_pod_logs(LONGHORN_NAMESPACE, LONGHORN_UNINSTALL_JOB_LABEL)
assert "error" not in logs
assert "level=fatal" not in logs
assert "level=error" not in logs, f"find string 'level=error' in uninstall log {logs}"
assert "level=fatal" not in logs, f"find string 'level=fatal' in uninstall log {logs}"
Comment on lines +32 to +33
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider using more robust log parsing

While the current checks are functional, they might miss variations in log levels. Consider using regular expressions for more robust matching.

-        assert "level=error" not in logs, f"find string 'level=error' in uninstall log {logs}"
-        assert "level=fatal" not in logs, f"find string 'level=fatal' in uninstall log {logs}"
+        import re
+        assert not re.search(r'level=(error|fatal)\b', logs), \
+               f"Found error or fatal level messages in uninstall log: {logs}"
📝 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.

Suggested change
assert "level=error" not in logs, f"find string 'level=error' in uninstall log {logs}"
assert "level=fatal" not in logs, f"find string 'level=fatal' in uninstall log {logs}"
import re
assert not re.search(r'level=(error|fatal)\b', logs), \
f"Found error or fatal level messages in uninstall log: {logs}"


def install_longhorn(self):
def install_longhorn(self, is_stable_version=False):
current_path=os.getcwd()
full_path = os.path.join(current_path, LONGHORN_INSTALL_SCRIPT_PATH)

if is_stable_version is True:
cmd = ['bash', '-c', f'IS_INSTALL_STABLE_VERSION=true {full_path}']
else:
cmd = ['bash', full_path]

try:
output = subprocess.check_output(['bash', full_path], timeout=LONGHORN_INSTALL_TIMEOUT)
output = subprocess.check_output(cmd, timeout=LONGHORN_INSTALL_TIMEOUT)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve error handling consistency

The error handling could be more consistent in logging both stdout and stderr.

         try:
             output = subprocess.check_output(cmd, timeout=LONGHORN_INSTALL_TIMEOUT)
             logging(output)
         except subprocess.CalledProcessError as e:
-            logging(f"Error: {e.stderr}")
+            logging(f"Command failed with exit code {e.returncode}")
+            logging(f"stdout: {e.output}")
+            logging(f"stderr: {e.stderr}")
+            raise
         except subprocess.TimeoutExpired as e:
-            logging(f"Command timed out after {e.timeout} seconds")
+            logging(f"Command timed out after {e.timeout} seconds")
+            logging(f"stdout: {e.output}")
+            raise

Also applies to: 48-51

logging(output)
except subprocess.CalledProcessError as e:
logging(f"Error: {e.stderr}")
logging(f"Command failed with exit code {e.returncode}")
logging(f"stdout: {e.output}")
logging(f"stderr: {e.stderr}")
raise
except subprocess.TimeoutExpired as e:
logging(f"Command timed out after {e.timeout} seconds")
logging(f"stdout: {e.output}")
raise
8 changes: 4 additions & 4 deletions e2e/libs/longhorn_deploy/longhorn_deploy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@ def __init__(self):
elif self._method == "helm":
self.longhorn = LonghornHelmChart()

def uninstall(self):
return self.longhorn.uninstall()
def uninstall(self, is_stable_version=False):
return self.longhorn.uninstall(is_stable_version)

def check_longhorn_crd_removed(self):
return self.longhorn.check_longhorn_crd_removed()

def install(self):
return self.longhorn.install()
def install(self, is_stable_version=False):
return self.longhorn.install(is_stable_version)
6 changes: 3 additions & 3 deletions e2e/libs/longhorn_deploy/longhorn_helm_chart.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

class LonghornHelmChart(Base):

def uninstall(self):
def uninstall(self, is_stable_version=False):
control_plane_nodes = Node.list_node_names_by_role(self, role="control-plane")
control_plane_node = control_plane_nodes[0]

Expand All @@ -19,5 +19,5 @@ def uninstall(self):
k8s.delete_namespace(namespace=LONGHORN_NAMESPACE)
k8s.wait_namespace_terminated(namespace=LONGHORN_NAMESPACE)

def install(self):
self.install_longhorn()
def install(self, is_stable_version=False):
self.install_longhorn(is_stable_version)
11 changes: 7 additions & 4 deletions e2e/libs/longhorn_deploy/longhorn_kubectl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,11 @@

class LonghornKubectl(Base):

def uninstall(self):
longhorn_branch = os.getenv("LONGHORN_REPO_BRANCH")
def uninstall(self, is_stable_version=False):
env_var = "LONGHORN_STABLE_VERSION" if is_stable_version else "LONGHORN_REPO_BRANCH"
longhorn_branch = os.getenv(env_var)
if not longhorn_branch:
raise ValueError(f"Required environment variable {env_var} is not set")

control_plane_nodes = Node.list_node_names_by_role(self, role="control-plane")
control_plane_node = control_plane_nodes[0]
Expand All @@ -30,5 +33,5 @@ def uninstall(self):
assert res, "delete uninstallation components failed"
k8s.wait_namespace_terminated(namespace=LONGHORN_NAMESPACE)

def install(self):
self.install_longhorn()
def install(self, is_stable_version=False):
self.install_longhorn(is_stable_version)
98 changes: 98 additions & 0 deletions e2e/tests/negative/pull_backup_from_another_longhorn.robot
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
*** Settings ***
Documentation Uninstallation Checks

Test Tags negative
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Consider revising the test tag.

The test is tagged as "negative" but appears to be validating successful backup and restore operations. Consider using a more appropriate tag like "backup" or "migration" to better reflect the test's purpose.

-Test Tags    negative
+Test Tags    backup    migration

Committable suggestion was skipped due to low confidence.


Resource ../keywords/common.resource
Resource ../keywords/setting.resource
Resource ../keywords/volume.resource
Resource ../keywords/persistentvolume.resource
Resource ../keywords/persistentvolumeclaim.resource
Resource ../keywords/workload.resource
Resource ../keywords/backup.resource
Resource ../keywords/snapshot.resource
Resource ../keywords/backupstore.resource
Resource ../keywords/longhorn.resource
Library ../libs/keywords/setting_keywords.py

Test Setup Set test environment
Test Teardown Cleanup test resources

*** Variables ***
${LOOP_COUNT} 1
${RETRY_COUNT} 300
${RETRY_INTERVAL} 1
${DATA_ENGINE} v1

*** Test Cases ***
Pull backup created by another Longhorn system
[Documentation] Pull backup created by another Longhorn system
... 1. Install test version of Longhorn.
... 2. Create volume, write data, and take backup.
... 3. Uninstall Longhorn.
... 4. Install test version of Longhorn.
... 5. Restore the backup create in step 2 and verify the data.
... 6. Uninstall Longhorn.
... 7. Install previous version of Longhorn.
... 8. Create volume, write data, and take backup.
... 9. Uninstall Longhorn.
... 10. Install test version of Longhorn.
... 11. Restore the backup create in step 8 and verify the data.
...
... Important
... - This test case need have set environment variable manually first if not run on Jenkins
... - LONGHORN_INSTALL_METHOD : helm or manifest
... - LONGHORN_REPO_BRANCH (ex:master)
... - CUSTOM_LONGHORN_MANAGER_IMAGE (if not using master-head)
... - CUSTOM_LONGHORN_ENGINE_IMAGE (if not using master-head)
... - CUSTOM_LONGHORN_INSTANCE_MANAGER_IMAGE (if not using master-head)
... - CUSTOM_LONGHORN_SHARE_MANAGER_IMAGE (if not using master-head)
... - CUSTOM_LONGHORN_BACKING_IMAGE_MANAGER_IMAGE (if not using master-head)
... - LONGHORN_STABLE_VERSION (ex:v1.6.3)
Given Set setting deleting-confirmation-flag to true
And Create volume 0 with dataEngine=${DATA_ENGINE}
And Attach volume 0
And Wait for volume 0 healthy
And Write data 0 300 MB to volume 0
When Create backup 0 for volume 0
Then Verify backup list contains no error for volume 0
And Verify backup list contains backup 0 of volume 0
Then Uninstall Longhorn
And Check Longhorn CRD removed

# Install current version then pull backup and verify data
Then Install Longhorn
And Set setting deleting-confirmation-flag to true
And Set backupstore
And Check backup synced from backupstore
And Create volume 1 from backup 0 in another cluster
And Wait for volume 1 detached
And Attach volume 1
And Wait for volume 1 healthy
Then Check volume 1 data is backup 0 created in another cluster
Then Uninstall Longhorn
And Check Longhorn CRD removed

# Install previous version and create backup
Then Install Longhorn stable version
And Set setting deleting-confirmation-flag to true
And Set backupstore
And Create volume 2 with dataEngine=${DATA_ENGINE}
And Attach volume 2
And Wait for volume 2 healthy
And Write data 1 300 MB to volume 2
When Create backup 1 for volume 2
Then Verify backup list contains no error for volume 2
And Verify backup list contains backup 1 of volume 2
Then Uninstall Longhorn stable version
And Check Longhorn CRD removed

# Install current version then pull backup and verify data
Then Install Longhorn
And Set backupstore
And Check backup synced from backupstore
And Create volume 3 from backup 1 in another cluster
And Wait for volume 3 detached
And Attach volume 3
And Wait for volume 3 healthy
Then Check volume 3 data is backup 1 created in another cluster
37 changes: 34 additions & 3 deletions e2e/utilities/longhorn-install.sh
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ source ../pipelines/utilities/longhorn_manifest.sh
# create and clean tmpdir
TMPDIR="/tmp/longhorn"
LONGHORN_NAMESPACE="longhorn-system"
LONGHORN_REPO_DIR="${TMPDIR}/longhorn"
LONGHORN_REPO_URI=${LONGHORN_REPO_URI:-"https://github.com/longhorn/longhorn.git"}
Comment on lines +16 to +17
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add directory validation and cleanup.

Add validation for TMPDIR and ensure clean state before operations:

 LONGHORN_REPO_DIR="${TMPDIR}/longhorn"
+# Validate TMPDIR exists and is writable
+if [[ ! -d "${TMPDIR}" ]] || [[ ! -w "${TMPDIR}" ]]; then
+    echo "Error: ${TMPDIR} does not exist or is not writable"
+    exit 1
+fi
+# Clean existing repo directory
+if [[ -d "${LONGHORN_REPO_DIR}" ]]; then
+    rm -rf "${LONGHORN_REPO_DIR}"
+fi
 LONGHORN_REPO_URI=${LONGHORN_REPO_URI:-"https://github.com/longhorn/longhorn.git"}
📝 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.

Suggested change
LONGHORN_REPO_DIR="${TMPDIR}/longhorn"
LONGHORN_REPO_URI=${LONGHORN_REPO_URI:-"https://github.com/longhorn/longhorn.git"}
LONGHORN_REPO_DIR="${TMPDIR}/longhorn"
# Validate TMPDIR exists and is writable
if [[ ! -d "${TMPDIR}" ]] || [[ ! -w "${TMPDIR}" ]]; then
echo "Error: ${TMPDIR} does not exist or is not writable"
exit 1
fi
# Clean existing repo directory
if [[ -d "${LONGHORN_REPO_DIR}" ]]; then
rm -rf "${LONGHORN_REPO_DIR}"
fi
LONGHORN_REPO_URI=${LONGHORN_REPO_URI:-"https://github.com/longhorn/longhorn.git"}

mkdir -p ${TMPDIR}
rm -rf "${TMPDIR}/"

Expand All @@ -23,19 +25,48 @@ install_longhorn_by_chart(){
wait_longhorn_status_running
}

install_longhorn_stable_by_chart(){
git clone --single-branch \
--branch "${LONGHORN_STABLE_VERSION}" \
"${LONGHORN_REPO_URI}" \
"${LONGHORN_REPO_DIR}"
Comment on lines +29 to +32
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add git clone security measures.

The git clone operation could be vulnerable to command injection if LONGHORN_STABLE_VERSION or LONGHORN_REPO_URI contains malicious values.

Add input validation:

+# Validate version format (e.g., v1.2.3 or master)
+if [[ ! "${LONGHORN_STABLE_VERSION}" =~ ^(v[0-9]+\.[0-9]+\.[0-9]+|master)$ ]]; then
+    echo "Error: Invalid version format: ${LONGHORN_STABLE_VERSION}"
+    exit 1
+fi
+
+# Validate repo URI against whitelist
+ALLOWED_REPOS=("https://github.com/longhorn/longhorn.git")
+if [[ ! " ${ALLOWED_REPOS[@]} " =~ " ${LONGHORN_REPO_URI} " ]]; then
+    echo "Error: Invalid repository URI"
+    exit 1
+fi
+
 git clone --single-branch \
📝 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.

Suggested change
git clone --single-branch \
--branch "${LONGHORN_STABLE_VERSION}" \
"${LONGHORN_REPO_URI}" \
"${LONGHORN_REPO_DIR}"
# Validate version format (e.g., v1.2.3 or master)
if [[ ! "${LONGHORN_STABLE_VERSION}" =~ ^(v[0-9]+\.[0-9]+\.[0-9]+|master)$ ]]; then
echo "Error: Invalid version format: ${LONGHORN_STABLE_VERSION}"
exit 1
fi
# Validate repo URI against whitelist
ALLOWED_REPOS=("https://github.com/longhorn/longhorn.git")
if [[ ! " ${ALLOWED_REPOS[@]} " =~ " ${LONGHORN_REPO_URI} " ]]; then
echo "Error: Invalid repository URI"
exit 1
fi
git clone --single-branch \
--branch "${LONGHORN_STABLE_VERSION}" \
"${LONGHORN_REPO_URI}" \
"${LONGHORN_REPO_DIR}"

helm upgrade --install longhorn "${LONGHORN_REPO_DIR}/chart/" --namespace "${LONGHORN_NAMESPACE}"
wait_longhorn_status_running
}
Comment on lines +28 to +35
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add version validation before git operations.

The function should validate LONGHORN_STABLE_VERSION before using it in git operations to prevent security issues.

Apply this diff:

 install_longhorn_stable_by_chart(){
+  # Validate version format (e.g., v1.2.3)
+  if [[ -z "${LONGHORN_STABLE_VERSION}" ]]; then
+    echo "Error: LONGHORN_STABLE_VERSION is not set"
+    return 1
+  fi
+  if [[ ! "${LONGHORN_STABLE_VERSION}" =~ ^v[0-9]+\.[0-9]+\.[0-9]+$ ]]; then
+    echo "Error: Invalid version format: ${LONGHORN_STABLE_VERSION}"
+    return 1
+  fi
+
   git clone --single-branch \

Committable suggestion skipped: line range outside the PR's diff.


install_longhorn_stable_by_manifest(){
LONGHORN_STABLE_VERSION=${LONGHORN_STABLE_VERSION}
LONGHORN_STABLE_MANIFEST_URL="https://raw.githubusercontent.com/longhorn/longhorn/${LONGHORN_STABLE_VERSION}/deploy/longhorn.yaml"
kubectl apply -f "${LONGHORN_STABLE_MANIFEST_URL}"
wait_longhorn_status_running
}
Comment on lines +37 to +42
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove redundant assignment and add error handling.

The function has the following issues:

  1. Line 38 contains a redundant assignment LONGHORN_STABLE_VERSION=${LONGHORN_STABLE_VERSION}
  2. Missing error handling for kubectl operations

Apply this diff:

 install_longhorn_stable_by_manifest(){
-  LONGHORN_STABLE_VERSION=${LONGHORN_STABLE_VERSION}
   LONGHORN_STABLE_MANIFEST_URL="https://raw.githubusercontent.com/longhorn/longhorn/${LONGHORN_STABLE_VERSION}/deploy/longhorn.yaml"  
-  kubectl apply -f "${LONGHORN_STABLE_MANIFEST_URL}"
+  if ! kubectl apply -f "${LONGHORN_STABLE_MANIFEST_URL}"; then
+    echo "Error: Failed to apply Longhorn manifest"
+    return 1
+  fi
   wait_longhorn_status_running
 }
📝 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.

Suggested change
install_longhorn_stable_by_manifest(){
LONGHORN_STABLE_VERSION=${LONGHORN_STABLE_VERSION}
LONGHORN_STABLE_MANIFEST_URL="https://raw.githubusercontent.com/longhorn/longhorn/${LONGHORN_STABLE_VERSION}/deploy/longhorn.yaml"
kubectl apply -f "${LONGHORN_STABLE_MANIFEST_URL}"
wait_longhorn_status_running
}
install_longhorn_stable_by_manifest(){
LONGHORN_STABLE_MANIFEST_URL="https://raw.githubusercontent.com/longhorn/longhorn/${LONGHORN_STABLE_VERSION}/deploy/longhorn.yaml"
if ! kubectl apply -f "${LONGHORN_STABLE_MANIFEST_URL}"; then
echo "Error: Failed to apply Longhorn manifest"
return 1
fi
wait_longhorn_status_running
}


install_longhorn(){
create_longhorn_namespace
install_backupstores
if [[ "${LONGHORN_INSTALL_METHOD}" == "helm" ]]; then
LONGHORN_REPO_URI=${LONGHORN_REPO_URI:-"https://github.com/longhorn/longhorn.git"}
LONGHORN_REPO_DIR="${TMPDIR}/longhorn"
install_longhorn_by_chart
elif [[ "${LONGHORN_INSTALL_METHOD}" == "manifest" ]]; then
generate_longhorn_yaml_manifest "${TF_VAR_tf_workspace}"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add comprehensive workspace validation.

The script uses TF_VAR_tf_workspace without proper validation. While basic existence check was suggested in past reviews, we should also validate the directory structure.

 elif [[ "${LONGHORN_INSTALL_METHOD}" == "manifest" ]]; then
+    if [[ -z "${TF_VAR_tf_workspace}" ]]; then
+        echo "Error: TF_VAR_tf_workspace is not set"
+        return 1
+    fi
+    if [[ ! -d "${TF_VAR_tf_workspace}" ]]; then
+        echo "Error: Workspace directory ${TF_VAR_tf_workspace} does not exist"
+        return 1
+    fi
     generate_longhorn_yaml_manifest "${TF_VAR_tf_workspace}"
📝 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.

Suggested change
generate_longhorn_yaml_manifest "${TF_VAR_tf_workspace}"
if [[ -z "${TF_VAR_tf_workspace}" ]]; then
echo "Error: TF_VAR_tf_workspace is not set"
return 1
fi
if [[ ! -d "${TF_VAR_tf_workspace}" ]]; then
echo "Error: Workspace directory ${TF_VAR_tf_workspace} does not exist"
return 1
fi
generate_longhorn_yaml_manifest "${TF_VAR_tf_workspace}"
🧰 Tools
🪛 Shellcheck

[warning] 49-49: TF_VAR_tf_workspace is referenced but not assigned.

(SC2154)

install_longhorn_by_manifest "${TF_VAR_tf_workspace}/longhorn.yaml"
fi
setup_longhorn_ui_nodeport
}
Comment on lines +43 to +54
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Validate TF_VAR_tf_workspace before use.

The script uses TF_VAR_tf_workspace without validating its existence, which could lead to silent failures.

Add validation before using the workspace variable:

 install_longhorn(){
   create_longhorn_namespace
   install_backupstores
   if [[ "${LONGHORN_INSTALL_METHOD}" == "helm" ]]; then
     install_longhorn_by_chart
   elif [[ "${LONGHORN_INSTALL_METHOD}" == "manifest" ]]; then
+    if [[ -z "${TF_VAR_tf_workspace}" ]]; then
+      echo "Error: TF_VAR_tf_workspace is not set"
+      exit 1
+    }
     generate_longhorn_yaml_manifest "${TF_VAR_tf_workspace}"
     install_longhorn_by_manifest "${TF_VAR_tf_workspace}/longhorn.yaml"
   fi
   setup_longhorn_ui_nodeport
 }

Committable suggestion was skipped due to low confidence.

🧰 Tools
🪛 Shellcheck

[warning] 49-49: TF_VAR_tf_workspace is referenced but not assigned.

(SC2154)


install_longhorn_stable_version(){
create_longhorn_namespace
install_backupstores
if [[ "${LONGHORN_INSTALL_METHOD}" == "helm" ]]; then
install_longhorn_stable_by_chart
elif [[ "${LONGHORN_INSTALL_METHOD}" == "manifest" ]]; then
install_longhorn_stable_by_manifest
fi
Comment on lines +59 to +63
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add validation for LONGHORN_INSTALL_METHOD.

The installation method should be validated before use to prevent undefined behavior.

Add method validation:

+VALID_INSTALL_METHODS=("helm" "manifest")
+if [[ ! " ${VALID_INSTALL_METHODS[@]} " =~ " ${LONGHORN_INSTALL_METHOD} " ]]; then
+    echo "Error: Invalid installation method: ${LONGHORN_INSTALL_METHOD}"
+    echo "Valid methods are: ${VALID_INSTALL_METHODS[*]}"
+    exit 1
+fi
+
 if [[ "${LONGHORN_INSTALL_METHOD}" == "helm" ]]; then
📝 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.

Suggested change
if [[ "${LONGHORN_INSTALL_METHOD}" == "helm" ]]; then
install_longhorn_stable_by_chart
elif [[ "${LONGHORN_INSTALL_METHOD}" == "manifest" ]]; then
install_longhorn_stable_by_manifest
fi
VALID_INSTALL_METHODS=("helm" "manifest")
if [[ ! " ${VALID_INSTALL_METHODS[@]} " =~ " ${LONGHORN_INSTALL_METHOD} " ]]; then
echo "Error: Invalid installation method: ${LONGHORN_INSTALL_METHOD}"
echo "Valid methods are: ${VALID_INSTALL_METHODS[*]}"
exit 1
fi
if [[ "${LONGHORN_INSTALL_METHOD}" == "helm" ]]; then
install_longhorn_stable_by_chart
elif [[ "${LONGHORN_INSTALL_METHOD}" == "manifest" ]]; then
install_longhorn_stable_by_manifest
fi

setup_longhorn_ui_nodeport
Comment on lines +56 to +64
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Reduce code duplication and add error handling.

The function duplicates logic from install_longhorn. Consider extracting common functionality and adding proper error handling.

+install_longhorn_common() {
+    if ! create_longhorn_namespace; then
+        echo "Error: Failed to create namespace"
+        return 1
+    fi
+    if ! install_backupstores; then
+        echo "Error: Failed to install backupstores"
+        return 1
+    fi
+}
+
 install_longhorn_stable_version(){
-    create_longhorn_namespace
-    install_backupstores
+    if ! install_longhorn_common; then
+        return 1
+    fi
     if [[ "${LONGHORN_INSTALL_METHOD}" == "helm" ]]; then
-        install_longhorn_stable_by_chart
+        if ! install_longhorn_stable_by_chart; then
+            return 1
+        fi
     elif [[ "${LONGHORN_INSTALL_METHOD}" == "manifest" ]]; then
-        install_longhorn_stable_by_manifest
+        if ! install_longhorn_stable_by_manifest; then
+            return 1
+        fi
     fi
-    setup_longhorn_ui_nodeport
+    if ! setup_longhorn_ui_nodeport; then
+        echo "Error: Failed to setup UI nodeport"
+        return 1
+    fi
 }
📝 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.

Suggested change
install_longhorn_stable_version(){
create_longhorn_namespace
install_backupstores
if [[ "${LONGHORN_INSTALL_METHOD}" == "helm" ]]; then
install_longhorn_stable_by_chart
elif [[ "${LONGHORN_INSTALL_METHOD}" == "manifest" ]]; then
install_longhorn_stable_by_manifest
fi
setup_longhorn_ui_nodeport
install_longhorn_common() {
if ! create_longhorn_namespace; then
echo "Error: Failed to create namespace"
return 1
fi
if ! install_backupstores; then
echo "Error: Failed to install backupstores"
return 1
fi
}
install_longhorn_stable_version(){
if ! install_longhorn_common; then
return 1
fi
if [[ "${LONGHORN_INSTALL_METHOD}" == "helm" ]]; then
if ! install_longhorn_stable_by_chart; then
return 1
fi
elif [[ "${LONGHORN_INSTALL_METHOD}" == "manifest" ]]; then
if ! install_longhorn_stable_by_manifest; then
return 1
fi
fi
if ! setup_longhorn_ui_nodeport; then
echo "Error: Failed to setup UI nodeport"
return 1
fi
}

}

install_longhorn
IS_INSTALL_STABLE_VERSION="${IS_INSTALL_STABLE_VERSION:-false}"
if [[ "${IS_INSTALL_STABLE_VERSION}" == "true" ]]; then
install_longhorn_stable_version
else
install_longhorn
fi
Comment on lines +67 to +72
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add environment variable validation.

The script should validate all required environment variables before proceeding with installation.

Add this validation before the if statement:

+# Required environment variables
+REQUIRED_VARS=(
+  "LONGHORN_INSTALL_METHOD"
+  "LONGHORN_NAMESPACE"
+)
+
+# Optional variables that become required based on installation method
+if [[ "${IS_INSTALL_STABLE_VERSION}" == "true" ]]; then
+  REQUIRED_VARS+=("LONGHORN_STABLE_VERSION")
+fi
+
+# Validate required variables
+for var in "${REQUIRED_VARS[@]}"; do
+  if [[ -z "${!var}" ]]; then
+    echo "Error: Required environment variable $var is not set"
+    exit 1
+  fi
+done
+
 if [[ "${IS_INSTALL_STABLE_VERSION}" == "true" ]]; then
📝 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.

Suggested change
IS_INSTALL_STABLE_VERSION="${IS_INSTALL_STABLE_VERSION:-false}"
if [[ "${IS_INSTALL_STABLE_VERSION}" == "true" ]]; then
install_longhorn_stable_version
else
install_longhorn
fi
IS_INSTALL_STABLE_VERSION="${IS_INSTALL_STABLE_VERSION:-false}"
# Required environment variables
REQUIRED_VARS=(
"LONGHORN_INSTALL_METHOD"
"LONGHORN_NAMESPACE"
)
# Optional variables that become required based on installation method
if [[ "${IS_INSTALL_STABLE_VERSION}" == "true" ]]; then
REQUIRED_VARS+=("LONGHORN_STABLE_VERSION")
fi
# Validate required variables
for var in "${REQUIRED_VARS[@]}"; do
if [[ -z "${!var}" ]]; then
echo "Error: Required environment variable $var is not set"
exit 1
fi
done
if [[ "${IS_INSTALL_STABLE_VERSION}" == "true" ]]; then
install_longhorn_stable_version
else
install_longhorn
fi

2 changes: 2 additions & 0 deletions pipelines/utilities/run_longhorn_e2e_test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ run_longhorn_e2e_test(){
yq e -i 'select(.spec.containers[0] != null).spec.containers[0].env += {"name": "CUSTOM_LONGHORN_SHARE_MANAGER_IMAGE", "value": "'${CUSTOM_LONGHORN_SHARE_MANAGER_IMAGE}'"}' "${LONGHORN_TESTS_MANIFEST_FILE_PATH}"
yq e -i 'select(.spec.containers[0] != null).spec.containers[0].env += {"name": "CUSTOM_LONGHORN_BACKING_IMAGE_MANAGER_IMAGE", "value": "'${CUSTOM_LONGHORN_BACKING_IMAGE_MANAGER_IMAGE}'"}' "${LONGHORN_TESTS_MANIFEST_FILE_PATH}"
yq e -i 'select(.spec.containers[0] != null).spec.containers[0].env += {"name": "LONGHORN_INSTALL_METHOD", "value": "'${LONGHORN_INSTALL_METHOD}'"}' "${LONGHORN_TESTS_MANIFEST_FILE_PATH}"
yq e -i 'select(.spec.containers[0] != null).spec.containers[0].env += {"name": "LONGHORN_STABLE_VERSION", "value": "'${LONGHORN_STABLE_VERSION}'"}' "${LONGHORN_TESTS_MANIFEST_FILE_PATH}"

LONGHORN_TEST_POD_NAME=`yq e 'select(.spec.containers[0] != null).metadata.name' ${LONGHORN_TESTS_MANIFEST_FILE_PATH}`

Expand Down Expand Up @@ -106,6 +107,7 @@ run_longhorn_e2e_test_out_of_cluster(){
-e CUSTOM_LONGHORN_SHARE_MANAGER_IMAGE="${CUSTOM_LONGHORN_SHARE_MANAGER_IMAGE}"\
-e CUSTOM_LONGHORN_BACKING_IMAGE_MANAGER_IMAGE="${CUSTOM_LONGHORN_BACKING_IMAGE_MANAGER_IMAGE}"\
-e LONGHORN_INSTALL_METHOD="${LONGHORN_INSTALL_METHOD}"\
-e LONGHORN_STABLE_VERSION="${LONGHORN_STABLE_VERSION}"\
--mount source="vol-${IMAGE_NAME}",target=/tmp \
"${LONGHORN_TESTS_CUSTOM_IMAGE}" "${ROBOT_COMMAND_ARGS[@]}"
docker stop "${CONTAINER_NAME}"
Expand Down