[PATCH v1 0/4] Introduce active disk internal snapshot support

[Peter Krempa]

On Sun, Jan 15, 2023 at 11:41:01 +0000, Or Ozeri wrote:
> 
> > -----Original Message-----
> > From: Peter Krempa <pkrempa at redhat.com>
> > Sent: Friday, 13 January 2023 17:21
> > To: Or Ozeri <ORO at il.ibm.com>
> > Cc: libvir-list at 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.

Please make sure to mention that you specifically care about RBD upfront
the next time.

Okay, that is fair enough then. But allow it just for RBD.

As noted below we don't want this mixing for qcow2 internal snapshots as
it will overcomplicate deletion and reversion code.

> > > 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.

We do not want this mixing for qcow2 based snapshots. It will be too
complicated. But since you are dealing with RBD internal snapshots you
can allow it sust for those.

Obviously we will not be able to delete those.


> > 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.

The name of the internal snapshot is equal to the <name> but it's the
same for all of them.

But the check needs to be done inside libvirt. There are configurations
in which the QCOW2 internal snapshot is no longer in the topmost image
in which case blockdev-snapshot-internal-sync will not fail.

We simply don't want to defer this check to qemu as it creates a bunch
of unnecessary corner cases.

Again you can allow specific snapshot names just for internal RBD
snapshots.

> > 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)