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

[John Ferlan]

On 10/23/2015 10:33 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 | 91 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 92 insertions(+)
> 

Other than a nit below this seems better than the last one. Hopefully
Peter Krempa can also take a look since he's far more familiar with any
snapshot gotcha's than I am.


> diff --git a/include/libvirt/libvirt-storage.h b/include/libvirt/libvirt-storage.h
> index 453089e..9fc3c2d 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 = 1 << 1, /* Force removal of volume, even if in use */
>  } virStorageVolDeleteFlags;
>  
>  typedef enum {
> diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c
> index 5ae4713..b494c11 100644
> --- a/src/storage/storage_backend_rbd.c
> +++ b/src/storage/storage_backend_rbd.c
> @@ -421,6 +421,75 @@ static int virStorageBackendRBDRefreshPool(virConnectPtr conn,
>      return ret;
>  }
>  
> +static int virStorageBackendRBDCleanupSnapshots(rbd_image_t image,
> +                                                virStoragePoolSourcePtr source,
> +                                                virStorageVolDefPtr vol)
> +{
> +    int ret = -1;
> +    int r = 0;
> +    int max_snaps = 128;
> +    int snap_count, protected;
> +    size_t i;
> +    rbd_snap_info_t *snaps;
> +
> +    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);
> +
> +    } while (snap_count == -ERANGE);
> +
> +    VIR_DEBUG("Found %d snapshots for volume %s/%s", snap_count,
> +              source->name, vol->name);
> +
> +    if (snap_count > 0) {
> +        for (i = 0; i < snap_count; i++) {
> +            if (rbd_snap_is_protected(image, snaps[i].name, &protected)) {
> +                virReportSystemError(-r, _("failed to verify if snapshot '%s/%s@%s' is protected"),
> +                                     source->name, vol->name,
> +                                     snaps[i].name);
> +                goto cleanup;
> +            }
> +
> +            if (protected == 1) {
> +                VIR_DEBUG("Snapshot %s/%s@%s is protected needs to be "
> +                          "unprotected", source->name, vol->name,
> +                          snaps[i].name);
> +
> +                if (rbd_snap_unprotect(image, snaps[i].name) < 0) {
> +                    virReportSystemError(-r, _("failed to unprotect snapshot '%s/%s@%s'"),
> +                                         source->name, vol->name,
> +                                         snaps[i].name);
> +                    goto cleanup;
> +                }
> +            }
> +
> +            VIR_DEBUG("Removing snapshot %s/%s@%s", 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'"),
> +                                     source->name, vol->name,
> +                                     snaps[i].name);
> +                goto cleanup;
> +            }
> +        }
> +    }
> +
> +    ret = 0;
> +
> + cleanup:
> +    if (snaps)
> +        rbd_snap_list_end(snaps);
> +
> +    VIR_FREE(snaps);
> +    return ret;
> +}
> +
>  static int virStorageBackendRBDDeleteVol(virConnectPtr conn,
>                                           virStoragePoolObjPtr pool,
>                                           virStorageVolDefPtr vol,
> @@ -431,6 +500,7 @@ static int virStorageBackendRBDDeleteVol(virConnectPtr conn,
>      virStorageBackendRBDState ptr;
>      ptr.cluster = NULL;
>      ptr.ioctx = NULL;
> +    rbd_image_t image = NULL;
>  
>      VIR_DEBUG("Removing RBD image %s/%s", pool->def->source.name, vol->name);
>  
> @@ -443,6 +513,24 @@ static int virStorageBackendRBDDeleteVol(virConnectPtr conn,
>      if (virStorageBackendRBDOpenIoCTX(&ptr, pool) < 0)
>          goto cleanup;
>  
> +    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;
> +    }
> +
> +    if (flags & VIR_STORAGE_VOL_DELETE_WITH_SNAPSHOTS) {
> +        if (virStorageBackendRBDCleanupSnapshots(image, &pool->def->source, vol) < 0)
> +            goto cleanup;
> +    }
> +
> +    /* We need to close the image before we can actually remove it */
> +    if (rbd_close(image) < 0)
> +        goto cleanup;
> +
> +    VIR_DEBUG("Removing volume %s/%s", pool->def->source.name, vol->name);
> +

Logic is
   perform rbd_open to get pointer to image
   if we have the flag, then call cleanup
   close the image

Why not move the rbd_open/close logic into the Cleanup helper passing
"ptr" instead of "image".  That way the Cleanup does all the open/close
logic and this code has none.

I don't mind moving that prior to pushing - but wanted to check first
(and of course hopefully get Peter to chime in on any thoughts). I'll
revisit this tomorrow for (hopefully) final processing ...

John

>      r = rbd_remove(ptr.ioctx, vol->name);
>      if (r < 0 && (-r) != ENOENT) {
>          virReportSystemError(-r, _("failed to remove volume '%s/%s'"),
> @@ -453,6 +541,9 @@ static int virStorageBackendRBDDeleteVol(virConnectPtr conn,
>      ret = 0;
>  
>   cleanup:
> +    if (image)
> +        rbd_close(image);
> +
>      virStorageBackendRBDCloseRADOSConn(&ptr);
>      return ret;
>  }
>