Skip to content

Commit

Permalink
Ensure use of localhost when stopping Redis
Browse files Browse the repository at this point in the history
This is a back-port of commit c84c6ed (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] #2861
  • Loading branch information
portante committed May 25, 2022
1 parent acae28b commit 9d90a97
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 1 deletion.
7 changes: 6 additions & 1 deletion lib/pbench/agent/tool_meister_stop.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"
Expand Down
40 changes: 40 additions & 0 deletions lib/pbench/test/unit/agent/test_tool_meister_stop.py
Original file line number Diff line number Diff line change
@@ -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}'"

0 comments on commit 9d90a97

Please sign in to comment.