live migration of windows 2012 r2 instance with virtio balloon driver fails from mitaka to queens.

Hi, the same doesn't seem to trigger for Xenial->Bionic which should be what matches Mitaka-Queens right?
Did you find a way to reproduce it on Xenial->Bionic as well to justify the change to (non UCA) Xenial users?

The Error you reported sounded familiar, for just the error you report there was a discussion pointing to different fixes:
=> https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg00141.html

These changes only got into Ubuntu >=Bionic and would also be missing.
I wonder if these two changes would fix your issue as well (less code change, less chance for regression) or if they should be added on top of yours (more thorough fix of the area).
Thea are part of the upstream 2.6.1, so they were in since yakkety - if you use those of 2.6.1 they are quite likely to apply.

The further reasons this a bit sounded known to me are:
https://bugs.launchpad.net/qemu/+bug/1838569
https://bugs.launchpad.net/cloud-archive/+bug/1848497
https://bugs.launchpad.net/cloud-archive/+bug/1882416
They ended up fixing a similar baloon migration issue around "Bad config data" but the "VQ 2 size 0x80" you address here was mentioned there as well.

These also illustrate some danger of these changes, that they might immediately work, but later on break.
Could you please extend your tests that are Mitaka->Queens migrations to become Mitaka->Queens->Ussuri migrations and ensure those are not affected in any bad way by re-breaking on the next step due to this?

Hi.

I tested yakkety(2.6.1) -> yakkty -> queens(2.11) and it worked.

so I did bisect then found those commits.

But I didn't test xenial->bionic,

but I'll update case after testing them as well.

Xenial -> Bionic
Mitaka -> Queens -> Ussuri

ah I missed one thing is that

I was able to reproduce this issue with xenial (non UCA, 2.5 ) to queens-staging. I'll test above case

Quick test with non UCA X -> X -> B, I can reproduce this the same way as description and patch fixed issue.

I'm going to test further

Test M M Q U

M M Q is ok

but Q -> U has issue below.

error: internal error: qemu unexpectedly closed the monitor: qemu-system-x86_64: -realtime mlock=off: warning: '-realtime mlock=...' is deprecated, please use '-overcommit mem-lock=...' instead
2020-09-09T00:56:27.768574Z qemu-system-x86_64: can't apply global SandyBridge-IBRS-x86_64-cpu.osxsave=off: Property '.osxsave' not found

With M-M-Q-U, and I faced issue described above, instance in Q crashed without special log in qemu
only found msg is below from syslog
Sep 9 04:50:53 colt libvirtd[1311]: 2020-09-09 04:50:53.224+0000: 1311: error : qemuMonitorIO:719 : internal error: End of file from qemu monitor

I tried to find trigger about this but no luck yet.
I tested M-M-Q and waited but no issue was found yet.

On Wed, Sep 9, 2020 at 3:20 AM Seyeong Kim <email address hidden> wrote:
...
> but Q -> U has issue below.
>
> error: internal error: qemu unexpectedly closed the monitor: qemu-system-x86_64: -realtime mlock=off: warning: '-realtime mlock=...' is deprecated, please use '-overcommit mem-lock=...' instead
> 2020-09-09T00:56:27.768574Z qemu-system-x86_64: can't apply global SandyBridge-IBRS-x86_64-cpu.osxsave=off: Property '.osxsave' not found

It seems you found a new combination to trigger bug 1825195 :-/
We fixed it where we could covering the Ubuntu releases.

The fixes for that are upstream since >=libvirt-5.4 and I think ussuri
should have that - right?
Could you check the version of libvirt on the U system and the
commandline it started qemu with?
It seems it added an osxsave attribute there which it shouldn't.

On Wed, Sep 9, 2020 at 7:05 AM Seyeong Kim <email address hidden> wrote:

> Sep 9 04:50:53 colt libvirtd[1311]: 2020-09-09 04:50:53.224+0000: 1311: error : qemuMonitorIO:719 : internal error: End of file from qemu monitor

This just means qemu went away and is not concerning on its own.

Gladly for the actual case here this isn't related to your
virtio-balloon changes ...

hey paelzer,

ah sorry

libvirt on Bionic was 4.0.0

I installed them libvirt-bin but I missed it is changed to libvirt-daemon-system

I re-installed libvirt-daemon-system and it is 6.0.0 now,

then M -> M -> Q -> U migration is working fine.

Thanks for the testing already.
Glad we could rule out the other issue you found.

In comment #7 you said "UCA X -> X -> B" triggers it, but why don't I see it with "X -> B" usually?
Does it trigger just for "X -> B" for you as well?
Or is there any config needed in the guest to trigger (balloon is default)?
Maybe attach the guest XML of the guest you are trying to migrate.

