|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC V5 2/5] Libxl Domain Snapshot API Design
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.
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |