diff mbox series

test: storage: Fill in default vol types for every pool

Message ID 50b34b98d9f891447203a7bfb64954430ad8981e.1551987121.git.crobinso@redhat.com
State Superseded
Headers show
Series test: storage: Fill in default vol types for every pool | expand

Commit Message

Cole Robinson March 7, 2019, 7:35 p.m. UTC 
Fill in a default volume type for every pool type, as reported
by the VolGetInfo API.

Signed-off-by: Cole Robinson <crobinso@redhat.com>

---
A more complete fix would take into account VolDef->type, so test
driver users could set this value via volume XML. That would require
adding a PostParse callback to the storage driver infrastructure.
This is still an improvement in the interim and would be part of
a complete fix anyways

 src/test/test_driver.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Comments

Andrea Bolognani March 8, 2019, 8:32 a.m. UTC | #1 
On Thu, 2019-03-07 at 14:35 -0500, Cole Robinson wrote:
> @@ -5164,13 +5164,26 @@ testStorageVolDelete(virStorageVolPtr vol,

>  static int

>  testStorageVolumeTypeForPool(int pooltype)

>  {

> -    switch (pooltype) {

> -        case VIR_STORAGE_POOL_DIR:

> -        case VIR_STORAGE_POOL_FS:

> -        case VIR_STORAGE_POOL_NETFS:

> -            return VIR_STORAGE_VOL_FILE;

> -        default:

> -            return VIR_STORAGE_VOL_BLOCK;

> +    switch ((virStoragePoolType) pooltype) {

> +    case VIR_STORAGE_POOL_DIR:

> +    case VIR_STORAGE_POOL_FS:

> +    case VIR_STORAGE_POOL_NETFS:

> +    case VIR_STORAGE_POOL_VSTORAGE:

> +        return VIR_STORAGE_VOL_FILE;

> +    case VIR_STORAGE_POOL_SHEEPDOG:

> +    case VIR_STORAGE_POOL_ISCSI_DIRECT:

> +    case VIR_STORAGE_POOL_GLUSTER:

> +    case VIR_STORAGE_POOL_RBD:

> +        return VIR_STORAGE_VOL_NETWORK;

> +    case VIR_STORAGE_POOL_LOGICAL:

> +    case VIR_STORAGE_POOL_DISK:

> +    case VIR_STORAGE_POOL_MPATH:

> +    case VIR_STORAGE_POOL_ISCSI:

> +    case VIR_STORAGE_POOL_SCSI:

> +    case VIR_STORAGE_POOL_ZFS:


Surely VIR_STORAGE_VOL_BLOCK should be returned here...

> +    case VIR_STORAGE_POOL_LAST:

> +    default:

> +        return VIR_STORAGE_VOL_BLOCK;

>      }


... and these last two would result in virReportEnumRangeError()
being called?


I won't comment on the actual change since it's out of my area of
expertise, though it looks sane enough from where I'm standing.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
John Ferlan March 8, 2019, 12:40 p.m. UTC | #2 
On 3/7/19 2:35 PM, Cole Robinson wrote:
> Fill in a default volume type for every pool type, as reported

> by the VolGetInfo API.

> 

> Signed-off-by: Cole Robinson <crobinso@redhat.com>

> ---

> A more complete fix would take into account VolDef->type, so test

> driver users could set this value via volume XML. That would require

> adding a PostParse callback to the storage driver infrastructure.

> This is still an improvement in the interim and would be part of

> a complete fix anyways

> 

>  src/test/test_driver.c | 27 ++++++++++++++++++++-------

>  1 file changed, 20 insertions(+), 7 deletions(-)

> 


At first I wondered if this had to do with changes I made starting at
commit 4ad00278f going thru commit 538b83ae, but it doesn't seem so.

> diff --git a/src/test/test_driver.c b/src/test/test_driver.c

> index ce0df1f8e3..3f37b2ede7 100644

> --- a/src/test/test_driver.c

> +++ b/src/test/test_driver.c

> @@ -5164,13 +5164,26 @@ testStorageVolDelete(virStorageVolPtr vol,

>  static int

>  testStorageVolumeTypeForPool(int pooltype)

>  {

> -    switch (pooltype) {

> -        case VIR_STORAGE_POOL_DIR:

> -        case VIR_STORAGE_POOL_FS:

> -        case VIR_STORAGE_POOL_NETFS:

> -            return VIR_STORAGE_VOL_FILE;

> -        default:

> -            return VIR_STORAGE_VOL_BLOCK;

> +    switch ((virStoragePoolType) pooltype) {

> +    case VIR_STORAGE_POOL_DIR:

> +    case VIR_STORAGE_POOL_FS:

> +    case VIR_STORAGE_POOL_NETFS:

> +    case VIR_STORAGE_POOL_VSTORAGE:

> +        return VIR_STORAGE_VOL_FILE;

> +    case VIR_STORAGE_POOL_SHEEPDOG:

> +    case VIR_STORAGE_POOL_ISCSI_DIRECT:


^^ This I would think should be BLOCK even though I see
virISCSIDirectRefreshVol uses NETWORK at volume creation.

Perhaps Michal can answer provide some insights.

> +    case VIR_STORAGE_POOL_GLUSTER:

> +    case VIR_STORAGE_POOL_RBD:

> +        return VIR_STORAGE_VOL_NETWORK;

> +    case VIR_STORAGE_POOL_LOGICAL:

> +    case VIR_STORAGE_POOL_DISK:

> +    case VIR_STORAGE_POOL_MPATH:

> +    case VIR_STORAGE_POOL_ISCSI:


^^^ To a degree ISCSI could be NETWORK, but may also be BLOCK. Still the
default seems to be BLOCK (see below)

> +    case VIR_STORAGE_POOL_SCSI:

> +    case VIR_STORAGE_POOL_ZFS:

> +    case VIR_STORAGE_POOL_LAST:


^^^ I agree w/ Andrea that this could an Enum error; however, it doesn't
seem the caller at this point can handle that.

> +    default:

> +        return VIR_STORAGE_VOL_BLOCK;

>      }

>  }

>  

> 


Using cscope I did an egrep search on "vol.*type.* = " as well as
"VIR_ALLOC.*vol" in order to help find places where a volume's type may
or should be set to see what kind of defaults are in use.

Beyond that consider that volume's in a pool volume list are recreated
at every refresh, so following the *Refresh logic to find types works
pretty well too.

For virStorageBackendRefreshLocal it's VIR_STORAGE_VOL_FILE
For virStorageBackendSCSINewLun it's VIR_STORAGE_VOL_BLOCK (used by scsi
and iscsi backends via calls to virStorageBackendSCSIFindLUs).

Perhaps a "more correct" answer comes from virStoragePoolTypeInfoLookup
or virStoragePoolOptionsForPoolType and adjustment of poolTypeInfo in
storage_conf.c?

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson March 8, 2019, 5:35 p.m. UTC | #3 
On 3/8/19 7:40 AM, John Ferlan wrote:
> 

> 

> On 3/7/19 2:35 PM, Cole Robinson wrote:

>> Fill in a default volume type for every pool type, as reported

>> by the VolGetInfo API.

>>

>> Signed-off-by: Cole Robinson <crobinso@redhat.com>

>> ---

>> A more complete fix would take into account VolDef->type, so test

>> driver users could set this value via volume XML. That would require

>> adding a PostParse callback to the storage driver infrastructure.

>> This is still an improvement in the interim and would be part of

>> a complete fix anyways

>>

>>  src/test/test_driver.c | 27 ++++++++++++++++++++-------

>>  1 file changed, 20 insertions(+), 7 deletions(-)

>>

> 

> At first I wondered if this had to do with changes I made starting at

> commit 4ad00278f going thru commit 538b83ae, but it doesn't seem so.

> 

>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c

>> index ce0df1f8e3..3f37b2ede7 100644

>> --- a/src/test/test_driver.c

>> +++ b/src/test/test_driver.c

>> @@ -5164,13 +5164,26 @@ testStorageVolDelete(virStorageVolPtr vol,

>>  static int

>>  testStorageVolumeTypeForPool(int pooltype)

>>  {

>> -    switch (pooltype) {

>> -        case VIR_STORAGE_POOL_DIR:

>> -        case VIR_STORAGE_POOL_FS:

>> -        case VIR_STORAGE_POOL_NETFS:

>> -            return VIR_STORAGE_VOL_FILE;

>> -        default:

>> -            return VIR_STORAGE_VOL_BLOCK;

>> +    switch ((virStoragePoolType) pooltype) {

>> +    case VIR_STORAGE_POOL_DIR:

>> +    case VIR_STORAGE_POOL_FS:

>> +    case VIR_STORAGE_POOL_NETFS:

>> +    case VIR_STORAGE_POOL_VSTORAGE:

>> +        return VIR_STORAGE_VOL_FILE;

>> +    case VIR_STORAGE_POOL_SHEEPDOG:

>> +    case VIR_STORAGE_POOL_ISCSI_DIRECT:

> 

> ^^ This I would think should be BLOCK even though I see

> virISCSIDirectRefreshVol uses NETWORK at volume creation.

> 

> Perhaps Michal can answer provide some insights.

> 


My understanding of volume type is that it's more an indication of how
the VM will access the resource. It has a direct effect on how tools
like virt-manager will craft <disk> XML to make use of the volume. So,
while a pool type=netfs NFS volume is from a network source, qemu will
access it like a regular host file. It would have type=file not type=network

iscsi direct advertises remote network volumes. qemu needs to access the
shares over the network using its native iscsi support. so type=network

>> +    case VIR_STORAGE_POOL_GLUSTER:

>> +    case VIR_STORAGE_POOL_RBD:

>> +        return VIR_STORAGE_VOL_NETWORK;

>> +    case VIR_STORAGE_POOL_LOGICAL:

>> +    case VIR_STORAGE_POOL_DISK:

>> +    case VIR_STORAGE_POOL_MPATH:

>> +    case VIR_STORAGE_POOL_ISCSI:

> 

> ^^^ To a degree ISCSI could be NETWORK, but may also be BLOCK. Still the

> default seems to be BLOCK (see below)

> 


where as plain type='iscsi' uses iscsiadm to turn those network shares
into local block devices, which qemu accesses like any other local block
device. so type=block

>> +    case VIR_STORAGE_POOL_SCSI:

>> +    case VIR_STORAGE_POOL_ZFS:

>> +    case VIR_STORAGE_POOL_LAST:

> 

> ^^^ I agree w/ Andrea that this could an Enum error; however, it doesn't

> seem the caller at this point can handle that.

> 

>> +    default:

>> +        return VIR_STORAGE_VOL_BLOCK;

>>      }

>>  }

>>  

>>

> 

> Using cscope I did an egrep search on "vol.*type.* = " as well as

> "VIR_ALLOC.*vol" in order to help find places where a volume's type may

> or should be set to see what kind of defaults are in use.

> 


I basically did the same in prep for this patch. There aren't too many
hits with:

git grep -E "VIR_STORAGE_VOL_(FILE|BLOCK|NETWORK|NETDIR|PLOOP)"

> Beyond that consider that volume's in a pool volume list are recreated

> at every refresh, so following the *Refresh logic to find types works

> pretty well too.

> 

> For virStorageBackendRefreshLocal it's VIR_STORAGE_VOL_FILE

> For virStorageBackendSCSINewLun it's VIR_STORAGE_VOL_BLOCK (used by scsi

> and iscsi backends via calls to virStorageBackendSCSIFindLUs).

> 

> Perhaps a "more correct" answer comes from virStoragePoolTypeInfoLookup

> or virStoragePoolOptionsForPoolType and adjustment of poolTypeInfo in

> storage_conf.c?

> 


Maybe, but it's adding something to common code to fix the fairly
artificial test driver case, so I'll need to validate it doesn't have
any side effects to the real storage drivers.

I'll post this as a v2 with the enum error bit changed. If that doesn't
get an ACK i'll try and do a more complete fix at a later time,
including the changes I mentioned in the commit message

Thanks,
Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
diff mbox series

Patch

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index ce0df1f8e3..3f37b2ede7 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -5164,13 +5164,26 @@  testStorageVolDelete(virStorageVolPtr vol,
 static int
 testStorageVolumeTypeForPool(int pooltype)
 {
-    switch (pooltype) {
-        case VIR_STORAGE_POOL_DIR:
-        case VIR_STORAGE_POOL_FS:
-        case VIR_STORAGE_POOL_NETFS:
-            return VIR_STORAGE_VOL_FILE;
-        default:
-            return VIR_STORAGE_VOL_BLOCK;
+    switch ((virStoragePoolType) pooltype) {
+    case VIR_STORAGE_POOL_DIR:
+    case VIR_STORAGE_POOL_FS:
+    case VIR_STORAGE_POOL_NETFS:
+    case VIR_STORAGE_POOL_VSTORAGE:
+        return VIR_STORAGE_VOL_FILE;
+    case VIR_STORAGE_POOL_SHEEPDOG:
+    case VIR_STORAGE_POOL_ISCSI_DIRECT:
+    case VIR_STORAGE_POOL_GLUSTER:
+    case VIR_STORAGE_POOL_RBD:
+        return VIR_STORAGE_VOL_NETWORK;
+    case VIR_STORAGE_POOL_LOGICAL:
+    case VIR_STORAGE_POOL_DISK:
+    case VIR_STORAGE_POOL_MPATH:
+    case VIR_STORAGE_POOL_ISCSI:
+    case VIR_STORAGE_POOL_SCSI:
+    case VIR_STORAGE_POOL_ZFS:
+    case VIR_STORAGE_POOL_LAST:
+    default:
+        return VIR_STORAGE_VOL_BLOCK;
     }
 }