From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id F1D921FF176
for <inbox@lore.proxmox.com>; Fri, 7 Mar 2025 13:03:51 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 782C81A1D9;
Fri, 7 Mar 2025 13:03:45 +0100 (CET)
Message-ID: <d1505a1b-25c2-4a87-91ae-002b6aba6d7e@proxmox.com>
Date: Fri, 7 Mar 2025 13:03:40 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
To: pve-devel@lists.proxmox.com
References: <mailman.800.1741212404.293.pve-devel@lists.proxmox.com>
<17b91ea7-58fb-4c81-a40b-780a3039749a@proxmox.com>
Content-Language: en-US
From: Mira Limbeck <m.limbeck@proxmox.com>
In-Reply-To: <17b91ea7-58fb-4c81-a40b-780a3039749a@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.300 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
Subject: Re: [pve-devel] [PATCH] fix #957 iscsi: don't check tcp connection
directly
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.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