Re: [PATCH v2 storage 08/15] iscsi: rework to update discovery db and simplify login

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

On Thu, 30 Apr 2026 19:27:06 +0200, Mira Limbeck wrote:
> diff --git a/src/PVE/Storage/ISCSIPlugin.pm b/src/PVE/Storage/ISCSIPlugin.pm
> @@ -699,32 +703,30 @@ sub activate_storage {
>      my $do_login = 0;
> +    my $login_targets = {};
>      for my $target (keys $targets->%*) {
> -        $do_login = 1 if !defined($targets->{$target}->{sessions});
> -    }
> -
> -    if (!$do_login) {
> -        # We should check that sessions for all portals are available
> -        for my $target (keys $targets->%*) {
> -            my $session_portals = [map { $_->{portal} } ($targets->{$target}->{sessions}->@*)];
> -            for my $portal ($targets->{$target}->{portals}->@*) {
> -                if (!grep(/^\Q$portal\E(?::3260)?$/, $session_portals->@*)) {
> -                    $do_login = 1;
> -                    last;
> -                }
> +        my $sessions = [map { $_->{portal} } ($targets->{$target}->{sessions}->@*)];
> +        for my $portal ($targets->{$target}->{portals}->@*) {
> +            if (!grep(/\Q$portal\E$/, $sessions->@*)) {
> +                push $login_targets->{$target}->@*, $portal;
>              }
>          }
>      }

This dropped the leading `^` anchor that the removed code had. With only the
trailing `$` the config portal is matched as a suffix of the session string,
so e.g. "10.0.0.4:3260" matches a session for host "110.0.0.4". Any reason
for not keeping the exact `^...$` match?

> @@ -259,7 +256,14 @@ sub iscsi_login {
> -    run_command([$ISCSIADM, '--mode', 'node', '--targetname', $target, '--login']);
> +    # iscsiadm warns if some target/portals are already logged in
> +    # a workaround would be to check each of them and only log in on those
> +    # that have no session yet, rather than just doing a log in per node
> +    my $cmd = [
> +        $ISCSIADM, '--mode', 'node', '--targetname', $target, '--login',
> +    ];
> +    eval { run_command($cmd); };
> +    warn $@ if $@;

Login was an un-evaled hard call before; now a total login failure only
warns and activate_storage returns success, while check_connection only
pings portals - so activation can report success with zero usable LUNs.
Might be still worth distinguishing "all logins failed" and propagating
that.