Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
- incremental_backup.lifecycle:
type = incremental_backup_lifecycle
start_vm = "no"
disk_type = "file"
target_disk = "vda"
disk_dict = {"type_name": ${disk_type}, "target":{"dev": "%s", "bus": "virtio"}, "driver": {"name": "qemu", "type": "qcow2"}}
full_checkpoint = "check_full"
inc_checkpoint = "check_inc1"
nbd_hostname = "localhost"
nbd_tcp_port = "10809"
backup_dict = {'mode': 'pull', 'server': {'name': '${nbd_hostname}', 'port': '${nbd_tcp_port}'}, 'disks': [{'name': 'vda', 'backup': 'yes', 'type': 'file', 'scratch': {'attrs': {'file': '%s'}}}]}
checkpoint_dict = {'name': '%s', 'disks': [{'name': 'vda', 'checkpoint': 'bitmap'}]}
inc_backup_dict = {'mode': 'pull', 'incremental': '${full_checkpoint}', 'server': {'name': '${nbd_hostname}', 'port': '${nbd_tcp_port}'}, 'disks': [{'name': 'vda', 'backup': 'yes', 'type': 'file', 'scratch': {'attrs': {'file': '%s'}}}]}
variants test_case:
- save_vm:
expected_error = "Timed out during operation: cannot acquire state change lock.*remoteDispatchDomainBackupBegin"
- managedsave:
expected_error = "Timed out during operation: cannot acquire state change lock.*remoteDispatchDomainBackupBegin"
- restart_service:
expected_error = "Timed out during operation: cannot acquire state change lock.*monitor"
- kill_qemu_during_libvirtd_restart:
expected_error = ""
240 changes: 240 additions & 0 deletions libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,240 @@
import os
import ast

from avocado.utils import process
from virttest import data_dir
from virttest import virsh
from virttest import utils_backup
from virttest.utils_libvirtd import Libvirtd

from virttest.libvirt_xml import vm_xml
from virttest.libvirt_xml import backup_xml
from virttest.libvirt_xml import checkpoint_xml
from virttest.utils_test import libvirt


def prepare_backup_xml(test, params, backup_type):
"""
Prepare the backup xml.

:return: return the backup options and the scratch file.
"""
scratch_file = data_dir.get_data_dir() + '/scratch_file_%s' % backup_type
backup_dict = ast.literal_eval(params.get("backup_dict", "{}") % scratch_file)
full_checkpoint = params.get("full_checkpoint")
inc_checkpoint = params.get("inc_checkpoint")
if backup_type == "inc":
backup_dict.update({'incremental': full_checkpoint})
checkpoint_dict = ast.literal_eval(params.get("checkpoint_dict") % inc_checkpoint)
else:
checkpoint_dict = ast.literal_eval(params.get("checkpoint_dict") % full_checkpoint)
backup_dev = backup_xml.BackupXML()
backup_dev.setup_attrs(**backup_dict)
test.log.debug("The backup xml is %s." % backup_dev)
checkpoint_dev = checkpoint_xml.CheckpointXML()
checkpoint_dev.setup_attrs(**checkpoint_dict)
backup_options = backup_dev.xml + " " + checkpoint_dev.xml
return backup_options


def start_full_backup(test, params):
"""
Start a full backup.

:return: return the backup file path.
"""
vm_name = params.get("main_vm")
backup_file_path = data_dir.get_data_dir() + '/full.backup'
target_disk = params.get("target_disk")
nbd_hostname = params.get("nbd_hostname")
nbd_tcp_port = params.get("nbd_tcp_port")
nbd_params = {
'nbd_protocol': "tcp",
'nbd_hostname': nbd_hostname,
'nbd_tcp_port': nbd_tcp_port,
'nbd_export': target_disk
}
backup_options = prepare_backup_xml(test, params, backup_type="full")
virsh.backup_begin(vm_name, backup_options, debug=True, ignore_status=False)
try:
utils_backup.pull_full_backup_to_file(nbd_params, backup_file_path)
except Exception as details:
test.fail("Fail to get full backup data: %s" % details)
Comment on lines +61 to +62
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use specific exception handling instead of catching all exceptions.

Catching all exceptions with Exception can mask programming errors and make debugging difficult.

Replace the broad exception handling with specific exceptions:

