Re: [Qemu-devel] [PATCH v3 2/2] tests/qemu-iotests: adding savevm/loadvm with postcopy flag test

[Max Reitz]

On 2017-12-04 16:01, Daniel Henrique Barboza wrote:
> 
> 
> On 12/04/2017 12:50 PM, Max Reitz wrote:
>> On 2017-12-03 21:13, Daniel Henrique Barboza wrote:
>>> Hi Max,
>>>
>>> On 12/01/2017 06:13 PM, Max Reitz wrote:
>>>> On 2017-11-16 23:35, Daniel Henrique Barboza wrote:
>>>>> This patch implements a test case for the scenario that was failing
>>>>> prior to the patch "migration/ram.c: do not set 'postcopy_running' in
>>>>> POSTCOPY_INCOMING_END".
>>>>>
>>>>> This new test file 198 was derived from the test file 181 authored
>>>>> by Kevin Wolf.
>>>>>
>>>>> CC: Kevin Wolf <address@hidden>
>>>>> CC: Max Reitz <address@hidden>
>>>>> CC: Cleber Rosa <address@hidden>
>>>>> Signed-off-by: Daniel Henrique Barboza <address@hidden>
>>>>>
>>>>> ---
>>>>> I CCed Cleber Rosa because this patch was developed at the same
>>>>> time his patch series "[PATCH 00/10] I/O tests cleanups" hit the
>>>>> mailing list. Most of the cleanups/fixes he made in the series was
>>>>> done in this new test as well.
>>>>>
>>>>>   tests/qemu-iotests/198     | 110
>>>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>>>   tests/qemu-iotests/198.out |  20 +++++++++
>>>>>   tests/qemu-iotests/group   |   1 +
>>>>>   3 files changed, 131 insertions(+)
>>>>>   create mode 100755 tests/qemu-iotests/198
>>>>>   create mode 100644 tests/qemu-iotests/198.out
>>>> Looks good overall, just two nitpicks below.
>>>>
>>>>> diff --git a/tests/qemu-iotests/198 b/tests/qemu-iotests/198
>>>>> new file mode 100755
>>>>> index 0000000000..786e37fc95
>>>>> --- /dev/null
>>>>> +++ b/tests/qemu-iotests/198
>>>>> @@ -0,0 +1,110 @@
>>>>> +#!/bin/bash
>>>>> +#
>>>>> +# Test savevm and loadvm after live migration with postcopy flag
>>>>> +#
>>>>> +# Copyright (C) 2017, IBM Corporation.
>>>>> +#
>>>>> +# This file is derived from tests/qemu-iotests/181 by Kevin Wolf
>>>>> +#
>>>>> +# This program is free software; you can redistribute it and/or
>>>>> modify
>>>>> +# it under the terms of the GNU General Public License as
>>>>> published by
>>>>> +# the Free Software Foundation; either version 2 of the License, or
>>>>> +# (at your option) any later version.
>>>>> +#
>>>>> +# This program is distributed in the hope that it will be useful,
>>>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>>> +# GNU General Public License for more details.
>>>>> +#
>>>>> +# You should have received a copy of the GNU General Public License
>>>>> +# along with this program.  If not, see
>>>>> <http://www.gnu.org/licenses/>.
>>>>> +#
>>>>> +# Creator/Owner : address@hidden
>>>>> +
>>>>> +seq=`basename $0`
>>>>> +echo "QA output created by $seq"
>>>>> +
>>>>> +status=1    # failure is the default!
>>>>> +
>>>>> +MIG_SOCKET="${TEST_DIR}/migrate"
>>>>> +
>>>>> +# get standard environment, filters and checks
>>>>> +. ./common.rc
>>>>> +. ./common.filter
>>>>> +. ./common.qemu
>>>>> +
>>>>> +_cleanup()
>>>>> +{
>>>>> +    rm -f "${MIG_SOCKET}"
>>>>> +    _cleanup_test_img
>>>>> +    _cleanup_qemu
>>>>> +}
>>>>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>>>>> +
>>>>> +_supported_fmt qcow2
>>>>> +_supported_proto generic
>>>>> +_supported_os Linux
>>>>> +
>>>>> +size=64M
>>>>> +_make_test_img $size
>>>>> +
>>>>> +echo
>>>>> +echo === Starting VMs ===
>>>>> +echo
>>>>> +
>>>>> +qemu_comm_method="monitor"
>>>>> +
>>>>> +if [ "$IMGOPTSSYNTAX" = "true" ]; then
>>>>> +    _launch_qemu \
>>>>> +        -drive "${TEST_IMG}",cache=${CACHEMODE},id=disk
>>>>> +else
>>>>> +    _launch_qemu \
>>>>> +        -drive
>>>>> file="${TEST_IMG}",cache=${CACHEMODE},driver=$IMGFMT,id=disk
>>>>> +fi
>>>>> +src=$QEMU_HANDLE
>>>>> +
>>>>> +if [ "$IMGOPTSSYNTAX" = "true" ]; then
>>>>> +    _launch_qemu \
>>>>> +        -drive "${TEST_IMG}",cache=${CACHEMODE},id=disk \
>>>>> +        -incoming "unix:${MIG_SOCKET}"
>>>>> +else
>>>>> +    _launch_qemu \
>>>>> +        -drive
>>>>> file="${TEST_IMG}",cache=${CACHEMODE},driver=$IMGFMT,id=disk \
>>>>> +        -incoming "unix:${MIG_SOCKET}"
>>>>> +fi
>>>>> +dest=$QEMU_HANDLE
>>>>> +
>>>>> +echo
>>>>> +echo === Set \'migrate_set_capability postcopy-ram on\' and
>>>>> migrate ===
>>>>> +echo
>>>>> +
>>>>> +silent=yes
>>>>> +_send_qemu_cmd $dest 'migrate_set_capability postcopy-ram on'
>>>>> "(qemu)"
>>>>> +_send_qemu_cmd $src 'migrate_set_capability postcopy-ram on' "(qemu)"
>>>>> +_send_qemu_cmd $src "migrate -d unix:${MIG_SOCKET}" "(qemu)"
>>>>> +
>>>>> +QEMU_COMM_TIMEOUT=1 qemu_cmd_repeat=10 silent=yes \
>>>>> +    _send_qemu_cmd $src "info migrate" "completed\|failed"
>>>>> +silent=yes _send_qemu_cmd $src "" "(qemu)"
>>>> Would it make sense to query the migration status non-silently here,
>>>> too, so that the test output would verify that it has actually
>>>> succeeded?
>>>>
>>>> (I suppose the savevm/loadvm coming up next would fail if the migration
>>>> failed, but maybe if it's easy enough...)
>>> Not exactly non-silently, but we can do something like this:
>>>
>>>
>>> echo
>>> echo === Check if migration was successful ===
>>> echo
>>>
>>> QEMU_COMM_TIMEOUT=1 silent=yes \
>>>      _send_qemu_cmd $src "info migrate" "completed"
>>> silent=yes _send_qemu_cmd $src "" "(qemu)"
>>>
>>>
>>> If migration failed, the test will fail with timeout error waiting for
>>> "completed"
>>> prior to savevm. What do you think?
>> Sounds good.  Shouldn't you keep the qemu_cmd_repeat=10, though?
> 
> I don't think it's needed. The command prior to it was:
> 
> +QEMU_COMM_TIMEOUT=1 qemu_cmd_repeat=10 silent=yes \
> +    _send_qemu_cmd $src "info migrate" "completed\|failed"
> +silent=yes _send_qemu_cmd $src "" "(qemu)"
> 
> 
> So it's guaranteed that the next 'info migrate' will contain the string
> "completed" or "failed".

Ah, right.  I thought you'd simply replace that by the other.  But
either way works for me.

Max

signature.asc
Description: OpenPGP digital signature