All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed
* [RFC V5 0/5] domain snapshot document and discussion
@ 2014-07-07  7:45 Bamvor Jian Zhang
  2014-07-07  7:46 ` [RFC V5 1/5] docs: libxl Domain Snapshot HOWTO Bamvor Jian Zhang
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Bamvor Jian Zhang @ 2014-07-07  7:45 UTC (permalink / raw
  To: xen-devel
  Cc: zzhou, ian.campbell, hahn, ian.jackson, cyliu, jfehlig, bjzhang,
	anthony.perard, davidkiarie4

Here is the fifth version about domain snapshot documents, the previous version
is here[1][2][3][4].

There are lots of useful suggestions in previous four round review. I would like
to thank you for all your support. From my point of view, the current version
is more close to what i am doing. I could send the patch out when community feel
document in good shape.

In addition, there is a API changes during document review. It would affect what
David kiarie work on the GSoC project(snapshot support for libvirt libxl driver).
It is important for me and David could get the help from reviewing this.

There are lots of potential features about snapshots. Now we try to focus on
basic features, providing API for libvirt libxl driver which supports the same
functionality as libvirt qemu driver. Advanced features are deferred to the
next phase.

Libxl manages the domain snapshot information through the configuration file.
All snapshot informatin for specific domain is stored here:
"/var/lib/xen/snapshots/<domain_uuid>/snapshotdata-<snapshot_name>.libxl-json",
This file is one for per snapshot per domain.

changes since v4:
reorganize the document
add docs/misc/snapshot-HOWTO.txt
add the explanation of domain snapshot api.

changes since v3:
split one document into difference document, hope it will be easier to
review and discuss.

changes since v2:
1), reorgnized the whole docments.
2), do not export the dedicated the disk snapshot commands.
3), others changes according to Ian and Jim's comment.

0000 this file
0001 how to use xl snapshot-xxx command
0002 introduce new struct and new api in order to implement domain
     snapshot in toolstack (xl or libvirt).
0003 manpage xl snapshot-xxx command
0004 discussion about xl snapshot implementation and what is not covered
     in current work.
0005 manpage for xl domain snapshot configuraiton

[1] http://lists.xen.org/archives/html/xen-devel/2014-04/msg00414.html
    http://lists.xen.org/archives/html/xen-devel/2014-04/msg00244.html
[2] http://lists.xen.org/archives/html/xen-devel/2014-04/msg02549.html
[3] http://lists.xen.org/archives/html/xen-devel/2014-05/msg01977.html
[4] http://lists.xen.org/archives/html/xen-devel/2014-06/msg02971.html

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC V5 1/5] docs: libxl Domain Snapshot HOWTO
  2014-07-07  7:45 [RFC V5 0/5] domain snapshot document and discussion Bamvor Jian Zhang
@ 2014-07-07  7:46 ` Bamvor Jian Zhang
  2014-07-07  7:46 ` [RFC V5 2/5] Libxl Domain Snapshot API Design Bamvor Jian Zhang
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 14+ messages in thread
From: Bamvor Jian Zhang @ 2014-07-07  7:46 UTC (permalink / raw
  To: xen-devel
  Cc: zzhou, ian.campbell, hahn, ian.jackson, cyliu, jfehlig, bjzhang,
	anthony.perard, davidkiarie4

libxl Domain Snapshot HOWTO
--------

Author:
   Bamvor Jian Zhang <bjzhang@suse.com>

* Overview:
    Two levels of snapshot: disk snapshot and domain snapshot
    Four types of snapshot operations: create, delete, list and revert.

    * Disk snapshot
        Disk snapshot is crash-consistent if the domain is running. It could be
        "internal" (like in qcow2 format, snapshot and delta are both in one
        image file), or "external" (snapshot in one file, delta in another).

    * Domain snapshot
        Domain snapshot includes disk snapshots and domain state saving. domain
        could be resumed to the very state when the snapshot was created. This
        kind of snapshot is also referred to as a domain checkpoint or system
        checkpoint.

        "domain snapshot create" means saving domain state and doing disk
        snapshots. In the beginning of creating domain snapshot, it will check
        whether it is snapshotable.

        "domain snapshot revert" means rolling back to the state of indicated
        snapshot.

* Operations of Snapshot

    * Create Snapshot:

        User could create internal or external snapshot with or without config
        file. Refer to xl.snapshot.5 ( RFC Doc 5/5) for details of the config
        file. The Config file could provide all the features of snapshot, e.g.
        User could determine which disk will be included in snapshot(one or all
        disks).

        Use Cases:

        1), create an internal snapshot with default name (autogenerated name:
            epoch second from 1, Jan 1970). Internal snapshot is only supported
            by qcow2 format.
            e.g.:
                #xl snapshot-create your_domain

        2), create an internal snapshot with specific snapshot name.
            e.g.:
                #xl snapshot-create -n domain_snapshot_name your_domain

        3), create an external snapshot
            To create an external snapshot, you must specify a config file, in
            which you could specify the external path and the snapshot type in
            'disk'.
            e.g.:
                #xl snapshot-create your_domain config_file

            #cat snapshot_external_config_file
            name = "1397207577"
            description="Snapshot after OS installation. External snapshot"
            memory="yes"
            disk=['hda=/var/lib/xen/snapshots/<domain_uuid>/disk_hda.qcow2,type=qcow2',
                  'hdc=/var/lib/xen/snapshots/<domain_uuid>/disk_hdc.qcow2,type=qcow2']

        4), create disk-only snapshot, which means no memory state will be saved
            in this snapshot. Could be internal and external.
            e.g.:
                (internal)
                #xl snapshot-create --disk-only your_domain

                (external)
                #xl snapshot-create your_domain disk_only_config_file

            specific config file (to indicate which disks to be snapshotted).
            #cat disk_only_config_file
            name = "1397207577"
            description="Snapshot after OS installation. External disk-only\
                         snapsot. Only care about the root file system"
            disk=['hda=/var/lib/xen/snapshots/<domain_uuid>/disk_hda.qcow2,type=qcow2']

    * List Snapshot:

        Use Cases:

        1), List short information of all snapshots for the specific domain.
            Short information include snapshot name, creation time, the domain
            state when create snapshot, whether it is a disk-only snapshot.

            e.g.:
                #xl snapshot-list your_domain

        2), List full information of one snapshot for specific domain.
            Full information include short information and all the disk snapshot
            relative to that domain snapshot.
            e.g.:
                #xl snapshot-list -l -n domain_snapshot_name your_domain

        3), list full information of all snapshots for specific domain.
            e.g.:
                #xl snapshot-list -l your_domain

    * Delete snapshot:

        Use Cases:

        1), delete a snapshot for specific domain.
            This operation will delete all relative to this snapshot
            e.g.:
               #xl snapshot-delete -n domain_snapshot_name your_domain

    * Revert snapshot:

        Use Cases:

        Curently, only internal snapshot revert is supportted.
        Following command will revert an internal disk snapshot and memory state.
        e.g.:
            #xl snapshot-revert -n domain_snapshot_name your_domain

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC V5 2/5] Libxl Domain Snapshot API Design
  2014-07-07  7:45 [RFC V5 0/5] domain snapshot document and discussion Bamvor Jian Zhang
  2014-07-07  7:46 ` [RFC V5 1/5] docs: libxl Domain Snapshot HOWTO Bamvor Jian Zhang
