From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <f.ebner@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 6BA6499235 for <pve-devel@lists.proxmox.com>; Tue, 10 Oct 2023 14:55:06 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 4768032F97 for <pve-devel@lists.proxmox.com>; Tue, 10 Oct 2023 14:54:36 +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>; Tue, 10 Oct 2023 14:54:34 +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 8EEAE446C3; Tue, 10 Oct 2023 14:54:34 +0200 (CEST) Message-ID: <46997106-a66c-cc16-15b8-a72d2fe7dac5@proxmox.com> Date: Tue, 10 Oct 2023 14:54:33 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.15.1 Content-Language: en-US To: ykonotopov@gnome.org, pve-devel@lists.proxmox.com References: <20230816115627.59681-1-ykonotopov@gnome.org> <20230816115627.59681-2-ykonotopov@gnome.org> From: Fiona Ebner <f.ebner@proxmox.com> In-Reply-To: <20230816115627.59681-2-ykonotopov@gnome.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 1.585 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 NICE_REPLY_A -3.339 Looks like a legit reply (A) 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. [proxmox.com, iscsiplugin.pm, plugin.pm] Subject: Re: [pve-devel] [PATCH storage 1/1] iscsi: allow to configure multiple portals 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: Tue, 10 Oct 2023 12:55:06 -0000 Hi, thank you for the contribution! Please prepend the commit title with "fix #254: ". Am 16.08.23 um 13:56 schrieb ykonotopov@gnome.org: > From: Yuri Konotopov <ykonotopov@gnome.org> > > Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=254 To accept contributions, we need your Signed-off-by trailer here and you need to agree to the Harmony CLA (assuming you haven't sent it to us already): https://pve.proxmox.com/wiki/Developer_Documentation#Software_License_and_Copyright > --- > PVE/Storage/ISCSIPlugin.pm | 44 +++++++++++++++++++++++--------------- > PVE/Storage/Plugin.pm | 18 ++++++++++++++++ > 2 files changed, 45 insertions(+), 17 deletions(-) > > diff --git a/PVE/Storage/ISCSIPlugin.pm b/PVE/Storage/ISCSIPlugin.pm > index a79fcb0..e056393 100644 > --- a/PVE/Storage/ISCSIPlugin.pm > +++ b/PVE/Storage/ISCSIPlugin.pm > @@ -69,24 +69,30 @@ sub iscsi_test_portal { > } > > sub iscsi_discovery { > - my ($portal) = @_; > + my @portals = @{$_[0]}; Style nit: Usually we'd still write this as my ($portals) = @_; and then use $portals->@* whenever we need to access the array below. Like that it's nicer to extend if a new parameter is added. > > check_iscsi_support (); > > my $res = {}; > - return $res if !iscsi_test_portal($portal); # fixme: raise exception here? > + foreach my $portal (@portals) > + { Style nit: We use 'for' instead of 'foreach' for new code and the opening bracket should be on the same line > + next if !iscsi_test_portal($portal); # fixme: raise exception here? > > - my $cmd = [$ISCSIADM, '--mode', 'discovery', '--type', 'sendtargets', '--portal', $portal]; > - 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 =~ 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 ($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; > + } > + }); > + > + # In case of multipath we want to exit on any portal available> + last; > + } > > return $res; > } > @@ -96,7 +102,7 @@ sub iscsi_login { > > check_iscsi_support(); > > - eval { iscsi_discovery($portal_in); }; > + eval { iscsi_discovery(PVE::Storage::Plugin::get_portals($portal_in)); }; > warn $@ if $@; > > run_command([$ISCSIADM, '--mode', 'node', '--targetname', $target, '--login']); > @@ -245,8 +251,8 @@ sub properties { > type => 'string', > }, > portal => { > - description => "iSCSI portal (IP or DNS name with optional port).", > - type => 'string', format => 'pve-storage-portal-dns', > + description => "iSCSI portal (IP or DNS name with optional port). Multiple portals can be separated by comma (for multipath).", Since separation for lists is (mostly) consistent in our API, you could also just write: "For multipath, multiple portals can be specified." > + type => 'string', format => 'pve-storage-portals-dns', Please note that for lists, you can just use the existing format and append "-list" in our schemas, i.e. using 'pve-storage-portal-dns-list' should give you what you want already and you don't need to register a new format. > }, > }; > } > @@ -403,8 +409,12 @@ sub deactivate_storage { > sub check_connection { > my ($class, $storeid, $scfg) = @_; > > - my $portal = $scfg->{portal}; > - return iscsi_test_portal($portal); > + foreach my $portal (@{PVE::Storage::Plugin::get_portals($scfg->{portal})}) { > + my $result = iscsi_test_portal($portal); > + return $result if $result; > + } > + > + return 0; > } > > sub volume_resize { > diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm > index c323085..ee69b14 100644 > --- a/PVE/Storage/Plugin.pm > +++ b/PVE/Storage/Plugin.pm > @@ -205,6 +205,13 @@ sub content_hash_to_string { > return join(',', @cta); > } > > +sub get_portals { > + my $portal_cfg = shift; > + my $portals = [split(',', $portal_cfg)]; > + map { s/^\s+|\s+$//g; } @{$portals}; You can use the PVE::Tools::split_list() helper instead of this one. > + return $portals; > +} > + > sub valid_content_types { > my ($type) = @_; > > @@ -301,6 +308,17 @@ sub verify_portal_dns { > return $portal; > } > > +PVE::JSONSchema::register_format('pve-storage-portals-dns', \&verify_portals_dns); > +sub verify_portals_dns { > + my ($portal_in, $noerr) = @_; > + > + foreach my $portal (@{get_portals($portal_in)}) { > + verify_portal_dns($portal, $noerr); > + } > + > + return $portal_in; > +} > + > PVE::JSONSchema::register_format('pve-storage-content', \&verify_content); > sub verify_content { > my ($ct, $noerr) = @_; Best Regards, Fiona