Bug 1233129

John,
In the case of gluster in following function, source will conflict once source dir and source host of any 2 pools are same. It does not take account of source name. So even if source names of 2 pools are different, they are treated to source conflicts. But in fact, they should not conflict.

virStoragePoolSourceFindDuplicate(virConnectPtr conn,
                                  virStoragePoolObjListPtr pools,
                                  virStoragePoolDefPtr def)
......
case VIR_STORAGE_POOL_GLUSTER:
            if (STREQ(pool->def->source.dir, def->source.dir) &&
                virStoragePoolSourceMatchSingleHost(&pool->def->source,
                                                    &def->source))
                matchpool = pool;
            break;

Repro steps
1. prepare 2 gluster volumes in a host
e.g. I have 2 volumes, gluster-vol1 and gluster-vol2 on my gluster server

# gluster volume info
 
Volume Name: gluster-vol1
Type: Distribute
Volume ID: 28c86ac7-eab3-436b-930f-8bfaa8d6559f
Status: Started
Snap Volume: no
Number of Bricks: 1
Transport-type: tcp
Bricks:
Brick1: 10.66.4.164:/br1
Options Reconfigured:
performance.readdir-ahead: on
server.allow-insecure: on
nfs.disable: on
auto-delete: disable
snap-max-soft-limit: 90
snap-max-hard-limit: 256
 
Volume Name: gluster-vol2
Type: Distribute
Volume ID: 7b96a8b7-56d4-4e94-bf4e-4fab7db56988
Status: Started
Snap Volume: no
Number of Bricks: 1
Transport-type: tcp
Bricks:
Brick1: 10.66.4.164:/br2
Options Reconfigured:
server.allow-insecure: on
performance.readdir-ahead: on
auto-delete: disable
snap-max-soft-limit: 90
snap-max-hard-limit: 256

2. define/start a gluster pool using gluster-vol1 as source
#virsh pool-define gluster.xml
#virsh pool-start gluster
# virsh pool-dumpxml gluster
<pool type='gluster'>
  <name>gluster</name>
  <uuid>6a65fea0-6546-41be-98f0-1c4180eadca9</uuid>
  <capacity unit='bytes'>75125227520</capacity>
  <allocation unit='bytes'>70293786624</allocation>
  <available unit='bytes'>4831440896</available>
  <source>
    <host name='10.66.4.164'/>
    <dir path='/'/>
    <name>gluster-vol1</name>
  </source>
</pool>

3. define one more gluster pool using gluster-vol2 as source
# cat gluster-pool.xml 
<pool type="gluster">
        <name>gluster1</name>
        <source>
          <name>gluster-vol2</name>  ----> source name is different from 1st gluster pool, but host and dir are same with 1st gluster pool
          <host name='10.66.4.164'/>
          <dir path='/'/>
        </source>
      </pool>
# virsh pool-define gluster-pool.xml 
error: Failed to define pool from gluster-pool.xml
error: operation failed: Storage source conflict with pool: 'gluster'

Regards 
Yang

Another concern about gluster pool is that, given a gluster volume consists of over 2 bricks, define 1st gluster pool with host-0, then define 2st gluster pool with host-1, both pools have same source name and dir path (IOW, both pools are using same gluster volume as source), the function cannot check thus conflict. Maybe it's difficult to check.

Repro steps
1. prepare a gluster volume consists of 2 bricks

# gluster volume info gluster-vol2
 
Volume Name: gluster-vol2
Type: Distribute
Volume ID: 7b96a8b7-56d4-4e94-bf4e-4fab7db56988
Status: Started
Snap Volume: no
Number of Bricks: 2
Transport-type: tcp
Bricks:
Brick1: 10.66.4.164:/br2
Brick2: 10.66.5.63:/br2
Options Reconfigured:
server.allow-insecure: on
performance.readdir-ahead: on
auto-delete: disable
snap-max-soft-limit: 90
snap-max-hard-limit: 256

2. define 1st gluster pool using Brick1 as source
# virsh pool-define gluster.xml
Pool gluster defined from gluster.xml
# virsh pool-dumpxml gluster
<pool type='gluster'>
  <name>gluster</name>
  <uuid>6a65fea0-6546-41be-98f0-1c4180eadca9</uuid>
  <capacity unit='bytes'>158970347520</capacity>
  <allocation unit='bytes'>107715432448</allocation>
  <available unit='bytes'>51254915072</available>
  <source>
    <host name='10.66.4.164'/>
    <dir path='/'/>
    <name>gluster-vol2</name>
  </source>
</pool>

3. define 2st gluster pool using Brick2 as source
# virsh pool-define gluster-pool.xml 
Pool gluster1 defined from gluster-pool.xml
# virsh pool-dumpxml gluster1
<pool type='gluster'>
  <name>gluster1</name>
  <uuid>1fa4a4c7-0828-436a-a7f4-e5655ab01968</uuid>
  <capacity unit='bytes'>158970347520</capacity>
  <allocation unit='bytes'>107715416064</allocation>
  <available unit='bytes'>51254931456</available>
  <source>
    <host name='10.66.5.63'/>
    <dir path='/'/>
    <name>gluster-vol2</name>
  </source>
