Fail VMIs in irrecoverable state after an interrupted postcopy migration #11479

vladikr · 2024-03-11T02:18:40Z

What this PR does

Before this PR:
Virtual machine instances would get stuck in a Paused/PostcopyFailed state.
Users cannot recover their VMs from this state.
Furthermore, the virt-handler will endlessly try to unpause the VMI, which will never succeed.

After this PR:
This PR lets the virt-handler Fail VMIs that have reached an irrecoverable state.
It is also adding functional tests to make sure that VMs will be eventually restarted.

Fixes # issues.redhat.com/browse/CNV-38195

virtual machines instance will no longer be stuck in an irrecoverable state after an interrupted postcopy migration. Instead, these will fail and could be restarted again.

fossedihelm

Thanks @vladikr!
I am still reviewing the second commit but can you please tell me if the missing tryGracefully is relevant? Thank you!

fossedihelm · 2024-03-12T12:54:13Z

pkg/virt-handler/vm.go

+	return d.helperVmShutdown(vmi, domain, tryGracefully)
+}
+
+func (d *VirtualMachineController) helperVmShutdown(vmi *v1.VirtualMachineInstance, domain *api.Domain, tryGracefully bool) error {


this tryGracefully is never used, so we perform gracefully shutdown if ACPI feature is available.

@fossedihelm eagle eyes! Thanks.

fossedihelm · 2024-03-12T13:01:16Z

tests/migration/postcopy.go

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * Copyright 2024 Red Hat, Inc.


Suggested change

* Copyright 2024 Red Hat, Inc.

* Copyright The KubeVirt Authors.

fossedihelm

@vladikr Sorry for the splitted review :)
Some comments below

fossedihelm · 2024-03-12T14:19:58Z

tests/migration/postcopy.go

+
+			})
+
+			Context("[Serial] should migrate using for postcopy", func() {


This [Serial] is misleading since actually one of the Entry of the DescribeTable inside of it will run in parallel due to the missing Serial decorator.

Thanks. Yes, the top level [Serial] was a mistake, it wasn't actually Serial

fossedihelm · 2024-03-12T14:21:09Z

tests/migration/postcopy.go

+						}, 150, 1*time.Second).Should(Equal(v1.MigrationFailed))
+					}
+
+					It("should make sure that VM restarts after failur", func() {


Suggested change

It("should make sure that VM restarts after failur", func() {

It("should make sure that VM restarts after failure", func() {

fossedihelm · 2024-03-12T14:31:44Z

tests/migration/postcopy.go

+						runMigrationKillerPod(vmi.Status.NodeName)
+
+						By("Making sure that post-copy migration failed")
+						observePostCopyMigrationFailure(migration)


This function can be dropped in favor of

Eventually(matcher.ThisMigration(migration), 150, 1*time.Second).Should(BeInPhase(v1.MigrationFailed))

fossedihelm · 2024-03-12T14:35:22Z

tests/migration/postcopy.go

+func beRestarted(oldUID types.UID) gomegatypes.GomegaMatcher {
+	return gstruct.PointTo(gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
+		"ObjectMeta": gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
+			"UID": Not(Equal(oldUID)),
+		}),
+		"Status": gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
+			"Phase": Equal(v1.Running),
+		}),
+	}))
+}


This function is great and it's the second time I see it. Maybe we should start thinking of creating a new file under matcher pkg.

Yes, it looks like there are many functions that need to be separated into the matcher. I prefer to do it separately in a follow up.
For now, I just made the beRestarted public.

Yes sure! I just to mention it to share the issue :)

fossedihelm · 2024-03-12T14:37:23Z

tests/migration/postcopy.go

+func confirmMigrationMode(virtClient kubecli.KubevirtClient, vmi *v1.VirtualMachineInstance, expectedMode v1.MigrationMode) {
+	By("Retrieving the VMI post migration")
+	vmi, err := virtClient.VirtualMachineInstance(vmi.Namespace).Get(context.Background(), vmi.Name, &metav1.GetOptions{})
+	Expect(err).ToNot(HaveOccurred())
+
+	By("Verifying the VMI's migration mode")
+	Expect(vmi.Status.MigrationState.Mode).To(Equal(expectedMode))
+}


Can we move this under libmigration pkg and remove the duplicate in the swap_test? I know it's outside of this PR :(

fossedihelm

Thank you @vladikr for your patience in the review process.
Last things from my side and then we are ready to go :)

fossedihelm · 2024-03-13T07:10:14Z

tests/migration/postcopy.go

