[PATCH storage 0/7] iscsi: per-node target and portal configuration

[PATCH storage 0/7] iscsi: per-node target and portal configuration

[Dietmar Maurer]

Where every node reaches the same SAN through a different portal, and
sometimes through a different target, a single cluster-wide
portal/target pair is not enough. Patch 5 adds an 'iscsi-node-map'
property to the iSCSI storage: static target and portal entries, each
optionally restricted to a set of nodes. Entries without a node list
act as the default, so only the nodes that differ need one, and a node
can resolve to several targets, which allows multipath across
per-controller target names. Mapped entries skip sendtargets discovery
and sync the node db records directly, which also covers SANs that
filter discovery or do not implement it.

Patch 7 adds 'periodic-discovery'. Re-running discovery before every
login resets tuned node.* settings, so with periodic-discovery=0 it
only runs to seed an empty node db. The default keeps the current
behavior.

Patches 1 and 2 are independent bug fixes and stand on their own.
Patch 2 is Mira's idea, from patches 11 and 12 of [0].

There is no GUI for either property yet, both are API and CLI only.

[0] solves the same problem one level up, with a cluster-wide
mapping/storage.cfg, a plugin framework, a CRUD API and an ACL path,
and Thomas found that direction right at a high level. I still think
the inline property fits iSCSI better: a mapping cannot usefully be
shared, since every entry names a target, and it has to list every
node while this needs only the exceptions. 

[0] https://lore.proxmox.com/pve-devel/20260430173220.441001-1-m.limbeck@proxmox.com/

Dietmar Maurer (7):
  iscsi: discovery: do not stop early on a foreign target
  iscsi: scan: do not persist discovery results in the node database
  iscsi: validate target names with a dedicated format
  iscsi: clarify that the portal property is the discovery address
  iscsi: add iscsi-node-map property for per-node target and portals
  iscsi: iscsi_portals: return empty list instead of fallback portal
  iscsi: add periodic-discovery flag to skip re-discovery on login

 src/PVE/Storage.pm             |   2 +-
 src/PVE/Storage/ISCSIPlugin.pm | 369 +++++++++++++++++++++++++++++----
 2 files changed, 326 insertions(+), 45 deletions(-)

-- 
2.47.3

[PATCH storage 1/7] iscsi: discovery: do not stop early on a foreign target

[Dietmar Maurer]

sendtargets returns every portal of a target in one call, so querying
further portals is pointless once the requested target showed up. The
check only looked at whether any target came back, so a portal that
serves other targets aborted the loop before the portals that actually
serve the requested one were queried.

Discovery without a requested target now queries all given portals,
which is what a caller enumerating a SAN wants.

Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
---
 src/PVE/Storage/ISCSIPlugin.pm | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Storage/ISCSIPlugin.pm b/src/PVE/Storage/ISCSIPlugin.pm
index 801b5d1..807adbd 100644
--- a/src/PVE/Storage/ISCSIPlugin.pm
+++ b/src/PVE/Storage/ISCSIPlugin.pm
@@ -134,6 +134,7 @@ sub iscsi_discovery {
     for my $portal ($portals->@*) {
         next if !iscsi_test_portal($target_in, $portal, $cache); # fixme: raise exception here?
 
+        my $target_found = 0;
         my $cmd = [$ISCSIADM, '--mode', 'discovery', '--type', 'sendtargets', '--portal', $portal];
         eval {
             run_command(
@@ -146,13 +147,16 @@ sub iscsi_discovery {
                         # one target can have more than one portal (multipath)
                         # and sendtargets should return all of them in single call
                         push @{ $res->{$target} }, $portal;
+
+                        $target_found = 1 if defined($target_in) && $target eq $target_in;
                     }
                 },
             );
         };
 
-        # In case of multipath we can stop after receiving targets from any available portal
-        last if scalar(keys %$res) > 0;
+        # sendtargets returns all portals of a target, so one hit is enough. Without a
+        # requested target, query every portal to get the full picture
+        last if $target_found;
     }
 
     return $res;
-- 
2.47.3

[PATCH storage 2/7] iscsi: scan: do not persist discovery results in the node database

[Dietmar Maurer]

Scanning a portal is an inspection operation, but sendtargets discovery
writes a node db record for every target the portal advertises. So
filling the target dropdown in the storage wizard, or an abandoned
wizard run, leaves the scanning node with records for targets nobody
configured, and makes it diverge from its peers.

