On 11/30/2011 12:24 PM, Chris Evich wrote:
> Change login key name from _duration to _timeout for clarity, since other
> variables named _duration are used differently. There was a race condition on
> end_time where test could fail if final savevm_delay plus testing time, took
> longer than pre-set end_time. Modify test to loop through a fixed number of
> save/restore cycles instead of testing for a fixed time period. The duration
> of each cycle is compared to the second cycle. Any lengthening of cycle
> duration beyond a threshold causes test to fail.
Ok, some comments, already went through most of them on irc:
1) The original purpose of the patch is to stop/save/load the vm many
times *while* the same is booting, so the original logic of the test is
right
2) Perhaps we could make some timeouts smaller so more start/stop
cycles could happen
3) I'd be fine with a fixed number of cycles, if we move the vm login
outside the loop. However, I think a variable number of cycles is smarter.
4) I'm looking at the original code and I think there's no possible
way that the mentioned race condition could happen, as the termination
condition for while loops is check on the beginning of the loop, not at
the end.
If we want to be safer, just use as the fail criteria whether we could
successfuly log into the VM or not, what can be accomplished with this
patch:
diff --git a/client/tests/kvm/tests/boot_savevm.py
b/client/tests/kvm/tests/boot_savevm.py
index 91199fb..d02463d 100644
--- a/client/tests/kvm/tests/boot_savevm.py
+++ b/client/tests/kvm/tests/boot_savevm.py
@@ -21,6 +21,7 @@ def run_boot_savevm(test, params, env):
savevm_login_delay = float(params.get("savevm_login_delay"))
end_time = time.time() + float(params.get("savevm_timeout"))
+ successful_login = False
while time.time() < end_time:
time.sleep(savevm_delay)
try:
@@ -46,10 +47,11 @@ def run_boot_savevm(test, params, env):
try:
vm.wait_for_login(timeout=savevm_login_delay)
+ successful_login = True
break
except Exception, detail:
logging.debug(detail)
- if (time.time() > end_time):
+ if not successful_login:
raise error.TestFail("Not possible to log onto the vm after %s
s" %
params.get("savevm_timeout"))
So I'd rather go with this approach. Let me know what you think about it.
> Chris Evich (6):
> kvm boot_savevm test: boot_savevm: Change login key name from _duration to
> _timeout for clarity
> kvm boot_savevm test: Test time differences instead of total time
> kvm boot_savevm test: Add test-duration floor check to catch breakage
> early
> kvm boot_savevm test: Fix bug from switch to for loop
> kvm boot_savevm test: Added some debugging output
> kvm boot_savevm test: Skip first cycle to avoid virgin cache effects.
>
> client/tests/kvm/subtests.cfg.sample | 13 ++++++++-
> client/tests/kvm/tests/boot_savevm.py | 43
> +++++++++++++++++++++++++-------
> 2 files changed, 44 insertions(+), 12 deletions(-)
>
> _______________________________________________
> Autotest mailing list
> [email protected]
> http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
_______________________________________________
Autotest mailing list
[email protected]
http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
