[pve-devel] [PATCH v2 container guest-common 0/2] snapshot parent checks for containers

[pve-devel] [PATCH v2 container guest-common 0/2] snapshot parent checks for containers

[Oguz Bektas]

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

[pve-devel] [PATCH v2 container 1/2] api: clone_vm: don't include snapshot properties

[Oguz Bektas]

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

[pve-devel] applied: [PATCH v2 container 1/2] api: clone_vm: don't include snapshot properties

[Thomas Lamprecht]

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!

[pve-devel] [PATCH v2 guest-common 2/2] snapshots: abort if new snapshot name is already parent to existing one

[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;
+	}
+
+	$snap = $snapshots->{$snapname} = {};
 
 	if ($save_vmstate && $class->__snapshot_check_running($vmid)) {
 	    $class->__snapshot_save_vmstate($vmid, $conf, $snapname, $storecfg);
-- 
2.30.2

Re: [pve-devel] [PATCH v2 guest-common 2/2] snapshots: abort if new snapshot name is already parent to existing one

[Fabian Ebner]

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