From 7d727fec5bd785c29a557b919d338c7ccb40fc14 Mon Sep 17 00:00:00 2001
From: Nick Dokos <ndokos@redhat.com>
Date: Fri, 9 Jun 2023 10:48:35 -0400
Subject: [PATCH]  v0.72.0: Fix handling of labeled hosts by
 pbench-postprocess-tools (#3456)

* v0.72: Fix handling of labeled hosts by pbench-postprocess-tools

Fixes #3454

pbench-postprocess-tools mishandles hosts with labels (added by
tool registration commands): it ignores the label and complains
that it cannot find the tool output directory. The tool output
directory path contains '<label>:<host>' as one element in the path
but pbench-postprocess-tools looks for a '<host>' element.

pbench-postprocess-tools parses the output of pbench-list-tools to
get the tool info it needs, but pbench-list-tools omits the label
from its output.

This PR fixes pbench-list-tools to add the label to its output
and pbench-postprocess-tools to parse that output, derive the label
and use it to construct the path of the tool output directory.


It also adds a "functional" test for pbench-list-tools to verify
the output when labeled hosts have registered tools and
fixes an incorrect legacy test (test-07) for pbench-postprocess-tools:
the test was using a labeled host, but it was testing the wrong
tool output directory (without the label). It adds a similar legacy test
(test-07.1) to test the unlabeled host case.

Bump the version to 0.72.1

PBENCH-1178
---
 agent/VERSION                                 |  2 +-
 .../pbench-postprocess-tools/test-07.1.txt    | 25 +++++++++++
 .../gold/pbench-postprocess-tools/test-07.txt | 14 +++----
 agent/util-scripts/pbench-postprocess-tools   | 14 +++++--
 .../tools-v1-foobar/testhost.example.com/foo  |  1 +
 .../testhost.example.com/iostat               |  1 +
 agent/util-scripts/unittests                  | 12 +++++-
 lib/pbench/cli/agent/commands/tools/list.py   | 37 +++++++++-------
 .../cli/commands/tools/test_tools_list.py     | 42 +++++++++++++++++++
 9 files changed, 119 insertions(+), 29 deletions(-)
 create mode 100644 agent/util-scripts/gold/pbench-postprocess-tools/test-07.1.txt
 create mode 100644 agent/util-scripts/samples/pbench-postprocess-tools/test-07.1/pbench/tools-v1-foobar/testhost.example.com/foo
 create mode 100644 agent/util-scripts/samples/pbench-postprocess-tools/test-07.1/pbench/tools-v1-foobar/testhost.example.com/iostat

diff --git a/agent/VERSION b/agent/VERSION
index 7375dee5f4..fac6d18325 100644
--- a/agent/VERSION
+++ b/agent/VERSION
@@ -1 +1 @@
-0.72.0
+0.72.1
diff --git a/agent/util-scripts/gold/pbench-postprocess-tools/test-07.1.txt b/agent/util-scripts/gold/pbench-postprocess-tools/test-07.1.txt
new file mode 100644
index 0000000000..24b0c722ae
--- /dev/null
+++ b/agent/util-scripts/gold/pbench-postprocess-tools/test-07.1.txt
@@ -0,0 +1,25 @@
++++ Running test-07.1 pbench-postprocess-tools --group=foobar --dir=/var/tmp/pbench-test-utils/pbench/42-iter/sample42
+--- Finished test-07.1 pbench-postprocess-tools (status=0)
+iostat: post-processing data
++++ pbench tree state
+/var/tmp/pbench-test-utils/pbench
+/var/tmp/pbench-test-utils/pbench/42-iter
+/var/tmp/pbench-test-utils/pbench/42-iter/sample42
+/var/tmp/pbench-test-utils/pbench/42-iter/sample42/tools-foobar
+/var/tmp/pbench-test-utils/pbench/42-iter/sample42/tools-foobar/testhost.example.com
+/var/tmp/pbench-test-utils/pbench/42-iter/sample42/tools-foobar/testhost.example.com/iostat
+/var/tmp/pbench-test-utils/pbench/42-iter/sample42/tools-foobar/testhost.example.com/iostat/csv
+/var/tmp/pbench-test-utils/pbench/42-iter/sample42/tools-foobar/testhost.example.com/iostat/iostat-postprocess.err
+/var/tmp/pbench-test-utils/pbench/42-iter/sample42/tools-foobar/testhost.example.com/iostat/iostat-postprocess.out
+/var/tmp/pbench-test-utils/pbench/42-iter/sample42/tools-foobar/testhost.example.com/iostat/iostat-stdout.txt
+/var/tmp/pbench-test-utils/pbench/42-iter/sample42/tools-foobar/testhost.example.com/postprocess.log
+/var/tmp/pbench-test-utils/pbench/tmp
+/var/tmp/pbench-test-utils/pbench/tools-v1-foobar
+/var/tmp/pbench-test-utils/pbench/tools-v1-foobar/testhost.example.com
+/var/tmp/pbench-test-utils/pbench/tools-v1-foobar/testhost.example.com/foo
+/var/tmp/pbench-test-utils/pbench/tools-v1-foobar/testhost.example.com/iostat
+=== /var/tmp/pbench-test-utils/pbench/tools-v1-foobar/testhost.example.com/foo:
+--interval=10
+=== /var/tmp/pbench-test-utils/pbench/tools-v1-foobar/testhost.example.com/iostat:
+--interval=10
+--- pbench tree state
diff --git a/agent/util-scripts/gold/pbench-postprocess-tools/test-07.txt b/agent/util-scripts/gold/pbench-postprocess-tools/test-07.txt
index 5ec4dfcbba..978445f486 100644
--- a/agent/util-scripts/gold/pbench-postprocess-tools/test-07.txt
+++ b/agent/util-scripts/gold/pbench-postprocess-tools/test-07.txt
@@ -6,13 +6,13 @@ iostat: post-processing data
 /var/tmp/pbench-test-utils/pbench/42-iter
 /var/tmp/pbench-test-utils/pbench/42-iter/sample42
 /var/tmp/pbench-test-utils/pbench/42-iter/sample42/tools-foobar
-/var/tmp/pbench-test-utils/pbench/42-iter/sample42/tools-foobar/testhost.example.com
-/var/tmp/pbench-test-utils/pbench/42-iter/sample42/tools-foobar/testhost.example.com/iostat
-/var/tmp/pbench-test-utils/pbench/42-iter/sample42/tools-foobar/testhost.example.com/iostat/csv
-/var/tmp/pbench-test-utils/pbench/42-iter/sample42/tools-foobar/testhost.example.com/iostat/iostat-postprocess.err
-/var/tmp/pbench-test-utils/pbench/42-iter/sample42/tools-foobar/testhost.example.com/iostat/iostat-postprocess.out
-/var/tmp/pbench-test-utils/pbench/42-iter/sample42/tools-foobar/testhost.example.com/iostat/iostat-stdout.txt
-/var/tmp/pbench-test-utils/pbench/42-iter/sample42/tools-foobar/testhost.example.com/postprocess.log
+/var/tmp/pbench-test-utils/pbench/42-iter/sample42/tools-foobar/bar:testhost.example.com
+/var/tmp/pbench-test-utils/pbench/42-iter/sample42/tools-foobar/bar:testhost.example.com/iostat
+/var/tmp/pbench-test-utils/pbench/42-iter/sample42/tools-foobar/bar:testhost.example.com/iostat/csv
+/var/tmp/pbench-test-utils/pbench/42-iter/sample42/tools-foobar/bar:testhost.example.com/iostat/iostat-postprocess.err
+/var/tmp/pbench-test-utils/pbench/42-iter/sample42/tools-foobar/bar:testhost.example.com/iostat/iostat-postprocess.out
+/var/tmp/pbench-test-utils/pbench/42-iter/sample42/tools-foobar/bar:testhost.example.com/iostat/iostat-stdout.txt
+/var/tmp/pbench-test-utils/pbench/42-iter/sample42/tools-foobar/bar:testhost.example.com/postprocess.log
 /var/tmp/pbench-test-utils/pbench/tmp
 /var/tmp/pbench-test-utils/pbench/tools-v1-foobar
 /var/tmp/pbench-test-utils/pbench/tools-v1-foobar/testhost.example.com
diff --git a/agent/util-scripts/pbench-postprocess-tools b/agent/util-scripts/pbench-postprocess-tools
index 0b84d4ef2f..c17daf9d3e 100755
--- a/agent/util-scripts/pbench-postprocess-tools
+++ b/agent/util-scripts/pbench-postprocess-tools
@@ -91,11 +91,14 @@ pbench-list-tools --group ${group} --with-option >${tmp} 2>/dev/null
 let failures=0
 while read hostentry ;do
 	# Parse an entry from the output of `pbench-list-tools` above.
-	# The format is: "group: <group>; host: <host>; tools: <tool> <options, <tool> <options>, ...
+	# The format is: "group: <group>; host: <host> [, label: <label>]; tools: <tool> <options, <tool> <options>, ...
 	IFS=';' read group_spec host_spec tools_spec <<<"${hostentry}"
-	IFS=':' read dummy host junk <<<"${host_spec}"
+	IFS=',' read hostpart labelpart <<<"${host_spec}"
+	IFS=':' read dummy host junk <<<"${hostpart}"
+	IFS=':' read dummy label junk <<<"${labelpart}"
 	IFS=':' read dummy tools_entry junk <<<"${tools_spec}"
 	host=${host# *}
+	label=${label# *}
 	# Associative array: the keys are the tool names, and the values are the options
 	declare -A tools=()
 	IFS=',' read -a otools <<<"${tools_entry# *}"
@@ -104,8 +107,11 @@ while read hostentry ;do
 		tools[${atool[0]}]=${atool[@]:1}
 	done
 
-	# FIXME: add support for label applied to the hostname directory.
-	host_tool_output_dir="${tool_output_dir}/${host}"
+	if [[ -z "${label}" ]] ;then
+		host_tool_output_dir="${tool_output_dir}/${host}"
+	else
+		host_tool_output_dir="${tool_output_dir}/${label}:${host}"
+	fi
 	if [[ -d "${host_tool_output_dir}" ]]; then
 		for tool_name in ${!tools[@]} ;do
 			if [[ ! -x "${pbench_bin}/tool-scripts/${tool_name}" ]]; then
diff --git a/agent/util-scripts/samples/pbench-postprocess-tools/test-07.1/pbench/tools-v1-foobar/testhost.example.com/foo b/agent/util-scripts/samples/pbench-postprocess-tools/test-07.1/pbench/tools-v1-foobar/testhost.example.com/foo
new file mode 100644
index 0000000000..fb3b24a6ff
--- /dev/null
+++ b/agent/util-scripts/samples/pbench-postprocess-tools/test-07.1/pbench/tools-v1-foobar/testhost.example.com/foo
@@ -0,0 +1 @@
+--interval=10
diff --git a/agent/util-scripts/samples/pbench-postprocess-tools/test-07.1/pbench/tools-v1-foobar/testhost.example.com/iostat b/agent/util-scripts/samples/pbench-postprocess-tools/test-07.1/pbench/tools-v1-foobar/testhost.example.com/iostat
new file mode 100644
index 0000000000..fb3b24a6ff
--- /dev/null
+++ b/agent/util-scripts/samples/pbench-postprocess-tools/test-07.1/pbench/tools-v1-foobar/testhost.example.com/iostat
@@ -0,0 +1 @@
+--interval=10
diff --git a/agent/util-scripts/unittests b/agent/util-scripts/unittests
index 2bf786a979..8ae0b8aa3f 100755
--- a/agent/util-scripts/unittests
+++ b/agent/util-scripts/unittests
@@ -366,6 +366,7 @@ declare -A tools=(
     [test-05]="pbench-start-tools"
     [test-06]="pbench-stop-tools"
     [test-07]="pbench-postprocess-tools"
+    [test-07.1]="pbench-postprocess-tools"
     [test-08]="pbench-kill-tools"
     [test-09]="pbench-kill-tools"
     [test-10]="pbench-kill-tools"
@@ -451,6 +452,7 @@ declare -A options=(
     [test-05]="--group=default --dir=42-iter/sample42"
     [test-06]="--group=default --dir=42-iter/sample42"
     [test-07]="--group=foobar --dir=${_testdir}/42-iter/sample42"
+    [test-07.1]="--group=foobar --dir=${_testdir}/42-iter/sample42"
     [test-08]="--group=barfoo --dir=42-iter/sample42"
     [test-09]="--group=barfoo"
     [test-10]="--dir=barfoo"
@@ -566,7 +568,10 @@ declare -A expected_status=(
 declare -A pre_hooks=(
     [test-05]='ln -s mock-pbench-tool-meister-client ${_testopt}/unittest-scripts/pbench-tool-meister-client'
     [test-06]='ln -s mock-pbench-tool-meister-client ${_testopt}/unittest-scripts/pbench-tool-meister-client; mkdir -p ${_testdir}/42-iter/sample42/tools-default/testhost.example.com/iostat; touch ${_testdir}/42-iter/sample42/tools-default/testhost.example.com/iostat/iostat-stdout.txt'
-    [test-07]='mkdir -p ${_testdir}/42-iter/sample42/tools-foobar/testhost.example.com/iostat; touch ${_testdir}/42-iter/sample42/tools-foobar/testhost.example.com/iostat/iostat-stdout.txt'
+    # with labeled host
+    [test-07]='mkdir -p ${_testdir}/42-iter/sample42/tools-foobar/bar:testhost.example.com/iostat; touch ${_testdir}/42-iter/sample42/tools-foobar/bar:testhost.example.com/iostat/iostat-stdout.txt'
+    # with unlabeled host
+    [test-07.1]='mkdir -p ${_testdir}/42-iter/sample42/tools-foobar/testhost.example.com/iostat; touch ${_testdir}/42-iter/sample42/tools-foobar/testhost.example.com/iostat/iostat-stdout.txt'
     [test-11.16]='mkdir ${_testdir}/tmp; (echo foo; echo bar) > ${_testdir}/tmp/good-remote-file'
     [test-11.17]='mkdir ${_testdir}/tmp; touch  ${_testdir}/tmp/empty-remote-file'
     [test-19]='ln -s mock-pbench-tool-meister-client ${_testopt}/unittest-scripts/pbench-tool-meister-client'
@@ -623,7 +628,10 @@ function sort_tmlogs {
 declare -A post_hooks=(
     [test-05]='rm ${_testopt}/unittest-scripts/pbench-tool-meister-client'
     [test-06]='rm ${_testopt}/unittest-scripts/pbench-tool-meister-client'
-    [test-07]='cat ${_testdir}/42-iter/sample42/tools-foobar/testhost.example.com/postprocess.log >> ${_testout} 2>&1'
+    # with labeled host
+    [test-07]='cat ${_testdir}/42-iter/sample42/tools-foobar/bar:testhost.example.com/postprocess.log >> ${_testout} 2>&1'
+    # with unlabeled host
+    [test-07.1]='cat ${_testdir}/42-iter/sample42/tools-foobar/testhost.example.com/postprocess.log >> ${_testout} 2>&1'
     [test-19]='rm ${_testopt}/unittest-scripts/pbench-tool-meister-client'
     [test-53]='sort_testlog; sort_tmlogs; filter_tmerrs'
     [test-54]='sed -Ei "s/^optional arguments:/options:/" ${_testout}'
diff --git a/lib/pbench/cli/agent/commands/tools/list.py b/lib/pbench/cli/agent/commands/tools/list.py
index f8f381e50e..e942dc40e3 100644
--- a/lib/pbench/cli/agent/commands/tools/list.py
+++ b/lib/pbench/cli/agent/commands/tools/list.py
@@ -29,16 +29,19 @@ def print_results(toolinfo: dict, with_option: bool) -> bool:
         """
         printed = False
         for group, gval in sorted(toolinfo.items()):
-            for host, tools in sorted(gval.items()):
+            for host, hostitems in sorted(gval.items()):
+                label = toolinfo[group][host]["label"]
+                host_string = f"host: {host}" + (f", label: {label}" if label else "")
+                tools = hostitems["tools"]
                 if tools:
                     if not with_option:
-                        s = ", ".join(sorted(tools.keys()))
+                        tool_string = ", ".join(sorted(tools.keys()))
                     else:
                         tools_with_options = [
                             f"{t} {' '.join(v)}" for t, v in sorted(tools.items())
                         ]
-                        s = ", ".join(tools_with_options)
-                    print(f"group: {group}; host: {host}; tools: {s}")
+                        tool_string = ", ".join(tools_with_options)
+                    print(f"group: {group}; {host_string}; tools: {tool_string}")
                     printed = True
         return printed
 
@@ -68,12 +71,16 @@ def execute(self) -> int:
                 for path in tg_dir:
                     host = path.name
                     tool_info[group][host] = {}
+
+                    label = path / "__label__"
+                    v = label.read_text().rstrip("\n") if label.exists() else None
+                    tool_info[group][host]["label"] = v
+
+                    tool_info[group][host]["tools"] = {}
+
                     for tool in sorted(self.tools(path)):
-                        tool_info[group][host][tool] = (
-                            sorted((path / tool).read_text().rstrip("\n").split("\n"))
-                            if opts
-                            else ""
-                        )
+                        v = sorted((path / tool).read_text().rstrip("\n").split("\n"))
+                        tool_info[group][host]["tools"][tool] = v if opts else ""
 
             if tool_info:
                 found = self.print_results(tool_info, self.context.with_option)
@@ -106,13 +113,13 @@ def execute(self) -> int:
 
                     host = path.name
                     tool_info[group][host] = {}
-                    # Check to see if the tool is in any of the hosts.
+                    tool_info[group][host]["label"] = None
+                    tool_info[group][host]["tools"] = {}
+
+                    # Check if the tool is in any of the hosts.
                     if tool in self.tools(path):
-                        tool_info[group][host][tool] = (
-                            sorted((path / tool).read_text().rstrip("\n").split("\n"))
-                            if opts
-                            else ""
-                        )
+                        v = sorted((path / tool).read_text().rstrip("\n").split("\n"))
+                        tool_info[group][host]["tools"][tool] = v if opts else ""
                         found = True
 
             if found:
diff --git a/lib/pbench/test/functional/agent/cli/commands/tools/test_tools_list.py b/lib/pbench/test/functional/agent/cli/commands/tools/test_tools_list.py
index f88dacedf7..3eb703ebb0 100644
--- a/lib/pbench/test/functional/agent/cli/commands/tools/test_tools_list.py
+++ b/lib/pbench/test/functional/agent/cli/commands/tools/test_tools_list.py
@@ -259,6 +259,39 @@ def tools_on_multiple_hosts(self, pbench_run):
                     tool = p / "perf"
                     tool.write_text("--record-opts='-a --freq=100'")
 
+    # Issue #3454
+    @pytest.fixture
+    def labels_on_multiple_hosts(self, pbench_run, tools_on_multiple_hosts):
+        # This fixture is meant to be called after the previous one
+        # (tools_on_multiple_hosts). The previous one establishes a
+        # tool-like directory structure; this one just embellishes it
+        # with labels on some host entries. Think of it as a
+        # double for-loop, like the one above, only unrolled.
+
+        # row 1
+        group = "default"
+
+        # column 1
+        host = "th1.example.com"
+        label = pbench_run / f"tools-v1-{group}" / host / "__label__"
+        label.write_text("foo")
+
+        # column 2
+        host = "th2.example.com"
+        label = pbench_run / f"tools-v1-{group}" / host / "__label__"
+        label.write_text("bar")
+
+        # row 2
+        group = "test"
+
+        # column 1
+        host = "th1.example.com"
+        label = pbench_run / f"tools-v1-{group}" / host / "__label__"
+        label.write_text("bar")
+
+        # column 2
+        # th2 has no label
+
     # Issue #2346
     def test_existing_group_options(self, single_group_tools, agent_config):
         command = ["pbench-list-tools", "--with-option"]
@@ -309,3 +342,12 @@ def test_multiple_hosts_with_options(self, tools_on_multiple_hosts, agent_config
             b"group: default; host: th1.example.com; tools: iostat --interval=30, mpstat --interval=300, sar --interval=10\ngroup: default; host: th2.example.com; tools: iostat --interval=30, mpstat --interval=300, perf --record-opts='-a --freq=100'\ngroup: test; host: th1.example.com; tools: iostat --interval=30, mpstat --interval=300, sar --interval=10\ngroup: test; host: th2.example.com; tools: iostat --interval=30, mpstat --interval=300, perf --record-opts='-a --freq=100'"
             in out
         )
+
+    def test_multiple_hosts_with_labels(self, labels_on_multiple_hosts, agent_config):
+        command = ["pbench-list-tools", "--with-option"]
+        out, err, exitcode = pytest.helpers.capture(command)
+        assert EMPTY == err and exitcode == 0
+        assert (
+            b"group: default; host: th1.example.com, label: foo; tools: iostat --interval=30, mpstat --interval=300, sar --interval=10\ngroup: default; host: th2.example.com, label: bar; tools: iostat --interval=30, mpstat --interval=300, perf --record-opts='-a --freq=100'\ngroup: test; host: th1.example.com, label: bar; tools: iostat --interval=30, mpstat --interval=300, sar --interval=10\ngroup: test; host: th2.example.com; tools: iostat --interval=30, mpstat --interval=300, perf --record-opts='-a --freq=100'"
+            in out
+        )