MTV-3120 | Initiate warm migration with storage offload#2807
MTV-3120 | Initiate warm migration with storage offload#2807mrnold merged 15 commits intokubev2v:mainfrom
Conversation
WalkthroughAdds 15 CDI/storage import and checkpoint annotation constants; updates vSphere builder to use shared annotations, reuse existing PVCs for warm storage-offload populators, enrich populator/DV annotations and deterministic naming; kubevirt DV creation skips GenerateName and sets adoption/pre-populated annotations for warm populators; migration reconciler waits for DV adoption and flips adoption annotation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Reconciler
participant Builder as vSphere Builder
participant Dest as Destination API
participant CDI as CDI Controller
rect rgba(200,220,255,0.12)
Note over Reconciler,Builder: Build phase for Warm + volume populators
Reconciler->>Builder: Build DataVolumes & Populators (warm-aware)
Builder->>Dest: List existing PVCs (labels: vmID + migration UID)
Builder->>Builder: Map PVCs by baseVolume(..., Warm)
Builder->>Dest: Create/Update DV/Populator with warm & import annotations
end
rect rgba(220,255,220,0.12)
Note over Reconciler,Dest: Adoption-wait before continuing
Reconciler->>Dest: Get matching PVCs
Dest-->>Reconciler: PVCs (ownerRefs?)
alt ownerRefs present
Reconciler->>Dest: Get DV for PVC.owner
Reconciler->>Dest: Update DV: set AnnAllowClaimAdoption="false"
Dest-->>Reconciler: DV updated
else owners missing
Reconciler-->>Reconciler: Requeue / wait until owners set
end
end
rect rgba(255,235,200,0.12)
Note over CDI,Dest: CDI adopts and populates
CDI->>Dest: Adopt PVC via DV and populate (VDDK/warm/backing & checkpoints)
Dest-->>CDI: Population completes
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.go⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (5)📚 Learning: 2025-09-19T03:47:22.138ZApplied to files:
📚 Learning: 2025-08-13T12:12:39.562ZApplied to files:
📚 Learning: 2025-09-17T07:33:45.540ZApplied to files:
📚 Learning: 2025-09-02T17:01:19.057ZApplied to files:
📚 Learning: 2025-08-13T03:31:49.934ZApplied to files:
🧬 Code graph analysis (1)pkg/controller/plan/adapter/vsphere/builder.go (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (8)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2807 +/- ##
=========================================
- Coverage 15.45% 8.28% -7.17%
=========================================
Files 112 381 +269
Lines 23377 52791 +29414
=========================================
+ Hits 3613 4375 +762
- Misses 19479 48068 +28589
- Partials 285 348 +63
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mrnold
requested review from
Hazanel,
mnecas,
solenoci and
yaacov
as code owners
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
pkg/controller/plan/migration.go (1)
744-776: Clarify the DV-creation gate logicThe condition reads negative and is easy to misread. Make the cold+populator skip explicit.
Apply:
- if !(r.builder.SupportsVolumePopulators(vm.Ref) && !r.Plan.Spec.Warm) { + // Create DVs unless we're doing cold migration with storage offload (populator). + if !(r.builder.SupportsVolumePopulators(vm.Ref) && !r.Plan.Spec.Warm) { ... }Or invert to an early-continue for readability.
pkg/controller/plan/adapter/vsphere/builder.go (5)
94-99: Remove duplicate AnnImportBackingFile constant; use base constantThis file redefines AnnImportBackingFile and also uses planbase.AnnImportBackingFile elsewhere. Keep a single source.
Apply:
-// Annotations -const ( - // CDI import backing file annotation on PVC - AnnImportBackingFile = "cdi.kubevirt.io/storage.import.backingFile" -) ... - return baseVolume(pvc.Annotations[AnnImportBackingFile], r.Plan.Spec.Warm) + return baseVolume(pvc.Annotations[planbase.AnnImportBackingFile], r.Plan.Spec.Warm)Also applies to: 1165-1168
1491-1494: Avoid short-var overshadowing for errYou shadow err here, which can mask earlier errors. Reuse the outer err.
Apply:
- err := r.ensurePopulatorServiceAccount(namespace) - if err != nil { + if err = r.ensurePopulatorServiceAccount(namespace); err != nil { return nil, err }
1891-1905: Possible nil-pointer when parsing URL; ensure safe hostname extractionIf liburl.Parse fails, h is nil and h.Hostname() panics. Also retain raw value on parse failure.
Apply:
- case "url": - h, err := liburl.Parse(string(value)) - if err != nil { - // ignore and try to use as is - dst.Data["GOVMOMI_HOSTNAME"] = value - } - dst.Data["GOVMOMI_HOSTNAME"] = []byte(h.Hostname()) + case "url": + if u, uErr := liburl.Parse(string(value)); uErr == nil { + dst.Data["GOVMOMI_HOSTNAME"] = []byte(u.Hostname()) + } else { + dst.Data["GOVMOMI_HOSTNAME"] = value + }
1403-1411: Default AccessModes to storage profile, not hard-coded RWManyHard-coding ReadWriteMany for block volumes may be unsupported on many classes. Prefer leaving empty (let StorageProfile default) unless user specified.
Apply:
- } else { - pvc.Spec.AccessModes = []core.PersistentVolumeAccessMode{core.ReadWriteMany} - } + }
1413-1453: Gate PVC-based CDI annotation usage behind a feature flag and add e2e tests
CDI docs/controllers honor cdi.kubevirt.io/storage.pod.phase, cdi.kubevirt.io/storage.prePopulated, cdi.kubevirt.io/storage.import.source and checkpoint.* annotations and accept them on PVCs for claim-adoption flows; gate this behavior behind a feature flag and add robust e2e coverage for warm/multi-stage imports and checkpoint transitions.
Location: pkg/controller/plan/adapter/vsphere/builder.go lines 1413–1453.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
pkg/controller/plan/adapter/base/doc.go(1 hunks)pkg/controller/plan/adapter/vsphere/builder.go(8 hunks)pkg/controller/plan/kubevirt.go(1 hunks)pkg/controller/plan/migration.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: Review the code as if you are a friendly but pedantic very senior developer in our team. We generally expect the following:
- Clarity and Simplicity: Write idiomatic Go. Code should be self-documenting. Complexity should be justified.
- Concurrency: Ensure goroutine safety. Use channels for communication and synchronization. Avoid race conditions.
- Error Handling: No silent failures. Errors must be checked, wrapped with context, and propagated. Use
errors.Isanderrors.As.- Resource Management: Prevent resource leaks. Defer
close()orcleanup()calls immediately after allocation. Manage goroutine lifecycles.- API Design: APIs (REST, gRPC) must be well-defined, versioned, and backward-compatible. Input validation is mandatory.
- Observability: Instrument code with structured logging, metrics (e.g., Prometheus), and tracing. Logs should provide context, not just messages.
- Security: Be vigilant against vulnerabilities (e.g., injection, insecure direct object references). Sanitize inputs. Do not hardcode secrets.
- Performance: Write efficient code, but avoid premature optimization. Be mindful of algorithmic complexity and memory allocations.
- coverage: Make sure that the code has unit tests.
Files:
pkg/controller/plan/adapter/base/doc.gopkg/controller/plan/kubevirt.gopkg/controller/plan/migration.gopkg/controller/plan/adapter/vsphere/builder.go
🧬 Code graph analysis (3)
pkg/controller/plan/kubevirt.go (1)
pkg/controller/plan/adapter/base/doc.go (3)
Builder(117-152)AnnAllowClaimAdoption(79-79)AnnPrePopulated(82-82)
pkg/controller/plan/migration.go (3)
pkg/controller/provider/web/ocp/datavolume.go (1)
DataVolume(93-96)pkg/controller/plan/context/migration.go (1)
Destination(158-165)pkg/controller/plan/adapter/base/doc.go (2)
Client(156-185)AnnAllowClaimAdoption(79-79)
pkg/controller/plan/adapter/vsphere/builder.go (6)
pkg/apis/forklift/v1beta1/plan/vm.go (1)
Warm(166-172)pkg/controller/plan/context/migration.go (3)
Context(43-67)Destination(158-165)Source(112-119)pkg/controller/plan/adapter/base/doc.go (14)
Client(156-185)AnnDiskSource(19-19)AnnEndpoint(49-49)AnnImportBackingFile(46-46)AnnSecret(52-52)AnnUUID(55-55)AnnThumbprint(58-58)AnnVddkInitImageURL(61-61)AnnPodPhase(64-64)AnnSource(85-85)AnnFinalCheckpoint(67-67)AnnCurrentCheckpoint(70-70)AnnPreviousCheckpoint(73-73)AnnCheckpointsCopied(76-76)pkg/apis/forklift/v1beta1/provider.go (2)
VDDK(65-65)Provider(116-121)pkg/settings/settings.go (2)
Settings(13-13)GetVDDKImage(128-135)pkg/settings/features.go (1)
Features(29-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Red Hat Konflux / forklift-controller-dev-preview-on-pull-request
- GitHub Check: Red Hat Konflux / forklift-api-dev-preview-on-pull-request
- GitHub Check: build
- GitHub Check: lint
🔇 Additional comments (4)
pkg/controller/plan/kubevirt.go (2)
1402-1406: Gate DV adoption/pre-population annotations behind warm+populator — LGTMSetting AnnAllowClaimAdoption="true" and AnnPrePopulated="true" only for warm+populator is correct. Ensure we toggle allowClaimAdoption back to "false" post-adoption (handled in migration.go with a follow-up fix below).
1412-1414: Avoid GenerateName for adoption path — LGTMSkipping GenerateName when warm+populator prevents new PVCs and enables claim adoption of the pre-created PVC.
Also applies to: 1416-1421
pkg/controller/plan/adapter/vsphere/builder.go (2)
527-556: PVC discovery for adoption — LGTM with a small improvementBuilding a backing-file→PVC map keyed by copy-offload is fine. Consider also validating that the PVC phase != Lost before using it.
1568-1571: Task name based on baseVolume — LGTMDeriving task names from the warm-adjusted disk key keeps progress accounting consistent across paths.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
pkg/controller/plan/adapter/vsphere/builder.go (4)
1396-1401: Don’t force RWX by defaultDefaulting to ReadWriteMany can break binding on storage that doesn’t support RWX. If not specified in the storage map, omit AccessModes and let the storage profile pick.
Apply:
- if mapped.Destination.AccessMode != "" { - pvc.Spec.AccessModes = []core.PersistentVolumeAccessMode{mapped.Destination.AccessMode} - } else { - pvc.Spec.AccessModes = []core.PersistentVolumeAccessMode{core.ReadWriteMany} - } + if mapped.Destination.AccessMode != "" { + pvc.Spec.AccessModes = []core.PersistentVolumeAccessMode{mapped.Destination.AccessMode} + }
1403-1409: Use constants; avoid magic string for offload markerReplace "copy-offload" literal with the new namespaced constant to prevent drift.
Apply:
- if annotations == nil { + if annotations == nil { pvc.Annotations = make(map[string]string) } else { pvc.Annotations = annotations } - pvc.Annotations[planbase.AnnDiskSource] = baseVolume(disk.File, r.Plan.Spec.Warm) - pvc.Annotations["copy-offload"] = baseVolume(disk.File, r.Plan.Spec.Warm) + pvc.Annotations[planbase.AnnDiskSource] = baseVolume(disk.File, r.Plan.Spec.Warm) + pvc.Annotations[planbase.AnnCopyOffload] = baseVolume(disk.File, r.Plan.Spec.Warm)
1884-1892: Fix potential nil deref when parsing URL for GOVMOMI_HOSTNAMEOn parse error, h may be unusable; don’t call h.Hostname().
Apply:
- case "url": - h, err := liburl.Parse(string(value)) - if err != nil { - // ignore and try to use as is - dst.Data["GOVMOMI_HOSTNAME"] = value - } - dst.Data["GOVMOMI_HOSTNAME"] = []byte(h.Hostname()) + case "url": + h, err := liburl.Parse(string(value)) + if err != nil { + // use the raw value as hostname fallback + dst.Data["GOVMOMI_HOSTNAME"] = value + } else { + dst.Data["GOVMOMI_HOSTNAME"] = []byte(h.Hostname()) + }
1470-1474: Typo in error messageMinor: “popoulators” → “populators”.
Apply:
- return nil, fmt.Errorf("failed to merge secrets for popoulators %w", err) + return nil, fmt.Errorf("failed to merge secrets for populators %w", err)
♻️ Duplicate comments (2)
pkg/controller/plan/migration.go (1)
778-819: Fix DV adoption wait: don’t rely on PVC OwnerRefs and guard nil annotationsOwnerRefs may be absent or ordered unpredictably; fetching DV by PVC name is more reliable during adoption. Also prevent a nil map panic on DataVolume.Annotations.
Apply:
- for _, pvc := range pvcs { - owners := pvc.GetOwnerReferences() - if len(owners) < 1 { - r.Log.Info("no owners listed on PVC yet", "pvc", pvc.Name) - return - } - for _, owner := range owners { - if owner.Kind != "DataVolume" { - continue - } - dataVolume := &cdi.DataVolume{} - err = r.Destination.Client.Get( - context.TODO(), - types.NamespacedName{Namespace: pvc.Namespace, Name: owner.Name}, - dataVolume) - if err != nil { - r.Log.Error(err, "error getting matching DataVolume for PVC", "pvc", pvc.Name) - return - } - - // Super hack alert: once the DataVolume has adopted the PVC, - // set the 'allowClaimAdoption' annotation to false. This gets - // CDI to allow the DataVolume to go to the Paused state, which - // allows forklift to reuse all the existing warm migration - // logic to continue after a storage offload initial copy. - dataVolume.Annotations[base.AnnAllowClaimAdoption] = "false" - err = r.Destination.Client.Update(context.TODO(), dataVolume) - if err != nil { - r.Log.Error(err, "error updating DataVolume, retrying", "dv", dataVolume.Name) - return - } - } - } + for _, pvc := range pvcs { + // DV and PVC share the same name during adoption; OwnerRefs may be missing/ordered arbitrarily. + dataVolume := &cdi.DataVolume{} + if err = r.Destination.Client.Get( + context.TODO(), + types.NamespacedName{Namespace: pvc.Namespace, Name: pvc.Name}, + dataVolume); err != nil { + r.Log.Error(err, "error getting matching DataVolume for PVC by name", "pvc", pvc.Name) + return + } + if dataVolume.Annotations == nil { + dataVolume.Annotations = map[string]string{} + } + // Once the DataVolume has adopted the PVC, toggle off to let CDI move to Paused. + dataVolume.Annotations[base.AnnAllowClaimAdoption] = "false" + if err = r.Destination.Client.Update(context.TODO(), dataVolume); err != nil { + r.Log.Error(err, "error updating DataVolume, retrying", "dv", dataVolume.Name) + return + } + }Run to spot remaining OwnerRef-based DV lookups and verify initial use of allowClaimAdoption=true on DV creation:
#!/bin/bash rg -n -C2 -P '\bOwnerReferences?\s*\(' --type=go pkg | sed -n '1,200p' rg -n -C2 -S 'allowClaimAdoption' --type=go pkgpkg/controller/plan/adapter/vsphere/builder.go (1)
625-633: Ensure DV adopts PVC regardless of DV source type; avoid VDDK-only keyingWhen useV2vForTransfer=true, dvSource.VDDK is nil, so the DV won’t adopt the pre-populated PVC. Key by disk file consistently.
Apply:
- if pvcMap != nil && dvSource.VDDK != nil { - // In a warm migration with storage offload, the PVC has already been created with - // the name template. Copy the result to the DataVolume so it can adopt the PVC. - if pvc, present := pvcMap[dvSource.VDDK.BackingFile]; present && pvc != nil { - dv.ObjectMeta.Name = pvc.Name - } - } else { + if pvcMap != nil { + // Match by backing file regardless of DV source type. + backingKey := baseVolume(disk.File, r.Plan.Spec.Warm) + if pvc, present := pvcMap[backingKey]; present && pvc != nil { + dv.ObjectMeta.Name = pvc.Name + } else { + // Fall back to template-based naming if no matching PVC + if err := r.setPVCNameFromTemplate(&dv.ObjectMeta, vm, diskIndex, disk); err != nil { + r.Log.Info("Failed to set PVC name from template", "error", err) + } + } + } else { // Set PVC name/generateName using template if configured if err := r.setPVCNameFromTemplate(&dv.ObjectMeta, vm, diskIndex, disk); err != nil { r.Log.Info("Failed to set PVC name from template", "error", err) } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
pkg/controller/plan/adapter/base/doc.go(1 hunks)pkg/controller/plan/adapter/vsphere/builder.go(10 hunks)pkg/controller/plan/migration.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: Review the code as if you are a friendly but pedantic very senior developer in our team. We generally expect the following:
- Clarity and Simplicity: Write idiomatic Go. Code should be self-documenting. Complexity should be justified.
- Concurrency: Ensure goroutine safety. Use channels for communication and synchronization. Avoid race conditions.
- Error Handling: No silent failures. Errors must be checked, wrapped with context, and propagated. Use
errors.Isanderrors.As.- Resource Management: Prevent resource leaks. Defer
close()orcleanup()calls immediately after allocation. Manage goroutine lifecycles.- API Design: APIs (REST, gRPC) must be well-defined, versioned, and backward-compatible. Input validation is mandatory.
- Observability: Instrument code with structured logging, metrics (e.g., Prometheus), and tracing. Logs should provide context, not just messages.
- Security: Be vigilant against vulnerabilities (e.g., injection, insecure direct object references). Sanitize inputs. Do not hardcode secrets.
- Performance: Write efficient code, but avoid premature optimization. Be mindful of algorithmic complexity and memory allocations.
- coverage: Make sure that the code has unit tests.
Files:
pkg/controller/plan/adapter/base/doc.gopkg/controller/plan/adapter/vsphere/builder.gopkg/controller/plan/migration.go
🧠 Learnings (1)
📚 Learning: 2025-08-13T12:12:39.562Z
Learnt from: yaacov
PR: kubev2v/forklift#2368
File: pkg/controller/provider/container/vsphere/model.go:1096-1103
Timestamp: 2025-08-13T12:12:39.562Z
Learning: In the vSphere model adapter for forklift, when processing VirtualDisk devices in updateDisks method, the controller returned by getDiskController(disk.ControllerKey) is guaranteed to not be nil according to yaacov, so nil checks are not needed in this context.
Applied to files:
pkg/controller/plan/adapter/vsphere/builder.go
🧬 Code graph analysis (2)
pkg/controller/plan/adapter/vsphere/builder.go (5)
pkg/apis/forklift/v1beta1/plan/vm.go (1)
Warm(166-172)pkg/controller/plan/context/migration.go (3)
Context(43-67)Destination(158-165)Source(112-119)pkg/controller/plan/adapter/base/doc.go (15)
Client(159-188)AnnImportBackingFile(46-46)AnnDiskSource(19-19)AnnEndpoint(49-49)AnnSecret(52-52)AnnUUID(55-55)AnnThumbprint(58-58)AnnVddkInitImageURL(61-61)AnnPodPhase(64-64)AnnSource(85-85)AnnFinalCheckpoint(67-67)AnnCurrentCheckpoint(70-70)AnnPreviousCheckpoint(73-73)AnnCheckpointsCopied(76-76)AnnImportPod(88-88)pkg/settings/settings.go (2)
Settings(13-13)GetVDDKImage(128-135)pkg/settings/features.go (1)
Features(29-45)
pkg/controller/plan/migration.go (2)
pkg/controller/plan/context/migration.go (1)
Destination(158-165)pkg/controller/plan/adapter/base/doc.go (2)
Client(159-188)AnnAllowClaimAdoption(79-79)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Red Hat Konflux / forklift-controller-dev-preview-on-pull-request
- GitHub Check: build
- GitHub Check: lint
🔇 Additional comments (2)
pkg/controller/plan/migration.go (1)
744-777: Gate DVs only when cold+offload — LGTMCondition reads correctly: create DVs for warm or when populators aren’t supported; skip only for cold+offload.
pkg/controller/plan/adapter/vsphere/builder.go (1)
1563-1566: Task name consistency — LGTMUsing AnnDiskSource keyed through baseVolume keeps populator task names stable across warm/cold.
Signed-off-by: Matthew Arnold <marnold@redhat.com>
Make sure DataVolume has the same name as the PVC, and add annotations to prevent CDI from doing any work while the PVC gets filled up. Signed-off-by: Matthew Arnold <marnold@redhat.com>
Signed-off-by: Matthew Arnold <marnold@redhat.com>
Annotate the populator PVC so that it looks like it has completed the first base disk copy of a warm migration. Add the usual VDDK login information so that future importer pods can copy snapshot deltas onto the populator PVC. The DataVolume will sit idle while the xcopy is running, CDI should not start an importer pod. Signed-off-by: Matthew Arnold <marnold@redhat.com>
After the DataVolume snapshot list has been updated, adjust the populator PVC annotations to match. Reset the pod phase to Unknown so that CDI starts an importer pod with the new snapshot pair. Signed-off-by: Matthew Arnold <marnold@redhat.com>
Signed-off-by: Matthew Arnold <marnold@redhat.com>
After the PVC has been adopted by the DataVolume, set the allowClaimAdoption annotation to false. This allows the DataVolume to go to Paused, which gets forklift to jump into the rest of the regular warm migration workflow after the initial storage offload copy. Signed-off-by: Matthew Arnold <marnold@redhat.com>
Now that the DataVolume successfully goes to Paused, it is no longer necessary to use this workaround to get CDI to create importer pods for new checkpoints. Signed-off-by: Matthew Arnold <marnold@redhat.com>
Signed-off-by: Matthew Arnold <marnold@redhat.com>
Signed-off-by: Matthew Arnold <marnold@redhat.com>
Clarify intentions of various annotations, remove duplicate backing file annotation, make cold offload condition more readable, add a defensive nil check, and check all PVC owners instead of just the first one. Signed-off-by: Matthew Arnold <marnold@redhat.com>
Signed-off-by: Matthew Arnold <marnold@redhat.com>
DataVolume ownership check can take a few iterations, and the populator PVC creation code did not correctly handle getting called more than once. Signed-off-by: Matthew Arnold <marnold@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/controller/plan/adapter/vsphere/builder.go (3)
611-619: Potential nil-map write on Labels
dv.ObjectMeta.Labelsis used without ensuring it’s initialized; will panic whendisk.Sharedis true and the template didn’t set labels.Apply:
dv := dvTemplate.DeepCopy() dv.Spec = dvSpec if dv.ObjectMeta.Annotations == nil { dv.ObjectMeta.Annotations = make(map[string]string) } + if dv.ObjectMeta.Labels == nil { + dv.ObjectMeta.Labels = make(map[string]string) + } dv.ObjectMeta.Annotations[planbase.AnnDiskSource] = baseVolume(disk.File, r.Plan.Spec.Warm) if disk.Shared { dv.ObjectMeta.Labels[Shareable] = "true" }
1276-1325: OK: Feature gate + per-VM datastore capability checkDropping the warm gate in favor of CopyOffload + per-disk datastore capability lookups makes sense.
Minor: reuse buildDatastoreMap() here to avoid duplicate datastore lookups.
1916-1923: Nil deref when parsing URL in mergeSecretsIf url.Parse fails,
his nil;h.Hostname()panics. Keep the fallback and only set hostname when parse succeeds.Apply:
- h, err := liburl.Parse(string(value)) - if err != nil { - // ignore and try to use as is - dst.Data["GOVMOMI_HOSTNAME"] = value - } - dst.Data["GOVMOMI_HOSTNAME"] = []byte(h.Hostname()) + if h, perr := liburl.Parse(string(value)); perr == nil && h != nil { + dst.Data["GOVMOMI_HOSTNAME"] = []byte(h.Hostname()) + } else { + // fallback: keep as provided (hostname or host:port or raw) + dst.Data["GOVMOMI_HOSTNAME"] = value + }
♻️ Duplicate comments (2)
pkg/controller/plan/adapter/vsphere/builder.go (2)
626-637: Adoption bug: DataVolume name lookup gated on dvSource.VDDKWarm+populator flows can still have
useV2vForTransfer=true, makingdvSource.VDDKnil. You’ll skip adoption even whenpvcMapis populated.Apply:
- if pvcMap != nil && dvSource.VDDK != nil { - // In a warm migration with storage offload, the PVC has already been created with - // the name template. Copy the result to the DataVolume so it can adopt the PVC. - if pvc, present := pvcMap[dvSource.VDDK.BackingFile]; present { - dv.ObjectMeta.Name = pvc.Name - } - } else { + if pvcMap != nil { + // Match by backing file regardless of DV source type. + backingKey := baseVolume(disk.File, r.Plan.Spec.Warm) + if pvc, present := pvcMap[backingKey]; present { + dv.ObjectMeta.Name = pvc.Name + } else { + // Set PVC name/generateName using template if configured + if err := r.setPVCNameFromTemplate(&dv.ObjectMeta, vm, diskIndex, disk); err != nil { + r.Log.Info("Failed to set PVC name from template", "error", err) + } + } + } else { // Set PVC name/generateName using template if configured if err := r.setPVCNameFromTemplate(&dv.ObjectMeta, vm, diskIndex, disk); err != nil { r.Log.Info("Failed to set PVC name from template", "error", err) } }
1160-1163: Identifier should fall back to AnnDiskSource when ImportBackingFile is missingMissing AnnImportBackingFile yields empty identifiers; use DiskSource as a fallback.
Apply:
-func (r *Builder) ResolvePersistentVolumeClaimIdentifier(pvc *core.PersistentVolumeClaim) string { - return baseVolume(pvc.Annotations[planbase.AnnImportBackingFile], r.Plan.Spec.Warm) -} +func (r *Builder) ResolvePersistentVolumeClaimIdentifier(pvc *core.PersistentVolumeClaim) string { + key := pvc.Annotations[planbase.AnnImportBackingFile] + if key == "" { + key = pvc.Annotations[planbase.AnnDiskSource] + } + return baseVolume(key, r.Plan.Spec.Warm) +}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
pkg/controller/plan/adapter/vsphere/builder.go(14 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: Review the code as if you are a friendly but pedantic very senior developer in our team. We generally expect the following:
- Clarity and Simplicity: Write idiomatic Go. Code should be self-documenting. Complexity should be justified.
- Concurrency: Ensure goroutine safety. Use channels for communication and synchronization. Avoid race conditions.
- Error Handling: No silent failures. Errors must be checked, wrapped with context, and propagated. Use
errors.Isanderrors.As.- Resource Management: Prevent resource leaks. Defer
close()orcleanup()calls immediately after allocation. Manage goroutine lifecycles.- API Design: APIs (REST, gRPC) must be well-defined, versioned, and backward-compatible. Input validation is mandatory.
- Observability: Instrument code with structured logging, metrics (e.g., Prometheus), and tracing. Logs should provide context, not just messages.
- Security: Be vigilant against vulnerabilities (e.g., injection, insecure direct object references). Sanitize inputs. Do not hardcode secrets.
- Performance: Write efficient code, but avoid premature optimization. Be mindful of algorithmic complexity and memory allocations.
- coverage: Make sure that the code has unit tests.
Files:
pkg/controller/plan/adapter/vsphere/builder.go
🧠 Learnings (4)
📓 Common learnings
Learnt from: mrnold
PR: kubev2v/forklift#2807
File: pkg/controller/plan/adapter/vsphere/builder.go:0-0
Timestamp: 2025-09-19T03:47:22.138Z
Learning: In forklift vSphere builder, the pvcMap population (for warm migration with volume populators) and dvSource type determination (based on useV2vForTransfer) are independent conditions. This means pvcMap can be non-nil while dvSource.VDDK is nil, leading to potential nil dereference when accessing dvSource.VDDK.BackingFile. The backing file key should be computed from disk.File instead.
📚 Learning: 2025-09-19T03:47:22.138Z
Learnt from: mrnold
PR: kubev2v/forklift#2807
File: pkg/controller/plan/adapter/vsphere/builder.go:0-0
Timestamp: 2025-09-19T03:47:22.138Z
Learning: In forklift vSphere builder, the pvcMap population (for warm migration with volume populators) and dvSource type determination (based on useV2vForTransfer) are independent conditions. This means pvcMap can be non-nil while dvSource.VDDK is nil, leading to potential nil dereference when accessing dvSource.VDDK.BackingFile. The backing file key should be computed from disk.File instead.
Applied to files:
pkg/controller/plan/adapter/vsphere/builder.go
📚 Learning: 2025-08-13T12:12:39.562Z
Learnt from: yaacov
PR: kubev2v/forklift#2368
File: pkg/controller/provider/container/vsphere/model.go:1096-1103
Timestamp: 2025-08-13T12:12:39.562Z
Learning: In the vSphere model adapter for forklift, when processing VirtualDisk devices in updateDisks method, the controller returned by getDiskController(disk.ControllerKey) is guaranteed to not be nil according to yaacov, so nil checks are not needed in this context.
Applied to files:
pkg/controller/plan/adapter/vsphere/builder.go
📚 Learning: 2025-09-17T07:33:45.540Z
Learnt from: yaacov
PR: kubev2v/forklift#2812
File: pkg/controller/plan/adapter/vsphere/builder.go:1645-1656
Timestamp: 2025-09-17T07:33:45.540Z
Learning: When TargetVmName contains dots (from utils.ChangeVmName returning DNS-1123 subdomain), PVC name templates like "{{.TargetVmName}}-disk-{{.DiskIndex}}" can fail label validation. The preferred solution is to document template functions for label conversion rather than adding new API fields like TargetVmNameLabel.
Applied to files:
pkg/controller/plan/adapter/vsphere/builder.go
🧬 Code graph analysis (1)
pkg/controller/plan/adapter/vsphere/builder.go (3)
pkg/controller/plan/context/migration.go (3)
Context(43-67)Destination(158-165)Source(112-119)pkg/controller/plan/adapter/base/doc.go (16)
Client(159-188)AnnImportBackingFile(46-46)AnnDiskSource(19-19)AnnEndpoint(49-49)AnnSecret(52-52)AnnUUID(55-55)AnnThumbprint(58-58)AnnVddkInitImageURL(61-61)AnnPodPhase(64-64)AnnSource(85-85)AnnFinalCheckpoint(67-67)AnnCurrentCheckpoint(70-70)AnnPreviousCheckpoint(73-73)AnnCheckpointsCopied(76-76)AnnImportPod(88-88)Builder(120-155)pkg/settings/features.go (1)
Features(29-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Red Hat Konflux / forklift-api-dev-preview-on-pull-request
- GitHub Check: Red Hat Konflux / forklift-controller-dev-preview-on-pull-request
- GitHub Check: build
- GitHub Check: lint
🔇 Additional comments (1)
pkg/controller/plan/adapter/vsphere/builder.go (1)
1596-1597: LGTM: stable task namingUsing AnnDiskSource with baseVolume is consistent with DV/PVC identifiers.
Signed-off-by: Matthew Arnold <marnold@redhat.com>
|
Side note: if copying deltas with CDI is the right thing to do long-term, then we should get this workflow officially supported in CDI so we aren't depending on internal behavior. But I think at some point we should look into the possibility of copying individual deltas with storage offload instead. |
These cause the plan to fail because there are multiple VSphereXcopyVolumePopulators on the same PVC. Signed-off-by: Matthew Arnold <marnold@redhat.com>
|
|
No one else was around, so I merged this myself to officially make it in by the deadline. |
2 participants
Allow warm migration with storage offload by populating the PV with xcopy, and then applying snapshot deltas via CDI. This works by having the usual warm migration DataVolume adopt the populator PVC, adding annotations so that CDI does not try to do the initial import itself.
Reference: MTV-3120
Summary by CodeRabbit
New Features
Improvements