[PATCH v3 storage 21/76] disks: lvm: allow volume groups on SAN LUNs and multipath maps

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

The node disk API only accepts local disks for volume-group creation,
rejecting multipath maps and iSCSI LUNs. Allow this operation to opt in
to iSCSI, Fibre Channel, NVMe over Fabrics, and multipath devices so it
can set up shared LVM. Other local disk operations remain unchanged.

Check and initialize SAN LUNs under the cluster storage configuration
lock, also held by storage-add when initializing a base volume. This
prevents two nodes from both finding a LUN unused and initializing it.

Resolve the submitted device reference again under the locks. Refuse a
local device that has become a SAN device while the task waited, since
it would otherwise bypass the cluster lock. Local devices still need
no quorum.

Co-authored-by: Dietmar Maurer <dietmar@proxmox.com>
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 src/PVE/API2/Disks/LVM.pm    |  62 ++++++++++++++++-----
 src/PVE/Diskmanage.pm        |  53 ++++++++++++++++--
 src/PVE/Storage/LVMPlugin.pm |  30 ++++++++--
 src/test/disk_api_test.pm    | 105 +++++++++++++++++++++++++++++++++++
 src/test/disklist_test.pm    |  23 ++++++++
 5 files changed, 250 insertions(+), 23 deletions(-)

diff --git a/src/PVE/API2/Disks/LVM.pm b/src/PVE/API2/Disks/LVM.pm
index 9f8f9516..feab0a3b 100644
--- a/src/PVE/API2/Disks/LVM.pm
+++ b/src/PVE/API2/Disks/LVM.pm
@@ -3,6 +3,7 @@ package PVE::API2::Disks::LVM;
 use strict;
 use warnings;
 
+use PVE::Storage;
 use PVE::Storage::LVMPlugin;
 use PVE::Diskmanage;
 use PVE::JSONSchema qw(get_standard_option);