@ 2014-07-07  7:46 ` Bamvor Jian Zhang
  2014-07-11 12:47   ` Ian Campbell
  2014-07-07  7:46 ` [RFC V5 3/5] docs: manpage for xl snapshot command Bamvor Jian Zhang
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Bamvor Jian Zhang @ 2014-07-07  7:46 UTC (permalink / raw
  To: xen-devel
  Cc: zzhou, ian.campbell, hahn, ian.jackson, cyliu, jfehlig, bjzhang,
	anthony.perard, davidkiarie4

Libxl Domain Snapshot API Design

1. New Structures

    Domain snapshot introduces two new structures:
    - "libxl_domain_snapshot" store domain snapshot information, it contains
       libxl_disk_snapshot array.
    - "libxl_disk_snapshot" stores disk snapshot related information.

    Both are defined in libxl_types.idl, which will generate the following
    libxl-json helper functions:

    char *libxl_domain_snapshot_to_json(libxl_ctx *ctx,
                                        libxl_domain_snapshot *p);
    int libxl_domain_snapshot_from_json(libxl_ctx *ctx,
                                        libxl_domain_snapshot *p, const char *s);
    char *libxl_disk_snapshot_to_json(libxl_ctx *ctx,
                                      libxl_disk_snapshot *p);
    int libxl_disk_snapshot_from_json(libxl_ctx *ctx,
                                      libxl_disk_snapshot *p, const char *s);

    These functions will be used internally, and are very userful when load/store
    domain snapshot configuration file (libxl-json format).

    Struct Details:

    libxl_disk_snapshot = Struct("disk_snapshot",[
        ("device",        string),              /* The name of disk: hda, hdc */

        ("name",          string),              /* The name of disk snapshot.
                                                 * Usually it is inherited from
                                                 * libxl_domain_snapshot.
                                                 */

        ("file",          string),              /* The new disk file after
                                                 * external snapshot. empty for
                                                 * internal snapshot.
                                                 */

        ("format",        libxl_disk_format),   /* The format of external
                                                 * snapshot file. For the
                                                 * internal snapshot, it's
                                                 * ignored and it should be
                                                 * LIBXL_DISK_FORMAT_UNKNOWN
                                                 */

        ("path",          string),              /* The path of current disk
                                                 * backend. It gets from
                                                 * libxl_device_disk_getinfo.
                                                 * It will be force-empty when
                                                 * store domain snapshot
                                                 * configuration in order to
                                                 * hide this from users.
                                                 */
        ])

    libxl_domain_snapshot = Struct("domain_snapshot",[
        ("name",          string),              /* The name of the domain
                                                 * snapshot. It should not be
                                                 * empty.
                                                 */

        ("description",   string),              /* The description of snapshot.
                                                 * It could be empty.
                                                 */

        ("creation_time", uint64),              /* The creation time of domain
                                                 * snapshot which is the epoch
                                                 * second from 1, Jan 1970.
                                                 */

        ("memory",        string),              /* The path to save domain
                                                 * memory image. 'empty' means
                                                 * it is a disk-only snapshot.
                                                 * note that "yes" or "no" is
                                                 * not allowed, this is different
                                                 * from xl.snapshot.pod.5
                                                 */

        /* Following state represents the domain state in the beginning of snapshot.
         * These state gets from libxl_domain_info.
         */
        ("running",       bool),
        ("blocked",       bool),
        ("paused",        bool),
        ("shutdown",      bool),
        ("dying",         bool),

        /* The array of disk snapshot information belong to this domain snapshot. */
        ("disks", Array(libxl_disk_snapshot, "num_disks")),
        ])


2. New Functions

  2.1 Management functions for domain snapshot config file (libxl-json format).

    /* There are two type of config file relative to domain snapshot: user
     * config file and internal domain snapshot configuration file(libxl-json
     * format). The relation of the two config files are like xl.cfg and
     * libxl-json for domain configuration.
     * The user visiable config file (KEY=VALUE format) is only used for
     * creation. The internal domain snapshot config file is located at
     * "/var/lib/xen/snapshots/<domain_uuid>"\
     * snapshotdata-<snapshot_name>.libxl-json". This file is only for internal
     * usage, not for users. user should not modify the libxl-json format file.
     *
     * Currently, libvirt use XML format snapshot configuration file for user
     * both input(snapshot create) and output(snapshot-dumpxml). And libvirt
     * qemu driver store with xml format as internal usage as well.
     * For libxl, if libxl hope it is easy to migrate domain between different
     * toolstack, then all the toolstack should use the same internal config
     * file: libxl-json format. it will not affect the user experience. e.g. xl
     * will use the KEY=VALUE format while libvirt will use the xml format.
     */
    /*
     * function:  To retrieve domain snapshot configuration file contents from
     *            "/var/lib/xen/snapshots/<domain_uuid>/"snapshotdata-\
     *            <snapshot->name>.libxl-json", and store the information
     *            to @snapshot.
     * @domid:    The domain id. It is used to get the uuid of domain.
     * @snapshot: The caller should provide valid @snapshot->name. On return,
     *            @snapshot will hold the domain snapshot information retrieved
     *            from the file.
     * return value:
     *            0:  success
     *            <0: fail. Config file doesn't exist or the file format is wrong.
     */

    int libxl_load_dom_snapshot_conf(libxl_ctx *ctx, uint32_t domid,
                                     libxl_domain_snapshot *snapshot);


    /*
     * function:  To convert @snapshot to libxl-json format, and save at
     *            /var/lib/xen/snapshots/<domain_uuid>/"snapshotdata-\
     *            <snapshot->name>.libxl-json"
     * @domid:    The domain id. It is used to get the uuid of domain.
     * @snapshot: @snapshot->name should be valid name in the caller's file
     *            system. Other strings in this strcut should not be NULL. For
     *            the empty item the caller should set as "".
     * return value:
     *            0:  successful
     *            ERROR_INVAL:  snapshot name is empty
     *            <0: fail. snapshot information invalid or write file fail.
     */

    int libxl_store_dom_snapshot_conf(libxl_ctx *ctx, uint32_t domid,
                                      libxl_domain_snapshot *snapshot);


    /*
     * function:  To delete configuration file of indicated domain snapshot:
     *            /var/lib/xen/snapshots/<domain_uuid>/"snapshotdata-\
     *            <snapshot->name>.libxl-json"
     * @domid:    The domain id. It is used to get the uuid of domain.
     * @snapshot: The caller should provide valid @snapshot->name. other value
     *            of this struct is ignored in this function.
     * return value:
     *            0:  successful
     *            <0: fail. file delete fail.
     */

    int libxl_delete_dom_snapshot_conf(libxl_ctx *ctx, uint32_t domid,
                                       libxl_domain_snapshot *snapshot);


    /*
     * function: To retrieve all snapshot info of the speicfic domain from
     *           /var/lib/xen/snapshots/<domain_uuid>", and return the
     *           result to @libxl_domain_snapshot array. Put number of
     *           snapshot(s) to @num. The caller is responsible for free
     *           the libxl_domain_snapshot array.
     *           This is useful when toolstack want to get all the snapshot
     *           information relative to the specific domain. e.g. xl
     *           snapshot-list or libvirt libxl driver load/reload.
     * @domid:   domain id. will get domain_uuid from domid.
     * @num:     hold the number of snapshot(s) as part of the return result.
     * return value:
     *           NULL:     no valid snapshot configuration for such domain.
     *           non-NULL: successful
     */

    libxl_domain_snapshot *
    libxl_list_dom_snapshot(libxl_ctx *ctx, uint32_t domid, int *num);


  2.2 functions for disk snapshot operations

    /*
     * function:  To create disk(s) snapshot according to config in @snapshot
     *            array. Disk (one or more) snapshot in this operation is
     *            handled by qmp transaction. The transaction operation ensures
     *            that all disks are consistent. This function is used in
     *            'domain snapshot create'.
     * @domid:    domain id
     * @snapshot: array of disk snapshot
     * @num:      number of disk snapshot struct in above array
     * return value:
     *            0:   successful
     *            <0:  fail
     */

    int libxl_disk_snapshot_create(libxl_ctx *ctx, int domid,
                                   libxl_disk_snapshot *snapshot, int num);


    /*
     * function:  To delete disk snapshot according to the config in @snapshot
     *            array. Only the internal snapshot is supported currently. It
     *            will call blockdev-snapshot-delete-internal-sync qmp
     *            command for each disk snapshot delete operation.
     * @domid:    domain id
     * @snapshot: array of disk snapshot
     * @num:      number of disk snapshot struct in above array
     * return value:
     *            0:   successful
     *            <0:  fail.
     */

    int libxl_disk_snapshot_delete(libxl_ctx *ctx, int domid,
                                   libxl_disk_snapshot *snapshot, int num);

    /*
     * function:  To revert the disk snapshot state according to @snapshot
     *            array. Since there is no qmp command to use and we cannot
     *            re-send paramters to inform it about the snapshot
     *            info when qemu running, so we will call "qemu-img snapshot\
     *            -a snapshot_name" to do revert operation. (better ideas??)
     * @domid:    domain id
     * @snapshot: array of disk snapshot
     * @num:      number of disk snapshot struct in above array
     * return value:
     *            0:   successful
     *            <0:  fail
     */
    int libxl_disk_snapshot_revert(libxl_ctx *ctx, int domid,
                                   libxl_disk_snapshot *snapshot, int num);

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC V5 3/5] docs: manpage for xl snapshot command
  2014-07-07  7:45 [RFC V5 0/5] domain snapshot document and discussion Bamvor Jian Zhang
  2014-07-07  7:46 ` [RFC V5 1/5] docs: libxl Domain Snapshot HOWTO Bamvor Jian Zhang
  2014-07-07  7:46 ` [RFC V5 2/5] Libxl Domain Snapshot API Design Bamvor Jian Zhang
@ 2014-07-07  7:46 ` Bamvor Jian Zhang
  2014-07-07  7:46 ` [RFC V5 4/5] xl snapshot-xxx implementation details Bamvor Jian Zhang
  2014-07-07  7:46 ` [RFC V5 5/5] docs: man page for xl.snapshot.cfg Bamvor Jian Zhang
  4 siblings, 0 replies; 14+ messages in thread
From: Bamvor Jian Zhang @ 2014-07-07  7:46 UTC (permalink / raw
  To: xen-devel
  Cc: zzhou, ian.campbell, hahn, ian.jackson, cyliu, jfehlig, bjzhang,
	anthony.perard, davidkiarie4

This file will be a patch for docs/misc/xl.pod.1

=head1 SNAPSHOT

There are two types of snapshots supported by libxl: disk snapshot and domain
snapshot and four types of operations: create, delete, list and revert.

Disk snapshot will only be crash-consistent if the domain is running. Disk
snapshots can also be internal (qcow2) or external (snapshot in one file, delta
in another).

Domain snapshots include disk snapshots and domain state, allowing to resume
the domain from the same state when the snapshot was created. This type of
snapshot is also referred to as a domain checkpoint or system checkpoint.

=over 4

=item B<snapshot-create> [I<configfile>] [I<OPTIONS>] I<domain-id>

create domain/disk snapshot.

The create subcommand takes a config file as first argument: see
L<xl.snapshot.cfg(5)> for full details of that file format and possible options.
If I<configfile> is missing, B<XL> creates the snapshot of domain from options.
I<configfile> has to be an absolute path to a file.
It is allowed to use both I<configfile> and I<OPTIONS>. Any conflict will be
reportted and the program will exit. However, it is highly recommended that use
I<configfile> or I<OPTIONS> alone.
This command allows dryrun mode. It will print domain snapshot configuration in
libxl-json format after parsing options and the config file and checking the
flags conflict.

B<OPTIONS>

=over 4

=item B<-n>

domain/disk snapshot name. if ignored, it will be the epoch second from 1, Jan
1970.

=item B<-D>, B<--disk-only>

create the internal disk snapshot for all disk(s) except the read-only disk
(e.g. cdrom). For external disk snapshot, user should give the proper options
(such as the external file and format) in I<configfile>.

=back

=item B<snapshot-delete> [I<OPTIONS>] I<domain-id>

delete domain/disk snapshot according to the snapshot configuration file, which
may include the memory check point file, the disk snapshot state/file and the
configuration file.

B<OPTIONS>

=over 4

=item B<-n>

snapshot name.

=back

=item B<snapshot-list> [I<OPTIONS>] I<domain-id>

Prints information for one or all snapshots for <domain-id>. If no snapshots
are specified it prints out information about all snapshots for specific domain

B<OPTIONS>

=over 4

=item B<-n>

snapshot name.

=item B<-l>, B<--long>

list the details for specific snapshot or all snapshots of specific domain.

=back

=item B<snapshot-revert> [I<OPTIONS>] I<domain-id>

revert domain snapshot according to domain snapshot configuration file.

B<OPTIONS>

=over 4

=item B<-n>

snapshot name.

=back

=back

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC V5 4/5] xl snapshot-xxx implementation details
  2014-07-07  7:45 [RFC V5 0/5] domain snapshot document and discussion Bamvor Jian Zhang
                   ` (2 preceding siblings ...)
  2014-07-07  7:46 ` [RFC V5 3/5] docs: manpage for xl snapshot command Bamvor Jian Zhang
@ 2014-07-07  7:46 ` Bamvor Jian Zhang
  2014-07-07  7:46 ` [RFC V5 5/5] docs: man page for xl.snapshot.cfg Bamvor Jian Zhang
  4 siblings, 0 replies; 14+ messages in thread
