Re: [pve-devel] [PATCH] fix #957 iscsi: don't check tcp connection directly

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

On 3/7/25 12:59, Mira Limbeck wrote:
> Thank you for the patch!
> 
> some comments inline
> 
>> +sub iscsi_test_session {
>> +    my ($portal, $sid) = @_;
>> +    my $cmd = [$ISCSIADM, '--mode', 'session', '--sid', $sid, '-P1'];
>> +
>> +    my $res = 0;
>> +    eval {
>> +        run_command($cmd, errmsg => 'iscsi session test failed', outfunc => sub {
>> +            my $line = shift;
>> +            if ($line =~ m/^\s+iSCSI Session State: LOGGED_IN\s*$/) {
>> +                $res = 1;
>> +            }
>> +        });
>> +    };
>> +    if (my $err = $@) {
>> +        die $err;
>> +    };
>> +    return $res;
>> +}
> You pass in `$portal` which is never used. This should be removed unless
> you have a use case for which it might be needed in the future?
> 
> 
>> -
>> +    my $cache = {};
>>      for my $portal (@$portals) {
> The $portal variable is never used below. Is it necessary to even loop
> over all of them when just checking the cached sessions?
> The session loop below will be run for each of the portals, leading to a
> portals * sessions amount of iscsi_test_session calls.
Sorry, read that wrong. Best case would be the same session for each of
the portals, so `portals * 1` since there's an early exit if the result
is truthy. And only in the worst case (no session that's logged in)
would be `portals * sessions`.

> 
>> -	my $result = iscsi_test_portal($portal);
>> -	return $result if $result;
>> +        my $sessions = iscsi_session($cache, $scfg->{target});
>> +        for my $session (@$sessions) {
>> +            my $result = iscsi_test_session($portal, $session->{session_id});
>> +            return $result if $result;
>> +        }
>>      }
> 
> Making this change in `check_connection` leads to storages never being
> activated, since there's an early exit in case the storage is not
> reachable (src/PVE/Storage.pm:1196):
> ```
>     if (! eval { $plugin->check_connection($storeid, $scfg) }) {
> 	die "connection check for storage '$storeid' failed - $@\n" if $@;
> 	die "storage '$storeid' is not online\n";
>     }
> ```
> 
> Maybe this could be changed to first see if there's a session available,
> and if not, call `iscsi_test_portal`.
> And if there's a session available, one can check the session state instead.
> With this, we would still need the portals list.
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel