5:28 a.m.
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
5:28 a.m.
New subject: [PATCH v2 1/6] 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.
If unspecified, the parent snapshot name will be used.
For now, make this attribute unsupported for qemu domains.
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 ++
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
5:28 a.m.
New subject: [PATCH v2 2/6] qemu: Block deletion and reverting on non-full internal snapshots
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(a)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
5:28 a.m.
New subject: [PATCH v2 3/6] qemu: Add internal support for 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 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(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 | 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
8 a.m.
New subject: [PATCH v2 3/6] qemu: Add internal support for active disk internal snapshots
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(a)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.
7:35 a.m.
New subject: [PATCH v2 3/6] qemu: Add internal support for active disk internal snapshots
-----Original Message-----
From: Peter Krempa <pkrempa(a)redhat.com>
Sent: Monday, 20 February 2023 16:00
To: Or Ozeri <ORO(a)il.ibm.com>
Cc: libvir-list(a)redhat.com; Danny Harnik <DANNYH(a)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.
5:28 a.m.
New subject: [PATCH v2 4/6] qemu: Allow active disk snapshots for RBD disks
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(a)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
8:09 a.m.
New subject: [PATCH v2 4/6] qemu: Allow active disk snapshots for RBD disks
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(a)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.
5:28 a.m.
New subject: [PATCH v2 5/6] qemu: Allow setting per-disk snapshot name for RBD disks
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(a)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
8:12 a.m.
New subject: [PATCH v2 5/6] qemu: Allow setting per-disk snapshot name for RBD disks
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(a)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.
7:46 a.m.
New subject: [PATCH v2 5/6] qemu: Allow setting per-disk snapshot name for RBD disks
-----Original Message-----
From: Peter Krempa <pkrempa(a)redhat.com>
Sent: Monday, 20 February 2023 16:13
To: Or Ozeri <ORO(a)il.ibm.com>
Cc: libvir-list(a)redhat.com; Danny Harnik <DANNYH(a)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?
5:28 a.m.
New subject: [PATCH v2 6/6] qemu: Allow mixing active internal and external active disk snapshots
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(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 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
8:22 a.m.
New subject: [PATCH v2 6/6] qemu: Allow mixing active internal and external active disk snapshots
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(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 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
8:23 a.m.
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.
4:43 a.m.
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(a)redhat.com>
Sent: Monday, 20 February 2023 16:24
To: Or Ozeri <ORO(a)il.ibm.com>
Cc: libvir-list(a)redhat.com; Danny Harnik <DANNYH(a)il.ibm.com>; Pavel
Hrdina <phrdina(a)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.
641
days inactive
676
days old
14 comments
3 participants
participants (3)
-
Or Ozeri -
Or Ozeri
-
Peter Krempa