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 AE9EB1FF176
for <inbox@lore.proxmox.com>; Fri, 7 Mar 2025 13:00:12 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id ECCAF1A0C7;
Fri, 7 Mar 2025 13:00:05 +0100 (CET)
Message-ID: <17b91ea7-58fb-4c81-a40b-780a3039749a@proxmox.com>
Date: Fri, 7 Mar 2025 12:59:32 +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>
Content-Language: en-US
From: Mira Limbeck <m.limbeck@proxmox.com>
In-Reply-To: <mailman.800.1741212404.293.pve-devel@lists.proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.302 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
RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
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>
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