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@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). 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 >
