migration state: add source pod and failure reason #11330
Conversation
|
/cc @vladikr |
|
/hold |
0501f02
to
6e2f63a
Compare
|
/hold cancel |
| @@ -837,7 +837,8 @@ func (c *MigrationController) handleMarkMigrationFailedOnVMI(migration *virtv1.V | |||
| return err | |||
| } | |||
| log.Log.Object(vmi).Infof("Marked Migration %s/%s failed on vmi due to target pod disappearing before migration kicked off.", migration.Namespace, migration.Name) | |||
| c.recorder.Event(vmi, k8sv1.EventTypeWarning, FailedMigrationReason, fmt.Sprintf("VirtualMachineInstance migration uid %s failed. reason: target pod is down", string(migration.UID))) | |||
| vmiCopy.Status.MigrationState.FailureReason = "Target pod is down" | |||
| c.recorder.Event(vmi, k8sv1.EventTypeWarning, FailedMigrationReason, fmt.Sprintf("VirtualMachineInstance migration uid %s failed. reason: %s", string(migration.UID), vmiCopy.Status.MigrationState.FailureReason)) | |||
There was a problem hiding this comment.
Is there a way that we will already have a Failure Reason from virt-handler at this point?
There was a problem hiding this comment.
Yeah that's possible. What do you suggest?
There was a problem hiding this comment.
Wouldn't creating the event for the migration object solve this?
There was a problem hiding this comment.
@Barakmor1 I think these are two separate records. We need to store this info in the status for convenience. That way everything related to this migration is stored in one place.
There was a problem hiding this comment.
Yeah that's possible. What do you suggest?
@jean-edouard I think we can merge. To simply check whether MigrationState.FailureReason has something already and add this message. wdyt?
| migration := libmigration.New(vmi.Name, vmi.Namespace) | ||
| migration = libmigration.RunMigrationAndExpectToCompleteWithDefaultTimeout(virtClient, migration) | ||
| By("Checking VMI, confirm migration state") | ||
| libmigration.ConfirmVMIPostMigration(virtClient, vmi, migration) | ||
| vmi = libmigration.ConfirmVMIPostMigration(virtClient, vmi, migration) | ||
| Expect(vmi.Status.MigrationState.SourcePod).To(Equal(sourcePod.Name)) |
There was a problem hiding this comment.
nit:
ExpectWithOffset would be better in that case
There was a problem hiding this comment.
The rest of the function doesn't use WithOffset, and I would argue that it's a good thing.
WithOffset effectively obfuscates which line of the function tripped. and this is a massive function called once by 2 different tests.
The code for the first test that uses the function is 2 lines long, and the second one has just 1 line: the call to this function!
So, in this case, moving the failure up one level effectively makes the line number meaningless.
WDYT?
pkg/controller/controller.go
Outdated
| @@ -346,6 +346,21 @@ func SetVMIMigrationPhaseTransitionTimestamp(oldVMIMigration *v1.VirtualMachineI | |||
| } | |||
| } | |||
|
|
|||
| func SetSourcePod(migration *v1.VirtualMachineInstanceMigration, vmi *v1.VirtualMachineInstance, podInformer cache.SharedIndexInformer) { | |||
| if migration.Status.Phase == v1.MigrationPending { | |||
There was a problem hiding this comment.
nit:
if migration.Status.Phase != v1.MigrationPending {
return
}
Signed-off-by: Jed Lejosne <jed@redhat.com>
Signed-off-by: Jed Lejosne <jed@redhat.com>
6e2f63a
to
563d33d
Compare
|
/lgtm |
|
@jean-edouard: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/approve Looks good to me. |
|
Thanks @jean-edouard ! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vladikr The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Required labels detected, running phase 2 presubmits: |
|
@jean-edouard should we backport this? |
|
/cherry-pick release-1.2 |
|
@jean-edouard: new pull request created: #11371 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What this PR does
Before this PR:
Migration state has no source pod and no failure reason
After this PR:
Migration state has a source pod and a failure reason
Fixes #
Why we need it and why it was done in this way
The following tradeoffs were made:
The following alternatives were considered:
Links to places where the discussion took place:
Special notes for your reviewer
Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
Release note