Skip to content

Add rack-aware execution with switch tray support#254

Open
cijohnson wants to merge 1 commit into
mainfrom
ichristo/cvs-exec-enhancement
Open

Add rack-aware execution with switch tray support#254
cijohnson wants to merge 1 commit into
mainfrom
ichristo/cvs-exec-enhancement

Conversation

@cijohnson

Copy link
Copy Markdown
Collaborator

Extends cvs exec and cvs generate to handle heterogeneous cluster topologies with separate compute and switch tray targets.

exec_plugin.py

  • Add --target {computes,switches,all} (default: computes, no regression)
  • Add --timeout (default 30s): per-node command output read timeout
  • Add --connect-timeout (default 15s): per-node SSH handshake timeout; unreachable hosts fail independently of --timeout
  • Add --json: emit structured JSON envelope {command, read_timeout, connect_timeout, output: {host: str}}
  • Add --verbose / -v: show SSH diagnostics (suppressed at ERROR level by default to avoid noisy SocketDisconnectError/pruning output)
  • Add _collect_switch_hosts(): returns flat list of switch IPs; credential resolution is NOT done here
  • Add _run_on_hosts(): shared SSH exec helper; num_retries=0 to prevent hangs on unreachable hosts
  • Add _emit_error(): stderr-aware error printer for --json mode
  • Add _print_text_output(): human-readable per-host result printer
  • Resolve switch credentials fleet-wide from the racks block top-level (switch_ssh_key_file takes priority over switch_ssh_password)
  • Emit DeprecationWarning when legacy rack_groups key is used

parsers/schemas.py

  • Add RackConfig model: per-rack schema (platform, arc_controller, switch_trays, rmc)
  • Add RacksBlock model: fleet-wide switch credentials + rack entries; switch creds are homogeneous across all racks (no per-rack overrides)
  • Add optional racks field to ClusterConfigFile (rack_groups deprecated)

generate_plugin.py

  • Add supports_raw_argv() hook for plugins with dynamic argument groups

New files

  • input/generate/rack_cluster_json.py: generator with dynamic --rackN groups
  • input/templates/cluster_file/rack_cluster_json.template: Jinja2 template
  • input/cluster_file/cluster_rack.json: example rack-aware cluster file
  • cli_plugins/unittests/test_exec_plugin.py: 41 unit tests

Motivation

Technical Details

Test Plan

Test Result

Submission Checklist

@cijohnson
cijohnson force-pushed the ichristo/cvs-exec-enhancement branch 2 times, most recently from f494ff0 to 46e3b43 Compare July 10, 2026 23:09

@solaiys solaiys left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review focused on blocking issues (per my review checklist), against origin/main. Found 2 blocking issues, both in the new reference cluster file cvs/input/cluster_file/cluster_rack.json. The Python changes (exec_plugin, generate_plugin, rack_cluster_json, schemas, globals) looked correct. No other reviews were present, so nothing to deduplicate.

