On 7/2/20 9:40 AM, Peter Krempa wrote:
The semantics of the backup operation don't strictly require that all
disks being backed up are part of the same incremental part (when a disk
was checkpointed/backed up separately or in a different VM), or even
they may not have an previous checkpoint at all (e.g. when the disk
was freshly hotplugged to the vm).
In such cases we can still create a common checkpoint for all of them
and backup differences according to configuration.
This patch adds a per-disk configuration of the checkpoint to do the
incremental backup from via the 'incremental' attribute and allows
perform full backups via the 'backupmode' attribute.
Note that no changes to the qemu driver are necessary to take advantage
of this as we already obey the per-disk 'incremental' field.
https://bugzilla.redhat.com/show_bug.cgi?id=1829829
Signed-off-by: Peter Krempa <pkrempa@xxxxxxxxxx>
---
docs/formatbackup.rst | 11 ++++
docs/schemas/domainbackup.rng | 16 ++++++
src/conf/backup_conf.c | 57 +++++++++++++++++++-
src/conf/backup_conf.h | 11 ++++
tests/domainbackupxml2xmlin/backup-pull.xml | 12 +++++
tests/domainbackupxml2xmlout/backup-pull.xml | 12 +++++
6 files changed, 118 insertions(+), 1 deletion(-)
diff --git a/docs/formatbackup.rst b/docs/formatbackup.rst
index 66583f562b..e5b6fc6eb0 100644
--- a/docs/formatbackup.rst
+++ b/docs/formatbackup.rst
@@ -65,6 +65,17 @@ were supplied). The following child elements and attributes are supported:
should take part in the backup and using ``no`` excludes the disk from
the backup.
+ ``backupmode``
+ This attribute overrides the implied backup mode inherited from the
+ definition of the backup itself. Value ``full`` forces a full backup
+ even if the backup calls for an incremental backup and ``incremental``
s/backup and/backup, and/
+ coupled with the attribute ``incremental='CHECKPOINTNAME`` for the disk
+ forces an incremental backup from ``CHECKPOINTNAME``.
+
+ ``incremental``
+ An optional attribute giving the name of an existing checkpoint of the
+ domain which overrides the one set by the ``<incremental>`` element.
+
``exportname``
Allows modification of the NBD export name for the given disk. By
default equal to disk target. Valid only for pull mode backups.
diff --git a/docs/schemas/domainbackup.rng b/docs/schemas/domainbackup.rng
index 5165175152..650f5cd4c3 100644
--- a/docs/schemas/domainbackup.rng
+++ b/docs/schemas/domainbackup.rng
@@ -89,6 +89,20 @@
</element>
</define>
+ <define name='backupDiskMode'>
+ <optional>
+ <attribute name='backupmode'>
+ <choice>
+ <value>full</value>
+ <value>incremental</value>
+ </choice>
+ </attribute>
+ </optional>
+ <optional>
+ <attribute name='incremental'/>
+ </optional>
+ </define>
As written, you validate:
backupmode="full" incremental="blah"
Better might be:
<define name='backupDiskMode'>
<optional>
<choice>
<attribute name='backupmode'>
<value>full</value>
</attribute>
<group>
<optional>
<attribute name='backupmode'>
<value>incremental</value>
</attribute>
</optional>
<optional>
<attribute name='incremental'/>
</optional>
</broup>
</choice>
</optional>
</define>
which also has the advantage of allowing the user to omit
backupmode='incremental' when supplying incremental='name' (since then
that mode is implied).
Do we need to restrict the set of values that can be supplied for a
incremental name? (That's a bigger issue than just this patch: for
example, do we want to refuse a checkpoint named "../foo"? As long as
checkpoint names don't match directly to file names, we aren't at risk
of a filesystem escape, but starting strict and relaxing later is better
than starting relaxed and wishing we had limited certain patterns after all)
@@ -465,6 +493,24 @@ virDomainBackupAlignDisks(virDomainBackupDefPtr def,
return -1;
}
+ if (backupdisk->backupmode == VIR_DOMAIN_BACKUP_DISK_BACKUP_MODE_FULL &&
+ backupdisk->incremental) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("'full' backup mode incompatible with 'incremental' for disk '%s'"),
+ backupdisk->name);
+ return -1;
+ }
You had to check this manually, instead of letting the .rng file enforce
it for you by the construct I listed above as an alternative.
+
+ if (backupdisk->backupmode == VIR_DOMAIN_BACKUP_DISK_BACKUP_MODE_INCREMENTAL &&
+ !backupdisk->incremental &&
+ !def->incremental) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("'incremental' backup mode of disk '%s' requires setting 'incremental' field for disk or backup"),
+ backupdisk->name);
+ return -1;
+ }
Do we really need to require that the user provides
backupmode='incremental', or if they omit it, can we just imply it based
on the presence of incremental='name'?
+++ b/tests/domainbackupxml2xmlin/backup-pull.xml
@@ -6,5 +6,17 @@
<scratch file='/path/to/file'/>
</disk>
<disk name='hda' backup='no'/>
+ <disk name='vdc' type='file' backupmode='full'>
+ <scratch file='/path/to/file'/>
+ </disk>
So this is a demo of overriding an overall incremental request with a
full for this disk.
+ <disk name='vdd' type='file' backupmode='incremental'>
+ <scratch file='/path/to/file'/>
+ </disk>
What incremental bitmap name do we default to if the overall backupjob
requested full? Or is that an error?
+ <disk name='vde' type='file' backupmode='incremental' incremental='blah'>
+ <scratch file='/path/to/file'/>
+ </disk>
This is a demo of using a different checkpoint for this disk than for
the overall job.
+ <disk name='vdf' type='file' incremental='bleh'>
+ <scratch file='/path/to/file'/>
+ </disk>
And this is a demo of allowing backupmode='incremental' to be skipped
when it makes sense.
</disks>
</domainbackup>
diff --git a/tests/domainbackupxml2xmlout/backup-pull.xml b/tests/domainbackupxml2xmlout/backup-pull.xml
index 24fce9c0e7..d2f84cda7a 100644
--- a/tests/domainbackupxml2xmlout/backup-pull.xml
+++ b/tests/domainbackupxml2xmlout/backup-pull.xml
@@ -6,5 +6,17 @@
<scratch file='/path/to/file'/>
</disk>
<disk name='hda' backup='no'/>
+ <disk name='vdc' backup='yes' type='file' backupmode='full'>
+ <scratch file='/path/to/file'/>
+ </disk>
+ <disk name='vdd' backup='yes' type='file' backupmode='incremental'>
+ <scratch file='/path/to/file'/>
+ </disk>
+ <disk name='vde' backup='yes' type='file' backupmode='incremental' incremental='blah'>
+ <scratch file='/path/to/file'/>
+ </disk>
+ <disk name='vdf' backup='yes' type='file' incremental='bleh'>
+ <scratch file='/path/to/file'/>
Why is backupmode='incremental' not present in output? Even when it can
be omitted in input, it makes sense for output to include the resulting
value of anything that was defaulted.
+ </disk>
</disks>
</domainbackup>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
