Re: [PATCH 14/24] backup: Allow configuring incremental backup per-disk individually

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(a)redhat.com&gt; > --- > 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