docs: update architecture diagram - #4833
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughThe architecture overview was reformatted and clarified. Obsolete diagrams were removed. NICo Core details were expanded. Service descriptions and the Fern configuration version were updated. ChangesArchitecture Overview
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
/ok to test |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4833.docs.buildwithfern.com/infra-controller |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@docs/architecture/overview.md`:
- Line 158: Clarify the health-capacity sentence in the Machine Update Manager
description by specifying whether the scheduler counts healthy Machines or
healthy hosts across the site, using the exact terminology established by the
implementation and surrounding documentation. Keep the explanation of limiting
concurrent updates and protecting the Machine health SLA unchanged.
- Line 92: Update the affected prose to use the compound adjectives
“machine-readable” at the gRPC reflection description, “admin-level” at the
referenced administration text, and “up-to-date” at the referenced status text;
at the HTTP reference, use “an HTTP-based” with the correct article. Preserve
the surrounding wording and meaning.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7f5fb9da-2a76-44ce-82df-5c71adda3b29
⛔ Files ignored due to path filters (1)
docs/static/nico_arch_diagram.svgis excluded by!**/*.svg
📒 Files selected for processing (1)
docs/architecture/overview.md
Contributor
Author
|
/ok to test |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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.
Inline comments:
In `@docs/architecture/overview.md`:
- Line 53: Update the DHCP description in the NICo architecture overview to use
“the host’s primary networking interfaces” and replace “network - which” with a
comma or em dash construction, preserving the sentence’s meaning.
- Line 49: Update the affected Markdown prose near the hotfix, resource, DHCP,
tenant, and host-console references to use the requested normalized forms:
bare-metal instances, NICo-managed resources, gRPC-based DHCP requests,
bare-metal tenants, and each host’s serial console. Preserve the existing
meaning and apply the same spelling and grammar corrections at all referenced
occurrences.
- Around line 77-78: Remove the stale NICo Core diagram reference, including the
JSX-style source note and the nico-core.png image entry, from the architecture
overview while leaving the surrounding section content intact.
- Around line 99-101: Narrow the “all resources” and “idempotent state-handling”
claims in the “State Machines” section to match the behavior documented by
state_handling.md, unless a complete cross-resource transition reference is
being added. Avoid asserting universal resource coverage or idempotency beyond
the documented scheduling and retry contract.
- Around line 24-28: Update the architecture overview to describe DPU components
as optional, removing the implication that every Managed Host has a DPU BMC or
runs DHCP on a DPU. Document the zero-DPU configuration using dpu_policy:
ignore, a primary HostInband NIC, and central NICo DHCP, and link to the
existing zero-DPU boot and lifecycle contract.
- Line 63: Update the managed-host boot behavior description in the architecture
overview to reflect that NICo may return a boot script, an exit script for a
provisioned OS, or an error script; HTTP chain failures open the iPXE error
menu, while local boot occurs only through the explicit localboot menu action.
Also document what happens when no local boot target exists, and remove the
inaccurate claim that the embedded script automatically tests for one.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 853fe7e9-236d-4c6b-84e4-8f4ca0eb83c5
📒 Files selected for processing (1)
docs/architecture/overview.md
zhaozhongn
approved these changes
Aug 12, 2026
Contributor
|
First set of suggestions for the diagram:
@polarweasel I'll research a bit more and get back to you with a few more suggestions. |
Contributor
|
Second set of suggestions. Sorry it's a lot, but I wanted to have all the information. Not all suggestions need to go into the diagram, since some connectors I'm suggesting might make the diagram very busy.
|
shayan1995
reviewed
Aug 12, 2026
shayan1995
reviewed
Aug 12, 2026
shayan1995
reviewed
Aug 12, 2026
shayan1995
reviewed
Aug 12, 2026
shayan1995
approved these changes
Aug 13, 2026
shayan1995
left a comment
shayan1995
left a comment
Contributor
There was a problem hiding this comment.
Language and terminology fixes look good — BlueField, InfiniBand, PostgreSQL, bare-metal, etc. are all corrected consistently. Removing the old site-controller-overview diagram is the right call. Left a couple of comments on content gaps but nothing blocking.
Contributor
Author
|
@thossain-nv I think I got all your changes into the diagram. Thanks so much for the very thorough review! As an aside, I feel like we're at the limit of what we can do with the current diagram, but I don't have the time to fully redo it at this point. Do we have anyone around who's really good at these things? Visual presentation of complex networks is definitely not my forte. 😬 |
thossain-nv
approved these changes
Aug 13, 2026
thossain-nv
left a comment
thossain-nv
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the diagram changes @polarweasel! It looks much better now.
polarweasel
enabled auto-merge (squash)
August 13, 2026 23:53
Signed-off-by: Alex Ball <aball@nvidia.com>
Signed-off-by: Alex Ball <aball@nvidia.com>
Signed-off-by: Alex Ball <aball@nvidia.com>
Signed-off-by: Alex Ball <aball@nvidia.com>
Signed-off-by: Alex Ball <aball@nvidia.com>
Signed-off-by: Alex Ball <aball@nvidia.com>
Signed-off-by: Alex Ball <aball@nvidia.com>
(i mean... it was almost the last update!) Signed-off-by: Alex Ball <aball@nvidia.com>
Signed-off-by: Alex Ball <aball@nvidia.com>
Signed-off-by: Alex Ball <aball@nvidia.com>
polarweasel
force-pushed
the
ab/arch-diagram-updates
branch
from
August 14, 2026 00:03
a229137 to
d353a7e
Compare
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
4 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.
Updating the architecture diagram, and removing the old and wrong NICo Site Controller overview diagram.
While I was in there, I did some lint/language fixes on the architecture overview page.
Related issues
Fixes internal bug ID 6376596.
Type of Change
Breaking Changes
Testing
Additional Notes