-    except Exception as details:
-        test.fail("Fail to get full backup data: %s" % details)
+    except (OSError, IOError, RuntimeError) as details:
+        test.fail("Fail to get full backup data: %s" % details)
📝 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
except Exception as details:
test.fail("Fail to get full backup data: %s" % details)
except (OSError, IOError, RuntimeError) as details:
test.fail("Fail to get full backup data: %s" % details)
🧰 Tools
🪛 Ruff (0.12.2)

60-60: Do not catch blind exception: Exception

(BLE001)

🤖 Prompt for AI Agents
In libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py around
lines 60-61, the code currently catches a broad Exception when failing to get
full backup data; replace this with explicit exception handlers for the errors
you actually expect (for example libvirt.libvirtError for libvirt API failures,
ValueError/KeyError for parsing issues, OSError or subprocess.CalledProcessError
for I/O or external command failures). Handle each expected exception separately
to call test.fail with the error details, and allow any unexpected exceptions to
propagate (or re-raise them) so programming errors aren’t masked.

test.log.debug("Full backup to %s" % backup_file_path)
return backup_file_path


def start_incremental_backup(test, params):
"""
Start an incremental backup.

:return: return the backup file path.
"""
vm_name = params.get("main_vm")
backup_file_path = data_dir.get_data_dir() + '/inc.backup'
nbd_hostname = params.get("nbd_hostname")
nbd_tcp_port = params.get("nbd_tcp_port")
target_disk = params.get("target_disk")
nbd_bitmap_name = "backup-" + target_disk
original_disk_size = params.get("original_disk_size", "10G")
nbd_params = {
'nbd_hostname': nbd_hostname,
'nbd_tcp_port': nbd_tcp_port,
'nbd_export': target_disk
}
backup_options = prepare_backup_xml(test, params, backup_type="inc")
virsh.backup_begin(vm_name, backup_options, debug=True, ignore_status=False)
try:
utils_backup.pull_incremental_backup_to_file(
nbd_params, backup_file_path, nbd_bitmap_name,
original_disk_size)
except Exception as details:
test.fail("Fail to get incremental backup data: %s" % details)
return backup_file_path


def test_save_vm(test, params, backup_file_list):
"""
Test save vm with incremental backup.

:return: return the list of backup files.
"""
if backup_file_list is None:
backup_file_list = []
vm_name = params.get("main_vm")
expected_error = params.get("expected_error")
test.log.info("Start full backup.")
backup_file_path = start_full_backup(test, params)
backup_file_list.append(backup_file_path)

test.log.info("Do save before abort the backup job.")
save_file = data_dir.get_data_dir() + '/%s.save' % vm_name
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if data_dir.get_tmp_dir() can be used by you which does not need cleanup. But this is not required.

save_result = virsh.save(vm_name, save_file, debug=True)
libvirt.check_result(save_result, expected_error)
abort_result = virsh.domjobabort(vm_name)
libvirt.check_exit_status(abort_result)

test.log.info("Do save after abort the backup job.")
virsh.save(vm_name, save_file, debug=True, ignore_status=False)
virsh.restore(save_file, debug=True, ignore_status=False)
if os.path.exists(save_file):
os.remove(save_file)

test.log.info("Start incremental backup.")
backup_file_path = start_incremental_backup(test, params)
backup_file_list.append(backup_file_path)
return backup_file_list


def test_managedsave(test, params, backup_file_list):
"""
Test managedsave vm with incremental backup.

:return: return the list of backup files
"""
if backup_file_list is None:
backup_file_list = []
vm_name = params.get("main_vm")
expected_error = params.get("expected_error")
test.log.info("Start full backup.")
backup_file_path = start_full_backup(test, params)
backup_file_list.append(backup_file_path)

test.log.info("Do managedsave before abort the backup job.")
save_result = virsh.managedsave(vm_name, debug=True)
libvirt.check_result(save_result, expected_error)
virsh.domjobabort(vm_name)

test.log.info("Do managedsave after abort the backup job.")
virsh.managedsave(vm_name, debug=True, ignore_status=False)
virsh.start(vm_name)
test.log.info("Start incremental backup.")
backup_file_path = start_incremental_backup(test, params)
backup_file_list.append(backup_file_path)
return backup_file_list


def test_restart_service(test, params, backup_file_list):
"""
Test restart libvirtd/virtqemud service after backup.

:return: return the list of backup files.
"""
if backup_file_list is None:
backup_file_list = []
vm_name = params.get("main_vm")
expected_error = params.get("expected_error")
test.log.info("Start full backup.")
backup_file_path = start_full_backup(test, params)
backup_file_list.append(backup_file_path)