usually X->X->B is reproducer here. X->B is working fine basically.

Windows guest should have virtio balloon driver.

and I think below setting is needed ( as the customer's xml has it )

virsh dommemstat --domain win2k12r2 --period 10 --config

and the customer said that
windows 2012r2, 2016 has issue but 2019 ( from their image ) is fine.
but we don't know what setting is there actually inside those images

Great, thanks!
That makes sense as the guest driver is part of the equation here.

So if you are willing to also verify this then (with the guests that
trigger the issue on X->X->B) I think there is nothing against this
anymore.
Do you want me to re-eval in depth and sponsor?

On Wed, Sep 9, 2020 at 1:11 PM Seyeong Kim <email address hidden> wrote:
>
> and the customer said that
> windows 2012r2, 2016 has issue but 2019 ( from their image ) is fine.
> but we don't know what setting is there actually inside those images
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1894772
>
> Title:
> live migration of windows 2012 r2 instance with virtio balloon driver
> fails from mitaka to queens.
>
> Status in qemu package in Ubuntu:
> Fix Released
> Status in qemu source package in Xenial:
> In Progress
>
> Bug description:
> [Impact]
>
> livemigration of windows 2012 r2 instance with virtio balloon driver
> from qemu 2.5(mitaka) to qemu 2.11(queens) is not working properly.
>
> Especially instance keep moving e.g 2.5 -> 2.5 -> 2.11
>
> Then It shows below msg from the 2nd mitaka node.
>
> Migration: [ 94 %]error: internal error: qemu unexpectedly closed the monitor: 2020-09-07T07:45:11.799345Z qemu-system-x86_64: warning: Unknown firmware file in legacy mode: etc/msr_feature_control
> 2020-09-07T07:45:12.765618Z qemu-system-x86_64: VQ 2 size 0x80 < last_avail_idx 0x1 - used_idx 0x2
> 2020-09-07T07:45:12.765642Z qemu-system-x86_64: Failed to load virtio-balloon:virtio
> 2020-09-07T07:45:12.765648Z qemu-system-x86_64: error while loading state for instance 0x0 of device '0000:00:07.0/virtio-balloon'
> 2020-09-07T07:45:12.766483Z qemu-system-x86_64: load of migration failed: Operation not permitted
>
> [Test Case]
>
> Deploy 2 mitaka-staging machines kvm host
> Deploy 1 queens-staging machines kvm host
>
> Setting NFS server and client between them.
>
> Deploy windows 2012r2 guest instance with virtio balloon driver on one
> of the mitaka host
>
> Migrate it from mitaka to mitaka (it should be ok )
> Migrate it from mitaka to queens ( it raises error )
>
> I can reproduce this issue with baremetal or vm host
>
> [Regressions]
> As this patch is qemu related, current instance should be restarted to have this fix.
> Also, this patch may cause failure of vm starting, migrating related to virtio drivers.
> Especially Windows guest vm.
>
> [Others]
>
> I bisected this issue and found one commit below, and the others are
> needed for this.
>
> ####
> From 4eae2a657d1ff5ada56eb9b4966eae0eff333b0b Mon Sep 17 00:00:00 2001
> From: Ladi Prosek <email address hidden>
> Date: Tue, 1 Mar 2016 12:14:03 +0100
> Subject: [PATCH] balloon: fix segfault and harden the stats queue
>
> The segfault here is triggered by the driver notifying the stats queue
> twice after adding a buffer to it. This effectively resets stats_vq_elem
> back to NULL and QEMU crashes on the next stats timer tick in
> balloon_stats_poll_cb.
>
> This is a regression introduced in 51b19ebe4320f3dc, although admittedly
> the device assumed ...

Hey Christian

Unfortunately, I found an issue between below steps

X(not patched) -> X(patched) -> Q - is working fine

X(not patched) -> X(not patched) -> Q(error)
                                 -> X(patched) -> Q - has the same error as original

The customer is in the last situation. so I need to find a fix for this as well.

I think handling VQ inuse has issue between different versions.

I'll let you know if I need help from you.

Thanks a lot.

I think below commit is included inside 2.5 qemu in Xenial but not in 2.11

and I tested it with upstream commit build with migration. but I haven't tested it yet

I'm going to test them with ubuntu releases as well.

If it is correct, we need patch > queens instead of mitaka

Description: make sure vdev->vq[i].inuse never goes below 0
 This is a work-around to fix live migrations after the patches for
 CVE-2016-5403 were applied. The true root cause still needs to be
 determined.
Origin: based on a patch by Len <email address hidden>
Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1647389

Index: qemu-2.5+dfsg/hw/virtio/virtio.c
===================================================================
--- qemu-2.5+dfsg.orig/hw/virtio/virtio.c 2017-04-05 09:48:17.420025137 -0400
+++ qemu-2.5+dfsg/hw/virtio/virtio.c 2017-04-05 09:49:59.565337543 -0400
@@ -1510,6 +1510,7 @@
     for (i = 0; i < num; i++) {
         if (vdev->vq[i].vring.desc) {
             uint16_t nheads;
+ int inuse_tmp;
             nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
             /* Check it isn't doing strange things with descriptor numbers. */
             if (nheads > vdev->vq[i].vring.num) {
@@ -1527,12 +1528,15 @@
              * Since max ring size < UINT16_MAX it's safe to use modulo
              * UINT16_MAX + 1 subtraction.
              */
- vdev->vq[i].inuse = (uint16_t)(vdev->vq[i].last_avail_idx -
+ inuse_tmp = (int)(vdev->vq[i].last_avail_idx -
                                 vring_used_idx(&vdev->vq[i]));
+
+ vdev->vq[i].inuse = (inuse_tmp < 0 ? 0 : inuse_tmp);
+
             if (vdev->vq[i].inuse > vdev->vq[i].vring.num) {
- error_report("VQ %d size 0x%x < last_avail_idx 0x%x - "
+ error_report("VQ %d inuse %u size 0x%x < last_avail_idx 0x%x - "
                              "used_idx 0x%x",
- i, vdev->vq[i].vring.num,
+ i, vdev->vq[i].inuse, vdev->vq[i].vring.num,
                              vdev->vq[i].last_avail_idx,
                              vring_used_idx(&vdev->vq[i]));
                 return -1;

I've tested above patch on queens qemu. and it works fine.

I don't have to fix Mitaka anymore.

if this patch is viable, we need to put this to every release since Queens. if not, migration from patched one to unpatched one will not work as I saw between Mitaka and Queens.

FYI I think this bug is the same as 1647389

In that LP,

Dave also mentioned commit 4eae2a657d1ff5ada56eb9b4966eae0eff333b0b is needed
but it is too large and changes a lot.

workaround https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1647389/comments/15
didnt work.

I worried that if we patch this to over Q,
I assume that live migration current running instances from current Q to patched Q

so I've tested some scenario

###########################
Scenario 1
###########################
Mitaka < -> Mitaka ( 2.5 )
->
Queens ( patched, 2.11 )
->
Focal : FAILED
###########################

###########################
Scenario 2
###########################
Mitaka < -> Mitaka ( 2.5 )
->
Queens ( patched, 2.11 )
->
Bionic : FAILED
###########################

###########################
Scenario 3
###########################
Bionic -> Bionic( patched, 2.11 )
###########################
Bionic <-> Bionic
->
Bionic( patched ) OK
###########################

Thanks for uncovering just another bug on the case (it is four now, so we can already see that it is complex and comes back in different aspects).

From looking at the debdiffs a few formalities:
- origin, please point to some place you go this from "based on a patch by Len <email address hidden>" is nothing you can find, the comment might be useful but should be accompanied by an URL

In general it seems this shall become a patch we forever apply on top (not upstream, needs to go to all releases now and in future), that kind of change almost always is wrong.

If that would be the right solution then upstream would apply it. We have seen too many cases where such a kind of forever-carry-on-delta just makes us worse and worse. Now it might be bad in Xenial, but due to that we can "infect" more and more with a bad behavior. E.g. when an upstream change makes this subtly fail even worse and we only realize later one.

Since the root cause seems to be like "... virtqueue got into an inconsistent state in 2.5.0 and it carried over ..." (from https://lists.gnu.org/archive/html/qemu-devel/2016-11/msg02830.html) I wonder if we have to make a hard cut at some point instead of "carry delta forever". More like "fix it in the old release and require a guest restart before migration".

If you think applying that change to all releases is indeed the right solution drive the upstream discussion and once applied there we can backport SRU it to the existing releases. That also will provide a good "origin" tag then as we can point at the commit in qemu git and drop it once we merge a version with it.

###########################
Scenario 4
###########################
Focal <-> Focal
->
Focal( patched ) OK
###########################
and
-> Focal -> Focal(patched) OK
###########################

Since this has been dormant since my soft-reject I think we need to mark it accordingly to be fair on expectations for this case.

The Groovy Gorilla has reached end of life, so this bug will not be fixed for that release