From b9b4222b370acba77704ce4dcc8f4521a944b6b0 Mon Sep 17 00:00:00 2001 From: speriaswamy-amd Date: Tue, 12 May 2026 15:56:28 -0400 Subject: [PATCH 1/3] cluster health check scripts should use clusterfile with optional fallback --- cvs/monitors/README.md | 60 +++-- cvs/monitors/check_cluster_health.py | 117 ++++++++-- cvs/monitors/unittests/__init__.py | 0 .../unittests/test_check_cluster_health.py | 209 ++++++++++++++++++ docs/how-to/run-cluster.rst | 17 +- 5 files changed, 367 insertions(+), 36 deletions(-) create mode 100644 cvs/monitors/unittests/__init__.py create mode 100644 cvs/monitors/unittests/test_check_cluster_health.py diff --git a/cvs/monitors/README.md b/cvs/monitors/README.md index ace795e94..01e753944 100644 --- a/cvs/monitors/README.md +++ b/cvs/monitors/README.md @@ -22,33 +22,67 @@ napshot of all counters (GPU/NIC) while your training/inference workloads are in ### Usage for Cluster Health Checker +The Cluster Health Checker consumes the same `--cluster_file` JSON used by +`cvs run` and `cvs exec` (see [`cvs/input/cluster_file/README.md`](../input/cluster_file/README.md) +for the schema). The node list, SSH username, and private key are all read +from the cluster file - no separate hosts file or credentials need to be +passed on the CLI. ``` -(myenv) [ubuntu-host]~/cvs:(main)$ -(myenv) [ubuntu-host]~/cvs:(main)$python3 ./cvs/monitors/check_cluster_health.py -h -usage: check_cluster_health.py [-h] --hosts_file HOSTS_FILE --username USERNAME (--password PASSWORD | --key_file KEY_FILE) - [--iterations ITERATIONS] [--time_between_iters TIME_BETWEEN_ITERS] [--report_file REPORT_FILE] +(myenv) [ubuntu-host]~/cvs:(main)$ cvs monitor check_cluster_health --help +usage: cvs monitor check_cluster_health [-h] (--cluster_file CLUSTER_FILE | --hosts_file HOSTS_FILE) + [--username USERNAME] [--password PASSWORD] [--key_file KEY_FILE] + [--iterations ITERATIONS] [--time_between_iters TIME_BETWEEN_ITERS] + [--report_file REPORT_FILE] Check Cluster Health options: -h, --help show this help message and exit + --cluster_file CLUSTER_FILE + Path to a CVS cluster JSON file (see cvs/input/cluster_file/cluster.json). + Provides node list, username, and SSH key. Recommended. --hosts_file HOSTS_FILE - File name with list of IP address one per line - --username USERNAME Username to ssh to the hosts - --password PASSWORD Password for username - --key_file KEY_FILE Private Keyfile for username + [DEPRECATED] File with one host IP/hostname per line. Use --cluster_file instead. + --username USERNAME SSH username (required with --hosts_file) + --password PASSWORD SSH password (only valid with --hosts_file) + --key_file KEY_FILE SSH private key file (only valid with --hosts_file) --iterations ITERATIONS Number of iterations to run the checks --time_between_iters TIME_BETWEEN_ITERS - Time duration to sleep between iterations .. + Time duration to sleep between iterations --report_file REPORT_FILE -(myenv) [ubuntu-host]~/cvs:(main)$ -(myenv) [ubuntu-host]~/cvs/cvs:(main)$ -(myenv) [ubuntu-host]~/cvs/cvs/monitors:(main)$ -(myenv) [ubuntu-host]~/cvs/cvs/monitors:(main)$python3 ./check_cluster_health.py --hosts_file /home/user1/hosts_file.txt --username user1 --key_file /home/user1/.ssh/id_rsa --iterations 2 + Output HTML report file path +``` + +#### Recommended: run with a cluster file + +``` +cvs monitor check_cluster_health \ + --cluster_file cvs/input/cluster_file/cluster.json \ + --iterations 2 +``` + +Or directly: ``` +python3 ./cvs/monitors/check_cluster_health.py \ + --cluster_file cvs/input/cluster_file/cluster.json \ + --iterations 2 +``` + +#### Deprecated: legacy hosts file + +`--hosts_file` is still accepted for backward compatibility but emits a +deprecation warning. Lines starting with `#` and blank lines are ignored. + +``` +python3 ./cvs/monitors/check_cluster_health.py \ + --hosts_file /home/user1/hosts_file.txt \ + --username user1 \ + --key_file /home/user1/.ssh/id_rsa \ + --iterations 2 +``` ### Debugging using RDMA Statistics Table diff --git a/cvs/monitors/check_cluster_health.py b/cvs/monitors/check_cluster_health.py index 1a63d2594..75f898a7c 100644 --- a/cvs/monitors/check_cluster_health.py +++ b/cvs/monitors/check_cluster_health.py @@ -6,6 +6,7 @@ ''' import sys +import json import logging import argparse import time @@ -16,10 +17,54 @@ from cvs.lib import linux_utils from cvs.lib import rocm_plib from cvs.lib import html_lib +from cvs.lib import utils_lib log = logging.getLogger() +def load_cluster_file(cluster_file_path): + """ + Load and validate a CVS cluster file for the cluster health monitor. + + Parses the JSON cluster file used by ``cvs run`` / ``cvs exec`` and + extracts the SSH credentials and node list. Path placeholders such as + ``{user-id}`` are resolved via :func:`utils_lib.resolve_cluster_config_placeholders`. + + Args: + cluster_file_path (str): Path to the cluster JSON file. + + Returns: + tuple: ``(node_list, username, priv_key_file)`` where ``node_list`` + is the list of host IPs / hostnames (the keys of ``node_dict``). + + Raises: + FileNotFoundError: If the cluster file does not exist. + ValueError: If the cluster file is missing required keys, has no + nodes, or contains invalid JSON. + """ + try: + with open(cluster_file_path, 'r') as f: + cluster = json.load(f) + except json.JSONDecodeError as e: + raise ValueError(f"Invalid JSON in cluster file '{cluster_file_path}': {e}") from e + + cluster = utils_lib.resolve_cluster_config_placeholders(cluster) + + username = cluster.get('username') + if not username: + raise ValueError(f"'username' missing from cluster file '{cluster_file_path}'") + + priv_key_file = cluster.get('priv_key_file') + if not priv_key_file: + raise ValueError(f"'priv_key_file' missing from cluster file '{cluster_file_path}'") + + node_list = list((cluster.get('node_dict') or {}).keys()) + if not node_list: + raise ValueError(f"'node_dict' is empty or missing in cluster file '{cluster_file_path}'") + + return node_list, username, priv_key_file + + def general_health_checks( phdl, ): @@ -332,11 +377,25 @@ def get_description(self): def get_parser(self): parser = argparse.ArgumentParser(description="Check Cluster Health") - parser.add_argument("--hosts_file", required=True, help="File name with list of IP address one per line") - parser.add_argument("--username", required=True, help="Username to ssh to the hosts") - group = parser.add_mutually_exclusive_group(required=True) - group.add_argument("--password", help="Password for username") - group.add_argument("--key_file", help="Private Keyfile for username") + # Source of hosts: either a CVS cluster file (preferred) or a legacy + # one-IP-per-line hosts file. Exactly one must be provided. + source = parser.add_mutually_exclusive_group(required=True) + source.add_argument( + "--cluster_file", + help=( + "Path to a CVS cluster JSON file " + "(see cvs/input/cluster_file/cluster.json). " + "Provides node list, username, and SSH key. Recommended." + ), + ) + source.add_argument( + "--hosts_file", + help="[DEPRECATED] File with one host IP/hostname per line. Use --cluster_file instead.", + ) + # Credentials - only required when --hosts_file is used. + parser.add_argument("--username", help="SSH username (required with --hosts_file)") + parser.add_argument("--password", help="SSH password (only valid with --hosts_file)") + parser.add_argument("--key_file", help="SSH private key file (only valid with --hosts_file)") parser.add_argument("--iterations", type=int, default=2, help="Number of iterations to run the checks") parser.add_argument( "--time_between_iters", type=int, default=60, help="Time duration to sleep between iterations" @@ -344,22 +403,50 @@ def get_parser(self): parser.add_argument("--report_file", default="./cluster_report.html", help="Output HTML report file path") return parser - def monitor(self, args): - """Execute the cluster health check monitoring.""" - # Read host IP addresses from input file. + def _resolve_connection(self, args): + """Return ``(node_list, username, pkey, password)`` based on parsed args. + + Centralizes the cluster_file vs. hosts_file argument handling so the + ``monitor`` method stays linear and the logic stays unit-testable. + """ + if args.cluster_file: + if args.username or args.password or args.key_file: + print( + "ERROR: --username/--password/--key_file are not allowed with --cluster_file; " + "credentials come from the cluster file. Aborting." + ) + sys.exit(1) + node_list, username, pkey = load_cluster_file(args.cluster_file) + return node_list, username, pkey, None + + # Legacy --hosts_file path. + print( + "WARNING: --hosts_file is deprecated. Switch to --cluster_file (see cvs/input/cluster_file/cluster.json)." + ) + if not args.username: + print("ERROR: --username is required when using --hosts_file. Aborting.") + sys.exit(1) + if bool(args.password) == bool(args.key_file): + print("ERROR: exactly one of --password or --key_file is required with --hosts_file. Aborting.") + sys.exit(1) with open(args.hosts_file, "r") as f: - node_list = [line.strip() for line in f if line.strip()] + node_list = [line.strip() for line in f if line.strip() and not line.lstrip().startswith("#")] if not node_list: - print("ERROR !! No hosts in the file, this is mandatory, aborting !!") + print(f"ERROR: no hosts found in '{args.hosts_file}'. Aborting.") sys.exit(1) + return node_list, args.username, args.key_file, args.password + + def monitor(self, args): + """Execute the cluster health check monitoring.""" + node_list, username, pkey, password = self._resolve_connection(args) html_report_file = args.report_file - # Connect to all hosts in the cluster .. - if args.key_file: - phdl = parallel_ssh_lib.Pssh(log, node_list, user=args.username, pkey=args.key_file) - elif args.password: - phdl = parallel_ssh_lib.Pssh(log, node_list, user=args.username, password=args.password) + # Connect to all hosts in the cluster. + if pkey: + phdl = parallel_ssh_lib.Pssh(log, node_list, user=username, pkey=pkey) + else: + phdl = parallel_ssh_lib.Pssh(log, node_list, user=username, password=password) start_time = phdl.exec('date') diff --git a/cvs/monitors/unittests/__init__.py b/cvs/monitors/unittests/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/cvs/monitors/unittests/test_check_cluster_health.py b/cvs/monitors/unittests/test_check_cluster_health.py new file mode 100644 index 000000000..213395dbd --- /dev/null +++ b/cvs/monitors/unittests/test_check_cluster_health.py @@ -0,0 +1,209 @@ +''' +Copyright 2026 Advanced Micro Devices, Inc. +All rights reserved. + +Unit tests for cvs/monitors/check_cluster_health.py. + +These cover the cluster-file loader and the CLI argument-resolution logic +so that future drifts in the cluster file shape, the deprecated --hosts_file +fallback, or the credential validation rules are caught here. +''' + +import argparse +import json +import os +import tempfile +import unittest +from unittest.mock import patch + +from cvs.monitors.check_cluster_health import CheckClusterHealthMonitor, load_cluster_file + + +def _write(tmpdir, name, contents): + """Write ``contents`` to ``tmpdir/name`` and return the full path.""" + path = os.path.join(tmpdir, name) + with open(path, 'w') as f: + f.write(contents) + return path + + +class TestLoadClusterFile(unittest.TestCase): + """Validate the cluster-file loader used by the monitor.""" + + def test_valid_cluster_file_returns_nodes_user_and_key(self): + cluster = { + "username": "alice", + "priv_key_file": "/home/alice/.ssh/id_rsa", + "node_dict": { + "10.0.0.1": {"bmc_ip": "NA", "vpc_ip": "10.0.0.1"}, + "10.0.0.2": {"bmc_ip": "NA", "vpc_ip": "10.0.0.2"}, + }, + } + with tempfile.TemporaryDirectory() as tmp: + path = _write(tmp, "cluster.json", json.dumps(cluster)) + nodes, user, pkey = load_cluster_file(path) + self.assertEqual(nodes, ["10.0.0.1", "10.0.0.2"]) + self.assertEqual(user, "alice") + self.assertEqual(pkey, "/home/alice/.ssh/id_rsa") + + def test_user_id_placeholder_is_resolved(self): + cluster = { + "username": "{user-id}", + "priv_key_file": "/home/{user-id}/.ssh/id_rsa", + "node_dict": {"host-a": {"bmc_ip": "NA", "vpc_ip": "host-a"}}, + } + with tempfile.TemporaryDirectory() as tmp: + path = _write(tmp, "cluster.json", json.dumps(cluster)) + with patch.dict(os.environ, {"USER": "bob"}, clear=False): + nodes, user, pkey = load_cluster_file(path) + self.assertEqual(nodes, ["host-a"]) + self.assertEqual(user, "bob") + self.assertEqual(pkey, "/home/bob/.ssh/id_rsa") + + def test_missing_file_raises(self): + with self.assertRaises(FileNotFoundError): + load_cluster_file("/nonexistent/path/cluster.json") + + def test_invalid_json_raises_value_error(self): + with tempfile.TemporaryDirectory() as tmp: + path = _write(tmp, "cluster.json", "{not json") + with self.assertRaises(ValueError): + load_cluster_file(path) + + def test_missing_username_raises_value_error(self): + cluster = { + "priv_key_file": "/k", + "node_dict": {"h1": {}}, + } + with tempfile.TemporaryDirectory() as tmp: + path = _write(tmp, "cluster.json", json.dumps(cluster)) + with self.assertRaisesRegex(ValueError, "username"): + load_cluster_file(path) + + def test_missing_priv_key_file_raises_value_error(self): + cluster = { + "username": "u", + "node_dict": {"h1": {}}, + } + with tempfile.TemporaryDirectory() as tmp: + path = _write(tmp, "cluster.json", json.dumps(cluster)) + with self.assertRaisesRegex(ValueError, "priv_key_file"): + load_cluster_file(path) + + def test_empty_node_dict_raises_value_error(self): + cluster = {"username": "u", "priv_key_file": "/k", "node_dict": {}} + with tempfile.TemporaryDirectory() as tmp: + path = _write(tmp, "cluster.json", json.dumps(cluster)) + with self.assertRaisesRegex(ValueError, "node_dict"): + load_cluster_file(path) + + +class TestArgParser(unittest.TestCase): + """Pin the CLI surface so a future refactor cannot silently change it.""" + + def setUp(self): + self.parser = CheckClusterHealthMonitor().get_parser() + + def test_cluster_file_alone_is_accepted(self): + args = self.parser.parse_args(["--cluster_file", "/tmp/c.json"]) + self.assertEqual(args.cluster_file, "/tmp/c.json") + self.assertIsNone(args.hosts_file) + + def test_hosts_file_alone_is_accepted(self): + args = self.parser.parse_args(["--hosts_file", "/tmp/h.txt", "--username", "u", "--key_file", "/k"]) + self.assertEqual(args.hosts_file, "/tmp/h.txt") + self.assertIsNone(args.cluster_file) + + def test_both_cluster_and_hosts_file_is_rejected(self): + with self.assertRaises(SystemExit): + self.parser.parse_args(["--cluster_file", "/c.json", "--hosts_file", "/h.txt"]) + + def test_neither_cluster_nor_hosts_file_is_rejected(self): + with self.assertRaises(SystemExit): + self.parser.parse_args(["--iterations", "1"]) + + +class TestResolveConnection(unittest.TestCase): + """Drive the credential-resolution branch points with synthetic Namespaces.""" + + def setUp(self): + self.monitor = CheckClusterHealthMonitor() + + @staticmethod + def _ns(**overrides): + defaults = dict( + cluster_file=None, + hosts_file=None, + username=None, + password=None, + key_file=None, + ) + defaults.update(overrides) + return argparse.Namespace(**defaults) + + def test_cluster_file_returns_creds_from_file(self): + cluster = { + "username": "u", + "priv_key_file": "/key", + "node_dict": {"h1": {}, "h2": {}}, + } + with tempfile.TemporaryDirectory() as tmp: + path = _write(tmp, "c.json", json.dumps(cluster)) + nodes, user, pkey, password = self.monitor._resolve_connection(self._ns(cluster_file=path)) + self.assertEqual(nodes, ["h1", "h2"]) + self.assertEqual((user, pkey, password), ("u", "/key", None)) + + def test_cluster_file_with_extra_creds_aborts(self): + cluster = {"username": "u", "priv_key_file": "/k", "node_dict": {"h1": {}}} + with tempfile.TemporaryDirectory() as tmp: + path = _write(tmp, "c.json", json.dumps(cluster)) + with self.assertRaises(SystemExit): + self.monitor._resolve_connection(self._ns(cluster_file=path, username="other")) + + def test_hosts_file_with_key_file(self): + with tempfile.TemporaryDirectory() as tmp: + hosts_path = _write(tmp, "hosts.txt", "1.1.1.1\n2.2.2.2\n# comment\n\n") + nodes, user, pkey, password = self.monitor._resolve_connection( + self._ns(hosts_file=hosts_path, username="u", key_file="/k") + ) + self.assertEqual(nodes, ["1.1.1.1", "2.2.2.2"]) + self.assertEqual((user, pkey, password), ("u", "/k", None)) + + def test_hosts_file_with_password(self): + with tempfile.TemporaryDirectory() as tmp: + hosts_path = _write(tmp, "hosts.txt", "1.1.1.1\n") + nodes, user, pkey, password = self.monitor._resolve_connection( + self._ns(hosts_file=hosts_path, username="u", password="pw") + ) + self.assertEqual(nodes, ["1.1.1.1"]) + self.assertEqual((user, pkey, password), ("u", None, "pw")) + + def test_hosts_file_missing_username_aborts(self): + with tempfile.TemporaryDirectory() as tmp: + hosts_path = _write(tmp, "hosts.txt", "1.1.1.1\n") + with self.assertRaises(SystemExit): + self.monitor._resolve_connection(self._ns(hosts_file=hosts_path, key_file="/k")) + + def test_hosts_file_both_password_and_key_aborts(self): + with tempfile.TemporaryDirectory() as tmp: + hosts_path = _write(tmp, "hosts.txt", "1.1.1.1\n") + with self.assertRaises(SystemExit): + self.monitor._resolve_connection( + self._ns(hosts_file=hosts_path, username="u", password="p", key_file="/k") + ) + + def test_hosts_file_neither_password_nor_key_aborts(self): + with tempfile.TemporaryDirectory() as tmp: + hosts_path = _write(tmp, "hosts.txt", "1.1.1.1\n") + with self.assertRaises(SystemExit): + self.monitor._resolve_connection(self._ns(hosts_file=hosts_path, username="u")) + + def test_empty_hosts_file_aborts(self): + with tempfile.TemporaryDirectory() as tmp: + hosts_path = _write(tmp, "hosts.txt", "# only a comment\n\n") + with self.assertRaises(SystemExit): + self.monitor._resolve_connection(self._ns(hosts_file=hosts_path, username="u", key_file="/k")) + + +if __name__ == "__main__": + unittest.main() diff --git a/docs/how-to/run-cluster.rst b/docs/how-to/run-cluster.rst index f7c96fe07..32aad2c9b 100644 --- a/docs/how-to/run-cluster.rst +++ b/docs/how-to/run-cluster.rst @@ -40,21 +40,22 @@ To run the monitor and generate a health report for a cluster: 5. Run the monitor with the applicable arguments for your use case: - - ``--hosts``: Direct the monitor to the file with the list of host IP addresses you want to check. - - ``--username``: Enter the username to SSH to the hosts. - - ``--password``: Enter the password to SSH to the hosts - - ``--key_file``: Enter the private Keyfile for the username. + - ``--cluster_file``: Path to the CVS cluster JSON file (the same file used by ``cvs run`` and ``cvs exec``). The monitor reads the node list, SSH username, and private key from this file. See :doc:`/reference/configuration-files/cluster-file` for the schema. - ``--iterations``: Enter the number of check iterations you want to run. - ``--time_between_iters``: Enter the time to wait between run iterations. - - ``--report_file``: Enter the directory you want the generated health file to save to. If you leave this argument empty, the file saves as ``cluster_report.html`` to the local directory. + - ``--report_file``: Enter the directory you want the generated health file to save to. If you leave this argument empty, the file saves as ``cluster_report.html`` to the local directory. - Here's an example command with some arguments set: + Here's an example command: .. code:: bash - cvs monitor check_cluster_health --hosts /home/user/input/host_file.txt --username myusername --key_file /home/user/input/.ssh/id_test --iterations 2 + cvs monitor check_cluster_health --cluster_file cvs/input/cluster_file/cluster.json --iterations 2 - The monitor logs into the nodes based on the hosts specified and captures information on potential error conditions or anomalies. + The monitor logs into the nodes listed in the cluster file and captures information on potential error conditions or anomalies. + + .. note:: + + The legacy ``--hosts_file`` / ``--username`` / ``--key_file`` / ``--password`` flags are still accepted for backward compatibility but are deprecated. Prefer ``--cluster_file`` so the monitor stays consistent with the rest of the CVS tooling. 6. Open the ``cluster_report.html`` file to view the generated health report for the cluster. From 3031d2d36f87fa26fafc53f8027e0932a33e0971 Mon Sep 17 00:00:00 2001 From: speriaswamy-amd Date: Tue, 12 May 2026 17:04:42 -0400 Subject: [PATCH 2/3] Address review: support CLUSTER_FILE env var for consistency with cvs exec/scp Mirror the cluster-file resolution used by cvs exec and cvs scp so users can `export CLUSTER_FILE=...` once and run any CVS command (exec, scp, monitor) without repeating --cluster_file. Env var takes precedence over the flag, matching the existing plugins. - Drop argparse-level "exactly one source" requirement so the env var can satisfy --cluster_file; enforce the rule at runtime in _resolve_connection along with explicit rejection of CLUSTER_FILE + --hosts_file mixing. - Update --cluster_file help, README, and run-cluster.rst to document the CLUSTER_FILE fallback and show the export-once usage pattern. - Add 4 new unit tests (env supplies path, env wins over flag, env + --hosts_file aborts, no source aborts) and make TestResolveConnection hermetic by clearing CLUSTER_FILE in setUp. 23/23 pass. Co-authored-by: Cursor --- cvs/monitors/README.md | 18 ++++++- cvs/monitors/check_cluster_health.py | 46 ++++++++++++---- .../unittests/test_check_cluster_health.py | 52 +++++++++++++++++-- docs/how-to/run-cluster.rst | 9 +++- 4 files changed, 111 insertions(+), 14 deletions(-) diff --git a/cvs/monitors/README.md b/cvs/monitors/README.md index 01e753944..c54aab698 100644 --- a/cvs/monitors/README.md +++ b/cvs/monitors/README.md @@ -23,11 +23,17 @@ napshot of all counters (GPU/NIC) while your training/inference workloads are in ### Usage for Cluster Health Checker The Cluster Health Checker consumes the same `--cluster_file` JSON used by -`cvs run` and `cvs exec` (see [`cvs/input/cluster_file/README.md`](../input/cluster_file/README.md) +`cvs run`, `cvs exec`, and `cvs scp` (see [`cvs/input/cluster_file/README.md`](../input/cluster_file/README.md) for the schema). The node list, SSH username, and private key are all read from the cluster file - no separate hosts file or credentials need to be passed on the CLI. +Just like `cvs exec` and `cvs scp`, the cluster file can be supplied either +via `--cluster_file ` or by exporting `CLUSTER_FILE=` once per +shell. The env var takes precedence when both are set, so users can +`export CLUSTER_FILE=...` once and then run any `cvs exec`, `cvs scp`, +`cvs monitor check_cluster_health`, etc. without repeating the path. + ``` (myenv) [ubuntu-host]~/cvs:(main)$ cvs monitor check_cluster_health --help usage: cvs monitor check_cluster_health [-h] (--cluster_file CLUSTER_FILE | --hosts_file HOSTS_FILE) @@ -42,6 +48,7 @@ options: --cluster_file CLUSTER_FILE Path to a CVS cluster JSON file (see cvs/input/cluster_file/cluster.json). Provides node list, username, and SSH key. Recommended. + Falls back to the CLUSTER_FILE environment variable when omitted. --hosts_file HOSTS_FILE [DEPRECATED] File with one host IP/hostname per line. Use --cluster_file instead. --username USERNAME SSH username (required with --hosts_file) @@ -63,6 +70,15 @@ cvs monitor check_cluster_health \ --iterations 2 ``` +Or set `CLUSTER_FILE` once and reuse it across CVS commands: + +``` +export CLUSTER_FILE=cvs/input/cluster_file/cluster.json +cvs monitor check_cluster_health --iterations 2 +cvs exec --cmd "hostname" +cvs scp --file /path/to/file.txt +``` + Or directly: ``` diff --git a/cvs/monitors/check_cluster_health.py b/cvs/monitors/check_cluster_health.py index 75f898a7c..3ea5151a3 100644 --- a/cvs/monitors/check_cluster_health.py +++ b/cvs/monitors/check_cluster_health.py @@ -5,6 +5,7 @@ All code contained here is Property of Advanced Micro Devices, Inc. ''' +import os import sys import json import logging @@ -376,16 +377,27 @@ def get_description(self): return "Generate a health report of your cluster by collecting various logs and metrics" def get_parser(self): - parser = argparse.ArgumentParser(description="Check Cluster Health") - # Source of hosts: either a CVS cluster file (preferred) or a legacy - # one-IP-per-line hosts file. Exactly one must be provided. - source = parser.add_mutually_exclusive_group(required=True) + parser = argparse.ArgumentParser( + description="Check Cluster Health", + epilog=( + "Examples:\n" + " cvs monitor check_cluster_health --cluster_file cluster.json --iterations 2\n" + " CLUSTER_FILE=cluster.json cvs monitor check_cluster_health --iterations 2" + ), + formatter_class=argparse.RawDescriptionHelpFormatter, + ) + # Source of hosts: either a CVS cluster file (preferred, can also be + # supplied via the CLUSTER_FILE env var) or a legacy one-IP-per-line + # hosts file. The "exactly one source" rule is enforced at runtime + # in _resolve_connection so the env var can satisfy --cluster_file. + source = parser.add_mutually_exclusive_group(required=False) source.add_argument( "--cluster_file", help=( "Path to a CVS cluster JSON file " "(see cvs/input/cluster_file/cluster.json). " - "Provides node list, username, and SSH key. Recommended." + "Provides node list, username, and SSH key. Recommended. " + "Falls back to the CLUSTER_FILE environment variable when omitted." ), ) source.add_argument( @@ -406,19 +418,35 @@ def get_parser(self): def _resolve_connection(self, args): """Return ``(node_list, username, pkey, password)`` based on parsed args. - Centralizes the cluster_file vs. hosts_file argument handling so the - ``monitor`` method stays linear and the logic stays unit-testable. + Resolution order for the cluster file mirrors ``cvs exec`` / ``cvs scp``: + the ``CLUSTER_FILE`` env var takes precedence, then ``--cluster_file``. + ``--hosts_file`` remains as a deprecated fallback. """ - if args.cluster_file: + # Mirror cvs exec / cvs scp: env var wins over --cluster_file, both + # win over the legacy --hosts_file path. + cluster_file = os.environ.get('CLUSTER_FILE') or args.cluster_file + + if cluster_file and args.hosts_file: + print("ERROR: --hosts_file cannot be combined with --cluster_file or CLUSTER_FILE. Aborting.") + sys.exit(1) + + if cluster_file: if args.username or args.password or args.key_file: print( "ERROR: --username/--password/--key_file are not allowed with --cluster_file; " "credentials come from the cluster file. Aborting." ) sys.exit(1) - node_list, username, pkey = load_cluster_file(args.cluster_file) + node_list, username, pkey = load_cluster_file(cluster_file) return node_list, username, pkey, None + if not args.hosts_file: + print( + "ERROR: no cluster file specified. Set the CLUSTER_FILE environment variable, " + "pass --cluster_file, or (deprecated) pass --hosts_file. Aborting." + ) + sys.exit(1) + # Legacy --hosts_file path. print( "WARNING: --hosts_file is deprecated. Switch to --cluster_file (see cvs/input/cluster_file/cluster.json)." diff --git a/cvs/monitors/unittests/test_check_cluster_health.py b/cvs/monitors/unittests/test_check_cluster_health.py index 213395dbd..0d6900107 100644 --- a/cvs/monitors/unittests/test_check_cluster_health.py +++ b/cvs/monitors/unittests/test_check_cluster_health.py @@ -118,9 +118,13 @@ def test_both_cluster_and_hosts_file_is_rejected(self): with self.assertRaises(SystemExit): self.parser.parse_args(["--cluster_file", "/c.json", "--hosts_file", "/h.txt"]) - def test_neither_cluster_nor_hosts_file_is_rejected(self): - with self.assertRaises(SystemExit): - self.parser.parse_args(["--iterations", "1"]) + def test_neither_cluster_nor_hosts_file_parses(self): + # Argparse no longer enforces "exactly one source" because the + # CLUSTER_FILE env var can satisfy --cluster_file. Runtime + # validation lives in _resolve_connection (see TestResolveConnection). + args = self.parser.parse_args(["--iterations", "1"]) + self.assertIsNone(args.cluster_file) + self.assertIsNone(args.hosts_file) class TestResolveConnection(unittest.TestCase): @@ -128,6 +132,12 @@ class TestResolveConnection(unittest.TestCase): def setUp(self): self.monitor = CheckClusterHealthMonitor() + # Each test gets a clean env so CLUSTER_FILE leakage from the host + # shell or other tests cannot influence the resolution branches. + self._env_patch = patch.dict(os.environ, {}, clear=False) + self._env_patch.start() + os.environ.pop('CLUSTER_FILE', None) + self.addCleanup(self._env_patch.stop) @staticmethod def _ns(**overrides): @@ -204,6 +214,42 @@ def test_empty_hosts_file_aborts(self): with self.assertRaises(SystemExit): self.monitor._resolve_connection(self._ns(hosts_file=hosts_path, username="u", key_file="/k")) + def test_cluster_file_env_var_supplies_path(self): + cluster = {"username": "envuser", "priv_key_file": "/envkey", "node_dict": {"h1": {}}} + with tempfile.TemporaryDirectory() as tmp: + path = _write(tmp, "c.json", json.dumps(cluster)) + os.environ['CLUSTER_FILE'] = path + nodes, user, pkey, password = self.monitor._resolve_connection(self._ns()) + self.assertEqual(nodes, ["h1"]) + self.assertEqual((user, pkey, password), ("envuser", "/envkey", None)) + + def test_cluster_file_env_var_takes_precedence_over_flag(self): + # Mirrors cvs exec / cvs scp: env wins. Pin this so a future + # refactor cannot silently flip the precedence. + env_cluster = {"username": "envuser", "priv_key_file": "/envkey", "node_dict": {"env-host": {}}} + flag_cluster = {"username": "flaguser", "priv_key_file": "/flagkey", "node_dict": {"flag-host": {}}} + with tempfile.TemporaryDirectory() as tmp: + env_path = _write(tmp, "env.json", json.dumps(env_cluster)) + flag_path = _write(tmp, "flag.json", json.dumps(flag_cluster)) + os.environ['CLUSTER_FILE'] = env_path + nodes, user, pkey, _ = self.monitor._resolve_connection(self._ns(cluster_file=flag_path)) + self.assertEqual(nodes, ["env-host"]) + self.assertEqual((user, pkey), ("envuser", "/envkey")) + + def test_cluster_file_env_var_combined_with_hosts_file_aborts(self): + cluster = {"username": "u", "priv_key_file": "/k", "node_dict": {"h1": {}}} + with tempfile.TemporaryDirectory() as tmp: + cluster_path = _write(tmp, "c.json", json.dumps(cluster)) + hosts_path = _write(tmp, "hosts.txt", "1.1.1.1\n") + os.environ['CLUSTER_FILE'] = cluster_path + with self.assertRaises(SystemExit): + self.monitor._resolve_connection(self._ns(hosts_file=hosts_path, username="u", key_file="/k")) + + def test_no_source_at_all_aborts(self): + # Neither --cluster_file, nor --hosts_file, nor CLUSTER_FILE in env. + with self.assertRaises(SystemExit): + self.monitor._resolve_connection(self._ns()) + if __name__ == "__main__": unittest.main() diff --git a/docs/how-to/run-cluster.rst b/docs/how-to/run-cluster.rst index 32aad2c9b..eb28b42fd 100644 --- a/docs/how-to/run-cluster.rst +++ b/docs/how-to/run-cluster.rst @@ -40,7 +40,7 @@ To run the monitor and generate a health report for a cluster: 5. Run the monitor with the applicable arguments for your use case: - - ``--cluster_file``: Path to the CVS cluster JSON file (the same file used by ``cvs run`` and ``cvs exec``). The monitor reads the node list, SSH username, and private key from this file. See :doc:`/reference/configuration-files/cluster-file` for the schema. + - ``--cluster_file``: Path to the CVS cluster JSON file (the same file used by ``cvs run``, ``cvs exec``, and ``cvs scp``). The monitor reads the node list, SSH username, and private key from this file. See :doc:`/reference/configuration-files/cluster-file` for the schema. If omitted, the monitor falls back to the ``CLUSTER_FILE`` environment variable, matching the behavior of ``cvs exec`` and ``cvs scp``. - ``--iterations``: Enter the number of check iterations you want to run. - ``--time_between_iters``: Enter the time to wait between run iterations. - ``--report_file``: Enter the directory you want the generated health file to save to. If you leave this argument empty, the file saves as ``cluster_report.html`` to the local directory. @@ -51,6 +51,13 @@ To run the monitor and generate a health report for a cluster: cvs monitor check_cluster_health --cluster_file cvs/input/cluster_file/cluster.json --iterations 2 + Or export ``CLUSTER_FILE`` once and reuse it across CVS commands: + + .. code:: bash + + export CLUSTER_FILE=cvs/input/cluster_file/cluster.json + cvs monitor check_cluster_health --iterations 2 + The monitor logs into the nodes listed in the cluster file and captures information on potential error conditions or anomalies. .. note:: From 226c28926578a78ef0c970e76a08ca4c2fa5c245 Mon Sep 17 00:00:00 2001 From: speriaswamy-amd Date: Tue, 12 May 2026 17:41:21 -0400 Subject: [PATCH 3/3] Address review: CLI flag wins over CLUSTER_FILE env, applied to all 3 cluster commands Per-review: an explicit --cluster_file should always take precedence over the CLUSTER_FILE env var. The previous code (in cvs exec, cvs scp, and the new check_cluster_health) had it inverted - env beat the flag - which violates standard Unix tooling expectations and is surprising when a user runs a one-off invocation against a different cluster after exporting CLUSTER_FILE for the common case. Flipped resolution order in all three places to: cluster_file = args.cluster_file or os.environ.get('CLUSTER_FILE') so the env var becomes the fallback. Updated --cluster_file help text, epilogs, README, and run-cluster.rst to describe the env var as a fallback. The unit test that pinned the old precedence is renamed to test_cluster_file_flag_takes_precedence_over_env_var and asserts the new direction. 23/23 monitor tests + 23/23 cli_plugins tests pass; ruff clean. Smoke-tested all three commands end-to-end with a wrong env path and a real flag path - flag wins in every case. Co-authored-by: Cursor --- cvs/cli_plugins/exec_plugin.py | 10 ++++++---- cvs/cli_plugins/scp_plugin.py | 10 ++++++---- cvs/monitors/README.md | 8 ++++---- cvs/monitors/check_cluster_health.py | 15 ++++++++------- .../unittests/test_check_cluster_health.py | 11 ++++++----- docs/how-to/run-cluster.rst | 2 +- 6 files changed, 31 insertions(+), 25 deletions(-) diff --git a/cvs/cli_plugins/exec_plugin.py b/cvs/cli_plugins/exec_plugin.py index 6e9b0367d..9d56ed018 100644 --- a/cvs/cli_plugins/exec_plugin.py +++ b/cvs/cli_plugins/exec_plugin.py @@ -15,7 +15,8 @@ def get_parser(self, subparsers): parser = subparsers.add_parser("exec", help="Execute a command on all nodes in the cluster") parser.add_argument("--cmd", required=True, help="Command to execute on all nodes") parser.add_argument( - "--cluster_file", help="Path to cluster configuration JSON file (overrides CLUSTER_FILE env var)" + "--cluster_file", + help="Path to cluster configuration JSON file (takes precedence over CLUSTER_FILE env var)", ) parser.set_defaults(_plugin=self) return parser @@ -24,11 +25,12 @@ def get_epilog(self): return """ Exec Commands: cvs exec --cmd "hostname" --cluster_file cluster.json Execute hostname on all nodes - CLUSTER_FILE=cluster.json cvs exec --cmd "hostname" Use env var for cluster file""" + CLUSTER_FILE=cluster.json cvs exec --cmd "hostname" Use env var for cluster file (fallback)""" def run(self, args): - # Determine cluster file: env var takes precedence, then --cluster_file - cluster_file = os.environ.get('CLUSTER_FILE') or args.cluster_file + # CLI flag wins; env var is the fallback. Matches cvs scp and + # cvs monitor check_cluster_health for consistency. + cluster_file = args.cluster_file or os.environ.get('CLUSTER_FILE') if not cluster_file: print("Error: No cluster file specified. Set CLUSTER_FILE environment variable or use --cluster_file.") sys.exit(1) diff --git a/cvs/cli_plugins/scp_plugin.py b/cvs/cli_plugins/scp_plugin.py index f3bdda979..d2aa620b7 100644 --- a/cvs/cli_plugins/scp_plugin.py +++ b/cvs/cli_plugins/scp_plugin.py @@ -18,7 +18,8 @@ def get_parser(self, subparsers): parser.add_argument("--dest", help="Remote destination path (defaults to same as source)") parser.add_argument("--recurse", action="store_true", help="Copy directories recursively") parser.add_argument( - "--cluster_file", help="Path to cluster configuration JSON file (overrides CLUSTER_FILE env var)" + "--cluster_file", + help="Path to cluster configuration JSON file (takes precedence over CLUSTER_FILE env var)", ) parser.add_argument("--parallel", type=int, default=20, help="Number of parallel SCP operations (default: 20)") parser.set_defaults(_plugin=self) @@ -31,7 +32,7 @@ def get_epilog(self): cvs scp --file /local/file.txt --dest /remote/file.txt Copy file to specific remote path cvs scp --file /local/dir --dest /remote/dir --recurse Copy directory recursively cvs scp --file /path/to/file.txt --parallel 10 Use 10 parallel SCP operations - CLUSTER_FILE=cluster.json cvs scp --file /path/to/file.txt Use env var for cluster file""" + CLUSTER_FILE=cluster.json cvs scp --file /path/to/file.txt Use env var for cluster file (fallback)""" def copy_file_to_host(self, log, host, username, pkey, local_file, remote_path, recurse=False): """Copy file to a single host via SCP.""" @@ -43,8 +44,9 @@ def copy_file_to_host(self, log, host, username, pkey, local_file, remote_path, return f"{host}: FAILED - {str(e)}" def run(self, args): - # Determine cluster file: env var takes precedence, then --cluster_file - cluster_file = os.environ.get('CLUSTER_FILE') or args.cluster_file + # CLI flag wins; env var is the fallback. Matches cvs exec and + # cvs monitor check_cluster_health for consistency. + cluster_file = args.cluster_file or os.environ.get('CLUSTER_FILE') if not cluster_file: print("Error: No cluster file specified. Set CLUSTER_FILE environment variable or use --cluster_file.") sys.exit(1) diff --git a/cvs/monitors/README.md b/cvs/monitors/README.md index c54aab698..b587b7fd2 100644 --- a/cvs/monitors/README.md +++ b/cvs/monitors/README.md @@ -30,9 +30,9 @@ passed on the CLI. Just like `cvs exec` and `cvs scp`, the cluster file can be supplied either via `--cluster_file ` or by exporting `CLUSTER_FILE=` once per -shell. The env var takes precedence when both are set, so users can -`export CLUSTER_FILE=...` once and then run any `cvs exec`, `cvs scp`, -`cvs monitor check_cluster_health`, etc. without repeating the path. +shell. An explicit `--cluster_file` flag always wins over the env var, so +you can `export CLUSTER_FILE=...` once for the common case and still +override it on a single invocation by passing `--cluster_file` on the CLI. ``` (myenv) [ubuntu-host]~/cvs:(main)$ cvs monitor check_cluster_health --help @@ -48,7 +48,7 @@ options: --cluster_file CLUSTER_FILE Path to a CVS cluster JSON file (see cvs/input/cluster_file/cluster.json). Provides node list, username, and SSH key. Recommended. - Falls back to the CLUSTER_FILE environment variable when omitted. + Takes precedence over the CLUSTER_FILE environment variable. --hosts_file HOSTS_FILE [DEPRECATED] File with one host IP/hostname per line. Use --cluster_file instead. --username USERNAME SSH username (required with --hosts_file) diff --git a/cvs/monitors/check_cluster_health.py b/cvs/monitors/check_cluster_health.py index 3ea5151a3..19980feb2 100644 --- a/cvs/monitors/check_cluster_health.py +++ b/cvs/monitors/check_cluster_health.py @@ -397,7 +397,7 @@ def get_parser(self): "Path to a CVS cluster JSON file " "(see cvs/input/cluster_file/cluster.json). " "Provides node list, username, and SSH key. Recommended. " - "Falls back to the CLUSTER_FILE environment variable when omitted." + "Takes precedence over the CLUSTER_FILE environment variable." ), ) source.add_argument( @@ -418,13 +418,14 @@ def get_parser(self): def _resolve_connection(self, args): """Return ``(node_list, username, pkey, password)`` based on parsed args. - Resolution order for the cluster file mirrors ``cvs exec`` / ``cvs scp``: - the ``CLUSTER_FILE`` env var takes precedence, then ``--cluster_file``. - ``--hosts_file`` remains as a deprecated fallback. + Resolution order for the cluster file matches ``cvs exec`` / ``cvs scp``: + an explicit ``--cluster_file`` flag takes precedence, then the + ``CLUSTER_FILE`` environment variable is used as a fallback. + ``--hosts_file`` remains as a deprecated last-resort path. """ - # Mirror cvs exec / cvs scp: env var wins over --cluster_file, both - # win over the legacy --hosts_file path. - cluster_file = os.environ.get('CLUSTER_FILE') or args.cluster_file + # CLI flag wins; env var is the fallback. This matches standard Unix + # tooling and keeps cvs exec / cvs scp / cvs monitor consistent. + cluster_file = args.cluster_file or os.environ.get('CLUSTER_FILE') if cluster_file and args.hosts_file: print("ERROR: --hosts_file cannot be combined with --cluster_file or CLUSTER_FILE. Aborting.") diff --git a/cvs/monitors/unittests/test_check_cluster_health.py b/cvs/monitors/unittests/test_check_cluster_health.py index 0d6900107..13d5a6f66 100644 --- a/cvs/monitors/unittests/test_check_cluster_health.py +++ b/cvs/monitors/unittests/test_check_cluster_health.py @@ -223,9 +223,10 @@ def test_cluster_file_env_var_supplies_path(self): self.assertEqual(nodes, ["h1"]) self.assertEqual((user, pkey, password), ("envuser", "/envkey", None)) - def test_cluster_file_env_var_takes_precedence_over_flag(self): - # Mirrors cvs exec / cvs scp: env wins. Pin this so a future - # refactor cannot silently flip the precedence. + def test_cluster_file_flag_takes_precedence_over_env_var(self): + # CLI flag wins over CLUSTER_FILE. Pin this so a future refactor + # cannot silently flip the precedence; it must stay in lockstep + # with cvs exec / cvs scp. env_cluster = {"username": "envuser", "priv_key_file": "/envkey", "node_dict": {"env-host": {}}} flag_cluster = {"username": "flaguser", "priv_key_file": "/flagkey", "node_dict": {"flag-host": {}}} with tempfile.TemporaryDirectory() as tmp: @@ -233,8 +234,8 @@ def test_cluster_file_env_var_takes_precedence_over_flag(self): flag_path = _write(tmp, "flag.json", json.dumps(flag_cluster)) os.environ['CLUSTER_FILE'] = env_path nodes, user, pkey, _ = self.monitor._resolve_connection(self._ns(cluster_file=flag_path)) - self.assertEqual(nodes, ["env-host"]) - self.assertEqual((user, pkey), ("envuser", "/envkey")) + self.assertEqual(nodes, ["flag-host"]) + self.assertEqual((user, pkey), ("flaguser", "/flagkey")) def test_cluster_file_env_var_combined_with_hosts_file_aborts(self): cluster = {"username": "u", "priv_key_file": "/k", "node_dict": {"h1": {}}} diff --git a/docs/how-to/run-cluster.rst b/docs/how-to/run-cluster.rst index eb28b42fd..d9b104093 100644 --- a/docs/how-to/run-cluster.rst +++ b/docs/how-to/run-cluster.rst @@ -40,7 +40,7 @@ To run the monitor and generate a health report for a cluster: 5. Run the monitor with the applicable arguments for your use case: - - ``--cluster_file``: Path to the CVS cluster JSON file (the same file used by ``cvs run``, ``cvs exec``, and ``cvs scp``). The monitor reads the node list, SSH username, and private key from this file. See :doc:`/reference/configuration-files/cluster-file` for the schema. If omitted, the monitor falls back to the ``CLUSTER_FILE`` environment variable, matching the behavior of ``cvs exec`` and ``cvs scp``. + - ``--cluster_file``: Path to the CVS cluster JSON file (the same file used by ``cvs run``, ``cvs exec``, and ``cvs scp``). The monitor reads the node list, SSH username, and private key from this file. See :doc:`/reference/configuration-files/cluster-file` for the schema. The flag takes precedence over the ``CLUSTER_FILE`` environment variable, matching the behavior of ``cvs exec`` and ``cvs scp``. - ``--iterations``: Enter the number of check iterations you want to run. - ``--time_between_iters``: Enter the time to wait between run iterations. - ``--report_file``: Enter the directory you want the generated health file to save to. If you leave this argument empty, the file saves as ``cluster_report.html`` to the local directory.