+						vmi := libvmi.NewFedora(libnet.WithMasqueradeNetworking()...)
+						vmi.Namespace = namespace
+						vmi.Spec.Domain.Resources.Requests[k8sv1.ResourceMemory] = resource.MustParse("3Gi")
+						vmi.Spec.Domain.Devices.Rng = &v1.Rng{}


Probably we could save some time using Alpine or Cirros instead of Fedora. Here we don't need stress-ng :)
And in general we can drop the namespace parameter since we are creating the VM using directly:
virtClient.VirtualMachine(testsuite.NamespacePrivileged).
Last, we can also use use libvmi Options:

Suggested change

vmi := libvmi.NewFedora(libnet.WithMasqueradeNetworking()...)

vmi.Namespace = namespace

vmi.Spec.Domain.Resources.Requests[k8sv1.ResourceMemory] = resource.MustParse("3Gi")

vmi.Spec.Domain.Devices.Rng = &v1.Rng{}

vmi := libvmi.NewFedora(

append(libnet.WithMasqueradeNetworking(),

libvmi.WithResourceMemory("2Gi"),

libvmi.WithRng())...,

)

@fossedihelm, we need stress-ng here.

to make sure there is enough time before the migration switches to post copy.

that the memory of that guest is already "polluted" so it will take time copy the rest of the memory to target after switch to post copy.

Right! Sorry, I missed the line! Sorry for the noise

fossedihelm · 2024-03-13T07:16:14Z

tests/migration/postcopy.go

+						// check VMI, confirm migration state
+						waitUntilMigrationMode(vmi, v1.MigrationPostCopy, 300)
+
+						// launch killer pod on every node that isn't the vmi's node


// leftorver
Actually we are doing the opposite :)

fossedihelm · 2024-03-13T07:24:09Z

tests/migration/postcopy.go

+	}))
+}
+
+func VMIMigrationWithGuestaAgent(virtClient kubecli.KubevirtClient, pvName string, memoryRequestSize string, migrationPolicy *migrationsv1.MigrationPolicy) {


nit, typo:
VMIMigrationWithGuestAgent
IMO we should do something to this "test" shared between two files. But definitely outside of the scope of this PR.
So we should do it in a follow up :)

fossedihelm · 2024-03-13T07:27:18Z

tests/framework/matcher/status.go

+ *
+ */
+
+package matcher


fossedihelm · 2024-03-13T07:29:28Z

tests/framework/matcher/status.go

+func BeReady() gomegatypes.GomegaMatcher {
+	return gstruct.PointTo(gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
+		"Status": gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
+			"Ready": BeTrue(),
+		}),
+	}))
+}


There is another beReady definition in tests/hotplug/cpu.go if you want to unify everything :D

fossedihelm · 2024-03-13T16:40:51Z

@vladikr To me looks good!
Once the conflicts are solved I will be happy to lgtm.
Thank you for the effort and the patience

fossedihelm · 2024-03-15T11:04:35Z

It was a long journey! Thank you @vladikr 🙂
/lgtm

Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

vladikr · 2024-03-18T21:30:38Z

/unhold

fossedihelm · 2024-03-19T05:11:36Z

/lgtm
/retest

kubevirt-commenter-bot · 2024-03-19T05:11:41Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-1.27-vgpu
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.29-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-storage
/test pull-kubevirt-e2e-k8s-1.27-sig-compute
/test pull-kubevirt-e2e-k8s-1.27-sig-operator
/test pull-kubevirt-e2e-k8s-1.28-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-storage
/test pull-kubevirt-e2e-k8s-1.28-sig-compute
/test pull-kubevirt-e2e-k8s-1.28-sig-operator

kubevirt-commenter-bot · 2024-03-19T11:47:24Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-commenter-bot · 2024-03-19T14:47:25Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fossedihelm · 2024-03-19T14:57:41Z

@vladikr code-lint lane is complaining about adding stuff in tests/utils file :)
/hold

Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

fossedihelm · 2024-03-19T20:22:24Z

/lgtm

kubevirt-commenter-bot · 2024-03-19T20:22:29Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-1.27-vgpu
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.29-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-storage
/test pull-kubevirt-e2e-k8s-1.27-sig-compute
/test pull-kubevirt-e2e-k8s-1.27-sig-operator
/test pull-kubevirt-e2e-k8s-1.28-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-storage
/test pull-kubevirt-e2e-k8s-1.28-sig-compute
/test pull-kubevirt-e2e-k8s-1.28-sig-operator

fossedihelm · 2024-03-20T06:40:01Z

/retest
/unhold
code lint lane has been fixed

