On 06/10/2009 06:03 PM, Michael Goldish wrote:
----- "Yolkfull Chow"<yz...@redhat.com> wrote:
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?
I'm not sure I understand. What does 'info status' do? We're talking about
an SSH shell, not the monitor. You can do whatever you like, like 'uname -a',
and 'ls /', but you should leave it up to the user to decide, so he/she
can specify different commands for different guests. Linux commands won't
work under Windows, so Linux and Windows must have different commands in
the config file. In the Linux section, under '- @Linux:' you can add
something like:
stress_boot:
stress_boot_test_command = uname -a
and under '- @Windows:':
stress_boot:
stress_boot_test_command = ver&& vol
These commands are just naive suggestions. I'm sure someone can think of
much more informative commands.
That's really good suggestions. Thanks, Michael. And can I use
'migration_test_command' instead?
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.
OK, I didn't notice that.
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.
Do you remember exactly where OSError was thrown? Do you happen to have
a backtrace? (I just want to be very it's not a bug.)
The OSError was thrown when checking all VMs are responsive and I got
many traceback about "OSError: [Errno 12] Cannot allocate memory".
Maybe since when last VM was created successfully with lucky, whereas
python cannot get physical memory after that when checking all sessions.
So can we now catch the OSError and tell user the number of max_vms is
too large?
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 think you should re-raise anyway. Otherwise, what's the point in writing
error messages such as "raise error.TestFail("Cannot boot vm anylonger")"?
I you don't re-raise, the user won't see the messages.
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
--
Yolkfull
Regards,
--
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
