[PATCH v3 storage 40/76] iscsi: name plain LUNs after their identity link

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

Storages without the multipath option name each LUN after the last of
its by-id links. udev can add aliases, such as SCSI name strings, that
sort after scsi-<WWID> but do not encode the LUN identity, so the
volume name alone does not tell which LUN it denotes. Use the
scsi-<WWID> link for such LUNs instead, so operations on the volume can
verify the LUN. The identities are only collected when a LUN has such
aliases.

Existing references to an alias name still resolve.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 src/PVE/Storage/ISCSIPlugin.pm          | 15 ++++--
 src/test/run_iscsi_device_list_tests.pl |  2 +-
 src/test/run_lun_identity_tests.pl      | 70 +++++++++++++++++++++++++
 3 files changed, 83 insertions(+), 4 deletions(-)

diff --git a/src/PVE/Storage/ISCSIPlugin.pm b/src/PVE/Storage/ISCSIPlugin.pm
index ad3afaf6..cadbda8a 100644
--- a/src/PVE/Storage/ISCSIPlugin.pm
+++ b/src/PVE/Storage/ISCSIPlugin.pm
@@ -26,6 +26,7 @@ use base qw(PVE::Storage::Plugin);
 my $ISCSIADM = '/usr/bin/iscsiadm';
 our $INITIATOR_NAME_FILE = '/etc/iscsi/initiatorname.iscsi';
 our $SYSFS = '/sys';
+our $BY_ID_DIR = '/dev/disk/by-id';
 my $MULTIPATHD_EXECUTABLES = { '/usr/sbin/multipathd' => 1, '/sbin/multipathd' => 1 };
 
 sub read_initiator_info {
@@ -437,7 +438,7 @@ sub load_stable_scsi_paths {
 
     my $stable_paths = {};
 
-    my $stabledir = "/dev/disk/by-id";
+    my $stabledir = $BY_ID_DIR;
 
     if (my $dh = IO::Dir->new($stabledir)) {
         foreach my $tmp (sort $dh->read) {
@@ -448,7 +449,7 @@ sub load_stable_scsi_paths {
                 my $path = "$stabledir/$tmp";
                 my $bdevdest = readlink($path);
                 if ($bdevdest && $bdevdest =~ m|^../../([^/]+)|) {
-                    $stable_paths->{$1} = $tmp;
+                    push $stable_paths->{$1}->@*, $tmp;
                 }
             }
         }
@@ -721,7 +722,8 @@ sub iscsi_device_list {
             $bdev = $multipathdev if $multipathdev;
         }
 
-        my $blockdev = $stable_paths->{$bdev};
+        my $names = $stable_paths->{$bdev} // [];
+        my $blockdev = $names->[-1];
         # scsi-<wwid> can name a member before assembly; dm-uuid-mpath-<wwid> only names the map.
         # Managed volumes keep the map name even while assembly is pending.
         if ($map_names) {
@@ -730,6 +732,13 @@ sub iscsi_device_list {
                 ? PVE::Multipath::Identity::path_wwid($ids, $path_dev)
                 : undef;
             $blockdev = defined($wwid) ? "dm-uuid-mpath-$wwid" : undef;
+        } elsif ((grep { m/^scsi-/ } @$names) > 1) {
+            # udev can add aliases, such as SCSI name strings, that sort after scsi-<wwid> but do
+            # not encode the LUN identity; prefer the name that does, so operations on the volume
+            # can verify which LUN it denotes.
+            $ids //= PVE::Multipath::Identity::collect(all => 1);
+            my $wwid = PVE::Multipath::Identity::path_wwid($ids, $path_dev);
+            $blockdev = "scsi-$wwid" if defined($wwid) && grep { $_ eq "scsi-$wwid" } @$names;
         }
         next if !$blockdev;
 
diff --git a/src/test/run_iscsi_device_list_tests.pl b/src/test/run_iscsi_device_list_tests.pl
index 6df3a416..00ce2684 100755
--- a/src/test/run_iscsi_device_list_tests.pl
+++ b/src/test/run_iscsi_device_list_tests.pl
@@ -55,7 +55,7 @@ session(4, 9, 'iqn.target-d');
 my $plugin = Test::MockModule->new('PVE::Storage::ISCSIPlugin');
 $plugin->mock(
     load_stable_scsi_paths => sub {
-        return { map { $_ => "scsi-3$_" } qw(sda sdb sdc sdd) };
+        return { map { $_ => ["scsi-3$_"] } qw(sda sdb sdc sdd) };
     },
 );
 
diff --git a/src/test/run_lun_identity_tests.pl b/src/test/run_lun_identity_tests.pl
index c5363e46..a49f27c4 100755
--- a/src/test/run_lun_identity_tests.pl
+++ b/src/test/run_lun_identity_tests.pl
@@ -597,6 +597,76 @@ is_deeply(
     'volumes: every path of a resolved LUN named after its map, an unresolved LUN left out',
 );
 
+{
+    # Without the option, LUNs are named after their by-id links, here in a scratch directory:
+    # { device => [lun, wwid, links] }
+    my $fixture = {
+        sdzzb => [0, '3W0', ['scsi-3W0', 'scsi-3W0-part1', 'scsi-8naa.W0']],
+        sdzzc => [1, '2E1', ['scsi-2E1', 'scsi-8eui.E1']],
+        sdzzd => [2, '3W2', ['scsi-3W2', 'scsi-SLIO-ORG_disk_W2']],
+        sdzze => [3, '3W3', ['scsi-3W3']],
+        sdzzf => [4, undef, ['scsi-3W4', 'scsi-8W4']],
+        sdzzg => [5, '3W5', ['scsi-8W5', 'scsi-SW5']],
+    };
+    my ($disks, $lines) = ({}, {});
+    for my $name (keys %$fixture) {
+        my ($lun, $wwid) = $fixture->{$name}->@*;
+        my $disk = { target => $T, lun => $lun, devt => "8:$lun", hctl => "9:0:0:$lun" };
+        $disks->{$name} = $disk;
+        $lines->{$name} = { $disk->%{qw(devt hctl target)}, init => 'ok', wwid => $wwid }
+            if defined($wwid);
+    }
+    my $plain = { luns => PVE::Multipath::Identity::resolve($disks, $lines, $naa_ok) };
+    my $by_id = tempdir(CLEANUP => 1);
+    my sub link_devices {
+        unlink(glob("$by_id/*"));
+        for my $name (@_) {
+            symlink("../../$name", "$by_id/$_") or die "$!\n" for $fixture->{$name}->[2]->@*;
+        }
+    }
+    local $PVE::Storage::ISCSIPlugin::BY_ID_DIR = $by_id;
+    my $collected = 0;
+    my $session_device_map = \&PVE::Storage::ISCSIPlugin::iscsi_session_device_map;
+    $identity->mock(collect => sub { $collected++; $plain });
+    $iscsi->unmock('load_stable_scsi_paths');
+    $iscsi->mock(
+        iscsi_session_device_map => sub {
+            return {
+                map {
+                    $_ => { target => $T, channel => 0, id => 0, lun => $disks->{$_}->{lun} }
+                    }
+                    keys %$disks
+            };
+        },
+    );
+    my $list = sub { [sort keys PVE::Storage::ISCSIPlugin::iscsi_device_list(0, $T)->{$T}->%*] };
+    link_devices(keys %$fixture);
+    my $listed = $list->();
+    is_deeply(
+        $listed,
+        [
+            '0.0.0.scsi-3W0',
+            '0.0.1.scsi-2E1',
+            '0.0.2.scsi-3W2',
+            '0.0.3.scsi-3W3',
+            '0.0.4.scsi-8W4',
+            '0.0.5.scsi-SW5',
+        ],
+        'plain volumes: the identity name over aliases, else the last name as before',
+    );
+    is($collected, 1, 'plain volumes: identities collected once');
+    link_devices('sdzze');
+    $collected = 0;
+    is_deeply($list->(), ['0.0.3.scsi-3W3'], 'plain volumes: a LUN without aliases');
+    is($collected, 0, 'plain volumes: needs no identities');
+
+    $iscsi->mock(
+        iscsi_session_device_map => $session_device_map,
+        load_stable_scsi_paths => sub { {} },
+    );
+    $identity->mock(collect => sub { $ids });
+}
+
 my $collected = 0;
 $identity->mock(collect => sub { $collected++; return { luns => {} } });
 my $derived = PVE::Storage::derived_multipath_wwids({
-- 
2.47.3