</pool>

Mixing disk, logical, and gluster in one bz is to say the least confusing.

With respect to disk/logical and using /dev/sdc - that's perhaps something that can be addressed.  Of course cross pool checking in general isn't address.  I'm sure you could come up with a way to take an iSCSI pool created /dev/sdX and use it for a disk pool (it's what I do), so restricting it is a double edged sword. I'm not as sure this is a "bug" per se - it may be one of those you know your storage configuration and not configure in this manner. If you do, then you're on your own.  I'm going to condnak design on this as the existing duplicate algorithm is heavily reliant on the pool types being the same.  Having to build in knowledge of pool types that "could" use a "similar" path could be a bit more tricky.  Realistically the best fix would be some sort of locking system that would handle these cross pool issues.

As for gluster, the check done in virStoragePoolSourceFindDuplicate is meant to cover some generic cases. I request a different bug on this.  Yes, it is much more difficult to check duplication.  You can go out of your way to make some sort of duplication occur, but is that a "real world" example?

(In reply to John Ferlan from comment #4)
> Mixing disk, logical, and gluster in one bz is to say the least confusing.
> 
> With respect to disk/logical and using /dev/sdc - that's perhaps something
> that can be addressed.  Of course cross pool checking in general isn't
> address.  I'm sure you could come up with a way to take an iSCSI pool
> created /dev/sdX and use it for a disk pool (it's what I do), so restricting
> it is a double edged sword. I'm not as sure this is a "bug" per se - it may
> be one of those you know your storage configuration and not configure in
> this manner. If you do, then you're on your own.  I'm going to condnak
> design on this as the existing duplicate algorithm is heavily reliant on the
> pool types being the same.  Having to build in knowledge of pool types that
> "could" use a "similar" path could be a bit more tricky.  Realistically the
> best fix would be some sort of locking system that would handle these cross
> pool issues.
> 
> As for gluster, the check done in virStoragePoolSourceFindDuplicate is meant
> to cover some generic cases. I request a different bug on this.  Yes, it is
> much more difficult to check duplication.  You can go out of your way to
> make some sort of duplication occur, but is that a "real world" example?

Opened a separate bug 1236438 to track issues for gluster. Yes, it is common example in the real world. As a cluster, a gluster volume often consists of multiple bricks. So users can use any brick as pool source.

Moving to consideration for 7.3

moving to 7.4

Posted some patches upstream that will resolve this:

https://www.redhat.com/archives/libvir-list/2017-April/msg00243.html

Pushed patches upstream:

commit 4143b194ce29867929b05517c178ef9371ebb8b4
Author: John Ferlan <jferlan>
Date:   Wed Apr 5 09:04:54 2017 -0400

    conf: Check for storage conflicts across pool types
    
...
    
    The virStoragePoolObjSourceFindDuplicate logic used by PoolCreateXML
    and PoolDefineXML avoids comparing the new definition against "other"
    pool types. This can cause unexpected corruption if two different pool
    source types used the same source device path. For example, a 'disk'
    pool using source type device=/dev/sdc could be unwittingly overwritten
    by using /dev/sdc for a 'logical' pool which also uses the source
    device path.
    
    So rather than blindly ignoring those checks when def->type !=
    pool->def->type - have the pool->def->type switch logic handle the
    check for which def->type's should be checked.

$ git describe 4143b194ce29867929b05517c178ef9371ebb8b4
v3.2.0-158-g4143b19
$

Verified components version:
libvirt-3.9.0-6.el7.x86_64

Verified steps:
1. Define/build/start a logical pool using /dev/sdb as source device.
# virsh pool-dumpxml HostVG
<pool type='logical'>
  <name>HostVG</name>
  <uuid>db456984-d54b-4e6d-a36d-ea939fab00d2</uuid>
  <capacity unit='bytes'>1069547520</capacity>
  <allocation unit='bytes'>314572800</allocation>
  <available unit='bytes'>754974720</available>
  <source>
    <device path='/dev/sdb'/>
    <name>HostVG</name>
    <format type='lvm2'/>
  </source>
  <target>
    <path>/dev/HostVG</path>
  </target>
</pool>

2. Define/create a disk pool using the same /dev/sdb sa source device.
# vim disk-pool.xml
<pool type='disk'>
  <name>disk1</name>
  <uuid>9146cba7-efb3-4059-b791-f45e70b1ed26</uuid>
  <source>
    <device path='/dev/sdb'/>
    <format type='dos'/>
  </source>
  <target>
    <path>/dev</path>
    <permissions>
      <mode>0755</mode>
    </permissions>
  </target>
</pool>

# virsh pool-define disk-pool.xml 
error: Failed to define pool from disk-pool.xml
error: operation failed: Storage source conflict with pool: 'HostVG'
# virsh pool-create disk-pool.xml 
error: Failed to create pool from disk-pool.xml
error: operation failed: Storage source conflict with pool: 'HostVG'

3. Change /dev/sdb to /dev/sdc in disk-pool.xml, define/create disk pool successfully.

4. Opposite test: define/create disk pool firstly, and then define/create logical pool using the same source device with disk pool, the result is expected.

Move it to verified.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHEA-2018:0704