Fix handling of labeled hosts by pbench-postprocess-tools (fwd port of …

…#3456) (#3472)

* Fix handling of labeled hosts by pbench-postprocess-tools (forward port of #3456)

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 1.0

PBENCH-1178

agent/VERSION

@@ -1 +1 @@
-0.72.0
+1.0

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

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

agent/util-scripts/pbench-postprocess-tools

@@ -87,11 +87,14 @@ fi
 let failures=0
 pbench-list-tools --group ${group} --with-option 2>/dev/null | 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# *}"
@@ -100,8 +103,11 @@ pbench-list-tools --group ${group} --with-option 2>/dev/null | while read hosten
 		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

agent/util-scripts/samples/pbench-postprocess-tools/test-07.1/pbench/tools-v1-foobar/testhost.example.com/foo

@@ -0,0 +1 @@
+--interval=10

agent/util-scripts/samples/pbench-postprocess-tools/test-07.1/pbench/tools-v1-foobar/testhost.example.com/iostat

@@ -0,0 +1 @@
+--interval=10

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}'

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 = hostitems["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 = [
+                        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
 
@@ -54,76 +57,65 @@ def execute(self) -> int:
             groups = self.groups
 
         opts = self.context.with_option
+        toolname = self.context.name
+        found = False
         tool_info = {}
 
-        if not self.context.name:
-            for group in groups:
-                tool_info[group] = {}
-                try:
-                    tg_dir = self.gen_tools_group_dir(group).glob("*/**")
-                except BadToolGroup:
-                    self.logger.error("Bad tool group: %s", group)
-                    return 1
-
-                for path in tg_dir:
-                    host = path.name
-                    tool_info[group][host] = {}
-                    for tool in sorted(self.tools(path)):
-                        tool_info[group][host][tool] = (
-                            sorted((path / tool).read_text().rstrip("\n").split("\n"))
-                            if opts
-                            else ""
-                        )
+        for group in groups:
+            tool_info[group] = {}
+            try:
+                tg_dir = self.gen_tools_group_dir(group)
+            except BadToolGroup:
+                self.logger.error("Bad tool group: %s", group)
+                return 1
 
-            if tool_info:
-                found = self.print_results(tool_info, self.context.with_option)
-                if not found:
-                    msg = "No tools found"
-                    if self.context.group:
-                        msg += f' in group "{self.context.group[0]}"'
-                    self.logger.warn(msg)
-            else:
-                self.logger.warn("No tool groups found")
+            for path in tg_dir.iterdir():
+                # skip __trigger__ if present
+                if not path.is_dir():
+                    continue
 
-            return 0
+                host = path.name
+                tool_info[group][host] = {"label": None, "tools": {}}
 
-        else:
-            # List the groups which include this tool
-            tool = self.context.name
-            found = False
-            for group in groups:
-                try:
-                    tg_dir = self.gen_tools_group_dir(group)
-                except BadToolGroup:
-                    self.logger.error("Bad tool group: %s", group)
-                    return 1
-
-                tool_info[group] = {}
-                for path in tg_dir.iterdir():
-                    # skip files like __label__ and __trigger__
-                    if not path.is_dir():
-                        continue
-
-                    host = path.name
-                    tool_info[group][host] = {}
-                    # Check to see 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 ""
-                        )
-                        found = True
+                toolsdict = tool_info[group][host]["tools"]
+                if toolname:
+                    # Check if the tool is in any of the hosts.
+                    found = toolname in self.tools(path)
+                    toolslist = [toolname] if found else []
+                else:
+                    # no tool name was specified
+                    toolslist = sorted(self.tools(path))
 
+                    label = path / "__label__"
+                    v = label.read_text().rstrip("\n") if label.exists() else None
+                    tool_info[group][host]["label"] = v
+
+                for tool in toolslist:
+                    v = (path / tool).read_text().rstrip("\n").split("\n")
+                    toolsdict[tool] = sorted(v) if opts else ""
+
+        if toolname:
             if found:
                 self.print_results(tool_info, self.context.with_option)
                 return 0
             else:
-                msg = f'Tool "{self.context.name}" not found in '
+                msg = f'Tool "{toolname}" not found in '
                 msg += self.context.group[0] if self.context.group else "any group"
                 self.logger.error(msg)
                 return 1
 
+        elif tool_info:
+            found = self.print_results(tool_info, self.context.with_option)
+            if not found:
+                msg = "No tools found"
+                if self.context.group:
+                    msg += f' in group "{self.context.group[0]}"'
+                self.logger.warn(msg)
+        else:
+            self.logger.warn("No tool groups found")
+
+        return 0
+
 
 def _group_option(f):
     """Group name option"""

lib/pbench/test/functional/agent/cli/commands/tools/test_tools_list.py

@@ -259,6 +259,41 @@ 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):
+        # Note that this fixture depends on `tools_on_multiple_hosts'.
+        # It embelishes the structure produced by
+        # `tools_on_multiple_hosts' by adding labels to some host
+        # entries in the tool-like directory structure that
+        # `tools_on_multiple_hosts' establishes. Think of it as a
+        # double for-loop, like the one in `tools_on_multiple_hosts',
+        # 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 +344,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
+        )