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

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

[Victor Seva via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 7747 bytes --]

From: Victor Seva <linuxmaniac@torreviejawireless.org>
To: pve-devel@lists.proxmox.com
Subject: [PATCH storage v4] fix #957 iscsi: improve iscsi_test_portal logic
Date: Sat, 15 Mar 2025 13:08:55 +0100
Message-ID: <20250315120854.95412-2-linuxmaniac@torreviejawireless.org>

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  

 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 ]);
 }
 
 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+$/) {
+	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;
+}
+
+sub iscsi_test_portal {
+    my ($target, $portal, $cache) = @_;
+    $cache //= {};
+
+    if (defined($target)) {
+	# 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});
+	}
+    }
+    # 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) = @_;
 
     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



[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

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

[Friedrich Weber]

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].

> +	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)?

> +	}
> +    }
> +    # 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

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

[Mira Limbeck]

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

Re: [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


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