Repository navigation
feat: detect shared disk device path dynamically - #538
Merged
krcmarik merged 4 commits intoJun 5, 2026
Merged
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughReplaces a hardcoded ChangesShared Disk Device Dynamic Detection
Sequence Diagram(s)sequenceDiagram
participant verify_shared_disk_data
participant _get_shared_disk_device
participant prepared_plan_source_vms_data
verify_shared_disk_data->>_get_shared_disk_device: request device for vm_name
_get_shared_disk_device->>prepared_plan_source_vms_data: read disks for vm_name
_get_shared_disk_device-->>verify_shared_disk_data: return detected /dev/vdX or raise ValueError
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
Member
Author
|
/verified |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@utilities/shared_disk.py`:
- Around line 117-119: The code assumes disks is non-empty and does
sorted_disks[0] which will raise an opaque IndexError for malformed VM data; add
a guard right after retrieving disks (the variable disks from
prepared_plan["source_vms_data"][vm_name]["disks"]) to check if not disks and
raise a clear ValueError that includes the vm_name and a short explanation
(e.g., "no disks found for VM <vm_name> in source_vms_data"). This makes the
failure explicit before computing sorted_disks and setting boot_controller.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6559942a-57fb-4abf-9bd0-ce782189620f
📒 Files selected for processing (2)
tests/tests_config/config.pyutilities/shared_disk.py
💤 Files with no reviewable changes (1)
- tests/tests_config/config.py
MiriSafra
force-pushed
the
feat/dynamic-shared-disk-device
branch
from
June 3, 2026 13:05
bef714a to
8b19867
Compare
|
Clean rebase detected — no code changes compared to previous head ( |
Member
Author
|
/verified |
MiriSafra
marked this pull request as draft
June 3, 2026 15:40
MiriSafra
changed the title
MiriSafra
marked this pull request as ready for review
June 3, 2026 16:00
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Member
Author
|
/verified |
krcmarik
reviewed
Jun 4, 2026
krcmarik
left a comment
krcmarik
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Found 1 issue(s) in this PR:
💡 Suggestions (1)
| File | Line | Issue |
|---|---|---|
utilities/shared_disk.py |
120 | The detection is probably good enough for the current test setup but relies on t |
Review generated by Claude Code
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
utilities/shared_disk.py (1)
164-213:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Detect the shared device separately for each VM.
For the
feat: detect shared disk device path dynamicallychange, Line 164 still keeps the old plan-wide assumption alive:_get_shared_disk_device()is VM-scoped, butverify_shared_disk_data()derives/dev/vdXfromvm1_nameonce and reuses that path for VM2 as well. If the two source VMs do not have identical disk layouts, VM2 will mount the wrong block device or fail before the data check.Proposed fix
- shared_disk_device = _get_shared_disk_device(prepared_plan, vm1_name) + vm1_shared_disk_device = _get_shared_disk_device(prepared_plan, vm1_name) + vm2_shared_disk_device = _get_shared_disk_device(prepared_plan, vm2_name) @@ - partition = f"{shared_disk_device}1" + vm1_partition = f"{vm1_shared_disk_device}1" + vm2_partition = f"{vm2_shared_disk_device}1" test_file_vm1 = f"{mount_point}/test-vm1.txt" test_file_vm2 = f"{mount_point}/test-vm2.txt" @@ - LOGGER.info(f"VM1 ({vm1_name}): Mounting shared disk {partition}") + LOGGER.info(f"VM1 ({vm1_name}): Mounting shared disk {vm1_partition}") with ssh_vm1: - _mount_shared_partition(ssh_vm1, partition, mount_point, "VM1") + _mount_shared_partition(ssh_vm1, vm1_partition, mount_point, "VM1") @@ - LOGGER.info(f"VM2 ({vm2_name}): Mounting shared disk {partition}") + LOGGER.info(f"VM2 ({vm2_name}): Mounting shared disk {vm2_partition}") with ssh_vm2: - _mount_shared_partition(ssh_vm2, partition, mount_point, "VM2") + _mount_shared_partition(ssh_vm2, vm2_partition, mount_point, "VM2") @@ - _run_cmd_on_vm(ssh_vm1, ["sudo", "blockdev", "--flushbufs", shared_disk_device], "VM1 flush buffers") - _run_cmd_on_vm(ssh_vm1, ["sudo", "mount", partition, mount_point], "VM1 remount") + _run_cmd_on_vm( + ssh_vm1, + ["sudo", "blockdev", "--flushbufs", vm1_shared_disk_device], + "VM1 flush buffers", + ) + _run_cmd_on_vm(ssh_vm1, ["sudo", "mount", vm1_partition, mount_point], "VM1 remount")🤖 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 `@utilities/shared_disk.py` around lines 164 - 213, The code assumes a single shared_disk_device computed once for vm1 and reused for vm2; instead call _get_shared_disk_device separately for each VM (e.g., compute shared_disk_device_vm1 = _get_shared_disk_device(prepared_plan, vm1_name) and shared_disk_device_vm2 = _get_shared_disk_device(prepared_plan, vm2_name)), derive partition_vm1 and partition_vm2 (f"{...}1"), and use partition_vm1 for all vm1 mounts/flushes and partition_vm2 for all vm2 mounts/reads/writes; update every place referencing shared_disk_device/partition (mount/unmount, _run_cmd_on_vm blockdev/flush, and mount commands) to use the per-VM variables so each VM uses its own detected device.
🤖 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.
Outside diff comments:
In `@utilities/shared_disk.py`:
- Around line 164-213: The code assumes a single shared_disk_device computed
once for vm1 and reused for vm2; instead call _get_shared_disk_device separately
for each VM (e.g., compute shared_disk_device_vm1 =
_get_shared_disk_device(prepared_plan, vm1_name) and shared_disk_device_vm2 =
_get_shared_disk_device(prepared_plan, vm2_name)), derive partition_vm1 and
partition_vm2 (f"{...}1"), and use partition_vm1 for all vm1 mounts/flushes and
partition_vm2 for all vm2 mounts/reads/writes; update every place referencing
shared_disk_device/partition (mount/unmount, _run_cmd_on_vm blockdev/flush, and
mount commands) to use the per-VM variables so each VM uses its own detected
device.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 499dbd0d-523d-46ad-a08a-ed8c0284e622
📒 Files selected for processing (2)
tests/tests_config/config.pyutilities/shared_disk.py
💤 Files with no reviewable changes (1)
- tests/tests_config/config.py
Member
Author
|
/verified |
Replace hardcoded shared_disk_device "/dev/vdc" with runtime detection based on SCSI controller layout. The shared disk is identified by being on a different SCSI controller than the boot disk (Multi-writer requires separate controller with bus sharing enabled).
MiriSafra
force-pushed
the
feat/dynamic-shared-disk-device
branch
from
June 4, 2026 10:11
dda1fd6 to
be35fef
Compare
|
Clean rebase detected — no code changes compared to previous head ( |
Collaborator
|
/lgtm |
|
New container for ghcr.io/redhatqe/mtv-api-tests:latest published |
This was referenced Jun 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
shared_disk_device: "/dev/vdc"with dynamic detection based on SCSI controller layout_get_shared_disk_device()helper that identifies the shared disk by finding the first disk on a different SCSI controller (Multi-writer requires separate controller with bus sharing)shared_disk_devicefrom test config — no longer neededChanged Files
utilities/shared_disk.py_get_shared_disk_device(), use inverify_shared_disk_data()tests/tests_config/config.pyshared_disk_devicekey