Re: [libvirt] [PATCH 3/3] storage: Adjustments to disk backend to use stateDir
On 01/14/2015 04:46 AM, Daniel P. Berrange wrote:
On Tue, Jan 13, 2015 at 04:12:32PM -0500, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1138516
>
> The disk backend uses 'parted' in order to create and delete partitions
> on the disk for use in the pool. At creation time, one can specify a
> specific "name" for the volume as well as a specific volume target
> format type. The name and volume target type "survive" only as long
> as the pool is not refreshed or the libvirtd not restarted/reloaded.
>
> The action immediately prior to the calling all the backend refreshPool
> API's is to clear out all the existing volumes from the pool. The
> theory being the refresh will be able to find all elements of the
> pool using some mechanism. The disk refreshPool backend will use the
> libvirt_parthelper utility to read the partitions found on the disk
> in order to regenerate the elements of the pool Unfortunately, since
> the "name" and target format type cannot be encoded, the data is now
> lost and the defaults are used (for the "name", the partition path
> is used and the default of 'none' is used for the target format type).
>
> This patch solves this by adding the ability to save the XML generated
> at create time into the stateDir and then use that during the refreshPool
> backend API call to restore the specific fields that are lost.
I don't really think we should be adding a lookaside cache for this.
Fair enough - although this may then turn into a documentation exercise.
I'm OK with that option - if that is what is desired.
We should simply not permit arbitrary user supplied names for disk
based volumes. The names should be required to match the defined
naming scheme for disk partitions. I could have sworn that we had
enforced that already when I first wrote this, but perhaps that
check got lost somewhere along the way. Likewise for the format - we
should just rely on partition table format data
I looked through some history and didn't see anywhere that supplied name
must match partition name was enforced, but there's a lot of code motion
and new features that may cloud the history. Perhaps it's the assumption
true in so many other pools that 'name' is what was used to create the
file, directory, lv, etc. There is code that fills in the name if not
provided with the partition name (which occurs after Refresh, reload,
restart).
I'm not quite sure how one could enforce a name given that parted
creates the partition name. One would have to know the device name of
the pool and the numerical sequence that parted would use (easy perhaps
know that perhaps partitions 1 & 2 exist that 3 will be next). However,
there's not necessarily a guarantee of that is there?
Reading the libvirt docs - the implication is that the volume "<name>"
must only be unique across the pool, so the following is "valid":
virsh vol-create-as disk-pool vol-linux --format linux 1G
So without assuming/describing the naming scheme of the underlying
parted how do we enforce that the name (vol-linux) is the same as what
parted generates? Or how do we enforce that the name provided ends up
being the same as the partition created? It's kind of a chicken/egg issue.
Not much is done with the target.format in the backend even though it's
described as providing the "partition type". When not provided on the
command line like above it defaults to 'none'. Other than for dos label
pools that would need to have extended partition before creating a
logical partition, there's not much "use" for the field other than to
perhaps store "something" for "someone" (until refresh, reload,
restart
loses it).
I don't see changing the code to enforce incoming name matches. Changing
the name to match the partition created seems to go against standard
practices and for the disk backend the target.format really doesn't
matter. I am open to try other suggestions, although outside of this
particular test failing perhaps there isn't much demand...
John