HEAD is failing virt-test on migration tests
diff mbox

Message ID 54DE89D6.7060404@suse.de
State New
Headers show

Commit Message

Alexander Graf Feb. 13, 2015, 11:33 p.m. UTC
On 13.02.15 12:23, Lucas Meneghel Rodrigues wrote:
> 
> 
> On Fri, Feb 13, 2015 at 9:18 AM, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>> On 13.02.15 10:04, Dr. David Alan Gilbert wrote:
>>>  * Alexander Graf (agraf@suse.de) wrote:
>>>>
>>>>
>>>>  On 13.02.15 01:29, Lucas Meneghel Rodrigues wrote:
>>>>>  Copying Alex.
>>>>>
>>>>>  OK, after bisecting, this is what I've got:
>>>>>
>>>>>  8118f0950fc77cce7873002a5021172dd6e040b5 is the first bad commit
>>>>>  commit 8118f0950fc77cce7873002a5021172dd6e040b5
>>>>>  Author: Alexander Graf <agraf@suse.de <mailto:agraf@suse.de>>
>>>>>  Date:   Thu Jan 22 15:01:39 2015 +0100
>>>>>
>>>>>      migration: Append JSON description of migration stream
>>>>>
>>>>>      One of the annoyances of the current migration format is the
>>>>> fact that
>>>>>      it's not self-describing. In fact, it's not properly
>>>>> describing at all.
>>>>>      Some code randomly scattered throughout QEMU elaborates
>>>>> roughly how to
>>>>>      read and write a stream of bytes.
>>>>>
>>>>>      We discussed an idea during KVM Forum 2013 to add a JSON
>>>>> description of
>>>>>      the migration protocol itself to the migration stream. This patch
>>>>>      adds a section after the VM_END migration end marker that
>>>>> contains
>>>>>      description data on what the device sections of the stream are
>>>>>  composed of.
>>>>>
>>>>>      This approach is backwards compatible with any QEMU version
>>>>> reading the
>>>>>      stream, because QEMU just stops reading after the VM_END
>>>>> marker and
>>>>>  ignores
>>>>>      any data following it.
>>>>>
>>>>>      With an additional external program this allows us to decipher
>>>>> the
>>>>>      contents of any migration stream and hopefully make migration
>>>>> bugs
>>>>>  easier
>>>>>      to track down.
>>>>>
>>>>>      Signed-off-by: Alexander Graf <agraf@suse.de
>>>>> <mailto:agraf@suse.de>>
>>>>>      Signed-off-by: Amit Shah <amit.shah@redhat.com
>>>>>  <mailto:amit.shah@redhat.com>>
>>>>>      Signed-off-by: Juan Quintela <quintela@redhat.com
>>>>>  <mailto:quintela@redhat.com>>
>>>>>
>>>>>  :040000 040000 e9a8888ac242a61fbd05bbb0daa3e8877970e738
>>>>>  61df81f831bc86b29f65883523ea95abb36f1ec5 Mhw
>>>>>  :040000 040000 fe0659bed17d86c43657c26622d64fd44a1af037
>>>>>  7092a6b6515a3d0077f68ff2d80dbd74597a244f Minclude
>>>>>  :040000 040000 d90d6f1fe839abf21a45eaba5829d5a6a22abeb1
>>>>>  c2b1dcda197d96657458d699c185e39ae45f3c6c Mmigration
>>>>>  :100644 100644 98895fee81edfbc659fc42d467e930d06b1afa7d
>>>>>  80407662ad3ed860d33a9d35f5c44b1d19c4612b Msavevm.c
>>>>>  :040000 040000 cf218bc2b841cd51ebe3972635be2cfbb1de9dfa
>>>>>  7aaf3d10ef7f73413b228e854fe6f04317151e46 Mtests
>>>>>
>>>>>  So there you go. I'm going to sleep, if you need any extra help
>>>>> let me know.
>>>>
>>>>  So the major difference with this patch applied is that the sender
>>>> could
>>>>  send more data than the receive wants to read. I can't see the actual
>>>>  migrate command you used down there.
>>>>
>>>>  I haven't seen this actually being a problem so far, as the receiver
>>>>  just close()s its file descriptor once it hits VM_EOF. This should
>>>> only
>>>>  break senders if they expect they can send more. That said, I think I
>>>>  only tested offline migration (via exec:), so maybe QEMU is behaving
>>>>  badly and actually wants to send all data and just fails the migration
>>>>  without?
>>>
>>>  Hmm, for such an odd change to the migration stream it's a surprise you
>>>  didn't test it live.
>>
>> Well, let's say I don't remember explicitly testing it live - I probably
>> did at one point.
>>
>> I just verified that migrating with tcp:... works fine in master.
> 
> It is working fine with tcp migration in master indeed. The thing is,
> virt-test tests a bunch of variants, among them fd. fd is the only one
> failing from the list of things we do test (which also happen to be the
> virt-test default test set).

Can you please test whether the patch below makes things work for you again?


Alex

