docs(nvl-domain-health): document NVL domain health report#3691
docs(nvl-domain-health): document NVL domain health report#3691jayzhudev wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughAdded an operator guide for NVLink domain health reports, covering report behavior, persistence, management surfaces, authentication, and CLI commands. Registered the guide under the “Operations (Day 2)” documentation navigation. ChangesNVLink Domain Health Reports
Estimated code review effort: 1 (Trivial) | ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
docs/operations/nvlink-domain-health-reports.md (3)
6-8: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winState replace-mode precedence precisely.
The linked contract in
docs/architecture/health_aggregation.md, Lines 291-302, definesreplaceas ignoring builtin probe alerts and considering only the override alerts. “Overrides that aggregate” is ambiguous about whether merge-mode external sources are also excluded; document the exact precedence to prevent operators from selecting the wrong mode.Suggested wording
-Multiple merge-mode sources coexist and contribute to the domain's aggregate health. A replace-mode source overrides that aggregate. +Multiple merge-mode sources coexist and contribute to the domain's aggregate health. In replace mode, builtin probe alerts are ignored and only the replacement report is considered; explicitly document whether other external sources are also excluded.As per path instructions, documentation must be technically correct and operator-usable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/operations/nvlink-domain-health-reports.md` around lines 6 - 8, Update the replace-mode description in the report aggregation section to state that it ignores builtin probe alerts and considers only the replace-mode override alerts, excluding merge-mode source contributions. Keep the existing explanation that multiple merge-mode sources contribute to aggregate health, and align terminology with the contract in health_aggregation.md.Source: Path instructions
15-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the API request shape, not only operation names.
The guide lists the RPCs and points operators to
InsertNVLinkDomainHealthReport, but does not show how to supply the domain, source, mode, or report payload. Add a minimal request example or link to the canonical API reference for list, insert, and remove operations.As per path instructions, documentation should be technically correct and usable by operators.
Also applies to: 63-65
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/operations/nvlink-domain-health-reports.md` around lines 15 - 16, Update the API section in nvlink-domain-health-reports.md to document the request shape for ListNVLinkDomainHealthReports, InsertNVLinkDomainHealthReport, and RemoveNVLinkDomainHealthReport, including domain, source, mode, and report payload fields where applicable. Add minimal technically correct request examples or links to the canonical API reference so operators can invoke each operation.Source: Path instructions
20-22: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMake RBAC requirements actionable.
“
HealthorForgeAdminCLIpermission scope” does not identify which scope applies to each API/CLI operation, nor which roles grant access to the web UI. Add an operation-to-scope mapping and link to the authoritative RBAC configuration.As per path instructions, operator documentation must provide clear and technically accurate deployment guidance.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/operations/nvlink-domain-health-reports.md` around lines 20 - 22, Update the RBAC guidance in the operations document to map each API and CLI operation to its required permission scope, distinguishing Health from ForgeAdminCLI usage. Add a link to the authoritative RBAC configuration and identify the roles that grant admin web UI access, while preserving the existing deployment authentication and network-policy requirements.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@docs/operations/nvlink-domain-health-reports.md`:
- Around line 6-8: Update the replace-mode description in the report aggregation
section to state that it ignores builtin probe alerts and considers only the
replace-mode override alerts, excluding merge-mode source contributions. Keep
the existing explanation that multiple merge-mode sources contribute to
aggregate health, and align terminology with the contract in
health_aggregation.md.
- Around line 15-16: Update the API section in nvlink-domain-health-reports.md
to document the request shape for ListNVLinkDomainHealthReports,
InsertNVLinkDomainHealthReport, and RemoveNVLinkDomainHealthReport, including
domain, source, mode, and report payload fields where applicable. Add minimal
technically correct request examples or links to the canonical API reference so
operators can invoke each operation.
- Around line 20-22: Update the RBAC guidance in the operations document to map
each API and CLI operation to its required permission scope, distinguishing
Health from ForgeAdminCLI usage. Add a link to the authoritative RBAC
configuration and identify the roles that grant admin web UI access, while
preserving the existing deployment authentication and network-policy
requirements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f1725265-2610-4ffb-90aa-9818dfd43134
📒 Files selected for processing (2)
docs/index.ymldocs/operations/nvlink-domain-health-reports.md
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3691.docs.buildwithfern.com/infra-controller |
polarweasel
left a comment
polarweasel
left a comment
There was a problem hiding this comment.
Just one question here, but it's maybe a big one :)
| - **API:** `ListNVLinkDomainHealthReports`, `InsertNVLinkDomainHealthReport`, | ||
| `RemoveNVLinkDomainHealthReport`. |
There was a problem hiding this comment.
I can't find these APIs in the docs. Have you added them somewhere yet? If so, let's get some actual links in this doc.
There was a problem hiding this comment.
These are RPCs in forge.proto:
infra-controller/crates/rpc/proto/forge.proto
Line 5305 in 0e59cee
Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
c0982eb to
e083bef
Compare
Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
e083bef to
929c2a3
Compare
2 participants
Add NVLink domain health report documentation to Operations navigation.
Related issues
Closes #3690
Type of Change
Breaking Changes
Testing