Re: [pve-devel] [PATCH storage v4] fix #957 iscsi: improve iscsi_test_portal logic

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

On 3/18/25 17:11, Friedrich Weber wrote:
> Hi, thanks for the new version! I think this is shaping up nicely. Some
> comments inline below, but only minor ones. So it may make sense to wait
> a couple of days for additional comments from others before sending a
> new version. I'll also run a few more tests this week and report back.
> 
>> don't check tcp connection directly if there are already
>> sessions.
>>
>> pvestatd is calling check_connection every 10 seconds.
>> This check produces a lot of noise at the iscsi server logging.
>>
>> Signed-off-by: Victor Seva <linuxmaniac@torreviejawireless.org>
>> ---
>>
>> changes since v3:
>>
>> * iscsi_test_session():
>>   read /sys/class/iscsi_session/session${sid}/state instead of
>>   $ISCSIADM call
>>
>> * iscsi_test_portal():
>>   add optional parameters target and cache. If target is defined,
>>   used it to check session status instead of check tcp port
>>
>> * iscsi_discovery():
>>   add optional parameters target and cache and pass it to
>>   iscsi_test_portal call
>>
>> * iscsi_login():
>>   add optional parameter cache and pass it to iscsi_discovery call
>>
>> * activate_storage():
>>   pass cache parameter to iscsi_login call
>>
>> * check_connection():
>>   add target and cache parameters to iscsi_test_portal call  
> 
> Thank you for adding these!
> 
>>
>>  src/PVE/Storage.pm             |  2 +-
>>  src/PVE/Storage/ISCSIPlugin.pm | 41 ++++++++++++++++++++++++++--------
>>  2 files changed, 33 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
>> index 3b4f041..c5d4ff8 100755
>> --- a/src/PVE/Storage.pm
>> +++ b/src/PVE/Storage.pm
>> @@ -1479,7 +1479,7 @@ sub scan_iscsi {
>>  	die "unable to parse/resolve portal address '${portal_in}'\n";
>>      }
>>  
>> -    return PVE::Storage::ISCSIPlugin::iscsi_discovery([ $portal ]);
>> +    return PVE::Storage::ISCSIPlugin::iscsi_discovery(undef, [ $portal ]);
>>  }
> 
> On first sight I wanted to suggest to keep $portals as the first
> argument and add $target and $cache as optional second+third argument,
> but now I see that `$target, $portals, $cache` allows us to have similar
> argument orderings for `iscsi_discovery`, `iscsi_login` and
> `iscsi_test_portal`, so I think this is fine.
> 
>>  
>>  sub storage_default_format {
>> diff --git a/src/PVE/Storage/ISCSIPlugin.pm b/src/PVE/Storage/ISCSIPlugin.pm<
>> index eb70453..c7bacea 100644
>> --- a/src/PVE/Storage/ISCSIPlugin.pm
>> +++ b/src/PVE/Storage/ISCSIPlugin.pm
>> @@ -58,9 +58,32 @@ sub iscsi_session_list {
>>      return $res;
>>  }
>>  
>> -sub iscsi_test_portal {
>> -    my ($portal) = @_;
>> +sub iscsi_test_session {
>> +    my ($sid) = @_;
>>  
>> +    if ($sid !~ m/^\d+$/) {
> 
> It's safer to change \d to [0-9], see [1].
We parse it with `(\S+)` in iscsi_session_list(). But since it looks
like it can only be ASCII [0][1], limiting it to `[0-9]` should be better.

> 
>> +	die "session_id: '$sid' is not a number\n";
>> +    }
>> +    my $state = file_read_firstline("/sys/class/iscsi_session/session${sid}/state");
>> +    if ($state eq 'LOGGED_IN') {
>> +	return 1;
>> +    }
>> +    return 0;
>> +}
> 
> Can be shortened to
> 
> 	return $state eq 'LOGGED_IN';
> 
> but it would be better to also check for definedness because
> file_read_firstline returns undef if the file does not exist, which is
> unlikely but not impossible:
> 
> 	return defined($state) && $state eq 'LOGGED_IN';
> 
>> +
>> +sub iscsi_test_portal {
>> +    my ($target, $portal, $cache) = @_;
>> +    $cache //= {};
>> +
>> +    if (defined($target)) {
> 
> Right, good catch, the $target == undef case is needed to allow initial
> discovery.
> 
>> +	# check session state instead if available
>> +	my $sessions = iscsi_session($cache, $target);
>> +	for my $session ($sessions->@*) {
>> +	    next if $session->{portal} ne $portal;
>> +	    return iscsi_test_session($session->{session_id});
> 
> So if we have a session but it is not LOGGED_IN, we return 0.
> I know this is what I suggested in my v3 comment, but now I'm not so
> sure anymore. Couldn't it be the case that the session is broken for
> some reason, but discovery would still works? In such a case, we would
> now consider the portal offline. We could instead fall back to a TCP ping:
> 	
> 	my $state = iscsi_test_session($session->{session_id});
> 	return $state if $state;
> 
> Any opinions (from others)?
After talking off-list about this, we do want to fall back to the
tcp_ping if the session is not logged in. For discovery no session is
needed. So even without a login, it might still be reachable via ping
and a discovery possible.

>> +	}
>> +    }
>> +    # check portal via tcp
>>      my ($server, $port) = PVE::Tools::parse_host_and_port($portal);
>>      return 0 if !$server;
>>      return PVE::Network::tcp_ping($server, $port || 3260, 2);
>> @@ -97,13 +120,13 @@ sub iscsi_portals {
>>  }
>>  
>>  sub iscsi_discovery {
>> -    my ($portals) = @_;
>> +    my ($target, $portals, $cache) = @_;
> 
> There is already a $target inside the deeply-nested `if`. To avoid
> confusion, it would be better to avoid variable shadowing. Probably the
> top-level one could be renamed to $discovery_target or $target_in or
> similar.
> 
>>  
>>      assert_iscsi_support();
>>  
>>      my $res = {};
>>      for my $portal ($portals->@*) {
>> -	next if !iscsi_test_portal($portal); # fixme: raise exception here?
>> +	next if !iscsi_test_portal($target, $portal, $cache); # fixme: raise exception here?
>>  
>>  	my $cmd = [$ISCSIADM, '--mode', 'discovery', '--type', 'sendtargets', '--portal', $portal];
>>  	eval {
>> @@ -127,11 +150,11 @@ sub iscsi_discovery {
>>  }
>>  
>>  sub iscsi_login {
>> -    my ($target, $portals) = @_;
>> +    my ($target, $portals, $cache) = @_;
>>  
>>      assert_iscsi_support();
>>  
>> -    eval { iscsi_discovery($portals); };
>> +    eval { iscsi_discovery($target, $portals, $cache); };
>>      warn $@ if $@;
>>  
>>      # Disable retries to avoid blocking pvestatd for too long, next iteration will retry anyway
>> @@ -445,7 +468,7 @@ sub activate_storage {
>>      }
>>  
>>      if ($do_login) {
>> -	eval { iscsi_login($scfg->{target}, $portals); };
>> +	eval { iscsi_login($scfg->{target}, $portals, $cache); };
>>  	warn $@ if $@;
>>      } else {
>>  	# make sure we get all devices
>> @@ -559,11 +582,11 @@ sub activate_volume {
>>  
>>  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($portal);
>> +	my $result = iscsi_test_portal($scfg->{target}, $portal, $cache);
>>  	return $result if $result;
>>      }
>>  
>> -- 
>> 2.43.0
>>
>>
> 
> 
> [1] https://pve.proxmox.com/wiki/Perl_Style_Guide#Matching_Digits_Fallacy
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
I think with Friedrich's suggested changes it should to be good to merge.


[0] https://linux.die.net/man/8/iscsiadm
[1] https://www.kernel.org/doc/Documentation/filesystems/sysfs.txt


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