Re: [Xen-devel] [RFC V8 2/3] libxl domain snapshot API design

["Chun Yan Liu" <cyliu@xxxxxxxx>]

>>> On 12/9/2014 at 07:11 PM, in message <1418123518.14361.20.camel@xxxxxxxxxx>,
Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote: 
> On Mon, 2014-12-08 at 22:04 -0700, Chun Yan Liu wrote: 
> > Partly. At least for domain disk snapshot create/delete, I prefer using 
> > qmp commands instead of calling qemu-img one by one. Using qmp 
> > commands, libvirt will need libxl's help. Of course, if libxl doesn't 
> > supply that, libvirt can call qemu-img to each disk one by one, 
> > not preferred but can do. 
>  
> You can't use qmp unless the domain is active, for an inactive domain 
> there is no qemu to talk to, so you have to use qemu-img anyway in that 
> case. Does libvirt not have existing code to do all this sort of thing? 
> (I thought so, but it turns out I may be wrong, see below). 

No. Even inlibvirt/qemu_driver (for kvm), it does the work itself through
qemu monitor commands.

>  
> And for an active domain I expect that *must* use qmp, since it seems 
> unlikely that you can go around changing things under the feet of an 
> active process (maybe I'm wrong?).

For active domain, I tried 'qemu-img snapshot' after pausing a domain,
the commands succeeded. But I also think using qmp commands is better
since qemu supplies transaction qmp, it avoids the trouble to roll back
status when using qemu-img to do disk snapshot one by one but only part of
disks succeed.

So, if disk snapshot functions can be provided to both libvirt and xl usage,
it's very helpful to libvirt side. In this way, I may prefer to put disk 
snapshot
functions to libxlu.

>  
> > > > Following the constraint that it's better NOT to supply disk snapshot  
> > > > functions in libxl, then we let xl and libvirt do that by themselve  
> > > > separately, that's OK.  
> > > >   
> > > > Then I think NO new API needs to be exported in libxl, since:  
> > > > * saving/restoring memory, there are already APIs.  
> > >   
> > > The principle is that if existing API doesn't work good enough for you  
> > > we will consider adding a new one.  
> > >   
> > > We probably need a new API. If you want to do a live snapshot, we would  
> > > need to notify xl that we are in the middle of pausing and resuming a  
> > > domain.   
> >  
> > This is where we discussed a lot. Do we really need 
> > libxl_domain_snapshot_create API? or does xl can do the work? 
> >  
> > Even for live snapshot, after calling libxl_domain_suspend with LIVE flags, 
> > memory is saved and domain is paused. xl then can call disk snapshot 
> > functions to finish disk snapshots, after all of that, call  
> libxl_domain_unpause 
> > to unpause the domain. So I don't think xl has any trouble to do that. 
> > In case there is some misunderstanding, please point out. 
>  
> My mistake, I incorrectly remembered that libxl_domain_suspend would 
> destroy (for save or migate) or resume (for checkpoint) the guest before 
> returning. Having refreshed my memory I see that you are correct: it 
> returns with the domain paused and it is up to the toolstack to resume 
> or destroy it as it wishes. Sorry for the confusion. 
>  
> Given that it does seem like the toolstack could indeed take the 
> disksnapshots itself without an additional API. 
>  
> > > However the current architecture for libvirt to use libxl is like  
> > >   
> > >   libvirt  
> > >   libxl  
> > >   [other lower level stuffs]  
> > >   
> > > So implementing snapshot management in xl cannot work for you either.  
> > > It's not part of the current architecture. 
>  
> This is correct, xl should not be involved in a libvirt control stack, 
> it is orthogonal. 
>  
> > You are right. I understand you are trying to suggest a way to ease the  
> job. 
> > Here just to make clear this way is really better and finally acceptable?  
> :-) 
> > Just IMO, I think xl snapshot-list is wanted, that means managing snapshots 
> > in xl is needed. 
>  
> The xl idiom is that you do this sort of operation with existing CLI 
> commands e.g. ls /var/lib/vm-images/*.qcow2, lvs, qemu-img etc. 
>  
> Adding snapshot-list to xl would be a whole load of work to create a 
> bunch of infrastructure which you do not need to do. 
>  
> My understanding was that your primary aim here was to enable snapshots 
> via libvirt and that xl support was just a nice to have. Is that right? 

We hope both :-)

Libvirt side already has some codes as I know and hopes to integrate with
libxl to enable snapshots. Of course the two toolstacks can have some
differences in commands, that's OK.

Libvirt side, to use unified virsh commands, it will supply
snapshot-create/delete/revert/list.

Xl side, if it's better to supply snapshot-create/revert, we can implement
like that. Then it IS much easier since no need to manage snapshots in xl,
then no save/retrieve json file things and no snapshot chain things. Do
we want/decide to follow this?

>  
> > > Not that I'm against the idea of managing domain snapshot in xl, I'm  
> > > trying to reduce workload here.  
> > >   
> > > > > The   
> > > > > downside is that now xl and libvirt are disconnected, but I think 
> > > > > it's   
> > > > > fine.  
> > > >   
> > > > Two things here:  
> > > > 1. connect xl and libvirt, then will need to manage snapshot info in  
> libxl   
> > > (or  
> > > >     libxlu) That's not preferred since the initial design. This is not  
> the   
> > > point  
> > > >     we discuss here.  
> > > > 2. for xl only, list snapshots and delete snapshots, also need to 
> > > > manage  
> > > >     snapshot info (in xl)  
> > > >   
> > > > Considering manage snapshot info in xl, only question is about idl and  
> > > > gentypes.py, expected structure is as following and expected to be 
> > > > saved  
> > > > into json file, but it contains xl namespace and libxl namespace 
> > > > things,  
> > > > gentypes.py will have problem. Better ideas?  
> > > >   
> > > > typedef struct xl_domain_snapshot {  
> > > >     char * name;  
> > > >     char * description;  
> > > >     uint64_t creation_time;  
> > > >     char * memory_path;  
> > > >     int num_disks;  
> > > >     libxl_disk_snapshot *disks;  
> > > >     char *parent;  
> > > >     bool *current;  
> > > > } xl_domain_snapshot;  
> > > >   
> > >   
> > > The libxl_disk_snapshot suggests that you still want storage management  
> > > inside libxl, which should probably be in libxlu?  
> >  
> > Yeah. I may put it in libxlu. 
>  
> This depends on who the consumers of this datastructure are: 
>  
> If xl only -> put it in xl itself. 
> If libvirt+xl -> put it in libxlu. 
>  
> My understanding was that libvirt already has data structures for 
> dealing with snapshots, but this was based entirely on the commands 
> listed by: 
>         virsh help | grep -E pool-\|snapshot- 
> which seemed to me to be pretty feature rich and suggested that libvirt 
> has a great deal of support for storage and snapshot management already. 
>  

Oh, I didn't say clearly. Here we  mean libxl_disk_snapshot should be
libxlu_disk_snapshot, that is, disk snapshot functions better in libxlu.
Not mean xl_domain_snapshot. If managing snapshots, it will be in xl.
Libvirt has its own data structures about managing domain snapshots.

Thanks,
Chunyan

> If libvirt already has generic infrastructure for managing snapshots 
> this then IMHO you should use it, not reimplement it on the Xen side 
> (whether in libxl, libxlu or xl), the additions to Xen should be limited 
> to providing the underlying functionality which libvirt's generic code 
> requires from the backend. 
>  
> However, Wei has suggested to me that perhaps libvirt's snapshotting 
> capabilities are not as generic internally as I might have imaged and 
> that it is up to each backend driver to reinvent things, is that true? 
>  
> If Wei's suggestion is correct then it may turn out that it is useful to 
> put some of the new generic code which you would need to write into 
> libxlu. 
>  
> Ian. 
>  
>  
>  


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel