feat(sku): validate v5 storage by drive size and PCI location#3717
Open
wminckler wants to merge 16 commits into
Open
feat(sku): validate v5 storage by drive size and PCI location#3717wminckler wants to merge 16 commits into
wminckler wants to merge 16 commits into
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:
WalkthroughSKU schema version 5 adds NVMe size and PCI-path metadata, carries it through RPC and discovery, generates per-drive storage constraints, and validates storage by PCI patterns and size ranges while retaining legacy matching for older schemas. ChangesSKU v5 drive-location validation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant HardwareEnumeration
participant NvmeDevice
participant GenerateSkuV5
participant DiffSkus
HardwareEnumeration->>NvmeDevice: populate size_mb and pci_path
NvmeDevice->>GenerateSkuV5: provide discovered NVMe inventory
GenerateSkuV5->>DiffSkus: provide schema-v5 Sku
DiffSkus->>DiffSkus: match PCI patterns and size bounds
DiffSkus-->>GenerateSkuV5: return storage diffs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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-07-20 15:32:14 UTC | Commit: 49ea651 |
![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
🧹 Nitpick comments (1)
crates/api-model/src/sku.rs (1)
657-927: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider table-driven tests for the new v5 matching/validation cases.
As per path instructions ("Testing: Prefer table-driven tests (e.g.,
scenarios!/check_cases) for validators and mapping logic—this PR's SKU/PATTERN validation and matching behavior should be covered via labeled input→expected-result cases."), the ~10 standalone#[test]functions here (exact match, missing location, extra drive, size out of range, broad-pattern starvation, invalid regex, unknown size, legacy v4) all funnel through the samev5_storage_diffs/diff_skuscall and differ mainly by input and an expected substring/emptiness assertion — a natural fit forcarbide-test-support'scheck_cases(given assertions vary between "is_empty" and "contains a substring," a rawscenarios!/value_scenarios!table may obscure intent, socheck_caseswith explicit case labels is likely the better fit here per the same guideline).🤖 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/sku.rs` around lines 657 - 927, The SKU matching tests should be consolidated into a table-driven test using carbide-test-support’s check_cases, covering the existing v5 and legacy v4 scenarios with labeled inputs and expected empty results or diagnostic substrings. Replace the standalone tests that exercise v5_storage_diffs/diff_skus with shared case data and explicit per-case expectations, preserving all current behaviors and assertions.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.
Inline comments:
In `@crates/api-db/src/sku.rs`:
- Around line 83-98: Update the error message constructed in
validate_storage_pci_patterns to begin with a lowercase phrase, while preserving
the existing pattern and regex error details and omitting any trailing period.
In `@crates/host-support/src/hardware_enumeration.rs`:
- Around line 228-251: Update get_nvme_size_mb to exclude partition devices from
the capacity sum by retaining only enumerated block devices whose devtype() is
"disk" before parsing and summing their size attributes. Preserve the existing
unknown-size handling and u32 conversion behavior.
---
Nitpick comments:
In `@crates/api-model/src/sku.rs`:
- Around line 657-927: The SKU matching tests should be consolidated into a
table-driven test using carbide-test-support’s check_cases, covering the
existing v5 and legacy v4 scenarios with labeled inputs and expected empty
results or diagnostic substrings. Replace the standalone tests that exercise
v5_storage_diffs/diff_skus with shared case data and explicit per-case
expectations, preserving all current behaviors and assertions.
🪄 Autofix (Beta)
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: c69234dd-f55a-4387-befd-bebe60d834d9
📒 Files selected for processing (13)
crates/api-core/src/tests/sku.rscrates/api-db/src/sku.rscrates/api-model/src/hardware_info.rscrates/api-model/src/hardware_info/test_data/x86_info.jsoncrates/api-model/src/sku.rscrates/api-model/src/test_support/machine_snapshot.rscrates/host-support/src/hardware_enumeration.rscrates/machine-a-tron/src/discovery_info.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/rpc/proto/machine_discovery.protocrates/rpc/src/model/hardware_info.rscrates/rpc/src/model/sku.rs
wminckler
force-pushed
the
feat/nvme-size-pci-2795
branch
from
July 20, 2026 20:42
5e9ad52 to
7fc6041
Compare
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3717.docs.buildwithfern.com/infra-controller |
kensimon
approved these changes
Jul 21, 2026
wminckler
force-pushed
the
feat/nvme-size-pci-2795
branch
from
July 21, 2026 18:03
7fc6041 to
e4eeef7
Compare
wminckler
force-pushed
the
feat/nvme-size-pci-2795
branch
from
July 21, 2026 21:09
e4eeef7 to
0f8f1d3
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
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 `@crates/api-db/src/sku.rs`:
- Around line 779-788: Update the NVMe mapping in the SKU generation flow around
SkuComponentStorage to reject any device whose size_mb or pci_path is missing
before creating a storage entry. Return an error identifying the affected NVMe
device, and only construct entries with validated capacity and PCI-location
metadata.
- Around line 764-769: Update the hardware-info lookup in the v5 SKU generator
to read from machine.status.hardware_info instead of machine.hardware_info.
Preserve the existing missing-value error behavior and subsequent hardware_info
handling.
🪄 Autofix (Beta)
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: b51b1f86-4079-4c5d-895f-0dbfc84fbee8
📒 Files selected for processing (13)
crates/api-core/src/tests/sku.rscrates/api-db/src/sku.rscrates/api-model/src/hardware_info.rscrates/api-model/src/hardware_info/test_data/x86_info.jsoncrates/api-model/src/sku.rscrates/api-model/src/test_support/machine_snapshot.rscrates/host-support/src/hardware_enumeration.rscrates/machine-a-tron/src/discovery_info.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/rpc/proto/machine_discovery.protocrates/rpc/src/model/hardware_info.rscrates/rpc/src/model/sku.rs
🚧 Files skipped from review as they are similar to previous changes (12)
- crates/rpc/build.rs
- crates/rpc/proto/machine_discovery.proto
- crates/api-core/src/tests/sku.rs
- crates/rpc/proto/forge.proto
- crates/api-model/src/test_support/machine_snapshot.rs
- crates/machine-a-tron/src/discovery_info.rs
- crates/api-model/src/hardware_info.rs
- crates/rpc/src/model/hardware_info.rs
- crates/api-model/src/hardware_info/test_data/x86_info.json
- crates/host-support/src/hardware_enumeration.rs
- crates/api-model/src/sku.rs
- crates/rpc/src/model/sku.rs
wminckler
force-pushed
the
feat/nvme-size-pci-2795
branch
2 times, most recently
from
July 22, 2026 14:40
90c6972 to
f6548ca
Compare
kensimon
approved these changes
Jul 22, 2026
wminckler
force-pushed
the
feat/nvme-size-pci-2795
branch
from
July 22, 2026 16:22
f6548ca to
43bcccb
Compare
kensimon
approved these changes
Jul 23, 2026
wminckler
force-pushed
the
feat/nvme-size-pci-2795
branch
from
July 23, 2026 15:32
7d41090 to
548f14e
Compare
Contributor
Author
|
the |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/api-model/src/sku.rs (1)
490-655: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSolid location-aware matching; global maximum matching is the correct choice here. The partition-then-saturate approach correctly prevents broad patterns from starving specific ones, and validating size post-assignment (as a diagnostic) keeps a right-location/wrong-size drive from being silently dropped. The fail-safe eligibility semantics are sound.
One observation for a follow-up (non-blocking): a constrained group whose pattern matches no drive emits both the per-pattern "not found" diff (Line 570) and the per-group "matched 0 at those PCI locations" diff (Line 621), producing two messages for a single root cause. Consider suppressing the count diff when a more specific per-pattern diff already covers the group.
🤖 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/sku.rs` around lines 490 - 655, Avoid duplicate diagnostics in diff_storage_by_location when a constrained group has no matching drive: track whether any valid PCI pattern for the group matches a discovered drive, and suppress the group-level “matched 0 at those PCI locations” count diff when none do. Preserve the existing per-pattern “not found” diagnostics and all count reporting for groups with at least one matching location.
🤖 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 `@crates/api-model/src/sku.rs`:
- Around line 490-655: Avoid duplicate diagnostics in diff_storage_by_location
when a constrained group has no matching drive: track whether any valid PCI
pattern for the group matches a discovered drive, and suppress the group-level
“matched 0 at those PCI locations” count diff when none do. Preserve the
existing per-pattern “not found” diagnostics and all count reporting for groups
with at least one matching location.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 01c48b0a-7752-4b0b-893b-e7b6e05d7657
⛔ Files ignored due to path filters (2)
rest-api/proto/core/gen/v1/machine_discovery_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.gorest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (16)
crates/admin-cli/src/sku/show/cmd.rscrates/api-core/src/tests/sku.rscrates/api-db/src/sku.rscrates/api-model/src/hardware_info.rscrates/api-model/src/hardware_info/test_data/x86_info.jsoncrates/api-model/src/sku.rscrates/api-model/src/test_support/machine_snapshot.rscrates/host-support/src/hardware_enumeration.rscrates/machine-a-tron/src/discovery_info.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/rpc/proto/machine_discovery.protocrates/rpc/src/model/hardware_info.rscrates/rpc/src/model/sku.rsrest-api/proto/core/src/v1/machine_discovery_nico.protorest-api/proto/core/src/v1/nico_nico.proto
🚧 Files skipped from review as they are similar to previous changes (9)
- crates/rpc/proto/machine_discovery.proto
- rest-api/proto/core/src/v1/nico_nico.proto
- crates/api-core/src/tests/sku.rs
- crates/api-model/src/hardware_info/test_data/x86_info.json
- crates/api-model/src/hardware_info.rs
- crates/rpc/src/model/sku.rs
- crates/host-support/src/hardware_enumeration.rs
- crates/api-model/src/test_support/machine_snapshot.rs
- crates/api-db/src/sku.rs
Carries drive capacity and sysfs device path through discovery (proto -> model -> RPC mapping -> host enumeration) so SKU validation can check drive size and PCI location. New fields are optional and serde-defaulted, so existing hardware inventories deserialize unchanged. Groundwork for SKU drive PCI/size validation (NVIDIA#2795). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Bill Minckler <wminckler@nvidia.com>
Adds SKU schema version 5, which validates storage by drive size range and PCI location instead of by model: - SkuComponentStorage gains min_size_mb, max_size_mb, and pci_patterns (regex). diff_skus branches on the expected SKU's schema version: v5 uses diff_storage_by_location (size range + count + PCI patterns, model ignored); v2-v4 keep the legacy model+count comparison. - Each expected v5 storage group must claim exactly count discovered drives via its PCI patterns (every pattern matching >=1 drive), each within the size range; unmatched drives are flagged. Fails safely on missing/ambiguous location, wrong size, or invalid pattern. - generate_sku_from_machine_at_version_5 emits one entry per NVMe drive (exact size as min==max, literal pci_path as pattern); an authored SKU can then widen the range or replace the path with a regex. - CURRENT_SKU_VERSION 4 -> 5; PCI patterns are compile-validated at SKU create/update time. - forge SkuComponentStorage proto + RPC mapping carry the new fields. Closes part of NVIDIA#2795. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Bill Minckler <wminckler@nvidia.com>
…nknown size Two v5 storage comparison fixes from code review: - Process location-constrained groups before generic (size-only) groups so a greedy generic group cannot claim a drive that only a constrained group can match, which produced false mismatches. - Treat an unknown drive size as matching only when no size bounds are configured; against an explicit min/max it is now a mismatch, matching the module's fail-safe contract. Adds regression tests for both. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Bill Minckler <wminckler@nvidia.com>
A broad location-constrained group (e.g. /nvme/) processed before a more specific one (e.g. 0000:04:00\.0) greedily claimed every matching drive, producing false count and missing-location diffs even when a valid overall assignment existed. Replace the greedy claim with a global maximum bipartite matching (Kuhn's algorithm) over expected-group slots so drives are assigned so as to saturate every group whenever possible. A pattern that matches no drive at all is now reported independently of assignment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Bill Minckler <wminckler@nvidia.com>
Regenerate the REST Core protobuf snapshots and Go bindings from crates/rpc/proto so they carry the new NvmeDevice.size_mb / pci_path and SkuComponentStorage.min_size_mb / max_size_mb / pci_patterns fields. Keeps the check-rest-core-proto-sync CI job green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Bill Minckler <wminckler@nvidia.com>
…ions - Lowercase the "invalid storage PCI pattern" DatabaseError Display phrase to match STYLE_GUIDE.md (errors are lowercase, no trailing period). - get_nvme_size_mb: filter enumerated block devices to DEVTYPE == "disk" so a partitioned NVMe's capacity isn't inflated by summing its partition sizes. match_parent walks the full block subtree, which includes partitions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Bill Minckler <wminckler@nvidia.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Bill Minckler <wminckler@nvidia.com>
…il NVMe size on unreadable namespace - v5 SKU path now reads machine.status.hardware_info to match the split machine record, consistent with earlier SKU versions - get_nvme_size_mb returns None when any namespace size is missing or unreadable rather than summing a partial total Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Bill Minckler <wminckler@nvidia.com>
A drive with no discovered size_mb or pci_path would produce an unconstrained v5 storage entry (open size range, empty pci_patterns) that validates any drive at any location or capacity. Because a generated SKU can be persisted as an expected SKU, incomplete discovery could silently author a permissive v5 SKU. Fail generation instead, naming the affected drive, consistent with the branch's fail-safe stance on unknown drive size. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The prior regeneration used buf 1.54, which formats generated Go struct-tag comments differently than the buf 1.70.0 pinned by CI (.github/workflows/rest-lint-and-test.yml), leaving the DNS question field comments misaligned and a stray trailing "//". Regenerate with the CI-matched buf version so the committed bindings match the proto sync check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Populate size_mb and a distinct per-drive pci_path for every NVMe device in the mock host fixtures, via nvme_size_mb/nvme_pci_path helpers. Without these, v5 SKU generation rejects the mock hosts for missing drive size or PCI location, so discovery-driven SKU flows can't be exercised against them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The storage table in the SKU detail view only rendered Model and Count, omitting the schema-v5 per-drive size range and PCI patterns. Add Size (MB) and PCI Patterns columns, using the same min-max / "*" convention as SkuComponentStorage's Display; both render empty for pre-v5 SKUs that don't set them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The field is named size_mb but holds MiB: the real get_nvme_size_mb divides the 512-byte sysfs sector count by 2048, and 512 * 2048 = 1,048,576 = 1 MiB. Label the `sku show` column "Size (MiB)" so the value reads truthfully (e.g. 3662840 = 3.84 TB, not 3.66 TB). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…drive When a constrained storage group's PCI pattern matched no drive, diff_storage_by_location emitted both a per-pattern "... not found" diff and a redundant group-level "matched 0 at those PCI locations" diff for the same root cause. Suppress the group-level count diff when none of the group's patterns matched a drive; keep it when at least one location matched but the group is still short. Adds tests for both paths. Addresses a CodeRabbit review nitpick. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
wminckler
force-pushed
the
feat/nvme-size-pci-2795
branch
from
July 23, 2026 19:51
4926e29 to
bfb8f0c
Compare
…onal Commit cd37f83 (NVIDIA#4003) changed StaticMachineEndpoint.id to Option<String> and updated the real constructors in endpoint/sources.rs, but the static_machine() test helper still passed a bare String, which broke the carbide-health lib-test build under clippy --all-targets. Wrap it in Some to match the field type. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
nvlitagaki
added a commit
to nvlitagaki/infra-controller
that referenced
this pull request
Jul 23, 2026
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
2 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.
Description
SKU verification previously compared storage only by drive model and count. That is too coarse for fleets where a chassis can be populated with drives of the same model but different capacities, or where a specific NVMe must sit at a specific PCI location. This PR introduces SKU schema v5, which records one storage entry per NVMe drive carrying its capacity (
size_mb) and concrete sysfs/PCI path (pci_path), and validates an expected SKU against discovered hardware by drive size range and PCI location rather than model.The comparison fails safe: it emits a diff (rather than silently matching) when an expected drive is missing, a location is ambiguous, a drive is the wrong size, a discovered drive is unexpected, or a drive's size could not be read. Expected drives are assigned to expected groups with a global maximum matching so a broad location group (e.g.
/nvme/) never starves a more specific one (e.g.0000:04:00\.0) when a valid overall assignment exists. Uncompilable PCI patterns are rejected at SKU authoring time.Related issues
Closes #2795
Type of Change
Breaking Changes
Older SKU schema versions remain supported and are still compared by model; v5 is generated only for new SKUs at
CURRENT_SKU_VERSION.Testing
Unit tests added/updated
Integration tests added/updated
Manual testing performed
No testing required (docs, internal refactor, etc.)
Unit tests in
crates/api-model/src/sku.rscover exact location+size match, missing/extra/wrong-size drives, invalid patterns, size-only groups, unknown drive size, and the broad-vs-specific group assignment (regression for the global-matching fix).sqlx-backed suite in
crates/api-core(SKU create/replace/verify/generate, version compatibility) passes.Manually verified against a local env via
nico-admin-cli: v5 generation, create-time duplicate detection driving the new matching (a broad/nvme/+ specific0000:04:00\.0expected SKU correctly matches the two-drive discovered layout), a genuinely non-matching layout accepted as distinct, and invalid-regex patterns rejected at create time.Additional Notes
Reviewer guidance: the core comparison logic is
diff_storage_by_locationincrates/api-model/src/sku.rs(global bipartite matching via Kuhn's algorithm over expected-group slots). v5 generation isgenerate_sku_from_machine_at_version_5incrates/api-db/src/sku.rs;NvmeDevicegainssize_mb/pci_pathsourced from host hardware enumeration.