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

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

[Victor Seva via pve-devel]

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

From: Victor Seva <linuxmaniac@torreviejawireless.org>
To: pve-devel@lists.proxmox.com
Subject: [PATCH] fix #957 iscsi: don't check tcp connection directly
Date: Wed,  5 Mar 2025 22:59:37 +0100
Message-ID: <20250305215937.53650-1-linuxmaniac@torreviejawireless.org>

pvestatd is checking this every 10 seconds. This check
produces a lot of noise at the iscsi server logging.

Use iscsiadm command to check the sessions status instead

Signed-off-by: Victor Seva <linuxmaniac@torreviejawireless.org>
---
 src/PVE/Storage/ISCSIPlugin.pm | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/src/PVE/Storage/ISCSIPlugin.pm b/src/PVE/Storage/ISCSIPlugin.pm
index eb70453..6a69653 100644
--- a/src/PVE/Storage/ISCSIPlugin.pm
+++ b/src/PVE/Storage/ISCSIPlugin.pm
@@ -66,6 +66,25 @@ sub iscsi_test_portal {
     return PVE::Network::tcp_ping($server, $port || 3260, 2);
 }
 
+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;
+}
+
 sub iscsi_portals {
     my ($target, $portal_in) = @_;
 
@@ -561,10 +580,13 @@ sub check_connection {
     my ($class, $storeid, $scfg) = @_;
 
     my $portals = iscsi_portals($scfg->{target}, $scfg->{portal});
-
+    my $cache = {};
     for my $portal (@$portals) {
-	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;
+        }
     }
 
     return 0;
-- 
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] fix #957 iscsi: don't check tcp connection directly

[Mira Limbeck]

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.

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

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

[Mira Limbeck]

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