[libvirt] [RFC] Storage volume clone API

[Cole Robinson]

Daniel Veillard wrote:
> On Wed, Apr 15, 2009 at 03:28:54PM -0400, Cole Robinson wrote:
>> Hi all,
>>
>> Attached is a stab at an API for cloning a storage volume. This patch
>> implements the command for FS volumes, along with a virsh command
>> 'vol-clone'. This builds on the previously posted 'drop pool lock during
>> volume creation' work.
>>
>> The new API call looks like:
>>
>> /**
>>  * virStorageVolClone:
>>  * @vol: pointer to storage vol to clone
>>  * @xmldesc: description of volume to create
>>  * @flags: flags for creation (unused, pass 0)
>>  *
>>  * Clone a storage volume within the parent pool.
>>  * Information for the new volume (name, perms)
>>  * are passed via a typical volume XML description
>>  * Not all pools support cloning of volumes
>>  *
>>  * return the storage volume, or NULL on error
>>  */
>> virStorageVolPtr
>> virStorageVolClone(virStorageVolPtr vol,
>>                    const char *xmldesc,
>>                    unsigned int flags)
> 
>   I was initially surprized by the xmldesc, but I think it makes perfect
> sense, based on existing APIs and the fact we may augment the amount of
> metadata and it's simpler to build the API directly with room for the
> extensions.
>   I wonder if the entry point could be made to accept NULL as the input,
> then libvirt generate a name/uuid, keep other attributes the same and
> the returned object can be inspected to discover name/uuid.
> 

That seems reasonable. If people think it's useful behavior, I can add it.

>> The user passes information about the new volume via <volume> XML.
>> Currently we only use the passed name and permissions values, but
>> backingStore info could also be relevant, and 'format' if we allow
>> cloning between image formats.
> 
>   We don't have an UUID for a volume ?
> 

No, the unique identifier is the <key> field which is the absolute path
to the volume on the host. I don't think any of the storage drivers
actually allow manually setting this field, it is either generated using
the parent pool path and the new volume name, or dictated by storage
(/dev/sda3 or similar).

>> Current limitations:
>>
>> - Parsing the 'clone' xml will complain if the 'capacity' element hasn't
>> been specified, even though it is meaningless for the new volume. Not
>> too sure what the right thing to do here is.
> 
>   add an argument to the parsing routine asking to disable that check ?
> 

Yeah I figured i'd end up down that route. I'll check it out.

>> - A clone of a raw sparse file will turn out fully allocated. Could
>> probably borrow code from 'cp' to try and get this right.
> 
>   I would not consider this a blocker though a change in behaviour there
> later on might be considered disruptive (or a great improvement
> depending who you're asking !)
> 

Agreed, not critical.

>> - Doesn't handle backing stores. I think we would need to call out to
>> 'qemu-img convert' to have any chance of getting this right. And if we
>> do that, we could also allow cloning from one image format to another.
>>
>> A lot of the patch is just plumbing, which I will break out into
>> separate patches on the next post. For now, most of the interesting bits
>> are at near the bottom.
> 
>   No need to keep the generated doc files in the patch IMHO
> 

Okay, I'll drop them.

Thanks,
Cole

> [...]
>> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
>> index a028b21..0f4dd27 100644
>> --- a/include/libvirt/libvirt.h.in
>> +++ b/include/libvirt/libvirt.h.in
>> @@ -1047,6 +1047,9 @@ const char*             virStorageVolGetKey             (virStorageVolPtr vol);
>>  virStorageVolPtr        virStorageVolCreateXML          (virStoragePoolPtr pool,
>>                                                           const char *xmldesc,
>>                                                           unsigned int flags);
>> +virStorageVolPtr        virStorageVolClone              (virStorageVolPtr vol,
>> +                                                         const char *xmldesc,
>> +                                                         unsigned int flags);
>>  int                     virStorageVolDelete             (virStorageVolPtr vol,
>>                                                           unsigned int flags);
>>  int                     virStorageVolRef                (virStorageVolPtr vol);
> 
>   Fine
> 
> [...]
>> +/**
>> + * virStorageVolClone:
>> + * @vol: pointer to storage vol to clone
>> + * @xmldesc: description of volume to create
>> + * @flags: flags for creation (unused, pass 0)
>> + *
>> + * Clone a storage volume within the parent pool.
>> + * Information for the new volume (name, perms)
>> + * are passed via a typical volume XML description
>> + * Not all pools support cloning of volumes
>> + *
>> + * return the storage volume, or NULL on error
>> + */
>> +virStorageVolPtr
>> +virStorageVolClone(virStorageVolPtr vol,
>> +                   const char *xmldesc,
>> +                   unsigned int flags)
>> +{
>> +    DEBUG("vol=%p, flags=%u", vol, flags);
>> +
>> +    virResetLastError();
>> +
>> +    if (!VIR_IS_STORAGE_VOL(vol)) {
>> +        virLibConnError(NULL, VIR_ERR_INVALID_STORAGE_VOL, __FUNCTION__);
>> +        return (NULL);
>> +    }
>> +
>> +    if (vol->conn->flags & VIR_CONNECT_RO) {
>> +        virLibConnError(vol->conn, VIR_ERR_OPERATION_DENIED, __FUNCTION__);
>> +        goto error;
>> +    }
>> +
>> +    if (vol->conn->storageDriver && vol->conn->storageDriver->volClone) {
>> +        virStorageVolPtr ret;
>> +        ret = vol->conn->storageDriver->volClone (vol, xmldesc, flags);
>> +        if (!ret)
>> +            goto error;
>> +        return ret;
>> +    }
>> +
>> +    virLibConnError (vol->conn, VIR_ERR_NO_SUPPORT, __FUNCTION__);
>> +
>> +error:
>> +    /* Copy to connection error object for back compatability */
>> +    virSetConnError(vol->conn);
>> +    return NULL;
>> +}
> 
>   okay too
> 
> [...]
>> +static int
>> +virStorageBackendFileSystemVolClone(virConnectPtr conn,
>> +                                    virStorageVolDefPtr origvol,
>> +                                    virStorageVolDefPtr newvol,
>> +                                    unsigned int flags ATTRIBUTE_UNUSED)
>> +{
>> +
>> +    int origfd = -1;
>> +    int newfd = -1;
>> +    int ret = -1;
>> +    size_t bytes = 1024 * 1024;
> 
>   so the copy is done megabytes by megabytes
> That sounds a reasonable intermediate compromize between the daemon
> memory usage and the I/O efficiency, feedback from an I/O expert or
> based on cp or tar code could be interesting.
> 
>    In general patch looks good to me, I got a bit lost in the pool
> locking, but finally it sounds fine.
> 
> Daniel
>