[pve-devel] [PATCH] [FIX] Fix snapshot logic and enable lvmthin storage type.

[Fabian Grünbichler]

comments inline again

> Dietmar Maurer <dietmar at proxmox.com> hat am 12. Januar 2016 um 07:47
> geschrieben:
> 
> 
> comments inline
> 
> > On January 10, 2016 at 10:45 AM Gerrit Venema <gmoniker at gmail.com> wrote:
> > 
> > 
> > This enables the container to use the lvmthin storage type if one is set up.
> > It also fixes some logic in the handling of the freeze, errors on rollback
> > and the handling of the config sections to keep a correct tree of snapshots.
> > 
> 
> 
> > @@ -1840,17 +1841,17 @@ sub snapshot_delete {
> >      my $del_snap =  sub {
> >  
> >  	check_lock($conf);
> > -
> > -	if ($conf->{parent} eq $snapname) {
> > -	    if ($conf->{snapshots}->{$snapname}->{snapname}) {
> > -		$conf->{parent} = $conf->{snapshots}->{$snapname}->{parent};
> > -	    } else {
> > -		delete $conf->{parent};
> > +	my $grand_parent = $conf->{snapshots}->{$snapname}->{parent};
> > +	foreach my $snapkey (keys %{$conf->{snapshots}}) {
> > +	    if ($conf->{snapshots}->{$snapkey}->{parent} eq $snapname) {
> > +		if ($grand_parent) {
> > +		    $conf->{snapshots}->{$snapkey}->{parent} = $grand_parent;
> > +		} else {
> > +	            delete $conf->{snapshots}->{$snapkey}->{parent};
> > +		}
> >  	    }
> >  	}
> 
> It is really hard to see the purpose of this code. I thought the current code
> works? If not, would you mind to send a test case (example code) which
> triggers
> the bug you want to fix?

The current code was indeed buggy/incomplete. 

The correct approach is a mix of both old and new: on snapshot removal, we want
to relink all children of the removed snapshot with the parent of the removed
snapshot, not just the currently active snapshot. If there is no such parent
(i.e., the removed snapshot was an old entry point of our directed snapshot
graph), we can simply delete the parent key for all the children (making them
new entry points/roots).

> 
> I think this patch contains 3 different things:
> 
> 1.) snapshot create bug fix
> 2.) snapshot config bug fix?
> 3.) enable lvmthin storage
> 
> It is usually better to split such patches.

See below for a more complete fix for 2.), thanks for the initial patch!

Regards,
Fabian

-- >8 --
Subject: [PATCH container] Rework snapshot config removal logic

Correctly update parent relations in config file upon snapshot removal. 

Previously, only the parent of the current state was updated/removed,
which led to broken parent relations if any snapshot other then the
immediate parent of the current snapshot was removed. To fix this,
the parent relation of all children snapshots of the removed snapshot
are updated/removed as well.

Based on code in qemu-server/PVE/QemuServer.pm and parts
of a patch by Gerrit Venema <gmoniker at gmail.com>

Signed-off-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
---
 src/PVE/LXC.pm | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 2899fa9..3e4b6b8 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -1865,17 +1865,29 @@ sub snapshot_delete {
 
     my $storecfg = PVE::Storage::config();
 
-    my $del_snap =  sub {
+    my $unlink_parent = sub {
 
-	check_lock($conf);
+	my ($confref, $new_parent) = @_;
 
-	if ($conf->{parent} eq $snapname) {
-	    if ($conf->{snapshots}->{$snapname}->{snapname}) {
-		$conf->{parent} = $conf->{snapshots}->{$snapname}->{parent};
+	if ($confref->{parent} && $confref->{parent} eq $snapname) {
+	    if ($new_parent) {
+		$confref->{parent} = $new_parent;
 	    } else {
-		delete $conf->{parent};
+		delete $confref->{parent};
 	    }
 	}
+    };
+
+    my $del_snap =  sub {
+
+	check_lock($conf);
+
+	my $grand_parent = $conf->{snapshots}->{$snapname}->{parent};
+	foreach my $snapkey (keys %{$conf->{snapshots}}) {
+	    &$unlink_parent($conf->{snapshots}->{$snapkey}, $grand_parent);
+	}
+
+	&$unlink_parent($conf, $grand_parent);
 
 	delete $conf->{snapshots}->{$snapname};
 
-- 
2.1.4