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

feat(backup): add cleanup snapshot after backup test case #2288

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

feat(backup): add cleanup snapshot after backup test case #2288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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/backup.resource
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ Create backup ${backup_id} for ${workload_kind} ${workload_id} volume
${volume_name}= get_workload_volume_name ${workload_name}
create_backup ${volume_name} ${backup_id}

Create backup ${backup_id} for volume ${volume_id} with cleanup snapshot ${cleanup_snapshot}
${volume_name} = generate_name_with_suffix volume ${volume_id}
create_backup ${volume_name} ${backup_id} ${cleanup_snapshot}

Verify backup list contains no error for volume ${volume_id}
${volume_name} = generate_name_with_suffix volume ${volume_id}
verify_no_error ${volume_name}
Expand Down Expand Up @@ -53,3 +57,7 @@ Volume ${volume_id} backup ${backup_id} should be able to create
Create backup ${backup_id} for volume ${volume_id}
Verify backup list contains no error for volume ${volume_id}
Verify backup list contains backup ${backup_id} of volume ${volume_id}

Check snapshot for backup ${backup_id} of volume ${volume_id} exists ${exists}
${volume_name} = generate_name_with_suffix volume ${volume_id}
check_snapshot_exists_for_backup ${volume_name} ${backup_id} ${exists}
14 changes: 12 additions & 2 deletions e2e/libs/backup/backup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ def __init__(self):
else:
self.backup = Rest()

def create(self, volume_name, backup_id):
return self.backup.create(volume_name, backup_id)
def create(self, volume_name, backup_id, cleanup_snapshot=False):
return self.backup.create(volume_name, backup_id, cleanup_snapshot)

def get(self, backup_id, volume_name):
return self.backup.get(backup_id, volume_name)
Expand Down Expand Up @@ -66,3 +66,13 @@ def cleanup_backup_volumes(self):

def cleanup_system_backups(self):
return self.backup.cleanup_system_backups()

def check_snapshot_exists_for_backup(self, volume_name, backup_id,
exists=True):
backup = self.backup.get(backup_id, volume_name)
snap_name = backup.snapshotName
snapshot_id = self.backup.snapshot.get_snapshot_id(snap_name)
snap = self.backup.snapshot.get(volume_name, snapshot_id)
snap_exists = False if snap.removed else True
assert snap_exists == exists, \
f"Snapshot {snap_name} exists: {snap_exists}, expected: {exists}"
Comment on lines +70 to +78
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 null check and simplify boolean expression.

The implementation should handle cases where backup.snapshotName might be None, and the boolean expression can be simplified.

Apply this diff to improve the implementation:

     def check_snapshot_exists_for_backup(self, volume_name, backup_id,
                                          exists=True):
         backup = self.backup.get(backup_id, volume_name)
+        if not backup or not backup.snapshotName:
+            raise ValueError(f"Backup {backup_id} not found or missing snapshot name")
         snap_name = backup.snapshotName
         snapshot_id = self.backup.snapshot.get_snapshot_id(snap_name)
         snap = self.backup.snapshot.get(volume_name, snapshot_id)
-        snap_exists = False if snap.removed else True
+        snap_exists = not snap.removed
         assert snap_exists == exists, \
             f"Snapshot {snap_name} exists: {snap_exists}, expected: {exists}"
📝 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
def check_snapshot_exists_for_backup(self, volume_name, backup_id,
exists=True):
backup = self.backup.get(backup_id, volume_name)
snap_name = backup.snapshotName
snapshot_id = self.backup.snapshot.get_snapshot_id(snap_name)
snap = self.backup.snapshot.get(volume_name, snapshot_id)
snap_exists = False if snap.removed else True
assert snap_exists == exists, \
f"Snapshot {snap_name} exists: {snap_exists}, expected: {exists}"
def check_snapshot_exists_for_backup(self, volume_name, backup_id,
exists=True):
backup = self.backup.get(backup_id, volume_name)
if not backup or not backup.snapshotName:
raise ValueError(f"Backup {backup_id} not found or missing snapshot name")
snap_name = backup.snapshotName
snapshot_id = self.backup.snapshot.get_snapshot_id(snap_name)
snap = self.backup.snapshot.get(volume_name, snapshot_id)
snap_exists = not snap.removed
assert snap_exists == exists, \
f"Snapshot {snap_name} exists: {snap_exists}, expected: {exists}"
🧰 Tools
🪛 Ruff (0.8.2)

76-76: Use not ... instead of False if ... else True

Replace with not ...

(SIM211)

8 changes: 5 additions & 3 deletions e2e/libs/backup/rest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,13 @@ def __init__(self):
self.snapshot = RestSnapshot()
self.retry_count, self.retry_interval = get_retry_count_and_interval()

def create(self, volume_name, backup_id):
def create(self, volume_name, backup_id, cleanup_snapshot=False):
# create snapshot
snapshot = self.snapshot.create(volume_name, backup_id)

volume = self.volume.get(volume_name)
volume.snapshotBackup(name=snapshot.name)
volume.snapshotBackup(name=snapshot.name,
cleanupBackupSnapshot=cleanup_snapshot)
# after backup request we need to wait for completion of the backup
# since the backup.cfg will only be available once the backup operation
# has been completed
Expand All @@ -44,7 +45,8 @@ def create(self, volume_name, backup_id):
f"expect volume lastBackupAt is not empty, but it's {volume.lastBackupAt}"

self.set_backup_id(backup.name, backup_id)
self.set_data_checksum(backup.name, self.volume.get_last_data_checksum(volume_name))
self.set_data_checksum(backup.name,
self.volume.get_last_data_checksum(volume_name))

return backup

Expand Down
10 changes: 8 additions & 2 deletions e2e/libs/keywords/backup_keywords.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ class backup_keywords:
def __init__(self):
self.backup = Backup()

def create_backup(self, volume_name, backup_id):
self.backup.create(volume_name, backup_id)
def create_backup(self, volume_name, backup_id, cleanup_snapshot=False):
self.backup.create(volume_name, backup_id, cleanup_snapshot)

def verify_no_error(self, volume_name):
self.backup.verify_no_error(volume_name)
Expand Down Expand Up @@ -57,3 +57,9 @@ def list_all_backups(self):

def assert_all_backups_before_uninstall_exist(self, backups_before_uninstall):
self.backup.assert_all_backups_before_uninstall_exist(backups_before_uninstall)

def check_snapshot_exists_for_backup(self, volume_name, backup_id,
exists=True):
self.backup.check_snapshot_exists_for_backup(volume_name,
backup_id,
exists)
14 changes: 14 additions & 0 deletions e2e/tests/regression/test_backup.robot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,3 +106,17 @@ Test Incremental Restore
And Delete pod 0
And Delete persistentvolumeclaim for volume 3
And Delete persistentvolume for volume 3

Test Cleanup Snapshot After Backup Completed
[Documentation] Test cleanup snapshot after backup completed
Given Create volume 0 with dataEngine=${DATA_ENGINE}
And Attach volume 0
And Wait for volume 0 healthy

When Write data 0 to volume 0
And Create backup 0 for volume 0
And Check snapshot for backup 0 of volume 0 exists True

When Write data 1 to volume 0
And Create backup 1 for volume 0 with cleanup snapshot True
And Check snapshot for backup 1 of volume 0 exists False
Loading