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

[Mira Limbeck <m.limbeck@proxmox.com>]

On 5/23/26 11:28 PM, Thomas Lamprecht wrote:
> 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?
No, that was an oversight.


> 
>> @@ -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.
Will adapt. Although with Samuel's findings on patch 07/15, it may be
necessary to rework the way logins work in general. Possibly to log in
to each target/portal combination separately, rather than all portals
for a target in one go.