[PATCH pve-storage 3/9] luncmd: lio: remove rest of old parsing logic

["Max R. Carrara" <m.carrara@proxmox.com>]

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 <m.carrara@proxmox.com>
---
 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(?<tpg_tag>[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/^(?<index>[0-9]+)$/;
+                my $index = $+{index};
+
+                $lun->{storage_object} =~ m|^(?<storage_object>$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