[PATCH 14/24] backup: Allow configuring incremental backup per-disk individually
Peter Krempa
pkrempa at redhat.com
Tue Jul 7 14:14:42 UTC 2020
On Thu, Jul 02, 2020 at 14:31:19 -0500, Eric Blake wrote:
> 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).
Nice, I'll use this one. My brain stopped working when doing the schema
and I couldn't figure this one out.
>
> 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)
I'll think about this and possbily post a separate patch. The same also
applies to the <incremental> element which also doesn't do validation.
Luckily it's not officially supported yet so we can still make it more
strict.
> > @@ -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.
Sure thing! I actually prefer the RNG solution.
>
> > +
> > + 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'?
No, this check validates that if an explicit incremental mode is
requested and neither the per-disk nor per-backup 'incremental' setting
is provided we reject such a config because we wouldn't be able to infer
which is the point where to backup from.
> > +++ 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?
It's an error as noted above.
> > + <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.
Well the code fills it in in 'virDomainBackupAlignDisks', but that
function is not called from the test suite.
'virDomainBackupAlignDisks' requires a domain definition to do some
matching around. While I agree that it should be tested, it's not really
in scope of this patch.
>
> > + </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