[PATCH v1 0/4] Introduce active disk internal snapshot support
Internal disk snapshots are currently only supported on non-active VMs. This patch series extends this support for active VMs running with qemu. This implementation for the qemu driver works even when there are other disks which ask for external snapshot. Thus we remove the restriction disallowing mixing of internal and external disk snapshots for active VMs. Note that mixing is still disallowed for non-active VMs, as it require a bit more work in a different area of the code. Additionally we add a new attribute to allow the user specifying a unique snapshot name for each internal disk snapshot. In case this optional attribute is not specified, snapshot name will be taken from the domain snapshot name, as it is currently done today. Or Ozeri (4): conf: add snapshotName attribute for internal disk snapshot qemu: Support active disk internal snapshots qemu: allow mixing active internal and external active disk snapshots qemu: switch offline internal disk snapshots to use snapshotName 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_domain.c | 2 +- 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 | 88 +++++++++++-------- .../disk_snapshot.xml | 2 +- .../disk_snapshot.xml | 2 +- .../disk_snapshot_redefine.xml | 2 +- tests/qemumonitorjsontest.c | 1 + 14 files changed, 125 insertions(+), 43 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. 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 ++ 4 files changed, 38 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 -- 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 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 (only RBD disks for start). 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 | 83 +++++++++++-------- .../disk_snapshot.xml | 2 +- .../disk_snapshot.xml | 2 +- .../disk_snapshot_redefine.xml | 2 +- tests/qemumonitorjsontest.c | 1 + 9 files changed, 84 insertions(+), 39 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 b8416808b3..9146ecae2f 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -639,20 +639,13 @@ qemuSnapshotPrepare(virDomainObj *vm, case VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL: found_internal = true; - if (def->state == VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT && active) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("active qemu domains require external disk " - "snapshots; disk %s requested internal"), - disk->name); - return -1; - } - if (qemuSnapshotPrepareDiskInternal(dom_disk, active) < 0) return -1; if (dom_disk->src->format > 0 && - dom_disk->src->format != VIR_STORAGE_FILE_QCOW2) { + dom_disk->src->format != VIR_STORAGE_FILE_QCOW2 && + dom_disk->src->protocol != VIR_STORAGE_NET_PROTOCOL_RBD) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("internal snapshot for disk %s unsupported " "for storage type %s"), @@ -727,7 +720,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")); @@ -843,6 +836,7 @@ qemuSnapshotDiskCleanup(qemuSnapshotDiskData *data, struct _qemuSnapshotDiskContext { qemuSnapshotDiskData *dd; size_t ndd; + bool has_internal; virJSONValue *actions; @@ -1061,17 +1055,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; @@ -1080,16 +1074,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); @@ -1173,7 +1184,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) @@ -1206,11 +1217,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; @@ -1220,8 +1231,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) @@ -1361,9 +1372,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
Previous commit added support for active internal 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. Offline snapshots still do not support mixing. 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 9146ecae2f..da29fa2853 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -727,10 +727,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
Previous commit added snapshotName attribute to internal disk snapshots. This commit changes the qemu domain to use this new attribute for offline internal disk snapshots. Signed-off-by: Or Ozeri <oro@il.ibm.com> --- src/qemu/qemu_domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2eb5653254..9ab1b5b9a3 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7099,7 +7099,7 @@ qemuDomainSnapshotForEachQcow2Raw(virQEMUDriver *driver, for (i = 0; i < ndisks; i++) { g_autoptr(virCommand) cmd = virCommandNewArgList(qemuimgbin, "snapshot", - op, snap->def->name, NULL); + op, snapdef->disks[i].snapshot_name, NULL); int format = virDomainDiskGetFormat(def->disks[i]); /* FIXME: we also need to handle LVM here */ -- 2.25.1
On Thu, Jan 12, 2023 at 02:44:35 -0600, Or Ozeri wrote:
Internal disk snapshots are currently only supported on non-active VMs. This patch series extends this support for active VMs running with qemu.
Could you elaborate how this will be useful? Generally disk only snapshots are of very limited use as when reverting you get to a state equating of a power loss of a real box. This means potentially corrupted files, state and other drawbacks. With external disk only snapshots this was at least somewhat useful as it made the original image read-only and thus accessible with a different process. With internal snapshots you still can't even read that image as it's still locked in write mode, so the utility value is even less.
This implementation for the qemu driver works even when there are other disks which ask for external snapshot. Thus we remove the restriction disallowing mixing of internal and external disk snapshots for active VMs. Note that mixing is still disallowed for non-active VMs, as it require a bit more work in a different area of the code.
Note that a few days ago Pavel pushed his series adding support for deletion of external snapshots. That code heavily depends on the fact that mixing internal/external snapshots was forbidden. Thus the patch in this series will not be acceptable until you fix the code that Pavel added. Also note that Pavel is working on reversion of external snapshots so that work might also colide.
Additionally we add a new attribute to allow the user specifying a unique snapshot name for each internal disk snapshot. In case this optional attribute is not specified, snapshot name will be taken from the domain snapshot name, as it is currently done today.
This bit is potentially dangerous as based on your code you don't seem to be checking that the name is unique across snapshots, which can cause potential problems when names between snapshots collide. This is also something that I feel requires elaboration of how it will be useful.
-----Original Message----- From: Peter Krempa <pkrempa@redhat.com> Sent: Friday, 13 January 2023 17:21 To: Or Ozeri <ORO@il.ibm.com> Cc: libvir-list@redhat.com Subject: [EXTERNAL] Re: [PATCH v1 0/4] Introduce active disk internal snapshot support
Could you elaborate how this will be useful? Generally disk only snapshots are of very limited use as when reverting you get to a state equating of a power loss of a real box.
This means potentially corrupted files, state and other drawbacks.
This will be useful for backup of the disks for disaster recovery. i.e. once the internal snapshots are taken, they will be backed up (using diffs of course) to a different geographic location.
With external disk only snapshots this was at least somewhat useful as it made the original image read-only and thus accessible with a different process.
With internal snapshots you still can't even read that image as it's still locked in write mode, so the utility value is even less.
The use-case we're considering is using RBD disks with raw format. These internal RBD snapshots are read-accessible even while the original disk is still being write-accessed.
This implementation for the qemu driver works even when there are other disks which ask for external snapshot. Thus we remove the restriction disallowing mixing of internal and external disk snapshots for active VMs. Note that mixing is still disallowed for non-active VMs, as it require a bit more work in a different area of the code.
Note that a few days ago Pavel pushed his series adding support for deletion of external snapshots. That code heavily depends on the fact that mixing internal/external snapshots was forbidden.
Thus the patch in this series will not be acceptable until you fix the code that Pavel added.
My code touches only snapshot creation. I see that for deletion, Pavel added a check which yields: "deletion of external and internal children disk snapshots not supported " So I don't think any change is required, unless you want to support a new feature of deletion of mixed snapshots. I don't think we should put a lot of effort into coding a full-matrix of support between any two features (i.e. snapshot deletion, and active internal snapshots) if there's no client use-case justifying this work.
Also note that Pavel is working on reversion of external snapshots so that work might also colide.
Like snapshot deletion, this sounds like something that can work alongside by simply adding a check which will disallow reverting in a mixed case.
This bit is potentially dangerous as based on your code you don't seem to be checking that the name is unique across snapshots, which can cause potential problems when names between snapshots collide.
I actually think you can specify a name for an internal snapshot today, if you set <name> under <domainsnapshot>, so my modification only extends this to allow per-disk name, instead of a single name for all disks. So in theory I think snapshot name can collide even in today's code. Second, IMHO this patch already handles collision to the best extent. If there's a name collision on one of the disks, the blockdev-snapshot-internal-sync for that disk will fail, and the encapsulating qmp_transaction will revert all its actions.
This is also something that I feel requires elaboration of how it will be useful.
Just like external snapshots allowing you to specify a custom path for the new file, some clients wish to use their own naming-convention for internal snapshots (instead of using the ctime names generated by libvirt)
-----Original Message----- From: Or Ozeri Sent: Sunday, 15 January 2023 13:41 To: Peter Krempa <pkrempa@redhat.com> Cc: libvir-list@redhat.com; Danny Harnik <DANNYH@il.ibm.com> Subject: RE: [EXTERNAL] Re: [PATCH v1 0/4] Introduce active disk internal snapshot support
Note that a few days ago Pavel pushed his series adding support for deletion of external snapshots. That code heavily depends on the fact that mixing internal/external snapshots was forbidden.
Thus the patch in this series will not be acceptable until you fix the code that Pavel added.
My code touches only snapshot creation. I see that for deletion, Pavel added a check which yields: "deletion of external and internal children disk snapshots not supported " So I don't think any change is required, unless you want to support a new feature of deletion of mixed snapshots.
I missed that this check is for snapshot children. But I guess a similar validation can be added to qemuSnapshotDeleteValidate, to ensure that user is not trying to delete a mixed snapshot.
On Sun, Jan 15, 2023 at 11:41:01 +0000, Or Ozeri wrote:
-----Original Message----- From: Peter Krempa <pkrempa@redhat.com> Sent: Friday, 13 January 2023 17:21 To: Or Ozeri <ORO@il.ibm.com> Cc: libvir-list@redhat.com Subject: [EXTERNAL] Re: [PATCH v1 0/4] Introduce active disk internal snapshot support
Could you elaborate how this will be useful? Generally disk only snapshots are of very limited use as when reverting you get to a state equating of a power loss of a real box.
This means potentially corrupted files, state and other drawbacks.
This will be useful for backup of the disks for disaster recovery. i.e. once the internal snapshots are taken, they will be backed up (using diffs of course) to a different geographic location.
With external disk only snapshots this was at least somewhat useful as it made the original image read-only and thus accessible with a different process.
With internal snapshots you still can't even read that image as it's still locked in write mode, so the utility value is even less.
The use-case we're considering is using RBD disks with raw format. These internal RBD snapshots are read-accessible even while the original disk is still being write-accessed.
Please make sure to mention that you specifically care about RBD upfront the next time. Okay, that is fair enough then. But allow it just for RBD. As noted below we don't want this mixing for qcow2 internal snapshots as it will overcomplicate deletion and reversion code.
This implementation for the qemu driver works even when there are other disks which ask for external snapshot. Thus we remove the restriction disallowing mixing of internal and external disk snapshots for active VMs. Note that mixing is still disallowed for non-active VMs, as it require a bit more work in a different area of the code.
Note that a few days ago Pavel pushed his series adding support for deletion of external snapshots. That code heavily depends on the fact that mixing internal/external snapshots was forbidden.
Thus the patch in this series will not be acceptable until you fix the code that Pavel added.
My code touches only snapshot creation. I see that for deletion, Pavel added a check which yields: "deletion of external and internal children disk snapshots not supported " So I don't think any change is required, unless you want to support a new feature of deletion of mixed snapshots. I don't think we should put a lot of effort into coding a full-matrix of support between any two features (i.e. snapshot deletion, and active internal snapshots) if there's no client use-case justifying this work.
We do not want this mixing for qcow2 based snapshots. It will be too complicated. But since you are dealing with RBD internal snapshots you can allow it sust for those. Obviously we will not be able to delete those.
Also note that Pavel is working on reversion of external snapshots so that work might also colide.
Like snapshot deletion, this sounds like something that can work alongside by simply adding a check which will disallow reverting in a mixed case.
This bit is potentially dangerous as based on your code you don't seem to be checking that the name is unique across snapshots, which can cause potential problems when names between snapshots collide.
I actually think you can specify a name for an internal snapshot today, if you set <name> under <domainsnapshot>, so my modification only extends this to allow per-disk name, instead of a single name for all disks. So in theory I think snapshot name can collide even in today's code. Second, IMHO this patch already handles collision to the best extent. If there's a name collision on one of the disks, the blockdev-snapshot-internal-sync for that disk will fail, and the encapsulating qmp_transaction will revert all its actions.
The name of the internal snapshot is equal to the <name> but it's the same for all of them. But the check needs to be done inside libvirt. There are configurations in which the QCOW2 internal snapshot is no longer in the topmost image in which case blockdev-snapshot-internal-sync will not fail. We simply don't want to defer this check to qemu as it creates a bunch of unnecessary corner cases. Again you can allow specific snapshot names just for internal RBD snapshots.
This is also something that I feel requires elaboration of how it will be useful.
Just like external snapshots allowing you to specify a custom path for the new file, some clients wish to use their own naming-convention for internal snapshots (instead of using the ctime names generated by libvirt)
participants (3)
-
Or Ozeri -
Or Ozeri
-
Peter Krempa