[libvirt] [PATCH] rbd: Remove snapshots if the FORCED flag has been provided
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 <wido@widodh.nl> --- 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(&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; + } + + 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, + 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, &protected)) + 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(&ptr); return ret; } -- 1.9.1
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 <wido@widodh.nl> --- 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 :|
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 <wido@widodh.nl> --- 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
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 <wido@widodh.nl> --- 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(&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; + } + + 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, + 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, &protected)) + 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 :|
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@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 */ } 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; 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; + 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); + } + } 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_WITH_SNAPSHOTS)) { + for (i = 0; i < snap_count; i++) { + if (rbd_snap_is_protected(image, snaps[i].name, &protected)) + 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); + + 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; + + 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; } -- 1.9.1
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@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; }
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@widodh.nl> --- include/libvirt/libvirt-storage.h | 1 + src/storage/storage_backend_rbd.c | 91 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+) 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); + 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; } -- 1.9.1
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@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; }
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@widodh.nl> --- include/libvirt/libvirt-storage.h | 1 + src/storage/storage_backend_rbd.c | 89 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+) 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..a37d286 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -421,6 +421,87 @@ static int virStorageBackendRBDRefreshPool(virConnectPtr conn, return ret; } +static int virStorageBackendRBDCleanupSnapshots(rados_ioctx_t ioctx, + 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; + rbd_image_t image = NULL; + + r = rbd_open(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); + + } 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); + + if (image) + rbd_close(image); + + return ret; +} + static int virStorageBackendRBDDeleteVol(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol, @@ -443,6 +524,14 @@ static int virStorageBackendRBDDeleteVol(virConnectPtr conn, if (virStorageBackendRBDOpenIoCTX(&ptr, pool) < 0) goto cleanup; + if (flags & VIR_STORAGE_VOL_DELETE_WITH_SNAPSHOTS) { + if (virStorageBackendRBDCleanupSnapshots(ptr.ioctx, &pool->def->source, + vol) < 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'"), -- 1.9.1
On 10/27/2015 10:16 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@widodh.nl> --- include/libvirt/libvirt-storage.h | 1 + src/storage/storage_backend_rbd.c | 89 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+)
With one minor adjustment shown below to initialize snaps, this is now pushed. John
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..a37d286 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -421,6 +421,87 @@ static int virStorageBackendRBDRefreshPool(virConnectPtr conn, return ret; }
+static int virStorageBackendRBDCleanupSnapshots(rados_ioctx_t ioctx, + 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;
snaps = NULL
+ rbd_image_t image = NULL; + + r = rbd_open(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); + + } 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); + + if (image) + rbd_close(image); + + return ret; +} + static int virStorageBackendRBDDeleteVol(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol, @@ -443,6 +524,14 @@ static int virStorageBackendRBDDeleteVol(virConnectPtr conn, if (virStorageBackendRBDOpenIoCTX(&ptr, pool) < 0) goto cleanup;
+ if (flags & VIR_STORAGE_VOL_DELETE_WITH_SNAPSHOTS) { + if (virStorageBackendRBDCleanupSnapshots(ptr.ioctx, &pool->def->source, + vol) < 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'"),
participants (3)
-
Daniel P. Berrange -
John Ferlan -
Wido den Hollander