Re: [PATCH v3 4/4] qemu_migration: Precreate missing storage

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

 



On 12/02/14 17:13, Michal Privoznik wrote:
> Based on previous commit, we can now precreate missing volumes. While
> digging out the functionality from storage driver would be nicer, if
> you've seen the code it's nearly impossible. So I'm going from the
> other end:
> 
> 1) For given disk target, disk path is looked up.
> 2) For the disk path, storage pool is looked up, a volume XML is
> constructed and then passed to virStorageVolCreateXML() which has all
> the knowledge how to create raw images, (encrypted) qcow(2) images,
> etc.
> 
> One of the advantages of this approach is, we don't have to care about
> image conversion - qemu does that for us. So for instance, users can
> transform qcow2 into raw on migration (if the correct XML is passed to
> the migration API).
> 
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
>  src/libvirt-domain.c      |   3 +
>  src/qemu/qemu_migration.c | 159 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 162 insertions(+)
> 
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 01bd9e4..8333183 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -4312,6 +4312,9 @@ virDomainMigrateToURI(virDomainPtr domain,
>   * If you want to copy non-shared storage within migration you
>   * can use either VIR_MIGRATE_NON_SHARED_DISK or
>   * VIR_MIGRATE_NON_SHARED_INC as they are mutually exclusive.
> + * As of 1.2.11 disks of some types ('file' and 'volume') are
> + * precreated automatically, if there's a pool defined on the
> + * destination for the disk path.
>   *
>   * If a hypervisor supports changing the configuration of the guest
>   * during migration, the @dxml parameter specifies the new config
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 36b7e43..11e2a2c 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -58,6 +58,7 @@
>  #include "virtypedparam.h"
>  #include "virprocess.h"
>  #include "nwfilter_conf.h"
> +#include "storage/storage_driver.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_QEMU
>  
> @@ -1451,6 +1452,161 @@ qemuMigrationRestoreDomainState(virConnectPtr conn, virDomainObjPtr vm)
>      return ret;
>  }
>  
> +
> +static int
> +qemuMigrationPrecreateDisk(virConnectPtr conn,
> +                           virDomainDiskDefPtr disk,
> +                           unsigned long long capacity)
> +{
> +    int ret = -1;
> +    virStoragePoolPtr pool = NULL;
> +    virStorageVolPtr vol = NULL;
> +    char *volName = NULL, *basePath = NULL;
> +    char *volStr = NULL;
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +    const char *format = NULL;
> +    unsigned int flags = 0;
> +
> +    VIR_DEBUG("Precreate disk type=%s", virStorageTypeToString(disk->src->type));
> +
> +    switch ((virStorageType) disk->src->type) {
> +    case VIR_STORAGE_TYPE_FILE:
> +        if (!virDomainDiskGetSource(disk)) {
> +            VIR_DEBUG("Dropping sourceless disk '%s'",
> +                      disk->dst);
> +            return 0;
> +        }
> +
> +        if (VIR_STRDUP(basePath, disk->src->path) < 0)
> +            goto cleanup;
> +
> +        if (!(volName = strrchr(basePath, '/'))) {
> +            virReportError(VIR_ERR_INVALID_ARG,
> +                           _("malformed disk path: %s"),
> +                           disk->src->path);
> +            goto cleanup;
> +        }
> +
> +        *volName = '\0';
> +        volName++;
> +
> +        if (!(pool = storagePoolLookupByTargetPath(conn, basePath)))
> +            goto cleanup;
> +        format = virStorageFileFormatTypeToString(disk->src->format);
> +        if (disk->src->format == VIR_STORAGE_FILE_QCOW2)
> +            flags |= VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA;
> +        break;
> +
> +    case VIR_STORAGE_TYPE_VOLUME:
> +        if (!(pool = virStoragePoolLookupByName(conn, disk->src->srcpool->pool)))
> +            goto cleanup;
> +        format = virStorageFileFormatTypeToString(disk->src->format);
> +        volName = disk->src->srcpool->volume;
> +        if (disk->src->format == VIR_STORAGE_FILE_QCOW2)
> +            flags |= VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA;
> +        break;
> +
> +    case VIR_STORAGE_TYPE_BLOCK:
> +    case VIR_STORAGE_TYPE_DIR:
> +    case VIR_STORAGE_TYPE_NETWORK:
> +    case VIR_STORAGE_TYPE_NONE:
> +    case VIR_STORAGE_TYPE_LAST:
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("cannot precreate storage for disk type '%s'"),
> +                       virStorageTypeToString(disk->src->type));
> +        goto cleanup;
> +        break;
> +    }
> +
> +    if ((vol = virStorageVolLookupByName(pool, volName))) {
> +        VIR_DEBUG("Skipping creation of already existing volume of name '%s'",
> +                  volName);
> +        ret = 0;
> +        goto cleanup;
> +    }
> +
> +    virBufferAddLit(&buf, "<volume>\n");
> +    virBufferAdjustIndent(&buf, 2);
> +    virBufferEscapeString(&buf, "<name>%s</name>\n", volName);
> +    virBufferAsprintf(&buf, "<capacity>%llu</capacity>\n", capacity);
> +    virBufferAddLit(&buf, "<target>\n");
> +    virBufferAdjustIndent(&buf, 2);
> +    virBufferAsprintf(&buf, "<format type='%s'/>\n", format);
> +    virBufferAdjustIndent(&buf, -2);
> +    virBufferAddLit(&buf, "</target>\n");
> +    virBufferAdjustIndent(&buf, -2);
> +    virBufferAddLit(&buf, "</volume>\n");
> +
> +    if (!(volStr = virBufferContentAndReset(&buf))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("unable to create volume XML"));
> +        goto cleanup;
> +    }
> +
> +    if (!(vol = virStorageVolCreateXML(pool, volStr, flags)))
> +        goto cleanup;
> +
> +    ret = 0;
> + cleanup:
> +    VIR_FREE(basePath);
> +    VIR_FREE(volStr);
> +    if (vol)
> +        virStorageVolFree(vol);
> +    if (pool)
> +        virStoragePoolFree(pool);

