2:44 a.m.
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
2:44 a.m.
New subject: [PATCH v1 1/4] conf: add snapshotName attribute for internal disk snapshot
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(a)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
2:44 a.m.
New subject: [PATCH v1 2/4] qemu: Support active disk internal snapshots
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(a)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
2:44 a.m.
New subject: [PATCH v1 3/4] qemu: allow mixing active internal and external active disk snapshots
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(a)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
2:44 a.m.
New subject: [PATCH v1 4/4] qemu: switch offline internal disk snapshots to use snapshotName
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(a)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
9:21 a.m.
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.
5:41 a.m.
-----Original Message-----
From: Peter Krempa <pkrempa(a)redhat.com>
Sent: Friday, 13 January 2023 17:21
To: Or Ozeri <ORO(a)il.ibm.com>
Cc: libvir-list(a)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)
6:08 a.m.
-----Original Message-----
From: Or Ozeri
Sent: Sunday, 15 January 2023 13:41
To: Peter Krempa <pkrempa(a)redhat.com>
Cc: libvir-list(a)redhat.com; Danny Harnik <DANNYH(a)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.
6:40 a.m.
On Sun, Jan 15, 2023 at 11:41:01 +0000, Or Ozeri wrote:
> -----Original Message-----
> From: Peter Krempa <pkrempa(a)redhat.com>
> Sent: Friday, 13 January 2023 17:21
> To: Or Ozeri <ORO(a)il.ibm.com>
> Cc: libvir-list(a)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)
676
days inactive
680
days old
8 comments
3 participants
participants (3)
-
Or Ozeri -
Or Ozeri
-
Peter Krempa