On Fri, Oct 04, 2024 at 10:25:07 +0200, Nikolai Barybin wrote:
>
> On 10/3/24 15:46, Peter Krempa wrote:
> > As explained in the commit which added the new internal snapshot
> > deletion code we don't want to do any form of strict checking whether
> > the libvirt metadata is consistent with the on-disk state as we didn't
> > historically do that.
> >
> > In order to be able to spot the cases add a warning into the logs if
> > such state is encountered. While warnings are easy to miss it's the only
> > reasonable way to do that. Users will be encouraged to file an issue
> > with the information, without requiring them to enable debug logs as
> > the reproduction of that issue may include very old historical state.
> >
> > The checker is deliberately added separately so that it can be easily
> > reverted once it's no longer needed.
> >
> > Signed-off-by: Peter Krempa<pkrempa@xxxxxxxxxx>
> > ---
> > src/qemu/qemu_snapshot.c | 54 ++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 54 insertions(+)
> >
> > diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> > index 4927ca0bfc..02c876c881 100644
> > --- a/src/qemu/qemu_snapshot.c
> > +++ b/src/qemu/qemu_snapshot.c
> > @@ -3731,6 +3731,14 @@ qemuSnapshotActiveInternalDeleteGetDevices(virDomainObj *vm,
> > g_auto(GStrv) devices = g_new0(char *, vm->def->ndisks + 2);
> > size_t ndevs = 0;
> > size_t i = 0;
> > + /* variables below are used for checking of corner cases */
> > + g_autoptr(GHashTable) foundDisks = virHashNew(NULL);
> > + g_auto(virBuffer) errMissingSnap = VIR_BUFFER_INITIALIZER;
> > + g_auto(virBuffer) errUnexpectedSnap = VIR_BUFFER_INITIALIZER;
> > + g_auto(virBuffer) errExtraDisks = VIR_BUFFER_INITIALIZER;
> > + GHashTableIter htitr;
> > + void *key;
> > + bool warn = false;
> >
> > if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, VIR_ASYNC_JOB_SNAPSHOT)))
> > return NULL;
> > @@ -3759,6 +3767,7 @@ qemuSnapshotActiveInternalDeleteGetDevices(virDomainObj *vm,
> > continue;
> >
> > devices[ndevs++] = g_strdup(format_nodename);
> > + g_hash_table_insert(foundDisks, g_strdup(domdisk->dst), NULL);
> > }
> >
> > if (vm->def->os.loader &&
> > @@ -3775,6 +3784,51 @@ qemuSnapshotActiveInternalDeleteGetDevices(virDomainObj *vm,
> > }
> > }
> >
> > + /* We currently don't want this code to become stricter than what 'delvm'
> > + * did thus we'll report if the on-disk state mismatches the snapshot
> > + * definition only as a warning */
> > + for (i = 0; i < snapdef->ndisks; i++) {
> > + virDomainSnapshotDiskDef *snapdisk = snapdef->disks + i;
> > +
> > + switch (snapdisk->snapshot) {
> > + case VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL:
> > + if (g_hash_table_contains(foundDisks, snapdisk->name)) {
> > + g_hash_table_remove(foundDisks, snapdisk->name);
> > + } else {
> > + virBufferAsprintf(&errMissingSnap, "%s ", snapdisk->name);
> > + warn = true;
> > + }
> > + break;
> > +
> > + case VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL:
> > + case VIR_DOMAIN_SNAPSHOT_LOCATION_MANUAL:
> > + case VIR_DOMAIN_SNAPSHOT_LOCATION_NO:
> > + case VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT:
> > + case VIR_DOMAIN_SNAPSHOT_LOCATION_LAST:
> > + if (g_hash_table_contains(foundDisks, snapdisk->name)) {
> > + virBufferAsprintf(&errUnexpectedSnap, "%s ", snapdisk->name);
> > + warn = true;
> > + } else {
> > + g_hash_table_remove(foundDisks, snapdisk->name);
> > + }
>
> This if-else statement looks illogical or I don't understand how it works.
> Probably you meant
>
> if (!g_hash_table_contains... ?
Yes there is a problem but the condition is correct. The code is
supposted to prepare the warning if the snapshot metadata doesn't say
that a disk should have an intenal snapshot but the disk image does have
it.
What's incorrect is that it shouldn't have the " } else {" line and the
entry should be removed from the hash table when the warning is
prepared ...
>
> > + }
> > + }
> > +
> > + g_hash_table_iter_init(&htitr, foundDisks);
> > +
> > + while (g_hash_table_iter_next(&htitr, &key, NULL)) {
> > + warn = true;
> > + virBufferAsprintf(&errExtraDisks, "%s ", (const char *) key);
... so that it doesn't trigger this.
> > + }
> > +
> > + if (warn) {
> > + VIR_WARN("inconsistent internal snapshot state (deletion): VM='%s' snapshot='%s' missing='%s' unexpected='%s' extra='%s",
> > + vm->def->name, snapname,
> > + virBufferCurrentContent(&errMissingSnap),
> > + virBufferCurrentContent(&errUnexpectedSnap),
> > + virBufferCurrentContent(&errExtraDisks));
> > + }
> > +
> > return g_steal_pointer(&devices);
> > }
I'll delete the '} else {' line on my local branch.
