[PATCH v2 storage 1/1] fix #254: iscsi: allow to configure multiple portals
Yuri Konotopov
ykonotopov at gnome.org
Mon Oct 16 18:08:40 CEST 2023
16.10.2023 17:58, Dominik Csapak пишет:
> hi,
Hi, Dominik!
Thanks for your review. I will try to address all issues.
Some comments are below.
>
> a few high level comments:
>
> first:
>
> is the iscsi.cfg really necessary? couldn't we just lookup on demand
> like we do now?
Until now I thought it is. For some reason I thought that open-iscsi
node list is not
persistent between reboots. However it's indeed can be done without
custom cache
file. I will rewrite that part. Thanks!
> second:
>
> i would favor an approach that uses an 'array' instead of the '-list'
> types.
> i'm not completely sure if one can just have that as a drop in
> replacement
> with old configs/api calls still working. if not, we could introduce
> a new 'portals' api parameter/storage config that is an array which could
> take precedence over the old 'portal' one. (that we could drop with the
> next major release) in that case we could even automatically convert from
> portal -> portals if we detect that in the api update
I'm not familiar with Proxmox formats but will try to investigate it.
>
> and a few (basic) comments inline:
>
> On 10/15/23 20:32, ykonotopov at gnome.org wrote:
>> From: Yuri Konotopov <ykonotopov at gnome.org>
>>
>
> it would be great to have a real commit message here. best would be a
> short description
> of what the patch wants to achieve, and maybe some more tricky
> implementation detail
> that is not obvious (most of what you wrote in the cover letter would
> be fine, sans
> the changelog + mention of 'Here is v2 patch'
I will add it. Git's send-email interface is not something I work often :-)
>
>> Signed-off-by: Yuri Konotopov <ykonotopov at gnome.org>
>> ---
>> PVE/API2/Storage/Scan.pm | 2 +-
>> PVE/Storage.pm | 10 +-
>> PVE/Storage/ISCSIPlugin.pm | 207 ++++++++++++++++++++++++++++++++-----
>> 3 files changed, 187 insertions(+), 32 deletions(-)
>>
>> diff --git a/PVE/API2/Storage/Scan.pm b/PVE/API2/Storage/Scan.pm
>> index d7a8743..1f9773c 100644
>> --- a/PVE/API2/Storage/Scan.pm
>> +++ b/PVE/API2/Storage/Scan.pm
>> @@ -305,7 +305,7 @@ __PACKAGE__->register_method({
>> node => get_standard_option('pve-node'),
>> portal => {
>> description => "The iSCSI portal (IP or DNS name with
>> optional port).",
>> - type => 'string', format => 'pve-storage-portal-dns',
>> + type => 'string', format => 'pve-storage-portal-dns-list',
>> },
>> },
>> },
>> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
>> index cec3996..0043507 100755
>> --- a/PVE/Storage.pm
>> +++ b/PVE/Storage.pm
>> @@ -1428,12 +1428,14 @@ sub resolv_portal {
>> sub scan_iscsi {
>> my ($portal_in) = @_;
>> - my $portal;
>> - if (!($portal = resolv_portal($portal_in))) {
>> - die "unable to parse/resolve portal address '${portal_in}'\n";
>> + my @portals = PVE::Tools::split_list($portal_in);
>
> not a big deal, but we usually pack that into a reference immediately:
>
> my $portals = [ <...>::split_list(...) ];
>
> then you can do
> for my $portal (@$portals)
> or
> for my $portal ($portals->@*)
>
> and don't have to create a reference later on when passing to
> iscsi_discovery
>
>> + for my $portal (@portals) {
>> + if (!resolv_portal($portal)) {
>> + die "unable to parse/resolve portal address '${portal}'\n";
>> + }
>> }
>> - return PVE::Storage::ISCSIPlugin::iscsi_discovery($portal);
>> + return PVE::Storage::ISCSIPlugin::iscsi_discovery(\@portals);
>> }
>> sub storage_default_format {
>> diff --git a/PVE/Storage/ISCSIPlugin.pm b/PVE/Storage/ISCSIPlugin.pm
>> index a79fcb0..6f4309f 100644
>> --- a/PVE/Storage/ISCSIPlugin.pm
>> +++ b/PVE/Storage/ISCSIPlugin.pm
>> @@ -18,6 +18,65 @@ use base qw(PVE::Storage::Plugin);
>> my $ISCSIADM = '/usr/bin/iscsiadm';
>> $ISCSIADM = undef if ! -X $ISCSIADM;
>> +my $iscsi_cfg = "/etc/pve/iscsi.cfg";
>> +
>> +sub read_config {
>> + my ($filename, $raw) = @_;
>> +
>> + my $cfg = {};
>> +
>> + return $cfg if ! -f $iscsi_cfg;
>> +
>> + my $content = PVE::Tools::file_get_contents($iscsi_cfg);
>> + return $cfg if !defined($content);
>> +
>> + my @lines = split /\n/, $content;
>> +
>> + my $target;
>> +
>> + for my $line (@lines) {
>> + $line =~ s/#.*$//;
>> + $line =~ s/^\s+//;
>> + $line =~ s/^;.*$//;
>> + $line =~ s/\s+$//;
>> + next if !$line;
>> +
>> + $target = $1 if $line =~ m/^\[(\S+)\]$/;
>> + if (!$target) {
>> + warn "no target - skip: $line\n";
>> + next;
>> + }
>> +
>> + if (!defined($cfg->{$target})) {
>> + $cfg->{$target} = [];
>> + }
>> +
>> + if ($line =~ m/^((?:$IPV4RE|\[$IPV6RE\]):\d+)$/) {
>> + push @{$cfg->{$target}}, $1;
>> + }
>> + }
>> +
>> + return $cfg;
>> +}
>> +
>> +sub write_config {
>> + my ($cfg) = @_;
>> +
>> + my $out = '';
>> +
>> + for my $target (sort keys %$cfg) {
>> + $out .= "[$target]\n";
>> + for my $portal (sort @{$cfg->{$target}}) {
>> + $out .= "$portal\n";
>> + }
>> + $out .= "\n";
>> + }
>> +
>> + PVE::Tools::file_set_contents($iscsi_cfg, $out);
>> +
>> + return;
>> +}
>
> didn't look too closely at read/write_config since i'm conviced that
> we don't
> actually need this
>
>> +
>> sub check_iscsi_support {
>> my $noerr = shift;
>> @@ -45,11 +104,10 @@ 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);
>> + if ($line =~
>> m/^tcp:\s+\[(\S+)\]\s+((?:$IPV4RE|\[$IPV6RE\]):\d+)\,\S+\s+(\S+)(\s+\S+)?\s*$/)
>> {
>
> at this point, i would love to have an example output line in a
> comment sot that we can see what
> we want to parse...
>
>> + my ($session, $portal, $target) = ($1, $2, $3);
>> # there can be several sessions per target (multipath)
>> - push @{$res->{$target}}, $session;
>> + push @{$res->{$target}}, [$session, $portal];
>
> instead of using a list here, you could use a hash again, so that later
> instead of $res->{$target}->[0] you could use $res->{$target}->{session}
> this makes the code more readable (otherwise one always has to
> lookup/remember what is
> in ->[0])
>
>> }
>> });
>> };
>> @@ -68,42 +126,68 @@ sub iscsi_test_portal {
>> return PVE::Network::tcp_ping($server, $port || 3260, 2);
>> }
>> -sub iscsi_discovery {
>> - my ($portal) = @_;
>> +sub iscsi_portals {
>> + my ($target) = @_;
>> check_iscsi_support ();
>> - my $res = {};
>> - return $res if !iscsi_test_portal($portal); # fixme: raise
>> exception here?
>> -
>> - my $cmd = [$ISCSIADM, '--mode', 'discovery', '--type',
>> 'sendtargets', '--portal', $portal];
>> + my $res = [];
>> + my $cmd = [$ISCSIADM, '--mode', 'node'];
>> run_command($cmd, outfunc => sub {
>> my $line = shift;
>> if ($line =~
>> m/^((?:$IPV4RE|\[$IPV6RE\]):\d+)\,\S+\s+(\S+)\s*$/) {
>
> is this regex...
>
>> - my $portal = $1;
>> - my $target = $2;
>> - # one target can have more than one portal (multipath).
>> - push @{$res->{$target}}, $portal;
>> + my ($portal, $portal_target) = ($1, $2);
>> + if ($portal_target eq $target) {
>> + push @{$res}, $portal;
>> + }
>> }
>> });
>> 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];
>> + run_command($cmd, outfunc => sub {
>> + my $line = shift;
>> +
>> + if ($line =~
>> m/^((?:$IPV4RE|\[$IPV6RE\]):\d+)\,\S+\s+(\S+)\s*$/) {
>
> and this the same? maybe this can be factored out?
Yes, the output of commands is same, but commands are different (one is
online and another is offline).
>
>> + 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
>
> isn't that line contradictory to the previous comment?
> we gather all portals that is resolved by any (the first?) portal, but
> no more?
No, there is no contradiction here.
There may be multiple portals for single target, but any portal is
suitable for
discovery.
> if we just need one, why do we push them into an list in the first place?
It looks like the return value of iscsi_discovery is not used. So we can
drop
$res completely and regex too.
>
>> + last;
>> + }
>> +
>> + 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 +217,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->@[0], '--rescan'];
>
> didn't know this was valid perl syntax, normally lists are accessed
> like this:
>
> $session->[0]
>
> (but as i said above, maybe we want to use a hash instead to have
> $session->{session} ?)
>
>> eval { run_command($cmd, outfunc => sub {}); };
>> warn $@ if $@;
>> }
>> @@ -245,8 +329,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). For multipath, multiple portals can be specified.",
>> + type => 'string', format => 'pve-storage-portal-dns-list',
>> },
>> };
>> }
>> @@ -264,6 +348,33 @@ sub options {
>> # Storage implementation
>> +sub on_add_hook {
>> + my ($class, $storeid, $scfg, %param) = @_;
>> +
>> + my @portals = PVE::Tools::split_list($scfg->{portal});
>> + my $target = $scfg->{target};
>> +
>> + my $portal_cfg = read_config();
>> + $portal_cfg->{$target} = \@portals;
>> +
>> + write_config($portal_cfg);
>> +
>> + return;
>> +}
>> +
>> +sub on_delete_hook {
>> + my ($class, $storeid, $scfg) = @_;
>> +
>> + my $portal_cfg = read_config();
>> + my $target = $scfg->{target};
>> +
>> + delete $portal_cfg->{$target};
>> +
>> + write_config($portal_cfg);
>> +
>> + return;
>> +}
>> +
>> sub parse_volname {
>> my ($class, $volname) = @_;
>> @@ -365,6 +476,21 @@ sub iscsi_session {
>> return $cache->{iscsi_sessions}->{$target};
>> }
>> +sub iscsi_session_portals {
>> + my ($cache, $target) = @_;
>> +
>> + my $res = [];
>> + my $sessions = iscsi_session($cache, $target);
>> +
>> + if (defined($sessions)) {
>> + for my $session ($sessions->@*) {
>> + push @{$res}, $session->@[1];
>> + }
>> + }
>
> this could probably be
>
> push @{$res}, $_->[1] for $sessions->@* if defined($sessions);
> or
> my $res = [ map { $_->[1] } (@$sessions // ())];
>
>
> (neither tested though)
>
>> +
>> + return $res;
>> +}
>> +
>> sub status {
>> my ($class, $storeid, $scfg, $cache) = @_;
>> @@ -379,14 +505,37 @@ sub activate_storage {
>> return if !check_iscsi_support(1);
>> - my $session = iscsi_session($cache, $scfg->{target});
>> - if (!defined ($session)) {
>> - eval { iscsi_login($scfg->{target}, $scfg->{portal}); };
>> + my $sessions = iscsi_session($cache, $scfg->{target});
>> + my $portal_cfg = read_config();
>> +
>> + my @portals = PVE::Tools::split_list($scfg->{portal});
>> + if (defined($portal_cfg->{$scfg->{target}})) {
>> + @portals = @{$portal_cfg->{$scfg->{target}}};
>> + }
>> +
>> + if (!defined ($sessions)) {
>> + eval { iscsi_login($scfg->{target}, \@portals); };
>> warn $@ if $@;
>> } else {
>> + my @discovered_portals = @{iscsi_portals($scfg->{target})};
>> + my @session_portals = @{iscsi_session_portals($cache,
>> $scfg->{target})};
>
> do i understand this right that you reuse the saved portals only for
> fresh
> sessions? since here you discover them again if there is already a
> session?
The iscsi_portrals function is not actually discover but rather
retrieval of last discovered
portals (during iscsi_discover).
Before I thought that we can extract discovered portals from open-iscsi
only when we
have active session, but we can do it without sessions too.
>
>> +
>> + for my $discovered_portal (@discovered_portals) {
>> + if (!grep(/^\Q$discovered_portal\E$/, @session_portals)) {
>> + eval { iscsi_login($scfg->{target}, \@discovered_portals); };
>
> isn't that already done via the iscsi_login from above when there is
> no session?
In case some portals were unavailable during iscsi_login we will not
have all sessions.
So we need to check that and login again when sessions are missing.
Otherwise we will
miss multipath.
>
>
>> + warn $@ if $@;
>> + last;
>> + }
>> + }
>> +
>> + if(join(",", sort(@discovered_portals)) ne join(",",
>> sort(@portals))) {
>> + $portal_cfg->{$scfg->{target}} = \@discovered_portals;
>> + write_config($portal_cfg);
>> + }
>> +
>> # make sure we get all devices
>> - iscsi_session_rescan($session);
>> + iscsi_session_rescan($sessions);
>> }
>> }
>> @@ -396,15 +545,19 @@ 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);
>> + for my $portal (PVE::Tools::split_list($scfg->{portal})) {
>> + my $result = iscsi_test_portal($portal);
>> + return $result if $result;
>> + }
>> +
>
> here you don't use the cache config at all...
>
>> + return 0;
>> }
>> sub volume_resize {
>
>
--
Best regards, Yuri Konotopov
More information about the pve-devel
mailing list