From: Bamvor Jian Zhang @ 2014-07-07  7:46 UTC (permalink / raw
  To: xen-devel
  Cc: zzhou, ian.campbell, hahn, ian.jackson, cyliu, jfehlig, bjzhang,
	anthony.perard, davidkiarie4

xl snapshot-xxx implementation details

Q: Why there is no universe API for domain snapshot?
A: This is my initial target while working on snapshot. with the common API,
   different toolstack(xl or libvirt) could call the same api for the same
   operation. life would be eaiser compare to the domain create, restore and
   ...

   The reason why I could not provide common API is that it is hard to handle
   the ao things in api. e.g. in domain snapshot create, libvirt may wait the
   memory save by waiting the ao complete flag.

   Another reason is that I could share more functions in xl command
   with xl snapshot command if i do not need to provide the common api. share
   the code mean easy to maintenance.

1, "xl snapshot-create"

    1), parse domain snapshot configuration file.
    2), fill all the variable including libxl_disk_snapshot array in
        libxl_domain_snapshot struct according to options or config file.
    3), save domain memory through save_domain if it is not a disk only
        snapshot.
    4), take disk snapshot by calling libxl_disk_snapshot_create() with
        libxl_domain_snapshot struct as parameter.
    5), save snapshot configuration(libxl-json format) to
        "/var/lib/xen/snapshots/domain_uuid/snapshotdata-<snapshot_name>\
        .libxl-json"
        by calling libxl_store_dom_snapshot_conf().

2, "xl snapshot-list"

    1), read all the domain snapshot configuration file(libxl-json format) under
        "/var/lib/xen/snapshots/domain_uuid". Parse each file and fill in
        libxl_domain_snapshot struct.
    2), display short or full information from these libxl_domain_snapshot array
        for one or all snapshots depend on "-n your_snapshot_name" options

3, "xl snapshot-delete"

    1), read snapshot configuration file from
        "/var/lib/xen/snapshots/domain_uuid/snapshotdata-<snapshot_name>\
        .libxl-json"
    2), delete snapshot and other related files/state mentioned in above config
        file.
        (1), delete memory image if it is not a disk only snapshot.
        (2), delete disk snapshot date by calling libxl_disk_snapshot_delete()
        (3), delete domain snapshot configuration by calling
             libxl_delete_dom_snapshot_conf()

4, "xl snapshot-revert"

    1), destroy the running domain by libxl_domain_destroy(). If domain is not
        running, skip this step.
    2), revert disk snapshot by calling libxl_disk_snapshot_revert().
    3), restore domain stae through create_domain functions in xl_cmdimpl.c

    Idealy, I should use libxl__xc_domain_restore for domain memory restore.
    But there will be issue when the domain configuration is different between
    current time and snapshot time. In my current implementation, I destroy
    the current domain, and after reverting disk snapshot restore the domain
    through create_domain with proper dom_info. So, the revert is not pull
    the domain status back. It is different from qemu snapshot implementation,
    qemu is pull the domain back to the snapshot point with loadvm hmp.


NOT DO:

1. non-qdisk backend. (Currently only qcow2 and raw are supported)

   After some discussion with Jim, it seems that the
   snapshot of these devices could be done by difference utils through
   libxl__exec. For tap/tap2, td-util or vhd-util.

   For lvm, snapshot create, delete, revert could be done by lvcreate,
   lvremove, lvconvert respectively.

2. live snapshot.

   I do not know how it should work exactly. If we talk about live memory save,
   I guess it is simialr to live migration. but i am not sure how to sync with
   live memory save and disk transaction(for creating disk snapshot).

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC V5 5/5] docs: man page for xl.snapshot.cfg
  2014-07-07  7:45 [RFC V5 0/5] domain snapshot document and discussion Bamvor Jian Zhang
                   ` (3 preceding siblings ...)
  2014-07-07  7:46 ` [RFC V5 4/5] xl snapshot-xxx implementation details Bamvor Jian Zhang
@ 2014-07-07  7:46 ` Bamvor Jian Zhang
  4 siblings, 0 replies; 14+ messages in thread
From: Bamvor Jian Zhang @ 2014-07-07  7:46 UTC (permalink / raw
  To: xen-devel
  Cc: zzhou, ian.campbell, hahn, ian.jackson, cyliu, jfehlig, bjzhang,
	anthony.perard, davidkiarie4

The following content will be the file: "docs/man/xl.snapshot.conf.pod.5"

=head1 NAME

xl.snapshot.cfg - XL Domain Snapshot Configuration File Syntax

=head1 DESCRIPTION

Without the snapshot configuration file, xl snapshot-create could create
internal domain snapshot or internal disk snapshot. To create a user-defined
domain snapshot, xl requires a domain snapshot config file. It is typically
located at `/var/lib/xen/snapshots/UUID/snapshotdata-d.NAME.libxl-json` where
UUID is the uuid of domain and the NAME is the snapshot name. See
snapshot-HOWTO.txt for use cases and examples.

=head1 SYNTAX

A domain config file consists of a series of C<KEY=VALUE> pairs. It shares the
same rules with xl.cfg

=head1 OPTIONS

=over 4

=item B<name="NAME">

Specifies the name of the domain snapshot. Names of snapshots existing in one
domain must be unique. If it is not provided, it will be the epoch second from
1, Jan 1970.

=item B<description="DESCRIPTION">

Optional. The snapshot description.

=item B<creationtime="CREATIONTIME">

The creation time of this snapshot. It will be the epoch second from 1, Jan
1970. This field could not be specified by user, it will be ignored in domain
snapshot creation.

=item B<memory="MEMORY">

Describes the location of memory save image. This image is as same as the image
in "xl save". The value could be the absolute path of the memory save image or
"yes" which means `/var/lib/xen/snapshots/UUID/NAME.save`. if it is "no" or is
not provied, this snapshot will be a disk only snapshot.

=item B<type="TYPE">

Describes the domain snapshot type. The possible value are "internal",
"external". if it is not provided, this snapshot will be a internal snapshot.
Note that each disk is allowed to choice their own snapshot type which is
determined by what information is provided in DISK_SPEC_STRING

=item B<disk=[ "DISK_SPEC_STRING", "DISK_SPEC_STRING", ...]>

The disk snapshot description. See F<docs/misc/xl-disk-configuration.txt> for
rules. For internal snapshot, user should only provide vdev. For external
snapshot, user should provide vdev, format and target. Format is the format of
external snapshot file, only qcow2 is allowed at this time. Target is disk file
after the external snapshot including absolute path.

=back

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC V5 2/5] Libxl Domain Snapshot API Design
  2014-07-07  7:46 ` [RFC V5 2/5] Libxl Domain Snapshot API Design Bamvor Jian Zhang
@ 2014-07-11 12:47   ` Ian Campbell
  2014-07-11 13:19     ` Ian Jackson
  2014-07-14 14:42     ` Bamvor Jian Zhang
  0 siblings, 2 replies; 14+ messages in thread
From: Ian Campbell @ 2014-07-11 12:47 UTC (permalink / raw
  To: Bamvor Jian Zhang
  Cc: zzhou, hahn, ian.jackson, cyliu, xen-devel, jfehlig,
	anthony.perard, davidkiarie4

On Mon, 2014-07-07 at 15:46 +0800, Bamvor Jian Zhang wrote:
> Libxl Domain Snapshot API Design

Thanks for splitting the libxl layer into a separate document. I think
it is useful to consider this bit first in isolation before getting hung
up on how xl might make use of it.

> 1. New Structures
> 
>     Domain snapshot introduces two new structures:
>     - "libxl_domain_snapshot" store domain snapshot information, it contains
>        libxl_disk_snapshot array.
>     - "libxl_disk_snapshot" stores disk snapshot related information.

>     Struct Details:
> 
>     libxl_disk_snapshot = Struct("disk_snapshot",[

I wonder if this should incorporate a libxl_device_disk (either inline
or by reference), or perhaps even simply merged into the disk struct.

I think this would remove the need for the device, file, format and path
fields and be more logical IMHO.

>         ("device",        string),              /* The name of disk: hda, hdc */
> 
>         ("name",          string),              /* The name of disk snapshot.
>                                                  * Usually it is inherited from
>                                                  * libxl_domain_snapshot.
>                                                  */
> 
>         ("file",          string),              /* The new disk file after
>                                                  * external snapshot. empty for
>                                                  * internal snapshot.
>                                                  */
> 
>         ("format",        libxl_disk_format),   /* The format of external
>                                                  * snapshot file. For the
>                                                  * internal snapshot, it's
>                                                  * ignored and it should be
>                                                  * LIBXL_DISK_FORMAT_UNKNOWN
>                                                  */
> 
>         ("path",          string),              /* The path of current disk
>                                                  * backend. It gets from
>                                                  * libxl_device_disk_getinfo.
>                                                  * It will be force-empty when
>                                                  * store domain snapshot
>                                                  * configuration in order to
>                                                  * hide this from users.
>                                                  */
>         ])
> 
>     libxl_domain_snapshot = Struct("domain_snapshot",[
>         ("name",          string),              /* The name of the domain
>                                                  * snapshot. It should not be
>                                                  * empty.
>                                                  */
> 
>         ("description",   string),              /* The description of snapshot.
>                                                  * It could be empty.
>                                                  */
> 
>         ("creation_time", uint64),              /* The creation time of domain
>                                                  * snapshot which is the epoch
>                                                  * second from 1, Jan 1970.
>                                                  */
> 
>         ("memory",        string),              /* The path to save domain
>                                                  * memory image. 'empty' means
>                                                  * it is a disk-only snapshot.
>                                                  * note that "yes" or "no" is
>                                                  * not allowed, this is different
>                                                  * from xl.snapshot.pod.5

Encoding all of that into a string with some strings having magic
properties isn't the right design.

This should be a defbool or a bool indicating whether or not memory is
included and a separate string which is the path if it is included.

>         /* Following state represents the domain state in the beginning of snapshot.
>          * These state gets from libxl_domain_info.

What is this used for?

>          */
>         ("running",       bool),
>         ("blocked",       bool),
>         ("paused",        bool),
>         ("shutdown",      bool),
>         ("dying",         bool),
> 
>         /* The array of disk snapshot information belong to this domain snapshot. */
>         ("disks", Array(libxl_disk_snapshot, "num_disks")),
>         ])
> 
> 
> 2. New Functions
> 
>   2.1 Management functions for domain snapshot config file (libxl-json format).
> 
>     /* There are two type of config file relative to domain snapshot: user
>      * config file and internal domain snapshot configuration file(libxl-json
>      * format). The relation of the two config files are like xl.cfg and
>      * libxl-json for domain configuration.
>      * The user visiable config file (KEY=VALUE format) is only used for
>      * creation. The internal domain snapshot config file is located at
>      * "/var/lib/xen/snapshots/<domain_uuid>"\
>      * snapshotdata-<snapshot_name>.libxl-json". This file is only for internal
>      * usage, not for users. user should not modify the libxl-json format file.
>      *
>      * Currently, libvirt use XML format snapshot configuration file for user
>      * both input(snapshot create) and output(snapshot-dumpxml). And libvirt
>      * qemu driver store with xml format as internal usage as well.
>      * For libxl, if libxl hope it is easy to migrate domain between different
>      * toolstack, then all the toolstack should use the same internal config
>      * file: libxl-json format. it will not affect the user experience. e.g. xl
>      * will use the KEY=VALUE format while libvirt will use the xml format.
>      */

There is a bunch of stuff here which IMHO does not belong in the libxl
API. In general libxl should be providing the mechanisms for doing
things but the policies and management (i.e. where to save things,
listing, deleting etc) belong in the application.

So all of the stuff to do with config file parsing and specification of
paths where things should be stored are issues for the toolstack and not
libxl. i.e. I'm sure libvirt has its own ideas about these things
already, which we should use, and I would expect things like the
location for the disk snapshots to be passed to xl snapshot somehow.

AFAICT the libxl API should be
    libxl_domain_snapshot_save(libxl_ctx *ctx, uint32_t domid,
                const char *name, libxl_domain_snapshot *snapshot)

Which should take a snapshot of domain domid using parameters from the
snapshot object and update snapshot with the specifics. The caller would
have to supply any necessary paths etc in the snapshot object. It might
be better to split libxl_domain_snapshot onto two, one representing the
parameters for a snapshot and one representing the resulting snapshot.

libxl shouldn't be doing anything with parsing json objects, storing
snapshot config files or picking/mandating the output paths or anything
like that.

Of course the application might choose to make use of the very
convenient libxl functions for converting libxl structs to/from json.
libvirt has an XML config format already, there is no reason not to use
it in that context.

Likewise on snapshot restore (or revert):
    libxl_domain_snapshot_restore(ctx, libxl_domain_snapshot *snapshot, 
                                  *r_domid)
should take the snapshot and make a new domain out of it. (someone would
need to specify what happens if another domain exists already in that
snapshot chain etc).

Any management of sets of snapshots, paths to be saving them in,
listing, deleting etc is down to the application.


>   2.2 functions for disk snapshot operations

Do these need to be exposed separately? You lbixl_domain_snapshot struct
already has a mechanism for indicating whether memory should be included
in the snapshot, so aren't all these functions equiavalent to the
domain_snapshot ones but with memory=no?

Anyway, if you think these are necessary then I think the same general
comments apply. libxl should be providing a mechanism for taking a
snapshot into an application provided location. But the application is
responsible for managing them, listing them, deciding where they should
live etc.

Ian.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC V5 2/5] Libxl Domain Snapshot API Design
  2014-07-11 12:47   ` Ian Campbell
