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

2020-07-07 Thread Eric Blake

On 7/7/20 10:23 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


s/an/a/


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 
---
v2:
 - backupmode=full incremental="..." is now rejected by the RNG
   schema
 - test output changes as we now validate virDomainBackupAlignDisks
 - typo fix


Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



[PATCH 4/4] backup: Allow configuring incremental backup per-disk individually

2020-07-07 Thread Peter Krempa
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 
---
v2:
- backupmode=full incremental="..." is now rejected by the RNG
  schema
- test output changes as we now validate virDomainBackupAlignDisks
- typo fix

 docs/formatbackup.rst | 11 +
 docs/schemas/domainbackup.rng | 23 +
 src/conf/backup_conf.c| 49 ++-
 src/conf/backup_conf.h| 11 +
 tests/domainbackupxml2xmlin/backup-pull.xml   | 12 +
 .../backup-pull-encrypted.xml |  6 +--
 .../backup-pull-internal-invalid.xml  |  6 +--
 .../backup-pull-seclabel.xml  |  4 +-
 tests/domainbackupxml2xmlout/backup-pull.xml  | 14 +-
 .../backup-push-encrypted.xml |  6 +--
 .../backup-push-seclabel.xml  |  4 +-
 tests/domainbackupxml2xmlout/backup-push.xml  |  2 +-
 tests/domainbackupxml2xmlout/empty.xml|  2 +-
 13 files changed, 133 insertions(+), 17 deletions(-)

diff --git a/docs/formatbackup.rst b/docs/formatbackup.rst
index 17431fe51a..1b9e6ebb22 100644
--- a/docs/formatbackup.rst
+++ b/docs/formatbackup.rst
@@ -69,6 +69,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 c0e17f512b..579b62a658 100644
--- a/docs/schemas/domainbackup.rng
+++ b/docs/schemas/domainbackup.rng
@@ -96,6 +96,27 @@
 
   

+
+  
+
+  
+
+  full
+
+
+  
+
+  incremental
+
+  
+  
+
+  
+
+  
+
+  
+
   
 
   
@@ -134,6 +155,7 @@
 
   
 
+
 
   
 
@@ -203,6 +225,7 @@
 
   
 
+
 
   
 
diff --git a/src/conf/backup_conf.c b/src/conf/backup_conf.c
index 5e4144d371..02319f7245 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)
 {
@@ -100,6 +107,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;
@@ -137,6 +145,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;
+}
+
+