From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yolkfull Chow Subject: Re: [KVM-AUTOTEST PATCH] A test patch - Boot VMs until one of them becomes unresponsive Date: Wed, 10 Jun 2009 16:10:18 +0800 Message-ID: <4A2F6A6A.9020108@redhat.com> References: <309210296.1536501244540673873.JavaMail.root@zmail05.collab.prod.int.phx2.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: Uri Lublin , kvm@vger.kernel.org To: Michael Goldish Return-path: Received: from mx2.redhat.com ([66.187.237.31]:36064 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757222AbZFJIK3 (ORCPT ); Wed, 10 Jun 2009 04:10:29 -0400 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n5A8AVuE011173 for ; Wed, 10 Jun 2009 04:10:31 -0400 In-Reply-To: <309210296.1536501244540673873.JavaMail.root@zmail05.collab.prod.int.phx2.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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" > To:kvm@vger.kernel.org > Cc: "Uri Lublin" > 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. > > >