vladikr · 2024-03-20T13:37:22Z

/test pull-kubevirt-e2e-k8s-1.27-sig-storage

vladikr · 2024-03-26T19:27:01Z

/cherry-pick release-1.2

kubevirt-bot · 2024-03-26T19:27:39Z

@vladikr: #11479 failed to apply on top of branch "release-1.2":

Applying: fail VMIs when it reaches an irrecoverable state
Applying: test: vms are restarted after irrecoverable post-copy migration
Using index info to reconstruct a base tree...
M	tests/migration/BUILD.bazel
M	tests/migration/migration.go
M	tests/utils.go
Falling back to patching base and 3-way merge...
Auto-merging tests/utils.go
Auto-merging tests/migration/migration.go
CONFLICT (content): Merge conflict in tests/migration/migration.go
Auto-merging tests/migration/BUILD.bazel
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 test: vms are restarted after irrecoverable post-copy migration
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.2

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.

fossedihelm · 2024-03-29T12:31:23Z

/cherry-pick release-1.2

@vladikr Do you have the capacity to create a manual cherry-pick? Thank you!

With kubevirt/kubevirt#11479 we can now drop the warning about VMs reaching an inconsistent state. Signed-off-by: Dan Kenigsberg <danken@redhat.com>

kubevirt-bot requested review from kbidarkar and victortoso March 11, 2024 02:18

vladikr force-pushed the irrecoverable_postcopy branch 2 times, most recently from 9e2cbef to 9183acb Compare March 11, 2024 23:39

kubevirt-bot added size/XL and removed size/L labels Mar 11, 2024

vladikr force-pushed the irrecoverable_postcopy branch from 9183acb to 7373b10 Compare March 11, 2024 23:42

fossedihelm reviewed Mar 12, 2024

View reviewed changes

vladikr force-pushed the irrecoverable_postcopy branch 2 times, most recently from c3bc4ba to 5380f7a Compare March 12, 2024 14:15

fossedihelm reviewed Mar 12, 2024

View reviewed changes

vladikr force-pushed the irrecoverable_postcopy branch 2 times, most recently from 90dca2f to aceca1d Compare March 12, 2024 20:46

fossedihelm reviewed Mar 13, 2024

View reviewed changes

vladikr force-pushed the irrecoverable_postcopy branch from aceca1d to 359df8a Compare March 13, 2024 15:26

vladikr force-pushed the irrecoverable_postcopy branch 2 times, most recently from ea01f70 to 6208133 Compare March 13, 2024 22:35

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 14, 2024

vladikr force-pushed the irrecoverable_postcopy branch from 6208133 to de72a51 Compare March 14, 2024 16:58

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 14, 2024

vladikr force-pushed the irrecoverable_postcopy branch from de72a51 to dce4336 Compare March 15, 2024 10:55

kubevirt-bot assigned fossedihelm Mar 15, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 15, 2024

vladikr force-pushed the irrecoverable_postcopy branch from 4938a4d to 552f7fd Compare March 18, 2024 20:24

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 18, 2024

vladikr force-pushed the irrecoverable_postcopy branch from 552f7fd to c6872da Compare March 18, 2024 20:37

tests: move ConfirmMigrationMode to libmigration

832a708

Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

vladikr force-pushed the irrecoverable_postcopy branch from c6872da to 832a708 Compare March 18, 2024 21:06

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 18, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 19, 2024

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 19, 2024

replace StartVMAndExpectRunning with RunVMAndExpectLaunchWithRunStrategy

7a59336

Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

kubevirt-bot added sig/storage and removed lgtm Indicates that a PR is ready to be merged. labels Mar 19, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 19, 2024

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 20, 2024

kubevirt-bot merged commit a18b070 into kubevirt:main Mar 20, 2024
38 checks passed

dankenigsberg mentioned this pull request Apr 10, 2024

cluster-configuration.md: improved allowPostCopy kubevirt/hyperconverged-cluster-operator#2872

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fail VMIs in irrecoverable state after an interrupted postcopy migration #11479

Fail VMIs in irrecoverable state after an interrupted postcopy migration #11479

	* Copyright 2024 Red Hat, Inc.
	* Copyright The KubeVirt Authors.


		})

		Context("[Serial] should migrate using for postcopy", func() {

	It("should make sure that VM restarts after failur", func() {
	It("should make sure that VM restarts after failure", func() {

Fail VMIs in irrecoverable state after an interrupted postcopy migration #11479

Fail VMIs in irrecoverable state after an interrupted postcopy migration #11479

Conversation

What this PR does

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment