[PATCH 14/24] backup: Allow configuring incremental backup per-disk individually
Eric Blake
eblake at redhat.com
Thu Jul 2 19:31:19 UTC 2020
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 at redhat.com>
> ---
> 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
More information about the libvir-list
mailing list