Re: [PATCH] tests/migration: Add some slack to auto converge

[Peter Xu <peterx@redhat.com>]

On Mon, Feb 10, 2020 at 07:57:31PM +0000, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> There's an assert in autoconverge that checks that we quit the
> iteration when we go below the expected threshold.  Philippe
> saw a case where this assert fired with the measured value
> slightly over the threshold. (about 3k out of a few million).
> 
> I can think of two reasons:
>   a) Rounding errors

Yes, I feel like we're having approximate values here and there in
migration code. One simple example is that we calculate bandwidth
using sent bytes and timestamps, however our sent bytes should in most
cases increase with target page size only (4096 on x86), and then to
divide an ~100ms value which changes too from time to time, then
everything comes out of these inaccurate numbers could be inaccurate..

>   b) That after we make the decision to quit iteration we do one
>     more sync and that sees a few more dirty pages.

True..  I also didn't see how to guarantee the extra 1% will cover
this case either...

> 
> So add 1% slack to the assertion, that should cover a and
> most cases of b, probably all we'll see for the test.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

... But if what we see are always 3K or around then maybe we only need
to fixup that special value.  Assuming this is for sure going to
unbreak tests:

Reviewed-by: Peter Xu <peterx@redhat.com>

(PS: I would probably remove this check as a whole :)

> ---
>  tests/qtest/migration-test.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index cf27ebbc9d..a78ac0c7da 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -1237,7 +1237,8 @@ static void test_migrate_auto_converge(void)
>      g_assert_cmpint(percentage, <=, max_pct);
>  
>      remaining = read_ram_property_int(from, "remaining");
> -    g_assert_cmpint(remaining, <, expected_threshold);
> +    g_assert_cmpint(remaining, <,
> +                    (expected_threshold + expected_threshold / 100));
>  
>      migrate_continue(from, "pre-switchover");
>  
> -- 
> 2.24.1
> 

-- 
Peter Xu