Re: [PATCH 14/24] backup: Allow configuring incremental backup per-disk individually
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
> > ---
> > 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
> > 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 @@
> >
> >
> >
> > +
> > +
> > +
> > +
> > + full
> > + incremental
> > +
> > +
> > +
> > +
> > +
> > +
> > +
>
> As written, you validate:
>
> backupmode="full" incremental="blah"
>
> Better might be:
>
>
>
>
>
> full
>
>
>
>
> incremental
>
>
>
>
>
>
>
>
>
>
> 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 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
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
---
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 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 @@
+
+
+
+
+ full
+ incremental
+
+
+
+
+
+
+
As written, you validate:
backupmode="full" incremental="blah"
Better might be:
full
incremental
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 @@
+
+
+
So this is a demo of overriding an overall incremental
[PATCH 14/24] backup: Allow configuring incremental backup per-disk individually
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
---
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``
+ 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 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 @@
+
+
+
+
+ full
+ incremental
+
+
+
+
+
+
+
+
@@ -127,6 +141,7 @@
+
@@ -196,6 +211,7 @@
+
diff --git a/src/conf/backup_conf.c b/src/conf/backup_conf.c
index e9eea5af75..4f28073ab2 100644
--- a/src/conf/backup_conf.c
+++ b/src/conf/backup_conf.c
@@ -56,6 +56,13 @@ VIR_ENUM_IMPL(virDomainBackupDiskState,
"cancelling",
"cancelled");
+VIR_ENUM_DECL(virDomainBackupDiskBackupMode);
+VIR_ENUM_IMPL(virDomainBackupDiskBackupMode,
+ VIR_DOMAIN_BACKUP_DISK_BACKUP_MODE_LAST,
+ "",
+ "full",
+ "incremental");
+
void
virDomainBackupDefFree(virDomainBackupDefPtr def)
{
@@ -96,6 +103,7 @@ virDomainBackupDiskDefParseXML(xmlNodePtr node,
g_autofree char *driver = NULL;
g_autofree char *backup = NULL;
g_autofree char *state = NULL;
+g_autofree char *backupmode = NULL;
int tmp;
xmlNodePtr srcNode;
unsigned int storageSourceParseFlags = 0;
@@ -133,6 +141,19 @@ virDomainBackupDiskDefParseXML(xmlNodePtr node,
def->exportbitmap = virXMLPropString(node, "exportbitmap");
}
+if ((backupmode = virXMLPropString(node, "backupmode"))) {
+if ((tmp = virDomainBackupDiskBackupModeTypeFromString(backupmode)) <
0) {
+virReportError(VIR_ERR_XML_ERROR,
+ _("invalid backupmode '%s' of disk '%s'"),
+ backupmode, def->name);
+return -1;
+}
+
+def->backupmode = tmp;
+}
+
+def->incremental = virXMLPropString(node, "incremental");
+
if (internal) {
if (!(state = virXMLPropString(node, "state")) ||
(tmp = virDomainBackupDiskStateTypeFromString(state)) < 0) {
@@ -342,6 +363,13 @@ virDomainBackupDiskDefFormat(virBufferPtr buf,
if (disk->backup == VIR_TRISTATE_BOOL_YES) {
virBufferAsprintf(, " type='%s'",
virStorageTypeToString(disk->store->type));
+if (disk->backupmode != VIR_DOMAIN_BACKUP_DISK_BACKUP_MODE_DEFAULT) {
+virBufferAsprintf(, " backupmode='%s'",
+