I just ACKed John's series that forbids the virStorageVolFree and
virStoragePoolFree functions in our codebase. You'll need to change them
to virObjectUnref().

> +    return ret;
> +}
> +
> +
> +static int
> +qemuMigrationPrecreateStorage(virConnectPtr conn,
> +                              virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
> +                              virDomainObjPtr vm,
> +                              qemuMigrationCookieNBDPtr nbd)
> +{
> +    int ret = -1;
> +    size_t i = 0;
> +
> +    if (!nbd || !nbd->ndisks)
> +        return 0;
> +
> +    for (i = 0; i < nbd->ndisks; i++) {
> +        virDomainDiskDefPtr disk;
> +        int indx;
> +        const char *diskSrcPath;
> +
> +        VIR_DEBUG("Looking up disk target '%s' (capacity=%lluu)",
> +                  nbd->disks[i].target, nbd->disks[i].capacity);

Following up on 3/4: here and below you use the "target" field without
verifying that it was provided.

Sounds like bikeshedding, but if we can be robust here, we should be. I
think the check should be in 3/4

> +
> +        if ((indx = virDomainDiskIndexByName(vm->def,
> +                                             nbd->disks[i].target, false)) < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("unable to find disk by target: %s"),
> +                           nbd->disks[i].target);
> +            goto cleanup;
> +        }
> +
> +        disk = vm->def->disks[indx];
> +        diskSrcPath = virDomainDiskGetSource(disk);
> +
> +        if (disk->src->shared || disk->src->readonly ||
> +            (diskSrcPath && virFileExists(diskSrcPath))) {
> +            /* Skip shared, read-only and already existing disks. */
> +            continue;
> +        }
> +
> +        VIR_DEBUG("Proceeding with disk source %s", NULLSTR(diskSrcPath));
> +
> +        if (qemuMigrationPrecreateDisk(conn, disk, nbd->disks[i].capacity) < 0)
> +            goto cleanup;
> +    }
> +
> +    ret = 0;
> + cleanup:
> +    return ret;
> +}
> +
> +
>  /**
>   * qemuMigrationStartNBDServer:
>   * @driver: qemu driver

ACK if you change the virStorage*Free funcs.

Peter

Attachment: signature.asc
Description: OpenPGP digital 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]