Repository navigation
feat: PVC-based shared disk device detection - #552
Merged
myakove merged 10 commits intoJun 16, 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Walkthrough
ChangesDestination-aware shared-disk device resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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
Branch Management
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
Security Checks
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
MiriSafra
changed the title
![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: 2
🤖 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 105-137: The docstring for the _get_pvc_names_from_vm function
claims "cloud-init excluded" in the Returns section, but the code doesn't
explicitly filter out cloud-init volumes; it only returns volumes that have a
persistentVolumeClaim attribute, which naturally excludes cloud-init volumes
that use cloudInitNoCloud or cloudInitConfigDrive sources. Remove the
"cloud-init excluded" text from the docstring's Returns section since this
filtering is implicit behavior based on volume type, not an explicit filter in
the code. Alternatively, if explicit clarity is preferred, add a comment in the
loop explaining that only persistentVolumeClaim volumes are included, which
naturally excludes cloud-init volumes by type.
- Around line 183-198: The function currently uses shared_pvcs.pop() which
arbitrarily selects a PVC when multiple shared PVCs exist, causing
non-deterministic behavior. Add a validation check after the existing empty
check (after the ValueError for no shared PVCs) to explicitly fail if there are
multiple shared PVCs. Check that len(shared_pvcs) equals 1 and raise a
ValueError if it exceeds 1, providing details about all the conflicting shared
PVCs found. Only proceed with the pop() call and subsequent device path
resolution (_index_to_device_path calls) if this validation passes.
🪄 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: 721a9aef-73aa-45af-9956-266c0546f421
📒 Files selected for processing (2)
tests/shared_disk/test_shared_disk_rhel_migration.pyutilities/shared_disk.py
Member
Author
|
/verified |
myakove
requested changes
Jun 14, 2026
myakove
reviewed
Jun 14, 2026
myakove
left a comment
myakove
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Found 3 issue(s) in this PR:
💡 Suggestions (3)
| File | Line | Issue |
|---|---|---|
utilities/shared_disk.py |
131 | Potential device path mismatch: _get_pvc_names_from_vm collects only PVC-b |
utilities/shared_disk.py |
171 | Previous review comment still unresolved: consider returning a dict instead of |
utilities/shared_disk.py |
148 | No bounds check — if idx >= 26, chr(ord('a') + idx) produces non-letter char |
Review generated by pi
- Assign vmi once to avoid race between check and use - Use TimeoutSampler sample instead of re-fetching vmi.instance (TOCTOU) - Preserve exception chain with 'from exc' for CI diagnostics - Handle missing vol_status.target as transient state (clear and retry) instead of raising immediately — KubeVirt populates targets incrementally - Extract magic numbers into named constants - Add debug logging after PVC device mapping
- Initialize sample=None before TimeoutSampler loop so the except handler never hits NameError if the loop never yields. - Replace captured vmi reference with lambda that re-fetches cnv_vm.vmi each iteration, preventing stale reference if VMI is recreated during the polling window.
MiriSafra
force-pushed
the
feat/pvc-based-shared-disk-detection
branch
from
June 15, 2026 10:00
dabf89b to
36b6855
Compare
|
Clean rebase detected — no code changes compared to previous head ( |
The early `if not cnv_vm.vmi` check contradicts the polling pattern: if VMI is transiently absent (restart, migration settling), it crashes immediately instead of letting the TimeoutSampler wait patiently. The lambda already handles VMI absence (`cnv_vm.vmi.instance if cnv_vm.vmi else None`), and `if not sample: continue` skips None returns. Removing the guard makes the function resilient to transient VMI absence while still producing clear diagnostics on timeout (phase: no-sample).
Member
Author
|
/verified |
Collaborator
|
/lgtm |
myakove
approved these changes
Jun 16, 2026
|
New container for ghcr.io/redhatqe/mtv-api-tests:latest published |
This was referenced Jun 18, 2026
This was referenced Jun 25, 2026
This was referenced Jul 5, 2026
3 tasks done
This was referenced Jul 21, 2026
This was referenced Aug 12, 2026
2 tasks
This was referenced Oct 4, 2026
This was referenced Oct 14, 2026
2 of 3 tasks
2 of 3 tasks
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
destination-side PVC lookup
VMIs — no assumptions about controller layout or disk ordering
from source config
Summary by CodeRabbit
Tests
Improvements