On Mon, Jan 10, 2011 at 06:36:57PM +0000, Daniel P. Berrange wrote:
> On Thu, Jan 06, 2011 at 02:27:14PM +0100, Jiri Denemark wrote:
> > To create a snapshot of a disk, the following API is introduced:
> >
> > int
> > virDomainDiskSnapshotCreate(virDomainPtr domain,
> > const char *disk,
> > const char *method,
> > const char *name,
> > char **modifiedDisk,
> > char **backupSource,
> > unsigned int flags);
> > @domain
> > pointer to domain object
> > @disk
> > XML definition of the disk to snapshot
> > @method
> > optional <diskSnapshot> XML element overriding the one from <disk>
> > element; if none of them is specified, default method according to disk
> > type (and pool) is used; e.g., qcow disk => qemu method; logical volume in
> > LVM pool => lvm method
>
> This I'd consider mandatory
>
> > @name
> > snapshot name
>
> What is the 'name' in this context ? In case of QCow2, AFAICT, the only identifier
> that really matters would be the filename, which is surely part of the <diskSnapshot>
> XML passed into @method. Likewise for non-QCow@ types, wouldn't @method already have
> the unique identifiers you need ?
>
> > @modifiedDisk
> > place where to store modified XML description of the disk which the domain
> > is now using; NULL is stored if domain is still using the original disk
> > (snapshot was created separately)
> > @backupSource
> > place where to store 'source' element from disk XML describing the disk
> > which can be used to take backups of 'disk' (i.e., read-only and immutable
> > snapshot); it might either be the same as provided in 'disk' or something
> > else (depending on method/implementation used); e.g., for qemu method, the
> > element describes previous disk source; lvm creates a new device for
> > snapshot and keeps writing into the original device
>
> At which point neither of these really have any good reason to
> exist.
>
> > @flags
> > OR'ed set of flags:
> > - VIR_DOMAIN_DISK_SNAPSHOT_QUIESCE_REQUIRED -- if no guest agent is
> > running/answering requests for consistent disk state, fail the API;
> > otherwise, the snapshot will be done regardless
>
> If the guest has multiple disks, then apps will need to iterate over
> each disk, invoking virDomainDiskSnapshotCreate for each one. If they
> do that, then there's a reasonable chance they'll want to do the
> quiesce once upfront, so all disks are consistent wrt each other.
> This kind of suggests we want a virDomainQuiesceStorage() and
> virDomainUnquiesceStorage() API pair.
>
> > I have a slight feeling that the API is a bit over-engineered but I'm not
> > entirely sure if it can be simplified and still provide the flexibility and
> > future-compatibility. I have this feeling especially about backupSource output
> > parameter which might possibly replaced with a simple char * (eventually
> > returned directly by the API instead of int) containing file/device path.
> > Another think which is not strictly needed is modifiedDisk. The caller can ask
> > for domain XML and look the device there if needed but that would be quite
> > complicated. Thus returning it from this API seemed useful and logical too,
> > since the API is possibly changing disk XML and it make sense to return the
> > changes.
> >
> > Deleting/merging snapshots previously created by virDomainDiskSnapshotCreate
> > is not covered by this proposal and will need to be added in the future to
> > complete disk snapshot support.
>
> As long as the new files created via this are all within scope of a storage
> pool, then they can be deleted that way. Merging snapshots is also probably
> something that'd probably want to be done via the storage pool APIs.
I'm actually wondering whether this new API is needed at all. If libvirt
is taking care of creating the actual snapshots, then all we really need
is a means to changing the source path for the existing disks. This is
then practically identical to the task of changing CDROM media, which we
can achieve with the virDomainUpdateDevice() API.
So, given a guest which has a disk 'CurrentFile.img', which we want
to snapshot, a complete operation could be
1. virDomainQuiesceStorage($dom)
2. $volxml =
"<volume>
<name>NewFile.img</name>
<target>
<path>/var/lib/libvirt/images/NewFile.img</path>
<format type='qcow2'/>
</target>
<backingStore>
<path>/var/lib/libvirt/images/CurrentFile.img</path>
<format type='qcow2'/>
</backingStore>
</volume>"
virStorageVolCreate($volxml);
3. $diskxml =
"<disk type='file' device='disk'>
<driver name='qemu' type='qcow2'/>
<source dev='/var/lib/libvirt/images/NewFile.img'/>
<target dev='hda' bus='ide'/>
</disk>"
virDomainUpdateDevice($dom, $diskxml);
4. virDomainUnquiesceStorage($dom);
Steps 2 + 3 can be repeated multiple times, once for each
disk to be snapshotted.
The virDomainDiskSnapshotCreate() API suggestion seems to be
just a syntactic sugar for the combination of the existing
virStorageVolCreate() and virDomainUpdateDevice(), but with
yet another XML format, and a requirement for the hypervisor
drivers to now call directly into our storage drivers.
The only scenario this would not work with, is if you wanted
to do a qcow2 internal snapshot. QEMU seem to generally
frown on continued use of internal snapshots, so I'm not sure
that is a good enough reason to add a new API.
Regards,
Daniel
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list