* Chris Evich <cev...@redhat.com> [2012-06-08 10:45:29]: > On 06/08/2012 02:06 AM, Onkar N Mahajan wrote: > > 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!
Hello Lucas, Please let us know, if any changes needed for this patch. We would like to send security patches based on this approach. --Pradeep > >> > >>> 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. > > > > Yes, well explained, you're right, and it can be changed back to the > default. Okay, I'm fine with this change then. > > >> 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 ? > > > > I guess my main fear of not testing the default, by default is that > things which are to work out-of-the-box might not be tested. I guess it > comes down to a difference in testing methodology. Either way, I'm okay > with this as-is, and as you mentioned it's easy to fix later if needed. > > >>> 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. > > I agree, though throwing an exception on error doesn't prevent this either. > > > 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 is also possible through the exception mechanism. However by > throwing an exception, callers don't need to be written 'defensively' by > default. This design is more inline with "the python-way" and the style > used elsewhere in autotest. Granted this is a subtle difference, but I > think there's value in keeping with the same style and conventions. > Plus, lmr doesn't like functions/methods that return multiple values anyway. > > > 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. > > Yep, and this flow is fine. The only difference is in using a > try/except vs if/else :D > > > > > 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. > > > > This usage is also supported by using exceptions, and in fact it's even > easier since you don't need to catch anything. If it breaks and raises, > the test fails w/o needing any extra code. > > >> > >>> + > >>> + """ > >>> + 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. > > Annoyingly, it must be extracted from the xml :( However, you're right > in that virsh only has a vncdisplay command. Along those lines, how > about adding a lower-level and more generic virsh_vncdisplay() function > that returns the port and ip address? > > I'd like the the vm class methods to support a wider usage, even if all > the capabilities aren't there immediately. When possible, methods > should not be tied closely to underlying implementation details. > > For example with graphics, I'd prefer a vm.graphics_info() (or similar > name) method provides details independent of the underlying graphics > device type. But it's perfectly acceptable if support for other types > gets added at a later date, and it just throws exceptions for the stubs. > > With this deign, we can easily extend the tests (later) by adding > support for additional graphics. It also allows for extending the > underlying support w/o disturbing the callers. Make sense? > > > 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. > > Absolutely, together we can do more than any individual can alone :) > Thank for your contributions! > > -- > 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 > _______________________________________________ Autotest mailing list Autotest@test.kernel.org http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
