[libvirt] [PATCH v2 4/4] qemu_migration: Precreate missing storage

[Peter Krempa]

On 12/01/14 15:57, 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 at redhat.com>
> ---
>  src/qemu/qemu_migration.c | 150 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 150 insertions(+)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 26df9c7..9ccfee2 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
>  
> @@ -1454,6 +1455,152 @@ 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, '/'))) {

For finding the base path you'd normally want to use mdir_name(), but
then you wouldn't be able to extract the volume name too.

> +            virReportError(VIR_ERR_INVALID_ARG,
> +                           _("malformed disk path: %s"),
> +                           disk->src->path);
> +            goto cleanup;
> +        }
> +
> +        *volName = '\0';
> +        volName++;


I guess this is good enough then.

> +
> +        if (!(pool = storagePoolLookupByPath(conn, basePath))) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("unable to find pool to %s"),

"unable to find pool for path '%s'" ?

> +                           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,
> +                       _("unsupported disk type '%s'"),

Possibly a more helpful message: "cannot precreate storage for disk type
'%s' ?

> +                       virStorageTypeToString(disk->src->type));
> +        goto cleanup;
> +        break;
> +    }
> +
> +    virBufferAddLit(&buf, "<volume>\n");
> +    virBufferAdjustIndent(&buf, 2);
> +    virBufferAsprintf(&buf, "<name>%s</name>", volName);

You need to use virBufferEscape for the name

> +    virBufferAsprintf(&buf, "<capacity>%llu</capacity>\n", capacity);
> +    virBufferAddLit(&buf, "<target>\n");
> +    virBufferAdjustIndent(&buf, 2);
> +    virBufferAsprintf(&buf, "<format type='%s'/>\n", format);

Format is okay as we control the strings.

> +    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);
> +    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);
> +
> +        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);
> +
> +        VIR_DEBUG("Proceeding with disk source %s", NULLSTR(diskSrcPath));


The storage pre-creation should be skipped in case the target file
already exists as previously the user had to pre-create the file
manually. In case we'd do this unconditionally we might break the case
somebody is already precreating it.


> +
> +        if (qemuMigrationPrecreateDisk(conn, disk, nbd->disks[i].capacity) < 0)
> +            goto cleanup;
> +    }
> +
> +    ret = 0;
> + cleanup:
> +    return ret;
> +}
> +
> +
>  /**
>   * qemuMigrationStartNBDServer:
>   * @driver: qemu driver
> @@ -2838,6 +2985,9 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
>          goto cleanup;
>      }
>  
> +    if (qemuMigrationPrecreateStorage(dconn, driver, vm, mig->nbd) < 0)
> +        goto cleanup;
> +
>      if (qemuMigrationJobStart(driver, vm, QEMU_ASYNC_JOB_MIGRATION_IN) < 0)
>          goto cleanup;
>      qemuMigrationJobSetPhase(driver, vm, QEMU_MIGRATION_PHASE_PREPARE);
> 

Otherwise looks good code-wise, but I don't see any mention of this new
feature in the documentation.

Peter

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20141202/fc55e137/attachment-0001.sig>