* [pve-devel] [PATCH v2 container guest-common 0/2] snapshot parent checks for containers
@ 2021-10-13 12:31 Oguz Bektas
2021-10-13 12:31 ` [pve-devel] [PATCH v2 container 1/2] api: clone_vm: don't include snapshot properties Oguz Bektas
2021-10-13 12:31 ` [pve-devel] [PATCH v2 guest-common 2/2] snapshots: abort if new snapshot name is already parent to existing one Oguz Bektas
0 siblings, 2 replies; 5+ messages in thread
From: Oguz Bektas @ 2021-10-13 12:31 UTC (permalink / raw)
To: pve-devel
v1->v2:
* remove other snapshot-related properties during clone
* add check when creating new snapshots and abort if new name already a parent for existing snapshot
pve-container:
Oguz Bektas (1):
api: clone_vm: don't include snapshot properties
src/PVE/API2/LXC.pm | 5 +++++
1 file changed, 5 insertions(+)
pve-guest-common:
Oguz Bektas (1):
snapshots: abort if new snapshot name is already parent to existing
one
src/PVE/AbstractConfig.pm | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 5+ messages in thread
* [pve-devel] [PATCH v2 container 1/2] api: clone_vm: don't include snapshot properties
2021-10-13 12:31 [pve-devel] [PATCH v2 container guest-common 0/2] snapshot parent checks for containers Oguz Bektas
@ 2021-10-13 12:31 ` Oguz Bektas
2021-10-14 10:01 ` [pve-devel] applied: " Thomas Lamprecht
2021-10-13 12:31 ` [pve-devel] [PATCH v2 guest-common 2/2] snapshots: abort if new snapshot name is already parent to existing one Oguz Bektas
1 sibling, 1 reply; 5+ messages in thread
From: Oguz Bektas @ 2021-10-13 12:31 UTC (permalink / raw)
To: pve-devel
apparently this caused a weird[0] bug... when a container with a snapshot was
cloned, it would take 'parent: foo' from the original container. if you
add a new snapshot 'bar', and then another one 'foo', this causes the
snapshots to become parents of each other (thus not parsed correctly in
the tree view of GUI nor with 'pct listsnapshot CTID')
we also drop these properties for VMs, so it makes sense to do the same
here as well.
[0]: https://forum.proxmox.com/threads/snapshots-of-one-lxc-disappeared.97711/
Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
src/PVE/API2/LXC.pm | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 1f2f1f0..05cfbad 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -1512,7 +1512,12 @@ __PACKAGE__->register_method({
# Replace the 'disk' lock with a 'create' lock.
$newconf->{lock} = 'create';
+ # delete all snapshot related config options
delete $newconf->{snapshots};
+ delete $newconf->{parent};
+ delete $newconf->{snaptime};
+ delete $newconf->{snapstate};
+
delete $newconf->{pending};
delete $newconf->{template};
if ($param->{hostname}) {
--
2.30.2
^ permalink raw reply [flat|nested] 5+ messages in thread
* [pve-devel] [PATCH v2 guest-common 2/2] snapshots: abort if new snapshot name is already parent to existing one
2021-10-13 12:31 [pve-devel] [PATCH v2 container guest-common 0/2] snapshot parent checks for containers Oguz Bektas
2021-10-13 12:31 ` [pve-devel] [PATCH v2 container 1/2] api: clone_vm: don't include snapshot properties Oguz Bektas
@ 2021-10-13 12:31 ` Oguz Bektas
2021-10-14 7:34 ` Fabian Ebner
1 sibling, 1 reply; 5+ messages in thread
From: Oguz Bektas @ 2021-10-13 12:31 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
src/PVE/AbstractConfig.pm | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/src/PVE/AbstractConfig.pm b/src/PVE/AbstractConfig.pm
index 3348d8a..6849664 100644
--- a/src/PVE/AbstractConfig.pm
+++ b/src/PVE/AbstractConfig.pm
@@ -721,14 +721,22 @@ sub __snapshot_prepare {
$conf->{lock} = 'snapshot';
+ my $snapshots = $conf->{snapshots};
+
die "snapshot name '$snapname' already used\n"
- if defined($conf->{snapshots}->{$snapname});
+ if defined($snapshots->{$snapname});
my $storecfg = PVE::Storage::config();
die "snapshot feature is not available\n"
if !$class->has_feature('snapshot', $conf, $storecfg, undef, undef, $snapname eq 'vzdump');
- $snap = $conf->{snapshots}->{$snapname} = {};
+ foreach my $existing_snapshot (keys %$snapshots) {
+ my $parent_name = $snapshots->{$existing_snapshot}->{parent} // '';
+ die "snapshot '$snapname' cannot be used, $snapname already a parent for $existing_snapshot\n"
+ if $snapname eq $parent_name;
+ }
+
+ $snap = $snapshots->{$snapname} = {};
if ($save_vmstate && $class->__snapshot_check_running($vmid)) {
$class->__snapshot_save_vmstate($vmid, $conf, $snapname, $storecfg);
--
2.30.2
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pve-devel] [PATCH v2 guest-common 2/2] snapshots: abort if new snapshot name is already parent to existing one
2021-10-13 12:31 ` [pve-devel] [PATCH v2 guest-common 2/2] snapshots: abort if new snapshot name is already parent to existing one Oguz Bektas
@ 2021-10-14 7:34 ` Fabian Ebner
0 siblings, 0 replies; 5+ messages in thread
From: Fabian Ebner @ 2021-10-14 7:34 UTC (permalink / raw)
To: pve-devel, Oguz Bektas
Am 13.10.21 um 14:31 schrieb Oguz Bektas:
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
> src/PVE/AbstractConfig.pm | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/src/PVE/AbstractConfig.pm b/src/PVE/AbstractConfig.pm
> index 3348d8a..6849664 100644
> --- a/src/PVE/AbstractConfig.pm
> +++ b/src/PVE/AbstractConfig.pm
> @@ -721,14 +721,22 @@ sub __snapshot_prepare {
>
> $conf->{lock} = 'snapshot';
>
> + my $snapshots = $conf->{snapshots};
> +
> die "snapshot name '$snapname' already used\n"
> - if defined($conf->{snapshots}->{$snapname});
> + if defined($snapshots->{$snapname});
>
> my $storecfg = PVE::Storage::config();
> die "snapshot feature is not available\n"
> if !$class->has_feature('snapshot', $conf, $storecfg, undef, undef, $snapname eq 'vzdump');
>
> - $snap = $conf->{snapshots}->{$snapname} = {};
> + foreach my $existing_snapshot (keys %$snapshots) {
> + my $parent_name = $snapshots->{$existing_snapshot}->{parent} // '';
> + die "snapshot '$snapname' cannot be used, $snapname already a parent for $existing_snapshot\n"
> + if $snapname eq $parent_name;
> + }
Personally, I'd prefer the "automatically fix it up"-approach, because
we we're the ones introducing the invalid property in the first place.
But if we go for the "error out"-approach, the error message should be
different. You're telling the user that the snapshot name cannot be
used, but they'll just wonder why. Instead, the error should rather tell
the user that manual fix-up is required and that the parent property is
invalid, because the parent doesn't exist.
> +
> + $snap = $snapshots->{$snapname} = {};
>
> if ($save_vmstate && $class->__snapshot_check_running($vmid)) {
> $class->__snapshot_save_vmstate($vmid, $conf, $snapname, $storecfg);
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [pve-devel] applied: [PATCH v2 container 1/2] api: clone_vm: don't include snapshot properties
2021-10-13 12:31 ` [pve-devel] [PATCH v2 container 1/2] api: clone_vm: don't include snapshot properties Oguz Bektas
@ 2021-10-14 10:01 ` Thomas Lamprecht
0 siblings, 0 replies; 5+ messages in thread
From: Thomas Lamprecht @ 2021-10-14 10:01 UTC (permalink / raw)
To: Proxmox VE development discussion, Oguz Bektas
On 13.10.21 14:31, Oguz Bektas wrote:
> apparently this caused a weird[0] bug... when a container with a snapshot was
> cloned, it would take 'parent: foo' from the original container. if you
> add a new snapshot 'bar', and then another one 'foo', this causes the
> snapshots to become parents of each other (thus not parsed correctly in
> the tree view of GUI nor with 'pct listsnapshot CTID')
>
> we also drop these properties for VMs, so it makes sense to do the same
> here as well.
>
> [0]: https://forum.proxmox.com/threads/snapshots-of-one-lxc-disappeared.97711/
>
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
> src/PVE/API2/LXC.pm | 5 +++++
> 1 file changed, 5 insertions(+)
>
>
applied this one already, thanks!
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-10-14 10:01 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-13 12:31 [pve-devel] [PATCH v2 container guest-common 0/2] snapshot parent checks for containers Oguz Bektas
2021-10-13 12:31 ` [pve-devel] [PATCH v2 container 1/2] api: clone_vm: don't include snapshot properties Oguz Bektas
2021-10-14 10:01 ` [pve-devel] applied: " Thomas Lamprecht
2021-10-13 12:31 ` [pve-devel] [PATCH v2 guest-common 2/2] snapshots: abort if new snapshot name is already parent to existing one Oguz Bektas
2021-10-14 7:34 ` Fabian Ebner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox