Inc_backup: fix a size check failure #6660
Conversation
WalkthroughThis change refactors Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libvirt/tests/src/incremental_backup/incremental_backup_pull_mode_info.py (1)
128-147: Hardencheck_domblk_infoagainst parsing issues and update the docstringRight now
block_allocationanddisk_capacity_bytesare only set inside the loop that parsesdomblkinfooutput. If for any reason the output format changes or a line is missing, this will raiseUnboundLocalErrorinstead of a clear test failure. The docstring also still documents the oldtarget_min/target_maxparameters.Consider initializing the parsed values to
None, validating them after the loop, and updating the docstring to the new parameters:- def check_domblk_info(dd_count, dd_seek): - """ - Check the domblk info. - - :params target_min: the min value of the block allocation - :params target_max: the max value of the block allocation - """ - test.log.debug("TEST STEP: check the domblk info") - domblk_output = virsh.domblkinfo(vm_name, target_disk, debug=True) - for line in domblk_output.stdout_text.splitlines(): - if "Allocation" in line: - block_allocation = float(line.split(":")[-1].strip()) - if "Capacity" in line: - disk_capacity_bytes = float(line.split(":")[-1].strip()) - capacity_MB = disk_capacity_bytes / 1024 / 1024 - write_end_MB = int(dd_seek) + int(dd_count) - expected_MB = min(write_end_MB, capacity_MB) - target_min = expected_MB * 0.8 - target_max = expected_MB * 1.2 - block_allocation = block_allocation / 1024 / 1024 + def check_domblk_info(dd_count, dd_seek): + """ + Check the domblk info. + + :param dd_count: dd block count used when writing to the disk (in MiB) + :param dd_seek: dd seek offset used when writing to the disk (in MiB) + """ + test.log.debug("TEST STEP: check the domblk info") + domblk_output = virsh.domblkinfo(vm_name, target_disk, debug=True) + + block_allocation_bytes = None + disk_capacity_bytes = None + for line in domblk_output.stdout_text.splitlines(): + if "Allocation" in line: + block_allocation_bytes = float(line.split(":")[-1].strip()) + if "Capacity" in line: + disk_capacity_bytes = float(line.split(":")[-1].strip()) + + if block_allocation_bytes is None or disk_capacity_bytes is None: + test.fail("Failed to parse Allocation/Capacity from domblkinfo output: %s" + % domblk_output.stdout_text) + + capacity_MB = disk_capacity_bytes / 1024 / 1024 + write_end_MB = int(dd_seek) + int(dd_count) + expected_MB = min(write_end_MB, capacity_MB) + target_min = expected_MB * 0.8 + target_max = expected_MB * 1.2 + block_allocation = block_allocation_bytes / 1024 / 1024This keeps the new capacity-aware logic but makes failures deterministic and easier to debug, and the docstring matches the actual API.
🧹 Nitpick comments (1)
libvirt/tests/src/incremental_backup/incremental_backup_pull_mode_info.py (1)
51-58: Keepdd_count/dd_seekusage consistent with the actual dd writes
check_domblk_infonow derives expectations fromdd_countanddd_seek, butwrite_datasstill hardcodescount="200", anddd_seekis used as a string first ("0") and then an int (300). This works today but makes the test fragile if parameters change.To keep everything in sync and avoid surprises, consider:
- Passing
dd_countintowrite_datasand using it instead of the literal"200".- Treating
dd_seekas an int everywhere and converting tostronly when building the dd command.For example:
- def write_datas(dd_seek): + def write_datas(dd_seek, dd_count): """ Write datas to the disk in guest. """ vm_session = vm.wait_for_login() new_disk = libvirt_disk.get_non_root_disk_name(vm_session)[0] - utils_disk.dd_data_to_vm_disk(vm_session, "/dev/%s" % new_disk, seek=dd_seek, count="200") + utils_disk.dd_data_to_vm_disk(vm_session, "/dev/%s" % new_disk, + seek=str(dd_seek), count=str(dd_count)) vm_session.close() @@ - dd_count = int(params.get("dd_count")) + dd_count = int(params.get("dd_count")) @@ - dd_seek = "0" + dd_seek = 0 @@ - write_datas(dd_seek) + write_datas(dd_seek, dd_count) @@ - check_domblk_info(dd_count, dd_seek) - dd_seek = 300 - write_datas(dd_seek) - check_domblk_info(dd_count, dd_seek) + check_domblk_info(dd_count, dd_seek) + dd_seek = 300 + write_datas(dd_seek, dd_count) + check_domblk_info(dd_count, dd_seek) @@ - check_domblk_info(dd_count, dd_seek) + check_domblk_info(dd_count, dd_seek)This keeps the new size-check behaviour but makes it much clearer that dd parameters and expectations are aligned, and avoids implicit type changes on
dd_seek.Also applies to: 128-147, 167-169, 177-177, 190-195
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libvirt/tests/src/incremental_backup/incremental_backup_pull_mode_info.py(4 hunks)
⏰ 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.11
- GitHub Check: Python 3.12
- GitHub Check: Python 3.9
| """ | ||
| Check the domblk info. | ||
| :params target_min: the min value of the block allocation |
There was a problem hiding this comment.
Please update docstring. Others LGTM.
There was a problem hiding this comment.
Finish to update the docstring, thanks.
Sometimes the actual block allocation isn't in the configured test scope. Because dd_seek is used in data writes and due to the sparsity of the qcow2 format. Now adjust the size check method to fix it. Signed-off-by: Meina Li <meili@redhat.com>
meinaLi
force-pushed
the
fix_domblkinfo
branch
from
74c8d04 to
40be6fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libvirt/tests/src/incremental_backup/incremental_backup_pull_mode_info.py (1)
174-174: Inconsistent type fordd_seek: string vs integer.
dd_seekis initialized as a string"0"at line 174, but reassigned to an integer300at line 195. While theint()casts incheck_domblk_info(line 147) handle this, the inconsistency could cause issues elsewhere (e.g., inwrite_dataswhich passesdd_seektoutils_disk.dd_data_to_vm_disk).Consider using a consistent type:
- dd_seek = "0" + dd_seek = 0or:
- dd_seek = 300 + dd_seek = "300"Also applies to: 195-195
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libvirt/tests/src/incremental_backup/incremental_backup_pull_mode_info.py(3 hunks)
⏰ 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_pull_mode_info.py (2)
128-138: Docstring updated as requested.The function signature and docstring now clearly document the new parameters
dd_countanddd_seek, along with the expected behavior and error conditions. This addresses the previous review feedback.
146-154: Logic correctly handles dd_seek offset and qcow2 sparsity.The calculation properly accounts for:
- The actual write endpoint (
dd_seek + dd_count)- Disk capacity as an upper bound
- A ±20% tolerance for qcow2 format overhead/sparsity
This addresses the PR objective of fixing size check failures when
dd_seekis used.
| for line in domblk_output.stdout_text.splitlines(): | ||
| if "Allocation" in line: | ||
| block_allocation = int(line.split(":")[-1].strip()) | ||
| block_allocation = float(line.split(":")[-1].strip()) | ||
| if "Capacity" in line: | ||
| disk_capacity_bytes = float(line.split(":")[-1].strip()) | ||
| capacity_MB = disk_capacity_bytes / 1024 / 1024 | ||
| write_end_MB = int(dd_seek) + int(dd_count) | ||
| expected_MB = min(write_end_MB, capacity_MB) | ||
| target_min = expected_MB * 0.8 | ||
| target_max = expected_MB * 1.2 | ||
| block_allocation = block_allocation / 1024 / 1024 |
There was a problem hiding this comment.
Potential NameError if domblkinfo output is missing expected fields.
If the domblkinfo output lacks "Allocation" or "Capacity" lines, block_allocation or disk_capacity_bytes will be unbound, causing a NameError at line 146 or 151. Consider initializing these variables before the loop or adding a guard.
test.log.debug("TEST STEP: check the domblk info")
domblk_output = virsh.domblkinfo(vm_name, target_disk, debug=True)
+ block_allocation = None
+ disk_capacity_bytes = None
for line in domblk_output.stdout_text.splitlines():
if "Allocation" in line:
block_allocation = float(line.split(":")[-1].strip())
if "Capacity" in line:
disk_capacity_bytes = float(line.split(":")[-1].strip())
+ if block_allocation is None or disk_capacity_bytes is None:
+ test.fail("Failed to parse Allocation or Capacity from domblkinfo output")
capacity_MB = disk_capacity_bytes / 1024 / 1024Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In libvirt/tests/src/incremental_backup/incremental_backup_pull_mode_info.py
around lines 141 to 151, the parsing loop assumes "Allocation" and "Capacity"
are always present which can leave block_allocation or disk_capacity_bytes
undefined; initialize block_allocation and disk_capacity_bytes to None (or
sensible defaults) before the loop and after the loop check for None and either
raise a clear exception or handle the missing values (e.g., skip the test, set
defaults, or fail with a message) so you avoid a NameError and provide a
deterministic failure mode.
3 participants
Sometimes the actual block allocation isn't in the configured test scope. Because dd_seek is used in data writes and due to the sparsity of the qcow2 format. Now adjust the size check method to fix it.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.