feat(vmop): improve vmop migration condition #1569
Open
+101
−10
Conversation
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
Reviewer's GuideEnhance VMOP migration conditions by introducing dynamic messaging based on migration failure reasons and pod scheduling status, refactoring event logging, updating dependencies, and applying minor handler and build config adjustments. Sequence diagram for dynamic migration condition messagingsequenceDiagram
participant H as LifecycleHandler
participant M as VirtualMachineInstanceMigration
participant P as Pod
participant C as Conditions
H->>M: getConditionCompletedMessageByReason(ctx, reason, mig)
H->>M: getTargetPod(ctx, mig)
M-->>H: returns Pod
H->>P: isPodPendingUnschedulable(pod)
P-->>H: returns bool
H->>C: SetCondition with dynamic message
Class diagram for updated LifecycleHandler methodsclassDiagram
class LifecycleHandler {
+syncOperationComplete(ctx, vmop)
+execute(ctx, vmop)
+getTargetPod(ctx, mig)
+getConditionCompletedMessageByReason(ctx, reason, mig)
}
class virtv1.VirtualMachineInstanceMigration {
+Status
+UID
+Namespace
}
class corev1.Pod {
+Status
+Namespace
+Name
+DeletionTimestamp
}
LifecycleHandler --> virtv1.VirtualMachineInstanceMigration : uses
LifecycleHandler --> corev1.Pod : uses
LifecycleHandler : getTargetPod returns Pod
LifecycleHandler : getConditionCompletedMessageByReason uses getTargetPod
LifecycleHandler : syncOperationComplete uses getConditionCompletedMessageByReason
LifecycleHandler : execute uses getConditionCompletedMessageByReason
Class diagram for new helper functions related to migration conditionsclassDiagram
class getMessageByMigrationFailedReason {
+mig: VirtualMachineInstanceMigration
+returns: string
}
class isPodPendingUnschedulable {
+pod: Pod
+returns: bool
}
getMessageByMigrationFailedReason --> virtv1.VirtualMachineInstanceMigration : parameter
isPodPendingUnschedulable --> corev1.Pod : parameter
LifecycleHandler ..> getMessageByMigrationFailedReason : uses
LifecycleHandler ..> isPodPendingUnschedulable : uses
Class diagram for MigratingHandler syncMigrating updateclassDiagram
class MigratingHandler {
+syncMigrating(ctx, s)
}
class completed {
+Message: string
}
MigratingHandler --> completed : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
yaroslavborbat
force-pushed
the
feat/vmop/improve-migration-conditions
branch
from
e2fc1e8 to
d4cedd3
Compare
yaroslavborbat
force-pushed
the
feat/vmop/improve-migration-conditions
branch
from
da078fe to
9bbe400
Compare
yaroslavborbat
requested review from
Isteb4k,
fl64,
nevermarine and
universal-itengineer
as code owners
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The new pod lookup and unschedulable-check logic in LifecycleHandler could be extracted into a dedicated helper or service to keep the handler focused on orchestration and improve testability.
- Consider centralizing the dynamic condition message templates (including punctuation) in one place to ensure consistency and simplify future updates or localizations.
- The getTargetPod method issues a direct list call on every reconcile; you might optimize it by leveraging a cached informer or limiting API calls in high-frequency paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new pod lookup and unschedulable-check logic in LifecycleHandler could be extracted into a dedicated helper or service to keep the handler focused on orchestration and improve testability.
- Consider centralizing the dynamic condition message templates (including punctuation) in one place to ensure consistency and simplify future updates or localizations.
- The getTargetPod method issues a direct list call on every reconcile; you might optimize it by leveraging a cached informer or limiting API calls in high-frequency paths.
## Individual Comments
### Comment 1
<location> `images/virtualization-artifact/pkg/controller/vmop/migration/internal/handler/lifecycle.go:519-520` </location>
<code_context>
virtv1.MigrationFailed: vmopcondition.ReasonOperationFailed,
}
+
+func getMessageByMigrationFailedReason(mig *virtv1.VirtualMachineInstanceMigration) string {
+ cond, found := conditions.GetKVVMIMCondition(virtv1.VirtualMachineInstanceMigrationFailed, mig.Status.Conditions)
+
</code_context>
<issue_to_address>
**suggestion:** Consider handling nil mig defensively in getMessageByMigrationFailedReason.
Accessing mig.Status.Conditions without checking if mig is nil can cause a panic. Add a nil check at the start of the function to prevent this.
```suggestion
func getMessageByMigrationFailedReason(mig *virtv1.VirtualMachineInstanceMigration) string {
if mig == nil {
return "migration object is nil"
}
cond, found := conditions.GetKVVMIMCondition(virtv1.VirtualMachineInstanceMigrationFailed, mig.Status.Conditions)
```
</issue_to_address>
### Comment 2
<location> `images/virtualization-artifact/pkg/controller/vmop/migration/internal/handler/lifecycle.go:584-586` </location>
<code_context>
+ return false
+}
+
+func (h LifecycleHandler) getConditionCompletedMessageByReason(ctx context.Context, reason vmopcondition.ReasonCompleted, mig *virtv1.VirtualMachineInstanceMigration) (string, error) {
+ msg := "Wait until operation is completed"
+ if reason == vmopcondition.ReasonMigrationPending || reason == vmopcondition.ReasonMigrationPrepareTarget {
</code_context>
<issue_to_address>
**suggestion:** Consider including more context in the unschedulable pod message.
Including the specific scheduling error or reason, if available, would make the message more informative for debugging and user feedback.
```suggestion
if isPodPendingUnschedulable(pod) {
unschedulableMsg := ""
for _, cond := range pod.Status.Conditions {
if cond.Type == corev1.PodScheduled &&
cond.Status == corev1.ConditionFalse &&
cond.Reason == corev1.PodReasonUnschedulable {
unschedulableMsg = cond.Message
break
}
}
if unschedulableMsg != "" {
msg += fmt.Sprintf(" (target pod is unschedulable: %s/%s, reason: %q)", pod.Namespace, pod.Name, unschedulableMsg)
} else {
msg += fmt.Sprintf(" (target pod is unschedulable: %s/%s)", pod.Namespace, pod.Name)
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Yaroslav Borbat <[email protected]>
yaroslavborbat
force-pushed
the
feat/vmop/improve-migration-conditions
branch
from
5152274 to
14449b5
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Description
Improve VMOp migration condition
This PR enhances the migration condition reporting for VM operations.
It adds more detailed and descriptive messages when a migration fails — for example:
These improvements make it easier to diagnose and understand the cause of migration failures.
Why do we need it, and what problem does it solve?
What is the expected result?
Checklist
Changelog entries