From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 880EC1FF13C for ; Thu, 25 Jun 2026 18:41:27 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 365352D0; Thu, 25 Jun 2026 18:41:27 +0200 (CEST) From: "Max R. Carrara" To: pve-devel@lists.proxmox.com Subject: [PATCH pve-storage 3/9] luncmd: lio: remove rest of old parsing logic Date: Thu, 25 Jun 2026 18:40:55 +0200 Message-ID: <20260625164110.706694-4-m.carrara@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260625164110.706694-1-m.carrara@proxmox.com> References: <20260625164110.706694-1-m.carrara@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1782405672186 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.080 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 Message-ID-Hash: 5PS5G4BZWGQBIOJMNAFXRRW5KFP663CZ X-Message-ID-Hash: 5PS5G4BZWGQBIOJMNAFXRRW5KFP663CZ X-MailFrom: m.carrara@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Instead of saving the entire config to a package-level variable, fetch and parse the remote config in the LUN command dispatching sub (serving as an entry point for in each iSCSI provider) and pass it along wherever needed. Do this by taking the opportunity to start modernizing the code in this package, introducing helpers for fetching and parsing the remote config, as well as for running remote commands in general. The new `run_remote_command()` function in particular will be used in future commits for better error reporting, too. Signed-off-by: Max R. Carrara --- src/PVE/Storage/LunCmd/LIO.pm | 278 +++++++++++++++++++--------------- 1 file changed, 152 insertions(+), 126 deletions(-) diff --git a/src/PVE/Storage/LunCmd/LIO.pm b/src/PVE/Storage/LunCmd/LIO.pm index 9dd25ad2..ae5f3188 100644 --- a/src/PVE/Storage/LunCmd/LIO.pm +++ b/src/PVE/Storage/LunCmd/LIO.pm @@ -34,13 +34,138 @@ my @CONFIG_FILES = ( ); my $BACKSTORE = '/backstores/block'; -my $SETTINGS = undef; - my @ssh_opts = ('-o', 'BatchMode=yes'); my @ssh_cmd = ('/usr/bin/ssh', @ssh_opts); my $id_rsa_path = '/etc/pve/priv/zfs'; my $targetcli = '/usr/bin/targetcli'; +my sub run_remote_command { + my ($scfg, $cmd, %param) = @_; + + my $out_log = ''; + my $err_log = ''; + my $merged_log = ''; + + my $outfunc = sub { + my $out = shift; + $out_log .= $out . "\n"; + $merged_log .= $out . "\n"; + }; + + my $errfunc = sub { + my $err = shift; + $err_log .= $err . "\n"; + $merged_log .= $err . "\n"; + }; + + my $timeout = $param{timeout} || 10; + + my $ssh_key_path = $id_rsa_path . '/' . $scfg->{portal} . '_id_rsa'; + my $target = 'root@' . $scfg->{portal}; + + my $remote_cmd = [ + @ssh_cmd, '-i', $ssh_key_path, $target, '--', $cmd->@*, + ]; + + my $result = 1; + + eval { + run_command( + $remote_cmd, + outfunc => $outfunc, + errfunc => $errfunc, + timeout => $timeout, + ); + }; + if (my $err = $@) { + $result = 0; + } + + return { + 'err-log' => $err_log, + 'merged-log' => $merged_log, + 'out-log' => $out_log, + 'remote-cmd' => $remote_cmd, + result => $result, + }; +} + +my sub fetch_remote_config { + my ($scfg, $timeout) = @_; + + for my $file (@CONFIG_FILES) { + my $fetch_res = run_remote_command( + $scfg, ['cat', $file], timeout => $timeout, + ); + if (!$fetch_res->{result} && $fetch_res->{'err-log'} !~ m/no such file or directory/i) { + my $err_msg = + "Failed to fetch config from iSCSI portal $scfg->{portal}:\n" + . $fetch_res->{'err-log'} . "\n"; + die $err_msg; + } + + if (my $config = $fetch_res->{'out-log'}) { + return JSON->new->utf8->decode($config); + } + } + + die "No configuration found. Install targetcli on $scfg->{portal}\n"; +} + +my sub parse_remote_config { + my ($scfg, $remote_config_json) = @_; + + my $tpg = $scfg->{lio_tpg} || die "Target Portal Group not set, aborting!\n"; + + $tpg =~ m/^tpg(?[0-9]+)$/; + my $tpg_tag = $+{tpg_tag}; + + if (!$tpg_tag) { + die "Target Portal Group has invalid value, must contain string 'tpg'" + . " and a suffix number, for example 'tpg17'\n"; + } + + my $id = $scfg->{portal} . '.' . $scfg->{target}; + my $parsed_config = {}; + my $found_target = 0; + + for my $target ($remote_config_json->{targets}->@*) { + # We are only interested in iSCSI targets + next if $target->{fabric} ne 'iscsi'; + next if $target->{wwn} ne $scfg->{target}; + + for my $tpg ($target->{tpgs}->@*) { + next if $tpg->{tag} != $tpg_tag; + + my $luns = []; + for my $lun ($tpg->{luns}->@*) { + $lun->{index} =~ m/^(?[0-9]+)$/; + my $index = $+{index}; + + $lun->{storage_object} =~ m|^(?$BACKSTORE/.*)$|; + my $storage_object = $+{storage_object}; + + if (!(defined($index) && defined($storage_object))) { + die "Invalid LUN definition in config!\n"; + } + + push($luns->@*, { index => $index, storage_object => $storage_object }); + } + + $parsed_config->{$id}->{luns} = $luns; + + $found_target = 1; + last; + } + } + + if (!$found_target) { + die "Target Portal Group '$tpg' not found!\n"; + } + + return $parsed_config; +} + my $execute_remote_command = sub { my ($scfg, $timeout, $remote_command, @params) = @_; @@ -82,110 +207,6 @@ my $execute_remote_command = sub { return $res; }; -# fetch targetcli configuration from the portal -my $read_config = sub { - my ($scfg, $timeout) = @_; - - my $msg = ''; - my $err = undef; - my $luncmd = 'cat'; - my $target; - my $retry = 1; - - $timeout = 10 if !$timeout; - - my $output = sub { $msg .= "$_[0]\n" }; - my $errfunc = sub { $err .= "$_[0]\n" }; - - $target = 'root@' . $scfg->{portal}; - - foreach my $oneFile (@CONFIG_FILES) { - my $cmd = - [@ssh_cmd, '-i', "$id_rsa_path/$scfg->{portal}_id_rsa", $target, $luncmd, $oneFile]; - eval { - run_command($cmd, outfunc => $output, errfunc => $errfunc, timeout => $timeout); - }; - if ($@) { - die $err if ($err !~ /No such file or directory/); - } - return $msg if $msg ne ''; - } - - die "No configuration found. Install targetcli on $scfg->{portal}\n" if $msg eq ''; - - return $msg; -}; - -my $get_config = sub { - my ($scfg) = @_; - my @conf = undef; - - my $config = $read_config->($scfg, undef); - die "Missing config file" unless $config; - - return $config; -}; - -# Return settings of a specific target -my $get_target_settings = sub { - my ($scfg) = @_; - - my $id = "$scfg->{portal}.$scfg->{target}"; - return undef if !$SETTINGS; - return $SETTINGS->{$id}; -}; - -# fetches and parses targetcli config from the portal -my $parser = sub { - my ($scfg) = @_; - my $tpg = $scfg->{lio_tpg} || die "Target Portal Group not set, aborting!\n"; - my $tpg_tag; - - if ($tpg =~ /^tpg(\d+)$/) { - $tpg_tag = $1; - } else { - die - "Target Portal Group has invalid value, must contain string 'tpg' and a suffix number, eg 'tpg17'\n"; - } - - my $config = $get_config->($scfg); - my $jsonconfig = JSON->new->utf8->decode($config); - - my $haveTarget = 0; - foreach my $target (@{ $jsonconfig->{targets} }) { - # only interested in iSCSI targets - next if !($target->{fabric} eq 'iscsi' && $target->{wwn} eq $scfg->{target}); - # find correct TPG - foreach my $tpg (@{ $target->{tpgs} }) { - if ($tpg->{tag} == $tpg_tag) { - my $res = []; - foreach my $lun (@{ $tpg->{luns} }) { - my ($idx, $storage_object); - if ($lun->{index} =~ /^(\d+)$/) { - $idx = $1; - } - if ($lun->{storage_object} =~ m|^($BACKSTORE/.*)$|) { - $storage_object = $1; - } - die "Invalid lun definition in config!\n" - if !(defined($idx) && defined($storage_object)); - push @$res, { index => $idx, storage_object => $storage_object }; - } - - my $id = "$scfg->{portal}.$scfg->{target}"; - $SETTINGS->{$id}->{luns} = $res; - $haveTarget = 1; - last; - } - } - } - - # seriously unhappy if the target server lacks iSCSI target configuration ... - if (!$haveTarget) { - die "target portal group tpg$tpg_tag not found!\n"; - } -}; - # Get prefix for backstores my $get_backstore_prefix = sub { my ($scfg) = @_; @@ -196,14 +217,15 @@ my $get_backstore_prefix = sub { # extracts the ZFS volume name from a device path my $extract_volname = sub { - my ($scfg, $lunpath) = @_; + my ($scfg, $config, $lunpath) = @_; my $volname = undef; my $base = get_base($scfg); if ($lunpath =~ /^$base\/$scfg->{pool}\/([\w\-\.]+)$/) { $volname = $1; my $prefix = $get_backstore_prefix->($scfg); - my $target = $get_target_settings->($scfg); + my $id = "$scfg->{portal}.$scfg->{target}"; + my $target = $config->{$id}; foreach my $lun (@{ $target->{luns} }) { # If we have a lun with the pool prefix matching this vol, then return this one # like pool-pve-vm-100-disk-0 @@ -219,12 +241,13 @@ my $extract_volname = sub { # retrieves the LUN index for a particular object my $list_view = sub { - my ($scfg, $timeout, $method, @params) = @_; + my ($scfg, $config, $timeout, $method, @params) = @_; my $lun = undef; my $object = $params[0]; - my $volname = $extract_volname->($scfg, $object); - my $target = $get_target_settings->($scfg); + my $volname = $extract_volname->($scfg, $config, $object); + my $id = "$scfg->{portal}.$scfg->{target}"; + my $target = $config->{$id}; return undef if !defined($volname); # nothing to search for.. @@ -239,11 +262,12 @@ my $list_view = sub { # determines, if the given object exists on the portal my $list_lun = sub { - my ($scfg, $timeout, $method, @params) = @_; + my ($scfg, $config, $timeout, $method, @params) = @_; my $object = $params[0]; - my $volname = $extract_volname->($scfg, $object); - my $target = $get_target_settings->($scfg); + my $volname = $extract_volname->($scfg, $config, $object); + my $id = "$scfg->{portal}.$scfg->{target}"; + my $target = $config->{$id}; foreach my $lun (@{ $target->{luns} }) { if ($lun->{storage_object} eq "$BACKSTORE/$volname") { @@ -256,14 +280,14 @@ my $list_lun = sub { # adds a new LUN to the target my $create_lun = sub { - my ($scfg, $timeout, $method, @params) = @_; + my ($scfg, $config, $timeout, $method, @params) = @_; - if ($list_lun->($scfg, $timeout, $method, @params)) { + if ($list_lun->($scfg, $config, $timeout, $method, @params)) { die "$params[0]: LUN already exists!"; } my $device = $params[0]; - my $volname = $extract_volname->($scfg, $device); + my $volname = $extract_volname->($scfg, $config, $device); # Here we create a new device, so we didn't get the volname prefixed with the pool name # as extract_volname couldn't find a matching vol yet $volname = $get_backstore_prefix->($scfg) . $volname; @@ -303,14 +327,15 @@ my $create_lun = sub { }; my $delete_lun = sub { - my ($scfg, $timeout, $method, @params) = @_; + my ($scfg, $config, $timeout, $method, @params) = @_; my $res = { msg => undef }; my $tpg = $scfg->{lio_tpg} || die "Target Portal Group not set, aborting!\n"; my $path = $params[0]; - my $volname = $extract_volname->($scfg, $path); - my $target = $get_target_settings->($scfg); + my $volname = $extract_volname->($scfg, $config, $path); + my $id = "$scfg->{portal}.$scfg->{target}"; + my $target = $config->{$id}; foreach my $lun (@{ $target->{luns} }) { next if $lun->{storage_object} ne "$BACKSTORE/$volname"; @@ -339,20 +364,20 @@ my $delete_lun = sub { }; my $import_lun = sub { - my ($scfg, $timeout, $method, @params) = @_; + my ($scfg, $config, $timeout, $method, @params) = @_; - return $create_lun->($scfg, $timeout, $method, @params); + return $create_lun->($scfg, $config, $timeout, $method, @params); }; # needed for example when the underlying ZFS volume has been resized my $modify_lun = sub { - my ($scfg, $timeout, $method, @params) = @_; + my ($scfg, $config, $timeout, $method, @params) = @_; # Nothing to do on volume modification for LIO return undef; }; my $add_view = sub { - my ($scfg, $timeout, $method, @params) = @_; + my ($scfg, $config, $timeout, $method, @params) = @_; return ''; }; @@ -372,9 +397,10 @@ sub run_lun_command { die "unknown command '$method'" unless exists $lun_cmd_map{$method}; - $parser->($scfg); + my $remote_config_json = fetch_remote_config($scfg, $timeout); + my $config = parse_remote_config($scfg, $remote_config_json); - my $msg = $lun_cmd_map{$method}->($scfg, $timeout, $method, @params); + my $msg = $lun_cmd_map{$method}->($scfg, $config, $timeout, $method, @params); return $msg; } -- 2.47.3