virsh_attach_device: add qcow2 disk at/dt test variants - #6911
Conversation
…lug/unplug - Implement full pool lifecycle (define > start > autostart > volume creation) - Support dirpool, fspool, and defaultpool with qcow2/raw formats - Validate attach-disk and attach-device-xml variants - Ensure cleanup via finally block (delete volume, destroy/undefine pool, remove target dir) Signed-off-by: Sneh Shikha Yadav <syadav@linux.ibm.com>
… variants - Add qcow2 disk variants for --live, --config, and --persistent attach cases - Enable attach followed by immediate detach for selected test scenarios - Introduce helpers for domblklist verification and device absence checks - Add support for non-root disk selection via /dev/disk/by-id - Handle --config attach with VM restart and verification - Validate persistent XML changes across VM restart - Extend detach verification for both active and persistent configurations Signed-off-by: Sneh Shikha Yadav <syadav@linux.ibm.com>
Add VirtualDiskQcow2Raw test variants to cover hot attach/detach of 100M qcow2 virtio disks across --live, --config, and --persistent modes. Enable attach+detach flow and validate behavior before/after attach, including restart handling for --config. Introduce VirtualDiskQcow2Raw class to support qcow2/raw formats using create_local_disk, set driver type, and verify disk via domblklist and guest checks. Signed-off-by: Sneh Shikha Yadav <syadav@linux.ibm.com>
WalkthroughThis PR adds a new Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libvirt/tests/src/storage/virsh_pool_vol.py`:
- Around line 376-400: The cleanup flow in virsh_pool_vol must not continue with
destructive filesystem operations after a failed pool destroy. Update the
teardown logic around pool_destroy, pool_undefine, and the subsequent rm
-rf/wipefs steps so that if an fs pool remains mounted or pool_destroy returns
failure, the code either bails out early or verifies the mount is gone before
touching pool_target or fs_source_dev. Use the existing pool_started,
pool_defined, and pool_type checks in the teardown path to gate the later
cleanup safely.
- Around line 178-214: The fs-pool device selection in virsh_pool_vol.py is too
permissive and can pick a non-mounted but still-in-use block device; update the
selection logic around the fs branch so it only uses an explicitly designated
scratch/loop device instead of scanning lsblk for the first “unused” disk or
partition. Adjust the code in the same block that builds fs_source_dev, calls
utlv.mkfs(), and later cleans up with wipefs so it no longer formats arbitrary
host storage, and make the same change in the related cleanup path referenced by
the review.
- Around line 345-364: The cleanup in virsh_pool_vol.py only detaches the live
disk when test_attach is set, so attach_device paths can leave the extra device
behind after later failures. Update the finally cleanup around the vm_name/_vm
handling to also detach disk_target for attach-device cases, using the existing
virsh.detach_disk logic after checking the VM is alive and the disk is present
in virsh.domblklist, so any successful attachment is always undone.
- Around line 51-75: `check_pool_list()` currently returns `False` when the pool
is found but inactive, which lets callers continue as if the assertion passed;
change this helper to fail the test in that inactive case just like the
missing/present checks do. Update the logic in `check_pool_list` so that when
`expect_present` is true and `pool_state` is "inactive", it raises a test
failure or otherwise asserts immediately instead of returning a boolean, keeping
the behavior consistent for callers such as `pool_start()` and
`pool_autostart()`.
In `@libvirt/tests/src/virsh_cmd/domain/virsh_attach_detach_disk.py`:
- Around line 741-752: The detach-disk flow is still validating active guest
state before the required reboot when --config is used. Update the detach-side
logic in virsh_attach_detach_disk.py alongside the existing attach-disk restart
handling so that detach-disk with config also restarts the VM, waits for login,
and then recomputes the guest and domblklist checks after restart using the
existing vm, vm.start, vm.wait_for_login, and check_domblklist paths.
- Around line 223-247: The root-disk exclusion logic in the host disk collection
block is only resolving one PKNAME hop, so it can miss the real boot disk when
`root_source` is on LVM, RAID, or device-mapper. Update the
`root_parent_result`/`root_disk` handling in `virsh_attach_detach_disk.py` to
fully resolve the underlying parent disk (or otherwise normalize to the actual
base block device) before filtering `host_disks`, so `lsblk_cmd` results never
include the boot device.
In `@libvirt/tests/src/virsh_cmd/domain/virsh_attach_device.py`:
- Around line 827-833: The override in make_image_file no longer preserves the
marker that VirtualDiskBasic.function still validates, so the inherited check in
super(...).function(index) will fail before the detach assertions run. Update
make_image_file to seed the formatted image with the expected sentinel based on
make_image_file_path(index), or adjust the test flow in
VirtualDiskBasic/function to skip that inherited content check when using
create_local_disk-generated images.
- Around line 1243-1245: The controller XML is being instantiated with the
literal device name instead of the controller bus/type, which bypasses
constructor-time setup in `vmxml.get_device_class('controller')` usage. Update
the `ppc_controller` creation in `virsh_attach_device.py` to follow the same
pattern used by `Controller.init_device()` and construct it with the appropriate
controller type/bus for the ppc64le virtio-scsi case, rather than patching
`.type` after creating it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4e0854d2-90ab-446a-88ed-3bbf5b63c555
📒 Files selected for processing (6)
libvirt/tests/cfg/storage/virsh_pool_vol.cfglibvirt/tests/cfg/virsh_cmd/domain/virsh_attach_detach_disk.cfglibvirt/tests/cfg/virsh_cmd/domain/virsh_attach_device.cfglibvirt/tests/src/storage/virsh_pool_vol.pylibvirt/tests/src/virsh_cmd/domain/virsh_attach_detach_disk.pylibvirt/tests/src/virsh_cmd/domain/virsh_attach_device.py
| def check_pool_list(expect_present=True): | ||
| """Return False only when pool_name is present but inactive.""" | ||
| result = virsh.pool_list("--all", ignore_status=True) | ||
| utlv.check_exit_status(result, False) | ||
| pool_line = next((line for line in result.stdout.strip().splitlines() | ||
| if pool_name in line), None) | ||
| found = pool_line is not None | ||
| pool_state = None | ||
| if pool_line: | ||
| pool_cols = pool_line.split() | ||
| if len(pool_cols) >= 2: | ||
| pool_state = pool_cols[1] | ||
| if expect_present and not found: | ||
| test.fail("Pool '%s' not found in 'virsh pool-list --all'" | ||
| % pool_name) | ||
| if expect_present and pool_state == "inactive": | ||
| logging.debug("pool-list check: pool='%s' present=%s state=%s (expected=%s)", | ||
| pool_name, found, pool_state, expect_present) | ||
| return False | ||
| if not expect_present and found: | ||
| test.fail("Pool '%s' still present in pool-list after cleanup" | ||
| % pool_name) | ||
| logging.debug("pool-list check: pool='%s' present=%s state=%s (expected=%s)", | ||
| pool_name, found, pool_state, expect_present) | ||
| return True |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail when the pool is defined but inactive.
Every caller uses check_pool_list() like an assertion, but the inactive case only returns False. After pool_start()/pool_autostart(), an inactive pool can therefore slip through and the test still reports success.
Suggested fix
def check_pool_list(expect_present=True):
- """Return False only when pool_name is present but inactive."""
+ """Assert pool presence/state in `virsh pool-list --all`."""
result = virsh.pool_list("--all", ignore_status=True)
utlv.check_exit_status(result, False)
pool_line = next((line for line in result.stdout.strip().splitlines()
if pool_name in line), None)
@@
if expect_present and not found:
test.fail("Pool '%s' not found in 'virsh pool-list --all'"
% pool_name)
if expect_present and pool_state == "inactive":
- logging.debug("pool-list check: pool='%s' present=%s state=%s (expected=%s)",
- pool_name, found, pool_state, expect_present)
- return False
+ test.fail("Pool '%s' is defined but inactive" % pool_name)
if not expect_present and found:
test.fail("Pool '%s' still present in pool-list after cleanup"
% pool_name)📝 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.
| def check_pool_list(expect_present=True): | |
| """Return False only when pool_name is present but inactive.""" | |
| result = virsh.pool_list("--all", ignore_status=True) | |
| utlv.check_exit_status(result, False) | |
| pool_line = next((line for line in result.stdout.strip().splitlines() | |
| if pool_name in line), None) | |
| found = pool_line is not None | |
| pool_state = None | |
| if pool_line: | |
| pool_cols = pool_line.split() | |
| if len(pool_cols) >= 2: | |
| pool_state = pool_cols[1] | |
| if expect_present and not found: | |
| test.fail("Pool '%s' not found in 'virsh pool-list --all'" | |
| % pool_name) | |
| if expect_present and pool_state == "inactive": | |
| logging.debug("pool-list check: pool='%s' present=%s state=%s (expected=%s)", | |
| pool_name, found, pool_state, expect_present) | |
| return False | |
| if not expect_present and found: | |
| test.fail("Pool '%s' still present in pool-list after cleanup" | |
| % pool_name) | |
| logging.debug("pool-list check: pool='%s' present=%s state=%s (expected=%s)", | |
| pool_name, found, pool_state, expect_present) | |
| return True | |
| def check_pool_list(expect_present=True): | |
| """Assert pool presence/state in `virsh pool-list --all`.""" | |
| result = virsh.pool_list("--all", ignore_status=True) | |
| utlv.check_exit_status(result, False) | |
| pool_line = next((line for line in result.stdout.strip().splitlines() | |
| if pool_name in line), None) | |
| found = pool_line is not None | |
| pool_state = None | |
| if pool_line: | |
| pool_cols = pool_line.split() | |
| if len(pool_cols) >= 2: | |
| pool_state = pool_cols[1] | |
| if expect_present and not found: | |
| test.fail("Pool '%s' not found in 'virsh pool-list --all'" | |
| % pool_name) | |
| if expect_present and pool_state == "inactive": | |
| test.fail("Pool '%s' is defined but inactive" % pool_name) | |
| if not expect_present and found: | |
| test.fail("Pool '%s' still present in pool-list after cleanup" | |
| % pool_name) | |
| logging.debug("pool-list check: pool='%s' present=%s state=%s (expected=%s)", | |
| pool_name, found, pool_state, expect_present) | |
| return True |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libvirt/tests/src/storage/virsh_pool_vol.py` around lines 51 - 75,
`check_pool_list()` currently returns `False` when the pool is found but
inactive, which lets callers continue as if the assertion passed; change this
helper to fail the test in that inactive case just like the missing/present
checks do. Update the logic in `check_pool_list` so that when `expect_present`
is true and `pool_state` is "inactive", it raises a test failure or otherwise
asserts immediately instead of returning a boolean, keeping the behavior
consistent for callers such as `pool_start()` and `pool_autostart()`.
| if pool_type == "fs": | ||
| root_src = process.run( | ||
| "findmnt -n -o SOURCE /", | ||
| shell=True, ignore_status=False).stdout_text.strip() | ||
| root_dev_name = os.path.basename(root_src) | ||
| root_pkname = process.run( | ||
| "lsblk -ndo PKNAME %s" % root_src, | ||
| shell=True, ignore_status=True).stdout_text.strip() | ||
| root_disk_name = root_pkname or root_dev_name | ||
| lsblk_cmd = "lsblk -dn -o NAME,TYPE,MOUNTPOINT,PKNAME" | ||
| lsblk_result = process.run(lsblk_cmd, shell=True, ignore_status=False) | ||
| raw_disk = None | ||
| for line in lsblk_result.stdout_text.strip().splitlines(): | ||
| cols = line.split(None, 3) | ||
| if len(cols) < 2: | ||
| continue | ||
| dev_name = cols[0] | ||
| dev_type = cols[1] | ||
| mountpoint = cols[2] if len(cols) > 2 else "" | ||
| parent_name = cols[3] if len(cols) > 3 else "" | ||
| if dev_type == "part": | ||
| if mountpoint: | ||
| continue | ||
| if parent_name == root_disk_name: | ||
| continue | ||
| fs_source_dev = "/dev/%s" % dev_name | ||
| break | ||
| if dev_type == "disk" and not mountpoint and dev_name != root_disk_name: | ||
| raw_disk = "/dev/%s" % dev_name | ||
| if not fs_source_dev and raw_disk: | ||
| fs_source_dev = raw_disk | ||
| if not fs_source_dev: | ||
| test.cancel("No safe unused partition or raw disk found for fs pool") | ||
| logging.info("Using fs pool source device: %s", fs_source_dev) | ||
| utlv.mkfs(fs_source_dev, pool_source_format) | ||
| pool_define_extra = "--source-dev %s --source-format %s" % ( | ||
| fs_source_dev, pool_source_format) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Do not auto-format the first “unused” block device.
This probe only excludes mounted devices and the root disk. Unmounted swap/LVM/RAID/multipath members or other data disks can still be selected, then mkfs() and cleanup wipefs -a will destroy host data. Please require an explicit scratch device/loop device for fs variants instead of formatting whatever lsblk returns first.
Also applies to: 397-400
🧰 Tools
🪛 Ruff (0.15.20)
[error] 179-179: Function call with shell=True parameter identified, security issue
(S604)
[error] 183-183: Function call with shell=True parameter identified, security issue
(S604)
[error] 188-188: Function call with shell=True parameter identified, security issue
(S604)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libvirt/tests/src/storage/virsh_pool_vol.py` around lines 178 - 214, The
fs-pool device selection in virsh_pool_vol.py is too permissive and can pick a
non-mounted but still-in-use block device; update the selection logic around the
fs branch so it only uses an explicitly designated scratch/loop device instead
of scanning lsblk for the first “unused” disk or partition. Adjust the code in
the same block that builds fs_source_dev, calls utlv.mkfs(), and later cleans up
with wipefs so it no longer formats arbitrary host storage, and make the same
change in the related cleanup path referenced by the review.
| if test_attach and vm_name: | ||
| _vm = env.get_vm(vm_name) | ||
| if _vm is None: | ||
| vm_type = params.get("vm_type", "libvirt") | ||
| _vm = env.create_vm(vm_type, params.get("target"), | ||
| vm_name, params, test.bindir) | ||
| if _vm is not None and not _vm.is_dead(): | ||
| bl = virsh.domblklist(vm_name, ignore_status=True) | ||
| if bl.exit_status == 0: | ||
| for _line in bl.stdout_text.strip().splitlines(): | ||
| _cols = _line.split() | ||
| if _cols and _cols[0] == disk_target: | ||
| logging.debug("Cleanup: detaching live target " | ||
| "'%s' from '%s'", | ||
| disk_target, vm_name) | ||
| virsh.detach_disk(vm_name, disk_target, | ||
| extra=dt_options, | ||
| ignore_status=True, debug=True) | ||
| break | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Cleanup should cover attach-device failures too.
The finally block only detaches disk_target when test_attach is true. If attach_device succeeds and detach_device or a later check fails, the extra disk stays attached and can block vol-delete/pool teardown or leak guest state into later cases.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libvirt/tests/src/storage/virsh_pool_vol.py` around lines 345 - 364, The
cleanup in virsh_pool_vol.py only detaches the live disk when test_attach is
set, so attach_device paths can leave the extra device behind after later
failures. Update the finally cleanup around the vm_name/_vm handling to also
detach disk_target for attach-device cases, using the existing virsh.detach_disk
logic after checking the VM is alive and the disk is present in
virsh.domblklist, so any successful attachment is always undone.
| # Destroy the pool | ||
| if pool_started: | ||
| logging.debug("Destroying pool '%s'", pool_name) | ||
| if not virsh.pool_destroy(pool_name): | ||
| logging.warning("pool-destroy returned non-zero for '%s'", | ||
| pool_name) | ||
|
|
||
| if pool_defined: | ||
| logging.debug("Undefining pool '%s'", pool_name) | ||
| res = virsh.pool_undefine(pool_name, ignore_status=True, debug=True) | ||
| if res.exit_status: | ||
| logging.warning("pool-undefine failed for '%s': %s", | ||
| pool_name, res.stderr.strip()) | ||
| else: | ||
| check_pool_list(expect_present=False) | ||
|
|
||
| if os.path.isdir(pool_target): | ||
| logging.debug("Removing pool target directory: %s", pool_target) | ||
| process.run("rm -rf %s" % pool_target, | ||
| shell=True, ignore_status=True) | ||
|
|
||
| if pool_type == "fs" and fs_source_dev: | ||
| logging.debug("Cleaning filesystem signature on source device: %s", | ||
| fs_source_dev) | ||
| process.run("wipefs -a %s" % fs_source_dev, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not run destructive cleanup after a failed pool-destroy.
If virsh pool-destroy leaves an fs pool mounted, the later rm -rf pool_target runs inside that mounted filesystem and wipefs -a still hits the backing device. Bail out or verify the mount is gone before either destructive step.
🧰 Tools
🪛 Ruff (0.15.20)
[error] 394-394: Function call with shell=True parameter identified, security issue
(S604)
[error] 400-400: Function call with shell=True parameter identified, security issue
(S604)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libvirt/tests/src/storage/virsh_pool_vol.py` around lines 376 - 400, The
cleanup flow in virsh_pool_vol must not continue with destructive filesystem
operations after a failed pool destroy. Update the teardown logic around
pool_destroy, pool_undefine, and the subsequent rm -rf/wipefs steps so that if
an fs pool remains mounted or pool_destroy returns failure, the code either
bails out early or verifies the mount is gone before touching pool_target or
fs_source_dev. Use the existing pool_started, pool_defined, and pool_type checks
in the teardown path to gate the later cleanup safely.
| root_parent_result = process.run( | ||
| "lsblk -ndo PKNAME %s" % root_source, | ||
| shell=True, | ||
| ignore_status=True | ||
| ) | ||
| root_parent = root_parent_result.stdout_text.strip() | ||
| if root_parent: | ||
| root_disk = "/dev/%s" % root_parent | ||
| else: | ||
| root_disk = root_source | ||
|
|
||
| lsblk_cmd = "lsblk -dn -o NAME,TYPE" | ||
| lsblk_result = process.run(lsblk_cmd, shell=True, ignore_status=False) | ||
| host_disks = [] | ||
| for line in lsblk_result.stdout_text.strip().splitlines(): | ||
| columns = line.split() | ||
| if len(columns) != 2: | ||
| continue | ||
| disk_name, disk_type = columns | ||
| if disk_type != "disk": | ||
| continue | ||
| disk_path = "/dev/%s" % disk_name | ||
| if disk_path == root_disk: | ||
| continue | ||
| host_disks.append(disk_path) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Root-disk filtering still admits LVM/device-mapper boot disks.
Line 223 only peels one PKNAME layer. On hosts where / sits on LVM, RAID, or another dm stack, that usually resolves to a partition or dm node, so host_disks can still include the actual system disk and this helper may hand /dev/disk/by-id/... for the boot device back to the test. That defeats the “non-root” contract and can attach the host root disk to the guest.
Proposed fix
+import shlex
+
root_source = root_source.split("[", 1)[0].strip()
- root_parent_result = process.run(
- "lsblk -ndo PKNAME %s" % root_source,
- shell=True,
- ignore_status=True
- )
- root_parent = root_parent_result.stdout_text.strip()
- if root_parent:
- root_disk = "/dev/%s" % root_parent
- else:
- root_disk = root_source
+ root_disk = root_source
+ while True:
+ disk_type = process.run(
+ "lsblk -ndo TYPE %s" % shlex.quote(root_disk),
+ shell=True,
+ ignore_status=True,
+ ).stdout_text.strip()
+ if disk_type == "disk":
+ break
+ parent = process.run(
+ "lsblk -ndo PKNAME %s" % shlex.quote(root_disk),
+ shell=True,
+ ignore_status=True,
+ ).stdout_text.strip()
+ if not parent:
+ break
+ root_disk = "/dev/%s" % parent📝 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.
| root_parent_result = process.run( | |
| "lsblk -ndo PKNAME %s" % root_source, | |
| shell=True, | |
| ignore_status=True | |
| ) | |
| root_parent = root_parent_result.stdout_text.strip() | |
| if root_parent: | |
| root_disk = "/dev/%s" % root_parent | |
| else: | |
| root_disk = root_source | |
| lsblk_cmd = "lsblk -dn -o NAME,TYPE" | |
| lsblk_result = process.run(lsblk_cmd, shell=True, ignore_status=False) | |
| host_disks = [] | |
| for line in lsblk_result.stdout_text.strip().splitlines(): | |
| columns = line.split() | |
| if len(columns) != 2: | |
| continue | |
| disk_name, disk_type = columns | |
| if disk_type != "disk": | |
| continue | |
| disk_path = "/dev/%s" % disk_name | |
| if disk_path == root_disk: | |
| continue | |
| host_disks.append(disk_path) | |
| root_disk = root_source | |
| while True: | |
| disk_type = process.run( | |
| "lsblk -ndo TYPE %s" % shlex.quote(root_disk), | |
| shell=True, | |
| ignore_status=True, | |
| ).stdout_text.strip() | |
| if disk_type == "disk": | |
| break | |
| parent = process.run( | |
| "lsblk -ndo PKNAME %s" % shlex.quote(root_disk), | |
| shell=True, | |
| ignore_status=True, | |
| ).stdout_text.strip() | |
| if not parent: | |
| break | |
| root_disk = "/dev/%s" % parent | |
| lsblk_cmd = "lsblk -dn -o NAME,TYPE" | |
| lsblk_result = process.run(lsblk_cmd, shell=True, ignore_status=False) | |
| host_disks = [] | |
| for line in lsblk_result.stdout_text.strip().splitlines(): | |
| columns = line.split() | |
| if len(columns) != 2: | |
| continue | |
| disk_name, disk_type = columns | |
| if disk_type != "disk": | |
| continue | |
| disk_path = "/dev/%s" % disk_name | |
| if disk_path == root_disk: | |
| continue | |
| host_disks.append(disk_path) |
🧰 Tools
🪛 Ruff (0.15.20)
[error] 223-223: Function call with shell=True parameter identified, security issue
(S604)
[error] 235-235: Function call with shell=True parameter identified, security issue
(S604)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libvirt/tests/src/virsh_cmd/domain/virsh_attach_detach_disk.py` around lines
223 - 247, The root-disk exclusion logic in the host disk collection block is
only resolving one PKNAME hop, so it can miss the real boot disk when
`root_source` is on LVM, RAID, or device-mapper. Update the
`root_parent_result`/`root_disk` handling in `virsh_attach_detach_disk.py` to
fully resolve the underlying parent disk (or otherwise normalize to the actual
base block device) before filtering `host_disks`, so `lsblk_cmd` results never
include the boot device.
| # For --config flag, verify disk appears after VM restart | ||
| check_domblklist_after_restart = True | ||
| if test_cmd == "attach-disk" and at_options.count("config"): | ||
| # Restart VM to verify --config changes take effect | ||
| vm.start() | ||
| vm.wait_for_login() | ||
| check_domblklist_after_restart = check_domblklist(vm_name, | ||
| device_target, | ||
| device_source) | ||
| # Destroy VM after validation | ||
| vm.destroy(gracefully=False) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
detach-disk --config is verified before the required reboot.
The new dt_options.count("config") assertions below expect the device to be gone from the active guest and domblklist, but this restart block only handles attach-disk. For a running VM, virsh detach-disk --config updates inactive XML only, so Lines 633-648 are still checking the pre-reboot active domain and will fail on the success path. Mirror the attach-side reboot here and recompute the guest/domblklist checks after that restart.
Proposed fix
check_domblklist_after_restart = True
if test_cmd == "attach-disk" and at_options.count("config"):
# Restart VM to verify --config changes take effect
vm.start()
vm.wait_for_login()
check_domblklist_after_restart = check_domblklist(vm_name,
device_target,
device_source)
# Destroy VM after validation
vm.destroy(gracefully=False)
+ elif test_cmd == "detach-disk" and dt_options.count("config") \
+ and pre_vm_state != "shut off":
+ vm.start()
+ vm.wait_for_login()
+ check_vm_after_cmd = check_vm_partition(vm, device, os_type,
+ device_target)
+ check_domblklist_after_cmd = check_domblklist_absent(vm_name,
+ device_target)
+ vm.destroy(gracefully=False)📝 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.
| # For --config flag, verify disk appears after VM restart | |
| check_domblklist_after_restart = True | |
| if test_cmd == "attach-disk" and at_options.count("config"): | |
| # Restart VM to verify --config changes take effect | |
| vm.start() | |
| vm.wait_for_login() | |
| check_domblklist_after_restart = check_domblklist(vm_name, | |
| device_target, | |
| device_source) | |
| # Destroy VM after validation | |
| vm.destroy(gracefully=False) | |
| # For --config flag, verify disk appears after VM restart | |
| check_domblklist_after_restart = True | |
| if test_cmd == "attach-disk" and at_options.count("config"): | |
| # Restart VM to verify --config changes take effect | |
| vm.start() | |
| vm.wait_for_login() | |
| check_domblklist_after_restart = check_domblklist(vm_name, | |
| device_target, | |
| device_source) | |
| # Destroy VM after validation | |
| vm.destroy(gracefully=False) | |
| elif test_cmd == "detach-disk" and dt_options.count("config") \ | |
| and pre_vm_state != "shut off": | |
| vm.start() | |
| vm.wait_for_login() | |
| check_vm_after_cmd = check_vm_partition(vm, device, os_type, | |
| device_target) | |
| check_domblklist_after_cmd = check_domblklist_absent(vm_name, | |
| device_target) | |
| vm.destroy(gracefully=False) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libvirt/tests/src/virsh_cmd/domain/virsh_attach_detach_disk.py` around lines
741 - 752, The detach-disk flow is still validating active guest state before
the required reboot when --config is used. Update the detach-side logic in
virsh_attach_detach_disk.py alongside the existing attach-disk restart handling
so that detach-disk with config also restarts the VM, waits for login, and then
recomputes the guest and domblklist checks after restart using the existing vm,
vm.start, vm.wait_for_login, and check_domblklist paths.
| def make_image_file(self, index): | ||
| """Override to create a properly formatted image via create_local_disk.""" | ||
| from virttest.utils_test import libvirt as utlv | ||
| utlv.create_local_disk("file", | ||
| path=self.make_image_file_path(index), | ||
| size=str(self.meg / 1024.0), | ||
| disk_format=self.diskformat) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
This override drops the sentinel that the inherited functional check still reads.
VirtualDiskBasic.function() only passes when the guest block device ends with the marker derived from make_image_file_path(index) (see Lines 745-793), but make_image_file() now just creates a blank qcow2/raw image. That makes the super(...).function(index) call at Line 902 fail before any of the new detach validation runs. Either seed the image with the expected marker again or replace the inherited check with one that matches formatted images.
Also applies to: 901-907
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libvirt/tests/src/virsh_cmd/domain/virsh_attach_device.py` around lines 827 -
833, The override in make_image_file no longer preserves the marker that
VirtualDiskBasic.function still validates, so the inherited check in
super(...).function(index) will fail before the detach assertions run. Update
make_image_file to seed the formatted image with the expected sentinel based on
make_image_file_path(index), or adjust the test flow in
VirtualDiskBasic/function to skip that inherited content check when using
create_local_disk-generated images.
| # Use vmxml.get_device_class to create controller device properly | ||
| controller_class = vmxml.get_device_class('controller') | ||
| ppc_controller = controller_class(type_name='controller') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Instantiate the controller XML with the bus/type, not the literal "controller".
Elsewhere in this file, vmxml.get_device_class('controller') is constructed with the controller type (Controller.init_device() at Lines 604-606). Passing "controller" here initializes the device under the wrong type and only patches .type afterward, which can miss constructor-time setup for the ppc64le virtio-scsi controller.
Proposed fix
- controller_class = vmxml.get_device_class('controller')
- ppc_controller = controller_class(type_name='controller')
+ controller_class = vmxml.get_device_class('controller')
+ ppc_controller = controller_class(type_name=device_bus)📝 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.
| # Use vmxml.get_device_class to create controller device properly | |
| controller_class = vmxml.get_device_class('controller') | |
| ppc_controller = controller_class(type_name='controller') | |
| # Use vmxml.get_device_class to create controller device properly | |
| controller_class = vmxml.get_device_class('controller') | |
| ppc_controller = controller_class(type_name=device_bus) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libvirt/tests/src/virsh_cmd/domain/virsh_attach_device.py` around lines 1243
- 1245, The controller XML is being instantiated with the literal device name
instead of the controller bus/type, which bypasses constructor-time setup in
`vmxml.get_device_class('controller')` usage. Update the `ppc_controller`
creation in `virsh_attach_device.py` to follow the same pattern used by
`Controller.init_device()` and construct it with the appropriate controller
type/bus for the ppc64le virtio-scsi case, rather than patching `.type` after
creating it.
- Validate memory hotplug followed by virsh dump per iteration - Ensure consistent VM state and avoid race conditions during runtime - Improve error handling, logging, and cleanup to prevent spurious failures Signed-off-by: Sneh Shikha Yadav <syadav@linux.ibm.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libvirt/tests/cfg/memory/memory_hotplug_virshdump.cfg`:
- Around line 37-42: The memory hotplug virshdump test configuration is causing
excessive disk usage because the dump files accumulate under the dump directory
across many attach+dump cycles. Update the memory_hotplug_virshdump scenario to
either clean up each generated dump after verification or reduce the
iteration/count settings in the memory_hotplug_virshdump config so the run does
not produce ~100 GiB of output. Keep the fix localized to the virshdump/memory
hotplug test configuration and any related cleanup logic that handles dump_path
and dump_options.
In `@libvirt/tests/src/memory/memory_hotplug_virshdump.py`:
- Around line 204-239: The hotplug prerequisite setup in the branch around the
NUMA check is skipping max-memory and slot reconciliation whenever
`_numa_present` is true. Update the logic in the `memory_hotplug_virshdump` test
so NUMA cell creation remains conditional on `not _numa_present`, but the
`_vmxml.max_mem_rt`, `_vmxml.max_mem_rt_slots`, `_vmxml.max_mem_rt_unit`,
`_vmxml.max_mem`, and `_vmxml.current_mem` assignments always run when needed.
Use the existing `_vmxml.sync()` path and the surrounding `VMCPUXML`/`_cpuxml`
handling to ensure guests with pre-existing NUMA still get hotplug capacity
configured.
- Around line 23-27: The pacing-related params are defined but not actually
used, so the hotplug/dump flow ignores the configured timing and iteration
controls. Update the test logic around the hotplug loop and dump handling in
memory_hotplug_virshdump.py so mem_hotplug_delay is read and slept between
iterations, virshdump_delay is applied before each dump, and
virshdump_iterations controls how many dump operations are performed instead of
always doing one immediate dump. Make sure the relevant helpers/main flow that
drives virsh.dump and the attach loop use these symbols consistently so the
config knobs really affect the scenario.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 37feed29-ae68-4aef-8538-2d8f0e5d193c
📒 Files selected for processing (2)
libvirt/tests/cfg/memory/memory_hotplug_virshdump.cfglibvirt/tests/src/memory/memory_hotplug_virshdump.py
| mem_hotplug_iterations = 16 | ||
| mem_hotplug_delay = 10 | ||
| virshdump_iterations = 16 | ||
| virshdump_delay = 2 | ||
| dump_path = "./virsh_dumps" | ||
| dump_options = "--memory-only --bypass-cache" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
This variant will write roughly 100 GiB of dump data per run.
With mem=2097152, tg_size=524288, and 16 attach+dump cycles, the guest grows from ~2.5 GiB to ~10 GiB before each --memory-only dump. The implementation keeps every file under ./virsh_dumps, so this single case ends up around 100 GiB of disk writes. That is likely to fill many test workers and fail unrelated jobs. Please either delete each dump after validating it or reduce the iteration/count sizing here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libvirt/tests/cfg/memory/memory_hotplug_virshdump.cfg` around lines 37 - 42,
The memory hotplug virshdump test configuration is causing excessive disk usage
because the dump files accumulate under the dump directory across many
attach+dump cycles. Update the memory_hotplug_virshdump scenario to either clean
up each generated dump after verification or reduce the iteration/count settings
in the memory_hotplug_virshdump config so the run does not produce ~100 GiB of
output. Keep the fix localized to the virshdump/memory hotplug test
configuration and any related cleanup logic that handles dump_path and
dump_options.
| mem_hotplug_iterations = int(params.get("mem_hotplug_iterations", "10")) | ||
| virshdump_iterations = int(params.get("virshdump_iterations", "5")) | ||
| virshdump_delay = int(params.get("virshdump_delay", "10")) | ||
| dump_path = params.get("dump_path", "./virsh_dumps") | ||
| dump_options = params.get("dump_options") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The new pacing controls are effectively dead code.
mem_hotplug_delay is never read, virshdump_delay is passed through but never slept on, and virshdump_iterations never affects the loop count. So this test always does one dump immediately after each attach, regardless of the cfg knobs. That means the new variant is not exercising the timing-sensitive scenario the config advertises.
Also applies to: 36-48, 133-177, 252-259
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libvirt/tests/src/memory/memory_hotplug_virshdump.py` around lines 23 - 27,
The pacing-related params are defined but not actually used, so the hotplug/dump
flow ignores the configured timing and iteration controls. Update the test logic
around the hotplug loop and dump handling in memory_hotplug_virshdump.py so
mem_hotplug_delay is read and slept between iterations, virshdump_delay is
applied before each dump, and virshdump_iterations controls how many dump
operations are performed instead of always doing one immediate dump. Make sure
the relevant helpers/main flow that drives virsh.dump and the attach loop use
these symbols consistently so the config knobs really affect the scenario.
| if not _numa_present: | ||
| logging.info("No NUMA nodes found on VM '%s' – configuring " | ||
| "prerequisites for DIMM hotplug", vm_name) | ||
|
|
||
| _mem_kib = int(params.get("mem", "2097152")) | ||
| _max_mem_kib = int(params.get("max_mem", "20971520")) | ||
| _slots = int(params.get("slots", "40")) | ||
| _mem_unit = params.get("mem_unit", "KiB") | ||
| _smp = int(params.get("smp", "2")) | ||
| _numa_cells = int(params.get("numa_cells", "1")) | ||
|
|
||
| _vmxml.max_mem_rt = _max_mem_kib | ||
| _vmxml.max_mem_rt_slots = _slots | ||
| _vmxml.max_mem_rt_unit = _mem_unit | ||
| _vmxml.max_mem = _mem_kib | ||
| _vmxml.current_mem = _mem_kib | ||
|
|
||
| _cpuxml = _vmxml.cpu | ||
| if _cpuxml is None: | ||
| _cpuxml = VMCPUXML() | ||
| _cell_mem = _mem_kib // _numa_cells | ||
| _cpu_range = "0-%d" % (_smp - 1) if _smp > 1 else "0" | ||
| _cell_dicts = [ | ||
| {"id": str(i), "cpus": _cpu_range, | ||
| "memory": str(_cell_mem), "unit": _mem_unit} | ||
| for i in range(_numa_cells) | ||
| ] | ||
| _cpuxml.numa_cell = _cpuxml.dicts_to_cells(_cell_dicts) | ||
| _vmxml.cpu = _cpuxml | ||
|
|
||
| _vmxml.sync() | ||
| logging.info("NUMA configured for VM '%s': %d cell(s), " | ||
| "mem=%d %s, maxMem=%d %s, slots=%d", | ||
| vm_name, _numa_cells, _mem_kib, _mem_unit, | ||
| _max_mem_kib, _mem_unit, _slots) | ||
| else: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not skip max-memory slot setup just because NUMA already exists.
This branch treats “NUMA cell present” as “DIMM hotplug prerequisites satisfied”, but the cfg’s max_mem/max_mem_rt/slots values are only applied inside the not _numa_present path. A guest that already has NUMA but lacks hotplug capacity will still fail attach-device --live, and this code will silently skip the required setup. Split these checks so NUMA creation is conditional, but max-memory/slot reconciliation always runs.
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 205-205: String contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libvirt/tests/src/memory/memory_hotplug_virshdump.py` around lines 204 - 239,
The hotplug prerequisite setup in the branch around the NUMA check is skipping
max-memory and slot reconciliation whenever `_numa_present` is true. Update the
logic in the `memory_hotplug_virshdump` test so NUMA cell creation remains
conditional on `not _numa_present`, but the `_vmxml.max_mem_rt`,
`_vmxml.max_mem_rt_slots`, `_vmxml.max_mem_rt_unit`, `_vmxml.max_mem`, and
`_vmxml.current_mem` assignments always run when needed. Use the existing
`_vmxml.sync()` path and the surrounding `VMCPUXML`/`_cpuxml` handling to ensure
guests with pre-existing NUMA still get hotplug capacity configured.
|
log: |
1 participant
Summary by CodeRabbit
--configdisk attach/detach scenarios, plus disk source selection from/dev/disk/by-id.virsh dumpper hotplug iteration.