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

[Friedrich Weber]

On 20/03/2025 11:15, Mira Limbeck wrote:
> [...]
>>
>>> +	# 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.

Yeah, let's fall back to a tcp_ping if there is a session that is not
LOGGED_IN. Sorry for the confusion with my earlier suggestion on v3.

One minor thing I forgot -- the commit message might benefit from a few
more details:

- mention that when we test connectivity of a portal, we now first check
whether a logged-in session to that portal is already present and fall
back to the TCP ping only if there is no such session
- acknowledge that this is not going to remove TCP pings (and thus the
log messages on the target side) completely -- TCP pings are still done
e.g. if there is no active session yet.

And finally, the first line is missing a colon after "fix #957", but
this is a really minor thing now -- we can also fix when applying, just
mentioning it here for completeness.

@Victor, if you send a v5 I'll do some final testing on that version
and, if everything looks good, add my Tested-by/Reviewed-by trailers there.

[1]
https://pve.proxmox.com/wiki/Developer_Documentation#Commits_and_Commit_Messages