From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <d.csapak@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits))
(No client certificate requested)
by lists.proxmox.com (Postfix) with ESMTPS id BAB609BAC1
for <pve-devel@lists.proxmox.com>; Fri, 20 Oct 2023 11:23:32 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 9CD9731A9F
for <pve-devel@lists.proxmox.com>; Fri, 20 Oct 2023 11:23:32 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
[94.136.29.106])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits))
(No client certificate requested)
by firstgate.proxmox.com (Proxmox) with ESMTPS
for <pve-devel@lists.proxmox.com>; Fri, 20 Oct 2023 11:23:30 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 3296D435A5;
Fri, 20 Oct 2023 11:23:30 +0200 (CEST)
Message-ID: <f883722d-341d-4946-9420-314578b3a8bb@proxmox.com>
Date: Fri, 20 Oct 2023 11:23:28 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Content-Language: en-US
To: Yuri Konotopov <ykonotopov@gnome.org>, pve-devel@lists.proxmox.com
References: <20231017161948.139795-1-ykonotopov@gnome.org>
<20231017161948.139795-2-ykonotopov@gnome.org>
From: Dominik Csapak <d.csapak@proxmox.com>
In-Reply-To: <20231017161948.139795-2-ykonotopov@gnome.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.012 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
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]
Subject: Re: [pve-devel] [PATCH v3 storage 1/1] fix #254: iscsi: add support
for multipath iscsi targets
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>
X-List-Received-Date: Fri, 20 Oct 2023 09:23:32 -0000
hi,
nice elegant solution for not needing to change the config
or introducing a new file :)
there is one caveats with this solution, but that should not
matter for most situations:
in case the configured portal is not accessible, any new node
in the cluster (or one that did not activate the storage until then)
will not be able to activate that storage
(without manually doing a discovery on another portal)
i don't think this is a huge problem though, the patch
still makes the situation a whole lot better
the patch looks mostly fine, some comments inline
On 10/17/23 18:19, Yuri Konotopov wrote:
> With this patch Proxmox now tries to login to all discovered portals in
> case some of them are not logged yet.
> In case of multipath configuration when initially configured portal is
> missing for some reason Proxmox don't lose iscsi storage now and can
> succesfully restore iscsi connection between reboots.
>
> Signed-off-by: Yuri Konotopov <ykonotopov@gnome.org>
> ---
> PVE/Storage.pm | 2 +-
> PVE/Storage/ISCSIPlugin.pm | 117 ++++++++++++++++++++++++++++---------
> 2 files changed, 89 insertions(+), 30 deletions(-)
>
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index cec3996..87755ac 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -1433,7 +1433,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([ $portal ]);
> }
>
> sub storage_default_format {
> diff --git a/PVE/Storage/ISCSIPlugin.pm b/PVE/Storage/ISCSIPlugin.pm
> index a79fcb0..1d47f3a 100644
> --- a/PVE/Storage/ISCSIPlugin.pm
> +++ b/PVE/Storage/ISCSIPlugin.pm
> @@ -18,6 +18,9 @@ use base qw(PVE::Storage::Plugin);
> my $ISCSIADM = '/usr/bin/iscsiadm';
> $ISCSIADM = undef if ! -X $ISCSIADM;
>
> +# Example: 192.168.122.252:3260,1 iqn.2003-01.org.linux-iscsi.proxmox-nfs.x8664:sn.00567885ba8f
> +my $ISCSI_TARGET_RE = qr/^((?:$IPV4RE|\[$IPV6RE\]):\d+)\,\S+\s+(\S+)\s*$/m;
the '/m' here is unnecessary since we only match single lines anyway
> +
> sub check_iscsi_support {
> my $noerr = shift;
>
> @@ -45,11 +48,12 @@ sub iscsi_session_list {
> eval {
> run_command($cmd, errmsg => 'iscsi session scan failed', outfunc => sub {
> my $line = shift;
> -
> - if ($line =~ m/^tcp:\s+\[(\S+)\]\s+\S+\s+(\S+)(\s+\S+)?\s*$/) {
> - my ($session, $target) = ($1, $2);
> + # example: tcp: [1] 192.168.122.252:3260,1 iqn.2003-01.org.linux-iscsi.proxmox-nfs.x8664:sn.00567885ba8f (non-flash)
> + if ($line =~ m/^tcp:\s+\[(\S+)\]\s+((?:$IPV4RE|\[$IPV6RE\]):\d+)\,\S+\s+(\S+)(\s+\S+)?\s*$/) {
nit: the last capture group is not really necessary AFAICS (since we don't use it)
> + my ($session_id, $portal, $target) = ($1, $2, $3);
> # there can be several sessions per target (multipath)
> - push @{$res->{$target}}, $session;
> + my %session = ( session_id => $session_id, portal => $portal );
> + push @{$res->{$target}}, \%session;
> }
> });
> };
> @@ -68,42 +72,77 @@ sub iscsi_test_portal {
> return PVE::Network::tcp_ping($server, $port || 3260, 2);
> }
>
> -sub iscsi_discovery {
> - my ($portal) = @_;
> +sub iscsi_portals {
> + my ($target, $portal_in) = @_;
>
> check_iscsi_support ();
>
> - my $res = {};
> - return $res if !iscsi_test_portal($portal); # fixme: raise exception here?
> + my $res = [];
> + my $cmd = [$ISCSIADM, '--mode', 'node'];
> + eval {
> + run_command($cmd, outfunc => sub {
> + my $line = shift;
>
> - my $cmd = [$ISCSIADM, '--mode', 'discovery', '--type', 'sendtargets', '--portal', $portal];
> - run_command($cmd, outfunc => sub {
> - my $line = shift;
> + if ($line =~ $ISCSI_TARGET_RE) {
> + my ($portal, $portal_target) = ($1, $2);
> + if ($portal_target eq $target) {
> + push @{$res}, $portal;
> + }
> + }
> + });
> + };
>
> - if ($line =~ m/^((?:$IPV4RE|\[$IPV6RE\]):\d+)\,\S+\s+(\S+)\s*$/) {
> - my $portal = $1;
> - my $target = $2;
> - # one target can have more than one portal (multipath).
> - push @{$res->{$target}}, $portal;
> - }
> - });
> + if ($@) {
> + warn $@;
> + return [ $portal_in ];
> + }
> +
> + return $res;
> +}
> +
> +sub iscsi_discovery {
> + my ($portals) = @_;
> +
> + check_iscsi_support ();
> +
> + my $res = {};
> + for my $portal ($portals->@*) {
> + next if !iscsi_test_portal($portal); # fixme: raise exception here?
> +
> + my $cmd = [$ISCSIADM, '--mode', 'discovery', '--type', 'sendtargets', '--portal', $portal];
> + eval {
> + run_command($cmd, outfunc => sub {
> + my $line = shift;
> +
> + if ($line =~ $ISCSI_TARGET_RE) {
> + my ($portal, $target) = ($1, $2);
> + # one target can have more than one portal (multipath)
> + # and sendtargets should return all of them in single call
> + push @{$res->{$target}}, $portal;
> + }
> + });
> + };
> +
> + # In case of multipath we want to exit on any portal available
> + last if !$@;
i don't really like this, because if the discovery output did not include any line
that we could parse as a target, it still exits the loop
i'd rather check if the $res variable has picked up any targets, since
we need at least one?
e.g. something like this:
last if scalar(keys %$res) > 0;
that way we ensure that we have at least one target or we go to the next portal
does that make sense?
> + }
>
> return $res;
> }
>
> sub iscsi_login {
> - my ($target, $portal_in) = @_;
> + my ($target, $portals) = @_;
>
> check_iscsi_support();
>
> - eval { iscsi_discovery($portal_in); };
> + eval { iscsi_discovery($portals); };
> warn $@ if $@;
>
> run_command([$ISCSIADM, '--mode', 'node', '--targetname', $target, '--login']);
> }
>
> sub iscsi_logout {
> - my ($target, $portal) = @_;
> + my ($target) = @_;
>
> check_iscsi_support();
>
> @@ -133,7 +172,7 @@ sub iscsi_session_rescan {
> }
>
> foreach my $session (@$session_list) {
> - my $cmd = [$ISCSIADM, '--mode', 'session', '--sid', $session, '--rescan'];
> + my $cmd = [$ISCSIADM, '--mode', 'session', '--sid', $session->{session_id}, '--rescan'];
> eval { run_command($cmd, outfunc => sub {}); };
> warn $@ if $@;
> }
> @@ -379,14 +418,28 @@ sub activate_storage {
>
> return if !check_iscsi_support(1);
>
> - my $session = iscsi_session($cache, $scfg->{target});
> + my $sessions = iscsi_session($cache, $scfg->{target});
> + my $portals = iscsi_portals($scfg->{target}, $scfg->{portal});
> + my $do_login = !defined($sessions);
>
> - if (!defined ($session)) {
> - eval { iscsi_login($scfg->{target}, $scfg->{portal}); };
> + if (!$do_login) {
> + # We should check that sessions for all portals are available
> + my $session_portals = [ map { $_->{portal} } (@$sessions) ];
> +
> + for my $portal (@$portals) {
> + if (!grep(/^\Q$portal\E$/, @$session_portals)) {
> + $do_login = 1;
> + last;
> + }
> + }
> + }
> +
> + if ($do_login) {
> + eval { iscsi_login($scfg->{target}, $portals); };
> warn $@ if $@;
> } else {
> # make sure we get all devices
> - iscsi_session_rescan($session);
> + iscsi_session_rescan($sessions);
> }
> }
>
> @@ -396,15 +449,21 @@ sub deactivate_storage {
> return if !check_iscsi_support(1);
>
> if (defined(iscsi_session($cache, $scfg->{target}))) {
> - iscsi_logout($scfg->{target}, $scfg->{portal});
> + iscsi_logout($scfg->{target});
> }
> }
>
> sub check_connection {
> my ($class, $storeid, $scfg) = @_;
>
> - my $portal = $scfg->{portal};
> - return iscsi_test_portal($portal);
> + my $portals = iscsi_portals($scfg->{target}, $scfg->{portal});
> +
> + for my $portal (@$portals) {
> + my $result = iscsi_test_portal($portal);
> + return $result if $result;
> + }
> +
> + return 0;
> }
>
> sub volume_resize {