11:35 a.m.
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(a)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