MTV-3504 | Change to DiskTransfer step only on PreflightInspection#2964
MTV-3504 | Change to DiskTransfer step only on PreflightInspection#2964yaacov merged 1 commit intokubev2v:mainfrom
Conversation
solenoci
requested review from
Hazanel,
mnecas,
mrnold and
yaacov
as code owners
WalkthroughStep handling now treats PhaseCreateDataVolumes separately: Migrator.Step queries Plan.ShouldRunPreflightInspection (which encapsulates vSphere, warm, skip-guest-conversion, and RunPreflightInspection checks) and routes the phase to DiskTransfer only when that predicate is true; otherwise it routes to Initialize. Plan gained the ShouldRunPreflightInspection method. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Step as Migrator.Step
participant Plan as Plan.ShouldRunPreflightInspection
participant DT as DiskTransfer
participant I as Initialize
Step->>Plan: ShouldRunPreflightInspection()
alt returns true
Step->>DT: route PhaseCreateDataVolumes -> DiskTransfer
Note right of DT #DDEFEF: preflight path (vSphere + warm + guest-conversion + flag)
else returns false
Step->>I: route PhaseCreateDataVolumes -> Initialize
Note right of I #F7F0DD: non-preflight/default path
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Possibly related PRs
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 (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.go⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (1)📚 Learning: 2025-09-19T03:47:22.164ZApplied to files:
🧬 Code graph analysis (2)pkg/apis/forklift/v1beta1/plan.go (1)
pkg/controller/plan/migrator/base/migrator.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). (2)
🔇 Additional comments (5)
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 |
solenoci
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
pkg/controller/plan/migrator/base/migrator.go(1 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/migrator/base/migrator.go
🧠 Learnings (1)
📚 Learning: 2025-09-19T03:47:22.164Z
Learnt from: mrnold
PR: kubev2v/forklift#2807
File: pkg/controller/plan/adapter/vsphere/builder.go:0-0
Timestamp: 2025-09-19T03:47:22.164Z
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/migrator/base/migrator.go
🧬 Code graph analysis (1)
pkg/controller/plan/migrator/base/migrator.go (5)
pkg/apis/forklift/v1beta1/doc.go (3)
PhaseConvertOpenstackSnapshot(67-67)PhaseWaitForDataVolumesStatus(86-86)PhaseCreateDataVolumes(71-71)pkg/controller/plan/migrator/base/doc.go (2)
DiskTransfer(29-29)Initialize(26-26)pkg/controller/plan/context/migration.go (1)
Context(43-67)pkg/apis/forklift/v1beta1/plan.go (1)
Plan(271-279)pkg/apis/forklift/v1beta1/plan/vm.go (1)
Warm(170-176)
⏰ 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: lint
- GitHub Check: build
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2964 +/- ##
=========================================
- Coverage 15.45% 8.26% -7.19%
=========================================
Files 112 381 +269
Lines 23377 53199 +29822
=========================================
+ Hits 3613 4398 +785
- Misses 19479 48453 +28974
- 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:
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
pkg/apis/forklift/v1beta1/plan.go(1 hunks)pkg/controller/plan/migrator/base/migrator.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/apis/forklift/v1beta1/plan.gopkg/controller/plan/migrator/base/migrator.go
🧠 Learnings (1)
📚 Learning: 2025-09-19T03:47:22.164Z
Learnt from: mrnold
PR: kubev2v/forklift#2807
File: pkg/controller/plan/adapter/vsphere/builder.go:0-0
Timestamp: 2025-09-19T03:47:22.164Z
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/migrator/base/migrator.go
🧬 Code graph analysis (2)
pkg/apis/forklift/v1beta1/plan.go (1)
pkg/apis/forklift/v1beta1/plan/vm.go (1)
Warm(170-176)
pkg/controller/plan/migrator/base/migrator.go (4)
pkg/apis/forklift/v1beta1/doc.go (3)
PhaseConvertOpenstackSnapshot(67-67)PhaseWaitForDataVolumesStatus(86-86)PhaseCreateDataVolumes(71-71)pkg/controller/plan/migrator/base/doc.go (2)
DiskTransfer(29-29)Initialize(26-26)pkg/controller/plan/context/migration.go (1)
Context(43-67)pkg/apis/forklift/v1beta1/plan.go (1)
Plan(271-279)
⏰ 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 (2)
pkg/controller/plan/migrator/base/migrator.go (2)
260-266: CreateDataVolumes now maps to DiskTransfer only when preflight will run — good.Please confirm expected UI for these cases:
- Cold migration: CreateDataVolumes appears under Initialize.
- Warm vSphere with RunPreflightInspection=false: Initialize.
- Warm vSphere with RunPreflightInspection=true: DiskTransfer.
Add unit tests for Step() to lock this behavior.
409-410: RunInspection predicate dropped RequiresConversion — intended?Previously RunInspection required provider.RequiresConversion in addition to other gates. Now it solely defers to ShouldRunPreflightInspection, changing behavior. If this removal is intentional, please note it in the PR and tests. If not, reintroduce the check:
- case RunInspection: - allowed = r.context.Plan.ShouldRunPreflightInspection() + case RunInspection: + allowed = r.context.Plan.ShouldRunPreflightInspection() && + r.context.Source.Provider.RequiresConversion()Would you like me to push tests to assert predicate behavior across providers and SkipGuestConversion?
| func (r *Plan) ShouldRunPreflightInspection() bool { | ||
| return r.IsSourceProviderVSphere() && | ||
| r.Spec.Warm && | ||
| !r.Spec.SkipGuestConversion && | ||
| r.Spec.RunPreflightInspection | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Honor Type=warm (not only Spec.Warm) and add doc comment.
ShouldRunPreflightInspection ignores Spec.Type and will return false for plans using Type=warm (Warm is deprecated). Also, the exported method lacks a doc comment.
Apply:
- func (r *Plan) ShouldRunPreflightInspection() bool {
- return r.IsSourceProviderVSphere() &&
- r.Spec.Warm &&
- !r.Spec.SkipGuestConversion &&
- r.Spec.RunPreflightInspection
- }
+// ShouldRunPreflightInspection reports whether a preflight inspection should run.
+// It applies only to warm migrations from vSphere when guest conversion is not skipped.
+// Note: Type supersedes the deprecated Warm boolean.
+func (r *Plan) ShouldRunPreflightInspection() bool {
+ isWarm := r.Spec.Type == MigrationWarm || r.Spec.Warm
+ return r.IsSourceProviderVSphere() &&
+ isWarm &&
+ !r.Spec.SkipGuestConversion &&
+ r.Spec.RunPreflightInspection
+}Please add table-driven tests covering:
- vSphere + Type=warm + RunPreflightInspection=true + SkipGuestConversion=false => true
- vSphere + Warm=true (Type unset) => true
- vSphere + Type=cold => false
- vSphere + SkipGuestConversion=true => false
- Non‑vSphere => false
🏁 Script executed:
#!/bin/bash
set -eux
# Find PlanSpec definition and its fields
rg -n 'type PlanSpec' -n -C5 pkg/apis/forklift/v1beta1
# Inspect PlanSpec fields for Type, Warm, SkipGuestConversion, RunPreflightInspection
rg -n 'PlanSpec' -n -C10 pkg/apis/forklift/v1beta1
# Locate MigrationWarm constant
rg -n 'MigrationWarm' -n -C5 pkg/apis/forklift/v1beta1
# Confirm IsSourceProviderVSphere signature
rg -n 'func (r \*Plan) IsSourceProviderVSphere' -n -C3 pkg/apis/forklift/v1beta1/plan.goLength of output: 11213
---
🏁 Script executed:
```shell
#!/bin/bash
set -eux
# PlanSpec struct definition with key fields
rg -nP 'type PlanSpec struct' -n -C15 pkg/apis/forklift/v1beta1/plan.go
# Check Warm field
rg -nP 'Warm ' -n -C3 pkg/apis/forklift/v1beta1/plan.go
# Check SkipGuestConversion field
rg -nP 'SkipGuestConversion' -n -C3 pkg/apis/forklift/v1beta1/plan.go
# Check RunPreflightInspection field
rg -nP 'RunPreflightInspection' -n -C3 pkg/apis/forklift/v1beta1/plan.go
# Check Type field
rg -nP 'Type ' -n -C3 pkg/apis/forklift/v1beta1/plan.go
# Locate IsSourceProviderVSphere method
rg -nP 'func \\(r \\*Plan\\) IsSourceProviderVSphere' -n -C3 pkg/apis/forklift/v1beta1/plan.go
Length of output: 8812
Honor Spec.Type in ShouldRunPreflightInspection and add doc comment
- Update
ShouldRunPreflightInspectionto treatType==MigrationWarmor the deprecatedWarmflag as warm migrations, and prepend a Go doc comment:
- func (r *Plan) ShouldRunPreflightInspection() bool {
- return r.IsSourceProviderVSphere() &&
- r.Spec.Warm &&
- !r.Spec.SkipGuestConversion &&
- r.Spec.RunPreflightInspection
- }
+// ShouldRunPreflightInspection reports whether a preflight inspection should run.
// It applies only to warm migrations from vSphere when guest conversion is not skipped.
// Note: Type supersedes the deprecated Warm boolean.
+func (r *Plan) ShouldRunPreflightInspection() bool {
+ isWarm := r.Spec.Type == MigrationWarm || r.Spec.Warm
+ return r.IsSourceProviderVSphere() &&
+ isWarm &&
+ !r.Spec.SkipGuestConversion &&
+ r.Spec.RunPreflightInspection
+}- Add table-driven tests covering:
• vSphere + Type=warm + RunPreflightInspection=true + SkipGuestConversion=false ⇒ true
• vSphere + Warm=true (Type unset) ⇒ true
• vSphere + Type=cold ⇒ false
• vSphere + SkipGuestConversion=true ⇒ false
• Non-vSphere ⇒ false
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (r *Plan) ShouldRunPreflightInspection() bool { | |
| return r.IsSourceProviderVSphere() && | |
| r.Spec.Warm && | |
| !r.Spec.SkipGuestConversion && | |
| r.Spec.RunPreflightInspection | |
| } | |
| // ShouldRunPreflightInspection reports whether a preflight inspection should run. | |
| // It applies only to warm migrations from vSphere when guest conversion is not skipped. | |
| // Note: Type supersedes the deprecated Warm boolean. | |
| func (r *Plan) ShouldRunPreflightInspection() bool { | |
| isWarm := r.Spec.Type == MigrationWarm || r.Spec.Warm | |
| return r.IsSourceProviderVSphere() && | |
| isWarm && | |
| !r.Spec.SkipGuestConversion && | |
| r.Spec.RunPreflightInspection | |
| } |
🤖 Prompt for AI Agents
In pkg/apis/forklift/v1beta1/plan.go around lines 368 to 373, the
ShouldRunPreflightInspection method currently only checks the deprecated
Spec.Warm boolean and misses honoring Spec.Type; update the method so it treats
a migration as warm when Spec.Type == MigrationWarm OR Spec.Warm is true, and
keep the other checks (IsSourceProviderVSphere, !Spec.SkipGuestConversion,
Spec.RunPreflightInspection); add a concise Go doc comment above the method
explaining that it returns true for vSphere warm migrations (determined by
Spec.Type==MigrationWarm or legacy Spec.Warm) when preflight inspection is
requested and guest conversion is not skipped; then add table-driven unit tests
covering: vSphere + Type=warm + RunPreflightInspection=true +
SkipGuestConversion=false ⇒ true; vSphere + Warm=true (Type unset) ⇒ true;
vSphere + Type=cold ⇒ false; vSphere + SkipGuestConversion=true ⇒ false;
Non-vSphere ⇒ false.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Good point, thank you
Issue: When running Preflight inspection, we need the CreateDataVolume phase to be present in the DiskTransfer step. However it was present in the the DiskTransfer step alson when running cold migration. Fix: Only assign CreateDataVolume phase to DiskTransfer step, when running Preflight Inspection. Ref: https://issues.redhat.com/browse/MTV-3504 Resolves: MTV-3504 Signed-off-by: Stefan Olenocin <solenoci@redhat.com>
|
3 participants
Issue: When running Preflight inspection, we need the CreateDataVolume phase to be present in the DiskTransfer step. However it was present in the the DiskTransfer step alson when running cold migration.
Fix: Only assign CreateDataVolume phase to DiskTransfer step, when running Preflight Inspection.
Ref: https://issues.redhat.com/browse/MTV-3504
Resolves: MTV-3504
Summary by CodeRabbit