MTV-5556: Warm precopy on Pure FlashArray vVol falls back to slow vmkfstools clone instead of array offload#6477
Conversation
📝 WalkthroughWalkthroughTwo files update disk backing resolution for vSphere flat backing chains. The vmware client implements parent-chain traversal to locate the correct backing node by filename match, determining whether results are VVol- or VMDK-backed. The Pure storage layer uses this matched backing to look up volumes by their VVol ID, with both files adding matching helpers to encapsulate chain-walk logic. ChangesParent-chain traversal for VVol backing resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 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 |
…fstools clone instead of array offload When a precopy snapshot is active, the top-of-chain backing's filename is the child disk (foo-000001.vmdk) while forklift passes the base path (foo.vmdk). The old top-only path comparison missed this and the populator fell through to the VMDK/vmkfstools fallback. vmkfstools still produced correct bytes (it opens foo.vmdk on the host and VASA serves the snap-vVol), but at host-mediated speed. Walk the Parent chain and return the BackingObjectId of the node whose filename matches the caller's vmdkPath. Warm precopy now matches at the parent, identifies the snap-vVol, and copies it via Pure REST. Signed-off-by: Wanru Liu <waliu@purestorage.com>
waliu-ps
force-pushed
the
warm-migration-fix1
branch
from
352b99c to
221111a
Compare
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@cmd/vsphere-xcopy-volume-populator/internal/pure/flashArray.go`:
- Around line 274-280: The current findMatchingFlatBacking uses matchesVMDKPath
which only compares basenames and can match across datastores; change it to
compare normalized full VMDK paths (including datastore and folder) rather than
basenames. Either reuse the same normalization routine used by
vmware.GetVMDiskBacking (or add a shared normalizeVMDKPath helper) and call that
from matchesVMDKPath or replace matchesVMDKPath with a full-path equality check
inside findMatchingFlatBacking; ensure comparisons use the normalized form so
FindVolumeByVVolID cannot pick the wrong Pure volume.
In `@cmd/vsphere-xcopy-volume-populator/internal/vmware/client.go`:
- Around line 246-263: Add unit tests for findMatchingFlatBacking to cover
direct filename match, parent-chain match, and no-match scenarios so the
BackingObjectId selection can't regress; create table-driven tests that
construct types.VirtualDiskFlatVer2BackingInfo chains (using distinct FileName
values and Parent links) and assert findMatchingFlatBacking returns the expected
node for (1) a direct match on the leaf, (2) a match only on an ancestor in the
Parent chain, and (3) nil when neither normalizedPath nor vmdkPath match any
node (also include a case exercising diskPathMatches matching logic); place
tests alongside client.go and reference findMatchingFlatBacking and
diskPathMatches in assertions.
- Around line 204-210: The debug log incorrectly prints backing.BackingObjectId
while the branch returns matched.BackingObjectId; update the log inside the
branch that creates the DiskBacking to log the matched VVol ID
(matched.BackingObjectId) and include context (vmdkPath and matched.FileName) so
the message reflects the actual VVol being returned; locate the log call near
the DiskBacking return in the function handling VVol-backed disks (references:
matched, backing, DiskBacking, vmdkPath) and replace or augment the log
accordingly.
🪄 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: 6a8b09d8-2fb5-4e48-8d4c-024ff52b6f2f
📒 Files selected for processing (2)
cmd/vsphere-xcopy-volume-populator/internal/pure/flashArray.gocmd/vsphere-xcopy-volume-populator/internal/vmware/client.go
| func (f *FlashArrayClonner) findMatchingFlatBacking(b *types.VirtualDiskFlatVer2BackingInfo, vmDisk populator.VMDisk) *types.VirtualDiskFlatVer2BackingInfo { | ||
| for cur := b; cur != nil; cur = cur.Parent { | ||
| if f.matchesVMDKPath(cur.FileName, vmDisk) { | ||
| return cur | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Do not match the backing chain by basename alone.
matchesVMDKPath collapses [ds-a] vm1/disk.vmdk and [ds-b] vm2/disk.vmdk to the same key. With the new parent-chain walk, the first same-basename hit wins, so FindVolumeByVVolID can resolve the wrong Pure volume and copy the wrong source disk. Please align this matcher with the fuller path comparison used in vmware.GetVMDiskBacking, or share a single normalized path matcher between the two layers.
Suggested direction
func (f *FlashArrayClonner) matchesVMDKPath(fileName string, vmDisk populator.VMDisk) bool {
- fileBase := filepath.Base(fileName)
- targetBase := filepath.Base(vmDisk.VmdkFile)
- return fileBase == targetBase
+ normalize := func(p string) string {
+ p = strings.TrimSpace(strings.ToLower(p))
+ p = strings.ReplaceAll(p, "[", "")
+ p = strings.ReplaceAll(p, "]", "")
+ return p
+ }
+
+ fileName = normalize(fileName)
+ target := normalize(vmDisk.VmdkFile)
+ if fileName == target {
+ return true
+ }
+
+ // Only fall back to basename matching when the caller did not provide
+ // datastore/folder information.
+ if !strings.Contains(target, "/") {
+ return filepath.Base(fileName) == filepath.Base(target)
+ }
+ return 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 `@cmd/vsphere-xcopy-volume-populator/internal/pure/flashArray.go` around lines
274 - 280, The current findMatchingFlatBacking uses matchesVMDKPath which only
compares basenames and can match across datastores; change it to compare
normalized full VMDK paths (including datastore and folder) rather than
basenames. Either reuse the same normalization routine used by
vmware.GetVMDiskBacking (or add a shared normalizeVMDKPath helper) and call that
from matchesVMDKPath or replace matchesVMDKPath with a full-path equality check
inside findMatchingFlatBacking; ensure comparisons use the normalized form so
FindVolumeByVVolID cannot pick the wrong Pure volume.
| if matched.BackingObjectId != "" { | ||
| klog.V(2).Infof("Disk %s is VVol-backed (BackingObjectId: %s)", vmdkPath, backing.BackingObjectId) | ||
| return &DiskBacking{ | ||
| VVolId: backing.BackingObjectId, | ||
| VVolId: matched.BackingObjectId, | ||
| IsRDM: false, | ||
| DeviceName: backing.FileName, | ||
| DeviceName: matched.FileName, | ||
| }, nil |
There was a problem hiding this comment.
Log the matched VVol ID here.
Line 205 still logs backing.BackingObjectId, but this branch now returns matched.BackingObjectId. In the warm-precopy snapshot case, that makes the debug line point at the wrong vVol and will mislead offload troubleshooting.
Proposed fix
if matched.BackingObjectId != "" {
- klog.V(2).Infof("Disk %s is VVol-backed (BackingObjectId: %s)", vmdkPath, backing.BackingObjectId)
+ klog.V(2).Infof("Disk %s is VVol-backed (BackingObjectId: %s)", vmdkPath, matched.BackingObjectId)
return &DiskBacking{
VVolId: matched.BackingObjectId,
IsRDM: false,
DeviceName: matched.FileName,🤖 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 `@cmd/vsphere-xcopy-volume-populator/internal/vmware/client.go` around lines
204 - 210, The debug log incorrectly prints backing.BackingObjectId while the
branch returns matched.BackingObjectId; update the log inside the branch that
creates the DiskBacking to log the matched VVol ID (matched.BackingObjectId) and
include context (vmdkPath and matched.FileName) so the message reflects the
actual VVol being returned; locate the log call near the DiskBacking return in
the function handling VVol-backed disks (references: matched, backing,
DiskBacking, vmdkPath) and replace or augment the log accordingly.
| // findMatchingFlatBacking returns the first node in b's Parent chain whose | ||
| // filename matches vmdkPath, or nil if none does. | ||
| // | ||
| // While a snapshot is active the chain is "child disk (foo-000001.vmdk) -> | ||
| // parent (foo.vmdk)", and VASA reports a different BackingObjectId per level: | ||
| // child -> base vVol (current writes), parent -> snap-vVol (frozen). The | ||
| // caller's vmdkPath picks which level matches. | ||
| func findMatchingFlatBacking(b *types.VirtualDiskFlatVer2BackingInfo, normalizedPath, vmdkPath string) *types.VirtualDiskFlatVer2BackingInfo { | ||
| for cur := b; cur != nil; cur = cur.Parent { | ||
| fn := strings.ToLower(cur.FileName) | ||
| if strings.Contains(fn, normalizedPath) || | ||
| strings.Contains(normalizedPath, fn) || | ||
| diskPathMatches(cur.FileName, vmdkPath) { | ||
| return cur | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add regression tests for the parent-chain matcher.
This helper now decides which backing node supplies the BackingObjectId, so it needs coverage for direct match, parent-chain match, and no-match cases. Without that, this warm-precopy fix can regress back to host clone fallback or select the wrong chain node without notice.
As per coding guidelines: "coverage: Make sure that the code has unit tests."
🤖 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 `@cmd/vsphere-xcopy-volume-populator/internal/vmware/client.go` around lines
246 - 263, Add unit tests for findMatchingFlatBacking to cover direct filename
match, parent-chain match, and no-match scenarios so the BackingObjectId
selection can't regress; create table-driven tests that construct
types.VirtualDiskFlatVer2BackingInfo chains (using distinct FileName values and
Parent links) and assert findMatchingFlatBacking returns the expected node for
(1) a direct match on the leaf, (2) a match only on an ancestor in the Parent
chain, and (3) nil when neither normalizedPath nor vmdkPath match any node (also
include a case exercising diskPathMatches matching logic); place tests alongside
client.go and reference findMatchingFlatBacking and diskPathMatches in
assertions.
|
@mrnold could you ptal? |
|
I think this looks okay, assuming the Code Rabbit comments can be cleaned up. |
3 participants
When a precopy snapshot is active, the top-of-chain backing's filename is the child disk (foo-000001.vmdk) while forklift passes the base path (foo.vmdk). The old top-only path comparison missed this and the populator fell through to the VMDK/vmkfstools fallback. vmkfstools still produced correct bytes (it opens foo.vmdk on the host and VASA serves the snap-vVol), but at host-mediated speed.
Walk the Parent chain and return the BackingObjectId of the node whose filename matches the caller's vmdkPath. Warm precopy now matches at the parent, identifies the snap-vVol, and copies it via Pure REST.
Summary by CodeRabbit
Release Notes