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

[Daniel P. Berrange]

On Thu, Oct 15, 2015 at 10:28:39AM +0200, 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_FORCED.
> 
> 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 
> ---
>  include/libvirt/libvirt-storage.h |  1 +
>  src/storage/storage_backend_rbd.c | 58 
> +++
>  2 files changed, 59 insertions(+)
> 
> diff --git a/include/libvirt/libvirt-storage.h 
> b/include/libvirt/libvirt-storage.h
> index 453089e..36ff979 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_FORCED = 2, /* 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 ac5085a..ca5e802 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;
>  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,59 @@ static int virStorageBackendRBDDeleteVol(virConnectPtr 
> conn,
>  if (virStorageBackendRBDOpenIoCTX(, pool) < 0)
>  goto cleanup;
>  
> +r = rbd_open(ptr.ioctx, vol->name, , 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, _snaps);
> +if (snap_count <= 0) {
> +VIR_FREE(snaps);
> +}
> +} while (snap_count == -ERANGE);
> +
> +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_FORCED)) {
> +for (i = 0; i < snap_count; i++) {
> +if (rbd_snap_is_protected(image, snaps[i].name, ))
> +goto cleanup;
> +
> +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;
> +}
> +
> +VIR_DEBUG("Removing snapshot %s/%s@%s", pool->def->source.name,
> +  vol->name, snaps[i].name);
> +
> +if (rbd_snap_remove(image, snaps[i].name) < 0) {
> +virReportSystemError(-r, _("failed to remove snapshot 
> '%s/%s@%s'"),

You're reporting '-r' as an errno, but this was last set on the
rbd_open call many lines earlier, so doesn't correspond to the
rbd_snap_remove error.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

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

[Wido den Hollander]

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_FORCED.

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 
---
 include/libvirt/libvirt-storage.h |  1 +
 src/storage/storage_backend_rbd.c | 58 +++
 2 files changed, 59 insertions(+)

diff --git a/include/libvirt/libvirt-storage.h 
b/include/libvirt/libvirt-storage.h
index 453089e..36ff979 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_FORCED = 2, /* 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 ac5085a..ca5e802 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;
 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,59 @@ static int virStorageBackendRBDDeleteVol(virConnectPtr 
conn,
 if (virStorageBackendRBDOpenIoCTX(, pool) < 0)
 goto cleanup;
 
+r = rbd_open(ptr.ioctx, vol->name, , 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, _snaps);
+if (snap_count <= 0) {
+VIR_FREE(snaps);
+}
+} while (snap_count == -ERANGE);
+
+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_FORCED)) {
+for (i = 0; i < snap_count; i++) {
+if (rbd_snap_is_protected(image, snaps[i].name, ))
+goto cleanup;
+
+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;
+}
+
+VIR_DEBUG("Removing snapshot %s/%s@%s", pool->def->source.name,
+  vol->name, snaps[i].name);
+
+if (rbd_snap_remove(image, snaps[i].name) < 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;
+
+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 +510,7 @@ static int virStorageBackendRBDDeleteVol(virConnectPtr conn,
 ret = 0;
 
  cleanup:
+VIR_FREE(snaps);
 virStorageBackendRBDCloseRADOSConn();
 return ret;
 }
-- 
1.9.1

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

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

[Daniel P. Berrange]

On Thu, Oct 15, 2015 at 10:28:39AM +0200, 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_FORCED.
> 
> 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 
> ---
>  include/libvirt/libvirt-storage.h |  1 +
>  src/storage/storage_backend_rbd.c | 58 
> +++
>  2 files changed, 59 insertions(+)
> 
> diff --git a/include/libvirt/libvirt-storage.h 
> b/include/libvirt/libvirt-storage.h
> index 453089e..36ff979 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_FORCED = 2, /* Force removal of volume, even if 
> in use */

Long term I could imagine there will be a number of reasons why
it might be forbidden to delete a volume by default. It would be
nice to selectively override these reasons. FORCED is quite a
generic name for a specific action. So how about naming it
VOL_DELETE_WITH_SNAPSHOTS


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

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

[Wido den Hollander]

On 15-10-15 10:44, Daniel P. Berrange wrote:
> On Thu, Oct 15, 2015 at 10:28:39AM +0200, 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_FORCED.
>>
>> 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 
>> ---
>>  include/libvirt/libvirt-storage.h |  1 +
>>  src/storage/storage_backend_rbd.c | 58 
>> +++
>>  2 files changed, 59 insertions(+)
>>
>> diff --git a/include/libvirt/libvirt-storage.h 
>> b/include/libvirt/libvirt-storage.h
>> index 453089e..36ff979 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_FORCED = 2, /* Force removal of volume, even if 
>> in use */
> 
> Long term I could imagine there will be a number of reasons why
> it might be forbidden to delete a volume by default. It would be
> nice to selectively override these reasons. FORCED is quite a
> generic name for a specific action. So how about naming it
> VOL_DELETE_WITH_SNAPSHOTS
> 

Seems like a good thing to me. Want me to submit a new patch?

Before I do so, any code-wide objections?

Wido

> 
> Regards,
> Daniel
> 

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

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

[Wido den Hollander]

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_FORCED.

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 
---
 include/libvirt/libvirt-storage.h |  1 +
 src/storage/storage_backend_rbd.c | 58 +++
 2 files changed, 59 insertions(+)

diff --git a/include/libvirt/libvirt-storage.h 
b/include/libvirt/libvirt-storage.h
index 453089e..36ff979 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_FORCED = 2, /* 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 ac5085a..ca5e802 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;
 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,59 @@ static int virStorageBackendRBDDeleteVol(virConnectPtr 
conn,
 if (virStorageBackendRBDOpenIoCTX(, pool) < 0)
 goto cleanup;
 
+r = rbd_open(ptr.ioctx, vol->name, , 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, _snaps);
+if (snap_count <= 0) {
+VIR_FREE(snaps);
+}
+} while (snap_count == -ERANGE);
+
+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_FORCED)) {
+for (i = 0; i < snap_count; i++) {
+if (rbd_snap_is_protected(image, snaps[i].name, ))
+goto cleanup;
+
+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;
+}
+
+VIR_DEBUG("Removing snapshot %s/%s@%s", pool->def->source.name,
+  vol->name, snaps[i].name);
+
+if (rbd_snap_remove(image, snaps[i].name) < 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;
+
+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 +510,7 @@ static int virStorageBackendRBDDeleteVol(virConnectPtr conn,
 ret = 0;
 
  cleanup:
+VIR_FREE(snaps);
 virStorageBackendRBDCloseRADOSConn();
 return ret;
 }
-- 
1.9.1

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