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.
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').
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.
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.
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.
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 =
13. Why are you catching OSError? If you get OSError it might be a framework bug.
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.
I know these are quite a few comments, but they're all rather minor and the test
is well written in my opinion.
Thanks,
Michael
----- Original Message -----
From: "Yolkfull Chow" <yzhou@xxxxxxxxxx>
To: kvm@xxxxxxxxxxxxxxx
Cc: "Uri Lublin" <uril@xxxxxxxxxx>
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.
--
Yolkfull
Regards,
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
