Cluster health monitor consumes the CVS cluster file#163
Merged
speriaswamy-amd merged 3 commits intoMay 19, 2026
Conversation
speriaswamy-amd
changed the title
… 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 <cursoragent@cursor.com>
… 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 <cursoragent@cursor.com>
cijohnson
approved these changes
May 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The
check_cluster_healthmonitor previously required an undocumented one-IP-per-line--hosts_fileplus separate--usernameand--password/--key_fileflags, which forced users to maintain a second source of truth alongside the cluster file already used bycvs runandcvs exec.This PR makes the monitor a first-class consumer of the existing cluster JSON:
--cluster_file <path>(recommended) — readsnode_dictkeys as the host list andusername/priv_key_fileas the SSH credentials, with{user-id}placeholders resolved via the existingutils_lib.resolve_cluster_config_placeholdershelper. No new schema, same filecvs run/cvs execalready use.--cluster_fileor--hosts_file"; passing credential flags alongside--cluster_fileis rejected.--hosts_filekept as a deprecated fallback with a runtimeWARNING, plus support for#comments and blank lines so existing host files keep working.load_cluster_file()andCheckClusterHealthMonitor._resolve_connection()so all argument-resolution logic is unit-testable in isolation;monitor()stays linear.cvs/monitors/README.mdanddocs/how-to/run-cluster.rstupdated to lead with--cluster_fileand mark the legacy flags as deprecated.cvs/monitors/unittests/test_check_cluster_health.py, 19 cases) cover the loader (happy path,{user-id}resolution, missing file, bad JSON, missing/empty fields), the argparse contract (cluster-only, hosts-only, both, neither), and_resolve_connection(credential rules for both code paths, comment/blank-line stripping).Test plan
python3 -m unittest cvs.monitors.unittests.test_check_cluster_health— 19/19 passruff checkandruff format --checkclean on changed filescvs monitor check_cluster_health --cluster_file cluster.json --iterations 2): authenticated against all nodes, completed both iterations, generated the expected 25-section HTML report, and correctly surfaced realamdgpurunlist warnings from one node'sdmesg--hosts_fileinvocation still works and prints the deprecation warningMade with Cursor