@ 2014-07-11 13:19     ` Ian Jackson
  2014-07-11 15:43       ` Bamvor Jian Zhang
  2014-07-14 14:42     ` Bamvor Jian Zhang
  1 sibling, 1 reply; 14+ messages in thread
From: Ian Jackson @ 2014-07-11 13:19 UTC (permalink / raw
  To: Ian Campbell
  Cc: zzhou, hahn, cyliu, xen-devel, jfehlig, Bamvor Jian Zhang,
	anthony.perard, davidkiarie4

Ian Campbell writes ("Re: [RFC V5 2/5] Libxl Domain Snapshot API Design"):
> On Mon, 2014-07-07 at 15:46 +0800, Bamvor Jian Zhang wrote:
...
> >         ("memory",        string),       /* The path to save domain
> >                                           * memory image. 'empty' means
> >                                           * it is a disk-only snapshot.
> >                                           * note that "yes" or "no" is
> >                                           * not allowed, this is different
> >                                           * from xl.snapshot.pod.5

As I wrote on IRC:

I still don't really understand how the distinction between snapshots
with memory=yes and memory=no is supposed to operate.  Is it that if
you take a snapshot of a running domain with memory=no, and then later
resume it, it is as if the guest had crashed at the time of the
snapshot ?  (How does this interact with disk IO barriers?)

I think the semantics should be spelled out somewhere.

(Also I assume that a memory snapshot isn't available unless there's a
running domain, but that also needs to be specified.)

Thanks,
Ian.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC V5 2/5] Libxl Domain Snapshot API Design
  2014-07-11 13:19     ` Ian Jackson
@ 2014-07-11 15:43       ` Bamvor Jian Zhang
  0 siblings, 0 replies; 14+ messages in thread
From: Bamvor Jian Zhang @ 2014-07-11 15:43 UTC (permalink / raw
  To: Ian Campbell, Ian Jackson
  Cc: Zhiqiang Zhou, hahn, Chun Yan Liu, xen-devel, Jim Fehlig,
	anthony.perard, davidkiarie4

Hi, Ian

> Ian Campbell writes ("Re: [RFC V5 2/5] Libxl Domain Snapshot API Design"):
> > On Mon, 2014-07-07 at 15:46 +0800, Bamvor Jian Zhang wrote:
> ...
> > >         ("memory",        string),       /* The path to save domain
> > >                                           * memory image. 'empty' means
> > >                                           * it is a disk-only snapshot.
> > >                                           * note that "yes" or "no" is
> > >                                           * not allowed, this is different
> > >                                           * from xl.snapshot.pod.5
>
> As I wrote on IRC:
sorry, i forget logging out my irc when i leave company.
>
> I still don't really understand how the distinction between snapshots
> with memory=yes and memory=no is supposed to operate.  Is it that if
> you take a snapshot of a running domain with memory=no, and then later
> resume it, it is as if the guest had crashed at the time of the
> snapshot ?  (How does this interact with disk IO barriers?)
yes, as you said, the domain may crash after disk stae resume if
there is no memory state resume.
basically, this is compatible with libvirt domain snapshot, ref[1].
after read the libvirt and qemu code again, i found that there is a cpu
stop qmp before create disk only snapshot when domain is running. such
qmp will call bdrv_drain_all and bdrv_flush_all for flush all data to
disk.
compare to libxl, in my current code, i call libxl_domain_pause before
creating disk only snapshot. it seems that libxl_domain_pause cause the
vcpu stop through hypercall. and i do not see the similar flush date to
disk operation along with vcpus stop.
how should i deal with it? could i call stop cpu qmp after
libxl_domain_pause in order to flush data to disk?
>
> I think the semantics should be spelled out somewhere.
>
> (Also I assume that a memory snapshot isn't available unless there's a
> running domain, but that also needs to be specified.)
yes, definitely. i will add this to the document.

regards

bamvor

[1] http://libvirt.org/formatsnapshot.html
>
> Thanks,
> Ian.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC V5 2/5] Libxl Domain Snapshot API Design
  2014-07-11 12:47   ` Ian Campbell
  2014-07-11 13:19     ` Ian Jackson
@ 2014-07-14 14:42     ` Bamvor Jian Zhang
  2014-07-14 15:18       ` Ian Campbell
  1 sibling, 1 reply; 14+ messages in thread
From: Bamvor Jian Zhang @ 2014-07-14 14:42 UTC (permalink / raw
  To: Ian Campbell
  Cc: Zhiqiang Zhou, hahn, ian.jackson, Chun Yan Liu, xen-devel,
	Jim Fehlig, anthony.perard, davidkiarie4

Hi, Ian

thanks your reply. your suggestion is very important for me. meanwhile i am
not sure i could follow you all the time. please correct me, thanks.
> On Mon, 2014-07-07 at 15:46 +0800, Bamvor Jian Zhang wrote:
> > Libxl Domain Snapshot API Design
>
> Thanks for splitting the libxl layer into a separate document. I think
> it is useful to consider this bit first in isolation before getting hung
> up on how xl might make use of it.
>
> > 1. New Structures
> >
> >     Domain snapshot introduces two new structures:
> >     - "libxl_domain_snapshot" store domain snapshot information, it contains
> >        libxl_disk_snapshot array.
> >     - "libxl_disk_snapshot" stores disk snapshot related information.
>
> >     Struct Details:
> >
> >     libxl_disk_snapshot = Struct("disk_snapshot",[
>
> I wonder if this should incorporate a libxl_device_disk (either inline
> or by reference), or perhaps even simply merged into the disk struct.
>
> I think this would remove the need for the device, file, format and path
> fields and be more logical IMHO.
yes, embedded the libxl_device_disk into libxl_disk_snapshot may be useful
for some logic, e.g. check whether the disk could snaphot, readonly or cdrom
do not support snapshot.
meanwhile for the external snapshot, there are two pdev_path: the one is the
disk path before snapshot, the other is disk path after external snapshot.
functions relative to external snapshot may not use the both pdev_path at
the same time, but it seems more clear if we have two pdev_path.
and there is a additional format for the new disk path above. So, the
libxl_disk_snapshot would be:
     libxl_disk_snapshot = Struct("disk_snapshot",[
        ("name",          string),                /* The name of disk snapshot. */
        ("type",          libxl_snapshot_format), /* snapshot type: internal,
                                                   * external
                                                   */
        ("pdev_path_new", string),),              /* The new disk file after
                                                    * external snapshot. empty for
                                                   * internal snapshot.
                                                   */
        ("format_new",    libxl_disk_format),     /* The format of external
                                                   * snapshot
                                                   */
        ("disk",          libxl_device_disk),
        ])

>
>
> >         ("device",        string),              /* The name of disk: hda, hdc */
> >
> >         ("name",          string),              /* The name of disk snapshot.
> >                                                  * Usually it is inherited from
> >                                                  * libxl_domain_snapshot.
> >                                                  */
> >
> >         ("file",          string),              /* The new disk file after
> >                                                  * external snapshot. empty for
> >                                                  * internal snapshot.
> >                                                  */
> >
> >         ("format",        libxl_disk_format),   /* The format of external
> >                                                  * snapshot file. For the
> >                                                  * internal snapshot, it's
> >                                                  * ignored and it should be
> >                                                  * LIBXL_DISK_FORMAT_UNKNOWN
> >                                                  */
> >
> >         ("path",          string),              /* The path of current disk
> >                                                  * backend. It gets from
> >                                                  * libxl_device_disk_getinfo.
> >                                                  * It will be force-empty when
> >                                                  * store domain snapshot
> >                                                  * configuration in order to
> >                                                  * hide this from users.
> >                                                  */
> >         ])
> >
> >     libxl_domain_snapshot = Struct("domain_snapshot",[
> >         ("name",          string),              /* The name of the domain
> >                                                  * snapshot. It should not be
> >                                                  * empty.
> >                                                  */
> >
> >         ("description",   string),              /* The description of snapshot.
> >                                                  * It could be empty.
> >                                                  */
> >
> >         ("creation_time", uint64),              /* The creation time of domain
> >                                                  * snapshot which is the epoch
> >                                                  * second from 1, Jan 1970.
> >                                                  */
> >
> >         ("memory",        string),              /* The path to save domain
> >                                                  * memory image. 'empty' means
> >                                                  * it is a disk-only snapshot.
> >                                                  * note that "yes" or "no" is
> >                                                  * not allowed, this is different
> >                                                  * from xl.snapshot.pod.5
>
> Encoding all of that into a string with some strings having magic
> properties isn't the right design.
>
> This should be a defbool or a bool indicating whether or not memory is
> included and a separate string which is the path if it is included.
i will change it in docs/man/xl.snapshot.pod.5
for the libxl_domain_snapshot, the memory attribute only support absolute path
of memory image.
>
> >         /* Following state represents the domain state in the beginning of snapshot.
> >          * These state gets from libxl_domain_info.
>
> What is this used for?
recording the state of domain while take the snapshot. basically, i add it because
libvirt need track the domain state: running or paused.
maybe i should record the above two state directly?
(running in libvirt means running or blocked in libxl).
>
> >          */
> >         ("running",       bool),
> >         ("blocked",       bool),
> >         ("paused",        bool),
> >         ("shutdown",      bool),
> >         ("dying",         bool),
> >
> >         /* The array of disk snapshot information belong to this domain snapshot. */
> >         ("disks", Array(libxl_disk_snapshot, "num_disks")),
> >         ])
> >
> >
> > 2. New Functions
> >
> >   2.1 Management functions for domain snapshot config file (libxl-json format).
> >
> >     /* There are two type of config file relative to domain snapshot: user
> >      * config file and internal domain snapshot configuration file(libxl-json
> >      * format). The relation of the two config files are like xl.cfg and
> >      * libxl-json for domain configuration.
> >      * The user visiable config file (KEY=VALUE format) is only used for
> >      * creation. The internal domain snapshot config file is located at
> >      * "/var/lib/xen/snapshots/<domain_uuid>"\
> >      * snapshotdata-<snapshot_name>.libxl-json". This file is only for internal
> >      * usage, not for users. user should not modify the libxl-json format file.
> >      *
> >      * Currently, libvirt use XML format snapshot configuration file for user
> >      * both input(snapshot create) and output(snapshot-dumpxml). And libvirt
> >      * qemu driver store with xml format as internal usage as well.
> >      * For libxl, if libxl hope it is easy to migrate domain between different
> >      * toolstack, then all the toolstack should use the same internal config
> >      * file: libxl-json format. it will not affect the user experience. e.g. xl
> >      * will use the KEY=VALUE format while libvirt will use the xml format.
> >      */
>
> There is a bunch of stuff here which IMHO does not belong in the libxl
> API. In general libxl should be providing the mechanisms for doing
> things but the policies and management (i.e. where to save things,
> listing, deleting etc) belong in the application.
agree. i should move it the other document.
>
> So all of the stuff to do with config file parsing and specification of
> paths where things should be stored are issues for the toolstack and not
> libxl. i.e. I'm sure libvirt has its own ideas about these things
> already, which we should use, and I would expect things like the
> location for the disk snapshots to be passed to xl snapshot somehow.
>
> AFAICT the libxl API should be
>     libxl_domain_snapshot_save(libxl_ctx *ctx, uint32_t domid,
>                 const char *name, libxl_domain_snapshot *snapshot)
>
> Which should take a snapshot of domain domid using parameters from the
> snapshot object and update snapshot with the specifics.
yes, this is the issue i want to discuss. i write it in 4/5. sorry for
confuse.
Q: Why there is no universe API for domain snapshot?
A: This is my initial target while working on snapshot. with the common API,
   different toolstack(xl or libvirt) could call the same api for the same
   operation. life would be eaiser compare to the domain create, restore and
   ...

   The reason why I could not provide common API is that it is hard to handle
   the ao things in api. e.g. in domain snapshot create, libvirt may wait the
   memory save by waiting the ao complete flag.

   Another reason is that I could share more functions in xl command
   with xl snapshot command if i do not need to provide the common api. share
   the code mean easy to maintenance.
> The caller would
> have to supply any necessary paths etc in the snapshot object. It might
> be better to split libxl_domain_snapshot onto two, one representing the
> parameters for a snapshot and one representing the resulting snapshot.
>
> libxl shouldn't be doing anything with parsing json objects, storing
> snapshot config files or picking/mandating the output paths or anything
> like that.
yes, i agree that libxl should not picking/mandating the output paths. it
should be determined by application. currently, in my code, all the paths
is get from user config file or generated by xl not libxl.
>
> Of course the application might choose to make use of the very
> convenient libxl functions for converting libxl structs to/from json.
> libvirt has an XML config format already, there is no reason not to use
> it in that context.
yes, libvirt use the XML config. i provide the load/store json config api
in order to migrate between xl and libvirt. what i mean is that when libvirt
libxl driver load/reload, it could get the snapshot created by libxl through
libxl_list_dom_snapshot.
if libvirt libxl driver could record the snapshot configuration with both
XML and libxl-json(through libxl_store_dom_snapshot_conf) format. then xl
could easily access the snapshot state created by libvirt libxl driver.
>
> Likewise on snapshot restore (or revert):
>     libxl_domain_snapshot_restore(ctx, libxl_domain_snapshot *snapshot,
>                                   *r_domid)
> should take the snapshot and make a new domain out of it. (someone would
> need to specify what happens if another domain exists already in that
> snapshot chain etc).
i need to think about it. generally, if we add the
libxl_domain_snapshot_restore api, i need to think about how to expose the
ao_how to the application for more than one stage. i mean i need ao_complete
twice: the first is after i call disk_revert(qemu-img). the second is after
memory restore. is there some existence code or document show me how to do it?
>
> Any management of sets of snapshots, paths to be saving them in,
> listing, deleting etc is down to the application.
>
>
> >   2.2 functions for disk snapshot operations
>
> Do these need to be exposed separately? You lbixl_domain_snapshot struct
> already has a mechanism for indicating whether memory should be included
> in the snapshot, so aren't all these functions equiavalent to the
> domain_snapshot ones but with memory=no?
the reasone why there are disk snapshot opration is there is no domain
snapshot operation(at least create, revert) in my current design. if we
could provide the domain snapshot operation, there is no need to expose
current disk snapshot operation.
>
> Anyway, if you think these are necessary then I think the same general
> comments apply. libxl should be providing a mechanism for taking a
> snapshot into an application provided location. But the application is
> responsible for managing them, listing them, deciding where they should
> live etc.
understand.

thanks

bamvor
>
> Ian.
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC V5 2/5] Libxl Domain Snapshot API Design
  2014-07-14 14:42     ` Bamvor Jian Zhang
