[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Mon, Oct 29, 2007 at 12:03:34PM +0000, Richard W.M. Jones wrote:
> 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

As we did with virsh attach-disk/attach-interface, vs attach-device
I think it would be helpful to provide a 'virsh vol-create' variant
which took a handful of explicit args as an alternative to the XML,

eg   virsh pool-create --type fs xenimages /var/lib/xen/images

It wouldn't have the full capabilities to set every single XML
attribute/element, but would be good enough so that admins didn't
need to write the XML for the 95% common case.
   
> 
> 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.

The config file are not hidden/secret - they're in the usual libvirt
daemon config location /etc/libvirt/storage/[pool name].xml

As we do with the 'default' network, we can easily ship a canned config
file for say, /var/lib/virt/images  as a local directory based pool.
Any other type of pool is going to be site-specific, but we can provide
a selection of common example configs as needed.

> I was envisaging a much more straightforward config file:
> 
>   /etc/libvirt.conf -------------------
> 
>   disk_storage_pools: [ "/var/lib/xen/images" ]
>   lvm_volgroups: [ "/dev/MyXenImages" ]
>   -------------------------------------

That is not sufficient to describe all the metadata for the different
types of storage pool.

> 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 honest even in the local case we need the daemon. The only reason
we previously get away without having a daemon when running locally, is
that the entire virt-manager app has run as root. Long term I think 
pretty much all libvirt clients will be going via the daemon, whether 
local or remote.

> (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

Opps, that error message is a bug. 

> 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.

Yep, I was planning to add a pool-info API to list the capacity vs 
allocation of the pool & name, uuid, etc, as with dom-info. Likeiwise
for the vol-info API

> 
> 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)

The format type was not quite right <format type='raw'>. It obviously
shouldn't dump core though!

NB, <format> is actually optional - it defaults to raw. Permissions
should be optional, but currently aren't. Allocation is optional too,
without it, you will get a sparse file.

> 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>

Yep, I intended permissions to be optional. Not all pools have a target,
some only have a source. Basically name,and either target or source
is the minimal set. Everything else should be optional.

> 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>

Yes, adding units is a good idea. That looks good as a minimal dataset.

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]