[libvirt] [PATCH 1/2] logical: implement volume resize

Tue Sep 19 01:29:50 UTC 2017

On 09/07/2017 08:56 AM, apolyakov at beget.ru wrote:
> From: Alexander Polyakov <apolyakov at beget.com>
> 
> Add new virStorageBackendLogicalResizeVol function to
> implement resize of logical volumes
> 
> Sparse volumes are not supported
> 
> Signed-off-by: Alexander Polyakov <apolyakov at beget.com>
> ---
>  m4/virt-storage-lvm.m4                |  4 ++++
>  src/storage/storage_backend_logical.c | 37 +++++++++++++++++++++++++++++++++++
>  2 files changed, 41 insertions(+)
> 

Curious - is there a way to create/modify an LV to use LUKS encryption
via some lvm* commands?  I have another bz and the only way that comes
to me right now is to create the LV using qemu-img with and then somehow
have it recognized/added so that LVM recognizes it. I haven't put much
thought into it yet though...

Anyway...

> diff --git a/m4/virt-storage-lvm.m4 b/m4/virt-storage-lvm.m4
> index a0ccca7a0..0932995b4 100644
> --- a/m4/virt-storage-lvm.m4
> +++ b/m4/virt-storage-lvm.m4
> @@ -30,6 +30,7 @@ AC_DEFUN([LIBVIRT_STORAGE_CHECK_LVM], [
>      AC_PATH_PROG([VGREMOVE], [vgremove], [], [$LIBVIRT_SBIN_PATH])
>      AC_PATH_PROG([LVREMOVE], [lvremove], [], [$LIBVIRT_SBIN_PATH])
>      AC_PATH_PROG([LVCHANGE], [lvchange], [], [$LIBVIRT_SBIN_PATH])
> +    AC_PATH_PROG([LVRESIZE], [lvresize], [], [$LIBVIRT_SBIN_PATH])
>      AC_PATH_PROG([VGCHANGE], [vgchange], [], [$LIBVIRT_SBIN_PATH])
>      AC_PATH_PROG([VGSCAN], [vgscan], [], [$LIBVIRT_SBIN_PATH])
>      AC_PATH_PROG([PVS], [pvs], [], [$LIBVIRT_SBIN_PATH])
> @@ -44,6 +45,7 @@ AC_DEFUN([LIBVIRT_STORAGE_CHECK_LVM], [
>        if test -z "$VGREMOVE" ; then AC_MSG_ERROR([We need vgremove for LVM storage driver]) ; fi
>        if test -z "$LVREMOVE" ; then AC_MSG_ERROR([We need lvremove for LVM storage driver]) ; fi
>        if test -z "$LVCHANGE" ; then AC_MSG_ERROR([We need lvchange for LVM storage driver]) ; fi
> +      if test -z "$LVRESIZE" ; then AC_MSG_ERROR([We need lvresize for LVM storage driver]) ; fi
>        if test -z "$VGCHANGE" ; then AC_MSG_ERROR([We need vgchange for LVM storage driver]) ; fi
>        if test -z "$VGSCAN" ; then AC_MSG_ERROR([We need vgscan for LVM storage driver]) ; fi
>        if test -z "$PVS" ; then AC_MSG_ERROR([We need pvs for LVM storage driver]) ; fi
> @@ -57,6 +59,7 @@ AC_DEFUN([LIBVIRT_STORAGE_CHECK_LVM], [
>        if test -z "$VGREMOVE" ; then with_storage_lvm=no ; fi
>        if test -z "$LVREMOVE" ; then with_storage_lvm=no ; fi
>        if test -z "$LVCHANGE" ; then with_storage_lvm=no ; fi
> +      if test -z "$LVRESIZE" ; then with_storage_lvm=no ; fi
>        if test -z "$VGCHANGE" ; then with_storage_lvm=no ; fi
>        if test -z "$VGSCAN" ; then with_storage_lvm=no ; fi
>        if test -z "$PVS" ; then with_storage_lvm=no ; fi
> @@ -75,6 +78,7 @@ AC_DEFUN([LIBVIRT_STORAGE_CHECK_LVM], [
>        AC_DEFINE_UNQUOTED([VGREMOVE],["$VGREMOVE"],[Location of vgremove program])
>        AC_DEFINE_UNQUOTED([LVREMOVE],["$LVREMOVE"],[Location of lvremove program])
>        AC_DEFINE_UNQUOTED([LVCHANGE],["$LVCHANGE"],[Location of lvchange program])
> +      AC_DEFINE_UNQUOTED([LVRESIZE],["$LVRESIZE"],[Location of lvresize program])
>        AC_DEFINE_UNQUOTED([VGCHANGE],["$VGCHANGE"],[Location of vgchange program])
>        AC_DEFINE_UNQUOTED([VGSCAN],["$VGSCAN"],[Location of vgscan program])
>        AC_DEFINE_UNQUOTED([PVS],["$PVS"],[Location of pvs program])
> diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c
> index 67f70e551..e5cb09922 100644
> --- a/src/storage/storage_backend_logical.c
> +++ b/src/storage/storage_backend_logical.c
> @@ -1072,6 +1072,42 @@ virStorageBackendLogicalVolWipe(virConnectPtr conn,
>      return -1;
>  }
>  

I see the lvmresize command allows you to supply which physical device
the extents would come from. Would that be something we might want to
try here as well?  Thankfully I don't think snapshot or mirror'd LV's
come into play (yet).... nor thin pool or stripes.

In any case, I don't know whether someone would "want" to choose a
specific PV to draw from or just let LVM make the decision and let it be
for now. If we did have another parameter we'd have to check if the PV
existed and was part of the VG... lot's more work, but didn't want to
miss noting it either. Just typing some thoughts...

> +static int
> +virStorageBackendLogicalResizeVol(virConnectPtr conn ATTRIBUTE_UNUSED,
> +                                   virStoragePoolObjPtr pool ATTRIBUTE_UNUSED,
> +                                   virStorageVolDefPtr vol,
> +                                   unsigned long long capacity,
> +                                   unsigned int flags)
                                     ^
The spacing for each of the argument rows is off by a single space.

> +{
> +    virCheckFlags(VIR_STORAGE_VOL_RESIZE_SHRINK |
> +                  VIR_STORAGE_VOL_RESIZE_DELTA, -1);
> +
> +    bool delta = flags & VIR_STORAGE_VOL_RESIZE_DELTA;
> +    bool shrink = flags & VIR_STORAGE_VOL_RESIZE_SHRINK;

Definitions go before the virCheckFlags - yes, I know the code isn't
consistent, but new code can be.

> +
> +    if (vol->target.sparse) {
> +        virReportError(VIR_ERR_NO_SUPPORT,
> +                        _("logical volume '%s' is sparse, volume resize "
> +                            "not supported"),
                             ^^
Off by 2 spaces here

> +                        vol->target.path);
> +        return -1;
> +    }
> +
> +    virCommandPtr cmd = virCommandNewArgList(LVRESIZE, "-L", NULL);

virCommandPtr cmd; needs to be at the top...

> +
> +    if (delta) {
> +        virCommandAddArgFormat(cmd, "%c%lluB", shrink ? '-' : '+',
> +                                 capacity);

Off by 2 spaces here too, but this can be on the previous line....

> +    } else {
> +        virCommandAddArgFormat(cmd, "%lluB", capacity);
> +    }

... meaning the { } { } are not necessary

Not that any other backend specifically checks... Should there be any
concern that capacity - shrink < 0 - I assume LVM may choke on that
right? Makes me wonder if this code should add a &status to the
virCommandRun below and then a check that the returned status is valid;
otherwise, it could appear that the command succeeded when in fact it
didn't.  Yes, that's a departure from other commands in this module, but
that doesn't mean they're necessarily right either.

> +    virCommandAddArgFormat(cmd, "%s", vol->target.path);
> +    int ret = virCommandRun(cmd, NULL);

int ret; at the top

I can fix the nit spacing things before pushing.

Let me know your thoughts on altering the virCommandRun to change NULL
to &status and then issue some sort of error:

if (status != 0) {
    virReportError(...)
    ...
}

NB: I didn't test with what status could return, I'm assuming == 0 is
good status, anything else is bad status.

The ReportError could be something to the effect that the command
failed.  If you add and output and error buffer (e.g.
virCommandSet{Output|Error}Buffer the error from the command can be
attached.

Reviewed-by: John Ferlan <jferlan at redhat.com>

John

> +
> +    virCommandFree(cmd);
> +    return ret;
> +}
> +
>  virStorageBackend virStorageBackendLogical = {
>      .type = VIR_STORAGE_POOL_LOGICAL,
>  
> @@ -1089,6 +1125,7 @@ virStorageBackend virStorageBackendLogical = {
>      .uploadVol = virStorageBackendVolUploadLocal,
>      .downloadVol = virStorageBackendVolDownloadLocal,
>      .wipeVol = virStorageBackendLogicalVolWipe,
> +    .resizeVol = virStorageBackendLogicalResizeVol,
>  };
>  
>  
> 