feat: add Windows shared disk migration test#562
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? |
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds Windows shared-disk migration test coverage, Windows-specific labeling and verification helpers, shared-disk verification refactoring, and related config, error, and guidance updates. ChangesWindows Shared Disk Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 |
|
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 462-465: The PowerShell command in the `_win_run_powershell` call
uses single-quoted strings for both `path` and `content` parameters, which will
break if either value contains a single quote character. Replace the
single-quoted strings with properly escaped double-quoted strings or use
PowerShell variable substitution to safely handle special characters.
Specifically, modify the f-string passed to `_win_run_powershell` in the
Set-Content command to escape any double-quotes or single-quotes in the
`content` and `path` variables, or consider using PowerShell's `-replace`
operator to safely construct the command string.
- Line 416: The raise statement with GuestCommandError("{vm_label}: SHARED
volume not found") at line 416 is unreachable dead code. The TimeoutSampler loop
either returns successfully when a valid sample is found or raises
TimeoutExpiredError which is caught and re-raised, meaning the loop never
completes naturally. Remove this unreachable raise statement entirely to clean
up the code.
🪄 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: 15b842be-b76a-402d-ad8d-4a3ee4d60b1a
📒 Files selected for processing (3)
tests/shared_disk/test_shared_disk_windows_migration.pytests/tests_config/config.pyutilities/shared_disk.py
|
/verified |
myakove
left a comment
myakove
left a comment
There was a problem hiding this comment.
Code Review
Found 7 issue(s) in this PR:
💡 Suggestions (7)
| File | Line | Issue |
|---|---|---|
utilities/shared_disk.py |
472 | [CRITICAL] AGENTS.md not updated for Windows shared-disk variant. |
utilities/shared_disk.py |
498 | [WARNING] Significant preamble duplication with verify_shared_disk_data() |
utilities/shared_disk.py |
467 | [WARNING] PowerShell command injection risk via single-quote breakout. |
utilities/shared_disk.py |
451 | [WARNING] No write flush/sync after Set-Content on shared disk. |
utilities/shared_disk.py |
507 | [WARNING] _get_shared_disk_devices() return value silently discarded. |
utilities/shared_disk.py |
401 | [WARNING] drive_letter = "" is dead initialization. |
tests/shared_disk/test_shared_disk_windows_migration.py |
36 | [SUGGESTION] shared_disk and vsphere markers are used here but not liste |
Review generated by pi
…c access, SRP extraction - Remove AssertionError from retry except tuple (regression from cycle 2) - Use direct prepared_plan["_shared_disk_label"] access per Deterministic Tests rule - Extract _get_guest_auth() from _execute_guest_label_command (SRP, 50-line guideline) - Move _disable_fast_startup call to caller label_shared_disk_on_source_windows
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
Code review by qodo was updated up to the latest commit 8fd8abc |
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`:
- Line 482: The return annotation and the related type annotation in the
shared_disk helpers still quote NamePasswordAuthentication even though deferred
annotations are already enabled, which triggers the lint warning. Update the
affected annotations in the functions around the shared_disk guest auth code to
use the unquoted vim.vm.guest.NamePasswordAuthentication type directly,
including the return type on the function near the current line and the matching
annotation later in the same flow.
- Around line 497-498: The shared disk label path is currently gated by
wait_for_vmware_guest_info(), which waits on NIC discovery instead of VMware
Guest Operations readiness. Update this check in the shared disk flow to use
vm.guest.guestOperationsReady, preferably by adding a small helper near
wait_for_vmware_guest_info() and calling it before labeling so the code only
waits for Guest Ops to be available.
🪄 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: 19127eff-162d-4588-93ff-78ec7b4a5734
📒 Files selected for processing (1)
utilities/shared_disk.py
from __future__ import annotations makes quoted type annotations redundant.
|
Code review by qodo was updated up to the latest commit b45f54b |
|
/verified |
myakove
left a comment
myakove
left a comment
There was a problem hiding this comment.
Code Review
Found 2 issue(s) in this PR:
💡 Suggestions (2)
| File | Line | Issue |
|---|---|---|
utilities/shared_disk.py |
489 | _get_guest_auth uses direct access on schema-optional keys |
utilities/shared_disk.py |
853 | [SUGGESTION] Redundant drive2 assignment. |
Review generated by pi
- _get_guest_auth: use .get() + ValueError for guest_vm_win_user/password instead of bare dict[] access on schema-optional keys - _win_do_bidirectional_verification: drop unused drive2 capture from _win_ensure_shared_volume_online (immediately overwritten after refresh)
|
Code review by qodo was updated up to the latest commit 82559bf |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
utilities/shared_disk.py (1)
803-807: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHIGH: Restore the write-through flush after
Set-Content.
Set-Contentcloses the file, but it does not guarantee VM1’s cached NTFS data is on the shared disk before VM2 refreshes and reads it. VM2’s_win_refresh_shared_disk()cannot flush VM1’s write cache, so this can still produce intermittent stale reads.Proposed fix
_win_run_powershell( ssh_conn, f"Set-Content -Path '{ps_path}' -Value '{ps_content}'", f"{vm_label} write {filename}", ) + _win_run_powershell( + ssh_conn, + ( + f"$s = [System.IO.File]::Open('{ps_path}', 'Open', 'ReadWrite', 'ReadWrite'); " + "try { $s.Flush($true) } finally { $s.Dispose() }" + ), + f"{vm_label} flush {filename}", + )This is the same stale-read risk previously flagged for
_win_write_marker, and the current code appears to have regressed by dropping the flush.🤖 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 803 - 807, Restore the missing write-through flush in the shared-disk write path: after `_win_run_powershell`/`Set-Content` in the `shared_disk.py` write flow, explicitly force VM1’s cached NTFS data to be committed before any VM2 refresh/read can happen. Update the same write logic used by the marker-writing path (`_win_write_marker`) so the write operation does not rely on `Set-Content` alone, and ensure the flush happens in the `ssh_conn`-backed PowerShell sequence before returning.
🤖 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.
Duplicate comments:
In `@utilities/shared_disk.py`:
- Around line 803-807: Restore the missing write-through flush in the
shared-disk write path: after `_win_run_powershell`/`Set-Content` in the
`shared_disk.py` write flow, explicitly force VM1’s cached NTFS data to be
committed before any VM2 refresh/read can happen. Update the same write logic
used by the marker-writing path (`_win_write_marker`) so the write operation
does not rely on `Set-Content` alone, and ensure the flush happens in the
`ssh_conn`-backed PowerShell sequence before returning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 653a1115-d18f-4a48-b0ee-3670e3724907
📒 Files selected for processing (1)
utilities/shared_disk.py
|
Code review by qodo was updated up to the latest commit f6e5d10 |
|
/verified |
myakove
left a comment
myakove
left a comment
There was a problem hiding this comment.
Code Review
Found 4 issue(s) in this PR:
💡 Suggestions (4)
| File | Line | Issue |
|---|---|---|
AGENTS.md |
789 | Test method naming section (line 810+) missing shared-disk patterns |
AGENTS.md |
943 | README marker table still missing shared_disk and vsphere |
utilities/shared_disk.py |
738 | [SUGGESTION] Redundant length check — _try_get_drive_letter() already retu |
utilities/shared_disk.py |
869 | [SUGGESTION] volume_label not re-validated at verification entry point. |
Review generated by pi
- Add shared_disk and vsphere markers to README test markers table - Add volume_label re-validation at verify_shared_disk_data_windows entry - Add shared-disk test naming patterns to AGENTS.md test method naming - Remove redundant len(sample)==1 check in _win_get_shared_drive_letter
|
Code review by qodo was updated up to the latest commit 09905d9 |
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)
497-511: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winLOW: Validate Guest Ops config before the Tools wait.
guest_vm_win_user,guest_vm_win_password, andsource_provider.hostare deterministic inputs. Checking them afterwait_for_vmware_guest_info()turns a local misconfiguration into an unnecessary 300s wait and VM power-on cycle before reporting the real problem.Suggested fix
- if not source_provider.wait_for_vmware_guest_info(owner_vm, timeout=_GUEST_TOOLS_READY_TIMEOUT): - raise ValueError(f"VMware Tools not available on '{owner_name}' after power-on, cannot label shared disk") - win_user = source_provider_data.get("guest_vm_win_user") win_pass = source_provider_data.get("guest_vm_win_password") if not win_user or not win_pass: raise ValueError( f"Windows Guest Operations requires 'guest_vm_win_user' and " f"'guest_vm_win_password' in provider config for '{owner_name}'" ) - auth = vim.vm.guest.NamePasswordAuthentication(username=win_user, password=win_pass, interactiveSession=False) - vcenter_host = source_provider.host if vcenter_host is None: raise ValueError(f"vCenter host not available for provider used by VM '{owner_name}'") + + if not source_provider.wait_for_vmware_guest_info(owner_vm, timeout=_GUEST_TOOLS_READY_TIMEOUT): + raise ValueError(f"VMware Tools not available on '{owner_name}' after power-on, cannot label shared disk") + + auth = vim.vm.guest.NamePasswordAuthentication(username=win_user, password=win_pass, interactiveSession=False)As per coding guidelines, "Fail fast when
Noneis not valid" and "Validate required inputs, configuration values, and fixture dependencies at the source in fixtures."🤖 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 497 - 511, Move the Guest Ops configuration validation in the shared disk flow to fail fast before calling wait_for_vmware_guest_info(), since guest_vm_win_user, guest_vm_win_password, and source_provider.host are required inputs and should be checked immediately. Update the logic around the existing source_provider_data lookups and source_provider.host access in the shared disk setup path so misconfiguration raises the same ValueError before any VMware Tools wait or VM power-on delay. Keep the existing checks and error messages, just reorder them ahead of the wait_for_vmware_guest_info() call.Source: Coding guidelines
🤖 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 497-511: Move the Guest Ops configuration validation in the shared
disk flow to fail fast before calling wait_for_vmware_guest_info(), since
guest_vm_win_user, guest_vm_win_password, and source_provider.host are required
inputs and should be checked immediately. Update the logic around the existing
source_provider_data lookups and source_provider.host access in the shared disk
setup path so misconfiguration raises the same ValueError before any VMware
Tools wait or VM power-on delay. Keep the existing checks and error messages,
just reorder them ahead of the wait_for_vmware_guest_info() call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c0f0f316-3414-48f3-9b66-2e88b145a6fb
📒 Files selected for processing (3)
AGENTS.mdREADME.mdutilities/shared_disk.py
AGENTS.md: fix contradictory wording — "base five then check_vms" implied check_vms runs twice. Now says "base five through test_migrate_vms" to match the actual 7-step order. shared_disk.py: move credential and vcenter_host validation before the 300s VMware Tools wait in _prepare_guest_ops_auth. Config errors are deterministic — no reason to wait 5 minutes before reporting them. Matches the fail-fast pattern used in post_migration.py.
|
Code review by qodo was updated up to the latest commit 5517276 |
|
/verified |
myakove
left a comment
myakove
left a comment
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 |
474 | **_disable_fast_startup catches exceptions and logs warning — violates "No Fal |
utilities/shared_disk.py |
927 | Retry catches OSError — too broad, can mask programming bugs |
utilities/shared_disk.py |
750 | Unreachable guard — if not drive_letter: can never execute |
Review generated by pi
| except (GuestCommandError, vim.fault.VimFault, TimeoutExpiredError, ConnectionError, OSError) as e: | ||
| # Best-effort: Fast Startup disable is non-critical — migration can succeed | ||
| # without it, but may produce ConversionHasWarnings. | ||
| LOGGER.warning(f"Failed to disable Fast Startup on '{owner_name}' (best-effort): {e}") |
There was a problem hiding this comment.
_disable_fast_startup catches exceptions and logs warning — violates "No Fallbacks, No Silent Recovery" (MUST)
Per AGENTS.md: "LOGGER.warning(...) used to paper over a failure instead of raising" and "except followed by a fallback code path" are both flagged patterns.
The inline comment explains this is "best-effort: non-critical" — which is a reasonable operational justification. However, per the MUST rule, if this intentional deviation is accepted, it should be explicitly documented as an exception to the no-fallbacks rule (not just an inline comment in the except block).
Suggestion: Add to the function's docstring:
Note:
Intentional deviation from no-fallbacks rule — Fast Startup disable is
a non-critical optimization that does not affect migration correctness.
Failure is logged and execution continues.
| SSHConnectionSetupError, | ||
| OSError, | ||
| ConnectionError, | ||
| GuestCommandError, |
There was a problem hiding this comment.
Retry catches OSError — too broad, can mask programming bugs
OSError includes FileNotFoundError, PermissionError, IsADirectoryError, etc. — exceptions that indicate programming bugs, not transient SSH/network issues. These would be silently retried for up to 300s before timing out with a misleading error.
The SSH-specific exceptions (SSHException, AuthenticationException, NoValidConnectionsError, ChannelException, SSHConnectionSetupError) already cover transient connection issues. ConnectionError covers socket-level failures.
| GuestCommandError, | |
| SSHConnectionSetupError, | |
| ConnectionError, | |
| GuestCommandError, |
If there's a specific OSError subclass from paramiko/SSH that needs catching, use that instead of the broad OSError.
| ) from exc | ||
|
|
||
| if not drive_letter: | ||
| raise GuestCommandError(f"{vm_label}: Shared volume '{volume_label}' not found") |
There was a problem hiding this comment.
Unreachable guard — if not drive_letter: can never execute
Same pattern as the existing CodeRabbit finding at line 459. TimeoutSampler either:
- Yields a truthy
sample→drive_letter = sample; breakat line 740-741 - Times out → raises
TimeoutExpiredError→ caught at line 742 and re-raised
The for loop never completes without break or exception, so line 750 is dead code.
| raise GuestCommandError(f"{vm_label}: Shared volume '{volume_label}' not found") | |
| LOGGER.info(f"{vm_label}: Shared volume '{volume_label}' is drive {drive_letter}:") |
6 participants
Summary
Changed Files
tests/shared_disk/test_shared_disk_windows_migration.pyutilities/shared_disk.pylabel_shared_disk_on_source_windows(),verify_shared_disk_data_windows(), and Windows-specific helpers (_win_run_powershell,_win_ensure_shared_volume_online,_win_get_shared_drive_letter,_win_write_marker,_win_read_marker,_win_refresh_shared_disk)tests/tests_config/config.pytest_shared_disk_windows_migrationconfig section with VM specs andmigrateSharedDisksoverridesutilities/vmware_guest_operations.pylibs/providers/vmware.pyfind_shared_vmdk_paths()public method wrapping internal VMDK detectionAGENTS.mdmigrate_shared_disksto VM config tableSummary by CodeRabbit
migrate_shared_disksflag.shared_disktest marker.