From ef6fde21007e62529799264f57a65c6bb3d0d414 Mon Sep 17 00:00:00 2001
From: Alexander Graf <agraf@suse.de>
Date: Sat, 14 Feb 2015 00:21:01 +0100
Subject: [PATCH] migration: Read JSON VM description on incoming migration

One of the really nice things about the VM description format is that it
goes
over the wire when live migration is happening. Unfortunately QEMU today
closes
any socket once it sees VM_EOF coming, so we never give the VMDESC the
chance to
actually land on the wire.

This patch makes QEMU read the description as well. This way we ensure that
anything wire tapping us in between will get the chance to also
interpret the
stream.

Along the way we also fix virt tests that assume that number_bytes_sent
on the
sender side is equal to number_bytes_read which was true before the VMDESC
patches and is true again with this patch.

Signed-off-by: Alexander Graf <agraf@suse.de>

     ret = 0;
@@ -1045,7 +1062,8 @@ out:
     }

     if (ret == 0) {
-        ret = qemu_file_get_error(f);
+        /* We may not have a VMDESC section, so ignore relative errors */
+        ret = file_error_after_eof;
     }

     return ret;

Comments

Dr. David Alan Gilbert Feb. 16, 2015, 6:57 p.m. UTC | #1
* Alexander Graf (agraf@suse.de) wrote:

<snip>

> Can you please test whether the patch below makes things work for you again?

The patch below fixes RDMA migration (same host); however, see comments.

> Alex
> 
> From ef6fde21007e62529799264f57a65c6bb3d0d414 Mon Sep 17 00:00:00 2001
> From: Alexander Graf <agraf@suse.de>
> Date: Sat, 14 Feb 2015 00:21:01 +0100
> Subject: [PATCH] migration: Read JSON VM description on incoming migration
> 
> One of the really nice things about the VM description format is that it
> goes
> over the wire when live migration is happening. Unfortunately QEMU today
> closes
> any socket once it sees VM_EOF coming, so we never give the VMDESC the
> chance to
> actually land on the wire.
> 
> This patch makes QEMU read the description as well. This way we ensure that
> anything wire tapping us in between will get the chance to also
> interpret the
> stream.
> 
> Along the way we also fix virt tests that assume that number_bytes_sent
> on the
> sender side is equal to number_bytes_read which was true before the VMDESC
> patches and is true again with this patch.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> diff --git a/savevm.c b/savevm.c
> index 8040766..ff4bead 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -929,6 +929,7 @@ int qemu_loadvm_state(QEMUFile *f)
>      uint8_t section_type;
>      unsigned int v;
>      int ret;
> +    int file_error_after_eof = -1;
> 
>      if (qemu_savevm_state_blocked(&local_err)) {
>          error_report("%s", error_get_pretty(local_err));
> @@ -1034,6 +1035,22 @@ int qemu_loadvm_state(QEMUFile *f)
>          }
>      }
> 
> +    file_error_after_eof = qemu_file_get_error(f);
> +
> +    /*
> +     * Try to read in the VMDESC section as well, so that dumping tools
> that
> +     * intercept our migration stream have the chance to see it.
> +     */
> +    if (qemu_get_byte(f) == QEMU_VM_VMDESCRIPTION) {

You could use qemu_peek_byte for that?

> +        uint32_t size = qemu_get_be32(f);
> +        uint8_t *buf = g_malloc(size);
> +
> +        if (buf) {
> +            qemu_get_buffer(f, buf, size);
> +            g_free(buf);
> +        }

This is slightly dangerous; a malformed file could send you a huge
value and get you to allocate lots of memory for no good reason.

You could do some clever; but personally I'd just loop around a
nice small buffer until it's gone.

As mentioned on IRC; I'm still worried though that this is only
a fix for loading on newer versions; migration to an older QEMU
with the same machine type would fail.
(Yes I know mythically that no one cares about this; but I do).

Dave

> +    }
> +
>      cpu_synchronize_all_post_init();
> 
>      ret = 0;
> @@ -1045,7 +1062,8 @@ out:
>      }
> 
>      if (ret == 0) {
> -        ret = qemu_file_get_error(f);
> +        /* We may not have a VMDESC section, so ignore relative errors */
> +        ret = file_error_after_eof;
>      }
> 
>      return ret;
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Alexander Graf Feb. 16, 2015, 8:24 p.m. UTC | #2
On 16.02.15 19:57, Dr. David Alan Gilbert wrote:
> * Alexander Graf (agraf@suse.de) wrote:
> 
> <snip>
> 
>> Can you please test whether the patch below makes things work for you again?
> 
> The patch below fixes RDMA migration (same host); however, see comments.
> 
>> Alex
>>
>> From ef6fde21007e62529799264f57a65c6bb3d0d414 Mon Sep 17 00:00:00 2001
>> From: Alexander Graf <agraf@suse.de>
>> Date: Sat, 14 Feb 2015 00:21:01 +0100
>> Subject: [PATCH] migration: Read JSON VM description on incoming migration
>>
>> One of the really nice things about the VM description format is that it
>> goes
>> over the wire when live migration is happening. Unfortunately QEMU today
>> closes
>> any socket once it sees VM_EOF coming, so we never give the VMDESC the
>> chance to
>> actually land on the wire.
>>
>> This patch makes QEMU read the description as well. This way we ensure that
>> anything wire tapping us in between will get the chance to also
>> interpret the
>> stream.
>>
>> Along the way we also fix virt tests that assume that number_bytes_sent
>> on the
>> sender side is equal to number_bytes_read which was true before the VMDESC
>> patches and is true again with this patch.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>
>> diff --git a/savevm.c b/savevm.c
>> index 8040766..ff4bead 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -929,6 +929,7 @@ int qemu_loadvm_state(QEMUFile *f)
>>      uint8_t section_type;
>>      unsigned int v;
>>      int ret;
>> +    int file_error_after_eof = -1;
>>
>>      if (qemu_savevm_state_blocked(&local_err)) {
>>          error_report("%s", error_get_pretty(local_err));
>> @@ -1034,6 +1035,22 @@ int qemu_loadvm_state(QEMUFile *f)
>>          }
>>      }
>>
>> +    file_error_after_eof = qemu_file_get_error(f);
>> +
>> +    /*
>> +     * Try to read in the VMDESC section as well, so that dumping tools
>> that
>> +     * intercept our migration stream have the chance to see it.
>> +     */
>> +    if (qemu_get_byte(f) == QEMU_VM_VMDESCRIPTION) {
> 
> You could use qemu_peek_byte for that?

It's what I had originally, but qemu_peek_byte() at the end of the day
is the exact same as qemu_get_byte, but doesn't increment the internal
buffer counter. So any error conditions that incur because the read
failed still happen with peek_byte and are a lot less intuitive.

> 
>> +        uint32_t size = qemu_get_be32(f);
>> +        uint8_t *buf = g_malloc(size);
>> +
>> +        if (buf) {
>> +            qemu_get_buffer(f, buf, size);
>> +            g_free(buf);
>> +        }
> 
> This is slightly dangerous; a malformed file could send you a huge
> value and get you to allocate lots of memory for no good reason.
> 
> You could do some clever; but personally I'd just loop around a
> nice small buffer until it's gone.

Good idea. Will change.

> As mentioned on IRC; I'm still worried though that this is only
> a fix for loading on newer versions; migration to an older QEMU
> with the same machine type would fail.
> (Yes I know mythically that no one cares about this; but I do).

Yeah, I guess I'll follow up with a fix to disable VMDESC submission on
older versions, just to be on the safe side.


Alex
Paolo Bonzini Feb. 16, 2015, 9:06 p.m. UTC | #3
On 16/02/2015 21:24, Alexander Graf wrote:
>> As mentioned on IRC; I'm still worried though that this is only
>> > a fix for loading on newer versions; migration to an older QEMU
>> > with the same machine type would fail.
>> > (Yes I know mythically that no one cares about this; but I do).
> Yeah, I guess I'll follow up with a fix to disable VMDESC submission on
> older versions, just to be on the safe side.

Can you make it a capability?

Paolo
Alexander Graf Feb. 16, 2015, 9:08 p.m. UTC | #4
On 16.02.15 22:06, Paolo Bonzini wrote:
> 
> 
> On 16/02/2015 21:24, Alexander Graf wrote:
>>> As mentioned on IRC; I'm still worried though that this is only
>>>> a fix for loading on newer versions; migration to an older QEMU
>>>> with the same machine type would fail.
>>>> (Yes I know mythically that no one cares about this; but I do).
>> Yeah, I guess I'll follow up with a fix to disable VMDESC submission on
>> older versions, just to be on the safe side.
> 
> Can you make it a capability?

When did live migration start to have capability negotiation? :)


