Re: [RFC V9 4/4] domain snapshot design: libxl/libxlu

[Ian Campbell <Ian.Campbell@citrix.com>]

On Tue, 2014-12-16 at 14:32 +0800, Chunyan Liu wrote:
> Changes to V8:
>   * remove libxl_domain_snapshot_create/delete/revert API
>   * export disk snapshot functionality for both xl and libvirt usage
> 
> ===========================================================================
> Libxl/libxlu Design
> 
> 1. New Structures
> 
> libxl_disk_snapshot = Struct("disk_snapshot",[
>     # target disk
>     ("disk",            libxl_device_disk),
> 
>     # disk snapshot name
>     ("name",            string),
> 
>     # internal/external disk snapshot?
>     ("external",        bool),
> 
>     # for external disk snapshot, specify following two field
>     ("external_format", string),
>     ("external_path",   string),

Should this be a KeyedUnion over a new LIBXL_DISK_SNAPSHOT_KIND enum
(with values INTERNAL and EXTERNAL)? This would automatically make the
binding between external==true and the fields which depend on that.

external_format should be of type libxl_disk_format, unless it is
referring to something else?

Is it possible for format to differ from the format of the underlying
disk? Perhaps taking a snapshot of a raw disk as a qcow? In any case
passing in UNKNOWN and letting libxl choose (probably by picking the
same as the underlying disk) should be supported.

> /*  This API might not be used by xl, since xl won't take care of deleting
>  *  snapshots. But for libvirt, since libvirt manages snapshots and will
>  *  delete snapshot, this API will be used.
>  */
> int libxl_disk_snapshot_delete(libxl_ctx *ctx, uint32_t domid,
>                                libxl_disk_snapshot *snapshot, int nb);

The three usecases I mentioned in the previous mail are important here,
because depending on which usecases you are considering there maybe a
many to one relationship between domains and a given snapshot (gold
image case). This interface cannot support that I think.

When we discussed this in previous iterations I suggested a libxl
command to tell a VM that it needed to reexamine its disks to see if any
of the chains had changed. I'm sure that's not the only potential answer
though.

> int libxl_disk_to_snapshot(libxl_ctx *ctx, uint32_t domid,
>                            libxl_disk_snapshot **snapshot, int *num);
> 
>     This is for domain snapshot create. If user doesn't specify disks,
>     then by default it will take internal disk snapshot to each domain
>     disk. This function will fill libxl_disk_snapshot according to domain
>     disks info.

Is this just a helper to produce an array to pass to
libxl_disk_snapshot_create? Or does it actually do stuff?

I think it's the former, but it could be clarified. I *think* this is
just a special case of libxl_device_disk_list which returns plausible
snapshot objects instead of the disks themselves.
> 
> For disk snapshot revert, no qmp command for that, it always calls
> external commands to finish the work, so put in libxlu (?):

I think rather than "no qmp" the issue is that "revert" is (at least as
far as libxl knows) essentially, destroy, rollback disks, restore from
RAM snapshot. So there is no qemu to speak to during the rollback. Is
that right?

Ian.