2:38 a.m.
Currently taking an internal inactive snapshot with a disk excluded from
snapshot using snapshot="no" will fail with error "Disk device '%s'
does
not support snapshotting".
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/qemu/qemu_domain.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 469942d201..6a70b72760 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6860,11 +6860,12 @@ qemuDomainSnapshotWriteMetadata(virDomainObj *vm,
static int
qemuDomainSnapshotForEachQcow2Raw(virQEMUDriver *driver,
virDomainDef *def,
- const char *name,
+ virDomainMomentObj *snap,
const char *op,
bool try_all,
int ndisks)
{
+ virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap);
const char *qemuimgbin;
size_t i;
bool skipped = false;
@@ -6877,11 +6878,12 @@ qemuDomainSnapshotForEachQcow2Raw(virQEMUDriver *driver,
for (i = 0; i < ndisks; i++) {
g_autoptr(virCommand) cmd = virCommandNewArgList(qemuimgbin,
"snapshot",
- op, name, NULL);
+ op, snap->def->name,
NULL);
int format = virDomainDiskGetFormat(def->disks[i]);
/* FIXME: we also need to handle LVM here */
- if (def->disks[i]->device != VIR_DOMAIN_DISK_DEVICE_DISK)
+ if (def->disks[i]->device != VIR_DOMAIN_DISK_DEVICE_DISK ||
+ snapdef->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE)
continue;
if (!virStorageSourceIsLocalStorage(def->disks[i]->src)) {
@@ -6903,7 +6905,7 @@ qemuDomainSnapshotForEachQcow2Raw(virQEMUDriver *driver,
} else if (STREQ(op, "-c") && i) {
/* We must roll back partial creation by deleting
* all earlier snapshots. */
- qemuDomainSnapshotForEachQcow2Raw(driver, def, name,
+ qemuDomainSnapshotForEachQcow2Raw(driver, def, snap,
"-d", false, i);
}
virReportError(VIR_ERR_OPERATION_INVALID,
@@ -6923,7 +6925,7 @@ qemuDomainSnapshotForEachQcow2Raw(virQEMUDriver *driver,
} else if (STREQ(op, "-c") && i) {
/* We must roll back partial creation by deleting
* all earlier snapshots. */
- qemuDomainSnapshotForEachQcow2Raw(driver, def, name,
+ qemuDomainSnapshotForEachQcow2Raw(driver, def, snap,
"-d", false, i);
}
return -1;
@@ -6942,7 +6944,7 @@ qemuDomainSnapshotForEachQcow2(virQEMUDriver *driver,
const char *op,
bool try_all)
{
- return qemuDomainSnapshotForEachQcow2Raw(driver, def, snap->def->name,
+ return qemuDomainSnapshotForEachQcow2Raw(driver, def, snap,
op, try_all, def->ndisks);
}
--
2.31.1
2:58 a.m.
On Wed, Feb 16, 2022 at 11:38:01 +0300, Nikolay Shirokovskiy wrote:
Currently taking an internal inactive snapshot with a disk excluded
from
snapshot using snapshot="no" will fail with error "Disk device
'%s' does
not support snapshotting".
The idea for internal snapshots since we wanted them to behave the same
as they do with 'savevm' is that you can't actually exclude a disk from
the snapshot.
Could you elaborate how you expect this to co-operate with live internal
snapshots?
6:33 a.m.
ср, 16 февр. 2022 г. в 11:58, Peter Krempa <pkrempa(a)redhat.com>:
On Wed, Feb 16, 2022 at 11:38:01 +0300, Nikolay Shirokovskiy wrote:
> Currently taking an internal inactive snapshot with a disk excluded from
> snapshot using snapshot="no" will fail with error "Disk device
'%s' does
> not support snapshotting".
The idea for internal snapshots since we wanted them to behave the same
as they do with 'savevm' is that you can't actually exclude a disk from
the snapshot.
Could you elaborate how you expect this to co-operate with live internal
snapshots?
I have an issue with readonly usb raw disk. Making internal snapshot of an
active domain
is possible in this case if I exclude the disk from snapshot explicitly or
implicitly.
However in case of inactive domain snapshot fails with the above error. So
I guess
we should allow this case.
So maybe we should check for readonly property of the disk instead? On
snapshot creation
there will be no difference as check in virDomainSnapshotAlignDisks will
fail for non
readonly disks. Yet it makes more sense to me to check snapdef disk
because qemuDomainSnapshotForEachQcow2Raw
is only to call qemu-img and the decision about the disk set is not its
concern. Also
the function is used on snapshot reverting/deletion and we want to base
on snapdef again and not the current state of readonly attribute on disk.
However it is not clear to me why there should be parity between active and
inactive
snapshot in terms of possible disk sets. We have constraint on set in case
of active
snapshot just due to API limitations. What are the reasons to mimic this
set for inactive
snapshot?
Nikolay
6:55 a.m.
On Wed, Feb 16, 2022 at 15:33:57 +0300, Nikolay Shirokovskiy wrote:
ср, 16 февр. 2022 г. в 11:58, Peter Krempa
<pkrempa(a)redhat.com>:
> On Wed, Feb 16, 2022 at 11:38:01 +0300, Nikolay Shirokovskiy wrote:
> > Currently taking an internal inactive snapshot with a disk excluded from
> > snapshot using snapshot="no" will fail with error "Disk device
'%s' does
> > not support snapshotting".
>
> The idea for internal snapshots since we wanted them to behave the same
> as they do with 'savevm' is that you can't actually exclude a disk from
> the snapshot.
>
> Could you elaborate how you expect this to co-operate with live internal
> snapshots?
>
>
I have an issue with readonly usb raw disk. Making internal snapshot of an
active domain
is possible in this case if I exclude the disk from snapshot explicitly or
implicitly.
Well, yeah, you can set it as explicitly excluded since it would be
excluded implicitly. The other way should not be possible.
However in case of inactive domain snapshot fails with the above
error. So
I guess
we should allow this case.
So maybe we should check for readonly property of the disk instead? On
snapshot creation
there will be no difference as check in virDomainSnapshotAlignDisks will
fail for non
readonly disks.
[...]
Yet it makes more sense to me to check snapdef disk
because qemuDomainSnapshotForEachQcow2Raw
is only to call qemu-img and the decision about the disk set is not its
concern. Also
the function is used on snapshot reverting/deletion and we want to base
on snapdef again and not the current state of readonly attribute on disk.
Okay, so the problem isn't actually with the code in this patch, because
virDomainSnapshotAlignDisks already ensures that the snapshot wouldn't
be done under the same conditions as with the live snapshots.
It's the commit message that is misleading because you didn't at all
mention that the disk was readonly and thus wouldn not take part in the
snapshot anyway.
Thus the code you are changing is correctly addressing the bug, where
the offline snapshotting function is attempting to take the snapshot of
the readonly disk.
I suggest the following commit message:
'qemuDomainSnapshotForEachQcow2Raw' doesn't properly handle the
'VIR_DOMAIN_SNAPSHOT_LOCATION_NONE' setting and thus doesn't skip disks
which were excluded from the snapshot due to being read-only.
With that commit message you can use:
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
However it is not clear to me why there should be parity between
active and
inactive
snapshot in terms of possible disk sets. We have constraint on set in case
of active
snapshot just due to API limitations. What are the reasons to mimic this
set for inactive
snapshot?
It minimizes the state matrix and possible corner cases especially since
the old qemu impl is not flexible in handling partial snapshots.
7:24 a.m.
ср, 16 февр. 2022 г. в 15:55, Peter Krempa <pkrempa(a)redhat.com>:
On Wed, Feb 16, 2022 at 15:33:57 +0300, Nikolay Shirokovskiy wrote:
> ср, 16 февр. 2022 г. в 11:58, Peter Krempa <pkrempa(a)redhat.com>:
>
> > On Wed, Feb 16, 2022 at 11:38:01 +0300, Nikolay Shirokovskiy wrote:
> > > Currently taking an internal inactive snapshot with a disk excluded
from
> > > snapshot using snapshot="no" will fail with error "Disk
device '%s'
does
> > > not support snapshotting".
> >
> > The idea for internal snapshots since we wanted them to behave the same
> > as they do with 'savevm' is that you can't actually exclude a disk
from
> > the snapshot.
> >
> > Could you elaborate how you expect this to co-operate with live
internal
> > snapshots?
> >
> >
> I have an issue with readonly usb raw disk. Making internal snapshot of
an
> active domain
> is possible in this case if I exclude the disk from snapshot explicitly
or
> implicitly.
Well, yeah, you can set it as explicitly excluded since it would be
excluded implicitly. The other way should not be possible.
> However in case of inactive domain snapshot fails with the above error.
So
> I guess
> we should allow this case.
>
> So maybe we should check for readonly property of the disk instead? On
> snapshot creation
> there will be no difference as check in virDomainSnapshotAlignDisks will
> fail for non
> readonly disks.
[...]
> Yet it makes more sense to me to check snapdef disk
> because qemuDomainSnapshotForEachQcow2Raw
> is only to call qemu-img and the decision about the disk set is not its
> concern. Also
> the function is used on snapshot reverting/deletion and we want to base
> on snapdef again and not the current state of readonly attribute on disk.
Okay, so the problem isn't actually with the code in this patch, because
virDomainSnapshotAlignDisks already ensures that the snapshot wouldn't
be done under the same conditions as with the live snapshots.
It's the commit message that is misleading because you didn't at all
mention that the disk was readonly and thus wouldn not take part in the
snapshot anyway.
Thus the code you are changing is correctly addressing the bug, where
the offline snapshotting function is attempting to take the snapshot of
the readonly disk.
I suggest the following commit message:
'qemuDomainSnapshotForEachQcow2Raw' doesn't properly handle the
'VIR_DOMAIN_SNAPSHOT_LOCATION_NONE' setting and thus doesn't skip disks
which were excluded from the snapshot due to being read-only.
With that commit message you can use:
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
Actually I was not aware of all the details at that moment and just
occasionally
the patch happened to be correct. So thanx for the review!
Nikolay
1010
days inactive
1010
days old
4 comments
2 participants
participants (2)
-
Nikolay Shirokovskiy -
Peter Krempa