Alex
Paolo Bonzini Feb. 16, 2015, 9:38 p.m. UTC | #5
On 16/02/2015 22:08, Alexander Graf wrote:
> > Can you make it a capability?
> When did live migration start to have capability negotiation? :)

Only capability without negotiation. :)

Negotiation is done above QEMU.

Paolo

Patch
diff mbox

diff --git a/savevm.c b/savevm.c
index 8040766..ff4bead 100644
--- a/savevm.c
+++ b/savevm.c
@@ -929,6 +929,7 @@  int qemu_loadvm_state(QEMUFile *f)
     uint8_t section_type;
     unsigned int v;
     int ret;
+    int file_error_after_eof = -1;

     if (qemu_savevm_state_blocked(&local_err)) {
         error_report("%s", error_get_pretty(local_err));
@@ -1034,6 +1035,22 @@  int qemu_loadvm_state(QEMUFile *f)
         }
     }

+    file_error_after_eof = qemu_file_get_error(f);
+
+    /*
+     * Try to read in the VMDESC section as well, so that dumping tools
that
+     * intercept our migration stream have the chance to see it.
+     */
+    if (qemu_get_byte(f) == QEMU_VM_VMDESCRIPTION) {
+        uint32_t size = qemu_get_be32(f);
+        uint8_t *buf = g_malloc(size);
+
+        if (buf) {
+            qemu_get_buffer(f, buf, size);
+            g_free(buf);
+        }
+    }
+
     cpu_synchronize_all_post_init();