On 06/09/2009 05:44 PM, Michael Goldish wrote:
The test looks pretty nicely written. Comments:
1. Consider making all the cloned VMs use image snapshots:
curr_vm = vm1.clone()
curr_vm.get_params()["extra_params"] += " -snapshot"
I'm not sure it's a good idea to let all VMs use the same disk image.
Or maybe you shouldn't add -snapshot yourself, but rather do it in the config
file for the first VM, and then all cloned VMs will have -snapshot as well.
Yes I use 'image_snapshot = yes' in config file.
2. Consider changing the message
" Booting the %dth guest" % num
to
"Booting guest #%d" % num
(because there's no such thing as 2th and 3th)
3. Consider changing the message
"Cannot boot vm anylonger"
to
"Cannot create VM #%d" % num
4. Why not add curr_vm to vms immediately after cloning it?
That way you can kill it in the exception handler later, without having
to send it a 'quit' if you can't login ('if not curr_vm_session').
Yes, good idea.
5. " %dth guest boots up successfully" % num --> again, 2th and 3th make no
sense.
Also, I wonder why you add those spaces before every info message.
6. "%dth guest's session is not responsive" --> same
(maybe use "Guest session #%d is not responsive" % num)
7. "Shut down the %dth guest" --> same
(maybe "Shutting down guest #%d"? or destroying/killing?)
8. Shouldn't we fail the test when we find an unresponsive session?
It seems you just display an error message. You can simply replace
logging.error( with raise error.TestFail(.
9. Consider using a stricter test than just vm_session.is_responsive().
vm_session.is_responsive() just sends ENTER to the sessions and returns
True if it gets anything as a result (usually a prompt, or even just a
newline echoed back). If the session passes this test it is indeed
responsive, so it's a decent test, but maybe you can send some command
(user configurable?) and test for some output. I'm really not sure this
is important, because I can't imagine a session would respond to a newline
but not to other commands, but who knows. Maybe you can send the first VM
a user-specified command when the test begins, remember the output, and
then send all other VMs the same command and make sure the output is the
same.
maybe use 'info status' and send command 'help' via session to vms and
compare their output?
10. I'm not sure you should use the param "kill_vm_gracefully" because that's
a postprocessor param (probably not your business). You can just call
destroy() in the exception handler with gracefully=False, because if the VMs
are non- responsive, I don't expect them to shutdown nicely with an SSH
command (that's what gracefully does). Also, we're using -snapshot, so
there's no reason to shut them down nicely.
Yes, I agree. :)
11. "Total number booted successfully: %d" % (num - 1) --> why not just num?
We really have num VMs including the first one.
Or you can say: "Total number booted successfully in addition to the first one"
but that's much longer.
Since after the first guest booted, I set num = 1 and then 'num += 1'
at first in while loop ( for the purpose of getting a new vm ).
So curr_vm is vm2 ( num is 2) now. If the second vm failed to boot up,
the num booted successfully should be (num - 1).
I would use enumerate(vms) that Uri suggested to make number easier to
count.
12. Consider adding a 'max_vms' (or 'threshold') user param to the test. If
num reaches 'max_vms', we stop adding VMs and pass the test. Otherwise the
test will always fail (which is depressing). If params.get("threshold") is
None or "", or in short -- 'if not params.get("threshold")', disable this
feature and keep adding VMs forever. The user can enable the feature with:
max_vms = 50
or disable it with:
max_vms =
This is a good idea for hardware resource limit of host.
13. Why are you catching OSError? If you get OSError it might be a framework
bug.
Since sometimes, vm.create() successfully but failed to ssh-login since
the running python cannot allocate physical memory (OSError).
Add max_vms could fix this problem I think.
14. At the end of the exception handler you should proably re-raise the
exception
you caught. Otherwise the user won't see the error message. You can simply
replace
'break' with 'raise' (no parameters), and it should work, hopefully.
Yes I should if add a 'max_vms'.
I know these are quite a few comments, but they're all rather minor and the test
is well written in my opinion.
Thank you, I will do modification according to your and Uri's comments,
and will re-submit it here later. :)
Thanks and Best Regards,
Yolkfull
Thanks,
Michael
----- Original Message -----
From: "Yolkfull Chow"<yz...@redhat.com>
To:kvm@vger.kernel.org
Cc: "Uri Lublin"<u...@redhat.com>
Sent: Tuesday, June 9, 2009 11:41:54 AM (GMT+0200) Auto-Detected
Subject: [KVM-AUTOTEST PATCH] A test patch - Boot VMs until one of them becomes
unresponsive
Hi,
This test will boot VMs until one of them becomes unresponsive, and
records the maximum number of VMs successfully started.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
