On Wed, Apr 22, 2020 at 03:49:57PM +0200, Peter Krempa wrote:
> On Thu, Apr 16, 2020 at 17:07:35 +0200, Pavel Mores wrote:
> > On Fri, Apr 03, 2020 at 05:56:59PM +0200, Pavel Mores wrote:
> > > On Tue, Mar 31, 2020 at 12:06:15PM +0200, Peter Krempa wrote:
> > > > On Tue, Mar 31, 2020 at 11:18:29 +0200, Pavel Mores wrote:
> > > > > + }
> > > > > +
> > > > > + if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN)
> > > > > + topPath = disk->src->path;
> > > > > + else
> > > > > + topPath = snapdisk->src->path;
> > > >
> > > > You must not use paths for doing this lookup. Paths at best work for
> > > > local files only and this would make the code not future proof.
> > > >
> > > > Also you want to verify that:
> > > > - images you want to merge are actually in the backing chain
> > > > - the backing chain looks as snapshots describe it (e.g you unplug vda
> > > > and plug a different chain back)
> > >
> > > Let me check with you how exactly to perform this verification.
> > >
> > > To retrieve the names of the disks included in a snapshot, I can use its
> > > virDomainSnapshotDef which contains an array of disks
> > > (virDomainSnapshotDiskDef's) whose 'name' member can be used to identify disks.
> > >
> > > To get the state of the chain at moment the snapshot was created, I can use the
> > > 'src' member and walk its 'backingStore' list to examine the whole chain.
> > >
> > > To get the currently running configuration, I assume I can use the names of the
> > > relevant disks (obtained from the snapshot as mentioned above) to get a
> > > virDomainDiskDefPtr for each of them, whose 'src' member points to a
> > > virStorageSource that represents the topmost image in the disk's chain. From
> > > there, I can walk the 'backingStore' list to get the other images in the chain
> > > all the way to the base image.
> > >
> > > The verification succeeds if the disk names and their chains in the snapshot
> > > and the running config correspond. Is the above correct?
> > >
> > > If so, I'm still not certain how to compare two virStorageSource's. How is
> > > identity of two different virStorageSource instances is established?
> >
> > After having a closer look into this I'm unfortunately even less clear as to
> > how to perform the checks mentioned in the review.
>
> Well unfortunately developing this is the main part why deleting
> snapshots is complicated.
>
> > In addition to the problem of establishing virStorageSource identity, a similar
> > problem seems to come up with disks. They often seem to be identified and
> > referred to by the target name, however what happens if a snapshot is taken for
> > 'vda', then the disk's target is changed to 'vdb' and now the snapshot is to be
>
> In such case I'd consier it as being different. Similarly as we can't
> really guarantee that the image described by a virStorageSource is the
> same as it was when taking the snapshot. As long as the file is named
> the same you can consider it identical IMO.
>
> > deleted? Is there a way to find out that the disk is still there, only under a
> > different name?
>
> No and it doesn't seem to be worth doing that.
>
> >
> > And as far as comparing chains goes - I know can retrieve the chain for a
> > running disk and I can retrieve what the chain looked like at the point a
> > snapshot was made. However, how do I establish they are equivalent? How can I
>
> What I meant is that you need to establish that the images you are going
> to merge and the images you are going to merge into are the same
> described by the snapshots and at the same time present in the current
> backing chain of the disk. If they are not in the snapshot metadata
> you'd potentially merge something unwanted, but this is covered by the
> previous paragraphs.
>
> You need to also verify that they are currently opened by qemu as you
> can't do blockjobs on images which are not part of the chain.
>
> > tell that snapshots in both chains compare identical in the first place? Is
> > the snapshot name stable and persistent enough to be useful for this? Is it
> > enough for chains to be equivalent that the chain at the point of snapshot
> > creation be a superset of the currently running chain? Probably yes, provided
> > snapshots can't be inserted in the middle of a chain - but could that ever
> > happen?
>
> The main problem is that the configuration may change from the time when
> the snapshot was taken. Either the XML or the on disk setup. In case a
> user issues a blockjob manually which merges parts of the chain you
> won't be able to delete the snapshot with data reorganisation via the
> API. Similarly to when the VM config changes to point to other images
> and or remove or add disks. The snapshot code should be able to at least
> properly reject the operation if it's unclear whether we can do the
> right thing.
>
> Unfortunately all the above need to be considered and thought out before
> because there isn't really any prior art in libvirt that could be
> copied. There are helpers to compare two virStorageSource structs, but
> that's far from all of the required logic.
>
> While the initial implementation can be limited in what it can do, the
> complement of what it can't do must be properly rejected to prevent any
> ambiguity.
Considering all of the above, how about the following algorithm for the initial
version. If nobody has any comments/objections I'd like to start implementing
this tomorrow.
- if the snapshot to be deleted or its parent has more than one child, bail
out. For the moment only linear snapshot structures are handled.
- for each disk that the snapshot to be deleted touched
- get its target name
- get disk definition (virDomainDiskDef) for that name and running VM by
calling qemuDomainDiskByName()
- if that fails the disk is no longer attached to the VM (or at least not
with the same target name) so bail out as the snapshot is not safe
to delete
- examine the 'type' of 'src' member of the disk definition - if it's not
VIR_STORAGE_TYPE_FILE bail out (for now) as I don't know how to establish
equivalence of virStorageSources of types other than file (I can do that
for files by comparing their paths). This assumes that all
virStorageSources in a chain will have the same type (correct me if I'm
wrong here). If it *can* happen that e.g. a storage source of type 'file'
has a backing store of type 'block' then this check is obviously not valid.
- walk the backing chain of the running disk and find the storage source of
the snapshot (virDomainSnapshotDiskDef::src) there, using the equivalence
check mentioned above (e.g. paths for files). Bail out on failure as the
disk in the currently running VM is probably a different one than the disk
that was attached to the VM under the target name at the time of snapshot
creation.
- now an additional check can be performed to ensure the currently running
disk is the same as it was when our snapshot was created. Assuming that
snapshots cannot be inserted in the middle of a chain, the backing chain
of the disk at the point of snapshot creation should be an ordered superset
of the part of the running disk's backing chain between the base image
and the our snapshot. E.g. if we create snapshots 1 2 3 4 5 6 and we want
to delete 6 at some later point, the running disk's backing chain might be
2 3 4 6 8 9 10. This should be fine since 6 is still there and its backing
chain is a subset of what it was when 6 was created (meaning that 1 and 5
have probably been deleted already which is fine). We don't care that 7
10 were created since 6 was and 7 was already deleted.
- now we can delete the snapshot by blockcommitting it into its parent.
I believe this incorporates all of the remarks above while only handling linear
snapshot structures and file-based storage sources for now (but extending this
with handling of storage sources types shouldn't be very hard once it's clear
how to tell if they are the same).
Thanks,
pvl
