From 9d90a97cc437d5e6884625c629062f696d8cae66 Mon Sep 17 00:00:00 2001
From: Peter Portante <peter.portante@redhat.com>
Date: Wed, 25 May 2022 11:21:46 -0400
Subject: [PATCH] Ensure use of `localhost` when stopping Redis

This is a back-port of commit c84c6ed32 (PR #2863) from `main`.

When `pbench-tool-meister-stop` is invoked where the Redis server was
created locally by `pbench-tool-meister-start`, we always want to use
the `localhost` IP address to talk to it.

Also added unit tests for the `tool_meister_stop.py` module's
`RedisServer` class, and corrected the `pbench-tool-meister-stop` CLI
param default value for the `--redis-server` switch.

Fixes issue #2861 [1].

[1] https://github.com/distributed-system-analysis/pbench/issues/2861
---
 lib/pbench/agent/tool_meister_stop.py         |  7 +++-
 .../test/unit/agent/test_tool_meister_stop.py | 40 +++++++++++++++++++
 2 files changed, 46 insertions(+), 1 deletion(-)
 create mode 100644 lib/pbench/test/unit/agent/test_tool_meister_stop.py

diff --git a/lib/pbench/agent/tool_meister_stop.py b/lib/pbench/agent/tool_meister_stop.py
index a5db710c51..17c27568b6 100644
--- a/lib/pbench/agent/tool_meister_stop.py
+++ b/lib/pbench/agent/tool_meister_stop.py
@@ -65,6 +65,11 @@ def __init__(self, spec: str, benchmark_run_dir: Path, def_host_name: str):
             )
         except FileNotFoundError:
             pass
+        else:
+            # The redis.pid file exists in the "tm" directory, which means this
+            # Redis server is locally managed.  Communication with it will
+            # always be through the "localhost" interface.
+            self.host = "localhost"
 
     def locally_managed(self) -> bool:
         return self.pid_file is not None
@@ -286,7 +291,7 @@ def main():
     parser.add_argument(
         "--redis-server",
         dest="redis_server",
-        default=os.environ.get("PBENCH_REDIS_SERVER", None),
+        default=os.environ.get("PBENCH_REDIS_SERVER", ""),
         help=(
             "Use an existing Redis server specified by <hostname>:<port>;"
             " implies the use of an existing Tool Data Sink and Tool Meisters"
diff --git a/lib/pbench/test/unit/agent/test_tool_meister_stop.py b/lib/pbench/test/unit/agent/test_tool_meister_stop.py
new file mode 100644
index 0000000000..03fc72bdba
--- /dev/null
+++ b/lib/pbench/test/unit/agent/test_tool_meister_stop.py
@@ -0,0 +1,40 @@
+"""Tests for the Tool Meister "stop" module.
+"""
+from pbench.agent.tool_meister_stop import RedisServer
+
+
+class TestRedisServer:
+    """Verify the RedisServer class use by Tool Meister "stop"."""
+
+    def test_locally_managed(self, tmp_path):
+        # Locally managed means we have a run directory, ...
+        rundir = tmp_path / "run-dir"
+        rundir.mkdir()
+        # ... with a "tm" sub-directory, ...
+        tmdir = rundir / "tm"
+        tmdir.mkdir()
+        # ... containing a "redis.pid" file.
+        pidfile = tmdir / "redis.pid"
+        pidfile.write_text("12345")
+
+        rs = RedisServer("", rundir, "notme.example.com")
+        assert (
+            rs.locally_managed()
+        ), "RedisServer incorrectly inferred a non-locally managed instance from a run directory with a 'tm/redis.pid' file"
+        assert (
+            rs.host == "localhost"
+        ), f"Expected 'RedisServer.host' to be 'localhost', got '{rs.host}'"
+
+    def test_not_locally_managed(self, tmp_path):
+        # Empty benchmark run directory indicates not locally managed.
+        rundir = tmp_path / "empty-run-dir"
+        rundir.mkdir()
+
+        rs_host = "redis.example.com"
+        rs = RedisServer(f"{rs_host}:4343", rundir, "notme.example.com")
+        assert (
+            not rs.locally_managed()
+        ), "RedisServer incorrectly inferred a locally managed instance from an empty run directory"
+        assert (
+            rs.host == "redis.example.com"
+        ), f"Expected 'RedisServer.host' to be '{rs_host}', got '{rs.host}'"