Re: [libvirt] [PATCH] Drop virStorageBackendLogicalMatchPoolSource

On Thu, Jun 16, 2016 at 07:44:05AM -0400, John Ferlan wrote: > > > On 06/16/2016 06:03 AM, Ján Tomko wrote: >> On Wed, Jun 15, 2016 at 06:45:59PM -0400, John Ferlan wrote: >>> >>> >>> On 06/15/2016 01:19 PM, Ján Tomko wrote: >>>> Regression introduced by commit 71b803a for [1] that prevents >>>> starting up >>>> a logical pool created with <source><device path=''></source> >>>> after it has been moved to a different physical volume. >>> >>> Is there a bug for this? XML examples? >>> >>> Is an empty source device path string a valid value? Reading >>> http://libvirt.org/formatstorage.html doesn't give me that impression. >> >> I meant any pool with a <device> specified, not an empty path. > > But the patch is specifically targeted at removing the _logical pool > check. So the question is, is it "legal" for a logical pool to "start" > with an empty string as a source device path? Secondarily I'm curious > what's the use case of that? I do not know what happens with an empty string. <source><device path=''></source> was not an exact snippet, just a hint that there is a device path specified. If you create a pool like this: <pool type='logical'> <name>vgname</name> <source> <device path='/dev/sda4'/> <name>vgname</name>

</source> <target> <path>/dev/vgname</path> </target> </pool> Then pvmove /dev/sda4 to /dev/sda5, libvirt will refuse that pool even though <name>vgname</name> is enough to uniquely identify a storage pool.

> >> >> The whole point of LVM is abstraction from the lower layers so we >> shouldn't ever be checking this. >> > > So it's OK to activate/start a pool where the source device path is > incorrect? For LVM, the important path is in <source><name>.

> >>> >>> "device >>> Provides the source for pools backed by physical devices (pool types >>> fs, logical, disk, iscsi, zfs). May be repeated multiple times >>> depending >>> on backend driver. Contains a required attribute path which is either >>> the fully qualified path to the block device node or for iscsi the >>> iSCSI >>> Qualified Name (IQN). Since 0.4.1" >>> >>> >>>> >>>> For logical pools <source><name> contains the name of the volume group >>>> and uniquely identifies the VG on the host. >>>> >>>> This also speeds up startup for pools that do not have any <device>s >>>> specified. >>>> >>>> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1025230 >>>> --- >>>> src/storage/storage_backend_logical.c | 104 >>>> +--------------------------------- >>>> 1 file changed, 2 insertions(+), 102 deletions(-) >>>> >>> >>> This essentially reverts a patch that was used to resolve a bz >>> without a >>> patch to resolve the issue in the bug. What's the proposal/patch to >>> resolve the issue from the bug? >>> >> >> The issue in the bug is just cosmetic and should not block fixing this >> regresion. >> > > How is it just cosmetic? Let's say we allow starting a pool with an > incorrect or invalid source device path. Any attempt to "use" or "list" > a volume in the pool would fail. > The pool works fine, the logical volumes are under the same path in /dev/, the only thing that has changed is the underlying PV device. > I will agree that it would seem unlikely in a "real world" situation > that an admin would create/configure a logical pool using a vgname that > is not associated with the provided source device path; however, to me > it seems just as unlikely that the source device path is either not > provided or is set to ''. > Not providing the source device path at all is perfectly fine for existing logical pools, they are only needed for building new pools. Jan