Reduce data that is transmitted and stored for memory devices - #4733
Reduce data that is transmitted and stored for memory devices#4733terickson-nvidia wants to merge 3 commits into
Conversation
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
🚥 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 |
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
|
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. |
8ce5f07 to
28c67ec
Compare
|
Temporarily taking out of draft to troubleshoot trufflehog false positive. Don't merge merge until tested on DPU. |
|
@coderabbitai review |
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
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
| total_mem += cap as u64 * mem.count as u64; | ||
| let key = (mem.mem_type.clone().unwrap_or_default(), cap); | ||
| mem_components | ||
| .entry(key.clone()) | ||
| .and_modify(|entry| entry.count += 1) | ||
| .and_modify(|entry| entry.count += mem.count) | ||
| .or_insert(SkuComponentMemory { | ||
| capacity_mb: key.1, | ||
| memory_type: key.0, | ||
| count: 1, | ||
| count: mem.count, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Use bounded arithmetic in both SKU aggregation paths.
The new count-aware aggregation uses unchecked additions. Use widened operands with saturating_add, or validate and reject overflowing counts before constructing SKU data.
crates/api-db/src/sku.rs#L554-L562: maketotal_memandentry.countoverflow-safe ingenerate_base_sku_from_hardware.crates/api-db/src/sku.rs#L414-L422: apply the same arithmetic policy ingenerate_sku_from_machine_at_version_0_or_1.
📍 Affects 1 file
crates/api-db/src/sku.rs#L554-L562(this comment)crates/api-db/src/sku.rs#L414-L422
🤖 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 `@crates/api-db/src/sku.rs` around lines 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.
| #[serde(default, deserialize_with = "deserialize_and_condense_memory_devices")] | ||
| pub memory_devices: Vec<MemoryDeviceGroup>, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the conversion and model surfaces before inspecting consumers.
ast-grep outline crates/rpc/src/model/hardware_info.rs --items all --type impl,function
ast-grep outline crates/api-model/src/hardware_info.rs --items all --type struct,function
# Find consumers that still read only the deprecated flat representation.
rg -n --type rust -C 4 '\.memory_devices\b|memory_devices:' crates rest-api
# Find grouped-memory readers and writers that require a coordinated rollout.
rg -n --type rust -C 4 '\.memory_device_groups\b|memory_device_groups:' crates rest-apiRepository: NVIDIA/infra-controller
Length of output: 203
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- API model ---'
sed -n '1,130p' crates/api-model/src/hardware_info.rs
printf '%s\n' '--- RPC model conversion ---'
sed -n '500,610p' crates/rpc/src/model/hardware_info.rs
printf '%s\n' '--- Host enumeration ---'
sed -n '850,950p' crates/host-support/src/hardware_enumeration.rs
printf '%s\n' '--- All flat-memory references ---'
rg -n --type rust -C 3 '\bmemory_devices\b' crates rest-api || true
printf '%s\n' '--- All grouped-memory references ---'
rg -n --type rust -C 3 '\bmemory_device_groups\b' crates rest-api || trueRepository: NVIDIA/infra-controller
Length of output: 40329
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- JSON memory types and deserializer ---'
sed -n '230,315p' crates/api-model/src/hardware_info.rs
rg -n -C 5 'message (DiscoveryInfo|MemoryDevice|MemoryDeviceGroup)|memory_device_groups|memory_devices' . -g '*.proto' -g '*.rs' -g '*.toml' | head -n 240
printf '%s\n' '--- Current change summary ---'
git diff --stat HEAD^
git diff -- crates/api-model/src/hardware_info.rs crates/rpc/src/model/hardware_info.rs crates/host-support/src/hardware_enumeration.rs crates/machine-a-tron/src/discovery_info.rs
printf '%s\n' '--- Repository references to compatibility and rollout ---'
rg -n -i -C 3 'memory.*(compat|legacy|deprecated|migration)|compat.*memory|non.break|mixed.version|rollback' crates rest-api README.md STYLE_GUIDE.md 2>/dev/null || trueRepository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Proto files ---'
fd -e proto . | head -n 80
printf '%s\n' '--- Memory schema declarations ---'
rg -n --glob '*.proto' -C 8 'memory_device_groups|memory_devices|message MemoryDevice' . || true
printf '%s\n' '--- Deserializer and memory model ---'
sed -n '250,310p' crates/api-model/src/hardware_info.rs
rg -n -C 6 'struct MemoryDevice(Group)?|impl.*MemoryDevice(Group)?|fn rehydrate|deserialize_and_condense' crates/api-model/src/hardware_info.rs
printf '%s\n' '--- Focused parent diff ---'
git diff --unified=25 HEAD^ -- crates/api-model/src/hardware_info.rs crates/rpc/src/model/hardware_info.rs crates/host-support/src/hardware_enumeration.rs crates/machine-a-tron/src/discovery_info.rs | sed -n '1,1000p'Repository: NVIDIA/infra-controller
Length of output: 24800
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Previous JSON model ---'
git show HEAD^:crates/api-model/src/hardware_info.rs | sed -n '25,75p'
git show HEAD^:crates/api-model/src/hardware_info.rs | sed -n '205,285p'
printf '%s\n' '--- Current serialization and JSONB boundaries ---'
rg -n -C 5 'HardwareInfo|hardware_info|serde_json|Jsonb|JSONB' crates/api-* crates/rpc/src/model/hardware_info.rs \
-g '*.rs' | grep -E 'hardware_info|HardwareInfo|serde_json|Jsonb|JSONB' | head -n 240
printf '%s\n' '--- Compatibility documentation around the new field ---'
sed -n '25,65p' crates/rpc/proto/machine_discovery.proto
sed -n '35,75p' rest-api/proto/core/src/v1/machine_discovery_nico.proto
printf '%s\n' '--- Legacy and grouped RPC conversion tests ---'
sed -n '2025,2205p' crates/rpc/src/model/hardware_info.rsRepository: NVIDIA/infra-controller
Length of output: 25482
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Memory migration history ---'
git log --oneline -20 -- crates/api-model/src/hardware_info.rs crates/rpc/proto/machine_discovery.proto
git log --all --oneline -S'memory_device_groups' -- crates/api-model/src/hardware_info.rs crates/rpc/proto/machine_discovery.proto | head -n 20
printf '%s\n' '--- Rollout and version support references ---'
rg -n -i --glob '*.md' --glob '*.rs' --glob '*.yaml' --glob '*.yml' \
'mixed.version|backward.compat|backwards.compat|rollback|wire.compat|deprecated.*memory|memory_device_groups|non.breaking' \
crates rest-api README.md STYLE_GUIDE.md 2>/dev/null | head -n 240 || true
printf '%s\n' '--- Exact JSON shape produced by current model ---'
sed -n '220,305p' crates/api-model/src/hardware_info.rsRepository: NVIDIA/infra-controller
Length of output: 26523
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Condensing change and affected writers ---'
git show --stat --oneline b9cec8b0
git show --format= --unified=12 b9cec8b0 -- \
crates/api-model/src/hardware_info.rs \
crates/rpc/src/model/hardware_info.rs \
crates/host-support/src/hardware_enumeration.rs \
crates/machine-a-tron/src/discovery_info.rs \
crates/rpc/proto/machine_discovery.proto | sed -n '1,1200p'Repository: NVIDIA/infra-controller
Length of output: 40246
Preserve compatibility for legacy memory readers.
When mixed-version deployments or rollback remain supported, populate memory_devices as well as memory_device_groups in outgoing DiscoveryInfo. Current writers send an empty legacy field, so legacy RPC clients receive no memory devices.
The JSONB representation changes flat entries into grouped objects with count. A legacy JSON reader treats each group as one device and undercounts memory when count > 1.
Use a capability-gated projection or a documented migration and rollout plan before treating this change as non-breaking. Retain the grouped-to-flat fallback for reads.
📍 Affects 4 files
crates/api-model/src/hardware_info.rs#L54-L55(this comment)crates/rpc/src/model/hardware_info.rs#L564-L570crates/host-support/src/hardware_enumeration.rs#L916-L918crates/machine-a-tron/src/discovery_info.rs#L232-L234crates/machine-a-tron/src/discovery_info.rs#L289-L291crates/machine-a-tron/src/discovery_info.rs#L356-L358crates/machine-a-tron/src/discovery_info.rs#L447-L449crates/machine-a-tron/src/discovery_info.rs#L580-L582
🤖 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 `@crates/api-model/src/hardware_info.rs` around lines 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.
Source: Path instructions
| let total = mem_info | ||
| .size_mb | ||
| .unwrap_or_default() | ||
| .saturating_mul(mem_info.count) as usize; | ||
|
|
||
| mem_map | ||
| .entry(name) | ||
| .and_modify(|e| { | ||
| *e = e.saturating_add(mem_info.size_mb.unwrap_or_default() as usize) | ||
| }) | ||
| .or_insert_with(|| mem_info.size_mb.unwrap_or_default() as usize); | ||
| .and_modify(|e| *e = e.saturating_add(total)) | ||
| .or_insert(total); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Use one explicit zero-count policy across memory aggregations.
MemoryDeviceGroup::rehydrate() treats count == 0 as no devices, but the aggregation paths emit zero-valued records. Skip zero-count groups before aggregation, or reject them at the model boundary.
crates/api-model/src/machine/capabilities.rs#L352-L360: skip zero-count groups before inserting intomem_map.crates/api-db/src/sku.rs#L414-L422: skip zero-count groups before updating SKU totals and components.crates/api-db/src/sku.rs#L554-L562: apply the same skip ingenerate_base_sku_from_hardware.
As per path instructions, grouped-memory changes must define and test “zero-count behavior”.
📍 Affects 2 files
crates/api-model/src/machine/capabilities.rs#L352-L360(this comment)crates/api-db/src/sku.rs#L414-L422crates/api-db/src/sku.rs#L554-L562
🤖 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 `@crates/api-model/src/machine/capabilities.rs` around lines 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.
Source: Path instructions
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Skip zero-count memory groups.
MemoryDeviceGroup::rehydrate() in crates/api-model/src/hardware_info.rs Lines 256-267 produces no devices when count == 0. This loop still inserts a 0 MB capability. Skip zero-count groups or reject them at the model boundary.
Proposed fix
for mem_info in hardware_info.memory_devices.iter() {
+ if mem_info.count == 0 {
+ continue;
+ }
let name = mem_infoAs per path instructions, grouped-memory changes must define and test “zero-count behavior”.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let total = mem_info | |
| .size_mb | |
| .unwrap_or_default() | |
| .saturating_mul(mem_info.count) as usize; | |
| mem_map | |
| .entry(name) | |
| .and_modify(|e| { | |
| *e = e.saturating_add(mem_info.size_mb.unwrap_or_default() as usize) | |
| }) | |
| .or_insert_with(|| mem_info.size_mb.unwrap_or_default() as usize); | |
| .and_modify(|e| *e = e.saturating_add(total)) | |
| .or_insert(total); | |
| if mem_info.count == 0 { | |
| continue; | |
| } | |
| let total = mem_info | |
| .size_mb | |
| .unwrap_or_default() | |
| .saturating_mul(mem_info.count) as usize; | |
| mem_map | |
| .entry(name) | |
| .and_modify(|e| *e = e.saturating_add(total)) | |
| .or_insert(total); |
🤖 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 `@crates/api-model/src/machine/capabilities.rs` around lines 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.
Source: Path instructions
| [2, 1, 4, 3] | ||
| ); | ||
| assert_eq!(discovery.memory_devices.len(), 2); | ||
| assert_eq!(discovery.memory_device_groups.len(), 1); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the complete memory group.
Line 979 only checks that one group exists. A wrong count, size, or memory type still passes. Assert the expected MemoryDeviceGroup value so this profile preserves its 983,040 MiB memory inventory.
As per path instructions, update tests for count preservation and round trips.
🤖 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 `@crates/machine-a-tron/src/discovery_info.rs` at 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.
Source: Path instructions
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
see also #2936
Type of Change
Breaking Changes
Testing
Additional Notes