test.log.info("Restart libvirtd/virtqemud service.")
Libvirtd().restart()

test.log.info("Start incremental backup")
backup_options = prepare_backup_xml(test, params, backup_type="inc")
result = virsh.backup_begin(vm_name, backup_options, debug=True)
libvirt.check_result(result, expected_error)
return backup_file_list


def test_kill_qemu_during_libvirtd_restart(test, params, backup_file_list):
"""
Kill qemu process between libvirtd stop/start when there is an existing pull-mode backup job.

:return: return the list of backup files.
"""
if backup_file_list is None:
backup_file_list = []
vm_name = params.get("main_vm")
test.log.info("Start full backup.")
backup_file_path = start_full_backup(test, params)
backup_file_list.append(backup_file_path)

test.log.info("Stop libvirt daemon and kill qemu process.")
Libvirtd().stop()
process.run("kill -9 `pidof qemu-kvm`", shell=True, ignore_status=False)
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Security risk: Avoid shell=True with process execution.

Using shell=True with process.run() creates a security vulnerability as it can be exploited through shell injection.

Use a safer approach without shell expansion:

-    process.run("kill -9 `pidof qemu-kvm`", shell=True, ignore_status=False)
+    # Get qemu-kvm PIDs and kill them
+    try:
+        pidof_result = process.run("pidof qemu-kvm", shell=False, ignore_status=True)
+        if pidof_result.exit_status == 0:
+            pids = pidof_result.stdout_text.strip().split()
+            for pid in pids:
+                process.run(["kill", "-9", pid], shell=False, ignore_status=False)
+    except Exception as e:
+        test.log.warning("Failed to kill qemu-kvm processes: %s" % e)
📝 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
process.run("kill -9 `pidof qemu-kvm`", shell=True, ignore_status=False)
# Get qemu-kvm PIDs and kill them
try:
pidof_result = process.run("pidof qemu-kvm", shell=False, ignore_status=True)
if pidof_result.exit_status == 0:
pids = pidof_result.stdout_text.strip().split() # stdout_text is provided by CmdResult([avocado-framework.readthedocs.io](https://avocado-framework.readthedocs.io/en/61.0/api/utils/avocado.utils.html?utm_source=openai))
for pid in pids:
process.run(["kill", "-9", pid], shell=False, ignore_status=False)
except Exception as e:
test.log.warning("Failed to kill qemu-kvm processes: %s" % e)
🧰 Tools
🪛 Ruff (0.12.2)

181-181: Function call with shell=True parameter identified, security issue

(S604)

🤖 Prompt for AI Agents
libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py around line
181: the current call uses process.run("kill -9 `pidof qemu-kvm`", shell=True)
which is a shell injection risk; replace it with a non-shell invocation such as
calling pkill directly (e.g. run ["pkill","-9","qemu-kvm"] with check/ignore
status as appropriate) or programmatically obtain PIDs by running pidof without
shell and then pass the numeric PIDs to kill (e.g. capture output from running
["pidof","qemu-kvm"], split into IDs, and call kill with a list of args),
ensuring shell=True is removed.


test.log.info("Start libvirt daemon and start the guest again.")
Libvirtd().start()
dom_state = virsh.domstate(vm_name).stdout.strip()
if "shut off" not in dom_state:
test.fail("The guest doesn't shutoff as expected!")
start_result = virsh.start(vm_name, debug=True)
libvirt.check_exit_status(start_result)
return backup_file_list


def run(test, params, env):
"""
Test vm lifecycle with incremental backup
"""
vm_name = params.get("main_vm")
vm = env.get_vm(vm_name)
case = params.get('test_case', '')

vmxml = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name)
vmxml_backup = vmxml.copy()

test_functions = {
'save_vm': test_save_vm,
'managedsave': test_managedsave,
'restart_service': test_restart_service,
'kill_qemu_during_libvirtd_restart': test_kill_qemu_during_libvirtd_restart
}
run_test = test_functions.get(case)
if not run_test:
test.error(f"Unknown test case: {case}")

try:
backup_file_list = None
if not vm.is_alive():
vm.start()
backup_file_list = run_test(test, params, backup_file_list)
finally:
utils_backup.clean_checkpoints(vm_name)
vmxml_backup.sync()
if backup_file_list:
for file in backup_file_list:
if os.path.exists(file):
os.remove(file)