Reduce data that is transmitted and stored for memory devices - #4733
Draft
terickson-nvidia wants to merge 8 commits into
Draft
Reduce data that is transmitted and stored for memory devices#4733terickson-nvidia wants to merge 8 commits into
terickson-nvidia wants to merge 8 commits into
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Summary by CodeRabbit
WalkthroughMemory inventory now uses grouped records with device counts. Protocols, discovery, RPC conversion, display formatting, capability totals, SKU generation, fixtures, and legacy compatibility handling were updated. ChangesGrouped memory device flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant HardwareDiscovery
participant DiscoveryInfo
participant RpcHardwareInfo
participant ManagedHostDisplay
HardwareDiscovery->>DiscoveryInfo: write memory_device_groups
DiscoveryInfo->>RpcHardwareInfo: convert grouped discovery data
RpcHardwareInfo->>ManagedHostDisplay: provide grouped memory data
ManagedHostDisplay->>ManagedHostDisplay: multiply sizes and counts
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-08-08 00:01:25 UTC | Commit: 28c67ec |
![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: 1
🤖 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 `@crates/rpc/proto/machine_discovery.proto`:
- Around line 206-210: Define and apply one explicit zero-count contract for
MemoryDeviceGroup across all grouped-memory paths. In
crates/rpc/proto/machine_discovery.proto lines 206-210 and
rest-api/proto/core/src/v1/machine_discovery_nico.proto lines 211-215, document
whether zero is invalid or normalizes to one; in
crates/api-model/src/hardware_info.rs lines 290-315 and
crates/rpc-utils/src/managed_host_display.rs lines 544-557, enforce that
contract before merging or calculating totals. In
crates/rpc/src/model/hardware_info.rs lines 2134-2184, add a table-driven
compatibility case covering zero counts and the chosen normalized or rejected
result.
🪄 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: bce7494d-d4b8-401d-aa89-79f2b15d1523
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/machine_discovery_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (11)
crates/api-db/src/sku.rscrates/api-model/src/hardware_info.rscrates/api-model/src/machine/capabilities.rscrates/api-model/src/test_support/machine_snapshot.rscrates/api-web/src/managed_host.rscrates/host-support/src/hardware_enumeration.rscrates/machine-a-tron/src/discovery_info.rscrates/rpc-utils/src/managed_host_display.rscrates/rpc/proto/machine_discovery.protocrates/rpc/src/model/hardware_info.rsrest-api/proto/core/src/v1/machine_discovery_nico.proto
terickson-nvidia
marked this pull request as draft
August 7, 2026 22:59
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
terickson-nvidia
force-pushed
the
tom/memory-devices
branch
from
August 7, 2026 23:26
8ce5f07 to
28c67ec
Compare
Contributor
Author
|
Temporarily taking out of draft to troubleshoot trufflehog false positive. Don't merge merge until tested on DPU. |
terickson-nvidia
marked this pull request as ready for review
August 7, 2026 23:58
Contributor
Author
|
@coderabbitai review |
Contributor
|
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@crates/api-db/src/sku.rs`:
- Around line 554-562: The SKU aggregation paths use unchecked arithmetic for
memory totals and component counts. In crates/api-db/src/sku.rs lines 554-562,
update generate_base_sku_from_hardware so total_mem and entry.count use widened
operands with saturating_add, or reject overflow before constructing SKU data;
apply the same policy in generate_sku_from_machine_at_version_0_or_1 at lines
414-422.
In `@crates/api-model/src/hardware_info.rs`:
- Around line 54-55: Preserve legacy compatibility across
crates/api-model/src/hardware_info.rs:54-55,
crates/rpc/src/model/hardware_info.rs:564-570,
crates/host-support/src/hardware_enumeration.rs:916-918, and
crates/machine-a-tron/src/discovery_info.rs:232-234, 289-291, 356-358, 447-449,
and 580-582 by populating both memory_devices and memory_device_groups in
outgoing DiscoveryInfo. Keep the grouped-to-flat fallback for reads, and use
capability gating or a documented migration/rollout plan so legacy clients
receive correctly expanded devices rather than one device per group when count
exceeds one.
In `@crates/api-model/src/machine/capabilities.rs`:
- Around line 352-360: Use a consistent zero-count policy for grouped-memory
aggregation: skip any MemoryDeviceGroup with count == 0 before updating results.
Apply this in crates/api-model/src/machine/capabilities.rs:352-360 around the
mem_map insertion, crates/api-db/src/sku.rs:414-422 before SKU totals/components
updates, and crates/api-db/src/sku.rs:554-562 within
generate_base_sku_from_hardware; define and add tests covering the zero-count
behavior.
- Around line 352-360: Update the memory capability aggregation around
MemoryDeviceGroup::rehydrate so groups with count == 0 do not insert or
contribute a zero-valued capability; skip them before calculating or updating
mem_map. Define the zero-count behavior explicitly and add or update tests
covering that no capability entry is produced for such groups.
In `@crates/machine-a-tron/src/discovery_info.rs`:
- Line 979: Update the test around the discovery memory group assertion to
compare the complete expected MemoryDeviceGroup, including count, size, memory
type, and the 983,040 MiB inventory, rather than checking only len(). Also
update the related tests to preserve the count and verify discovery round trips.
🪄 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: d5cd1f6f-28fb-45cd-910f-b9b8a6f8b90e
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/machine_discovery_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (11)
crates/api-db/src/sku.rscrates/api-model/src/hardware_info.rscrates/api-model/src/machine/capabilities.rscrates/api-model/src/test_support/machine_snapshot.rscrates/api-web/src/managed_host.rscrates/host-support/src/hardware_enumeration.rscrates/machine-a-tron/src/discovery_info.rscrates/rpc-utils/src/managed_host_display.rscrates/rpc/proto/machine_discovery.protocrates/rpc/src/model/hardware_info.rsrest-api/proto/core/src/v1/machine_discovery_nico.proto
terickson-nvidia
marked this pull request as draft
August 8, 2026 01:27
terickson-nvidia
force-pushed
the
tom/memory-devices
branch
from
August 10, 2026 16:08
d2c9358 to
b8351a6
Compare
Contributor
Author
|
The last commit condenses only consecutive ranges of memory devices with the same size+type, without empty slot placeholders, since that best matches the information we currently preserve: "populated slot N" but not "physical slot N". |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/api-model/src/hardware_info.rs`:
- Around line 311-316: The three memory-device count accumulation sites must
prevent u32 overflow by using saturating addition. Update the grouping logic
around MemoryDeviceGroup in crates/api-model/src/hardware_info.rs:283-291 and
311-316, plus the enumeration accumulation in
crates/host-support/src/hardware_enumeration.rs:929-937; add a regression test
covering merging counts u32::MAX and 1, preserving the resulting count at
u32::MAX.
🪄 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: 049385b3-6930-4838-8688-363b4f5ea091
📒 Files selected for processing (7)
crates/api-db/src/sku.rscrates/api-model/src/hardware_info.rscrates/host-support/src/hardware_enumeration.rscrates/machine-a-tron/src/discovery_info.rscrates/rpc-utils/src/managed_host_display.rscrates/rpc/src/model/hardware_info.rscrates/rpc/src/protos/mod.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/api-db/src/sku.rs
- crates/rpc-utils/src/managed_host_display.rs
- crates/machine-a-tron/src/discovery_info.rs
- crates/rpc/src/model/hardware_info.rs
terickson-nvidia
force-pushed
the
tom/memory-devices
branch
from
August 11, 2026 18:45
9882418 to
617b98a
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
1 participant
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.
For better scaling, this PR reduces the size of data transmitted and stored for memory devices. It adds a count field to {size, type} so that repeating MemoryDevice records can be condensed into a single MemoryDeviceGroup record. Wherever the code expects the MemoryDevice list, the condensed list is rehydrated from the new internal format.
Related issues
#4819
Type of Change
Breaking Changes
Testing
Additional Notes
A note about breaking changes:
If new code is installed, it will write condensed memory device groups, and the deprecated flat list of memory devices will be empty. If code is rolled back to an earlier version, it will read the empty list and the memory devices will be
lost.
But that's a re-enumeration problem, not a permanent data loss.
I don't think that counts as a breaking change since currently downgrade is not supported (see
docs/development/release_and_qa_process.md). Nothing is broken when upgrade applies this change. All the user-visible outputs remain the same; only the internal, persisted format is condensed, and deserialization handles both old and new formats.