"switch_trays": [
"10.0.2.1",
"10.0.2.2",
"10.0.2.3"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking — invalid JSON. "10.0.2.3" is missing a trailing comma before "10.0.2.4", so this file fails to parse (json.JSONDecodeError: Expecting ',' delimiter at line 126). Since the docs point users to this file as the reference template for --target switches/--target all, cvs exec will fail immediately in ExecPlugin.run when loading it. Please add the missing comma after "10.0.2.3".

Comment on lines +89 to +111
"10.0.0.14": {
"bmc_ip": "10.0.1.14",
"vpc_ip": "10.0.0.13",
"rack_id": "rack-01"
},
"10.0.0.15": {
"bmc_ip": "10.0.1.15",
"vpc_ip": "10.0.0.13",
"rack_id": "rack-01"
},
"10.0.0.16": {
"bmc_ip": "10.0.1.16",
"vpc_ip": "10.0.0.13",
"rack_id": "rack-01"
},
"10.0.0.17": {
"bmc_ip": "10.0.1.17",
"vpc_ip": "10.0.0.13",
"rack_id": "rack-01"
},
"10.0.0.18": {
"bmc_ip": "10.0.1.18",
"vpc_ip": "10.0.0.13",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking — wrong vpc_ip values. Nodes 10.0.0.14 through 10.0.0.18 all set "vpc_ip": "10.0.0.13" (copy-paste error). vpc_ip is used for inter-node communication, so a user copying this template gets five nodes silently pointing at the wrong host. Each node should use its own IP (10.0.0.1410.0.0.18).

Extends cvs exec and cvs generate to handle heterogeneous cluster
topologies with separate compute and switch tray targets.

exec_plugin.py
- Add --target {computes,switches,all} (default: computes, no regression)
- Add --timeout (default 30s): per-node command output read timeout
- Add --connect-timeout (default 15s): per-node SSH handshake timeout;
  unreachable hosts fail independently of --timeout
- Add --json: emit structured JSON envelope
  {command, read_timeout, connect_timeout, output: {host: str}}
- Add --verbose / -v: show SSH diagnostics (suppressed at ERROR level
  by default to avoid noisy SocketDisconnectError/pruning output)
- Add _collect_switch_hosts(): returns flat list of switch IPs;
  credential resolution is NOT done here
- Add _run_on_hosts(): shared SSH exec helper; num_retries=0 to
  prevent hangs on unreachable hosts
- Add _emit_error(): stderr-aware error printer for --json mode
- Add _print_text_output(): human-readable per-host result printer
- Resolve switch credentials fleet-wide from the racks block top-level
  (switch_ssh_key_file takes priority over switch_ssh_password)
- Emit DeprecationWarning when legacy rack_groups key is used

parsers/schemas.py
- Add RackConfig model: per-rack schema (platform, arc_controller,
  switch_trays, rmc)
- Add RacksBlock model: fleet-wide switch credentials + rack entries;
  switch creds are homogeneous across all racks (no per-rack overrides)
- Add optional racks field to ClusterConfigFile (rack_groups deprecated)

generate_plugin.py
- Add supports_raw_argv() hook for plugins with dynamic argument groups

New files
- input/generate/rack_cluster_json.py: generator with dynamic --rackN groups
- input/templates/cluster_file/rack_cluster_json.template: Jinja2 template
- input/cluster_file/cluster_rack.json: example rack-aware cluster file
- cli_plugins/unittests/test_exec_plugin.py: 41 unit tests

Signed-off-by: ichristo <ichristo@amd.com>
@cijohnson
cijohnson force-pushed the ichristo/cvs-exec-enhancement branch from 46e3b43 to fcff0b4 Compare July 20, 2026 21:21
@solaiys

solaiys commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Re-review after force-push (head fcff0b46)

Re-reviewed the squashed/force-pushed revision. Both blocking issues from my earlier review are resolved, and the reworked Python is sound. Blocking issues only below.

Blocking issues

No blocking issues found.

  • Invalid JSON (missing comma) — FIXED. cvs/input/cluster_file/cluster_rack.json now parses cleanly (validated json.load at head). The switch_trays list (10.0.2.110.0.2.6) is well-formed.
  • Wrong vpc_ip values — FIXED. All 18 nodes now set vpc_ip to their own IP (10.0.0.110.0.0.18); no more copy-paste 10.0.0.13.
  • New Pssh call sites are signature-compatible (checked against the real source, since the unit tests mock Pssh). Pssh resolves to MultiProcessPssh, whose __init__ accepts password, stop_on_errors, env_vars directly and forwards num_retries=0 / timeout=<connect-timeout> through **ssh_client_kwargs to ParallelSSHClient; exec(cmd, timeout=...) accepts the read timeout. So cvs exec (all targets) won't TypeError.
  • No regression to existing generators. supports_raw_argv() defaults to False on the base class; only the new rack generator opts in, so _run_generator's parse_known_args path is scoped to it. The Jinja template emits valid JSON (credential lines always precede ≥1 rack; --rack0 is required), and --json mode keeps stdout clean (warnings/logging go to stderr).
  • Schema additions are backward-compatible (racks/rack_groups, RackConfig/RacksBlock, rack_id via extra="allow"); deprecated rack_groups is handled with a warning and racks takes precedence.

Reviewed after force-push: 0 blocking issues (both previously-raised blockers resolved) — clean.

@solaiys solaiys left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants