[PATCH v2 0/6] Introduce active RBD disk snapshot support
Internal disk snapshots are currently only supported on non-active VMs. For RBD disks only, this patch series extends this support for active VMs running with qemu. We also add the option to set a name for each RBD snapshot, and allow taking them alongside other external disk snapshots (mixing). Deletion and reverting to snapshots containing RBD snapshots is not allowed, and is validated. Note that taking RBD snapshots on a non-active VM is still unsupported. Changes in v2: - reduce patch to RBD use-case only (e.g. not including qcow2 internal snapshots) - add validation to disallow RBD snapshots deletion / reverting Or Ozeri (6): conf: Add snapshotName attribute for internal disk snapshot qemu: Block deletion and reverting on non-full internal snapshots qemu: Add internal support for active disk internal snapshots qemu: Allow active disk snapshots for RBD disks qemu: Allow setting per-disk snapshot name for RBD disks qemu: Allow mixing active internal and external active disk snapshots docs/formatsnapshot.rst | 5 + src/conf/schemas/domainsnapshot.rng | 4 + src/conf/snapshot_conf.c | 56 ++++++++++ src/conf/snapshot_conf.h | 5 + src/libvirt_private.syms | 1 + src/qemu/qemu_monitor.c | 9 ++ src/qemu/qemu_monitor.h | 5 + src/qemu/qemu_monitor_json.c | 14 +++ src/qemu/qemu_monitor_json.h | 5 + src/qemu/qemu_snapshot.c | 105 ++++++++++++------ .../disk_snapshot.xml | 2 +- .../disk_snapshot.xml | 2 +- .../disk_snapshot_redefine.xml | 2 +- tests/qemumonitorjsontest.c | 1 + 14 files changed, 181 insertions(+), 35 deletions(-) -- 2.25.1
Add a new attribute to snapshot disks XML element, which allows specifying the name that will be used to refer to internal snapshots. If unspecified, the parent snapshot name will be used. For now, make this attribute unsupported for qemu domains. Signed-off-by: Or Ozeri <oro@il.ibm.com> --- docs/formatsnapshot.rst | 5 +++++ src/conf/schemas/domainsnapshot.rng | 4 ++++ src/conf/snapshot_conf.c | 27 +++++++++++++++++++++++++++ src/conf/snapshot_conf.h | 2 ++ src/qemu/qemu_snapshot.c | 9 +++++++++ 5 files changed, 47 insertions(+) diff --git a/docs/formatsnapshot.rst b/docs/formatsnapshot.rst index 085c712053..d98066dd8c 100644 --- a/docs/formatsnapshot.rst +++ b/docs/formatsnapshot.rst @@ -145,6 +145,11 @@ The top-level ``domainsnapshot`` element may contain the following elements: driver and supported values are ``file``, ``block`` and ``network`` :since:`(since 1.2.2)`. + :since:`Since 9.1.0` the ``disk`` element supports an optional attribute + ``snapshotName`` if the ``snapshot`` attribute is set to ``internal``. This + attribute specifies the name that will be used to refer to the + internal disk snapshot. + ``source`` If the snapshot mode is external (whether specified or inherited), diff --git a/src/conf/schemas/domainsnapshot.rng b/src/conf/schemas/domainsnapshot.rng index 4048266f1d..19f097d2b3 100644 --- a/src/conf/schemas/domainsnapshot.rng +++ b/src/conf/schemas/domainsnapshot.rng @@ -209,6 +209,10 @@ <value>manual</value> </attribute> </choice> + <optional> + <attribute name="snapshotName"> + </attribute> + </optional> </element> </define> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 9bf3c78353..58a6afa26d 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -150,6 +150,8 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, VIR_STORAGE_TYPE_FILE) < 0) return -1; + def->snapshot_name = virXMLPropString(node, "snapshotName"); + if (src->type == VIR_STORAGE_TYPE_VOLUME || src->type == VIR_STORAGE_TYPE_DIR) { virReportError(VIR_ERR_XML_ERROR, @@ -192,6 +194,10 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, (src->path || src->format)) def->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; + if (def->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT && + def->snapshot_name) + def->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; + def->name = g_steal_pointer(&name); def->src = g_steal_pointer(&src); @@ -670,6 +676,15 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDef *snapdef, return -1; } + if (snapdisk->snapshot_name && + snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("snapshotName for disk '%s' requires " + "use of internal snapshot mode"), + snapdisk->name); + return -1; + } + if (snapdisk->src->path && snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -714,6 +729,16 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDef *snapdef, if (virDomainSnapshotDefAssignExternalNames(snapdef) < 0) return -1; + /* set default snapshot name for internal snapshots */ + for (i = 0; i < snapdef->ndisks; i++) { + virDomainSnapshotDiskDef * disk = &snapdef->disks[i]; + + if (disk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL && + !disk->snapshot_name) { + disk->snapshot_name = g_strdup(snapdef->parent.name); + } + } + return 0; } @@ -748,6 +773,8 @@ virDomainSnapshotDiskDefFormat(virBuffer *buf, if (disk->snapshot > 0) virBufferAsprintf(&attrBuf, " snapshot='%s'", virDomainSnapshotLocationTypeToString(disk->snapshot)); + if (disk->snapshot_name) + virBufferAsprintf(&attrBuf, " snapshotName='%s'", disk->snapshot_name); if (disk->snapshotDeleteInProgress) virBufferAddLit(&childBuf, "<snapshotDeleteInProgress/>\n"); diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 96c77ef42b..c133c105c7 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -57,6 +57,8 @@ struct _virDomainSnapshotDiskDef { /* details of wrapper external file. src is always non-NULL. * XXX optimize this to allow NULL for internal snapshots? */ virStorageSource *src; + + char *snapshot_name; /* snapshot name for internal snapshots */ }; void diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index b8416808b3..1aa2f05300 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -660,6 +660,15 @@ qemuSnapshotPrepare(virDomainObj *vm, virStorageFileFormatTypeToString(dom_disk->src->format)); return -1; } + + if (disk->snapshot_name) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("snapshot name setting for disk %s unsupported " + "for storage type %s"), + disk->name, + virStorageFileFormatTypeToString(dom_disk->src->format)); + return -1; + } break; case VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL: -- 2.25.1
An upcoming commit will add support for creating a disks-only snapshot using internal disk snapshots. Deleting or reverting to these will not be supported, at least not for now. This commit adds a validation for this. Signed-off-by: Or Ozeri <oro@il.ibm.com> --- src/conf/snapshot_conf.c | 29 +++++++++++++++++++++++++++++ src/conf/snapshot_conf.h | 3 +++ src/libvirt_private.syms | 1 + src/qemu/qemu_snapshot.c | 12 ++++++++++++ 4 files changed, 45 insertions(+) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 58a6afa26d..879fe4c7a1 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -945,6 +945,35 @@ virDomainSnapshotIsExternal(virDomainMomentObj *snap) return virDomainSnapshotDefIsExternal(def); } +bool +virDomainSnapshotDefIsNonFullInternal(virDomainSnapshotDef *def) +{ + bool has_internal = false; + bool is_full = true; + size_t i; + + for (i = 0; i < def->ndisks; i++) { + if (def->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL) { + has_internal = true; + } else { + is_full = false; + } + } + + if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL) + return !is_full; + + return has_internal; +} + +bool +virDomainSnapshotIsNonFullInternal(virDomainMomentObj *snap) +{ + virDomainSnapshotDef *def = virDomainSnapshotObjGetDef(snap); + + return virDomainSnapshotDefIsNonFullInternal(def); +} + int virDomainSnapshotRedefinePrep(virDomainObj *vm, virDomainSnapshotDef *snapdef, diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index c133c105c7..8f40748602 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -125,6 +125,9 @@ int virDomainSnapshotAlignDisks(virDomainSnapshotDef *snapshot, bool virDomainSnapshotDefIsExternal(virDomainSnapshotDef *def); bool virDomainSnapshotIsExternal(virDomainMomentObj *snap); +bool virDomainSnapshotDefIsNonFullInternal(virDomainSnapshotDef *def); +bool virDomainSnapshotIsNonFullInternal(virDomainMomentObj *snap); + int virDomainSnapshotRedefinePrep(virDomainObj *vm, virDomainSnapshotDef *snapdef, virDomainMomentObj **snap, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 576ec8f95f..0d38e86936 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1027,6 +1027,7 @@ virDomainSnapshotDiskDefFree; virDomainSnapshotDiskDefParseXML; virDomainSnapshotFormatConvertXMLFlags; virDomainSnapshotIsExternal; +virDomainSnapshotIsNonFullInternal; virDomainSnapshotRedefinePrep; virDomainSnapshotStateTypeFromString; virDomainSnapshotStateTypeToString; diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 1aa2f05300..c1855b3028 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1828,6 +1828,12 @@ qemuSnapshotRevertValidate(virDomainObj *vm, return -1; } + if (virDomainSnapshotIsNonFullInternal(snap)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("revert to a non-full internal snapshot not supported yet")); + return -1; + } + if (!snap->def->dom) { virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, _("snapshot '%s' lacks domain '%s' rollback info"), @@ -3061,6 +3067,12 @@ qemuSnapshotDeleteValidate(virDomainObj *vm, virDomainMomentObj *snap, unsigned int flags) { + if (virDomainSnapshotIsNonFullInternal(snap)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("deletion of a non-full internal snapshot not supported yet")); + return -1; + } + if (!virDomainSnapshotIsExternal(snap) && virDomainObjIsActive(vm)) { ssize_t i; -- 2.25.1
libvirt supports taking external disk snapshots on a running VM, using qemu's "blockdev-snapshot" command. qemu also supports "blockdev-snapshot-internal-sync" to do the same for internal snapshots. This commit wraps this (old) qemu capability to allow future libvirt users to take internal disk snapshots on a running VM. This will only work for disk types which support internal snapshots, and thus we require the disk type to be part of a white list of known types. For this commit, the list of supported formats is empty. An upcoming commit will allow RBD disks to use this new capability. Signed-off-by: Or Ozeri <oro@il.ibm.com> --- src/qemu/qemu_monitor.c | 9 +++ src/qemu/qemu_monitor.h | 5 ++ src/qemu/qemu_monitor_json.c | 14 ++++ src/qemu/qemu_monitor_json.h | 5 ++ src/qemu/qemu_snapshot.c | 72 ++++++++++++------- .../disk_snapshot.xml | 2 +- .../disk_snapshot.xml | 2 +- .../disk_snapshot_redefine.xml | 2 +- tests/qemumonitorjsontest.c | 1 + 9 files changed, 82 insertions(+), 30 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 38f89167e0..f6dab34243 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4225,6 +4225,15 @@ qemuMonitorTransactionSnapshotBlockdev(virJSONValue *actions, } +int +qemuMonitorTransactionInternalSnapshotBlockdev(virJSONValue *actions, + const char *device, + const char *name) +{ + return qemuMonitorJSONTransactionInternalSnapshotBlockdev(actions, device, name); +} + + int qemuMonitorTransactionBackup(virJSONValue *actions, const char *device, diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 2d16214ba2..1bfd1ccbc2 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1411,6 +1411,11 @@ qemuMonitorTransactionSnapshotBlockdev(virJSONValue *actions, const char *node, const char *overlay); +int +qemuMonitorTransactionInternalSnapshotBlockdev(virJSONValue *actions, + const char *device, + const char *name); + typedef enum { QEMU_MONITOR_TRANSACTION_BACKUP_SYNC_MODE_NONE = 0, QEMU_MONITOR_TRANSACTION_BACKUP_SYNC_MODE_INCREMENTAL, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index db99017555..002a6caa52 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -8307,6 +8307,20 @@ qemuMonitorJSONTransactionSnapshotBlockdev(virJSONValue *actions, NULL); } + +int +qemuMonitorJSONTransactionInternalSnapshotBlockdev(virJSONValue *actions, + const char *device, + const char *name) +{ + return qemuMonitorJSONTransactionAdd(actions, + "blockdev-snapshot-internal-sync", + "s:device", device, + "s:name", name, + NULL); +} + + VIR_ENUM_DECL(qemuMonitorTransactionBackupSyncMode); VIR_ENUM_IMPL(qemuMonitorTransactionBackupSyncMode, QEMU_MONITOR_TRANSACTION_BACKUP_SYNC_MODE_LAST, diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 6f376cf9b7..313004f327 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -779,6 +779,11 @@ qemuMonitorJSONTransactionSnapshotBlockdev(virJSONValue *actions, const char *node, const char *overlay); +int +qemuMonitorJSONTransactionInternalSnapshotBlockdev(virJSONValue *actions, + const char *device, + const char *name); + int qemuMonitorJSONTransactionBackup(virJSONValue *actions, const char *device, diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index c1855b3028..e82352ba7d 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -736,7 +736,7 @@ qemuSnapshotPrepare(virDomainObj *vm, } /* disk snapshot requires at least one disk */ - if (def->state == VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT && !external) { + if (def->state == VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT && !external && !found_internal) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("disk-only snapshots require at least " "one disk to be selected for snapshot")); @@ -852,6 +852,7 @@ qemuSnapshotDiskCleanup(qemuSnapshotDiskData *data, struct _qemuSnapshotDiskContext { qemuSnapshotDiskData *dd; size_t ndd; + bool has_internal; virJSONValue *actions; @@ -1070,17 +1071,17 @@ qemuSnapshotDiskPrepareOne(qemuSnapshotDiskContext *snapctxt, /** - * qemuSnapshotDiskPrepareActiveExternal: + * qemuSnapshotDiskPrepareActive: * * Collects and prepares a list of structures that hold information about disks * that are selected for the snapshot. */ static qemuSnapshotDiskContext * -qemuSnapshotDiskPrepareActiveExternal(virDomainObj *vm, - virDomainMomentObj *snap, - bool reuse, - GHashTable *blockNamedNodeData, - virDomainAsyncJob asyncJob) +qemuSnapshotDiskPrepareActive(virDomainObj *vm, + virDomainMomentObj *snap, + bool reuse, + GHashTable *blockNamedNodeData, + virDomainAsyncJob asyncJob) { g_autoptr(qemuSnapshotDiskContext) snapctxt = NULL; size_t i; @@ -1089,16 +1090,33 @@ qemuSnapshotDiskPrepareActiveExternal(virDomainObj *vm, snapctxt = qemuSnapshotDiskContextNew(snapdef->ndisks, vm, asyncJob); for (i = 0; i < snapdef->ndisks; i++) { - if (snapdef->disks[i].snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) - continue; + switch (snapdef->disks[i].snapshot) { + case VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL: { + if (qemuSnapshotDiskPrepareOne(snapctxt, + vm->def->disks[i], + snapdef->disks + i, + blockNamedNodeData, + reuse, + true) < 0) + return NULL; + break; + } - if (qemuSnapshotDiskPrepareOne(snapctxt, - vm->def->disks[i], - snapdef->disks + i, - blockNamedNodeData, - reuse, - true) < 0) - return NULL; + case VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL: { + snapctxt->has_internal = true; + if (qemuMonitorTransactionInternalSnapshotBlockdev(snapctxt->actions, + vm->def->disks[i]->src->nodeformat, + snapdef->disks[i].snapshot_name) < 0) + return NULL; + break; + } + + case VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT: + case VIR_DOMAIN_SNAPSHOT_LOCATION_NO: + case VIR_DOMAIN_SNAPSHOT_LOCATION_MANUAL: + case VIR_DOMAIN_SNAPSHOT_LOCATION_LAST: + continue; + } } return g_steal_pointer(&snapctxt); @@ -1182,7 +1200,7 @@ qemuSnapshotDiskCreate(qemuSnapshotDiskContext *snapctxt) int rc; /* check whether there's anything to do */ - if (snapctxt->ndd == 0) + if (snapctxt->ndd == 0 && !snapctxt->has_internal) return 0; if (qemuDomainObjEnterMonitorAsync(snapctxt->vm, snapctxt->asyncJob) < 0) @@ -1215,11 +1233,11 @@ qemuSnapshotDiskCreate(qemuSnapshotDiskContext *snapctxt) /* The domain is expected to be locked and active. */ static int -qemuSnapshotCreateActiveExternalDisks(virDomainObj *vm, - virDomainMomentObj *snap, - GHashTable *blockNamedNodeData, - unsigned int flags, - virDomainAsyncJob asyncJob) +qemuSnapshotCreateActiveDisks(virDomainObj *vm, + virDomainMomentObj *snap, + GHashTable *blockNamedNodeData, + unsigned int flags, + virDomainAsyncJob asyncJob) { bool reuse = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0; g_autoptr(qemuSnapshotDiskContext) snapctxt = NULL; @@ -1229,8 +1247,8 @@ qemuSnapshotCreateActiveExternalDisks(virDomainObj *vm, /* prepare a list of objects to use in the vm definition so that we don't * have to roll back later */ - if (!(snapctxt = qemuSnapshotDiskPrepareActiveExternal(vm, snap, reuse, - blockNamedNodeData, asyncJob))) + if (!(snapctxt = qemuSnapshotDiskPrepareActive(vm, snap, reuse, + blockNamedNodeData, asyncJob))) return -1; if (qemuSnapshotDiskCreate(snapctxt) < 0) @@ -1370,9 +1388,9 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver, /* the domain is now paused if a memory snapshot was requested */ - if ((ret = qemuSnapshotCreateActiveExternalDisks(vm, snap, - blockNamedNodeData, flags, - VIR_ASYNC_JOB_SNAPSHOT)) < 0) + if ((ret = qemuSnapshotCreateActiveDisks(vm, snap, + blockNamedNodeData, flags, + VIR_ASYNC_JOB_SNAPSHOT)) < 0) goto cleanup; /* the snapshot is complete now */ diff --git a/tests/qemudomainsnapshotxml2xmlin/disk_snapshot.xml b/tests/qemudomainsnapshotxml2xmlin/disk_snapshot.xml index cf5ea0814e..87b6251a7f 100644 --- a/tests/qemudomainsnapshotxml2xmlin/disk_snapshot.xml +++ b/tests/qemudomainsnapshotxml2xmlin/disk_snapshot.xml @@ -4,7 +4,7 @@ <disks> <disk name='/dev/HostVG/QEMUGuest1'/> <disk name='hdb' snapshot='no'/> - <disk name='hdc' snapshot='internal'/> + <disk name='hdc' snapshot='internal' snapshotName='snap1'/> <disk name='hdd' snapshot='external'> <source/> <driver type='qed'/> diff --git a/tests/qemudomainsnapshotxml2xmlout/disk_snapshot.xml b/tests/qemudomainsnapshotxml2xmlout/disk_snapshot.xml index 76c543d25c..6cf93183d5 100644 --- a/tests/qemudomainsnapshotxml2xmlout/disk_snapshot.xml +++ b/tests/qemudomainsnapshotxml2xmlout/disk_snapshot.xml @@ -5,7 +5,7 @@ <disks> <disk name='/dev/HostVG/QEMUGuest1'/> <disk name='hdb' snapshot='no'/> - <disk name='hdc' snapshot='internal'/> + <disk name='hdc' snapshot='internal' snapshotName='snap1'/> <disk name='hdd' snapshot='external' type='file'> <driver type='qed'/> </disk> diff --git a/tests/qemudomainsnapshotxml2xmlout/disk_snapshot_redefine.xml b/tests/qemudomainsnapshotxml2xmlout/disk_snapshot_redefine.xml index 24b41ba7c5..f574793edf 100644 --- a/tests/qemudomainsnapshotxml2xmlout/disk_snapshot_redefine.xml +++ b/tests/qemudomainsnapshotxml2xmlout/disk_snapshot_redefine.xml @@ -10,7 +10,7 @@ <disks> <disk name='hda' snapshot='no'/> <disk name='hdb' snapshot='no'/> - <disk name='hdc' snapshot='internal'/> + <disk name='hdc' snapshot='internal' snapshotName='snap1'/> <disk name='hdd' snapshot='external' type='file'> <driver type='qed'/> <source file='/path/to/generated4'/> diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 1db1f2b949..1269c74e43 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -2587,6 +2587,7 @@ testQemuMonitorJSONTransaction(const void *opaque) qemuMonitorTransactionBitmapDisable(actions, "node4", "bitmap4") < 0 || qemuMonitorTransactionBitmapMerge(actions, "node5", "bitmap5", &mergebitmaps) < 0 || qemuMonitorTransactionSnapshotBlockdev(actions, "node7", "overlay7") < 0 || + qemuMonitorTransactionInternalSnapshotBlockdev(actions, "device1", "snapshot1") < 0 || qemuMonitorTransactionBackup(actions, "dev8", "job8", "target8", "bitmap8", QEMU_MONITOR_TRANSACTION_BACKUP_SYNC_MODE_NONE) < 0 || qemuMonitorTransactionBackup(actions, "dev9", "job9", "target9", "bitmap9", -- 2.25.1
On Wed, Feb 15, 2023 at 05:28:19 -0600, Or Ozeri wrote:
libvirt supports taking external disk snapshots on a running VM, using qemu's "blockdev-snapshot" command. qemu also supports "blockdev-snapshot-internal-sync" to do the same for internal snapshots. This commit wraps this (old) qemu capability to allow future libvirt users to take internal disk snapshots on a running VM. This will only work for disk types which support internal snapshots, and thus we require the disk type to be part of a white list of known types. For this commit, the list of supported formats is empty. An upcoming commit will allow RBD disks to use this new capability.
Signed-off-by: Or Ozeri <oro@il.ibm.com> ---
[...] There's too much going on in this commit: - adds new monitor API and its tests - renames function for preparing snapshot data - adds the code to do internal snapshot into it - modifies checks to report error - modifies the external memory snapshot code to work with the new snapshot Split out at least the rename and monitor addition into two separate patches. Similarly the bit which shuffles around and prepares the snapshot data can be done as a separate commit.
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index c1855b3028..e82352ba7d 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -736,7 +736,7 @@ qemuSnapshotPrepare(virDomainObj *vm,
[...]
@@ -1229,8 +1247,8 @@ qemuSnapshotCreateActiveExternalDisks(virDomainObj *vm,
/* prepare a list of objects to use in the vm definition so that we don't * have to roll back later */ - if (!(snapctxt = qemuSnapshotDiskPrepareActiveExternal(vm, snap, reuse, - blockNamedNodeData, asyncJob))) + if (!(snapctxt = qemuSnapshotDiskPrepareActive(vm, snap, reuse, + blockNamedNodeData, asyncJob))) return -1;
if (qemuSnapshotDiskCreate(snapctxt) < 0)
[...]
@@ -1370,9 +1388,9 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver,
/* the domain is now paused if a memory snapshot was requested */
- if ((ret = qemuSnapshotCreateActiveExternalDisks(vm, snap, - blockNamedNodeData, flags, - VIR_ASYNC_JOB_SNAPSHOT)) < 0) + if ((ret = qemuSnapshotCreateActiveDisks(vm, snap, + blockNamedNodeData, flags, + VIR_ASYNC_JOB_SNAPSHOT)) < 0)
This modification is done to a function named `qemuSnapshotCreateActiveExternal` but clearly enabled creation of internal snapshots. You'll need to address that one too. Since your patches add intermingling (at least) partial of snapshots the code will need to be split up separately. 1) check that the old-style full internal snapshot is requested and deal with that as a special case 2) separate qemuSnapshotCreateActiveExternal into: - 2.1) - the bit that creates the external memory snapshot - 2.2) - the bit that creates the external disk snapshot - 2.3) - modify the bit that creates external disk snapshot to also do the internal disk snapshot for rbd - 2.4) - call them in proper sequence. I suspect you also want to do a external memory snapshot including the internal RBD snapshot as s transaction. Good bit is that the oldschool internal snapshot is a completely separate case and can't be intermingled here. But the existing code must be modified to honour the change of semantics you are introducing.
-----Original Message----- From: Peter Krempa <pkrempa@redhat.com> Sent: Monday, 20 February 2023 16:00 To: Or Ozeri <ORO@il.ibm.com> Cc: libvir-list@redhat.com; Danny Harnik <DANNYH@il.ibm.com> Subject: [EXTERNAL] Re: [PATCH v2 3/6] qemu: Add internal support for active disk internal snapshots
This modification is done to a function named `qemuSnapshotCreateActiveExternal` but clearly enabled creation of internal snapshots. You'll need to address that one too.
By address, you mean rename? It treats both external and disks-only (which can be internal / external / mixed) So maybe qemuSnapshotCreateActiveExternalOrDisksOnly? I can't think of a neat name for it :\
Since your patches add intermingling (at least) partial of snapshots the code will need to be split up separately.
1) check that the old-style full internal snapshot is requested and deal with that as a special case
This check already exists, so I guess no change is required?
2) separate qemuSnapshotCreateActiveExternal into: - 2.1) - the bit that creates the external memory snapshot - 2.2) - the bit that creates the external disk snapshot
You mean separate each of the bits mentioned above to a new function? The bit that creates the memory snapshot also pauses the VM, and unpause after all is done (including the disks snapshot). I'm guessing you do not want to include this as part of the 2.1 mentioned above?
- 2.3) - modify the bit that creates external disk snapshot to also do the internal disk snapshot for rbd - 2.4) - call them in proper sequence. I suspect you also want to do a external memory snapshot including the internal RBD snapshot as s transaction.
Actually no, there is no current use-case for that (that I know of).
Good bit is that the oldschool internal snapshot is a completely separate case and can't be intermingled here.
But the existing code must be modified to honour the change of semantics you are introducing.
This commit removes the check disallowing users to take active disk-only snapshots of RBD disks. The actual support for this functionality was added in a previous commit. Signed-off-by: Or Ozeri <oro@il.ibm.com> --- src/qemu/qemu_snapshot.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index e82352ba7d..a10bdf7bf2 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -630,6 +630,8 @@ qemuSnapshotPrepare(virDomainObj *vm, for (i = 0; i < def->ndisks; i++) { virDomainSnapshotDiskDef *disk = &def->disks[i]; virDomainDiskDef *dom_disk = vm->def->disks[i]; + bool is_raw_rbd = (dom_disk->src->format == VIR_STORAGE_FILE_RAW && + dom_disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD); if (disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_NO && qemuDomainDiskBlockJobIsActive(dom_disk)) @@ -639,7 +641,7 @@ qemuSnapshotPrepare(virDomainObj *vm, case VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL: found_internal = true; - if (def->state == VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT && active) { + if (def->state == VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT && active && !is_raw_rbd) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("active qemu domains require external disk " "snapshots; disk %s requested internal"), @@ -652,7 +654,8 @@ qemuSnapshotPrepare(virDomainObj *vm, return -1; if (dom_disk->src->format > 0 && - dom_disk->src->format != VIR_STORAGE_FILE_QCOW2) { + dom_disk->src->format != VIR_STORAGE_FILE_QCOW2 && + !is_raw_rbd) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("internal snapshot for disk %s unsupported " "for storage type %s"), -- 2.25.1
On Wed, Feb 15, 2023 at 05:28:20 -0600, Or Ozeri wrote:
This commit removes the check disallowing users to take active disk-only snapshots of RBD disks. The actual support for this functionality was added in a previous commit.
Signed-off-by: Or Ozeri <oro@il.ibm.com> --- src/qemu/qemu_snapshot.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index e82352ba7d..a10bdf7bf2 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -630,6 +630,8 @@ qemuSnapshotPrepare(virDomainObj *vm, for (i = 0; i < def->ndisks; i++) { virDomainSnapshotDiskDef *disk = &def->disks[i]; virDomainDiskDef *dom_disk = vm->def->disks[i]; + bool is_raw_rbd = (dom_disk->src->format == VIR_STORAGE_FILE_RAW && + dom_disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD);
You must not access 'src->protocol' unless you first check that src->type is VIR_STORAGE_TYPE_NETWORK.
This commit adds the option for setting per-disk snapshot name for RBD disks. All other disk types are still disallowed to use the snapshotName attribute. Signed-off-by: Or Ozeri <oro@il.ibm.com> --- src/qemu/qemu_snapshot.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index a10bdf7bf2..c72bdb4723 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -664,7 +664,7 @@ qemuSnapshotPrepare(virDomainObj *vm, return -1; } - if (disk->snapshot_name) { + if (disk->snapshot_name && !is_raw_rbd) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("snapshot name setting for disk %s unsupported " "for storage type %s"), -- 2.25.1
On Wed, Feb 15, 2023 at 05:28:21 -0600, Or Ozeri wrote:
This commit adds the option for setting per-disk snapshot name for RBD disks. All other disk types are still disallowed to use the snapshotName attribute.
Signed-off-by: Or Ozeri <oro@il.ibm.com> --- src/qemu/qemu_snapshot.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index a10bdf7bf2..c72bdb4723 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -664,7 +664,7 @@ qemuSnapshotPrepare(virDomainObj *vm, return -1; }
- if (disk->snapshot_name) { + if (disk->snapshot_name && !is_raw_rbd) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("snapshot name setting for disk %s unsupported " "for storage type %s"),
I don't see anything that updates the equivalent of dom_disk->src->snapshot after the snapshot: The 'snapshot' property is filled as the equivalent property when formatting the backend definition for the 'rbd' disk. In case when the 'snapshot' field is meant to actually mean label the 'old' state. You then must actually tweak the snapshot metadata to point to it. That will allow proper reversion of the image.
-----Original Message----- From: Peter Krempa <pkrempa@redhat.com> Sent: Monday, 20 February 2023 16:13 To: Or Ozeri <ORO@il.ibm.com> Cc: libvir-list@redhat.com; Danny Harnik <DANNYH@il.ibm.com> Subject: [EXTERNAL] Re: [PATCH v2 5/6] qemu: Allow setting per-disk snapshot name for RBD disks
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index a10bdf7bf2..c72bdb4723 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -664,7 +664,7 @@ qemuSnapshotPrepare(virDomainObj *vm, return -1; }
- if (disk->snapshot_name) { + if (disk->snapshot_name && !is_raw_rbd) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("snapshot name setting for disk %s unsupported " "for storage type %s"),
I don't see anything that updates the equivalent of dom_disk->src-
snapshot after the snapshot:
The 'snapshot' property is filled as the equivalent property when formatting the backend definition for the 'rbd' disk.
In case when the 'snapshot' field is meant to actually mean label the 'old' state. You then must actually tweak the snapshot metadata to point to it. That will allow proper reversion of the image.
We actually block reverting to this type of snapshot, see " revert to a non-full internal snapshot not supported yet" in qemuSnapshotRevertValidate. Given this, is this change still required? Actually I'm not sure I understand. Is setting dom_disk->src->snapshot enough? Or can you please point me to another place in the code where a change is required?
Previous commit added support for active RBD disk snapshots, using the same qmp_transaction used also for external snapshots. The same transaction can be used to mix internal and external snapshots. To allow this, this commit simply removes the check that disallows mixing for active snapshots. Note that previous commits added validation disallowing deletion and reverting of RBD disks. The same validation applies to the mixed type of snapshots introduced by this commit. Signed-off-by: Or Ozeri <oro@il.ibm.com> --- src/qemu/qemu_snapshot.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index c72bdb4723..81b3e94988 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -746,10 +746,9 @@ qemuSnapshotPrepare(virDomainObj *vm, return -1; } - /* For now, we don't allow mixing internal and external disks. - * XXX technically, we could mix internal and external disks for + /* For now, we don't allow mixing internal and external disks for * offline snapshots */ - if ((found_internal && external) || + if ((found_internal && external && !active) || (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL && external) || (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL && found_internal)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", -- 2.25.1
On Wed, Feb 15, 2023 at 05:28:22 -0600, Or Ozeri wrote:
Previous commit added support for active RBD disk snapshots, using the same qmp_transaction used also for external snapshots. The same transaction can be used to mix internal and external snapshots. To allow this, this commit simply removes the check that disallows mixing for active snapshots. Note that previous commits added validation disallowing deletion and reverting of RBD disks. The same validation applies to the mixed type of snapshots introduced by this commit.
Signed-off-by: Or Ozeri <oro@il.ibm.com> --- src/qemu/qemu_snapshot.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index c72bdb4723..81b3e94988 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -746,10 +746,9 @@ qemuSnapshotPrepare(virDomainObj *vm, return -1; }
- /* For now, we don't allow mixing internal and external disks. - * XXX technically, we could mix internal and external disks for + /* For now, we don't allow mixing internal and external disks for * offline snapshots */
The comment is not really correct now: We allow after this patch: - full internal snapshots with internal memory snapshot when all disks are qcow2 - if the VM is online: - a disk-only snapshot - external only - some or all RBD internal snapshots can be combined in - external memory snapshot combined with the above (does not yet work, see below)
- if ((found_internal && external) || + if ((found_internal && external && !active) || (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL && external) || (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL && found_internal)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
What this also does not forbid is a combination of a full-internal old-style snapshot while defining some snapshot names for random RBD disks, which will not work. Additionally when def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL the code will not do what you want. It might even break. In such case we still do want to forbid an internal snapshot even if it is on RBD. Obviously unless you've tested and verified that it actually works. The above also doesn't allow a external memory snapshot being combined with internal RBD snapshots
On Wed, Feb 15, 2023 at 05:28:16 -0600, Or Ozeri wrote:
Internal disk snapshots are currently only supported on non-active VMs. For RBD disks only, this patch series extends this support for active VMs running with qemu. We also add the option to set a name for each RBD snapshot, and allow taking them alongside other external disk snapshots (mixing). Deletion and reverting to snapshots containing RBD snapshots is not allowed, and is validated. Note that taking RBD snapshots on a non-active VM is still unsupported.
Changes in v2: - reduce patch to RBD use-case only (e.g. not including qcow2 internal snapshots) - add validation to disallow RBD snapshots deletion / reverting
Or Ozeri (6): conf: Add snapshotName attribute for internal disk snapshot qemu: Block deletion and reverting on non-full internal snapshots qemu: Add internal support for active disk internal snapshots qemu: Allow active disk snapshots for RBD disks qemu: Allow setting per-disk snapshot name for RBD disks qemu: Allow mixing active internal and external active disk snapshots
Note that also Pavel (CC'd) might want to add more comments as he's currently working on the snapshot code.
Hi Peter, I'm missing some clarifications here: https://listman.redhat.com/archives/libvir-list/2023-March/238546.html https://listman.redhat.com/archives/libvir-list/2023-March/238548.html Could you please have a look? Thanks, Or
-----Original Message----- From: Peter Krempa <pkrempa@redhat.com> Sent: Monday, 20 February 2023 16:24 To: Or Ozeri <ORO@il.ibm.com> Cc: libvir-list@redhat.com; Danny Harnik <DANNYH@il.ibm.com>; Pavel Hrdina <phrdina@redhat.com> Subject: [EXTERNAL] Re: [PATCH v2 0/6] Introduce active RBD disk snapshot support
On Wed, Feb 15, 2023 at 05:28:16 -0600, Or Ozeri wrote:
Internal disk snapshots are currently only supported on non-active VMs. For RBD disks only, this patch series extends this support for active VMs running with qemu. We also add the option to set a name for each RBD snapshot, and allow taking them alongside other external disk snapshots (mixing). Deletion and reverting to snapshots containing RBD snapshots is not allowed, and is validated. Note that taking RBD snapshots on a non-active VM is still unsupported.
Changes in v2: - reduce patch to RBD use-case only (e.g. not including qcow2 internal snapshots) - add validation to disallow RBD snapshots deletion / reverting
Or Ozeri (6): conf: Add snapshotName attribute for internal disk snapshot qemu: Block deletion and reverting on non-full internal snapshots qemu: Add internal support for active disk internal snapshots qemu: Allow active disk snapshots for RBD disks qemu: Allow setting per-disk snapshot name for RBD disks qemu: Allow mixing active internal and external active disk snapshots
Note that also Pavel (CC'd) might want to add more comments as he's currently working on the snapshot code.
participants (3)
-
Or Ozeri -
Or Ozeri
-
Peter Krempa