feat(MTV-4803): per-disk xcopyUsed verification for mixed datastore migration

[prabinovRedhat]

Summary

• Adds test_check_xcopy_used to TestCopyoffloadMixedDatastoreMigration (MTV-565) using a new verify_xcopy_used_per_datastore() utility for migrations where disks on different vSphere datastores have different expected XCOPY behavior.
• Maps provider-configured MoRef IDs (datastore_id → expect xcopyUsed=1, non_xcopy_datastore_id → expect xcopyUsed=0) to populate pod log display names for per-pod verification.
• Improves populate pod log parsing: single log read per pod, clearer errors when copy-offload fails before xcopyUsed is logged, and INFO logs with expected vs actual values plus PASS/FAIL.

Test plan

• Run TestCopyoffloadMixedDatastoreMigration on vSphere with mixed XCOPY and non-XCOPY datastores configured in provider JSON
• Confirm both populate pods log expected= / actual= with (PASS) for XCOPY and fallback disks
• Confirm existing copy-offload tests using verify_xcopy_used() are unchanged

Jira: MTV-4803

Summary by CodeRabbit

• Tests

  • Enhanced XCOPY validation test to check per-datastore behavior, verifying XCOPY usage separately for each datastore and ensuring expected outcomes per datastore.

• Refactor

  • Improved copy-offload verification with richer log parsing, clearer PASS/FAIL reporting that includes datastore context, and more robust datastore identification and observation checks.

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 65e5817a-4dd5-4c23-a122-768d9f64adb1

📥 Commits

Reviewing files that changed from the base of the PR and between 7df610b and dbe942d.

📒 Files selected for processing (2)

• tests/copyoffload/test_copyoffload_migration.py
• utilities/copyoffload_migration.py

---

Walkthrough

This PR enhances XCOPY copy-offload verification with regex-based log parsing to extract datastore display names and xcopyUsed values, refactors verify_xcopy_used to use these helpers, adds verify_xcopy_used_per_datastore for per-datastore assertions, and updates the mixed-datastore test to validate XCOPY usage per datastore.

Changes

Per-Datastore XCOPY Verification

Layer / File(s)	Summary
Log parsing helpers and result logging utilities/copyoffload_migration.py	Regex constants and helpers extract source datastore display names and xcopyUsed values (including originating log lines) from populate pod logs; standardized result logger emits PASS/FAIL with optional datastore context.
Refactored verify_xcopy_used with new helpers utilities/copyoffload_migration.py	verify_xcopy_used now uses the new parsing helper (returns value + matched log line) and routes verification output through the standardized logger; assertion messages include the matched log line.
Per-datastore verification implementation utilities/copyoffload_migration.py	Added _resolve_datastore_id_from_display_name and verify_xcopy_used_per_datastore to map parsed datastore names to MoRef IDs, assert expected xcopyUsed per datastore, track observed datastore IDs, and optionally require all configured datastores be seen.
Mixed-datastore test update tests/copyoffload/test_copyoffload_migration.py	Test imports and uses verify_xcopy_used_per_datastore; derives XCOPY-capable and non-XCOPY datastore IDs from provider config, resolves datastore names, and validates XCOPY usage per datastore.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• RedHatQE/mtv-api-tests#490: Related copy-offload XCOPY verification/test updates that interact with the same verification helpers.
• RedHatQE/mtv-api-tests#486: Adds copy-offload tests relying on verify_xcopy_used which was modified here.

Suggested labels

lgtm-coderabbitai[bot], cherry-pick-v2.11, cherry-pick-v2.10

Suggested reviewers

• myakove
• krcmarik
• solenoci

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly and accurately describes the main change: adding per-datastore XCOPY verification capability for mixed-datastore migrations, which matches the core functionality added across both modified files.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[redhat-qe-bot]

Report bugs in Issues

Welcome! 🎉

This pull request will be automatically processed with the following features:

🔄 Automatic Actions

