Re: [PATCH v2 storage 07/15] iscsi: introduce helper to update discovery db

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

On Thu, 30 Apr 2026 19:27:05 +0200, Mira Limbeck wrote:
> diff --git a/src/PVE/Storage/ISCSIPlugin.pm b/src/PVE/Storage/ISCSIPlugin.pm
> @@ -368,6 +398,140 @@ sub iscsi_device_list {
> +    } else {
> +        my $portals = undef;
> +        my $target = undef;
> +        for my $config_target (keys $local_cfg->{targets}->%*) {
> +            $target = $config_target;
> +            $portals = iscsi_portals($config_target, $local_cfg->{targets}->{$config_target}->[0]);
> +            last;
> +        }
> +        die "no target found for discovery\n" if !defined($target);
> +        die "no portals found for discovery\n" if !defined($portals);
> +
> +        my $res = eval { iscsi_discovery($target, $portals, $cache); };
> +        warn $@ if $@;

With 08/15 calling update_iscsi_discovery_db() unconditionally at the top of
activate_storage, a plain (non-mapping) iSCSI storage now runs a persistent
sendtargets discovery plus a per-portal tcp_ping every pvestatd cycle, so
every ~10s. But currently (without this series), discovery runs only when a
login was actually needed. On a SAN exposing many targets/portals this is a
recurring cost and a behavior change. Could be gated on need (no session /
changed portal set), or use non-persistent discovery here too.

> +        if (defined($res->{$target})) {
> +            $local_cfg->{targets}->{$target} = [map { $_->{portal} } $res->{$target}->@*];

Bisectability: this maps `$_->{portal}` over the discovery result, but
iscsi_discovery only returns hashrefs from 13/15 - through 07-12 it still pushes
plain portal strings. So once 08/15 calls this unconditionally, plain-iSCSI
activate_storage dies ("Can't use a string as a HASH ref") every cycle until
13. Worth reordering so each commit builds and runs.

> +        } else {
> +            # no portal is discovered, keep already configured node entries
> +            # in that case, otherwise we might remove sessions still in use
> +            #
> +            # cleanup in that case should be done manually after verifying that
> +            # active sessions are no longer needed
> +            $local_cfg->{targets}->{$target} = $portals;
> +        }
> +    }

The "keep configured entries, clean up manually" caution is good - but 09/15
then adds a second pass that does not honor it; see reply on 09.