fix: Remove accidentally added controller files#3203
Conversation
amitosw15
requested review from
Hazanel,
mnecas,
mrnold,
solenoci and
yaacov
as code owners
WalkthroughThe Builder interface method SupportsVolumePopulators was changed from taking a VM reference to being parameterless across adapter implementations and all migration/kubevirt call sites; adapter implementations adjusted internal heuristics (notably vsphere and ovirt) and migration flows now use a builder-wide capability check. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Migration
participant Builder
participant VM as "VM (per-VM)"
rect #dfefff
Note over Migration,VM: Old flow — per-VM capability query
Migration->>VM: iterate vmRef
VM->>Migration: vmRef
Migration->>Builder: SupportsVolumePopulators(vmRef)
Builder-->>Migration: bool (per-VM)
end
rect #eaffdf
Note over Migration,Builder: New flow — builder-wide capability query
Migration->>Builder: SupportsVolumePopulators()
Builder-->>Migration: bool (global)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 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:
🔇 Additional comments (2)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
amitosw15
force-pushed
the
powerstore-fixes
branch
from
2378233 to
4b22854
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/controller/plan/migration.go (1)
790-833: Critical: Compilation error - incorrect method signature.Line 790 calls
r.builder.SupportsVolumePopulators(vm.Ref), but the method signature was updated to take no parameters. This inconsistency will cause a compilation failure.Apply this diff to fix the method call:
- if r.builder.SupportsVolumePopulators(vm.Ref) && r.Plan.IsWarm() { + if r.builder.SupportsVolumePopulators() && r.Plan.IsWarm() {Note: The redundant
r.Plan.IsWarm()check could be removed since the newSupportsVolumePopulators()implementation already returnsfalsefor warm migrations (see line 1276 inpkg/controller/plan/adapter/vsphere/builder.go), but keeping it doesn't break functionality.pkg/controller/plan/adapter/vsphere/builder.go (1)
524-524: Critical: Compilation error - incorrect method call.Line 524 calls
r.SupportsVolumePopulators(vmRef), but the method signature was updated to take no parameters (line 1275). This will cause a compilation failure.Apply this diff:
- if r.Plan.IsWarm() && r.SupportsVolumePopulators(vmRef) { + if r.Plan.IsWarm() && r.SupportsVolumePopulators() {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
pkg/controller/plan/adapter/base/doc.go(1 hunks)pkg/controller/plan/adapter/ocp/builder.go(1 hunks)pkg/controller/plan/adapter/openstack/builder.go(1 hunks)pkg/controller/plan/adapter/ova/builder.go(1 hunks)pkg/controller/plan/adapter/ovirt/builder.go(1 hunks)pkg/controller/plan/adapter/vsphere/builder.go(1 hunks)pkg/controller/plan/migration.go(4 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/ovirt/builder.gopkg/controller/plan/adapter/ocp/builder.gopkg/controller/plan/adapter/ova/builder.gopkg/controller/plan/adapter/openstack/builder.gopkg/controller/plan/adapter/base/doc.gopkg/controller/plan/migration.gopkg/controller/plan/adapter/vsphere/builder.go
🧬 Code graph analysis (5)
pkg/controller/plan/adapter/ovirt/builder.go (3)
pkg/controller/plan/adapter/base/doc.go (1)
Builder(120-155)pkg/apis/forklift/v1beta1/plan.go (1)
Plan(271-279)pkg/controller/plan/context/migration.go (1)
Destination(158-165)
pkg/controller/plan/adapter/ocp/builder.go (1)
pkg/controller/plan/adapter/base/doc.go (1)
Builder(120-155)
pkg/controller/plan/adapter/ova/builder.go (2)
pkg/controller/plan/adapter/base/doc.go (1)
Builder(120-155)pkg/controller/plan/adapter/doc.go (1)
Builder(15-15)
pkg/controller/plan/adapter/openstack/builder.go (1)
pkg/controller/plan/adapter/base/doc.go (1)
Builder(120-155)
pkg/controller/plan/adapter/vsphere/builder.go (3)
pkg/controller/plan/adapter/base/doc.go (1)
Builder(120-155)pkg/settings/features.go (1)
Features(31-51)pkg/apis/forklift/v1beta1/mapping.go (2)
OffloadPlugin(55-57)VSphereXcopyPluginConfig(87-94)
⏰ 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 (4)
pkg/controller/plan/migration.go (4)
391-396: LGTM - Consistent API update.The removal of the
vmRefparameter fromSupportsVolumePopulators()is correctly applied here, making the check builder-wide rather than VM-specific.
727-749: LGTM - API updated consistently.The parameterless
SupportsVolumePopulators()call is correctly applied in the volume populator creation path.
756-787: LGTM - Fallback logic preserved.The negated check correctly triggers the non-populator DataVolume path when volume populators are not supported.
886-916: LGTM - Progress tracking updated correctly.The parameterless call correctly determines which progress update path to use (populator vs. DataVolume).
| // FIXME rgolan - the behaviour needs to be per disk hense this method is flawed. Needs a bigger change. | ||
| // For now this method returns true, if there's a mapping (backend by copy-offload-mapping ConfigMap, that | ||
| // maps StoragetClasses to Vsphere data stores | ||
| func (r *Builder) SupportsVolumePopulators() bool { | ||
| if !settings.Settings.Features.CopyOffload || r.Plan.Spec.Warm { | ||
| return false | ||
| } | ||
|
|
||
| // Get the VM to access its disks | ||
| vm := &model.VM{} | ||
| err := r.Source.Inventory.Find(vm, vmRef) | ||
| if err != nil { | ||
| klog.Errorf("failed to get VM to detect volume populators support: %s", err) | ||
| return false | ||
| } | ||
|
|
||
| if !r.Context.Plan.Spec.MigrateSharedDisks { | ||
| vm.RemoveSharedDisks() | ||
| } | ||
|
|
||
| // Build datastore map for lookups | ||
| dsMap := make(map[string]*api.StoragePair) | ||
| dsMapIn := r.Context.Map.Storage.Spec.Map | ||
| for i := range dsMapIn { | ||
| mapped := &dsMapIn[i] | ||
| ref := mapped.Source | ||
| for _, m := range dsMapIn { | ||
| ref := m.Source | ||
| ds := &model.Datastore{} | ||
| err := r.Source.Inventory.Find(ds, ref) | ||
| if err != nil { | ||
| klog.Errorf("failed to get datastore to detect volume populators support: %s", err) | ||
| return false | ||
| } | ||
| dsMap[ds.ID] = mapped | ||
| } | ||
|
|
||
| // Check each disk's datastore for offload capability | ||
| for _, disk := range vm.Disks { | ||
| mapped, found := dsMap[disk.Datastore.ID] | ||
| if !found { | ||
| // Skip unmapped datastores (will be caught by validation elsewhere) | ||
| continue | ||
| } | ||
| if m.OffloadPlugin != nil && m.OffloadPlugin.VSphereXcopyPluginConfig != nil { | ||
| klog.V(2).Infof("found offload plugin: config %+v on ds map %+v", m.OffloadPlugin.VSphereXcopyPluginConfig, dsMapIn) | ||
| return true | ||
|
|
||
| // If any disk's datastore doesn't have offload capability, we cannot use populators for this VM | ||
| if mapped.OffloadPlugin == nil || mapped.OffloadPlugin.VSphereXcopyPluginConfig == nil { | ||
| klog.V(2).Infof("VM %s has disk on datastore %s without offload capability, cannot use volume populators", vmRef.String(), disk.Datastore.ID) | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| // All disks are on datastores with offload capability | ||
| klog.V(2).Infof("VM %s: all disks are on datastores with offload capability, can use volume populators", vmRef.String()) | ||
| return true | ||
| return false | ||
| } |
There was a problem hiding this comment.
Address the architectural limitation and improve error handling.
The FIXME comment on line 1272 correctly identifies that this implementation is flawed - volume populator support should be determined per-disk based on the specific datastore, not as a builder-wide capability. This could lead to incorrect behavior when a VM has disks on different datastores with varying offload plugin support.
Additionally, the error handling on line 1285 silently logs and returns false, which could mask real inventory lookup failures and mislead the caller into thinking the feature is simply not supported rather than that an error occurred.
Consider these improvements:
- Per-disk API: Refactor to accept disk/datastore information and return support status per-disk:
func (r *Builder) DiskSupportsVolumePopulator(datastoreID string) (bool, error)- Explicit error handling: Return errors instead of logging and returning false:
err := r.Source.Inventory.Find(ds, ref)
if err != nil {
- klog.Errorf("failed to get datastore to detect volume populators support: %s", err)
- return false
+ return false, liberr.Wrap(err, "datastore", ref.String())
}- Cache the result: Since this iterates all datastores and queries inventory, consider caching the result during
Load()to avoid repeated expensive lookups.
Based on coding guidelines: Per Go best practices, errors should be checked, wrapped with context, and propagated rather than silently logged.
🤖 Prompt for AI Agents
In pkg/controller/plan/adapter/vsphere/builder.go around lines 1272-1296, the
current Builder.SupportsVolumePopulators implementation is incorrect because it
reports a builder-wide capability instead of per-disk/datastore support and it
swallows inventory lookup errors; change the design to a per-datastore check
such as DiskSupportsVolumePopulator(datastoreID string) (bool, error) (or
similar), make the method return an explicit error instead of logging+returning
false, wrap inventory lookup errors with context and propagate them to the
caller, and move/compute datastore support into a cache populated during Load()
to avoid repeated expensive Find calls; update callers to use the new per-disk
API and handle propagated errors accordingly.
| if !settings.Settings.Features.CopyOffload || r.Plan.Spec.Warm { | ||
| return false | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Note: Warm migration check makes call-site conditions redundant.
The check on line 1276 returns false for warm migrations, which makes the && r.Plan.IsWarm() condition at line 524 redundant (it will never be true when SupportsVolumePopulators() is true). While not incorrect, this could be clarified.
Consider one of these approaches:
-
Document the early-exit: Add a comment explaining that this method returns false for warm plans, so callers don't need additional warm checks.
-
Remove the warm check here: If the caller-side logic needs flexibility, remove the warm check from this method and let callers decide:
func (r *Builder) SupportsVolumePopulators() bool {
- if !settings.Settings.Features.CopyOffload || r.Plan.Spec.Warm {
+ if !settings.Settings.Features.CopyOffload {
return false
}Then update call sites to explicitly check warm status when needed.
🤖 Prompt for AI Agents
In pkg/controller/plan/adapter/vsphere/builder.go around lines 1276-1278, the
method currently returns false when the plan is Warm, which makes caller-side
checks like "&& r.Plan.IsWarm()" redundant; either add a clear comment on this
early-exit explaining that the method intentionally returns false for warm plans
so callers do not need additional warm checks, or remove the warm check from
this method and then update all call sites to perform an explicit IsWarm() check
where required (pick one approach consistently and adjust callers accordingly).
5c9f5a3 to
eaade99
Compare
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3203 +/- ##
=========================================
- Coverage 15.45% 8.36% -7.10%
=========================================
Files 112 391 +279
Lines 23377 53729 +30352
=========================================
+ Hits 3613 4494 +881
- Misses 19479 48884 +29405
- Partials 285 351 +66
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:
|
rgolangh
left a comment
rgolangh
left a comment
There was a problem hiding this comment.
2 comments. ping me when needed
This commit removes the controller files that were accidentally included in the initial PowerStore support merge (PR kubev2v#2595). The core feature logic remains unchanged. Resolves: MTV-3114 Related: kubev2v#2595 Signed-off-by: Amit Weinstock <aweinsto@redhat.com>
amitosw15
force-pushed
the
powerstore-fixes
branch
from
eaade99 to
5e75b0e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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/adapter/vsphere/builder.go (3)
524-550: Warm-offload block is effectively dead with current builder semanticsr.Plan.IsWarm() && r.SupportsVolumePopulators() will never be true for providers where SupportsVolumePopulators() returns false during warm (e.g., vSphere), so pvcMap is never built. Drop the redundant IsWarm() check here or adjust the builder to not short-circuit on warm. Also, adoption should match by disk.File, not dvSource, to work when Source=Blank. Based on learnings
Apply this change inside DataVolumes (adoption works regardless of source type):
- 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 { + // In a warm migration with storage offload, the PVC has already been created with + // the name template. Match using the backing file computed from the disk file. + if pvc, present := pvcMap[baseVolume(disk.File, r.Plan.IsWarm())]; 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) } }
1891-1899: Nil dereference on URL parse failure in mergeSecretsIf liburl.Parse fails, h is nil, then h.Hostname() panics. Wrap with else.
- 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()) + h, err := liburl.Parse(string(value)) + if err != nil { + // use as-is when not a URL + dst.Data["GOVMOMI_HOSTNAME"] = value + } else { + dst.Data["GOVMOMI_HOSTNAME"] = []byte(h.Hostname()) + }
1339-1355: Potential nil-pointer dereference on OffloadPluginmapped.OffloadPlugin or mapped.OffloadPlugin.VSphereXcopyPluginConfig can be nil; dereferencing them will panic.
- vsphereInstance := r.Context.Plan.Provider.Source.GetName() - storageVendorProduct := mapped.OffloadPlugin.VSphereXcopyPluginConfig.StorageVendorProduct - storageVendorSecretRef := mapped.OffloadPlugin.VSphereXcopyPluginConfig.SecretRef + vsphereInstance := r.Context.Plan.Provider.Source.GetName() + if mapped.OffloadPlugin == nil || mapped.OffloadPlugin.VSphereXcopyPluginConfig == nil { + r.Log.Info("offload plugin not configured for datastore mapping; skipping PVC/populator creation", "datastoreID", ds.ID, "storageClass", storageClass) + continue + } + storageVendorProduct := mapped.OffloadPlugin.VSphereXcopyPluginConfig.StorageVendorProduct + storageVendorSecretRef := mapped.OffloadPlugin.VSphereXcopyPluginConfig.SecretRefpkg/controller/plan/kubevirt.go (2)
1430-1434: Remove redundant IsWarm() here; let builder decideWith builders already encoding warm constraints, this conjunction blocks valid cases and makes the branch unreachable for some providers.
- if r.Plan.IsWarm() && r.Builder.SupportsVolumePopulators() { + if r.Builder.SupportsVolumePopulators() { // For storage offload, tie DataVolume to pre-imported PVC annotations[planbase.AnnAllowClaimAdoption] = "true" annotations[planbase.AnnPrePopulated] = "true" }
1444-1449: Consistent condition for DV template namingMirror the condition above; avoid coupling to Plan.IsWarm() here as well.
- if !(r.Builder.SupportsVolumePopulators() && r.Plan.IsWarm()) { + if !r.Builder.SupportsVolumePopulators() { // For storage offload warm migrations, the template should have already // been applied to the PVC that will be adopted by this DataVolume, so // only add generateName for other migration types. dvTemplate.ObjectMeta.GenerateName = r.getGeneratedName(vm) }pkg/controller/plan/migration.go (1)
790-833: Warm gating inconsistency: use vm.Warm instead of r.Plan.IsWarm()Elsewhere you gate warm-specific logic on vm.Warm != nil. Here it uses r.Plan.IsWarm(), which can diverge per-VM. Align to vm.Warm for correctness.
Apply this diff:
- if r.builder.SupportsVolumePopulators() && r.Plan.IsWarm() { + if r.builder.SupportsVolumePopulators() && vm.Warm != nil {
♻️ Duplicate comments (1)
pkg/controller/plan/adapter/vsphere/builder.go (1)
1272-1296: Builder-wide capability is flawed and hides errors; make it per-disk and propagate errorsThis method should report support per datastore/disk and return errors instead of logging and returning false. Cache support at Load() time.
Minimal improvement if interface cannot change now: at least don’t swallow errors.
- err := r.Source.Inventory.Find(ds, ref) - if err != nil { - klog.Errorf("failed to get datastore to detect volume populators support: %s", err) - return false - } + err := r.Source.Inventory.Find(ds, ref) + if err != nil { + r.Log.Error(err, "failed to get datastore to detect volume populators support", "datastore", ref.String()) + return false + }Longer-term: introduce DiskSupportsVolumePopulator(datastoreID string) (bool, error) and cache results during Load(). Based on coding guidelines
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
pkg/controller/plan/adapter/base/doc.go(1 hunks)pkg/controller/plan/adapter/ocp/builder.go(1 hunks)pkg/controller/plan/adapter/openstack/builder.go(1 hunks)pkg/controller/plan/adapter/ova/builder.go(1 hunks)pkg/controller/plan/adapter/ovirt/builder.go(1 hunks)pkg/controller/plan/adapter/vsphere/builder.go(2 hunks)pkg/controller/plan/kubevirt.go(2 hunks)pkg/controller/plan/migration.go(5 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/ovirt/builder.gopkg/controller/plan/adapter/vsphere/builder.gopkg/controller/plan/migration.gopkg/controller/plan/kubevirt.gopkg/controller/plan/adapter/ocp/builder.gopkg/controller/plan/adapter/openstack/builder.gopkg/controller/plan/adapter/ova/builder.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/adapter/vsphere/builder.go
🧬 Code graph analysis (6)
pkg/controller/plan/adapter/ovirt/builder.go (1)
pkg/controller/plan/adapter/base/doc.go (1)
Builder(120-155)
pkg/controller/plan/adapter/vsphere/builder.go (3)
pkg/controller/plan/adapter/base/doc.go (1)
Builder(120-155)pkg/settings/features.go (1)
Features(31-51)pkg/apis/forklift/v1beta1/mapping.go (2)
OffloadPlugin(55-57)VSphereXcopyPluginConfig(87-94)
pkg/controller/plan/kubevirt.go (2)
pkg/controller/plan/adapter/base/doc.go (1)
Builder(120-155)pkg/controller/plan/adapter/doc.go (1)
Builder(15-15)
pkg/controller/plan/adapter/ocp/builder.go (1)
pkg/controller/plan/adapter/base/doc.go (1)
Builder(120-155)
pkg/controller/plan/adapter/openstack/builder.go (1)
pkg/controller/plan/adapter/base/doc.go (1)
Builder(120-155)
pkg/controller/plan/adapter/ova/builder.go (2)
pkg/controller/plan/adapter/base/doc.go (1)
Builder(120-155)pkg/controller/plan/adapter/doc.go (1)
Builder(15-15)
⏰ 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 (5)
pkg/controller/plan/adapter/ocp/builder.go (1)
657-659: LGTMReturning false is consistent; no populator path for OCP source.
pkg/controller/plan/adapter/base/doc.go (1)
143-145: Interface update looks fine; verify all implementations/callers are updatedThe parameterless SupportsVolumePopulators() aligns with usage in adapters and kubevirt.
pkg/controller/plan/adapter/ova/builder.go (1)
520-523: LGTMOVA does not support populators; consistent with false.
pkg/controller/plan/adapter/ovirt/builder.go (1)
671-673: LGTMCapability correctly gated by plan warmth and destination host.
pkg/controller/plan/migration.go (1)
392-396: <!-- [scratchpad]
[task review_file_1/1 verify_concerns]
Completed verification with detailed context.[observations]
- Line 790:
if r.builder.SupportsVolumePopulators() && r.Plan.IsWarm()is intentional redundancy.
- vSphere builder returns false when IsWarm=true, so block unreachable for vSphere warm (by design)
- This is not a bug; it's conditional execution for specific provider/migration combinations
- DeletePopulatedPVCs (line 1336-1346): already iterates over getPVCs and deletes by ownership/labels
- Does NOT re-check builder support internally; cleanup logic is robust to builder state changes
- Line 392 gate is just a pre-check optimization, not a blocker
- Plan.IsWarm() is canonical (plan-level state); vm.Warm is per-VM status tracking (different purpose)
- Builder implementations are non-uniform by design (handled via if-else routing in migration.go)
[analysis]
- Original review concern is PARTIALLY VALID but OVERSTATED:
- Concern 1 (line 392 cleanup gate): MITIGATED—actual deletion is label/owner-based, not builder-dependent
- Concern 2 (line 790 redundancy): FALSE POSITIVE—intentional design for provider-specific behavior
- Concern 3 (non-uniform support): ACKNOWLEDGED—code handles via routing, not a bug
- Concern 4 (warm inconsistency): NOT A BUG—vm.Warm and Plan.IsWarm() serve different purposes
[pending]
None. Sufficient evidence gathered.[done]
Review comment is overly alarmist. The code is actually more robust than alleged. Key insights:
- Builder gate at line 392 is pre-check; actual cleanup uses ownership
- Line 790 design is correct (conditional for provider-specific behavior)
- Non-uniformity is handled by design in routing
Review comment contains overstated concerns; actual implementation is more robust than alleged
Your original review raises concerns about builder-wide SupportsVolumePopulators() gating, but the implementation mitigates the risks you flagged:
Line 392 cleanup gate: DeletePopulatedPVCs (lines 1336–1346) doesn't rely solely on the builder flag. It queries PVCs by owner references and deletes them based on labels/ownership. The gate is a pre-check optimization, not a hard blocker; if PVCs exist, they are deleted robustly.
Line 790 apparent redundancy: The condition
r.builder.SupportsVolumePopulators() && r.Plan.IsWarm()is intentional. For vSphere,SupportsVolumePopulators()returnsfalsewhenIsWarm()is true, so this block correctly bypasses adoption logic for warm vSphere migrations. This is provider-specific routing, not a reachability bug.Non-uniform builder support: The code handles this by design—different providers take different paths (vSphere/oRHV conditionally, OVA/OCP always false, OpenStack always true). This is reflected in the if-else logic at lines 727/756, which routes to PopulatorVolumes or CDI DataVolumes accordingly.
Warm inconsistency:
vm.Warmtracks per-VM status for checkpoints;Plan.IsWarm()is the plan-level decision source (per plan.go line 284–286). These serve different purposes and are used appropriately in their contexts.The code is actually more resilient than your comment suggests. No action required.
| func (r *Builder) SupportsVolumePopulators() bool { | ||
| return true | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Gate support on feature flags/config instead of always true
Returning true unconditionally may enable unsupported paths. Tie this to CopyOffload (and any provider prerequisites).
func (r *Builder) SupportsVolumePopulators() bool {
- return true
+ return settings.Settings.Features.CopyOffload
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/controller/plan/adapter/openstack/builder.go around lines 882-885, the
SupportsVolumePopulators method currently returns true unconditionally; change
it to return true only when the CopyOffload feature flag/config is enabled and
any provider-specific prerequisites are met (e.g., provider capabilities or
config on the Builder/Plan); implement a nil-safe check against the feature
flags/config and provider capability fields (or a helper like
r.Plan.HasFeature("CopyOffload") && r.providerSupportsCopyOffload()), and
add/update unit tests to cover both enabled and disabled cases.
|
|
||
| // Create DataVolumes unless this is a cold migration using storage offload | ||
| if r.Plan.IsWarm() || !r.builder.SupportsVolumePopulators(vm.Ref) { | ||
| if !r.builder.SupportsVolumePopulators() { |
There was a problem hiding this comment.
This one should stay too, a warm migration with storage offload needs to make DataVolumes.
There was a problem hiding this comment.
done.
generally this line is kind of the 'else' of line 727, and it looks redundant to call again r.builder.SupportsVolumePopulator.
Modify migration logic to handle warm migrations and volume population support. Signed-off-by: Roy Golan <rgolan@redhat.com>
|
5 participants
This commit removes the controller files that were accidentally included in the initial PowerStore support merge (PR #2595). The core feature logic remains unchanged.
Summary by CodeRabbit