• Reviewer Assignment: Reviewers are automatically assigned based on the OWNERS file in the repository root
• Size Labeling: PR size labels (XS, S, M, L, XL, XXL) are automatically applied based on changes
• Issue Creation: Disabled for this repository
• Branch Labeling: Branch-specific labels are applied to track the target branch
• Auto-verification: Auto-verified users have their PRs automatically marked as verified
• Labels: All label categories are enabled (default configuration)

📋 Available Commands

PR Status Management

• /wip - Mark PR as work in progress (adds WIP: prefix to title)
• /wip cancel - Remove work in progress status
• /hold - Block PR merging (approvers only)
• /hold cancel - Unblock PR merging
• /verified - Mark PR as verified
• /verified cancel - Remove verification status
• /reprocess - Trigger complete PR workflow reprocessing (useful if webhook failed or configuration changed)
• /regenerate-welcome - Regenerate this welcome message

Review & Approval

• /lgtm - Approve changes (looks good to me)
• /approve - Approve PR (approvers only)
• /automerge - Enable automatic merging when all requirements are met (maintainers and approvers only)
• /assign-reviewers - Assign reviewers based on OWNERS file
• /assign-reviewer @username - Assign specific reviewer
• /check-can-merge - Check if PR meets merge requirements

Testing & Validation

• /retest tox - Run Python test suite with tox
• /retest build-container - Rebuild and test container image
• /retest conventional-title - Validate commit message format
• /retest all - Run all available tests

Container Operations

• /build-and-push-container - Build and push container image (tagged with PR number)
  • Supports additional build arguments: /build-and-push-container --build-arg KEY=value

Cherry-pick Operations

• /cherry-pick <branch> - Schedule cherry-pick to target branch when PR is merged
  • Multiple branches: /cherry-pick branch1 branch2 branch3

Label Management

• /<label-name> - Add a label to the PR
• /<label-name> cancel - Remove a label from the PR

✅ Merge Requirements

This PR will be automatically approved when the following conditions are met:

1. Approval: /approve from at least one approver
2. Status Checks: All required status checks must pass
3. No Blockers: No wip, hold, has-conflicts labels and PR must be mergeable (no conflicts)
4. Verified: PR must be marked as verified

📊 Review Process

Approvers and Reviewers

Approvers:

• myakove
• solenoci

Reviewers:

• krcmarik
• myakove
• solenoci

Available Labels

• hold
• verified
• wip
• lgtm
• approve
• automerge

AI Features

• Conventional Title: Mode: fix (claude/claude-opus-4-6[1m])
• Cherry-Pick Conflict Resolution: Enabled (claude/claude-opus-4-6[1m])
• Test Oracle: Triggers: approved (cursor/gpt-5.4-xhigh-fast); /test-oracle can be used anytime

💡 Tips

• WIP Status: Use /wip when your PR is not ready for review
• Verification: The verified label is removed on new commits unless the push is detected as a clean rebase
• Cherry-picking: Cherry-pick labels are processed when the PR is merged
• Container Builds: Container images are automatically tagged with the PR number
• Permission Levels: Some commands require approver permissions
• Auto-verified Users: Certain users have automatic verification and merge privileges

For more information, please refer to the project documentation or contact the maintainers.

[coderabbitai]

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 `@tests/copyoffload/test_copyoffload_migration.py`:
- Around line 2471-2474: The dict construction using
source_provider.get_datastore_name_by_id for xcopy_datastore_id and
non_xcopy_datastore_id must validate results and fail early if either call
returns None: call source_provider.get_datastore_name_by_id for each id, check
"if name is None:" and raise a clear error (or assert) naming the missing
datastore id and variable (e.g., xcopy_datastore_id / non_xcopy_datastore_id),
then populate datastore_names_by_id only with the validated non-None names;
reference the datastore_names_by_id mapping,
source_provider.get_datastore_name_by_id, xcopy_datastore_id and
non_xcopy_datastore_id to locate where to add these checks.

🪄 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: f23933fe-b672-4d2d-b4ae-0f7ad475e2c9

📥 Commits

Reviewing files that changed from the base of the PR and between 5ed39c2 and 7df610b.

📒 Files selected for processing (2)

