[libvirt] [PATCH] Drop virStorageBackendLogicalMatchPoolSource

[Ján Tomko]

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