Re: [Autotest] [KVM-AUTOTEST PATCH 3/7] KVM test: new test timedrift_with_migration

[Dor Laor <dlaor@redhat.com>]

On 10/12/2009 05:28 PM, Lucas Meneghel Rodrigues wrote:
> Hi Michael, I am reviewing your patchset and have just a minor remark
> to make here:
>
> On Wed, Oct 7, 2009 at 2:54 PM, Michael Goldish<mgoldish@redhat.com>  wrote:
>> This patch adds a new test that checks the timedrift introduced by migrations.
>> It uses the same parameters used by the timedrift test to get the guest time.
>> In addition, the number of migrations the test performs is controlled by the
>> parameter 'migration_iterations'.
>>
>> Signed-off-by: Michael Goldish<mgoldish@redhat.com>
>> ---
>>   client/tests/kvm/kvm_tests.cfg.sample              |   33 ++++---
>>   client/tests/kvm/tests/timedrift_with_migration.py |   95 ++++++++++++++++++++
>>   2 files changed, 115 insertions(+), 13 deletions(-)
>>   create mode 100644 client/tests/kvm/tests/timedrift_with_migration.py
>>
>> diff --git a/client/tests/kvm/kvm_tests.cfg.sample b/client/tests/kvm/kvm_tests.cfg.sample
>> index 540d0a2..618c21e 100644
>> --- a/client/tests/kvm/kvm_tests.cfg.sample
>> +++ b/client/tests/kvm/kvm_tests.cfg.sample
>> @@ -100,19 +100,26 @@ variants:
>>          type = linux_s3
>>
>>      - timedrift:    install setup
>> -        type = timedrift
>>          extra_params += " -rtc-td-hack"
>> -        # Pin the VM and host load to CPU #0
>> -        cpu_mask = 0x1
>> -        # Set the load and rest durations
>> -        load_duration = 20
>> -        rest_duration = 20
>> -        # Fail if the drift after load is higher than 50%
>> -        drift_threshold = 50
>> -        # Fail if the drift after the rest period is higher than 10%
>> -        drift_threshold_after_rest = 10
>> -        # For now, make sure this test is executed alone
>> -        used_cpus = 100
>> +        variants:
>> +            - with_load:
>> +                type = timedrift
>> +                # Pin the VM and host load to CPU #0
>> +                cpu_mask = 0x1


Let's use -smp 2 always.

btw: we need not to parallel the load test with standard tests.

>> +                # Set the load and rest durations
>> +                load_duration = 20
>> +                rest_duration = 20
>
> Even the default duration here seems way too brief here, is there any
> reason why 20s was chosen instead of, let's say, 1800s? I am under the
> impression that 20s of load won't be enough to cause any noticeable
> drift...
>
>> +                # Fail if the drift after load is higher than 50%
>> +                drift_threshold = 50
>> +                # Fail if the drift after the rest period is higher than 10%
>> +                drift_threshold_after_rest = 10
>
> I am also curious about those tresholds and the reasoning behind them.
> Is there any official agreement on what we consider to be an
> unreasonable drift?
>
> Another thing that struck me out is drift calculation: On the original
> timedrift test, the guest drift is normalized against the host drift:
>
> drift = 100.0 * (host_delta - guest_delta) / host_delta
>
> While in the new drift tests, we consider only the guest drift. I
> believe is better to normalize all tests based on one drift
> calculation criteria, and those values should be reviewed, and at
> least a certain level of agreement on our development community should
> be reached.

I think we don't need to calculate drift ratio. We should define a 
threshold in seconds, let's say 2 seconds. Beyond that, there should not 
be any drift.

Do we support migration to a different host? We should, especially in 
this test too. The destination host reading should also be used.

Apart for that, good patchset, and good thing you refactored some of the 
code to shared utils.

>
> Other than this concern that came to my mind, the new tests look good
> and work fine here. I had to do a slight rebase in one of the patches,
> very minor stuff. The default values and the drift calculation can be
> changed on a later time. Thanks!
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html