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 [IPv6:2a01:7e0:0:424::9])
by lore.proxmox.com (Postfix) with ESMTPS id DCE511FF168
for <inbox@lore.proxmox.com>; Tue, 18 Mar 2025 17:12:00 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 835163306F;
Tue, 18 Mar 2025 17:11:48 +0100 (CET)
Message-ID: <833ae8dd-6eeb-4983-9358-8a2a59e96807@proxmox.com>
Date: Tue, 18 Mar 2025 17:11:43 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
References: <mailman.51.1742117805.416.pve-devel@lists.proxmox.com>
Content-Language: en-US
From: Friedrich Weber <f.weber@proxmox.com>
In-Reply-To: <mailman.51.1742117805.416.pve-devel@lists.proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.009 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
URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See
http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more
information. [iscsiplugin.pm, storage.pm, proxmox.com]
Subject: Re: [pve-devel] [PATCH storage v4] fix #957 iscsi: improve
iscsi_test_portal logic
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>
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