On Thu, 2012-06-07 at 10:52 -0400, Chris Evich wrote: > On 06/07/2012 04:46 AM, Onkar N Mahajan wrote: > > Hello All, > > > > With this patch , you will be able to run tests that use multiple VMs > > with libvirt. One such > > test is VLAN test ; Which libvirt autotest currently is not capable of > > running. > > Capability to run multiple VMs simultaneoulsy and 'explicit' > > sharing/restricting of resources with > > permissions important for my forthcoming security patches which will test > > libvirt security infrastructure > > (namely svirt). This patch only attempts segregate VM disk resources > > (CDROM, VM images) under a VM > > directory which is created per VM at the time of installation. Forthcoming > > patches will address > > *explicit* sharing/restricting of resources with configuration options. > > Cool! I can't wait to see them! > > > My intention here was to 'do more with less code' , i.e., minimally > > disturb existing autotest code. > > For unattended_install , all you need to do is : > > Great, this is a good mentality/approach. As-is, most of the libvirt > Autotest code was 'organically' developed/evolved. We'll be the first > to admit, some if it is pretty scary. Since it's still very immature, > please don't be afraid of helping (or asking us to) fix broken areas. > It's much better for the long run if we spend a little extra time to get > the fundamentals proper, before more and more tests are added. > > > (1) create a VM directory under /tmp/libvirt_autotest_root/ whose name > > must exactly match > > param 'main_vm' ( in base.cfg ) for e.g., > > /tmp/libvirt_autotest_root/vm1/ , and > > (2) create iso directory under VM directory > > (/tmp/libvirt_autotest_root/vm1/)- namely > > 'linux/iso' (or simply 'iso' - recommended , but requires small > > change in guest-os.cfg for > > your guest at param 'cdrom_cd1' , for instance originally your > > 'cdrom_cd1' would look like > > 'cdrom_cd1 = isos/linux/RHELX.iso' , but now it can just be > > 'cdrom_cd1 = isos/RHELX.iso' ) > > (3) Place your guest CDROM in the driectory created in (1) and (2) > > i.e., /tmp/libvirt_autotest_root/vm1/linux/iso ( or simply > > /tmp/libvirt_autotest_root/vm1/iso > > , see recomendation in (2)) > > > > 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 | 69 > > +++++++++++++++++++++++++- > > client/virt/virt_utils.py | 3 + > > 3 files changed, 77 insertions(+), 7 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 > > I don't quite follow the need for vm-specific directories. Are you > intending to use them for explicit selinux contexts and permissions > testing? If so, this is probably okay, but: > Yes, for every device (resource) used by the VM , libvirt (dynamic seclabel) or admin (static seclebel) will assign explicit seclebels which would be used by selinux to protect the VMs from being unauthorized access. > My concern for svirt testing this way (and our using /tmp/... in > general) is just making things more difficult and further from the > intended end-user use-case. > > In both Fedora and RHEL, default SELinux policy and file contexts are > already setup for using /var/lib/libvirt/... under many different > scenarios.
As mentioned earlier, this segregation of resources was done with minimal changes to what already exists and is a convention in autotest, for specific needs like storing images at default location used by libvirt ( /var/lib/libvirt/ ...) requires minor (to be precise 6 lines) changes in the configuration file tests-shared.cfg. And this works !! > I'm wondering if there's a good reason for libvirt autotest > to even use /tmp at all, or if it's just done to mirror kvm autotest. > Virtualization testing , not all is performed using autotest , so to avoid security and other tests from interfering with other tests (which might be performed manually in parallel) this is required. But if this interference is acceptable - configuration option mentioned above is available. For now, IMHO we can continue with this , and later when *need arises* we can think of changing to default libvirt directory. > I've always thought we should roll with the defaults more, with most of > this stuff under /var/lib/libvirt/... where it's expected anyway. i.e. > I think there's more value from a testing perspective in exercising it > the way it was intended to be used first, and secondarily supporting > special-case setups as needed. > Default location is just one type of storage pool ( http://libvirt.org/storage.html ) under libvirt , there are others too, which are more likely to be used in production scenarios ( due to their obvious performance benefits over the default pool (dir)), with storage management (and security) prospective. These are also important and need to be tested. > IIRC, there's nothing stopping setting non-standard contexts/permissions > on stuff under /var/lib/libvirt/... > yes, as mentioned above , this is possible with this implementation with trivial configuration changes. > Can anyone think of a reason libvirt autotest should not default to the > defaults??? > > N.B. 3-4 months ago, I was successful in testing a move of everything > under /var/lib/libvirt and running with selinux enabled/enforcing w/o > complaint. So it's possible to do. > yes , I agree that this is possible , but can you think of , for now, any erroneous test results as a result of using this standard autotest directory convention ? > > diff --git a/client/virt/libvirt_vm.py b/client/virt/libvirt_vm.py > > index cbed8aa..035cedb 100644 > > --- a/client/virt/libvirt_vm.py > > +++ b/client/virt/libvirt_vm.py > > @@ -457,6 +457,34 @@ def virsh_detach_device(name, xml_file, extra = "", > > uri = ""): > > logging.error("Detaching device from VM %s failed." % name) > > return False > > > > +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 : > > + (<rc of virsh list command> ,<dictionary 'dom-name': > > + [<dom-id> ,<dom-status> ]) > > This return seems a bit convoluted since only a True/False rc is > returned anyway. Why not just return the list of dictionaries, and > throwing exceptions to signal problems (see below also)? First , It makes sense to peep into the dictionary only if virsh has returned successfully. In virsh_list(), the context is wether 'virsh list' has passed or failed - we need to log this for easier debugging in case of failure. This error will be logged in the log file : logging.error("Failed listing domains : %s" % (detail)) Then this unclean exit signal will be propagated upwards until in nxt_free_vnc_port() : Here we are concerned wether or not we are able to get the peer domain information (in this case vnc ports) , if we are not able to get it , then we must throw an obvious error message : logging.error("Unable to get peer domain information") by seeing at this we can easily get to the conclusion that something is wrong with libvirt - more investigation on this will reveal the "Failed listing domains ..." message with details of failure. Secondly, in cases where we only need to check if 'virsh list' works, we just need to use the rc status. This might be needed in some future tests. > > > + > > + """ > > + cmd = "list %s" % option > > + try: > > + cmd_res = virsh_cmd(cmd, uri) > > + d = {} > > + for line in cmd_res.split("\n"): > > + l = [] > > + m = re.search('^\s([0-9]*\-?)\s+(\w+)\s+(.*)', line) > > Would '^\s*([0-9]*\-?)\s+(\w+)\s+(.*)' be safer in case starting > whitespace gets lost for whatever reason? > ^\s* in the above regex , addresses this issue. > > + if m: > > + l.append(m.group(1)) > > + l.append(m.group(3)) > > + d[m.group(2)] = l > > + return (True, d) > > + except error.CmdError, detail: > > If we catch exceptions at this low level, it will negate 'error testing' > possibilities - granted testing 'virsh list' with invalid options is of > arguable value. However IMHO we should leave options open. > explained earlier > > + logging.error("Failed listing domains : %s" % (detail)) > > + return (False, None) > > + > > > > class VM(virt_vm.BaseVM): > > """ > > @@ -529,6 +557,43 @@ class VM(virt_vm.BaseVM): > > """ > > return virsh_is_alive(self.name, self.connect_uri) > > > > + def list_peer_domain(self, option="--all"): > > Can we call this list_peer_domains (plural)? I think it will 'read' > better in test code. > > + """ > > + Returns list of domains for the uri > > + """ > > + (rc, val) = virsh_list(option, self.connect_uri) > > + return (rc, val) > > This is fine. > > > + > > + > > + def nxt_free_vnc_port(self): > > + """ > > + Returns max VNC port in use for a particular uri > > + """ > > + (rc, val) = self.list_peer_domain() > > + if not rc: > > + logging.error("Unable to get peer domain information") > > + sys.exit(1) > > + else: > > + port = 0 > > + for key in val.keys(): > > + """ > > + This currently , only works with VMs installed with > > + valid VNC port , and doesn't work with domains installed > > + with<graphics type='vnc' port='-1' autoport='yes'/>. So > > + domains installed with autotest will be installed with > > valid > > + port numbers , externally installed VMs (installed with for > > + instance virt-manager, virt-install - must be started > > before this > > + test is performed.To get valid results and for libvirt > > + autotest to work successfully. > > + """ > > + cmd_res = virsh_cmd("vncdisplay %s" % key, > > self.connect_uri) > > + m = re.search('(.*)\:(\d+)', cmd_res) > > + if m.group(2)>= port: > > + port = m.group(2) > > + p = int(port) > > + if p<= self.vnc_port: > > + p += self.vnc_port > > + return p+1 > This is a VERY specific/limited use method that doesn't throw exceptions > when expectations aren't met. However, libvirt autotest's handling of > vnc/spice/etc graphics is fairly 'wonky' and I've been meaning to clean > it up for a while now. > See, I have tried making nxt_free_vnc_port() available for general usage (vnc/spice/etc) as you mentioned , but this requires virsh to have vncdisplay counterparts to spice/etc - which AFAIK are currently not available. see line : cmd_res = virsh_cmd("vncdisplay %s" % key, self.connect_uri) And so this function has been named nxt_free_*vnc*_port() to emphasize that this is currently only for VNC. I agree , that there are other means to get the ports , but this requires more work for now. > What are your thoughts (and testing ideas) on if we got rid of all the > ancillary port finding code and just rely on libvirt to auto-assign by > default? Maybe that would make this method unnecessary? AFAIK , this will cause problems with network filters tests. > > N/B: runtime auto-assign is what KVM autotest does. > > > def is_dead(self): > > """ > > @@ -1161,7 +1226,9 @@ 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) > > + p = self.nxt_free_vnc_port() > > + self.vnc_port = virt_utils.find_free_port(p, 6100) > > + > > > > # 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 4bbfc7b..e9ce755 100644 > > --- a/client/virt/virt_utils.py > > +++ b/client/virt/virt_utils.py > > @@ -678,6 +678,9 @@ def create_image(params, root_dir): > > """ > > format = params.get("image_format", "qcow2") > > image_filename = get_image_filename(params, root_dir) > > + > > + if not os.path.exists(os.path.dirname(image_filename)): > > + os.mkdir(os.path.dirname(image_filename)) > > size = params.get("image_size", "10G") > > if params.get("create_with_dd") == "yes" and format == "raw": > > # maps K,M,G,T => (count, bs) > > -- > > 1.7.4.4 > > > > > > Thanks a lot for taking time to review my patch. Thank you very much. -- Onkar N Mahajan System Software Engineer, IBM Linux Technology Center, Bangalore,India _______________________________________________ Autotest mailing list Autotest@test.kernel.org http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