Ask iscsiadm to leave the node db alone for scans. Discovery on the
login path keeps persisting, since logging in works off those records.

Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
---
 src/PVE/Storage.pm             | 2 +-
 src/PVE/Storage/ISCSIPlugin.pm | 6 +++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 64ea9da..81df50f 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -1655,7 +1655,7 @@ sub scan_iscsi {
         die "unable to parse/resolve portal address '${portal_in}'\n";
     }
 
-    return PVE::Storage::ISCSIPlugin::iscsi_discovery(undef, [$portal]);
+    return PVE::Storage::ISCSIPlugin::iscsi_discovery(undef, [$portal], undef, 1);
 }
 
 sub storage_default_format {
diff --git a/src/PVE/Storage/ISCSIPlugin.pm b/src/PVE/Storage/ISCSIPlugin.pm
index 807adbd..9944806 100644
--- a/src/PVE/Storage/ISCSIPlugin.pm
+++ b/src/PVE/Storage/ISCSIPlugin.pm
@@ -125,8 +125,11 @@ sub iscsi_portals {
     }
 }
 
+# Discovery persists a node db record for every returned target unless
+# $nonpersistent is set. Callers that only inspect a portal should set it,
+# callers that log in afterwards rely on the records.
 sub iscsi_discovery {
-    my ($target_in, $portals, $cache) = @_;
+    my ($target_in, $portals, $cache, $nonpersistent) = @_;
 
     assert_iscsi_support();
 
@@ -136,6 +139,7 @@ sub iscsi_discovery {
 
         my $target_found = 0;
         my $cmd = [$ISCSIADM, '--mode', 'discovery', '--type', 'sendtargets', '--portal', $portal];
+        push @$cmd, '--op', 'nonpersistent' if $nonpersistent;
         eval {
             run_command(
                 $cmd,
-- 
2.47.3

[PATCH storage 3/7] iscsi: validate target names with a dedicated format

[Dietmar Maurer]

The target property accepted any string, so typos or pasted portal
addresses were only caught later when iscsiadm fails. Validate against
the iSCSI name grammar from RFC 7143 (iqn, eui and naa types, 223 byte
limit). Accept uppercase letters even where the grammar only permits
lowercase, because such admin-typed names exist in the wild and work,
and rejecting them would break existing setups on upgrade.

This also covers the iSCSI direct and ZFS over iSCSI plugins, which
share the property.

Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
---
 src/PVE/Storage/ISCSIPlugin.pm | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/src/PVE/Storage/ISCSIPlugin.pm b/src/PVE/Storage/ISCSIPlugin.pm
index 9944806..0165a58 100644
--- a/src/PVE/Storage/ISCSIPlugin.pm
+++ b/src/PVE/Storage/ISCSIPlugin.pm
@@ -334,6 +334,27 @@ sub iscsi_device_list {
 
 # Configuration
 
+# Name grammar from RFC 7143 section 6.1: iqn, eui and naa types with a
+# 223 byte limit. The IQN grammar only permits lowercase, but accept any
+# case because admin-typed names with uppercase letters exist and work.
+sub verify_iscsi_target {
+    my ($target, $noerr) = @_;
+
+    if (
+        length($target) > 223
+        || $target !~ m/^(?:
+            iqn\.\d{4}-\d{2}(?:\.[a-z0-9-]+)+(?::[a-z0-9.:-]+)?
+            |eui\.[0-9a-f]{16}
+            |naa\.(?:[0-9a-f]{16}|[0-9a-f]{32})
+        )$/xi
+    ) {
+        return undef if $noerr;
+        die "value does not look like a valid iSCSI target name\n";
+    }
+    return $target;
+}
+PVE::JSONSchema::register_format('pve-storage-iscsi-target', \&verify_iscsi_target);
+
 sub type {
     return 'iscsi';
 }
@@ -351,6 +372,7 @@ sub properties {
         target => {
             description => "iSCSI target.",
             type => 'string',
+            format => 'pve-storage-iscsi-target',
         },
         portal => {
             description => "iSCSI portal (IP or DNS name with optional port).",
-- 
2.47.3

[PATCH storage 4/7] iscsi: clarify that the portal property is the discovery address

[Dietmar Maurer]

The value is only used for sendtargets discovery and as a ping
fallback; the actual connection portals come from discovery.

Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
---
 src/PVE/Storage/ISCSIPlugin.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/PVE/Storage/ISCSIPlugin.pm b/src/PVE/Storage/ISCSIPlugin.pm
index 0165a58..85366ba 100644
--- a/src/PVE/Storage/ISCSIPlugin.pm
+++ b/src/PVE/Storage/ISCSIPlugin.pm
@@ -375,7 +375,8 @@ sub properties {
             format => 'pve-storage-iscsi-target',
         },
         portal => {
-            description => "iSCSI portal (IP or DNS name with optional port).",
+            description =>
+                "Address used for iSCSI sendtargets discovery (IP or DNS name with optional port).",
             type => 'string',
             format => 'pve-storage-portal-dns',
         },
-- 
2.47.3

[PATCH storage 5/7] iscsi: add iscsi-node-map property for per-node target and portals

[Dietmar Maurer]

On setups where every node reaches the same SAN through a different
portal and target, a single cluster-wide portal/target pair is not
enough. Allow configuring a list of static mappings, each optionally
restricted to a set of nodes.

Mapped entries bypass sendtargets discovery: node db records are
created for exactly the listed portals and stale records are pruned,
so this also works when the SAN filters or does not implement
discovery. Entries explicitly listing a node replace the entries
without a node list on that node, and portals of entries with the
same target are merged. Entries without a target apply to the
cluster-wide target, so per-node portals do not require repeating
it. A node can resolve to multiple targets, which allows multipath
across per-controller target names. Nodes without an applicable
entry keep using the cluster-wide target and discovery address.

Pruning logs out the session on a dropped portal first, because
iscsiadm refuses to remove a record that a session still uses. So a
portal removed from the configuration does not keep a session alive
that nothing re-establishes. A stale session is kept as long as no
configured portal has one, so replacing a portal cannot leave the
target without a path; the next poll prunes it once the replacement
is up.

Resolving the configuration for the local node is a package sub, so
code that needs to know which targets and how many portals a node
should end up with can reuse it.

Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
---
 src/PVE/Storage/ISCSIPlugin.pm | 297 +++++++++++++++++++++++++++++----
 1 file changed, 264 insertions(+), 33 deletions(-)

diff --git a/src/PVE/Storage/ISCSIPlugin.pm b/src/PVE/Storage/ISCSIPlugin.pm
index 85366ba..585b8c2 100644
--- a/src/PVE/Storage/ISCSIPlugin.pm
+++ b/src/PVE/Storage/ISCSIPlugin.pm
@@ -7,6 +7,7 @@ use File::stat;
 use IO::Dir;
 use IO::File;
 
+use PVE::INotify;
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::Storage::Plugin;
 use PVE::Tools
@@ -125,6 +126,117 @@ sub iscsi_portals {
     }
 }
 
+# normalize for comparison: iscsiadm reports portals with an explicit port
+my sub normalize_portal {
+    my ($portal) = @_;
+
+    my ($host, $port) = PVE::Tools::parse_host_and_port($portal);
+    return undef if !$host;
+    $port //= 3260;
+    return "$host:$port";
+}
+
+# make the node db records for $target match exactly the given portals
+sub iscsi_sync_node_records {
+    my ($target, $portals, $cache) = @_;
+
+    assert_iscsi_support();
+
+    my $wanted = {};
+    for my $portal (@$portals) {
+        my $key = normalize_portal($portal);
+        $wanted->{$key} = $portal if defined($key);
+    }
+
+    my $existing = {};
+    eval {
+        run_command(
+            [$ISCSIADM, '--mode', 'node'],
+            outfunc => sub {
+                my $line = shift;
+
+                if ($line =~ $ISCSI_TARGET_RE) {
+                    my ($portal, $record_target) = ($1, $2);
+                    if ($record_target eq $target) {
+                        my $key = normalize_portal($portal);
+                        $existing->{$key} = $portal if defined($key);
+                    }
+                }
+            },
+        );
+    };
+
+    for my $key (sort keys %$wanted) {
+        next if $existing->{$key};
+        my $cmd = [
+            $ISCSIADM,
+            '--mode',
+            'node',
+            '--op',
+            'new',
+            '--targetname',
+            $target,
+            '--portal',
+            $wanted->{$key},
+        ];
+        eval { run_command($cmd); };
+        warn $@ if $@;
+    }
+
+    # prune stale records, so removed portals no longer get logged into
+    my $stale = [grep { !$wanted->{$_} } sort keys %$existing];
+    return if !scalar(@$stale);
+
+    # iscsiadm refuses to drop a record that a session still uses, so the
+    # session has to go first
+    my $sids = {};
+    for my $session (@{ iscsi_session($cache, $target) // [] }) {
+        my $key = normalize_portal($session->{portal});
+        $sids->{$key} = $session->{session_id} if defined($key);
+    }
+
+    # A portal that replaced another one only gets a session from the login
+    # that follows, so tearing the old one down here would leave the target
+    # without a path. Keep it until a configured portal took over.
+    my $keep_last_path =
+        !grep { $sids->{$_} && iscsi_test_session($sids->{$_}) } keys %$wanted;
+
+    for my $key (@$stale) {
+        my $portal = $existing->{$key};
+
+        if (defined(my $sid = $sids->{$key})) {
+            if ($keep_last_path) {
+                warn "keeping stale iscsi session $sid for $target via $portal:"
+                    . " no configured portal has a session yet\n";
+                next;
+            }
+
+            print "logging out of stale iscsi session $sid: $target via $portal\n";
+            eval { run_command([$ISCSIADM, '--mode', 'session', '--sid', $sid, '--logout']); };
+            if (my $err = $@) {
+                warn $err;
+                next; # dropping the record would fail as well
+            }
+            # session state changed, force a re-query
+            delete $cache->{iscsi_sessions};
+        }
+
+        my $cmd = [
+            $ISCSIADM,
+            '--mode',
+            'node',
+            '--op',
+            'delete',
+            '--targetname',
+            $target,
+            '--portal',
+            $portal,
+        ];
+        eval { run_command($cmd); };
+        warn $@ if $@;
+    }
+}
+
 # Discovery persists a node db record for every returned target unless
 # $nonpersistent is set. Callers that only inspect a portal should set it,
 # callers that log in afterwards rely on the records.
@@ -167,12 +279,15 @@ sub iscsi_discovery {
 }
 
 sub iscsi_login {
-    my ($target, $portals, $cache) = @_;
+    my ($target, $portals, $cache, $static) = @_;
 
     assert_iscsi_support();
 
-    eval { iscsi_discovery($target, $portals, $cache); };
-    warn $@ if $@;
+    # static configurations have their node db records synced already
+    if (!$static) {
+        eval { iscsi_discovery($target, $portals, $cache); };
+        warn $@ if $@;
+    }
 
     # Disable retries to avoid blocking pvestatd for too long, next iteration will retry anyway
     eval {
@@ -367,6 +482,75 @@ sub plugindata {
     };
 }
 
+my $iscsi_node_map_fmt = {
+    nodes => get_standard_option(
+        'pve-node-list',
+        {
+            description => "List of nodes for which this entry applies (all nodes if unset).",
+            optional => 1,
+        },
+    ),
+    target => {
+        description => "iSCSI target used for this entry (defaults to the storage's target).",
+        type => 'string',
+        format => 'pve-storage-iscsi-target',
+        optional => 1,
+    },
+    portals => {
+        description => "Static list of portals used for this target (bypasses discovery).",
+        type => 'string',
+        format => 'pve-storage-portal-dns-list',
+    },
+};
+
+# Resolves the storage configuration for the local node into
+# { static => 0|1, targets => [{ target, portals }, ...] }, where 'static'
+# tells whether the portals come from an iscsi-node-map entry rather than
+# from discovery.
+#
+# Entries explicitly listing the node replace the entries without a node
+# list; portals of entries with the same target are merged. Falls back to
+# the cluster-wide target and discovery address when no entry applies.
+sub get_node_targets {
+    my ($scfg) = @_;
+
+    my $nodename = PVE::INotify::nodename();
+
+    my $explicit = [];
+    my $default = [];
+    for my $item (@{ $scfg->{'iscsi-node-map'} // [] }) {
+        my $entry = PVE::JSONSchema::parse_property_string($iscsi_node_map_fmt, $item);
+        $entry->{target} //= $scfg->{target};
+        if (!defined($entry->{nodes})) {
+            push @$default, $entry;
+        } elsif (grep { $_ eq $nodename } PVE::Tools::split_list($entry->{nodes})) {
+            push @$explicit, $entry;
+        }
+    }
+
+    my $entries = scalar(@$explicit) ? $explicit : $default;
+    if (!scalar(@$entries)) {
+        return {
+            static => 0,
+            targets => [{ target => $scfg->{target}, portals => [$scfg->{portal}] }],
+        };
+    }
+
+    my $portals = {};
+    my $targets = [];
+    for my $entry (@$entries) {
+        my $target = $entry->{target};
+        push @$targets, $target if !$portals->{$target};
+        $portals->{$target}->{$_} = 1 for PVE::Tools::split_list($entry->{portals});
+    }
+
+    return {
+        static => 1,
+        targets =>
+            [map { { target => $_, portals => [sort keys %{ $portals->{$_} }] } } @$targets],
+    };
+}
+
 sub properties {
     return {
         target => {
@@ -380,6 +564,18 @@ sub properties {
             type => 'string',
             format => 'pve-storage-portal-dns',
         },
+        'iscsi-node-map' => {
+            description => "Static target and portal configuration, bypassing sendtargets"
+                . " discovery. Entries that explicitly list a node replace the entries"
+                . " without a node list on that node. Portals of entries with the same"
+                . " target are merged.",
+            type => 'array',
+            optional => 1,
+            items => {
+                type => 'string',
+                format => $iscsi_node_map_fmt,
+            },
+        },
     };
 }
 
@@ -387,6 +583,7 @@ sub options {
     return {
         portal => { fixed => 1 },
         target => { fixed => 1 },
+        'iscsi-node-map' => { optional => 1 },
         nodes => { optional => 1 },
         disable => { optional => 1 },
         content => { optional => 1 },
@@ -465,11 +662,14 @@ sub list_images {
 
     # we have no owner for iscsi devices
 
-    my $target = $scfg->{target};
-
-    if (my $dat = $cache->{iscsi_devices}->{$target}) {
+    my $seen = {};
+    for my $node_target (@{ get_node_targets($scfg)->{targets} }) {
+        my $dat = $cache->{iscsi_devices}->{ $node_target->{target} };
+        next if !$dat;
 
         foreach my $volname (keys %$dat) {
+            # a LUN reached via multiple targets shares its stable path
+            next if $seen->{$volname}++;
 
             my $volid = "$storeid:$volname";
 
@@ -500,8 +700,14 @@ sub iscsi_session {
 sub status {
     my ($class, $storeid, $scfg, $cache) = @_;
 
-    my $session = iscsi_session($cache, $scfg->{target});
-    my $active = defined($session) ? 1 : 0;
+    # activate_storage keeps trying to log into targets without a session
+    my $active = 0;
+    for my $node_target (@{ get_node_targets($scfg)->{targets} }) {
+        if (defined(iscsi_session($cache, $node_target->{target}))) {
+            $active = 1;
+            last;
+        }
+    }
 
     return (0, 0, 0, $active);
 }
@@ -511,28 +717,41 @@ sub activate_storage {
 
     return if !assert_iscsi_support(1);
 
-    my $sessions = iscsi_session($cache, $scfg->{target});
-    my $portals = iscsi_portals($scfg->{target}, $scfg->{portal});
-    my $do_login = !defined($sessions);
+    my $node_targets = get_node_targets($scfg);
+    my $static = $node_targets->{static};
 
-    if (!$do_login) {
-        # We should check that sessions for all portals are available
-        my $session_portals = [map { $_->{portal} } (@$sessions)];
+    for my $node_target (@{ $node_targets->{targets} }) {
+        my ($target, $portals) = ($node_target->{target}, $node_target->{portals});
 
-        for my $portal (@$portals) {
-            if (!grep(/^\Q$portal\E$/, @$session_portals)) {
-                $do_login = 1;
-                last;
+        if ($static) {
+            iscsi_sync_node_records($target, $portals, $cache);
+        } else {
+            $portals = iscsi_portals($target, $portals->[0]);
+        }
+
+        my $sessions = iscsi_session($cache, $target);
+        my $do_login = !defined($sessions);
+
+        if (!$do_login) {
+            # We should check that sessions for all portals are available
+            my $session_portals =
+                { map { (normalize_portal($_->{portal}) // '') => 1 } (@$sessions) };
+
+            for my $portal (@$portals) {
+                if (!$session_portals->{ normalize_portal($portal) // '' }) {
+                    $do_login = 1;
+                    last;
+                }
             }
         }
-    }
 
-    if ($do_login) {
-        eval { iscsi_login($scfg->{target}, $portals, $cache); };
-        warn $@ if $@;
-    } else {
-        # make sure we get all devices
-        iscsi_session_rescan($sessions);
+        if ($do_login) {
+            eval { iscsi_login($target, $portals, $cache, $static); };
+            warn $@ if $@;
+        } else {
+            # make sure we get all devices
+            iscsi_session_rescan($sessions);
+        }
     }
 }
 
@@ -541,8 +760,11 @@ sub deactivate_storage {
 
     return if !assert_iscsi_support(1);
 
-    if (defined(iscsi_session($cache, $scfg->{target}))) {
-        iscsi_logout($scfg->{target});
+    for my $node_target (@{ get_node_targets($scfg)->{targets} }) {
+        my $target = $node_target->{target};
+        if (defined(iscsi_session($cache, $target))) {
+            iscsi_logout($target);
+        }
     }
 }
 
@@ -640,18 +862,27 @@ sub activate_volume {
     my $device_path = $udev_query_path->($real_path);
     my $resolved_paths = $resolve_virtual_devices->($device_path);
 
-    my $found = $check_devices_part_of_target->($resolved_paths, $scfg->{target});
-    die "volume '$volname' not part of target '$scfg->{target}'\n" if !$found;
+    my $found = 0;
+    for my $node_target (@{ get_node_targets($scfg)->{targets} }) {
+        $found = $check_devices_part_of_target->($resolved_paths, $node_target->{target});
+        last if $found;
+    }
+    die "volume '$volname' not part of any configured target\n" if !$found;
 }
 
 sub check_connection {
     my ($class, $storeid, $scfg) = @_;
     my $cache = {};
-    my $portals = iscsi_portals($scfg->{target}, $scfg->{portal});
 
-    for my $portal (@$portals) {
-        my $result = iscsi_test_portal($scfg->{target}, $portal, $cache);
-        return $result if $result;
+    my $node_targets = get_node_targets($scfg);
+    for my $node_target (@{ $node_targets->{targets} }) {
+        my ($target, $portals) = ($node_target->{target}, $node_target->{portals});
+        $portals = iscsi_portals($target, $portals->[0]) if !$node_targets->{static};
+
+        for my $portal (@$portals) {
+            my $result = iscsi_test_portal($target, $portal, $cache);
+            return $result if $result;
+        }
     }
 
     return 0;
-- 
2.47.3

[PATCH storage 6/7] iscsi: iscsi_portals: return empty list instead of fallback portal

[Dietmar Maurer]

Returning the configured discovery address when the node db has no
records (or iscsiadm fails) hides whether records exist from callers.
Move the fallback to the call sites, so an upcoming change can gate
sendtargets discovery on the node db being empty.

No functional change.

Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
---
 src/PVE/Storage/ISCSIPlugin.pm | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/src/PVE/Storage/ISCSIPlugin.pm b/src/PVE/Storage/ISCSIPlugin.pm
index 585b8c2..85f33aa 100644
--- a/src/PVE/Storage/ISCSIPlugin.pm
+++ b/src/PVE/Storage/ISCSIPlugin.pm
@@ -94,7 +94,7 @@ sub iscsi_test_portal {
 }
 
 sub iscsi_portals {
-    my ($target, $portal_in) = @_;
+    my ($target) = @_;
 
     assert_iscsi_support();
 
@@ -115,15 +115,12 @@ sub iscsi_portals {
             },
         );
     };
-
-    my $err = $@;
-    warn $err if $err;
-
-    if ($err || !scalar(@$res)) {
-        return [$portal_in];
-    } else {
-        return $res;
+    if (my $err = $@) {
+        warn $err;
+        return [];
     }
+
+    return $res;
 }
 
 # normalize for comparison: iscsiadm reports portals with an explicit port
@@ -726,7 +723,9 @@ sub activate_storage {
         if ($static) {
             iscsi_sync_node_records($target, $portals, $cache);
         } else {
-            $portals = iscsi_portals($target, $portals->[0]);
+            # fall back to the configured discovery address on an empty node db
+            my $recorded = iscsi_portals($target);
+            $portals = $recorded if scalar(@$recorded);
         }
 
         my $sessions = iscsi_session($cache, $target);
@@ -877,7 +876,11 @@ sub check_connection {
     my $node_targets = get_node_targets($scfg);
     for my $node_target (@{ $node_targets->{targets} }) {
         my ($target, $portals) = ($node_target->{target}, $node_target->{portals});
-        $portals = iscsi_portals($target, $portals->[0]) if !$node_targets->{static};
+        if (!$node_targets->{static}) {
+            # fall back to the configured discovery address on an empty node db
+            my $recorded = iscsi_portals($target);
+            $portals = $recorded if scalar(@$recorded);
+        }
 
         for my $portal (@$portals) {
             my $result = iscsi_test_portal($target, $portal, $cache);
-- 
2.47.3

[PATCH storage 7/7] iscsi: add periodic-discovery flag to skip re-discovery on login

[Dietmar Maurer]

Every login first re-runs sendtargets discovery, which auto-persists
node records and resets their node.* settings to defaults. This
clobbers tuned records (timeouts, iface binding) and re-imposes the
portal's full advertised set, fighting intentional per-node record
divergence.

With periodic-discovery=0, discovery only runs when the node db has
no record for the target, seeding it once; later logins use the
existing records as-is. Login stays poll-driven either way, so
dropped sessions are still re-established on the next poll. The
default (1) keeps the current re-discover-on-login behavior. Static
iscsi-node-map configurations already bypass discovery, so the flag
has no effect there.

Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
---
 src/PVE/Storage/ISCSIPlugin.pm | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/src/PVE/Storage/ISCSIPlugin.pm b/src/PVE/Storage/ISCSIPlugin.pm
index 85f33aa..33197b9 100644
--- a/src/PVE/Storage/ISCSIPlugin.pm
+++ b/src/PVE/Storage/ISCSIPlugin.pm
@@ -276,12 +276,12 @@ sub iscsi_discovery {
 }
 
 sub iscsi_login {
-    my ($target, $portals, $cache, $static) = @_;
+    my ($target, $portals, $cache, $skip_discovery) = @_;
 
     assert_iscsi_support();
 
-    # static configurations have their node db records synced already
-    if (!$static) {
+    # skipped when the node db records for the target already exist
+    if (!$skip_discovery) {
         eval { iscsi_discovery($target, $portals, $cache); };
         warn $@ if $@;
     }
@@ -573,6 +573,15 @@ sub properties {
                 format => $iscsi_node_map_fmt,
             },
         },
+        'periodic-discovery' => {
+            description => "Re-run sendtargets discovery before every login (default)."
+                . " When disabled, discovery only runs when the node database has no"
+                . " record for the target, and logins use the existing records as-is."
+                . " Has no effect when a static iscsi-node-map configuration applies.",
+            type => 'boolean',
+            default => 1,
+            optional => 1,
+        },
     };
 }
 
@@ -581,6 +590,7 @@ sub options {
         portal => { fixed => 1 },
         target => { fixed => 1 },
         'iscsi-node-map' => { optional => 1 },
+        'periodic-discovery' => { optional => 1 },
         nodes => { optional => 1 },
         disable => { optional => 1 },
         content => { optional => 1 },
@@ -716,16 +726,22 @@ sub activate_storage {
 
     my $node_targets = get_node_targets($scfg);
     my $static = $node_targets->{static};
+    my $periodic_discovery = $scfg->{'periodic-discovery'} // 1;
 
     for my $node_target (@{ $node_targets->{targets} }) {
         my ($target, $portals) = ($node_target->{target}, $node_target->{portals});
+        my $skip_discovery = $static;
 
         if ($static) {
             iscsi_sync_node_records($target, $portals, $cache);
         } else {
             # fall back to the configured discovery address on an empty node db
             my $recorded = iscsi_portals($target);
-            $portals = $recorded if scalar(@$recorded);
+            if (scalar(@$recorded)) {
+                $portals = $recorded;
+                # seed-once mode: the node db already holds records for this target
+                $skip_discovery = 1 if !$periodic_discovery;
+            }
         }
 
         my $sessions = iscsi_session($cache, $target);
@@ -745,7 +761,7 @@ sub activate_storage {
         }
 
         if ($do_login) {
-            eval { iscsi_login($target, $portals, $cache, $static); };
+            eval { iscsi_login($target, $portals, $cache, $skip_discovery); };
             warn $@ if $@;
         } else {
             # make sure we get all devices
-- 
2.47.3