Inc_backup: Automate incremental backup lifecycle test cases #6554
Conversation
meinaLi
force-pushed
the
inc_lifecycle
branch
from
6183805 to
d364035
Compare
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py (1)
214-216: File cleanup could fail silently.The file cleanup in the finally block doesn't handle potential exceptions, which could mask cleanup issues.
Add error handling for file cleanup:
for file in backup_file_list: if os.path.exists(file): - os.remove(file) + try: + os.remove(file) + test.log.debug(f"Removed backup file: {file}") + except OSError as e: + test.log.warning(f"Failed to remove backup file {file}: {e}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/incremental_backup/incremental_backup_lifecycle.cfg(1 hunks)libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/incremental_backup/incremental_backup_lifecycle.cfg
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py (3)
libvirt/tests/src/graphics/graphics_functional.py (1)
restore(231-242)libvirt/tests/src/passthrough/robustness/passthrough_robustness.py (2)
start(176-225)stop(227-238)provider/guest_os_booting/guest_os_booting_base.py (1)
get_vm(18-56)
🪛 Ruff (0.12.2)
libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py
22-22: Use of possibly insecure function; consider using ast.literal_eval
(S307)
27-27: Use of possibly insecure function; consider using ast.literal_eval
(S307)
29-29: Use of possibly insecure function; consider using ast.literal_eval
(S307)
60-60: Do not catch blind exception: Exception
(BLE001)
181-181: Function call with shell=True parameter identified, security issue
(S604)
203-203: Use of possibly insecure function; consider using ast.literal_eval
(S307)
libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py
Outdated
Show resolved
Hide resolved
| except Exception as details: | ||
| test.fail("Fail to get full backup data: %s" % details) |
There was a problem hiding this comment.
🛠️ 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.
| 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.
libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py
Outdated
Show resolved
Hide resolved
libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py
Outdated
Show resolved
Hide resolved
libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py
Outdated
Show resolved
Hide resolved
|
|
||
| test.log.info("Stop libvirt daemon and kill qemu process.") | ||
| Libvirtd().stop() | ||
| process.run("kill -9 `pidof qemu-kvm`", shell=True, ignore_status=False) |
There was a problem hiding this comment.
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.
| 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.
libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py
Outdated
Show resolved
Hide resolved
libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py
Outdated
Show resolved
Hide resolved
meinaLi
force-pushed
the
inc_lifecycle
branch
from
d364035 to
6b660f8
Compare
meinaLi
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py (7)
168-171: Fix typo in docstring (“exsiting” → “existing”).- Kill qemu process between libvirtd stop/start when there is an exsiting pull-mode backup job. + Kill qemu process between libvirtd stop/start when there is an existing pull-mode backup job.
204-205: Avoid global state for backup_file_list (pass state explicitly).Globals hinder test isolation and readability.
Refactor tests to accept a backup_file_list (or a small state object) passed from run(), and return it, removing the global entirely.
1-1: Security: replace eval() with ast.literal_eval for config parsing.Avoid executing arbitrary code when parsing params.
import os +import ast @@ - backup_dict = eval(params.get("backup_dict", "{}") % scratch_file) + backup_dict = ast.literal_eval(params.get("backup_dict", "{}") % scratch_file) @@ - checkpoint_dict = eval(params.get("checkpoint_dict") % inc_checkpoint) + checkpoint_dict = ast.literal_eval(params.get("checkpoint_dict") % inc_checkpoint) @@ - checkpoint_dict = eval(params.get("checkpoint_dict") % full_checkpoint) + checkpoint_dict = ast.literal_eval(params.get("checkpoint_dict") % full_checkpoint)Also applies to: 22-29
58-61: Don’t catch broad Exception — handle expected errors explicitly.Improve debuggability and avoid masking bugs.
- except Exception as details: + except (OSError, IOError, RuntimeError) as details: test.fail("Fail to get full backup data: %s" % details)
76-88: Incremental backup: make disk size configurable, include protocol, and add error handling.Prevents brittle tests and aligns with full-backup handling.
- nbd_bitmap_name = "backup-" + target_disk - original_disk_size = "10G" + 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 - } + nbd_params = { + 'nbd_protocol': "tcp", + 'nbd_hostname': nbd_hostname, + 'nbd_tcp_port': nbd_tcp_port, + 'nbd_export': target_disk + } @@ - utils_backup.pull_incremental_backup_to_file( - nbd_params, backup_file_path, nbd_bitmap_name, - original_disk_size) + try: + utils_backup.pull_incremental_backup_to_file( + nbd_params, backup_file_path, nbd_bitmap_name, original_disk_size) + except (OSError, IOError, RuntimeError) as details: + test.fail("Fail to get incremental backup data: %s" % details)
179-185: Security: avoid shell=True; kill qemu-kvm safely.Prevent shell injection; handle absence of processes gracefully.
- Libvirtd().stop() - process.run("kill -9 `pidof qemu-kvm`", shell=True, ignore_status=False) + Libvirtd().stop() + # Get qemu-kvm PIDs and kill them without using a shell + pidof_res = process.run(["pidof", "qemu-kvm"], shell=False, ignore_status=True) + if pidof_res.exit_status == 0: + for pid in pidof_res.stdout_text.strip().split(): + process.run(["kill", "-9", pid], shell=False, ignore_status=False)
201-206: Replace eval-based dispatcher with explicit mapping.Eliminates code execution risk and clarifies allowed cases.
- vmxml_backup = vmxml.copy() - run_test = eval('test_%s' % case) - global backup_file_list - backup_file_list = [] + vmxml_backup = vmxml.copy() + test_functions = { + "save_vm": test_save_vm, + "managedsave": test_managedsave, + "restart_service": test_restart_service, + "kill_qemu_process": test_kill_qemu_process, + } + run_test = test_functions.get(case) + if not run_test: + test.error(f"Unknown test case: {case}") + global backup_file_list + backup_file_list = []
🧹 Nitpick comments (3)
libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py (3)
15-21: Fix docstring: return value mismatch.Function returns only backup_options, not “backup options and the scratch file”.
def prepare_backup_xml(test, params, backup_type): """ Prepare the backup xml. - :params return: return the backup options and the scratch file. + :returns: backup options string to pass to virsh.backup_begin. """
104-113: Check domjobabort result and remove save file to avoid leftovers.Strengthens assertions and keeps workspace clean.
- virsh.domjobabort(vm_name) + abort_res = virsh.domjobabort(vm_name, debug=True) + libvirt.check_exit_status(abort_res) @@ virsh.restore(save_file, debug=True, ignore_status=False) + if os.path.exists(save_file): + os.remove(save_file)
137-143: Validate VM start result after managedsave.Ensure guest actually starts.
- virsh.start(vm_name) + start_res = virsh.start(vm_name, debug=True) + libvirt.check_exit_status(start_res)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/incremental_backup/incremental_backup_lifecycle.cfg(1 hunks)libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/incremental_backup/incremental_backup_lifecycle.cfg
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py (2)
libvirt/tests/src/graphics/graphics_functional.py (1)
restore(231-242)provider/guest_os_booting/guest_os_booting_base.py (1)
get_vm(18-56)
🪛 Ruff (0.12.2)
libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py
22-22: Use of possibly insecure function; consider using ast.literal_eval
(S307)
27-27: Use of possibly insecure function; consider using ast.literal_eval
(S307)
29-29: Use of possibly insecure function; consider using ast.literal_eval
(S307)
60-60: Do not catch blind exception: Exception
(BLE001)
181-181: Function call with shell=True parameter identified, security issue
(S604)
203-203: Use of possibly insecure function; consider using ast.literal_eval
(S307)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.8
- GitHub Check: Python 3.9
- GitHub Check: Python 3.12
- GitHub Check: Python 3.11
meinaLi
force-pushed
the
inc_lifecycle
branch
from
6b660f8 to
33b733b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py (7)
1-3: Missing import for ast.literal_eval.Add ast import to support safe parsing.
import os +import ast
58-62: Avoid broad except; catch expected errors only.Catching Exception masks bugs.
- except Exception as details: + except (OSError, IOError, RuntimeError) as details: test.fail("Fail to get full backup data: %s" % details)
86-92: Avoid broad except in incremental backup as well.Mirror the narrowed exceptions used in full backup.
- except Exception as details: + except (OSError, IOError, RuntimeError) as details: test.fail("Fail to get incremental backup data: %s" % details)
95-101: Refactor: pass backup_file_list explicitly to test_ functions.*Avoid globals; pass the list from run() to each test.
Apply these diffs:
-def test_save_vm(test, params): +def test_save_vm(test, params, backup_file_list): @@ - backup_file_list.append(backup_file_path) + backup_file_list.append(backup_file_path)-def test_managedsave(test, params): +def test_managedsave(test, params, backup_file_list): @@ - backup_file_list.append(backup_file_path) + backup_file_list.append(backup_file_path)-def test_restart_service(test, params): +def test_restart_service(test, params, backup_file_list): @@ - return backup_file_list + return backup_file_list-def test_kill_qemu_process(test, params): +def test_kill_qemu_process(test, params, backup_file_list): @@ - backup_file_list.append(backup_file_path) + backup_file_list.append(backup_file_path)And in run():
- backup_file_list = run_test(test, params) + backup_file_list = run_test(test, params, backup_file_list)Also applies to: 126-132, 152-158, 174-180, 216-217
21-29: Security: replace eval(...) with ast.literal_eval for config parsing.Executing eval on params is unsafe. Use ast.literal_eval.
Apply this diff:
- backup_dict = eval(params.get("backup_dict", "{}") % scratch_file) + backup_dict = ast.literal_eval(params.get("backup_dict", "{}") % scratch_file) @@ - checkpoint_dict = eval(params.get("checkpoint_dict") % inc_checkpoint) + checkpoint_dict = ast.literal_eval(params.get("checkpoint_dict") % inc_checkpoint) @@ - checkpoint_dict = eval(params.get("checkpoint_dict") % full_checkpoint) + checkpoint_dict = ast.literal_eval(params.get("checkpoint_dict") % full_checkpoint)Add the import at the top (see separate comment).
185-188: Security: avoid shell=True; kill qemu processes safely.shell=True with backticks is injection-prone. Use non-shell commands.
- Libvirtd().stop() - process.run("kill -9 `pidof qemu-kvm`", shell=True, ignore_status=False) + Libvirtd().stop() + # Get qemu-kvm PIDs and kill them safely without a shell + pidof_result = process.run("pidof qemu-kvm", shell=False, ignore_status=True) + if pidof_result.exit_status == 0: + for pid in pidof_result.stdout_text.strip().split(): + process.run(["kill", "-9", pid], shell=False, ignore_status=False) + else: + test.fail("pidof qemu-kvm returned no PIDs; guest may not be running as expected")
209-211: Security: replace dynamic eval dispatch and drop global state.Use an explicit dispatch map and avoid global backup_file_list.
- run_test = eval('test_%s' % case) - global backup_file_list - backup_file_list = [] + test_functions = { + 'save_vm': test_save_vm, + 'managedsave': test_managedsave, + 'restart_service': test_restart_service, + 'kill_qemu_process': test_kill_qemu_process, + } + run_test = test_functions.get(case) + if not run_test: + test.error(f"Unknown test case: {case}") + backup_file_list = []Follow-up: update test_* signatures to accept backup_file_list (see below) and pass it in run().
🧹 Nitpick comments (4)
libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py (4)
15-20: Docstring mismatch with return value.Function returns only backup_options, not scratch file.
- :return: return the backup options and the scratch file. + :return: backup options string for virsh.backup_begin.
56-58: Ensure backup_begin failures are not ignored.Explicitly fail on non-zero exit.
- virsh.backup_begin(vm_name, backup_options, debug=True) + result = virsh.backup_begin(vm_name, backup_options, debug=True, ignore_status=False) + libvirt.check_exit_status(result)
79-83: Include nbd_protocol for consistency.Full backup passes nbd_protocol, incremental should too.
- nbd_params = { - 'nbd_hostname': nbd_hostname, - 'nbd_tcp_port': nbd_tcp_port, - 'nbd_export': target_disk - } + nbd_params = { + 'nbd_protocol': "tcp", + 'nbd_hostname': nbd_hostname, + 'nbd_tcp_port': nbd_tcp_port, + 'nbd_export': target_disk + }
141-142: Check domjobabort result.Validate abort succeeded for consistency with test_save_vm.
- virsh.domjobabort(vm_name) + abort_result = virsh.domjobabort(vm_name) + libvirt.check_exit_status(abort_result)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/incremental_backup/incremental_backup_lifecycle.cfg(1 hunks)libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/incremental_backup/incremental_backup_lifecycle.cfg
🧰 Additional context used
🪛 Ruff (0.12.2)
libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py
22-22: Use of possibly insecure function; consider using ast.literal_eval
(S307)
27-27: Use of possibly insecure function; consider using ast.literal_eval
(S307)
29-29: Use of possibly insecure function; consider using ast.literal_eval
(S307)
60-60: Do not catch blind exception: Exception
(BLE001)
90-90: Do not catch blind exception: Exception
(BLE001)
187-187: Function call with shell=True parameter identified, security issue
(S604)
209-209: Use of possibly insecure function; consider using ast.literal_eval
(S307)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.12
- GitHub Check: Python 3.9
- GitHub Check: Python 3.11
- GitHub Check: Python 3.8
🔇 Additional comments (2)
libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py (2)
78-78: Good: disk size is now configurable.Reading original_disk_size from params improves flexibility.
176-176: Typo fixed in docstring.“existing” spelling is correct now.
libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py
Outdated
Show resolved
Hide resolved
libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py
Outdated
Show resolved
Hide resolved
| 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 |
There was a problem hiding this comment.
not sure if data_dir.get_tmp_dir() can be used by you which does not need cleanup. But this is not required.
libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py
Outdated
Show resolved
Hide resolved
meinaLi
force-pushed
the
inc_lifecycle
branch
from
33b733b to
2876a5b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py (1)
194-198: Avoid shell=True; kill qemu-kvm safely.Replace shell pipeline with non-shell calls to prevent injection and improve robustness. (Previously flagged; reiterating.)
- Libvirtd().stop() - process.run("kill -9 `pidof qemu-kvm`", shell=True, ignore_status=False) + Libvirtd().stop() + # Kill all qemu-kvm PIDs without using a shell + pidof_res = process.run(["pidof", "qemu-kvm"], ignore_status=True) + if pidof_res.exit_status == 0: + for pid in pidof_res.stdout_text.strip().split(): + process.run(["kill", "-9", pid], ignore_status=False)
🧹 Nitpick comments (6)
libvirt/tests/cfg/incremental_backup/incremental_backup_lifecycle.cfg (3)
13-13: Remove unused inc_backup_dict (avoid config drift).The Python test constructs incremental options programmatically; this dict is never consumed. Drop it to reduce confusion.
- 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'}}}]}
16-20: Relax expected_error regex to be libvirt-version agnostic.Embedding driver internals like remoteDispatchDomainBackupBegin/monitor is brittle across versions/transports. Match the stable text only.
- expected_error = "Timed out during operation: cannot acquire state change lock.*remoteDispatchDomainBackupBegin" + expected_error = "Timed out during operation: cannot acquire state change lock" - expected_error = "Timed out during operation: cannot acquire state change lock.*remoteDispatchDomainBackupBegin" + expected_error = "Timed out during operation: cannot acquire state change lock" - expected_error = "Timed out during operation: cannot acquire state change lock.*monitor" + expected_error = "Timed out during operation: .*monitor"
10-10: Port collision risk on shared runners.Hard-coding 10809 may clash if suites run in parallel. Consider making the port overridable per job (and document it), or auto-pick a free port.
libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py (3)
16-21: Docstring-return mismatch.Function returns only backup_options, not “backup options and the scratch file.” Update the docstring or return the scratch path and clean it up later.
- :return: return the backup options and the scratch file. + :return: backup options string for virsh.backup_begin.
60-62: Use narrower exceptions for clearer failures.Catching Exception masks programming errors. Prefer CmdError/OS errors (and optionally a utils_backup-specific error if exposed).
- except Exception as details: + except (process.CmdError, OSError, RuntimeError) as details: test.fail("Fail to get full backup data: %s" % details)- except Exception as details: + except (process.CmdError, OSError, RuntimeError) as details: test.fail("Fail to get incremental backup data: %s" % details)Also applies to: 91-92
80-85: Add nbd_protocol to incremental pull (match full backup params).Keep NBD params consistent; avoids hidden defaults changing behavior.
nbd_params = { + 'nbd_protocol': "tcp", 'nbd_hostname': nbd_hostname, 'nbd_tcp_port': nbd_tcp_port, 'nbd_export': target_disk }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/incremental_backup/incremental_backup_lifecycle.cfg(1 hunks)libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py (1)
libvirt/tests/src/incremental_backup/incremental_backup_pull_mode_tls.py (2)
backup_begin(85-95)pull_full_backup_to_file(123-142)
🪛 Ruff (0.14.1)
libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py
61-61: Do not catch blind exception: Exception
(BLE001)
91-91: Do not catch blind exception: Exception
(BLE001)
196-196: Function call with shell=True parameter identified, security issue
(S604)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.12
- GitHub Check: Python 3.9
- GitHub Check: Python 3.11
- GitHub Check: Python 3.8
🔇 Additional comments (3)
libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py (3)
22-31: Good move: ast.literal_eval for config dicts.Safer than eval; avoids arbitrary code execution.
219-227: Good move: explicit dispatch map.Removes unsafe eval-based dispatch; clearer and safer.
106-116: Verify backup job is active before save/abort operations; add domjobinfo check.The test lacks verification that the backup job is running when save/abort are called. Other similar tests in the codebase (incremental_backup_pull_mode.py:375–377, incremental_backup_fd_transport.py:81–82) verify job status with
domjobinfobefore performing concurrent operations. Without this check, ifpull_full_backup_to_fileblocks until completion (likely, given the synchronous call pattern), the save and abort operations run after the job ends, defeating the concurrency test.Add
domjobinfoverification immediately afterstart_full_backup()returns to confirm "Backup" is in the job status before proceeding with save/abort. This also applies to thetest_managedsavefunction at lines 143–147.
libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py
Outdated
Show resolved
Hide resolved
meinaLi
force-pushed
the
inc_lifecycle
branch
from
2876a5b to
03f61f8
Compare
libvirt/tests/src/incremental_backup/incremental_backup_lifecycle.py
Outdated
Show resolved
Hide resolved
libvirt/tests/cfg/incremental_backup/incremental_backup_lifecycle.cfg
Outdated
Show resolved
Hide resolved
Automate cases: RHEL-187642 pull mode backup and managed save vm together RHEL-187641 pull mode backup and save vm together RHEL-199045 Start an existing backup job after libvirtd restarted RHEL-199046 kill qemu process between libvirtd stop/start when there is an existing pull-mode backup job Signed-off-by: Meina Li <[email protected]>
meinaLi
force-pushed
the
inc_lifecycle
branch
from
03f61f8 to
67b54eb
Compare
4 participants
Automate cases:
RHEL-187642 pull mode backup and managed save vm together
RHEL-187641 pull mode backup and save vm together
RHEL-199045 Start an existing backup job after libvirtd restarted
RHEL-199046 kill qemu process between libvirtd stop/start when there is an existing pull-mode backup job
Summary by CodeRabbit