• tests/copyoffload/test_copyoffload_migration.py
• utilities/copyoffload_migration.py

[prabinovRedhat]

/verified

[prabinovRedhat]

cc: @tshefi

[myakove]

/lgtm
/approve

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0)

Context used

✅ Tickets: MTV-4803

1. Missing datastore coverage details 🐞 Bug ◔ Observability

Description

When require_all_datastores_seen fails, the thrown ValueError only prints the verified datastore
IDs and omits which IDs were expected and which are missing. This makes failures harder to diagnose
from CI logs.

Code

utilities/copyoffload_migration.py[R555-558]

Evidence

The current exception message only logs the verified set even though the expected set is in scope
(used in the condition) and the missing set is directly computable at that point.

utilities/copyoffload_migration.py[555-559]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The `ValueError` raised when not all configured datastores are seen only reports `verified_datastore_ids`. It should also report the expected set and the missing IDs to speed up debugging.

### Issue Context
At the failure point you have both `verified_datastore_ids` and `expected_xcopy_by_datastore_id.keys()` available, so you can compute and print the missing IDs.

### Fix Focus Areas
- utilities/copyoffload_migration.py[555-559]

### Suggested change
Compute and include missing IDs:
```python
expected_ids = set(expected_xcopy_by_datastore_id.keys())
missing_ids = expected_ids - verified_datastore_ids
raise ValueError(
   "Migration must include at least one disk from each configured datastore; "
   f"expected datastore IDs: {sorted(expected_ids)}; "
   f"verified datastore IDs: {sorted(verified_datastore_ids)}; "
   f"missing datastore IDs: {sorted(missing_ids)}"
)
```

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Misleading coverage parameter docs 🐞 Bug ⚙ Maintainability

Description

The verify_xcopy_used_per_datastore() docstring claims require_all_datastores_seen should be set
False when multiple disks share a datastore, but the implementation only checks whether every
configured datastore ID appears at least once. This guidance can lead callers to disable a
meaningful coverage check for the wrong reason.

Code

utilities/copyoffload_migration.py[R489-491]

Evidence

The docstring’s rationale (“multiple disks may share a datastore”) conflicts with the actual check,
which uses a set of seen datastore IDs and compares it to the configured expected IDs—disk sharing
does not change this condition.

utilities/copyoffload_migration.py[489-491]
utilities/copyoffload_migration.py[518-556]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`verify_xcopy_used_per_datastore()` documents `require_all_datastores_seen` with guidance that doesn’t match the actual semantics (it’s about whether *all configured datastore IDs* must be observed at least once, not whether multiple disks share the same datastore).

### Issue Context
The function tracks `verified_datastore_ids` as a set, so multiple disks on the same datastore are irrelevant to the condition; only presence/absence of each configured datastore ID matters.

### Fix Focus Areas
- utilities/copyoffload_migration.py[489-492]

### Suggested change
Rewrite the parameter description to something like:
- “Set False if the migration may legitimately not include disks from every configured datastore ID (i.e., you only want to validate per-pod expectations for whichever datastores appear).”

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Previous review results

Review updated until commit dbe942d

Results up to commit N/A

Looking for bugs?

Check back in a few minutes. An AI review agent is analyzing this pull request.

[redhat-qe-bot2]

Clean rebase detected — no code changes compared to previous head (7df610b).
The following labels were preserved: verified, approved-myakove, lgtm-myakove, commented-prabinovRedhat, commented-coderabbitai[bot], changes-requested-coderabbitai[bot].

[prabinovRedhat]

/verified

[qodo-code-review]

Code review by qodo was updated up to the latest commit dbe942d

[prabinovRedhat]

/cherry-pick v2.11 v2.10

[redhat-qe-bot1]

Cherry-picked PR feat(MTV-4803): per-disk xcopyUsed verification for mixed datastore migration into v2.11: #533

[redhat-qe-bot2]

New container for ghcr.io/redhatqe/mtv-api-tests:latest published

[redhat-qe-bot1]

Cherry-picked PR feat(MTV-4803): per-disk xcopyUsed verification for mixed datastore migration into v2.10: #534