MTV-5529 | Rework reusing of DI conversion CRs#6846
Open
solenoci wants to merge 8 commits into
Open
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe PR refactors deep-inspection Conversion discovery by extracting the lookup logic from Migration's inlined state machine into two new reusable public methods on KubeVirt: ChangesDeep Inspection Conversion Lookup and Usage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6846 +/- ##
==========================================
- Coverage 15.45% 10.80% -4.65%
==========================================
Files 112 521 +409
Lines 23377 62539 +39162
==========================================
+ Hits 3613 6758 +3145
- Misses 19479 55208 +35729
- Partials 285 573 +288
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Collaborator
Author
|
@coderabbitai pls review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get the PR details and diff
gh pr view 6846 --repo kubev2v/forklift --json title,body,files,additions,deletions
echo "=== PR DIFF ==="
gh pr diff 6846 --repo kubev2v/forkliftLength of output: 28581 🏁 Script executed: #!/bin/bash
# Check if GetDeepInspectionConversion is still used anywhere outside its definition
rg -n "GetDeepInspectionConversion" --type goLength of output: 432 Confirmed: Here's a summary of all findings:
Otherwise the logic is solid — the scoped lookup correctly prevents cross-plan reuse and the phase decision flow matches the documentation exactly. 🐇 |
Hazanel
reviewed
Jun 2, 2026
Contributor
There was a problem hiding this comment.
Isn't this a dead code now?
Collaborator
Author
There was a problem hiding this comment.
address from coderabbit review
Hazanel
reviewed
Jun 2, 2026
|
|
||
| // deepInspectionMatchesPlan reports whether the Conversion CR is compatible with | ||
| // what the plan would create for the given VM. Two fields affect this: | ||
| // - DiskEncryption.Type: a CR without dsik encryption can't be used for CR with disk encryption set and vice versa, |
Collaborator
Author
There was a problem hiding this comment.
address from coderabbit review
Hazanel
reviewed
Jun 2, 2026
| // All errors are consumed internally and recorded on the step, the caller never | ||
| // receives a non-nil error from this method. | ||
| func (r *Migration) runPreflightDeepInspection(vm *plan.VMStatus, step *plan.Step) (requeue bool) { | ||
| snapshotMoref := vm.Warm.Precopies[0].Snapshot |
Contributor
There was a problem hiding this comment.
Please add a nil guard here
Contributor
|
Hey @coderabbitai ensureConversion strips LabelPlan from the lookup labels before matching. Can this cause Phase 3 of one plan to match and overwrite a different plan's CR for the same VM, and if so, should LabelPlan be included in the lookup? |
![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
🧹 Nitpick comments (1)
pkg/controller/plan/migration.go (1)
1535-1648: 🏗️ Heavy liftMissing unit test coverage for new deep inspection methods.
The Codecov report indicates 0% coverage on the patch. The new
runPreflightDeepInspectionandhandlePlanDeepInspectionCRmethods, along with the helper functions inkubevirt.go, should have unit tests covering the key scenarios:
- Plan-owned CR exists (Phase 1 → Phase 3)
- Standalone CR found and succeeded (Phase 2 reuse)
- Standalone CR found but failed (Phase 2 delete and create new)
- Standalone CR in progress (Phase 2 requeue)
- No existing CR (create new)
As per coding guidelines: "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 `@pkg/controller/plan/migration.go` around lines 1535 - 1648, Add unit tests covering runPreflightDeepInspection and handlePlanDeepInspectionCR and the new kubevirt helper methods to restore coverage; specifically write table-driven tests that mock r.kubevirt responses to exercise: (1) plan-owned CR exists and transitions to Phase 3 (call handlePlanDeepInspectionCR path), (2) standalone CR found and succeeded (propagateInspectionConcerns and NextPhase), (3) standalone CR found but failed (DeleteConversion called and CreateDeepInspectionConversion invoked), (4) standalone CR in progress (returns requeue true), and (5) no existing CR (CreateDeepInspectionConversion called and returns requeue true). Use test doubles/fakes for kubevirt methods (GetPlanDeepInspectionConversion, GetStandaloneDeepInspectionConversion, DeleteConversion, CreateDeepInspectionConversion) and assert step.Error/step.AddError behavior, NextPhase calls, and returned requeue booleans; name tests clearly (e.g., TestRunPreflightDeepInspection_PlanOwnedExists, _StandaloneSucceeded, _StandaloneFailedDeletesAndCreates, _StandaloneInProgressRequeues, _NoExistingCRCreatesAndRequeues) so CI verifies coverage for these functions.
🤖 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 `@pkg/controller/plan/migration.go`:
- Around line 1556-1558: In runPreflightDeepInspection, add a defensive check
before accessing vm.Warm.Precopies[0] — verify vm != nil, vm.Warm != nil and
len(vm.Warm.Precopies) > 0 before reading vm.Warm.Precopies[0].Snapshot; if the
check fails, log a concise warning (using the controller's logger) and return
requeue=true to avoid a panic and retry later. Ensure you update all code paths
in runPreflightDeepInspection that assume vm.Warm.Precopies[0] so they only run
after the guard passes.
---
Nitpick comments:
In `@pkg/controller/plan/migration.go`:
- Around line 1535-1648: Add unit tests covering runPreflightDeepInspection and
handlePlanDeepInspectionCR and the new kubevirt helper methods to restore
coverage; specifically write table-driven tests that mock r.kubevirt responses
to exercise: (1) plan-owned CR exists and transitions to Phase 3 (call
handlePlanDeepInspectionCR path), (2) standalone CR found and succeeded
(propagateInspectionConcerns and NextPhase), (3) standalone CR found but failed
(DeleteConversion called and CreateDeepInspectionConversion invoked), (4)
standalone CR in progress (returns requeue true), and (5) no existing CR
(CreateDeepInspectionConversion called and returns requeue true). Use test
doubles/fakes for kubevirt methods (GetPlanDeepInspectionConversion,
GetStandaloneDeepInspectionConversion, DeleteConversion,
CreateDeepInspectionConversion) and assert step.Error/step.AddError behavior,
NextPhase calls, and returned requeue booleans; name tests clearly (e.g.,
TestRunPreflightDeepInspection_PlanOwnedExists, _StandaloneSucceeded,
_StandaloneFailedDeletesAndCreates, _StandaloneInProgressRequeues,
_NoExistingCRCreatesAndRequeues) so CI verifies coverage for these functions.
🪄 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: CHILL
Plan: Pro
Run ID: ee9aca56-60dd-478d-b2a0-3319011f7a3c
⛔ Files ignored due to path filters (1)
docs/conversion-cr.mdis excluded by!**/*.md
📒 Files selected for processing (2)
pkg/controller/plan/kubevirt.gopkg/controller/plan/migration.go
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get the full ensureConversion function and surrounding context
rg -n "ensureConversion\|LabelPlan\|GetPlanDeepInspection\|deepInspectionMatch\|Phase" --type go pkg/controller/plan/kubevirt.go | head -120
echo "=== ensureConversion implementation ==="
ast-grep --pattern 'func ($r) ensureConversion($$$) {
$$$
}'
echo "=== GetPlanDeepInspectionConversion ==="
ast-grep --pattern 'func ($r) GetPlanDeepInspectionConversion($$$) {
$$$
}'
echo "=== deepInspectionMatchesPlan ==="
ast-grep --pattern 'func ($r) deepInspectionMatchesPlan($$$) {
$$$
}'Length of output: 925 🏁 Script executed: #!/bin/bash
# Get broader context around ensureConversion in kubevirt.go
grep -n "ensureConversion\|LabelPlan\|label\|Label" pkg/controller/plan/kubevirt.go | head -80
echo "=== Full content around ensureConversion ==="
grep -n "func.*ensureConversion\|func.*DeepInspection\|func.*deepInspection" pkg/controller/plan/kubevirt.go
echo "=== Get the PR diff for kubevirt.go ==="
gh pr diff 6846 --repo kubev2v/forklift -- pkg/controller/plan/kubevirt.goLength of output: 5868
How // line 585-595 (kubevirt.go)
func (r *KubeVirt) ensureConversion(cr *api.Conversion) (*api.Conversion, error) {
lookupLabels := make(map[string]string, len(cr.Labels))
for k, v := range cr.Labels {
if k != convctx.LabelPlan { // ← LabelPlan stripped
lookupLabels[k] = v
}
}
// searches by: LabelVM + LabelConversionType + extra (e.g. LabelRetryAllowed)When
The cross-plan overwrite scenario: If Plan A and Plan B both migrate the same VM and both reach Phase 3 simultaneously: sequenceDiagram
participant PlanB
participant PlanA
participant Cluster
PlanB->>Cluster: CreateDeepInspectionConversion(vm=X) → CR with LabelPlan=B
PlanA->>Cluster: ensureConversion lookup (LabelVM=X, LabelType=DI, no LabelPlan)
Cluster-->>PlanA: returns Plan B's CR (matches!)
PlanA->>Cluster: existing.Labels = cr.Labels → stamps LabelPlan=A onto Plan B's CR
Plan B's DI CR is effectively stolen — its Why Suggested fix: For Phase 3 specifically (creating a new plan-owned CR), the lookup should include // Phase 3 create: first try an exact plan-scoped lookup
existing, err := r.getConversion(cr.Labels) // includes LabelPlan
if err != nil || existing == nil {
// Fall back to plan-agnostic lookup (handles plan re-creation)
existing, err = r.getConversion(lookupLabels) // without LabelPlan
}This would prevent Plan A from matching Plan B's CR while still handling the re-creation case. Would you like me to open a follow-up issue for this? |
Collaborator
Author
|
Addressed the logic flow issue. Now the flow looks like this |
Contributor
|
lgtm |
Issue: When a plan tries to reuse a deep inspection CR, it doesn't scope it's search to the plan owned/standalone and is reusing old/wring CRs. Fix: change the search mechanism to first look for plan DI CRs, if not found then look for standalone CRs (with excluding other plans CRs), and if not found create a new plan owned one. Resolves: MTV-5529 Ref: https://redhat.atlassian.net/browse/MTV-5529 Signed-off-by: Stefan Olenocin <solenoci@redhat.com>
Signed-off-by: Stefan Olenocin <solenoci@redhat.com>
Signed-off-by: Stefan Olenocin <solenoci@redhat.com>
Signed-off-by: Stefan Olenocin <solenoci@redhat.com>
Signed-off-by: Stefan Olenocin <solenoci@redhat.com>
Signed-off-by: Stefan Olenocin <solenoci@redhat.com>
Signed-off-by: Stefan Olenocin <solenoci@redhat.com>
Collaborator
Author
|
@coderabbitai PTAL |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== Latest PR diff ==="
gh pr diff 6846 --repo kubev2v/forklift
echo ""
echo "=== Check for GetDeepInspectionConversion (dead code) ==="
rg -n "GetDeepInspectionConversion" --type go
echo ""
echo "=== Check for typo 'dsik' ==="
rg -n "dsik" --type go
echo ""
echo "=== Check incomplete comment 'evaluate it di CR' ==="
rg -n "evaluate it" --type go
echo ""
echo "=== ensureConversion full implementation ==="
rg -n "func.*ensureConversion\|LabelPlan\|lookupLabel" --type go pkg/controller/plan/kubevirt.go | head -60
echo ""
echo "=== Check runPreflightDeepInspection ==="
rg -n "runPreflightDeepInspection\|Phase 1\|Phase 2\|Phase 3" --type go pkg/controller/plan/migration.go | head -40
echo ""
echo "=== vmDiskEncType comment check ==="
rg -n "vmDiskEncType\|Clevis\|LUKS\|clevis" --type go pkg/controller/plan/kubevirt.go | head -20Length of output: 35075
All prior issues resolved ✅
New code observations
One minor doc/code discrepancy The Phase 2 table in
But the code intentionally does not delete standalone CRs in either case ( | `Failed`/`Canceled` | Create a plan owned CR (standalone is not deleted); Phase 1 picks up the new CR on the next reconcile. |Everything else looks solid. Logic matches the updated flow diagram and the documentation. 🐇 🧠 Learnings used |
Signed-off-by: Stefan Olenocin <solenoci@redhat.com>
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@docs/conversion-cr.md`:
- Around line 213-230: The fenced decision-flow block for
PhasePreflightInspection lacks a language tag causing markdown lint errors;
update the opening fence from ``` to a language-tagged fence such as ```text (or
```flow) for the block that begins with "PhasePreflightInspection" so the
markdown linter recognizes the code block and the content (Phase 1 / Phase 2 /
Phase 3 flow) remains unchanged.
- Line 170: Update the phrasing to use hyphenated compound adjectives: replace
occurrences of “two phase” with “two-phase” and “plan owned” with “plan-owned”
(this applies to the sentence containing “two phase lookup” and the other
occurrences at the indicated locations), ensuring consistency across the phrases
“two-phase lookup” and “plan-owned one” in the docs/conversion-cr.md text.
- Around line 198-206: Update the docs to reflect the actual controller
behavior: replace the table rows and explanatory paragraph so they state that
for standalone CRs in `Failed`/`Canceled` state the controller will create a
plan-owned CR and requeue without deleting the standalone CR (the code path is
implemented in runPreflightDeepInspection and handleDeepInspectionCR), and
adjust the rationale that ensureConversion’s plan-label-agnostic lookup means
the controller intentionally leaves the standalone CR in place while adding a
plan-owned CR rather than deleting it before Phase 3; also mirror this change in
the other referenced block (around lines 222-226) so the documentation matches
ensureConversion, runPreflightDeepInspection, and handleDeepInspectionCR
behavior.
In `@pkg/controller/plan/kubevirt.go`:
- Around line 664-690: GetGuestConversion currently lists Conversion CRs only by
convctx.LabelPlan and convctx.LabelVM, so it can return old conversions; scope
the lookup to the active migration by adding a requirement on
convctx.LabelMigration equal to the current migration identifier (the same value
EnsureConversion sets) before calling r.List. Update GetGuestConversion to
include the convctx.LabelMigration selector (use the same migration ID field
that EnsureConversion stamps, e.g. r.Migration.UID or the migration id variable
EnsureConversion uses) so only conversions for the active migration are
returned.
In `@pkg/controller/plan/migration.go`:
- Around line 1695-1700: The call to r.kubevirt.DeleteConversion(cr) can delete
standalone DI pods/secrets because DeleteConversion currently removes resources
by vmID+conversionType only; change the code so cancellation only deletes
resources owned by that plan CR: add a new method (e.g., DeleteConversionOwned
or extend DeleteConversion to accept an owner UID/flag) that filters deletions
by the CR's ownerReference/UID or an owner label, and use that method here (call
DeleteConversionOwned(cr) or DeleteConversion(cr, ownerUID=true)) so only
resources with an ownerReference/label matching cr are removed, leaving
standalone DI resources intact.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 685b74de-048b-443f-a9b7-8070035ab072
📒 Files selected for processing (5)
docs/conversion-cr.mdpkg/controller/conversion/builder.gopkg/controller/conversion/context/doc.gopkg/controller/plan/kubevirt.gopkg/controller/plan/migration.go
💤 Files with no reviewable changes (1)
- pkg/controller/conversion/context/doc.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
🤖 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 `@docs/conversion-cr.md`:
- Around line 213-230: The fenced decision-flow block for
PhasePreflightInspection lacks a language tag causing markdown lint errors;
update the opening fence from ``` to a language-tagged fence such as ```text (or
```flow) for the block that begins with "PhasePreflightInspection" so the
markdown linter recognizes the code block and the content (Phase 1 / Phase 2 /
Phase 3 flow) remains unchanged.
- Line 170: Update the phrasing to use hyphenated compound adjectives: replace
occurrences of “two phase” with “two-phase” and “plan owned” with “plan-owned”
(this applies to the sentence containing “two phase lookup” and the other
occurrences at the indicated locations), ensuring consistency across the phrases
“two-phase lookup” and “plan-owned one” in the docs/conversion-cr.md text.
- Around line 198-206: Update the docs to reflect the actual controller
behavior: replace the table rows and explanatory paragraph so they state that
for standalone CRs in `Failed`/`Canceled` state the controller will create a
plan-owned CR and requeue without deleting the standalone CR (the code path is
implemented in runPreflightDeepInspection and handleDeepInspectionCR), and
adjust the rationale that ensureConversion’s plan-label-agnostic lookup means
the controller intentionally leaves the standalone CR in place while adding a
plan-owned CR rather than deleting it before Phase 3; also mirror this change in
the other referenced block (around lines 222-226) so the documentation matches
ensureConversion, runPreflightDeepInspection, and handleDeepInspectionCR
behavior.
In `@pkg/controller/plan/kubevirt.go`:
- Around line 664-690: GetGuestConversion currently lists Conversion CRs only by
convctx.LabelPlan and convctx.LabelVM, so it can return old conversions; scope
the lookup to the active migration by adding a requirement on
convctx.LabelMigration equal to the current migration identifier (the same value
EnsureConversion sets) before calling r.List. Update GetGuestConversion to
include the convctx.LabelMigration selector (use the same migration ID field
that EnsureConversion stamps, e.g. r.Migration.UID or the migration id variable
EnsureConversion uses) so only conversions for the active migration are
returned.
In `@pkg/controller/plan/migration.go`:
- Around line 1695-1700: The call to r.kubevirt.DeleteConversion(cr) can delete
standalone DI pods/secrets because DeleteConversion currently removes resources
by vmID+conversionType only; change the code so cancellation only deletes
resources owned by that plan CR: add a new method (e.g., DeleteConversionOwned
or extend DeleteConversion to accept an owner UID/flag) that filters deletions
by the CR's ownerReference/UID or an owner label, and use that method here (call
DeleteConversionOwned(cr) or DeleteConversion(cr, ownerUID=true)) so only
resources with an ownerReference/label matching cr are removed, leaving
standalone DI resources intact.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 685b74de-048b-443f-a9b7-8070035ab072
📒 Files selected for processing (5)
docs/conversion-cr.mdpkg/controller/conversion/builder.gopkg/controller/conversion/context/doc.gopkg/controller/plan/kubevirt.gopkg/controller/plan/migration.go
💤 Files with no reviewable changes (1)
- pkg/controller/conversion/context/doc.go
🛑 Comments failed to post (5)
docs/conversion-cr.md (3)
170-170:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix compound adjective hyphenation for phase/ownership terms.
Use hyphenated forms in these edited phrases for consistency/readability: “two-phase”, “plan-owned”.
Also applies to: 196-196, 202-202, 209-209
🧰 Tools
🪛 LanguageTool
[grammar] ~170-~170: Use a hyphen to join words.
Context: ...reflightInspection` step through a two phase lookup that can reuse a compatib...(QB_NEW_EN_HYPHEN)
[grammar] ~170-~170: Use a hyphen to join words.
Context: ...ndalone CR before falling back to a plan owned one. #### Compatibility check Be...(QB_NEW_EN_HYPHEN)
🤖 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 `@docs/conversion-cr.md` at line 170, Update the phrasing to use hyphenated compound adjectives: replace occurrences of “two phase” with “two-phase” and “plan owned” with “plan-owned” (this applies to the sentence containing “two phase lookup” and the other occurrences at the indicated locations), ensuring consistency across the phrases “two-phase lookup” and “plan-owned one” in the docs/conversion-cr.md text.Source: Linters/SAST tools
198-206:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeep-inspection flow docs don’t match controller behavior for standalone
Failed/CanceledCRs.The table/flow says standalone
Failed/CanceledCRs are deleted before Phase 3, butrunPreflightDeepInspection+handleDeepInspectionCRcurrently create a plan-owned CR and requeue without deleting the standalone CR. Please align this section (including the “deleted before Phase 3” rationale) with the actual logic to avoid operator confusion.Also applies to: 222-226
🧰 Tools
🪛 LanguageTool
[grammar] ~202-~202: Use a hyphen to join words.
Context: ...ll through to Phase 3 to create the plan owned CR. | | None found ...(QB_NEW_EN_HYPHEN)
🤖 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 `@docs/conversion-cr.md` around lines 198 - 206, Update the docs to reflect the actual controller behavior: replace the table rows and explanatory paragraph so they state that for standalone CRs in `Failed`/`Canceled` state the controller will create a plan-owned CR and requeue without deleting the standalone CR (the code path is implemented in runPreflightDeepInspection and handleDeepInspectionCR), and adjust the rationale that ensureConversion’s plan-label-agnostic lookup means the controller intentionally leaves the standalone CR in place while adding a plan-owned CR rather than deleting it before Phase 3; also mirror this change in the other referenced block (around lines 222-226) so the documentation matches ensureConversion, runPreflightDeepInspection, and handleDeepInspectionCR behavior.
213-230:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language tag to the fenced decision-flow block.
The code fence should specify a language (e.g., ```text) to satisfy markdown linting.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 213-213: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/conversion-cr.md` around lines 213 - 230, The fenced decision-flow block for PhasePreflightInspection lacks a language tag causing markdown lint errors; update the opening fence from ``` to a language-tagged fence such as ```text (or ```flow) for the block that begins with "PhasePreflightInspection" so the markdown linter recognizes the code block and the content (Phase 1 / Phase 2 / Phase 3 flow) remains unchanged.Source: Linters/SAST tools
pkg/controller/plan/kubevirt.go (1)
664-690:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope guest Conversion CR lookup to the active migration.
GetGuestConversion()ignoresconvctx.LabelMigration, butEnsureConversion()stamps that label when it creates the CR. Because old guest Conversion CRs are not cleaned up here, a later retry/run can leave multipleRemote/InPlaceCRs for the same plan+VM. Once the pod is gone,updateConversionProgress()can read an older succeeded/failed CR and complete the current step with the wrong status.Suggested fix
func (r *KubeVirt) GetGuestConversion(vm *plan.VMStatus) (*api.Conversion, error) { typeReq, err := k8slabels.NewRequirement( convctx.LabelConversionType, selection.In, []string{string(api.Remote), string(api.InPlace)}, ) if err != nil { return nil, liberr.Wrap(err) } - selector := k8slabels.SelectorFromSet(map[string]string{ - convctx.LabelPlan: string(r.Plan.UID), - convctx.LabelVM: vm.ID, - }).Add(*typeReq) + match := map[string]string{ + convctx.LabelPlan: string(r.Plan.UID), + convctx.LabelVM: vm.ID, + } + if r.Migration != nil { + match[convctx.LabelMigration] = string(r.Migration.UID) + } + selector := k8slabels.SelectorFromSet(match).Add(*typeReq)🤖 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 `@pkg/controller/plan/kubevirt.go` around lines 664 - 690, GetGuestConversion currently lists Conversion CRs only by convctx.LabelPlan and convctx.LabelVM, so it can return old conversions; scope the lookup to the active migration by adding a requirement on convctx.LabelMigration equal to the current migration identifier (the same value EnsureConversion sets) before calling r.List. Update GetGuestConversion to include the convctx.LabelMigration selector (use the same migration ID field that EnsureConversion stamps, e.g. r.Migration.UID or the migration id variable EnsureConversion uses) so only conversions for the active migration are returned.pkg/controller/plan/migration.go (1)
1695-1700:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeleting a canceled plan-owned DI CR can tear down standalone DI resources too.
This new
DeleteConversion(cr)path is unsafe for the standalone/plan-owned split introduced here.DeleteConversion()currently deletes pods and secrets by onlyvmID + conversionType, so canceling a plan-owned deep-inspection CR can also remove a standalone deep-inspection pod/secret for the same VM that this plan does not own.Suggested fix
// pkg/controller/plan/kubevirt.go func (r *KubeVirt) deleteConversionPod(cr *api.Conversion) error { ... matchLabels := map[string]string{ convctx.LabelVM: cr.Labels[convctx.LabelVM], convctx.LabelConversionType: string(cr.Spec.Type), } + if planID := cr.Labels[convctx.LabelPlan]; planID != "" { + matchLabels[convctx.LabelPlan] = planID + } ... } func (r *KubeVirt) deleteConversionSecrets(cr *api.Conversion) error { ... matchLabels := map[string]string{ convctx.LabelVM: vmID, convctx.LabelConversionType: string(cr.Spec.Type), } + if planID := cr.Labels[convctx.LabelPlan]; planID != "" { + matchLabels[convctx.LabelPlan] = planID + } ... }🤖 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 `@pkg/controller/plan/migration.go` around lines 1695 - 1700, The call to r.kubevirt.DeleteConversion(cr) can delete standalone DI pods/secrets because DeleteConversion currently removes resources by vmID+conversionType only; change the code so cancellation only deletes resources owned by that plan CR: add a new method (e.g., DeleteConversionOwned or extend DeleteConversion to accept an owner UID/flag) that filters deletions by the CR's ownerReference/UID or an owner label, and use that method here (call DeleteConversionOwned(cr) or DeleteConversion(cr, ownerUID=true)) so only resources with an ownerReference/label matching cr are removed, leaving standalone DI resources intact.
mnecas
reviewed
Jun 8, 2026
| { | ||
| Env: nil, // set by type-specific builder | ||
| ImagePullPolicy: core.PullAlways, | ||
| ImagePullPolicy: core.PullIfNotPresent, |
Collaborator
Author
There was a problem hiding this comment.
we don't, I used pullAlways for testing purposes and thought that I should adhere to the rest of pod definitions, will revert.
solenoci
added a commit
to solenoci/forklift
that referenced
this pull request
Jun 8, 2026
Issue: DI conversion CR reusing has a bug where it is reusing wrong conversions from a different plan. The fix could to properly reuse the conversions with a defined flow. kubev2v#6846 However that fix is complicated and could introduce more bugs. Fix: Instead of implementing the proper reusing of resources kubev2v#6846 simplify the logic to not reuse DI conversions at all for now. Later we can add the proper method defined in the other PR. Resolves: MTV-5529 Signed-off-by: Stefan Olenocin <solenoci@redhat.com>
solenoci
added a commit
to solenoci/forklift
that referenced
this pull request
Jun 8, 2026
Issue: DI conversion CR reusing has a bug where it is reusing wrong conversions from a different plan. The fix could to properly reuse the conversions with a defined flow. kubev2v#6846 However that fix is complicated and could introduce more bugs. Fix: Instead of implementing the proper reusing of resources kubev2v#6846 simplify the logic to not reuse DI conversions at all for now. Later we can add the proper method defined in the other PR. Resolves: MTV-5529 Signed-off-by: Stefan Olenocin <solenoci@redhat.com>
Hazanel
pushed a commit
that referenced
this pull request
Jun 8, 2026
Issue: DI conversion CR reusing has a bug where it is reusing wrong conversions from a different plan. The fix could to properly reuse the conversions with a defined flow. #6846 However that fix is complicated and could introduce more bugs. Fix: Instead of implementing the proper reusing of resources #6846 simplify the logic to not reuse DI conversions at all for now. Later we can add the proper method defined in the other PR. Resolves: MTV-5529 --------- Signed-off-by: Stefan Olenocin <solenoci@redhat.com>
solenoci
added a commit
that referenced
this pull request
Jun 8, 2026
…Rs (#7019) **Backport:** #7008 Issue: DI conversion CR reusing has a bug where it is reusing wrong conversions from a different plan. The fix could to properly reuse the conversions with a defined flow. #6846 However that fix is complicated and could introduce more bugs. Fix: Instead of implementing the proper reusing of resources #6846 simplify the logic to not reuse DI conversions at all for now. Later we can add the proper method defined in the other PR. Resolves: MTV-5529 --------- Signed-off-by: Stefan Olenocin <solenoci@redhat.com> Co-authored-by: Stefan Olenocin <solenoci@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue: When a plan tries to reuse a deep inspection CR, it doesn't scope it's search to the plan owned/standalone and is reusing old/wring CRs.
Fix: change the search mechanism to first look for plan DI CRs, if not found then look for standalone CRs (with excluding other plans CRs), and if not found create a new plan owned one.
Resolves: MTV-5529
Ref: https://redhat.atlassian.net/browse/MTV-5529
Summary by CodeRabbit
Bug Fixes
Performance
Documentation