Re: [KVM-AUTOTEST PATCH] A test patch - Boot VMs until one of them becomes unresponsive

[Yolkfull Chow <yzhou@redhat.com>]

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"<yzhou@redhat.com>
> To:kvm@vger.kernel.org
> Cc: "Uri Lublin"<uril@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.
>
>
>