Re: [Libvir] PATCH: 0/7 Implementation of storage APIs

General comments. (I haven't yet read the patches line by line, but there are some things that I noticed that we can get back to later.) I patched up my libvirt with all the patches (the remote patches turned out to be necessary after all for reasons I now understand -- see below). Out of the box, I can do: # virsh pool-list Name State Autostart ----------------------------------------- Now I have to add my storage pools manually. Even after reading previous mail on the XML formats, it was very unclear what the correct format to use was: # cat /tmp/pool.xml <pool type="fs"> <name>xenimages</name> <uuid>12345678-1234-1234-1234-123456781234</uuid> <target dir="/var/lib/xen/images"/> <permissions> <mode>0700</mode> <owner>0</owner> <group>0</group> <label>xen_image_t</label> </permissions> </pool> # src/virsh pool-create /tmp/pool.xml Pool xenimages created from /tmp/pool.xml The first problem here is that we've got the "secret config files" again. I know that Xen made this idiotic mistake of hiding the config files from the world, and in libvirt we have to work around it for domains, but there's no particular reason why we have to copy this bad, non-Unix-like design decision into other parts of our API. I was envisaging a much more straightforward config file: /etc/libvirt.conf ------------------- disk_storage_pools: [ "/var/lib/xen/images" ] lvm_volgroups: [ "/dev/MyXenImages" ] ------------------------------------- With this, you don't need to put storage logic into libvirtd. It can all be discovered on demand, just using the config file and the commands already included in storage_backend_*.c The upshot is that we don't need a daemon to manage storage pools, except in the remote case where it's just there to do things remotely. (To be fair, the proposed patch seems to have a config file in /etc/libvirt/storage/storage.conf, but it's not implemented at the moment and it is unclear what will go in here, and whether a suitable default can be created to allow storage pools to default to something sensible on Fedora). # src/virsh pool-list Name State Autostart ----------------------------------------- libvir: Storage error : pool has no config file xenimages active no autostart There's no pool-info binding in virsh yet, but there is a corresponding call at the libvirt level and thankfully the output is a struct. Create a volume: # cat /tmp/vol.xml <volume> <name>tempvol</name> <uuid>12345678-1234-1234-1234-123456781234</uuid> <capacity>100000</capacity> <allocation>100000</allocation> <format> <type>raw</type> </format> <permissions> <mode>0700</mode> <owner>0</owner> <group>0</group> <label>xen_image_t</label> </permissions> </volume> # src/virsh vol-create xenimages /tmp/vol.xml (libvirtd dumped core at this point) And I still don't think that passing XML descriptions is a good idea. But because you're envisaging being able to create complex volumes like qcow, encrypted, etc., I will temper this by saying that we should have a small core XML which all drivers MUST support. For example, every driver must support pools and volumes described like this (verbatim, with no extra fields): <pool type="xxx"> <name>xenimages</name> <!-- target should make sense for the xxx driver, eg. path for fs, LUN name for iSCSI, etc. There is no need for the dir=... attribute --> <target>/var/lib/xen/images</target> <!-- permissions should default to sensible values --> </pool> and: <volume> <name>tempvol</name> <!-- I noticed that units are missing from original --> <capacity unit="GiB">10</capacity> <!-- permissions and format should default to sensible --> </volume> Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903