@ 2014-07-14 15:18       ` Ian Campbell
  2014-07-16  9:27         ` Bamvor Jian Zhang
  0 siblings, 1 reply; 14+ messages in thread
From: Ian Campbell @ 2014-07-14 15:18 UTC (permalink / raw
  To: Bamvor Jian Zhang
  Cc: Zhiqiang Zhou, hahn, ian.jackson, Chun Yan Liu, xen-devel,
	Jim Fehlig, anthony.perard, davidkiarie4

On Mon, 2014-07-14 at 08:42 -0600, Bamvor Jian Zhang wrote:
> Hi, Ian
> 
> thanks your reply. your suggestion is very important for me. meanwhile i am
> not sure i could follow you all the time. please correct me, thanks.
> > On Mon, 2014-07-07 at 15:46 +0800, Bamvor Jian Zhang wrote:
> > > Libxl Domain Snapshot API Design
> >
> > Thanks for splitting the libxl layer into a separate document. I think
> > it is useful to consider this bit first in isolation before getting hung
> > up on how xl might make use of it.
> >
> > > 1. New Structures
> > >
> > >     Domain snapshot introduces two new structures:
> > >     - "libxl_domain_snapshot" store domain snapshot information, it contains
> > >        libxl_disk_snapshot array.
> > >     - "libxl_disk_snapshot" stores disk snapshot related information.
> >
> > >     Struct Details:
> > >
> > >     libxl_disk_snapshot = Struct("disk_snapshot",[
> >
> > I wonder if this should incorporate a libxl_device_disk (either inline
> > or by reference), or perhaps even simply merged into the disk struct.
> >
> > I think this would remove the need for the device, file, format and path
> > fields and be more logical IMHO.
> yes, embedded the libxl_device_disk into libxl_disk_snapshot may be useful
> for some logic, e.g. check whether the disk could snaphot, readonly or cdrom
> do not support snapshot.
> meanwhile for the external snapshot, there are two pdev_path: the one is the
> disk path before snapshot, the other is disk path after external snapshot.
> functions relative to external snapshot may not use the both pdev_path at
> the same time, but it seems more clear if we have two pdev_path.
> and there is a additional format for the new disk path above.

Perhaps what you want is two libxl_device_disk fields then, parent and
child?

Or looking at it another way the input to a disk snapshot operation
would be a libxl_device_disk and some options and the output would be
another, different, libxl_device_disk.

> >
> >
> > >         ("device",        string),              /* The name of disk: hda, hdc */
> > >
> > >         ("name",          string),              /* The name of disk snapshot.
> > >                                                  * Usually it is inherited from
> > >                                                  * libxl_domain_snapshot.
> > >                                                  */
> > >
> > >         ("file",          string),              /* The new disk file after
> > >                                                  * external snapshot. empty for
> > >                                                  * internal snapshot.
> > >                                                  */
> > >
> > >         ("format",        libxl_disk_format),   /* The format of external
> > >                                                  * snapshot file. For the
> > >                                                  * internal snapshot, it's
> > >                                                  * ignored and it should be
> > >                                                  * LIBXL_DISK_FORMAT_UNKNOWN
> > >                                                  */
> > >
> > >         ("path",          string),              /* The path of current disk
> > >                                                  * backend. It gets from
> > >                                                  * libxl_device_disk_getinfo.
> > >                                                  * It will be force-empty when
> > >                                                  * store domain snapshot
> > >                                                  * configuration in order to
> > >                                                  * hide this from users.
> > >                                                  */
> > >         ])
> > >
> > >     libxl_domain_snapshot = Struct("domain_snapshot",[
> > >         ("name",          string),              /* The name of the domain
> > >                                                  * snapshot. It should not be
> > >                                                  * empty.
> > >                                                  */
> > >
> > >         ("description",   string),              /* The description of snapshot.
> > >                                                  * It could be empty.
> > >                                                  */
> > >
> > >         ("creation_time", uint64),              /* The creation time of domain
> > >                                                  * snapshot which is the epoch
> > >                                                  * second from 1, Jan 1970.
> > >                                                  */
> > >
> > >         ("memory",        string),              /* The path to save domain
> > >                                                  * memory image. 'empty' means
> > >                                                  * it is a disk-only snapshot.
> > >                                                  * note that "yes" or "no" is
> > >                                                  * not allowed, this is different
> > >                                                  * from xl.snapshot.pod.5
> >
> > Encoding all of that into a string with some strings having magic
> > properties isn't the right design.
> >
> > This should be a defbool or a bool indicating whether or not memory is
> > included and a separate string which is the path if it is included.
> i will change it in docs/man/xl.snapshot.pod.5

My question was about the libxl interface given here, not xl so I don't
think updating xl.snapshot.pod is going to be helpful.

(note that I was querying because of the use of "empty" as something
magic, not because of the comment about xl.snapshot)

> >
> > >         /* Following state represents the domain state in the beginning of snapshot.
> > >          * These state gets from libxl_domain_info.
> >
> > What is this used for?
> recording the state of domain while take the snapshot. basically, i add it because
> libvirt need track the domain state: running or paused.
> maybe i should record the above two state directly?

I think so. I think you also need to state clearly what the expected
semantics are. e.g. does "paused" incorporate I/O quiesced?

> >
> > So all of the stuff to do with config file parsing and specification of
> > paths where things should be stored are issues for the toolstack and not
> > libxl. i.e. I'm sure libvirt has its own ideas about these things
> > already, which we should use, and I would expect things like the
> > location for the disk snapshots to be passed to xl snapshot somehow.
> >
> > AFAICT the libxl API should be
> >     libxl_domain_snapshot_save(libxl_ctx *ctx, uint32_t domid,
> >                 const char *name, libxl_domain_snapshot *snapshot)
> >
> > Which should take a snapshot of domain domid using parameters from the
> > snapshot object and update snapshot with the specifics.
> yes, this is the issue i want to discuss. i write it in 4/5. sorry for
> confuse.

I'm afraid I don't understand what you are trying to say with this Q+A.

> Q: Why there is no universe API for domain snapshot?
> A: This is my initial target while working on snapshot. with the common API,
>    different toolstack(xl or libvirt) could call the same api for the same
>    operation. life would be eaiser compare to the domain create, restore and

easier in what sense?

>    ...
> 
>    The reason why I could not provide common API is that it is hard to handle
>    the ao things in api. e.g. in domain snapshot create, libvirt may wait the
>    memory save by waiting the ao complete flag.

The libxl API almost certainly does need to be async and use ao_how etc.
That's pretty much unavoidable.

I'm also confused because it appeared to me that what I was commenting
on was a common API, but here you say you cannot provide that.

>    Another reason is that I could share more functions in xl command
>    with xl snapshot command if i do not need to provide the common api. share
>    the code mean easy to maintenance.

share with what? 

Sorry, I'm totally confused about what you are trying to say in this
entire section.

> > The caller would
> > have to supply any necessary paths etc in the snapshot object. It might
> > be better to split libxl_domain_snapshot onto two, one representing the
> > parameters for a snapshot and one representing the resulting snapshot.
> >
> > libxl shouldn't be doing anything with parsing json objects, storing
> > snapshot config files or picking/mandating the output paths or anything
> > like that.
> yes, i agree that libxl should not picking/mandating the output paths. it
> should be determined by application. currently, in my code, all the paths
> is get from user config file or generated by xl not libxl.

If that is the case then I'm afraid that this document does not reflect
that reality.

> > Of course the application might choose to make use of the very
> > convenient libxl functions for converting libxl structs to/from json.
> > libvirt has an XML config format already, there is no reason not to use
> > it in that context.
> yes, libvirt use the XML config. i provide the load/store json config api
> in order to migrate between xl and libvirt. what i mean is that when libvirt
> libxl driver load/reload, it could get the snapshot created by libxl through
> libxl_list_dom_snapshot.
> if libvirt libxl driver could record the snapshot configuration with both
> XML and libxl-json(through libxl_store_dom_snapshot_conf) format. then xl
> could easily access the snapshot state created by libvirt libxl driver.

I don't think "portability" of snapshots in this way is required and it
is making your API more complex than it needs to be IMHO.

It is fine and expected that xl and libvirt snapshots are manageable
only by the toolstack which creates them.

> > Likewise on snapshot restore (or revert):
> >     libxl_domain_snapshot_restore(ctx, libxl_domain_snapshot *snapshot,
> >                                   *r_domid)
> > should take the snapshot and make a new domain out of it. (someone would
> > need to specify what happens if another domain exists already in that
> > snapshot chain etc).
> i need to think about it. generally, if we add the
> libxl_domain_snapshot_restore api, i need to think about how to expose the
> ao_how to the application for more than one stage. i mean i need ao_complete
> twice: the first is after i call disk_revert(qemu-img). the second is after
> memory restore. is there some existence code or document show me how to do it?

Why do you need to ao complete twice? The ao should complete once at the
end of the aggregate operation.

If what you want is a progress indication then perhaps the interface
needs to take one or more libxl_asyncprogress_how objects to describe
those stages. Domain create uses this to notify when the console is
ready for example.

> >
> > Any management of sets of snapshots, paths to be saving them in,
> > listing, deleting etc is down to the application.
> >
> >
> > >   2.2 functions for disk snapshot operations
> >
> > Do these need to be exposed separately? You lbixl_domain_snapshot struct
> > already has a mechanism for indicating whether memory should be included
> > in the snapshot, so aren't all these functions equiavalent to the
> > domain_snapshot ones but with memory=no?
> the reasone why there are disk snapshot opration is there is no domain
> snapshot operation(at least create, revert) in my current design. if we
> could provide the domain snapshot operation, there is no need to expose
> current disk snapshot operation.

Why don't you provide a domain snapshot operation? I thought that was
the whole point of this new interface.

Ian.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC V5 2/5] Libxl Domain Snapshot API Design
  2014-07-14 15:18       ` Ian Campbell
@ 2014-07-16  9:27         ` Bamvor Jian Zhang
  2014-07-16  9:50           ` Ian Campbell
  0 siblings, 1 reply; 14+ messages in thread
From: Bamvor Jian Zhang @ 2014-07-16  9:27 UTC (permalink / raw
  To: Ian Campbell
  Cc: Zhiqiang Zhou, hahn, ian.jackson, Chun Yan Liu, xen-devel,
	Jim Fehlig, anthony.perard, davidkiarie4

Hi, Ian
> On Mon, 2014-07-14 at 08:42 -0600, Bamvor Jian Zhang wrote:
> > Hi, Ian
> >
> > thanks your reply. your suggestion is very important for me. meanwhile i am
> > not sure i could follow you all the time. please correct me, thanks.
> > > On Mon, 2014-07-07 at 15:46 +0800, Bamvor Jian Zhang wrote:
> > > > Libxl Domain Snapshot API Design
> > >
> > > Thanks for splitting the libxl layer into a separate document. I think
> > > it is useful to consider this bit first in isolation before getting hung
> > > up on how xl might make use of it.
> > >
> > > > 1. New Structures
> > > >
> > > >     Domain snapshot introduces two new structures:
> > > >     - "libxl_domain_snapshot" store domain snapshot information, it contains
> > > >        libxl_disk_snapshot array.
> > > >     - "libxl_disk_snapshot" stores disk snapshot related information.
> > >
> > > >     Struct Details:
> > > >
> > > >     libxl_disk_snapshot = Struct("disk_snapshot",[
> > >
> > > I wonder if this should incorporate a libxl_device_disk (either inline
> > > or by reference), or perhaps even simply merged into the disk struct.
> > >
> > > I think this would remove the need for the device, file, format and path
> > > fields and be more logical IMHO.
> > yes, embedded the libxl_device_disk into libxl_disk_snapshot may be useful
> > for some logic, e.g. check whether the disk could snaphot, readonly or cdrom
> > do not support snapshot.
> > meanwhile for the external snapshot, there are two pdev_path: the one is the
> > disk path before snapshot, the other is disk path after external snapshot.
> > functions relative to external snapshot may not use the both pdev_path at
> > the same time, but it seems more clear if we have two pdev_path.
> > and there is a additional format for the new disk path above.
>
> Perhaps what you want is two libxl_device_disk fields then, parent and
> child?
yes.
>
> Or looking at it another way the input to a disk snapshot operation
> would be a libxl_device_disk and some options and the output would be
> another, different, libxl_device_disk.
Understand. There are two issues:
1. If i use the libxl_device_disk, the old path(before external snapshot)
is missing. This will be inconvenient if we revert the external snapshot.
2. When toolstack save the domain snapshot configuration file
(libxl-json), this configuration is longer than use libxl_disk_snapshot.
e.g. the libxl_device_disk output:
    "disks": [
        {
            "backend_domid": 0,
            "backend_domname": null,
            "pdev_path": "/var/lib/xen/snapshots/5c84adcc-bd59-788a-96d2-195f9b599cfe/disk_hda.qcow2",
            "vdev": "hda",
            "format": "qcow2",
            "script": null,
            "removable": 0,
            "readwrite": 1,
            "is_cdrom": 0,
            "direct_io_safe": false
        },
        ...
libxl_disk_snapshot output:
    "disks": [
        {
            "device": "hda",
            "name": "external snapshot 20140710",
            "file": "/var/lib/xen/snapshots/5c84adcc-bd59-788a-96d2-195f9b599cfe/disk_hda.qcow2",
            "format": "qcow2",
            "path": "",
            "type": "external"
        },
>
> > >
> > >
> > > >         ("device",        string),              /* The name of disk: hda, hdc */
> > > >
> > > >         ("name",          string),              /* The name of disk snapshot.
> > > >                                                  * Usually it is inherited from
> > > >                                                  * libxl_domain_snapshot.
> > > >                                                  */
> > > >
> > > >         ("file",          string),              /* The new disk file after
> > > >                                                  * external snapshot. empty for
> > > >                                                  * internal snapshot.
> > > >                                                  */
> > > >
> > > >         ("format",        libxl_disk_format),   /* The format of external
> > > >                                                  * snapshot file. For the
> > > >                                                  * internal snapshot, it's
> > > >                                                  * ignored and it should be
> > > >                                                  * LIBXL_DISK_FORMAT_UNKNOWN
> > > >                                                  */
> > > >
> > > >         ("path",          string),              /* The path of current disk
> > > >                                                  * backend. It gets from
> > > >                                                  * libxl_device_disk_getinfo.
> > > >                                                  * It will be force-empty when
> > > >                                                  * store domain snapshot
> > > >                                                  * configuration in order to
> > > >                                                  * hide this from users.
> > > >                                                  */
> > > >         ])
> > > >
> > > >     libxl_domain_snapshot = Struct("domain_snapshot",[
> > > >         ("name",          string),              /* The name of the domain
> > > >                                                  * snapshot. It should not be
> > > >                                                  * empty.
> > > >                                                  */
> > > >
> > > >         ("description",   string),              /* The description of snapshot.
> > > >                                                  * It could be empty.
> > > >                                                  */
> > > >
> > > >         ("creation_time", uint64),              /* The creation time of domain
> > > >                                                  * snapshot which is the epoch
> > > >                                                  * second from 1, Jan 1970.
> > > >                                                  */
> > > >
> > > >         ("memory",        string),              /* The path to save domain
> > > >                                                  * memory image. 'empty' means
> > > >                                                  * it is a disk-only snapshot.
> > > >                                                  * note that "yes" or "no" is
> > > >                                                  * not allowed, this is different
> > > >                                                  * from xl.snapshot.pod.5
> > >
> > > Encoding all of that into a string with some strings having magic
> > > properties isn't the right design.
> > >
> > > This should be a defbool or a bool indicating whether or not memory is
> > > included and a separate string which is the path if it is included.
> > i will change it in docs/man/xl.snapshot.pod.5
>
> My question was about the libxl interface given here, not xl so I don't
> think updating xl.snapshot.pod is going to be helpful.
>
> (note that I was querying because of the use of "empty" as something
> magic, not because of the comment about xl.snapshot)
got it. i will add bool.
>
> > >
> > > >         /* Following state represents the domain state in the beginning of snapshot.
> > > >          * These state gets from libxl_domain_info.
> > >
> > > What is this used for?
> > recording the state of domain while take the snapshot. basically, i add it because
> > libvirt need track the domain state: running or paused.
> > maybe i should record the above two state directly?
>
> I think so. I think you also need to state clearly what the expected
> semantics are. e.g. does "paused" incorporate I/O quiesced?
After think about it, at least i need active and inactive state. The active
allow domain snapshot and disk-only snapshot. The inactive state only allow
the disk-only snapshot.
Meanwhile i am not suse if i need the paused state. Libvirt qemu driver record
this state. I guess it is because qemu will flush all io to disk when domain
paused. This may be useful for user to know the exactly disk status. e.g., if
user create a disk snapshot when domain paused, the disk state should be
coherent.
But i do not find the libxl do the similar flush operation for pause.
> > >
> > > So all of the stuff to do with config file parsing and specification of
> > > paths where things should be stored are issues for the toolstack and not
> > > libxl. i.e. I'm sure libvirt has its own ideas about these things
> > > already, which we should use, and I would expect things like the
> > > location for the disk snapshots to be passed to xl snapshot somehow.
> > >
> > > AFAICT the libxl API should be
> > >     libxl_domain_snapshot_save(libxl_ctx *ctx, uint32_t domid,
> > >                 const char *name, libxl_domain_snapshot *snapshot)
> > >
> > > Which should take a snapshot of domain domid using parameters from the
> > > snapshot object and update snapshot with the specifics.
> > yes, this is the issue i want to discuss. i write it in 4/5. sorry for
> > confuse.
>
> I'm afraid I don't understand what you are trying to say with this Q+A.
>
> > Q: Why there is no universe API for domain snapshot?
> > A: This is my initial target while working on snapshot. with the common API,
> >    different toolstack(xl or libvirt) could call the same api for the same
> >    operation. life would be eaiser compare to the domain create, restore and
>
> easier in what sense?
>
> >    ...
> >
> >    The reason why I could not provide common API is that it is hard to handle
> >    the ao things in api. e.g. in domain snapshot create, libvirt may wait the
> >    memory save by waiting the ao complete flag.
>
> The libxl API almost certainly does need to be async and use ao_how etc.
> That's pretty much unavoidable.
>
> I'm also confused because it appeared to me that what I was commenting
> on was a common API, but here you say you cannot provide that.
>
> >    Another reason is that I could share more functions in xl command
> >    with xl snapshot command if i do not need to provide the common api. share
> >    the code mean easy to maintenance.
>
> share with what?
e.g. share the create_domain functions in tools/libxl/xl_cmdimpl.c for domain
revert.
>
> Sorry, I'm totally confused about what you are trying to say in this
> entire section.
>
> > > The caller would
> > > have to supply any necessary paths etc in the snapshot object. It might
> > > be better to split libxl_domain_snapshot onto two, one representing the
> > > parameters for a snapshot and one representing the resulting snapshot.
> > >
> > > libxl shouldn't be doing anything with parsing json objects, storing
> > > snapshot config files or picking/mandating the output paths or anything
> > > like that.
> > yes, i agree that libxl should not picking/mandating the output paths. it
> > should be determined by application. currently, in my code, all the paths
> > is get from user config file or generated by xl not libxl.
>
> If that is the case then I'm afraid that this document does not reflect
> that reality.
i will update the document.
>
> > > Of course the application might choose to make use of the very
> > > convenient libxl functions for converting libxl structs to/from json.
> > > libvirt has an XML config format already, there is no reason not to use
> > > it in that context.
> > yes, libvirt use the XML config. i provide the load/store json config api
> > in order to migrate between xl and libvirt. what i mean is that when libvirt
> > libxl driver load/reload, it could get the snapshot created by libxl through
> > libxl_list_dom_snapshot.
> > if libvirt libxl driver could record the snapshot configuration with both
> > XML and libxl-json(through libxl_store_dom_snapshot_conf) format. then xl
> > could easily access the snapshot state created by libvirt libxl driver.
>
> I don't think "portability" of snapshots in this way is required and it
> is making your API more complex than it needs to be IMHO.
>
> It is fine and expected that xl and libvirt snapshots are manageable
> only by the toolstack which creates them.
ok.
>
> > > Likewise on snapshot restore (or revert):
> > >     libxl_domain_snapshot_restore(ctx, libxl_domain_snapshot *snapshot,
> > >                                   *r_domid)
> > > should take the snapshot and make a new domain out of it. (someone would
> > > need to specify what happens if another domain exists already in that
> > > snapshot chain etc).
> > i need to think about it. generally, if we add the
> > libxl_domain_snapshot_restore api, i need to think about how to expose the
> > ao_how to the application for more than one stage. i mean i need ao_complete
> > twice: the first is after i call disk_revert(qemu-img). the second is after
> > memory restore. is there some existence code or document show me how to do it?
>
> Why do you need to ao complete twice? The ao should complete once at the
> end of the aggregate operation.

When I wrote this version, I though there are two things time consuming,
one is disk snapshot apply (that may call 'qemu-img' to do the work), the
other is memory restore. And I thought these two should be one after another,
so to avoid waiting qemu-img completion, we might need an ao operation,
and in its ao_complete, call next stage work (memory restore).

Now thinking it again, yes, I think one ao could be OK. The two work should
be independent, then we can simply do them parallelly, so don't need a separate
ao operation for qemu-img work completion. In this way, one ao is enough.

regards

bamvor

>
> If what you want is a progress indication then perhaps the interface
> needs to take one or more libxl_asyncprogress_how objects to describe
> those stages. Domain create uses this to notify when the console is
> ready for example.
>
> > >
> > > Any management of sets of snapshots, paths to be saving them in,
> > > listing, deleting etc is down to the application.
> > >
> > >
> > > >   2.2 functions for disk snapshot operations
> > >
> > > Do these need to be exposed separately? You lbixl_domain_snapshot struct
> > > already has a mechanism for indicating whether memory should be included
> > > in the snapshot, so aren't all these functions equiavalent to the
> > > domain_snapshot ones but with memory=no?
> > the reasone why there are disk snapshot opration is there is no domain
> > snapshot operation(at least create, revert) in my current design. if we
> > could provide the domain snapshot operation, there is no need to expose
> > current disk snapshot operation.
>
> Why don't you provide a domain snapshot operation? I thought that was
> the whole point of this new interface.
>
> Ian.
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC V5 2/5] Libxl Domain Snapshot API Design
  2014-07-16  9:27         ` Bamvor Jian Zhang
@ 2014-07-16  9:50           ` Ian Campbell
  2014-07-17  8:02             ` Bamvor Jian Zhang
  0 siblings, 1 reply; 14+ messages in thread
From: Ian Campbell @ 2014-07-16  9:50 UTC (permalink / raw
  To: Bamvor Jian Zhang
  Cc: Zhiqiang Zhou, hahn, ian.jackson, Chun Yan Liu, xen-devel,
	Jim Fehlig, anthony.perard, davidkiarie4

On Wed, 2014-07-16 at 03:27 -0600, Bamvor Jian Zhang wrote:
> > Or looking at it another way the input to a disk snapshot operation
> > would be a libxl_device_disk and some options and the output would be
> > another, different, libxl_device_disk.
> Understand. There are two issues:
> 1. If i use the libxl_device_disk, the old path(before external snapshot)
> is missing. This will be inconvenient if we revert the external snapshot.

I don't follow. Surely the old path is the path of the already existing
disk which you are taking a snapshot of?

> 2. When toolstack save the domain snapshot configuration file
> (libxl-json), this configuration is longer than use libxl_disk_snapshot.
> e.g. the libxl_device_disk output:

But it fully describes everything you would need to attach it to a
guest, doesn't it? Isn't that a good thing?

> >
> > > >
> > > > >         /* Following state represents the domain state in the beginning of snapshot.
> > > > >          * These state gets from libxl_domain_info.
> > > >
> > > > What is this used for?
> > > recording the state of domain while take the snapshot. basically, i add it because
> > > libvirt need track the domain state: running or paused.
> > > maybe i should record the above two state directly?
> >
> > I think so. I think you also need to state clearly what the expected
> > semantics are. e.g. does "paused" incorporate I/O quiesced?
> After think about it, at least i need active and inactive state. The active
> allow domain snapshot and disk-only snapshot. The inactive state only allow
> the disk-only snapshot.

What do active and inactive mean? Paused? I/O Quiesced? Shutdown?

> Meanwhile i am not suse if i need the paused state. Libvirt qemu driver record
> this state. I guess it is because qemu will flush all io to disk when domain
> paused. This may be useful for user to know the exactly disk status. e.g., if
> user create a disk snapshot when domain paused, the disk state should be
> coherent.

Not necessarily, if you paused while the guest was halfway through a
journal update and do a snapshot with no memory then you would end up
recovering the journal when you next mount the filesystem. That's
different from quiescing I/O which requires a guest agent AFAIK.

> But i do not find the libxl do the similar flush operation for pause.

Correct, AFAIK.

> > >    ...
> > >
> > >    The reason why I could not provide common API is that it is hard to handle
> > >    the ao things in api. e.g. in domain snapshot create, libvirt may wait the
> > >    memory save by waiting the ao complete flag.
> >
> > The libxl API almost certainly does need to be async and use ao_how etc.
> > That's pretty much unavoidable.
> >
> > I'm also confused because it appeared to me that what I was commenting
> > on was a common API, but here you say you cannot provide that.
> >
> > >    Another reason is that I could share more functions in xl command
> > >    with xl snapshot command if i do not need to provide the common api. share
> > >    the code mean easy to maintenance.
> >
> > share with what?
> e.g. share the create_domain functions in tools/libxl/xl_cmdimpl.c for domain
> revert.

I think this is of secondary concern to having a useful and correct
libxl level API.

I'm still not sure what the "common API" you are referring to is. What
function exactly are you proposing to make common or conversely which
functions do you think it is not possible to make common?


> > > > Likewise on snapshot restore (or revert):
> > > >     libxl_domain_snapshot_restore(ctx, libxl_domain_snapshot *snapshot,
> > > >                                   *r_domid)
> > > > should take the snapshot and make a new domain out of it. (someone would
> > > > need to specify what happens if another domain exists already in that
> > > > snapshot chain etc).
> > > i need to think about it. generally, if we add the
> > > libxl_domain_snapshot_restore api, i need to think about how to expose the
> > > ao_how to the application for more than one stage. i mean i need ao_complete
> > > twice: the first is after i call disk_revert(qemu-img). the second is after
> > > memory restore. is there some existence code or document show me how to do it?
> >
> > Why do you need to ao complete twice? The ao should complete once at the
> > end of the aggregate operation.
> 
> When I wrote this version, I though there are two things time consuming,
> one is disk snapshot apply (that may call 'qemu-img' to do the work), the
> other is memory restore. And I thought these two should be one after another,
> so to avoid waiting qemu-img completion, we might need an ao operation,
> and in its ao_complete, call next stage work (memory restore).
> 
> Now thinking it again, yes, I think one ao could be OK. The two work should
> be independent, then we can simply do them parallelly, so don't need a separate
> ao operation for qemu-img work completion. In this way, one ao is enough.
> 

I think what you are saying is right, but I would suggest you take a
look at "Some libxl operations can take a long time" in libxl.h to make
sure.

In general any libxl op which can take a long time can be made async by
adding a single ao to it. That op is then treated as one long op,
regardless of how many subops it spawns internally.

But if the op you are talking about is actually multiple ops done by the
application then multiple ao's are needed.

IOW each libxl_* has a single ao. If an app is calling multiple libxl_*
functions then each needs an ao of its own.

Ian.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC V5 2/5] Libxl Domain Snapshot API Design
  2014-07-16  9:50           ` Ian Campbell
@ 2014-07-17  8:02             ` Bamvor Jian Zhang
  0 siblings, 0 replies; 14+ messages in thread
From: Bamvor Jian Zhang @ 2014-07-17  8:02 UTC (permalink / raw
  To: Ian Campbell
  Cc: Zhiqiang Zhou, hahn, ian.jackson, Chun Yan Liu, xen-devel,
	Jim Fehlig, anthony.perard, davidkiarie4

Hi, Ian

> On Wed, 2014-07-16 at 03:27 -0600, Bamvor Jian Zhang wrote:
> > > Or looking at it another way the input to a disk snapshot operation
> > > would be a libxl_device_disk and some options and the output would be
> > > another, different, libxl_device_disk.
> > Understand. There are two issues:
> > 1. If i use the libxl_device_disk, the old path(before external snapshot)
> > is missing. This will be inconvenient if we revert the external snapshot.
>
> I don't follow. Surely the old path is the path of the already existing
> disk which you are taking a snapshot of?
I mean the old path could not be retrieved from any struct. Both
libxl_device_disk and libxl_domain_snapshot has no old path info.
Then to do external snapshot apply, we may have to get the backing file
from qemu-img|qmp output. It is a little bit inconvenient.
>
> > 2. When toolstack save the domain snapshot configuration file
> > (libxl-json), this configuration is longer than use libxl_disk_snapshot.
> > e.g. the libxl_device_disk output:
>
> But it fully describes everything you would need to attach it to a
> guest, doesn't it? Isn't that a good thing?
It is good for me. I just wondering if you think it is too long.
>
> > >
> > > > >
> > > > > >         /* Following state represents the domain state in the beginning of snapshot.
> > > > > >          * These state gets from libxl_domain_info.
> > > > >
> > > > > What is this used for?
> > > > recording the state of domain while take the snapshot. basically, i add it because
> > > > libvirt need track the domain state: running or paused.
> > > > maybe i should record the above two state directly?
> > >
> > > I think so. I think you also need to state clearly what the expected
> > > semantics are. e.g. does "paused" incorporate I/O quiesced?
> > After think about it, at least i need active and inactive state. The active
> > allow domain snapshot and disk-only snapshot. The inactive state only allow
> > the disk-only snapshot.
>
> What do active and inactive mean? Paused? I/O Quiesced? Shutdown?
'Active' means running or paused. 'Inactive' means shutdown.
>
> > Meanwhile i am not suse if i need the paused state. Libvirt qemu driver record
> > this state. I guess it is because qemu will flush all io to disk when domain
> > paused. This may be useful for user to know the exactly disk status. e.g., if
> > user create a disk snapshot when domain paused, the disk state should be
> > coherent.
>
> Not necessarily, if you paused while the guest was halfway through a
> journal update and do a snapshot with no memory then you would end up
> recovering the journal when you next mount the filesystem. That's
> different from quiescing I/O which requires a guest agent AFAIK.
>
> > But i do not find the libxl do the similar flush operation for pause.
>
> Correct, AFAIK.
>
> > > >    ...
> > > >
> > > >    The reason why I could not provide common API is that it is hard to handle
> > > >    the ao things in api. e.g. in domain snapshot create, libvirt may wait the
> > > >    memory save by waiting the ao complete flag.
> > >
> > > The libxl API almost certainly does need to be async and use ao_how etc.
> > > That's pretty much unavoidable.
> > >
> > > I'm also confused because it appeared to me that what I was commenting
> > > on was a common API, but here you say you cannot provide that.
> > >
> > > >    Another reason is that I could share more functions in xl command
> > > >    with xl snapshot command if i do not need to provide the common api. share
> > > >    the code mean easy to maintenance.
> > >
> > > share with what?
> > e.g. share the create_domain functions in tools/libxl/xl_cmdimpl.c for domain
> > revert.
>
> I think this is of secondary concern to having a useful and correct
> libxl level API.
>
> I'm still not sure what the "common API" you are referring to is. What
> function exactly are you proposing to make common or conversely which
> functions do you think it is not possible to make common?
There is no domain snapshot level api in my original proposal. And after
discussing with you, I feel that i could provide the API like:
libxl_domain_snapshot_create, libxl_domain_snapshot_restore
>
>
> > > > > Likewise on snapshot restore (or revert):
> > > > >     libxl_domain_snapshot_restore(ctx, libxl_domain_snapshot *snapshot,
> > > > >                                   *r_domid)
> > > > > should take the snapshot and make a new domain out of it. (someone would
> > > > > need to specify what happens if another domain exists already in that
> > > > > snapshot chain etc).
> > > > i need to think about it. generally, if we add the
> > > > libxl_domain_snapshot_restore api, i need to think about how to expose the
> > > > ao_how to the application for more than one stage. i mean i need ao_complete
> > > > twice: the first is after i call disk_revert(qemu-img). the second is after
> > > > memory restore. is there some existence code or document show me how to do it?
> > >
> > > Why do you need to ao complete twice? The ao should complete once at the
> > > end of the aggregate operation.
> >
> > When I wrote this version, I though there are two things time consuming,
> > one is disk snapshot apply (that may call 'qemu-img' to do the work), the
> > other is memory restore. And I thought these two should be one after another,
> > so to avoid waiting qemu-img completion, we might need an ao operation,
> > and in its ao_complete, call next stage work (memory restore).
> >
> > Now thinking it again, yes, I think one ao could be OK. The two work should
> > be independent, then we can simply do them parallelly, so don't need a separate
> > ao operation for qemu-img work completion. In this way, one ao is enough.
> >
>
> I think what you are saying is right, but I would suggest you take a
> look at "Some libxl operations can take a long time" in libxl.h to make
> sure.
>
> In general any libxl op which can take a long time can be made async by
> adding a single ao to it. That op is then treated as one long op,
> regardless of how many subops it spawns internally.
>
> But if the op you are talking about is actually multiple ops done by the
> application then multiple ao's are needed.
>
> IOW each libxl_* has a single ao. If an app is calling multiple libxl_*
> functions then each needs an ao of its own.
After reading the libxl.h, libxl_domain_suspend and other codes, I think
I could do the long running event one-by-one. Take "revert domain snapshot"
as example:

First, fork process to do disk snapshot revert through qemu-img, when
child process exits enters callback e.g. helper_exited(). In this function,
start second stage work: restore memory (another child process and set callback).
When restore memory process exits, enters its callback, here do remaining work
if there is and return ao_complete. Code like this:

static void helper_exited(libxl__egc *egc, libxl__ev_child *ch,
                          pid_t pid, int status);
static void memory_retore_callback();

libxl_domain_snapshot_restore(libxl_ctx *ctx, uint32_t domid,
                              libxl_domain_snapshot *snapshot,
                              const libxl_asyncop_how *ao_how)
{
    AO_CREATE(ctx, domid, ao_how);

    //revert disk snapshot
    ...
    eid_t pid = libxl__ev_child_fork(gc, child, helper_exited);
    ...


    return AO_INPROGRESS;
}

static void helper_exited(libxl__egc *egc, libxl__ev_child *ch,
                          pid_t pid, int status)
{
    //set memory_retore_callback
    //call memory retore
}

static void memory_retore_callback()
{
    //write domain snapshot configuraiton(libxl-json format)
    ao_complete();
}

How do you think about it?

regards

bamvor
>
> Ian.

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2014-07-17  8:02 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-07-07  7:45 [RFC V5 0/5] domain snapshot document and discussion Bamvor Jian Zhang
2014-07-07  7:46 ` [RFC V5 1/5] docs: libxl Domain Snapshot HOWTO Bamvor Jian Zhang
2014-07-07  7:46 ` [RFC V5 2/5] Libxl Domain Snapshot API Design Bamvor Jian Zhang
2014-07-11 12:47   ` Ian Campbell
2014-07-11 13:19     ` Ian Jackson
2014-07-11 15:43       ` Bamvor Jian Zhang
2014-07-14 14:42     ` Bamvor Jian Zhang
2014-07-14 15:18       ` Ian Campbell
2014-07-16  9:27         ` Bamvor Jian Zhang
2014-07-16  9:50           ` Ian Campbell
2014-07-17  8:02             ` Bamvor Jian Zhang
2014-07-07  7:46 ` [RFC V5 3/5] docs: manpage for xl snapshot command Bamvor Jian Zhang
2014-07-07  7:46 ` [RFC V5 4/5] xl snapshot-xxx implementation details Bamvor Jian Zhang
2014-07-07  7:46 ` [RFC V5 5/5] docs: man page for xl.snapshot.cfg Bamvor Jian Zhang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.