Hey, Thanks for keeping working on this and posting your work. It's normal for patches to go through a few rounds before getting accepted. Please don't take feedback in a negative way or get discourraged, we're just trying to help :)
I think it's really cool that you want to add svirt tests and all these parts that go along. I have put some comments inline below. I think as this patch grows, you may consider splitting it up into a collection of smaller patches. If you arrange the patches in order of dependencies, it also helps. This is all up on the wiki very recently at: https://github.com/autotest/autotest/wiki/SubmissionChecklist I was happy to see that pylint didn't complain at all about your patch, this is good. I'm sometimes guilty of sending patches before checking, so don't learn from me :D On 06/26/2012 01:11 PM, Onkar N Mahajan wrote: > Hello Chris, > Please find the improved patch for adding to libvirt - support > for running tests with multiple VMs without interfering with > other VM resources. Please let me know if there are any issues- > my security patches are based on this - > > From 5cff67ca3f004381ddcbe862ce20ae6894bb742e Mon Sep 17 00:00:00 2001 > From: Onkar N Mahajan<onkar.n.maha...@linux.vnet.ibm.com> > Date: Tue, 26 Jun 2012 22:31:11 +0530 > Subject: [PATCH] Libvirt-Autotest: Support for running multiple guest > (Improved) Signed-off-by: Onkar N Mahajan > <onkar.n.maha...@linux.vnet.ibm.com> > > --- > client/tests/libvirt/tests-shared.cfg.sample | 12 +- > client/virt/libvirt_vm.py | 159 > +++++++++++++++++++++++++- > client/virt/virt_utils.py | 38 +++++-- > client/virt/virt_vm.py | 54 +++++++++- > 4 files changed, 246 insertions(+), 17 deletions(-) > > diff --git a/client/tests/libvirt/tests-shared.cfg.sample > b/client/tests/libvirt/tests-shared.cfg.sample > index 1fe0894..64cf516 100644 > --- a/client/tests/libvirt/tests-shared.cfg.sample > +++ b/client/tests/libvirt/tests-shared.cfg.sample > @@ -28,13 +28,13 @@ include virtio-win.cfg > # * The parameters cdrom_unattended, floppy, kernel and initrd are generated > # by LIBVIRT autotest, so remember to put them under a writable location > # (for example, the cdrom share can be read only) > -image_name(_.*)? ?<= /tmp/libvirt_autotest_root/images/ > -cdrom(_.*)? ?<= /tmp/libvirt_autotest_root/ > -floppy ?<= /tmp/libvirt_autotest_root/ > -image_dir = /tmp/libvirt_autotest_root/ > +image_name(_.*)? ?<= /tmp/libvirt_autotest_root/$main_vm/ > +cdrom(_.*)? ?<= /tmp/libvirt_autotest_root/$main_vm/ > +floppy ?<= /tmp/libvirt_autotest_root/$main_vm/ > +image_dir = /tmp/libvirt_autotest_root/$main_vm/ > Linux..unattended_install: > - kernel ?<= /tmp/libvirt_autotest_root/ > - initrd ?<= /tmp/libvirt_autotest_root/ > + kernel ?<= /tmp/libvirt_autotest_root/$main_vm/ > + initrd ?<= /tmp/libvirt_autotest_root/$main_vm/ > > # Uncomment the following lines to enable abort-on-error mode: > #abort_on_error = yes When I run the autotest "noobie" get_started.py script it still goes after the old directory: 15:40:56 INFO | 15:40:56 INFO | 3 - Verifying iso (make sure we have the OS ISO needed for the default test set) 15:40:56 INFO | Found /tmp/libvirt_autotest_root/isos/linux/Fedora-17-x86_64-DVD.iso 15:40:56 INFO | Expected SHA1 sum: 7a748072cc366ee3bdcd533afc70eda239c977c7 15:40:56 INFO | Would you like to check /tmp/libvirt_autotest_root/isos/linux/Fedora-17-x86_64-DVD.iso? It might take a while (y/n) n 15:41:09 INFO | File /tmp/libvirt_autotest_root/isos/linux/Fedora-17-x86_64-DVD.iso present, but chose to not verify it However, when I run the cartesian parser and show contents for default tests, I get: # shared/cartesian_config.py tests/libvirt/tests.cfg --contents ... cdrom_cd1 = /tmp/libvirt_autotest_root/vm1/isos/linux/Fedora-17-x86_64-DVD.iso cdrom_unattended = /tmp/libvirt_autotest_root/vm1/images/f17-64/ks.iso ... This will most certainly break, unless something happens to move/link the cdrom image under /tmp/libvirt_autotest_root/vm1/isos/linux/... But if we just keep copying or moving images from the default to $main_vm/isos/ we'll fill up /tmp very quickly. However, I'm guessing that you have tests which need to set specific selinux labels on iso images, and using bind-mount or symlinks would apply those labels to everything. Also consider that older libvirt and older anaconda can't deal with multiple cdroms (content and kickstart) needed for install. Another problem with current libvirt configuration setup is that the media/location prefixing in tests-shared.cfg happens AFTER the subtests.cfg where normally the test-specific variant parameters would live. I don't think there's a way to get this working for your tests, maintain the current configuration layout and not touch the unattended_install test - which I'm guessing that you're avoiding (I don't blame you) :D This is a *VERY* tricky problem to solve, while not breaking other tests. At the same time, I see our current setup as somewhat broken to start :S Rather than continue the brokenness with more patches on-top, maybe we can think of how to fix the setup so it works for everyone? I'd summarize the basic problem as: "How to specify a per-guest, per-OS, and per-VM image/media location where subtests.cfg variants may modify image/media location when needed (i.e. your security tests)" Maybe do these image-prefix parameters need to go into their own configuration module inbetween guest-os.cfg and subtest.cfg? Let's think about this more. There must be a solution that will improve the layout, and let your tests have per-vm private images location when they need them. Maybe this issue we should tackle as seperate thread? Do you have other ideas how this can be solved? > diff --git a/client/virt/libvirt_vm.py b/client/virt/libvirt_vm.py > index d97ac0c..df9fd9b 100644 > --- a/client/virt/libvirt_vm.py > +++ b/client/virt/libvirt_vm.py > @@ -461,6 +461,156 @@ def virsh_detach_device(name, xml_file, extra="", > uri=""): > logging.error("Detaching device from VM %s failed." % name) > return False > > +def virsh_vncdisplay(name, uri=""): > + """ > + Returns the VNC display information for the domain > + Returns : [<IP>,<port> ] > + """ > + > + cmd = "vncdisplay %s" % name > + try: > + cmdresult = virsh_cmd(cmd, uri).stdout.strip() > + # Match IP|'':<port> > + m = re.search('^((?:25[0-]|2[0-4][0-9]|[01]?[0-9][0-9]?\.){3}' > + '(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))?\:(\d+)', > + cmdresult) I'm all for using regex when it makes sense, but this code is just matching numbers, '.' and ':'. Is regex the right tool for the job? I'm wondering there's a simpler way that would be a little easier on the eyes, and less prone to single-character typo's? If not, then doing it this way is okay. > + ip = m.group(1) > + if ip == None: > + ip = "0.0.0.0" > + vncinfo = [ ip, m.group(2)] > + return vncinfo > + except error.CmdError, details: > + out = str(details) > + if out.strip() == "": > + domxml = virsh_dumpxml(self.name, self.connect_uri) > + dom = minidom.parseString(domxml) > + if dom.getElementsByTagName('graphics'): > + display = node.childNodes[0] > + ap = display.attributes["autoport"] Does this work if "autoport" is not defined? IIRC it's not a required attribute and it's not used/supported on older libvirt versions. If you can, give this a try on a RHEL5 or Fedora 11 host and make sure it works. If not, I can double-check for you also. > + if ap.value =="yes": > + # VNC display configured with autoport='yes' > + return ["0.0.0.0", -1] > + else: > + raise virt_vm.UnsupportedDisplayError(self.name, "vnc") > + raise virt_vm.LibvirtVirshCmdError(name, cmd, uri, details) > + > + > +def virsh_list(option="--all", uri = ""): > + """ > + - list domains - > + @param option: Can take various values see 'virsh help list' > + for more details. > + @param uri : connect uri > + > + Returns a list : > + Returns dictionary with each entry > + as :<'dom-name'>: [<dom-id> ,<dom-status> ] > + > + """ > + cmd = "list %s" % option > + try: > + cmd_res = virsh_cmd(cmd, uri).stdout.strip() > + d = {} > + for line in cmd_res.split("\n"): > + l = [] > + m = re.search('^\s([0-9]*\-?)\s+([0-9a-zA-Z\.-_]+)\s+(.*)', line) Using regex here is fine, and right tool to use I think. I think this needs to be ^\s*... to match zero-or-more spaces at the beginning. i.e. there's room for a space and 2-digits worth of ID. But we should not count on how virsh decides to display ID's > 99. > + if m: > + l.append(m.group(1)) > + l.append(m.group(3)) > + d[m.group(2)] = l > + return d Good, yes, I like this dictionary of id/status list building, it's easy to read and understand. > + except error.CmdError, details: > + raise virt_vm.LibvirtVirshCmdError(virsh_cmd=cmd, connuri=uri, > + details=details) > + > + > +def list_all_domains(uri, option="--all"): Since "--all" is an option, maybe method should be called 'list_domains' instead? But if you don't want to change the name it's okay, not a show-stopper. > + """ > + Returns list of domains for the uri > + """ > + try: > + val = virsh_list(option, uri) > + return val > + except virt_vm.LibvirtVirshCmdError: > + raise virt_vm.LibvirtFailedGettingInfoError(connuri=uri) > + Overall I like this new way, it's very short, simple, and to the point. It also matches the style of many other methods which is good. > + > +def get_graphics_info(name, uri, displaytype): One small thing here, I think it's okay to have the parameter default displaytype="vnc". IMHO "vnc" will be specified most frequently, so it's nice for test-writers if it's the default. > + """ > + Returns Graphics display Information configured for a given > + domain. > + ** Currently supports only VNC display ** > + """ > + if displaytype == "vnc": > + try: > + """ > + Add virsh functions to get graphics information > + here. > + """ Why is this here? Should it be a comment instead of a floatin string value? It just looks strange for some reason, maybe I had too much coffee :D > + vncinfo = virsh_vncdisplay(name, uri) > + if vncinfo: > + ip = vncinfo[0] > + port = vncinfo[1] > + return [ip, port] Why not just return vncinfo? Is there reason to break it down, then build it back into list? > + except virt_vm.UnsupportedDisplayError: > + raise NotImplementedError("Only VMs with VNC" > + "displays are supported") > + except virt_vm.LibvirtVirshCmdError: > + raise virt_vm.LibvirtFailedGettingInfoError(name, uri) I think this is fine for now. > + > + > +def get_free_port(name, uri, type, startport, endport): type = "vnc" would be nice default here too. > + """ > + Get first free port available for a given application. > + ** Currently supports only VNC ** > + Returns : port (>0) : success > + -1 : Failure > + """ > + if type == "vnc": > + val={} > + assigned_ports=[] > + maxport = 0 > + port = 0 > + try: > + val = list_all_domains(uri) > + except virt_vm.LibvirtFailedGettingInfoError: > + logging.error("Failed to get domains information for " > + "%s" % (self.connect_uri)) > + return -1 > + for key in val.keys(): > + try: > + vncinfo = get_graphics_info(key, uri, type) > + p = int(vncinfo[1]) > + if p>= startport: > + p = p-int(startport) > + if p>= maxport: > + maxport = p > + assigned_ports.append(p) > + except (virt_vm.LibvirtFailedGettingInfoError, > + NotImplementedError): > + logging.error("Failed to get information for VM %s " > + "at URI %s" % (name, uri)) > + return -1 > + cmd = "vncserver -list | grep ^:[0-9]* | cut -c1-4" > + cmd_result = str(utils.run(cmd)) > + for line in cmd_result.split("\n"): > + m = re.search('^:(\d)\s*$', line) > + if m: > + vncsp = m.group(1) > + vncserverport = int(vncsp) > + if vncserverport>= maxport: > + maxport = vncserverport > + assigned_ports.append(vncserverport) > + for p in range(0,endport-startport): > + if not p in assigned_ports: > + port = p > + if virt_utils.find_free_port(port,port+1): > + break > + else: > + continue > + port += startport > + return port > + Maybe I need MORE coffee, I think I'm not understanding something here :D Could you explain why we need this function? Oh! duhhhhh, nevermind, I should just scroll down :D ... > > class VM(virt_vm.BaseVM): > """ > @@ -485,6 +635,7 @@ class VM(virt_vm.BaseVM): > self.process = None > self.serial_console = None > self.redirs = {} > + self.displaytype= None > self.vnc_port = 5900 > self.vnclisten = "0.0.0.0" > self.pci_assignable = None > @@ -934,6 +1085,7 @@ class VM(virt_vm.BaseVM): > virt_install_cmd += add_location(help, location) > > if params.get("display") == "vnc": > + self.displaytype = "vnc" > if params.get("vnc_port"): > vm.vnc_port = int(params.get("vnc_port")) > virt_install_cmd += add_vnc(help, vm.vnc_port) > @@ -941,8 +1093,10 @@ class VM(virt_vm.BaseVM): > vm.vnclisten = params.get("vnclisten") > virt_install_cmd += add_vnclisten(help, vm.vnclisten) > elif params.get("display") == "sdl": > + self.displaytype = "sdl" > virt_install_cmd += add_sdl(help) > elif params.get("display") == "nographic": > + self.displaytype = "nographic" > virt_install_cmd += add_nographic(help) > > video_device = params.get("video_device") > @@ -1188,7 +1342,10 @@ class VM(virt_vm.BaseVM): > > # Find available VNC port, if needed > if params.get("display") == "vnc": > - self.vnc_port = virt_utils.find_free_port(5900, 6100) > + self.displaytype = "vnc" > + port = get_free_port(self.name, self.connect_uri, > self.displaytype, > + 5900, 6100) > + self.vnc_port = port > Can't we just let libvirt figure it out if there's no port? IIRC if vncport == 0 libvirt behaves like autoport=true (but autoport isn't available on older libvirt). Maybe I'm wrong. > # Find available spice port, if needed > if params.get("spice"): > diff --git a/client/virt/virt_utils.py b/client/virt/virt_utils.py > index 1c4d90d..5079cbb 100644 > --- a/client/virt/virt_utils.py > +++ b/client/virt/virt_utils.py > @@ -3889,14 +3889,36 @@ def virt_test_assistant(test_name, test_dir, > base_dir, default_userspace_paths, > logging.info("%d - Verifying directories (check if the directory > structure " > "expected by the default test config is there)", step) > sub_dir_list = ["images", "isos", "steps_data"] > - for sub_dir in sub_dir_list: > - sub_dir_path = os.path.join(base_dir, sub_dir) > - if not os.path.isdir(sub_dir_path): > - logging.debug("Creating %s", sub_dir_path) > - os.makedirs(sub_dir_path) > - else: > - logging.debug("Dir %s exists, not creating" % > - sub_dir_path) > + > + if test_name == 'libvirt': > + base_fl = "%s/%s" % (test_dir, "base.cfg") > + if not os.path.isfile(base_fl): > + base_fl = "%s/%s" % (common_dir, "base.cfg.sample") > + cmd = "cat %s | grep \"^main_vm\s=\s.*\" | cut -d \"=\" -f 2|sed > 's/[^\w]//'" % base_fl > + try: > + vmdir = utils.run("%s" % cmd).stdout > + except error.CmdError, detail: > + logging.error("Failed to fetch 'main_vm' parameter from base.cfg > :%s", detail) > + sys.exit(1) > + base_dir = "%s/%s" % (base_dir, vmdir.strip()) > + for sub_dir in sub_dir_list: > + sub_dir_path = os.path.join(base_dir, sub_dir) > + if not os.path.isdir(sub_dir_path): > + logging.debug("Creating %s", sub_dir_path) > + os.makedirs(sub_dir_path) > + else: > + logging.debug("Dir %s exists, not creating" % > + sub_dir_path) > + else: > + for sub_dir in sub_dir_list: > + sub_dir_path = os.path.join(base_dir, sub_dir) > + if not os.path.isdir(sub_dir_path): > + logging.debug("Creating %s", sub_dir_path) > + os.makedirs(sub_dir_path) > + else: > + logging.debug("Dir %s exists, not creating" % > + sub_dir_path) > + > logging.info("") > step += 1 > logging.info("%d - Creating config files from samples (copy the default > " This libvirt-specific stuff in the generic virt_test_assistant looks a little out of place. Should virt_test_assistant() maybe be broken down into per-vm_type sub-functions? I'm not sure how to address this. The big if test_name == 'libvirt' looks wierd to me. I'll look at it again tomorrow and maybe have another idea (after fresh coffee :D) > diff --git a/client/virt/virt_vm.py b/client/virt/virt_vm.py > index 534f8e3..201992d 100644 > --- a/client/virt/virt_vm.py > +++ b/client/virt/virt_vm.py > @@ -1,8 +1,8 @@ > -import logging, time, glob, re > +import os, logging, time, glob, re, shutil, string > from autotest.client.shared import error > +from autotest.client import utils > import virt_utils, virt_remote > > - > class VMError(Exception): > pass > > @@ -312,6 +312,56 @@ class VMUSBControllerPortFullError(VMUSBControllerError): > def __str__(self): > return ("No available USB Controller port left for VM %s." % > self.name) > > +class VMDisplayError(VMError): > + pass > + > +class UnsupportedDisplayError(VMDisplayError): > + def __init__(self, name, display): > + VMDisplayError.__init__(self, name, displaytype) > + self.name = name > + self.display = displaytype > + > + def __str__(self): > + return ("%s display not supported for this VM '%s' on this host." % > + (string.upper(self.display), self.name)) > + > +""" > +Libvirt error handling starts below. > +""" > + > +class LibvirtError(Exception): > + pass > + > +class LibvirtFailedGettingInfoError(LibvirtError): > + def __init__(self, name, connuri): > + LibvirtError.__init__(self, name, connuri) > + self.name = name > + self.uri = connuri > + > + def __str__(self, name=None, uri =""): > + if self.name: > + return ("Libvirt failed to get information for VM '%s' at" > + " connection URI '%s'" % (self.name, self.uri)) > + else: > + return ("Libvirt failed to get information for " > + "connection '%s' : %s" % (self.uri)) > + > +class LibvirtVirshCmdError(LibvirtError): > + def __init__(self, name, virsh_cmd, connuri, details=""): > + LibvirtError.__init__(self, name, virsh_cmd, connuri, details) > + self.virshcmd = virsh_cmd > + self.name = name > + self.uri = connuri > + self.details = details > + > + def __str__(self): > + if self.name : > + return ("Error executing command '%s' for VM '%s' at" > + " connection URI '%s' : " > + "%s" % (self.virshcmd, self.name, self.uri, > self.details)) > + else: > + return ("Error in executing commmand '%s' for connect URI" > + " %s : %s" % (self.virshcmd, self.uri, self.details)) > class BaseVM(object): > """ Is there a reason these new libvirt-specific exceptions need to be in the shared virt-vm module? If not, please consider moving the definitions into libvirt_vm. i.e. I don't think the pure KVM tests will ever need to raise libvirt exceptions. Thanks again for this contribution, it's a lot of nice and needed code. I'm happy to help get it accepted, let's see if we can work out how to solve the configuration problem together. It's not easy problem but I think it needs to be fixed. I'll talk with lmr about it to see if he has any ideas. -- Chris Evich, RHCA, RHCE, RHCDS, RHCSS Quality Assurance Engineer e-mail: cevich + `@' + redhat.com o: 1-888-RED-HAT1 x44214 _______________________________________________ Autotest mailing list Autotest@test.kernel.org http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
