Re: [PATCH v8 19/21] backup: Wire up qemu full pull backup commands over QMP

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Apr 17, 2019 at 09:09:19 -0500, Eric Blake wrote:
> Time to actually issue the QMP transactions that start and
> stop backup commands (for now, just pull mode, not push).
> Starting a job has to kick off several pre-req steps, then
> a transaction, and additionally spawn an NBD server for pull
> mode; ending a job as well as failing partway through
> beginning a job has to unwind the earlier steps.  Implementing
> push mode, as well as incremental pull and checkpoint creation,
> is deferred to later patches.
> 
> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> ---
>  src/qemu/qemu_domain.c  |  18 ++-
>  src/qemu/qemu_driver.c  | 310 ++++++++++++++++++++++++++++++++++++++--
>  src/qemu/qemu_process.c |   9 ++
>  3 files changed, 323 insertions(+), 14 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index f4be948bae..db00c473d0 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2684,17 +2684,25 @@ qemuDomainObjPrivateXMLParseBlockjobs(virQEMUDriverPtr driver,
>  {
>      xmlNodePtr node;
>      int tmp;
> +    size_t i;
>      VIR_AUTOFREE(char *) active = NULL;
> 
>      if ((active = virXPathString("string(./blockjobs/@active)", ctxt)) &&
>          (tmp = virTristateBoolTypeFromString(active)) > 0)
>          priv->reconnectBlockjobs = tmp;
> 
> -    if ((node = virXPathNode("./domainbackup", ctxt)) &&
> -        !(priv->backup = virDomainBackupDefParseNode(ctxt->doc, node,
> -                                                     driver->xmlopt,
> -                                                     VIR_DOMAIN_BACKUP_PARSE_INTERNAL)))
> -        return -1;
> +    if ((node = virXPathNode("./domainbackup", ctxt))) {
> +        if (!(priv->backup = virDomainBackupDefParseNode(ctxt->doc, node,
> +                                                         driver->xmlopt,
> +                                                         VIR_DOMAIN_BACKUP_PARSE_INTERNAL)))
> +            return -1;
> +        /* The backup job is only stored in XML if backupBegin
> +         * succeeded at exporting the disk, so no need to store disk
> +         * state when we can just force-reset it to a known-good
> +         * value. */

So why isn't it stored in a known-good state in the first place?

> +        for (i = 0; i < priv->backup->ndisks; i++)
> +            priv->backup->disks[i].state = VIR_DOMAIN_BACKUP_DISK_STATE_EXPORT;
> +    }
> 
>      return 0;
>  }
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index cca0b550b8..044b26196e 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -17693,8 +17693,80 @@ qemuDomainCheckpointDelete(virDomainCheckpointPtr checkpoint,
>      return ret;
>  }
> 
> -static int qemuDomainBackupBegin(virDomainPtr domain, const char *diskXml,
> -                                 const char *checkpointXml, unsigned int flags)
> +static int
> +qemuDomainBackupPrepare(virQEMUDriverPtr driver, virDomainObjPtr vm,
> +                        virDomainBackupDefPtr def)
> +{
> +    int ret = -1;
> +    size_t i;
> +
> +    if (qemuBlockNodeNamesDetect(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
> +        goto cleanup;

See previous comments regarding this function call.

> +    for (i = 0; i < def->ndisks; i++) {
> +        virDomainBackupDiskDef *disk = &def->disks[i];
> +        virStorageSourcePtr src = vm->def->disks[disk->idx]->src;
> +
> +        if (!disk->store)
> +            continue;
> +        if (virAsprintf(&disk->store->nodeformat, "tmp-%s", disk->name) < 0)

Use a somewhat more descriptive temporary prefix. Also if blockdev-del fails
after a job you won't be ever able to start a new job until the VM is
restarted as the node name will always be the same.

> +            goto cleanup;
> +        if (!disk->store->format)
> +            disk->store->format = VIR_STORAGE_FILE_QCOW2;
> +        if (def->incremental) {
> +            if (src->format != VIR_STORAGE_FILE_QCOW2) {
> +                virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> +                               _("incremental backup of %s requires qcow2"),
> +                               disk->name);
> +                goto cleanup;
> +            }
> +        }
> +    }
> +    ret = 0;
> + cleanup:
> +    return ret;
> +}
> +
> +/* Called while monitor lock is held. Best-effort cleanup. */
> +static int
> +qemuDomainBackupDiskCleanup(virQEMUDriverPtr driver, virDomainObjPtr vm,
> +                            virDomainBackupDiskDef *disk, bool incremental)
> +{
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    const char *node = vm->def->disks[disk->idx]->src->nodeformat;
> +    int ret = 0;
> +
> +    if (!disk->store)
> +        return 0;
> +    if (disk->state >= VIR_DOMAIN_BACKUP_DISK_STATE_EXPORT) {
> +        /* No real need to use nbd-server-remove, since we will
> +         * shortly be calling nbd-server-stop. */
> +    }
> +    if (incremental && disk->state >= VIR_DOMAIN_BACKUP_DISK_STATE_BITMAP &&
> +        qemuMonitorDeleteBitmap(priv->mon, node, disk->store->nodeformat) < 0) {
> +        VIR_WARN("Unable to remove temp bitmap for disk %s after backup",
> +                 disk->name);
> +        ret = -1;
> +    }
> +    if (disk->state >= VIR_DOMAIN_BACKUP_DISK_STATE_READY &&
> +        qemuMonitorBlockdevDel(priv->mon, disk->store->nodeformat) < 0) {

You also need to blockdev-del the storage layer here.

> +        VIR_WARN("Unable to remove temp disk %s after backup",
> +                 disk->name);
> +        ret = -1;
> +    }
> +    if (disk->state >= VIR_DOMAIN_BACKUP_DISK_STATE_LABEL)
> +        qemuDomainDiskChainElementRevoke(driver, vm, disk->store);
> +    if (disk->state >= VIR_DOMAIN_BACKUP_DISK_STATE_CREATED &&
> +        disk->store->detected && unlink(disk->store->path) < 0) {
> +        VIR_WARN("Unable to unlink temp disk %s after backup",
> +                 disk->store->path);
> +        ret = -1;
> +    }
> +    return ret;
> +}
> +
> +static int
> +qemuDomainBackupBegin(virDomainPtr domain, const char *diskXml,
> +                      const char *checkpointXml, unsigned int flags)
>  {
>      virQEMUDriverPtr driver = domain->conn->privateData;
>      virDomainObjPtr vm = NULL;

[...]

> @@ -17747,25 +17826,145 @@ static int qemuDomainBackupBegin(virDomainPtr domain, const char *diskXml,
>      if (!(def = virDomainBackupDefParseString(diskXml, driver->xmlopt, 0)))
>          goto cleanup;
> 
> +    if (def->type == VIR_DOMAIN_BACKUP_TYPE_PULL) {
> +        if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NBD_BITMAP)) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("qemu binary lacks pull-mode backup support"));
> +            goto cleanup;
> +        }
> +        if (!def->server) {
> +            if (VIR_ALLOC(def->server) < 0)
> +                goto cleanup;
> +            def->server->transport = VIR_STORAGE_NET_HOST_TRANS_TCP;
> +            if (VIR_STRDUP(def->server->name, "localhost") < 0)
> +                goto cleanup;
> +        }
> +        switch ((virStorageNetHostTransport)def->server->transport) {
> +        case VIR_STORAGE_NET_HOST_TRANS_TCP:
> +            /* TODO: Update qemu.conf to provide a port range,
> +             * probably starting at 10809, for obtaining automatic
> +             * port via virPortAllocatorAcquire, as well as store
> +             * somewhere if we need to call virPortAllocatorRelease
> +             * during BackupEnd. Until then, user must provide port */
> +            if (!def->server->port) {
> +                virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +                               _("<domainbackup> must specify TCP port for now"));
> +                goto cleanup;
> +            }
> +            break;
> +        case VIR_STORAGE_NET_HOST_TRANS_UNIX:
> +            /* TODO: Do we need to mess with selinux? */
> +            break;
> +        case VIR_STORAGE_NET_HOST_TRANS_RDMA:
> +        case VIR_STORAGE_NET_HOST_TRANS_LAST:
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("unexpected transport in <domainbackup>"));
> +            goto cleanup;
> +        }
> +    } else {
> +        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +                       _("push mode backups not supported yet"));
> +        goto cleanup;
> +    }
> +    if (def->incremental) {
> +        if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BITMAP_MERGE)) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("qemu binary lacks persistent bitmaps support"));
> +            goto cleanup;
> +        }
> +        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +                       _("cannot create incremental backups yet"));
> +        goto cleanup;
> +    }
> +
> +    if (!(qemuImgPath = qemuFindQemuImgBinary(driver)))
> +        goto cleanup;
> +
>      /* We are going to modify the domain below. */
>      if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
>          goto cleanup;
> 
> -    priv = vm->privateData;
>      if (priv->backup) {
>          virReportError(VIR_ERR_OPERATION_INVALID,
>                         "%s", _("another backup job is already running"));
>          goto endjob;
>      }
> 
> -    if (virDomainBackupAlignDisks(def, vm->def, suffix) < 0)
> +    if (virDomainBackupAlignDisks(def, vm->def, suffix) < 0 ||
> +        qemuDomainBackupPrepare(driver, vm, def) < 0)
>          goto endjob;
> 
>      /* actually start the checkpoint. 2x2 array of push/pull, full/incr,
>         plus additional tweak if checkpoint requested */
> -    /* TODO: issue QMP commands:
> -       - pull: nbd-server-start with <server> from user (or autogenerate server)
> -       - push/pull: blockdev-add per <disk>
> +    qemuDomainObjEnterMonitor(driver, vm);
> +    /* - push/pull: blockdev-add per <disk> */
> +    for (i = 0; i < def->ndisks; i++) {
> +        virDomainBackupDiskDef *disk = &def->disks[i];
> +        virJSONValuePtr file;
> +        virStorageSourcePtr src = vm->def->disks[disk->idx]->src;
> +        const char *node = src->nodeformat;
> +
> +        if (!disk->store)
> +            continue;
> +        if (qemuDomainStorageFileInit(driver, vm, disk->store, src) < 0)

virStorageFileDeinit is never called in this function.

> +            goto endmon;
> +        if (disk->store->detected) {
> +            if (virStorageFileCreate(disk->store) < 0) {
> +                virReportSystemError(errno,
> +                                     _("failed to create image file '%s'"),
> +                                     NULLSTR(disk->store->path));
> +                goto endmon;
> +            }
> +            disk->state = VIR_DOMAIN_BACKUP_DISK_STATE_CREATED;
> +        }
> +        if (qemuDomainDiskChainElementPrepare(driver, vm, disk->store, false,
> +                                              true) < 0)
> +            goto endmon;
> +        disk->state = VIR_DOMAIN_BACKUP_DISK_STATE_LABEL;
> +        if (disk->store->detected) {
> +            virBuffer buf = VIR_BUFFER_INITIALIZER;
> +
> +            /* Force initialization of scratch/target file to new qcow2 */
> +            if (!(cmd = virCommandNewArgList(qemuImgPath,
> +                                             "create",
> +                                             "-f",
> +                                             virStorageFileFormatTypeToString(disk->store->format),
> +                                             "-o",
> +                                             NULL)))
> +                goto endmon;
> +            virBufferAsprintf(&buf, "backing_fmt=%s,backing_file=",
> +                              virStorageFileFormatTypeToString(src->format));
> +            virQEMUBuildBufferEscapeComma(&buf, src->path);
> +            virCommandAddArgBuffer(cmd, &buf);
> +
> +            virQEMUBuildBufferEscapeComma(&buf, disk->store->path);
> +            virCommandAddArgBuffer(cmd, &buf);
> +            if (virCommandRun(cmd, NULL) < 0)
> +                goto endmon;
> +            virCommandFree(cmd);
> +            cmd = NULL;
> +        }
> +
> +        if (virJSONValueObjectCreate(&file,
> +                                     "s:driver", "file",
> +                                     "s:filename", disk->store->path,
> +                                     NULL) < 0)
> +            goto endmon;
> +        if (virJSONValueObjectCreate(&json,
> +                                     "s:driver", virStorageFileFormatTypeToString(disk->store->format),

We have helpers to create these froma virStorageSource. How about reusing those?

> +                                     "s:node-name", disk->store->nodeformat,
> +                                     "a:file", &file,

The rest of the blockdev integration code which is already present
always plugs in the storage and format layer separately. This should do
the same.

> +                                     "s:backing", node, NULL) < 0) {
> +            virJSONValueFree(file);
> +            goto endmon;
> +        }
> +        if (qemuMonitorBlockdevAdd(priv->mon, json) < 0)
> +            goto endmon;
> +        json = NULL;
> +        disk->state = VIR_DOMAIN_BACKUP_DISK_STATE_READY;
> +    }
> +
> +    /* TODO:
>         - incr: bitmap-add of tmp, bitmap-merge per <disk>
>         - transaction, containing:
>           - push+full: blockdev-backup sync:full
> @@ -17773,8 +17972,76 @@ static int qemuDomainBackupBegin(virDomainPtr domain, const char *diskXml,
>           - pull+full: blockdev-backup sync:none
>           - pull+incr: blockdev-backup sync:none, bitmap-disable of tmp
>           - if checkpoint: bitmap-disable of old, bitmap-add of new
> +    */
> +    if (!(json = virJSONValueNewArray()))
> +        goto endmon;
> +    for (i = 0; i < def->ndisks; i++) {
> +        virDomainBackupDiskDef *disk = &def->disks[i];
> +        virStorageSourcePtr src = vm->def->disks[disk->idx]->src;
> +
> +        if (!disk->store)
> +            continue;
> +        if (qemuMonitorJSONTransactionAdd(json,
> +                                          "blockdev-backup",
> +                                          "s:device", src->nodeformat,
> +                                          "s:target", disk->store->nodeformat,
> +                                          "s:sync", "none",
> +                                          "s:job-id", disk->name,

This should probably be somewhat more descriptive to prevent collisions
if we ever want to allow multiple jobs

> +                                          NULL) < 0)
> +            goto endmon;
> +    }
> +    if (qemuMonitorTransaction(priv->mon, &json) < 0)
> +        goto endmon;
> +    job_started = true;
> +
> +    /*
> +       - pull: nbd-server-start with <server> from user (or autogenerate server)
>         - pull: nbd-server-add per <disk>, including bitmap for incr
>      */
> +    if (def->type == VIR_DOMAIN_BACKUP_TYPE_PULL) {
> +        if (qemuMonitorNBDServerStart(priv->mon, def->server, NULL) < 0)
> +            goto endmon;
> +        nbd_running = true;
> +        for (i = 0; i < def->ndisks; i++) {
> +            virDomainBackupDiskDef *disk = &def->disks[i];
> +
> +            if (!disk->store)
> +                continue;
> +            if (qemuMonitorNBDServerAdd(priv->mon, disk->store->nodeformat,
> +                                        disk->name, false,
> +                                        def->incremental ? disk->name :
> +                                        NULL) < 0)
> +                goto endmon;
> +            disk->state = VIR_DOMAIN_BACKUP_DISK_STATE_EXPORT;
> +        }
> +    }
> +
> +    ret = 0;

Attachment: signature.asc
Description: PGP signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux