[PATCH v3 storage 38/76] api: scan: flag duplicate physical volumes on distinct LUNs

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

A snapshot clone presented beside its original can carry the same LVM
physical volume identity. LVM refuses to activate or change a volume
group with such a duplicate, so flag the affected LUNs in the inventory.

Compare SCSI LUNs by their multipath WWID and NVMe namespaces by their
kernel identity, not the udev WWN that distinct LUNs of an array can
share. Devices without a trusted identity do not take part. Only
whole-device PVs recorded by udev are compared: this is a diagnostic
hint, not a safety check.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 src/PVE/API2/Storage/Scan.pm       |   9 +++
 src/PVE/Diskmanage.pm              |   9 +++
 src/PVE/Storage/SANScan.pm         |  52 ++++++++++++-
 src/test/run_san_lun_scan_tests.pl | 116 +++++++++++++++++++++++++++++
 4 files changed, 185 insertions(+), 1 deletion(-)

diff --git a/src/PVE/API2/Storage/Scan.pm b/src/PVE/API2/Storage/Scan.pm
index 93c78ac2..f6263150 100644
--- a/src/PVE/API2/Storage/Scan.pm
+++ b/src/PVE/API2/Storage/Scan.pm
@@ -513,6 +513,15 @@ __PACKAGE__->register_method({
                     type => 'string',
                     optional => 1,
                 },
+                'duplicate-pv' => {
+                    description =>
+                        "Another listed LUN has the same LVM physical volume UUID, as"
+                        . " can happen with a snapshot clone. LVM refuses to activate or change a"
+                        . " volume group with such a duplicate. The check compares only udev"
+                        . " records for physical volumes on whole devices, so it may miss duplicates.",
+                    type => 'boolean',
+                    optional => 1,
+                },
                 'used-by-storage' => {
                     description => "The ID of an existing storage definition that uses this"
                         . " device, either through the LVM volume group on it or because it"
diff --git a/src/PVE/Diskmanage.pm b/src/PVE/Diskmanage.pm
index 0aa005d2..58a627a7 100644
--- a/src/PVE/Diskmanage.pm
+++ b/src/PVE/Diskmanage.pm
@@ -245,6 +245,15 @@ sub parse_wipefs_signatures {
     return $probe->{signatures};
 }
 
+# Read cached metadata only; never probe the device.
+sub udev_properties {
+    my ($dev) = @_;
+
+    my $devno = file_read_firstline("/sys/class/block/$dev/dev") // return {};
+    my $data = eval { file_get_contents("/run/udev/data/b$devno") } // return {};
+    return { $data =~ m/^E:([^=\n]+)=(.*)$/mg };
+}
+
 sub is_iscsi_device {
     my ($dev) = @_;
 
diff --git a/src/PVE/Storage/SANScan.pm b/src/PVE/Storage/SANScan.pm
index 415efdba..4015361b 100644
--- a/src/PVE/Storage/SANScan.pm
+++ b/src/PVE/Storage/SANScan.pm
@@ -2,6 +2,9 @@ package PVE::Storage::SANScan;
 
 use v5.36;
 
+use Cwd qw(abs_path);
+use File::Basename qw(basename);
+
 use PVE::Diskmanage;
 use PVE::Multipath;
 use PVE::Multipath::ClusterConfig;
@@ -10,6 +13,51 @@ use PVE::Multipath::Identity;
 use PVE::Storage;
 use PVE::Storage::ISCSIPlugin;
 use PVE::Storage::LVMPlugin;
+use PVE::Tools qw(dir_glob_foreach file_read_firstline);
+
+# NVMe maps use their members' namespace identity, not multipathd's WWID. Follow current slave
+# links to avoid reused names.
+my sub lun_compare_key($entry, $disk) {
+    my $scsi_key = defined($entry->{wwid}) ? "scsi $entry->{wwid}" : undef;
+    if (!defined($disk->{slaves})) {
+        my ($namespace) = $entry->{devpath} =~ m|^/dev/(nvme\d+n\d+)$|;
+        return $scsi_key if !defined($namespace);
+        my $id = file_read_firstline("/sys/block/$namespace/wwid");
+        return defined($id) && length($id) ? "nvme $id" : undef;
+    }
+
+    my $dm = basename(abs_path($entry->{devpath}) // return undef);
+    my @members;
+    dir_glob_foreach(
+        "/sys/block/$dm/slaves",
+        '([a-z]\S*)',
+        sub($name, $match) { push @members, $name },
+    );
+    return undef if !@members;
+    return $scsi_key if !grep { !m/^sd/ } @members;
+    return undef if grep { !m/^nvme/ } @members;
+    my %ids = map { (file_read_firstline("/sys/block/$dm/slaves/$_/wwid") // '') => 1 } @members;
+    return undef if scalar(keys %ids) != 1 || exists($ids{''});
+    return 'nvme ' . (keys %ids)[0];
+}
+
+my sub flag_duplicate_pvs($records) {
+    my ($pv_of, $pv_luns) = ({}, {});
+    for my $record ($records->@*) {
+        my $key = $record->{identity} // next;
+        my $dev = abs_path($record->{row}->{devpath}) // next;
+        my $props = PVE::Diskmanage::udev_properties(basename($dev));
+        next if ($props->{ID_FS_TYPE} // '') ne 'LVM2_member';
+        # the rules for device-mapper devices only keep the encoded form
+        my $uuid = $props->{ID_FS_UUID_ENC} // $props->{ID_FS_UUID} // next;
+        $pv_of->{$record} = $uuid;
+        $pv_luns->{$uuid}->{$key} = 1;
+    }
+    for my $record ($records->@*) {
+        my $uuid = $pv_of->{$record} // next;
+        $record->{row}->{'duplicate-pv'} = 1 if scalar(keys $pv_luns->{$uuid}->%*) > 1;
+    }
+}
 
 my sub collect_inventory($cfg, $node) {
     # Collect identities before disks so path_wwid() rejects names reused during enumeration.
@@ -208,7 +256,8 @@ my sub build_lun_rows($cfg, $inventory) {
                 $entry->{multipath} = 'unmanaged';
             }
         }
-        push $records->@*, { row => $entry, disk => $disk };
+        push $records->@*,
+            { row => $entry, disk => $disk, identity => lun_compare_key($entry, $disk) };
     };
     for my $dev (sort keys %$disks) {
         my $disk = $disks->{$dev};
@@ -223,6 +272,7 @@ sub scan($cfg, $node) {
     my $inventory = collect_inventory($cfg, $node);
     my $records = build_lun_rows($cfg, $inventory);
 
+    flag_duplicate_pvs($records);
     return [map { $_->{row} } $records->@*];
 }
 
diff --git a/src/test/run_san_lun_scan_tests.pl b/src/test/run_san_lun_scan_tests.pl
index c9f130ff..1771a5c8 100755
--- a/src/test/run_san_lun_scan_tests.pl
+++ b/src/test/run_san_lun_scan_tests.pl
@@ -1,5 +1,10 @@
 #!/usr/bin/perl
 
+# Unit test for the SAN LUN scan: the WWID it reports for a path without a map, from the LUN
+# identities of PVE::Multipath::Identity, and the duplicate-PV flag, which compares LUNs only under
+# an identity that is known, never under the fallback names of a device. The disk listing, udev,
+# the LUN identities and the multipath state are mocked.
+
 use strict;
 use warnings;
 
@@ -64,6 +69,32 @@ $diskmanage->mock(
         };
     },
     get_nvme_path_status => sub { {} },
+    udev_properties => sub {
+        my ($dev) = @_;
+        my $pv = $dev =~ m/^dm-(.+)$/ ? $maps->{$1}->{pv} : ($devices->{$dev} // {})->{pv};
+        return defined($pv) ? { ID_FS_TYPE => 'LVM2_member', ID_FS_UUID_ENC => $pv } : {};
+    },
+);
+# a map /dev/mapper/<wwid> is the dm device dm-<wwid>, its current members are 'members', or else
+# 'slaves' as listed
+my $scan = Test::MockModule->new('PVE::Storage::SANScan');
+$scan->mock(
+    abs_path => sub { $_[0] =~ s|^/dev/mapper/|/dev/dm-|r },
+    dir_glob_foreach => sub {
+        my ($dir, $regex, $func) = @_;
+        my ($wwid) = $dir =~ m|^/sys/block/dm-(.+)/slaves$| or return;
+        my $map = $maps->{$wwid} // return;
+        for my $name (($map->{members} // $map->{slaves})->@*) {
+            my @matches = $name =~ m/^($regex)$/;
+            $func->(@matches) if @matches;
+        }
+    },
+    file_read_firstline => sub {
+        my ($file) = @_;
+        return $namespace_ids->{$1} if $file =~ m|^/sys/block/(\w+)/wwid$|;
+        return $namespace_ids->{$1} if $file =~ m|^/sys/block/dm-[^/]+/slaves/(\w+)/wwid$|;
+        return undef;
+    },
 );
 my $multipath = Test::MockModule->new('PVE::Multipath');
 $multipath->mock(
@@ -150,6 +181,11 @@ my sub scan {
     }
     return { map { $_->{devpath} =~ s|^/dev/(mapper/)?||r => $_ } @$res };
 }
+my sub flagged {
+    my ($res) = @_;
+    return [sort grep { $res->{$_}->{'duplicate-pv'} } keys %$res];
+}
+
 # a map of LUN 0 over sdb and sdc, and sdd, a path of it without a map yet
 ($devices, $maps) = (
     {
@@ -168,6 +204,7 @@ $reused->{sdd} = 1;
 ok(!defined(scan()->{sdd}->{wwid}), 'wwid: none for a name that denotes another device by now');
 is(scan()->{sdd}->{'wwid-unknown'}, 1, 'changed path exports identity uncertainty');
 $reused = {};
+is_deeply(flagged($res), [], 'duplicate: not a map and a path of the same LUN');
 $resolvable = 0;
 $res = scan();
 is($res->{sdd}->{wwid}, '3W0', 'wwid: also without the daemon, through the map of its LUN');
@@ -186,22 +223,101 @@ ok(
 );
 ok(!defined($res->{sdl}->{multipath}), 'and no multipath state either, as it is not known');
 is($res->{sdl}->{'wwid-unknown'}, 1, 'daemon unavailable exports uncertainty');
+is_deeply(flagged($res), [], 'duplicate: and not flagged then');
 $resolvable = 1;
+is_deeply(flagged(scan()), ['3W0', 'sdl'], 'duplicate: but once the daemon names it');
 
 # two paths of LUN 1 without a map
 ($devices, $maps) =
     ({ sd('sde', 1, 'PV1', daemon => '3W1'), sd('sdf', 1, 'PV1', daemon => '3W1') }, {});
+is_deeply(flagged(scan()), [], 'duplicate: not two paths of one LUN');
 $devices->{sdf}->{daemon} = '3OTHER';
 $res = scan();
 is($res->{$_}->{'wwid-unknown'}, 1, "disagreeing LUN path $_ is uncertain") for qw(sde sdf);
 $devices->{sdf}->{daemon} = '3W1';
 
+# a clone, LUN 2, of the PV of the map of LUN 0
+($devices, $maps) = (
+    { sd('sdb', 0, undef, map => '3W0'), sd('sdg', 2, 'PV0', daemon => '3W2') },
+    { '3W0' => { slaves => ['sdb'], pv => 'PV0' } },
+);
+is_deeply(flagged(scan()), ['3W0', 'sdg'], 'duplicate: a clone and its original, both flagged');
+
+# LUNs 3 and 4 share the truncated udev WWN of their array
+($devices, $maps) = (
+    {
+        sd('sdh', 3, 'PV3', daemon => '3W3', wwn => '0x600a098038323553'),
+        sd('sdi', 4, 'PV4', daemon => '3W4', wwn => '0x600a098038323553'),
+    },
+    {},
+);
+is_deeply(flagged(scan()), [], 'duplicate: not distinct PVs on LUNs sharing a WWN');
+$devices->{sdi}->{pv} = 'PV3';
+is_deeply(flagged(scan()), ['sdh', 'sdi'], 'duplicate: one PV on two LUNs sharing a WWN');
+
 # local disks without any identity
 ($devices, $maps) = ({ sd('sdj', undef, 'PV5'), sd('sdk', undef, 'PV5') }, {});
+is_deeply(flagged(scan()), [], 'duplicate: not devices without an identity, whatever their WWN');
 ok(!scan()->{sdj}->{'wwid-unknown'}, 'unlisted path is not marked uncertain');
 $devices->{sdj}->{daemon} = '';
 is(scan()->{sdj}->{'wwid-unknown'}, 1, 'listed path without identity is uncertain');
 
+# NVMe namespaces, and maps over NVMe paths
+($devices, $maps) = (
+    {
+        nvme0n1 =>
+            { disk => { devpath => '/dev/nvme0n1', size => 1024, used => 'LVM' }, pv => 'PV6' },
+        nvme1n1 =>
+            { disk => { devpath => '/dev/nvme1n1', size => 1024, used => 'LVM' }, pv => 'PV6' },
+    },
+    {},
+);
+$namespace_ids = { nvme0n1 => 'eui.0001', nvme1n1 => 'eui.0002' };
+is_deeply(flagged(scan()), ['nvme0n1', 'nvme1n1'], 'duplicate: two namespaces with one PV');
+$namespace_ids->{nvme1n1} = 'eui.0001';
+is_deeply(flagged(scan()), [], 'duplicate: not two heads of one namespace');
+
+$devices =
+    { nvme4n1 =>
+        { disk => { devpath => '/dev/nvme4n1', size => 1024, used => 'LVM' }, pv => 'PV7' } };
+$maps = { CUSTOM => { slaves => ['nvme2n1', 'nvme3n1'], pv => 'PV7' } };
+$namespace_ids = { nvme2n1 => 'uuid.a', nvme3n1 => 'uuid.a', nvme4n1 => 'uuid.a' };
+is_deeply(
+    flagged(scan()),
+    [],
+    'duplicate: not a map over NVMe paths that multipathd names its own way, and a path of it',
+);
+$namespace_ids->{nvme3n1} = 'uuid.b';
+$namespace_ids->{nvme4n1} = 'uuid.c';
+is_deeply(flagged(scan()), [], 'duplicate: not a map whose paths disagree');
+$namespace_ids->{nvme3n1} = 'uuid.a';
+is_deeply(flagged(scan()), ['CUSTOM', 'nvme4n1'], 'duplicate: a map over NVMe paths and a clone');
+
+# a map that has lost all its members, next to a namespace with the same PV
+$devices =
+    { nvme5n1 =>
+        { disk => { devpath => '/dev/nvme5n1', size => 1024, used => 'LVM' }, pv => 'PV8' } };
+$maps = { EMPTY => { slaves => ['nvme6n1'], members => [], pv => 'PV8' } };
+$namespace_ids = { nvme5n1 => 'uuid.e', nvme6n1 => 'uuid.e' };
+is_deeply(flagged(scan()), [], 'duplicate: not a map without members, whatever the listing says');
+
+# a member without an identity, and members of two transports
+$maps = { NOID => { slaves => ['nvme6n1', 'nvme7n1'], pv => 'PV8' } };
+is_deeply(flagged(scan()), [], 'duplicate: not a map with a member without an identity');
+$maps = { MIXED => { slaves => ['nvme6n1', 'sdz'], pv => 'PV8' } };
+is_deeply(flagged(scan()), [], 'duplicate: not a map with members of two transports');
+
+# a namespace identity longer than multipathd keeps
+my $long = 'nvme.1af4-' . ('4e4554415050' x 20);
+$devices =
+    { nvme8n1 =>
+        { disk => { devpath => '/dev/nvme8n1', size => 1024, used => 'LVM' }, pv => 'PV9' } };
+$maps = { SHORTENED => { slaves => ['nvme9n1'], pv => 'PV9' } };
+$namespace_ids = { nvme8n1 => $long, nvme9n1 => $long };
+is_deeply(flagged(scan()), [], 'duplicate: a map and a path of one namespace with a long identity');
+$namespace_ids->{nvme8n1} = "${long}0";
+is_deeply(flagged(scan()), ['SHORTENED', 'nvme8n1'], 'duplicate: and two namespaces');
+
 # the real collection and derivation on a fake sysfs: a failing daemon query runs only once, and
 # both the scan and the storage derivation do without its data
 {
-- 
2.47.3