RE: [PATCH v1 0/4] Introduce active disk internal snapshot support
-----Original Message-----
From: Peter Krempa <pkrempa(a)redhat.com>
Sent: Friday, 13 January 2023 17:21
To: Or Ozeri <ORO(a)il.ibm.com>
Cc: libvir-list(a)redhat.com
Subject: [EXTERNAL] Re: [PATCH v1 0/4] Introduce active disk internal
snapshot support
Could you elaborate how this will be useful? Generally disk only snapshots
are of very limited use as when reverting you get to a state equating of a
power loss of a real box.
This means potentially corrupted files, state and other drawbacks.
This will be useful for backup of the disks for disaster recovery.
i.e. once the internal snapshots are taken, they will be backed up (using diffs of course)
to a different geographic location.
With external disk only snapshots this was at least somewhat useful
as it
made the original image read-only and thus accessible with a different
process.
With internal snapshots you still can't even read that image as it's still
locked
in write mode, so the utility value is even less.
The use-case we're considering is using RBD disks with raw format.
These internal RBD snapshots are read-accessible even while the original disk is still
being write-accessed.
> This implementation for the qemu driver works even when there
are
> other disks which ask for external snapshot.
> Thus we remove the restriction disallowing mixing of internal and
> external disk snapshots for active VMs.
> Note that mixing is still disallowed for non-active VMs, as it require
> a bit more work in a different area of the code.
Note that a few days ago Pavel pushed his series adding support for deletion
of external snapshots. That code heavily depends on the fact that mixing
internal/external snapshots was forbidden.
Thus the patch in this series will not be acceptable until you fix the code that
Pavel added.
My code touches only snapshot creation.
I see that for deletion, Pavel added a check which yields:
"deletion of external and internal children disk snapshots not supported "
So I don't think any change is required, unless you want to support a new feature of
deletion of mixed snapshots.
I don't think we should put a lot of effort into coding a full-matrix of support
between any two features
(i.e. snapshot deletion, and active internal snapshots) if there's no client use-case
justifying this work.
Also note that Pavel is working on reversion of external snapshots so
that
work might also colide.
Like snapshot deletion, this sounds like something that can work alongside by simply
adding a check which
will disallow reverting in a mixed case.
This bit is potentially dangerous as based on your code you don't seem to be
checking that the name is unique across snapshots, which can cause potential
problems when names between snapshots collide.
I actually think you can specify a name for an internal snapshot today, if you set
<name> under <domainsnapshot>, so my modification only extends this to allow
per-disk name, instead of a single name for all disks.
So in theory I think snapshot name can collide even in today's code.
Second, IMHO this patch already handles collision to the best extent.
If there's a name collision on one of the disks, the blockdev-snapshot-internal-sync
for that disk will fail,
and the encapsulating qmp_transaction will revert all its actions.
This is also something that I feel requires elaboration of how it
will be useful.
Just like external snapshots allowing you to specify a custom path for the new file, some
clients wish to use their own
naming-convention for internal snapshots (instead of using the ctime names generated by
libvirt)