[libvirt] [PATCH] rbd: Remove snapshots if the DELETE_WITH_SNAPSHOTS flag has been provided

[John Ferlan]

On 10/15/2015 10:38 AM, Wido den Hollander wrote:
> When a RBD volume has snapshots it can not be removed.
> 
> This patch introduces a new flag to force volume removal,
> VIR_STORAGE_VOL_DELETE_WITH_SNAPSHOTS.
> 
> With this flag any existing snapshots will be removed prior to
> removing the volume.
> 
> No existing mechanism in libvirt allowed us to pass such information,
> so that's why a new flag was introduced.
> 
> Signed-off-by: Wido den Hollander <wido at widodh.nl>
> ---
>  include/libvirt/libvirt-storage.h |  1 +
>  src/storage/storage_backend_rbd.c | 59 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 60 insertions(+)
> 
> diff --git a/include/libvirt/libvirt-storage.h b/include/libvirt/libvirt-storage.h
> index 453089e..80da5a3 100644
> --- a/include/libvirt/libvirt-storage.h
> +++ b/include/libvirt/libvirt-storage.h
> @@ -115,6 +115,7 @@ typedef enum {
>  typedef enum {
>      VIR_STORAGE_VOL_DELETE_NORMAL = 0, /* Delete metadata only    (fast) */
>      VIR_STORAGE_VOL_DELETE_ZEROED = 1 << 0,  /* Clear all data to zeros (slow) */
> +    VIR_STORAGE_VOL_DELETE_WITH_SNAPSHOTS = 2, /* Force removal of volume, even if in use */

s/2/(1 << 1)

>  } virStorageVolDeleteFlags;
>  
>  typedef enum {
> diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c
> index 5ae4713..71f7d10 100644
> --- a/src/storage/storage_backend_rbd.c
> +++ b/src/storage/storage_backend_rbd.c
> @@ -428,9 +428,13 @@ static int virStorageBackendRBDDeleteVol(virConnectPtr conn,
>  {
>      int ret = -1;
>      int r = 0;
> +    int max_snaps = 128;
> +    int i, snap_count, protected;

fail syntax-check - must use "size_t i;" since it's a loop counter

>      virStorageBackendRBDState ptr;
>      ptr.cluster = NULL;
>      ptr.ioctx = NULL;
> +    rbd_snap_info_t *snaps;
> +    rbd_image_t image = NULL;
>  
>      VIR_DEBUG("Removing RBD image %s/%s", pool->def->source.name, vol->name);
>  
> @@ -443,6 +447,60 @@ static int virStorageBackendRBDDeleteVol(virConnectPtr conn,
>      if (virStorageBackendRBDOpenIoCTX(&ptr, pool) < 0)
>          goto cleanup;
>  

Is any of this useful without VIR_STORAGE_VOL_DELETE_WITH_SNAPSHOTS? I
also think you'd leak whatever rbd_snap_list places into the 'snaps'
entries since rbd_snap_list_end would only be called if the bit was set.

Can this be turned into a helper routine instead, e.g.

    if (flags & VIR_STORAGE_VOL_DELETE_WITH_SNAPSHOTS) {
        if (virStorageBackendRBDCleanupSnapshots(...args...) < 0)
            goto cleanup;
    }

Your new helper should set some kind of error on failure; otherwise,
you'll get a generic libvirt failed for some reason type error which
could be confusing.  IOW: Anywhere you 'goto cleanup;' will need some
sort of message.

I'm assuming 'failure' means the rbd_remove() was going to fail anyway,
so going to cleanup on failure to cleanup is "ok"


> +    r = rbd_open(ptr.ioctx, vol->name, &image, NULL);
> +    if (r < 0) {
> +       virReportSystemError(-r, _("failed to open the RBD image '%s'"),
> +                            vol->name);
> +       goto cleanup;
> +    }
> +
> +    do {
> +        if (VIR_ALLOC_N(snaps, max_snaps))
> +            goto cleanup;
> +
> +        snap_count = rbd_snap_list(image, snaps, &max_snaps);
> +        if (snap_count <= 0) {
> +            VIR_FREE(snaps);
> +        }

^^ fails syntax-check (the extra { } for one-liner)

> +    } while (snap_count == -ERANGE);

Too bad there wasn't a way to pass (image, NULL, &max_snaps); where
max_snaps = 0 to start out with in order to get the number to use for
max_snaps.  Although I assume the loop would only need to be executed at
most 2 times. So no big deal.

> +
> +    VIR_DEBUG("Found %d snapshots for volume %s/%s", snap_count,
> +              pool->def->source.name, vol->name);
> +
> +    if (snap_count > 0 && (flags & VIR_STORAGE_VOL_DELETE_WITH_SNAPSHOTS)) {
> +        for (i = 0; i < snap_count; i++) {
> +            if (rbd_snap_is_protected(image, snaps[i].name, &protected))
> +                goto cleanup;

No error message?

> +
> +            if (protected == 1) {
> +                VIR_DEBUG("Snapshot %s/%s@%s is protected needs to be "
> +                          "unprotected", pool->def->source.name, vol->name,
> +                          snaps[i].name);
> +
> +                if (rbd_snap_unprotect(image, snaps[i].name) < 0)
> +                    goto cleanup;

No error message?

> +            }
> +
> +            VIR_DEBUG("Removing snapshot %s/%s@%s", pool->def->source.name,
> +                      vol->name, snaps[i].name);
> +
> +            r = rbd_snap_remove(image, snaps[i].name);
> +            if (r < 0) {
> +                virReportSystemError(-r, _("failed to remove snapshot '%s/%s@%s'"),
> +                                     pool->def->source.name, vol->name,
> +                                     snaps[i].name);
> +                goto cleanup;
> +            }
> +        }
> +
> +        rbd_snap_list_end(snaps);
> +    }
> +
> +    if (rbd_close(image) < 0)
> +        goto cleanup;

perhaps this should be moved to cleanup and changed to:

   if (image)
       rbd_close(image);

because all those goto's inside your "if" condition will not close image.

Also does rbd_snap_list_end need to be called if any of the goto
cleanup's happen?  Can I assume that will run through the list of snaps
and free the snaps[i].name?  and anything else - so yes, I think it
needs to be in cleanup as well (lots easier when this code is a helper).

John
> +
> +    VIR_DEBUG("Removing volume %s/%s", pool->def->source.name, vol->name);
> +
>      r = rbd_remove(ptr.ioctx, vol->name);
>      if (r < 0 && (-r) != ENOENT) {
>          virReportSystemError(-r, _("failed to remove volume '%s/%s'"),
> @@ -453,6 +511,7 @@ static int virStorageBackendRBDDeleteVol(virConnectPtr conn,
>      ret = 0;
>  
>   cleanup:
> +    VIR_FREE(snaps);
>      virStorageBackendRBDCloseRADOSConn(&ptr);
>      return ret;
>  }
>