@@ -131,7 +132,8 @@ __PACKAGE__->register_method({
             name => get_standard_option('pve-storage-id'),
             device => {
                 type => 'string',
-                description => 'The block device you want to create the volume group on',
+                description => 'The block device you want to create the volume group on.'
+                    . ' Multipath mapped devices are supported.',
             },
             add_storage => {
                 description => "Configure storage using the Volume Group",
@@ -149,11 +151,17 @@ __PACKAGE__->register_method({
         my $user = $rpcenv->get_user();
 
         my $name = $param->{name};
-        my $dev = $param->{device};
+        my $reference = $param->{device};
         my $node = $param->{node};
+        my $dev = PVE::Diskmanage::verify_blockdev_path($reference);
+        my $is_san = sub {
+            my ($device) = @_;
+            return PVE::Diskmanage::is_multipath_device($device)
+                || PVE::Diskmanage::is_san_device($device);
+        };
+        my $san = $is_san->($dev);
 
-        $dev = PVE::Diskmanage::verify_blockdev_path($dev);
-        PVE::Diskmanage::assert_disk_unused($dev);
+        PVE::Diskmanage::assert_disk_unused($dev, 1);
 
         my $storage_params = {
             type => 'lvm',
@@ -176,19 +184,45 @@ __PACKAGE__->register_method({
 
         my $worker = sub {
             PVE::Diskmanage::locked_disk_action(sub {
-                PVE::Diskmanage::assert_disk_unused($dev);
-                die "volume group with name '${name}' already exists on node '${node}'\n"
-                    if PVE::Storage::LVMPlugin::lvm_vgs()->{$name};
+                my $create = sub {
+                    if ($san) {
+                        $dev = PVE::Diskmanage::verify_blockdev_path($reference);
+                    }
+                    die "cannot use '$dev' - device topology changed\n"
+                        if !$san && $is_san->($dev);
+                    PVE::Diskmanage::assert_disk_unused($dev, 1);
+                    die "volume group with name '${name}' already exists on node '${node}'\n"
+                        if PVE::Storage::LVMPlugin::lvm_vgs()->{$name};
 
-                if (PVE::Diskmanage::is_partition($dev)) {
-                    eval { PVE::Diskmanage::change_parttype($dev, '8E00'); };
-                    warn $@ if $@;
+                    if (!$san && PVE::Diskmanage::is_partition($dev)) {
+                        eval { PVE::Diskmanage::change_parttype($dev, '8E00'); };
+                        warn $@ if $@;
+                    }
+
+                    my $guard = {
+                        reference => $san ? $reference : $dev,
+                        san => $san,
+                    };
+                    PVE::Storage::LVMPlugin::lvm_create_volume_group(
+                        $guard->{reference},
+                        $name,
+                        undef,
+                        $guard,
+                    );
+
+                    PVE::Diskmanage::udevadm_trigger($dev);
+                };
+
+                # Serialize with the storage API so two nodes cannot initialize the same LUN.
+                if ($san) {
+                    PVE::Storage::lock_storage_config(
+                        $create,
+                        "creating volume group '$name' failed",
+                    );
+                } else {
+                    $create->();
                 }
 
-                PVE::Storage::LVMPlugin::lvm_create_volume_group($dev, $name);
-
-                PVE::Diskmanage::udevadm_trigger($dev);
-
                 if ($param->{add_storage}) {
                     PVE::API2::Storage::Config->create_or_update(
                         $name, $node, $storage_params, $verify_params,
diff --git a/src/PVE/Diskmanage.pm b/src/PVE/Diskmanage.pm
index 807ef0ca..caad0fee 100644
--- a/src/PVE/Diskmanage.pm
+++ b/src/PVE/Diskmanage.pm
@@ -112,16 +112,23 @@ sub assert_local_device {
 }
 
 sub disk_is_used {
-    my ($disk) = @_;
+    my ($disk, $allow_san) = @_;
 
     my $dev = $disk;
     $dev =~ s|^/dev/||;
 
     my $disklist = get_disks($dev, 1, 1);
 
-    die "'$disk' is not a valid local disk\n" if !defined($disklist->{$dev});
+    if (!defined($disklist->{$dev})) {
+        if ($allow_san) {
+            my $multipath = lookup_multipath_map($disk);
+            return defined($multipath->{used}) ? 1 : 0 if defined($multipath);
+        }
 
-    assert_local_device($disk, $disklist);
+        die "'$disk' is not a valid local disk\n";
+    }
+
+    assert_local_device($disk, $disklist) if !$allow_san;
 
     return $disklist->{$dev}->{used} ? 1 : 0;
 }
@@ -941,6 +948,42 @@ sub get_multipath_disks {
     return $disklist;
 }
 
+sub is_san_device {
+    my ($devpath) = @_;
+
+    my $dev = strip_dev(abs_path($devpath) // $devpath);
+    $dev = strip_dev(get_blockdev("/dev/$dev")) if is_partition("/dev/$dev");
+    if ($dev =~ m/^nvme\d+n\d+$/) {
+        my $transport = get_nvme_transport("/sys/block/$dev") // '';
+        return $transport =~ m/^(?:tcp|rdma|fc)$/ ? 1 : 0;
+    }
+    my $link = readlink("/sys/block/$dev") // return 0;
+    return $link =~ m{/session\d+/|/rport-\d+} ? 1 : 0;
+}
+
+# An unreadable dm UUID counts as multipath so callers fail closed.
+sub is_multipath_device {
+    my ($devpath) = @_;
+
+    my $path = abs_path($devpath) // $devpath;
+    my $dev = strip_dev($path);
+    return 0 if $dev !~ m/^dm-\d+$/;
+
+    my $uuid = file_read_firstline("/sys/block/$dev/dm/uuid");
+    return 1 if !defined($uuid);
+    return $uuid =~ m/^(?:part\d+-)?mpath-/ ? 1 : 0;
+}
+
+sub lookup_multipath_map {
+    my ($devpath) = @_;
+
+    my $path = abs_path($devpath) // $devpath;
+    my $dev = strip_dev($path);
+    return undef if $dev !~ m/^dm-\d+$/;
+
+    return get_multipath_disks()->{$dev};
+}
+
 sub get_partnum {
     my ($part_path) = @_;
 
@@ -995,8 +1038,8 @@ sub locked_disk_action {
 }
 
 sub assert_disk_unused {
-    my ($dev) = @_;
-    die "device '$dev' is already in use\n" if disk_is_used($dev);
+    my ($dev, $allow_san) = @_;
+    die "device '$dev' is already in use\n" if disk_is_used($dev, $allow_san);
     return;
 }
 
diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index d718d8f8..916f1b8f 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -147,10 +147,26 @@ sub clear_first_sector {
 }
 
 sub lvm_create_volume_group {
-    my ($device, $vgname, $shared) = @_;
+    my ($device, $vgname, $shared, $guard) = @_;
+    $guard = {%$guard} if $guard;
 
-    my $requested_device = $device;
-    $device = PVE::Diskmanage::verify_blockdev_path($device);
+    my $requested_device = $guard ? $guard->{reference} : $device;
+    $device = PVE::Diskmanage::verify_blockdev_path($requested_device);
+    my $san = $guard && $guard->{san};
+    if ($guard) {
+        if (!$san) {
+            $guard->{verify} = sub {
+                die "cannot use '$device' - device topology changed\n"
+                    if PVE::Diskmanage::is_multipath_device($device)
+                    || PVE::Diskmanage::is_san_device($device);
+            };
+            $guard->{verify}->();
+        }
+    }
+    my $check_instance = sub { $guard->{verify}->() if $guard && $guard->{verify} };
+    my $check_safe = sub {
+        $check_instance->();
+    };
     my $refuse_data = sub {
         my (@types) = @_;
         die "device '$device' contains data ("
@@ -208,6 +224,7 @@ sub lvm_create_volume_group {
     }
     my %signatures = map { $_->{type} => 1 } $probe->@*;
     my $res = lvm_pv_info($device, $signatures{LVM2_member});
+    $check_instance->();
 
     # A PV without metadata copies can belong to a VG whose other devices are not visible. The
     # sentinel is not a VG name or orphan proof, even if the use flag contradicts it.
@@ -223,17 +240,20 @@ sub lvm_create_volume_group {
     delete $signatures{LVM2_member} if $res && !$res->{vgname};
     $refuse_data->(keys %signatures) if %signatures;
 
-    clear_first_sector($device); # else pvcreate fails
+    $check_safe->();
+    clear_first_sector($device);
 
     # we use --metadatasize 250k, which reseults in "pe_start = 512"
     # so pe_start is aligned on a 128k boundary (advantage for SSDs)
     my $cmd = ['/sbin/pvcreate', '--metadatasize', '250k', $device];
 
+    $check_safe->();
     run_command($cmd, errmsg => "pvcreate '$device' error");
 
     $cmd = ['/sbin/vgcreate', $vgname, $device];
     # push @$cmd, '-c', 'y' if $shared; # we do not use this yet
 
+    $check_safe->();
     run_command(
         $cmd,
         errmsg => "vgcreate $vgname $device error",
@@ -1067,6 +1087,8 @@ sub on_add_hook {
         assert_device_discard_supported($path)
             if discard_on_remove_requested($scfg->{'on-volume-remove'});
 
+        # The storage API runs this hook under the cluster-wide storage configuration lock, which
+        # the disk API also takes for shared devices, so two nodes cannot initialize the same LUN.
         lvm_create_volume_group($path, $scfg->{vgname}, $scfg->{shared});
     }
 
diff --git a/src/test/disk_api_test.pm b/src/test/disk_api_test.pm
index c7735c0f..893b4e04 100644
--- a/src/test/disk_api_test.pm
+++ b/src/test/disk_api_test.pm
@@ -6,7 +6,10 @@ use warnings;
 use lib qw(..);
 
 use PVE::API2::Disks;
+use PVE::API2::Disks::LVM;
 use PVE::Diskmanage;
+use PVE::Storage;
+use PVE::Storage::LVMPlugin;
 
 use Test::MockModule;
 use Test::More;
@@ -110,4 +113,106 @@ is_deeply(
     'partitions inherit the visibility of their parent disk',
 );
 
+subtest 'a volume group on a map is created under the storage configuration lock' => sub {
+    my $storage = Test::MockModule->new('PVE::Storage');
+    my $disk = $diskmanage_module;
+    my $lvm = Test::MockModule->new('PVE::Storage::LVMPlugin');
+    my $rpc = Test::MockModule->new('PVE::RPCEnvironment');
+    my (@stack, @created);
+    my $lock = sub {
+        my ($name, $code) = @_;
+        push @stack, $name;
+        my $res = eval { $code->() };
+        my $err = $@;
+        pop @stack;
+        die $err if $err;
+        return $res;
+    };
+    $storage->mock(lock_storage_config => sub { $lock->('storage.cfg', $_[0]) });
+    $disk->mock(locked_disk_action => sub { $lock->('disk', $_[0]) });
+    $disk->mock(verify_blockdev_path => sub { $_[0] });
+    $disk->mock(assert_disk_unused => sub { });
+    $disk->mock(is_partition => sub { 0 });
+    $disk->mock(udevadm_trigger => sub { });
+    $lvm->mock(lvm_vgs => sub { {} });
+    $lvm->mock(lvm_create_volume_group => sub { push @created, [$_[0], join('>', @stack)] });
+    {
+
+        package MultipathTestRPC;
+        sub get_user { 'root@pam' }
+        sub check { 1 }
+        sub fork_worker { $_[4]->(); return 'test-upid' }
+    }
+    $rpc->mock(get => sub { bless {}, 'MultipathTestRPC' });
+
+    my ($is_map, $is_san) = (1, 0);
+    $disk->mock(is_multipath_device => sub { $is_map });
+    $disk->mock(is_san_device => sub { $is_san });
+    PVE::API2::Disks::LVM->create({ node => 'n1', name => 'vg1', device => '/dev/dm-3' });
+    ($is_map, $is_san) = (0, 1);
+    PVE::API2::Disks::LVM->create({ node => 'n1', name => 'vg2', device => '/dev/sdc' });
+    ($is_map, $is_san) = (0, 0);
+    PVE::API2::Disks::LVM->create({ node => 'n1', name => 'vg3', device => '/dev/sdb' });
+    is_deeply(
+        \@created,
+        [
+            ['/dev/dm-3', 'disk>storage.cfg'],
+            ['/dev/sdc', 'disk>storage.cfg'],
+            ['/dev/sdb', 'disk'],
+        ],
+        'a map or SAN LUN is initialized under the cluster-wide lock, a local disk is not',
+    );
+    @created = ();
+    $disk->mock(assert_disk_unused => sub { die "device is already in use\n" });
+    eval {
+        PVE::API2::Disks::LVM->create({
+            node => 'n1',
+            name => 'vg',
+            device => '/dev/sdc',
+        });
+    };
+    like($@, qr/already in use/, 'disk API rejects a used inventory entry');
+    is_deeply(\@created, [], 'used inventory prevents initialization');
+    $disk->mock(assert_disk_unused => sub { });
+    $lvm->mock(lvm_vgs => sub { { vg => {} } });
+    eval {
+        PVE::API2::Disks::LVM->create({
+            node => 'n1',
+            name => 'vg',
+            device => '/dev/sdc',
+        });
+    };
+    like(
+        $@,
+        qr/volume group with name 'vg' already exists/,
+        'stale inventory does not permit an existing VG',
+    );
+    is_deeply(\@created, [], 'existing VG prevents initialization');
+    $disk->unmock($_)
+        for qw(
+        locked_disk_action verify_blockdev_path assert_disk_unused is_partition
+        udevadm_trigger is_multipath_device is_san_device
+        );
+};
+
+subtest 'only multipath maps count as shared devices' => sub {
+    my $disk = $diskmanage_module;
+    my $uuid;
+    $disk->mock(abs_path => sub { $_[0] =~ m!^/dev/(?:mapper|disk)/! ? '/dev/dm-900' : $_[0] });
+    $disk->mock(file_read_firstline => sub { $uuid });
+    $uuid = 'mpath-3600abc';
+    ok(PVE::Diskmanage::is_multipath_device('/dev/disk/by-id/dm-name-lun'), 'a map alias');
+    $uuid = 'part1-mpath-3600abc';
+    ok(PVE::Diskmanage::is_multipath_device('/dev/mapper/lunp1'), 'a partition of a map');
+    $uuid = 'LVM-abcdef';
+    ok(!PVE::Diskmanage::is_multipath_device('/dev/mapper/vg-lv'), 'not a logical volume');
+    $uuid = undef;
+    ok(
+        PVE::Diskmanage::is_multipath_device('/dev/mapper/x'),
+        'an unreadable identity is shared',
+    );
+    ok(!PVE::Diskmanage::is_multipath_device('/dev/sdb'), 'a plain disk is not');
+    $disk->unmock($_) for qw(abs_path file_read_firstline);
+};
+
 done_testing();
diff --git a/src/test/disklist_test.pm b/src/test/disklist_test.pm
index dd4e4d73..21823daa 100644
--- a/src/test/disklist_test.pm
+++ b/src/test/disklist_test.pm
@@ -236,12 +236,35 @@ sub test_disk_list {
             print Dumper($multipath) if $print;
             $testcount++;
             is_deeply($multipath, $expected_multipath, 'multipath list should be the same');
+
+            eval { PVE::Diskmanage::assert_disk_unused('/dev/dm-0'); };
+            $testcount++;
+            like($@, qr/not a valid local disk/,
+                'multipath devices require an explicit opt-in');
+
+            eval { PVE::Diskmanage::assert_disk_unused('/dev/mapper/mpatha', 1); };
+            $testcount++;
+            is($@, '', 'unused multipath alias should pass');
+
+            for my $dev (qw(dm-1 dm-2 dm-3 dm-4)) {
+                eval { PVE::Diskmanage::assert_disk_unused("/dev/$dev", 1); };
+                $testcount++;
+                like($@, qr/already in use/, "used multipath device $dev should fail");
+            }
+
+            eval { PVE::Diskmanage::assert_disk_unused('/dev/dm-99', 1); };
+            $testcount++;
+            like($@, qr/not a valid local disk/, 'unknown device mapper device should fail');
         }
 
         if ($testcasedir eq 'iscsi') {
             eval { PVE::Diskmanage::assert_disk_unused('/dev/sda'); };
             $testcount++;
             like($@, qr/attached over iSCSI/, 'local disk operations reject an iSCSI LUN');
+
+            eval { PVE::Diskmanage::assert_disk_unused('/dev/sda', 1); };
+            $testcount++;
+            is($@, '', 'the SAN opt-in accepts an unused iSCSI LUN');
         }
 
         done_testing($testcount);
-- 
2.47.3