[pve-devel] [PATCH v4 storage] fix #2085: Handle non-default mount point in path() by introducing new mountpoint property

[Fabian Ebner]

When adding a zfspool storage with 'pvesm add' the mount point is now added
automatically to the storage configuration if it can be determined.
path() does not assume the default mountpoint anymore, fixing 2085.

Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
---

Changes from v3:
    * create a new 'mounpoint' property instead of using 'path', since
      'path' is used to check whether storage is filesystem based

 PVE/Storage/ZFSPoolPlugin.pm   | 31 +++++++++++++++++++++++++++++--
 test/run_test_zfspoolplugin.pl | 26 ++++++++++++++------------
 2 files changed, 43 insertions(+), 14 deletions(-)

diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
index b8adf1c..507795b 100644
--- a/PVE/Storage/ZFSPoolPlugin.pm
+++ b/PVE/Storage/ZFSPoolPlugin.pm
@@ -32,6 +32,10 @@ sub properties {
 	    description => "use sparse volumes",
 	    type => 'boolean',
 	},
+	mountpoint => {
+	    description => "mount point",
+	    type => 'string', format => 'pve-storage-path',
+	},
     };
 }
 
@@ -44,6 +48,7 @@ sub options {
 	disable => { optional => 1 },
 	content => { optional => 1 },
 	bwlimit => { optional => 1 },
+	mountpoint => { optional => 1 },
     };
 }
 
@@ -142,17 +147,39 @@ sub parse_volname {
 
 # virtual zfs methods (subclass can overwrite them)
 
+sub on_add_hook {
+    my ($class, $storeid, $scfg, %param) = @_;
+
+    my $cfg_mountpoint = $scfg->{mountpoint};
+    my $mountpoint;
+
+    # ignore failure, pool might currently not be imported
+    eval {
+	$mountpoint = $class->zfs_get_properties($scfg, 'mountpoint', $scfg->{pool}, 1);
+	PVE::JSONSchema::check_format(properties()->{mountpoint}->{format}, $mountpoint);
+    };
+
+    if (defined($cfg_mountpoint)) {
+	if (defined($mountpoint) && !($cfg_mountpoint =~ m|^\Q$mountpoint\E/?$|)) {
+	    warn "warning for $storeid - mountpoint: $cfg_mountpoint " .
+		 "does not match current mount point: $mountpoint\n";
+	}
+    } else {
+	$scfg->{mountpoint} = $mountpoint;
+    }
+}
+
 sub path {
     my ($class, $scfg, $volname, $storeid, $snapname) = @_;
 
     my ($vtype, $name, $vmid) = $class->parse_volname($volname);
 
     my $path = '';
+    my $mountpoint = $scfg->{mountpoint} // "/$scfg->{pool}";
 
     if ($vtype eq "images") {
 	if ($name =~ m/^subvol-/ || $name =~ m/^basevol-/) {
-	    # fixme: we currently assume standard mount point?!
-	    $path = "/$scfg->{pool}/$name";
+	    $path = "$mountpoint/$name";
 	} else {
 	    $path = "/dev/zvol/$scfg->{pool}/$name";
 	}
diff --git a/test/run_test_zfspoolplugin.pl b/test/run_test_zfspoolplugin.pl
index 785d3b7..9c5e841 100755
--- a/test/run_test_zfspoolplugin.pl
+++ b/test/run_test_zfspoolplugin.pl
@@ -16,6 +16,7 @@ my $verbose = undef;
 
 my $storagename = "zfstank99";
 my $subvol = 'regressiontest';
+my $mountpoint = "${subvol}_mnt";
 
 #volsize in GB
 my $volsize = 1;
@@ -142,10 +143,10 @@ my $test19 = sub {
     $fail = 0;
     eval {
 	@res = PVE::Storage::path($cfg, "$storagename:$ctdisk");
-	if ($res[0] ne "\/regressiontest\/$ctdisk") {
+	if ($res[0] ne "\/$mountpoint\/$ctdisk") {
 	    $count++;
 	    $fail = 1;
-	    warn "Test 19 d: path is not correct: expected \'/regressiontest\/$ctdisk'\  get \'$res[0]\'";
+	    warn "Test 19 d: path is not correct: expected \'\/$mountpoint\/$ctdisk'\  get \'$res[0]\'";
 	}
 	if ($res[1] ne "202") {
 	    if (!$fail) {
@@ -171,10 +172,10 @@ my $test19 = sub {
     $fail = 0;
     eval {
 	@res = PVE::Storage::path($cfg, "$storagename:$ctbase");
-	if ($res[0] ne "\/regressiontest\/$ctbase") {
+	if ($res[0] ne "\/$mountpoint\/$ctbase") {
 	    $count++;
 	    $fail = 1;
-	    warn "Test 19 e: path is not correct: expected \'\/regressiontest\/$ctbase'\  get \'$res[0]\'";
+	    warn "Test 19 e: path is not correct: expected \'\/$mountpoint\/$ctbase'\  get \'$res[0]\'";
 	}
 	if ($res[1] ne "200") {
 	    if (!$fail) {
@@ -200,10 +201,10 @@ my $test19 = sub {
     $fail = 0;
     eval {
 	@res = PVE::Storage::path($cfg, "$storagename:$ctbase\/$ctlinked");
-	if ($res[0] ne "\/regressiontest\/$ctlinked") {
+	if ($res[0] ne "\/$mountpoint\/$ctlinked") {
 	    $count++;
 	    $fail = 1;
-	    warn "Test 19 f: path is not correct: expected \'\/regressiontest\/$ctlinked'\  get \'$res[0]\'";
+	    warn "Test 19 f: path is not correct: expected \'\/$mountpoint\/$ctlinked'\  get \'$res[0]\'";
 	}
 	if ($res[1] ne "201") {
 	    if (!$fail) {
@@ -1188,11 +1189,11 @@ my $test7 = sub {
     eval {
 	PVE::Storage::volume_snapshot($cfg, "$storagename:$ctdisk", 'snap1');
 
-	run_command("touch \/$zpath\/$ctdisk\/test.txt", outfunc => $parse_guid);
+	run_command("touch \/$mountpoint\/$ctdisk\/test.txt", outfunc => $parse_guid);
 	eval {
 	    PVE::Storage::volume_snapshot_rollback($cfg, "$storagename:$ctdisk", 'snap1');
 	    eval {
-		run_command("ls \/$zpath\/$ctdisk\/test.txt", errofunc => sub {});
+		run_command("ls \/$mountpoint\/$ctdisk\/test.txt", errofunc => sub {});
 	    };
 	    if (!$@) {
 		$count++;
@@ -1212,11 +1213,11 @@ my $test7 = sub {
     eval {
 	PVE::Storage::volume_snapshot($cfg, "$storagename:$ctbase", 'snap1');
 
-	run_command("touch \/$zpath\/$ctbase\/test.txt", outfunc => $parse_guid);
+	run_command("touch \/$mountpoint\/$ctbase\/test.txt", outfunc => $parse_guid);
 	eval {
 	    PVE::Storage::volume_snapshot_rollback($cfg, "$storagename:$ctbase", 'snap1');
 	    eval {
-		run_command("ls \/$zpath\/$ctbase\/test.txt", errofunc => sub {});
+		run_command("ls \/$mountpoint\/$ctbase\/test.txt", errofunc => sub {});
 	    };
 	    if (!$@) {
 		$count++;
@@ -1236,7 +1237,7 @@ my $test7 = sub {
     eval {
 	PVE::Storage::volume_snapshot($cfg, "$storagename:$ctbase/$ctlinked", 'snap1');
 
-	run_command("touch \/$zpath\/$ctlinked\/test.txt", outfunc => $parse_guid);
+	run_command("touch \/$mountpoint\/$ctlinked\/test.txt", outfunc => $parse_guid);
 	eval {
 	    PVE::Storage::volume_snapshot_rollback($cfg, "$storagename:$ctbase/$ctlinked", 'snap1');
 	    eval {
@@ -2650,7 +2651,7 @@ sub setup_zpool {
     }
     my $pwd = cwd();
     eval {
-	run_command("zpool create $subvol $pwd\/zpool.img");
+	run_command("zpool create -m \/$mountpoint $subvol $pwd\/zpool.img");
     };
     if ($@) {
 	clean_up_zpool();
@@ -2698,6 +2699,7 @@ $cfg = {'ids' => {
 	    'rootdir' => 1
 	},
 		'pool' => $subvol,
+		'mountpoint' => "\/$mountpoint",
 		'type' => 'zfspool'
     },
 	},
-- 
2.20.1