Unify logging and CLI output across CVS by cijohnson · Pull Request #122 · ROCm/cvs

cijohnson · 2026-04-14T21:43:56Z

Use cvs.lib.globals.log in library and test code; replace print with appropriate log.info, log.warning, or log.error where messages are diagnostic or operational.
Correct log levels for failures, timeouts, skips, and validation messages.
Keep print() for user-facing CLI paths: main.py plugin load, extension.ini parse warnings, monitors/base.py, generators (heatmap, cluster_json), and check_cluster_health monitor output.
Remove duplicate log.info after log.warn/log.error in verify_lib snapshot diff.
Fix html_lib heatmap tests to assert on logging with assertLogs instead of mocked stdout.

Made-with: Cursor

Motivation

Technical Details

Test Plan

Test Result

Submission Checklist

Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.

solaiys · 2026-04-15T18:15:57Z

-                    print(intf)
-                    print(l_dict)
+                    log.info(intf)
+                    log.info(l_dict)


This will give TypeError
should be like this:
log.info("%s %s .....", var1, var2, var3 , .....)
or use f-string.

solaiys

Check all the log.info(var1, var2, .... ) instances and update with the f-string or "%s...."

- Use cvs.lib.globals.log in library and test code; replace print with appropriate log.info, log.warning, or log.error where messages are diagnostic or operational. - Correct log levels for failures, timeouts, skips, and validation messages. - Keep print() for user-facing CLI paths: main.py plugin load, extension.ini parse warnings, monitors/base.py, generators (heatmap, cluster_json), and check_cluster_health monitor output. - Remove duplicate log.info after log.warn/log.error in verify_lib snapshot diff. - Fix html_lib heatmap tests to assert on logging with assertLogs instead of mocked stdout. - make lint: run Ruff as before, then Pylint on cvs/ with only logging logging-too-many-args (E1205) and logging-too-few-args (E1206); install pylint in .ruff_venv alongside ruff. Made-with: Cursor Signed-off-by: Ignatious Johnson <ichristo@amd.com>

solaiys

LGTM

Consolidated fix for four small, narrow bugs in cvs/lib/linux_utils.py. Each fix has its own regression test in cvs/lib/unittests/test_linux_utils.py following the existing MagicMock pattern; all four new tests fail against the pre-fix code and pass here. 1. get_rdma_nic_dict missing match-guard (real impact on banff today): match.group(1) was called without checking that the strict inner pattern matched. DOWN rdma links omit the `netdev <name>` clause, so the first DOWN device raised AttributeError and aborted the parse for all nodes. The sibling get_active_rdma_nic_dict already guards with `if match:`; add the same guard here. Caller check_cluster_health.py:73 wraps the call in try/except, so the health-report HTML was silently missing RDMA NIC data on banff. Live observation on banff-cyxtera-s70-2: sudo rdma link link mlx5_0/1 ... state DOWN physical_state DISABLED ... link mlx5_8/1 state ACTIVE physical_state LINK_UP netdev ens14np0 ### get_rdma_nic_dict on banff ### AttributeError: 'NoneType' object has no attribute 'group' 2. get_ip_addr_dict int_nam leaks across nodes (latent, multi-node): int_nam was initialized once outside the per-node loop, so after parsing node A it retained A's last interface name. If node B's first matching line was a property line (mtu/state/mac/inet/inet6) rather than an interface header, the code did `ip_dict['nodeB']['<nodeA-iface>']['mtu'] = ...` and raised KeyError. Move `int_nam = None` inside the per-node loop and add an `if int_nam is None: continue` guard after the header block so property handlers no-op until the first header is seen. Header lines carry `mtu N`/`state UP` inline, so they still fall through because int_nam is just set by the header block above. 3. get_linux_perf_tuning_dict never returns (no callers today, primes the function for future wiring-in): The function built out_dict but ended without a `return`, so every caller received None. The docstring itself flagged it. Append the missing return and update the docstring. Live-confirmed on banff (TYPE: NoneType, VALUE: None). 4. get_dns_dict dead branches (no callers today, same reasoning): Duplicated `elif re.search('Protocols', ...)` branch and every body was `print('')`, so dns_dict[node] was always {}. Replace the dead branches with proper regex captures for Protocols (collected as a list since it appears once globally and once per Link), Current DNS Server, DNS Servers (space-split list), and DNS Domain. Live-confirmed on banff: resolvectl returned usable data but dns_dict was {'banff-...': {}}. Coordination: no overlap with PR #15 (multi-OS additions appended after line 814) or PR #122 (logging-only conversions elsewhere in the file).

cijohnson requested review from anujmittal-amd, solaiys, speriaswamy-amd and venksrin09 and removed request for speriaswamy-amd April 14, 2026 21:44