fix(collector): emit lossless label/taint readings - #2093
Conversation
Recipe evidence checkNo leaf overlays affected by this PR. This gate is warning-only and never blocks merge. |
|
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:
📝 WalkthroughWalkthroughNode topology collection now emits lossless label and taint items with explicit context, node membership, counts, and truncation metadata. New accessors decode item-based readings and legacy folded maps. Templates, GPU consumers, fingerprint extraction, snapshot comparison, and redaction support both representations. Documentation and tests cover collisions, malformed data, ordering, truncation, and compatibility. Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
Actionable comments posted: 3
🤖 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 `@pkg/client/v1/gpu_driver_state.go`:
- Around line 420-424: Update hasMultipleValues to use exact-key matching for
node.kubernetes.io/instance-type while preserving child-key prefix matching for
nvidia.com/gpu.*. Add a lossless regression case covering multiple values under
an instance-type.* label without the exact instance-type key, ensuring it does
not report a heterogeneous GPU pool.
In `@pkg/collector/topology/readings.go`:
- Around line 200-214: Update itemNodes to require node-count and truncated
metadata for Items, validating both types and rejecting malformed values instead
of applying legacy defaults. Ensure node-count is nonnegative and at least the
visible node count, and require truncated to agree with
IsTruncatedNodeList(list), including rejecting false when the rendered suffix
indicates truncation. Add regression coverage for each invalid or inconsistent
metadata state.
- Around line 149-152: Update taintReadingsFromItems to require a non-empty
context.effect and validate it against the supported taint effects before
appending each TaintReading; return an error for missing, empty, or unsupported
values. Add malformed-item tests covering missing/empty effects and invalid
effect values.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 40b760c7-2117-46f6-bbba-799f48b7cccd
📒 Files selected for processing (19)
.agents/skills/aicr-analyzing-snapshots/SKILL.mddocs/contributor/collector.mddocs/integrator/data-flow.mddocs/integrator/measurement-api.mdexamples/templates/snapshot-template.md.tmplpkg/client/v1/gpu_driver_state.gopkg/client/v1/gpu_driver_state_test.gopkg/collector/topology/readings.gopkg/collector/topology/readings_test.gopkg/collector/topology/template_test.gopkg/collector/topology/topology.gopkg/collector/topology/topology_test.gopkg/constraints/gpu_nodes.gopkg/constraints/gpu_nodes_test.gopkg/evidence/redact/redact_test.gopkg/fingerprint/from_measurements.gopkg/fingerprint/from_measurements_test.gopkg/snapshotter/snapshot.gopkg/snapshotter/snapshot_test.go
Coverage Report ✅
Coverage BadgeMerging this branch changes the coverage (3 decrease, 3 increase)
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. |
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 `@pkg/collector/topology/readings.go`:
- Around line 206-235: Update the validation in splitNodeList to compare
node-count only with the number of named nodes, excluding the retained
truncation marker from len(nodes), while still returning the marker in Nodes.
Preserve the existing truncated and complete consistency checks, and add a
regression case covering a list such as n1,n2 (+1 more) with node-count 3.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 15b6bb5e-84e9-40bd-8f42-47f2fb6fa575
📒 Files selected for processing (7)
docs/integrator/measurement-api.mdpkg/client/v1/gpu_driver_state.gopkg/client/v1/gpu_driver_state_test.gopkg/collector/topology/readings.gopkg/collector/topology/readings_test.gopkg/evidence/redact/redact_test.gopkg/fingerprint/from_measurements_test.go
mchmarny
left a comment
mchmarny
left a comment
There was a problem hiding this comment.
Requesting changes: 2 correctness blockers in the lossless topology Items path. I reviewed exact head 7be2ea3d0b70ba8b9cb6988352a488d0325e1d35 across collector encoding/decoding, the GPU-node constraint, fingerprint consumers, the shipped template, the legacy fallback, and live CI. Checks are green, but the inline issues can accept contradictory or internally inconsistent topology data.
Coverage passes overall at 82.2%; the reported package deltas for pkg/constraints (-0.77%) and pkg/fingerprint (-1.93%) also exceed the repository 0.5% flag threshold.
| "regenerate the snapshot with a current aicr build", r.RawKey, r.Nodes), | ||
| map[string]any{ctxConstraint: GPUNodesLabelConstraintName, ctxReading: r.RawKey}) | ||
| } | ||
| entries = append(entries, labelNodeSet{ |
There was a problem hiding this comment.
Blocking: this Items path never enforces the Kubernetes invariant that a node has only one value for a given label key. A structurally valid snapshot can list gpu-a under both key=true and key=false; evaluateEveryGPUNodeHasValue ignores the false entry and returns PASS because the true entry covers the universe. The legacy Data path rejects overlapping value sets, so making Items authoritative weakens the failure direction. Reject duplicate node membership within and across readings for the same key, and add a regression asserting ErrCodeInvalidRequest for conflicting Items.
| fmt.Sprintf("topology item %d: %q is %v but %q %s the truncation marker", | ||
| idx, itemDataTruncated, truncated, itemDataNodeList, | ||
| map[bool]string{true: "carries", false: "does not carry"}[marker])) | ||
| case truncated && count <= len(nodes): |
There was a problem hiding this comment.
Blocking: truncated Items only require node-count > len(nodes), so node-list: "n1,n2 (+3 more)", node-count: 3, truncated: true is accepted even though the suffix implies five nodes. countGPUNodesFromItems then records 3 while the rendered list says 5, violating the documented true-total contract. Parse the +N suffix and require node-count == visible names + N; cover both lower and higher mismatches.
yuanchen8911
left a comment
yuanchen8911
left a comment
There was a problem hiding this comment.
The lossless items encoding is well built where it counts: the legacy taint/label Data encoders are unchanged (non-collision output verified byte-identical; summary counts now derive from items, so collision-cluster summaries correct their previous undercount), the shared topology.LabelReadings/TaintReadings decoder keeps the fallback branch in one place for Go consumers (the shipped template implements its own fallback), and the collision-determinism and legacy-compat coverage is thorough. Verified at 7be2ea3, CI green.
There are two blocking issues, both at the boundaries rather than in the core logic.
First, the upgrade path breaks the documented drift gate: a baseline captured before this release has no items, pkg/diff compares items unconditionally, and aicr diff --fail-on-drift then exits with a conflict on an unchanged cluster — hundreds of false adds. For the label/taint subtypes, treat items as authoritative and skip data when both snapshots carry items; otherwise compare data and suppress the one-sided items representation. Mixed-vintage comparisons must also normalize the representation-sensitive summary label/taint counts, which otherwise still trip the gate on collision clusters. This removes upgrade-only drift; when both snapshots carry items, ignoring folded data also stops collision flapping, while legacy-vs-legacy comparisons retain the pre-existing limitation.
Second, emitting the full node list in both data and items roughly doubles the topology payload; with the default --max-nodes-per-entry=0 and agent mode's mandatory ConfigMap handoff (plus the reader's own 1 MiB cap), a large-cluster snapshot that fit before this change can now fail at capture — an independent 100-node/150-label probe measured the topology YAML alone growing from ~548 KB to ~1.12 MB.
Two smaller boundary-validation issues follow the repo's fail-closed rule for externally produced snapshots (measurement-api.md is a documented integrator contract): the items decoder accepts a node listed under two values of the same label key, where the legacy decoder rejects overlapping node sets; and the truncation check never parses the "(+N more)" count, so an item whose node-count contradicts its own marker is accepted.
Two additional consistency gaps affect the legacy fallback: NodeCount is not the documented true total when truncated, and legacy taint decoding leaves the synthesized .Effect suffix attached to the logical Key.
The rest is test-depth and rendering polish, inline.
| ). | ||
| WithSubtype(measurement.Subtype{Name: "taint", Data: taintData}). | ||
| WithSubtype(measurement.Subtype{Name: "label", Data: labelData}). | ||
| WithSubtype(measurement.Subtype{Name: "taint", Data: taintData, Items: taintItems}). |
There was a problem hiding this comment.
Emitting items makes every pre-upgrade baseline report drift against a post-upgrade snapshot of the same unchanged cluster. compareItems (pkg/diff/diff.go:220) runs unconditionally, so a nil-items baseline yields an items.length change plus one added-change per item field, and aicr diff --fail-on-drift returns a conflict — the exact CI pattern documented in docs/integrator/automation.md.
Suggested rule for pkg/diff: for the label/taint subtypes, treat items as authoritative and skip data when both snapshots carry items; otherwise compare data and suppress the one-sided items representation. Mixed-vintage comparisons must also normalize the representation-sensitive summary label/taint counts — a pre-upgrade collision snapshot has undercounted summary.label-count/taint-count, so the corrected counts alone would still trip the gate. This removes upgrade-only drift; ignoring folded data when both sides carry items also stops same-version collision flapping (encodeTaints/encodeLabels iterate unordered maps with last-writer-wins on folded-key collisions), while legacy-vs-legacy comparisons retain the pre-existing limitation.
| WithSubtype(measurement.Subtype{Name: "taint", Data: taintData}). | ||
| WithSubtype(measurement.Subtype{Name: "label", Data: labelData}). | ||
| WithSubtype(measurement.Subtype{Name: "taint", Data: taintData, Items: taintItems}). | ||
| WithSubtype(measurement.Subtype{Name: "label", Data: labelData, Items: labelItems}). |
There was a problem hiding this comment.
The items entries duplicate the full node-list strings already carried in data, roughly doubling the topology subtype. With the default --max-nodes-per-entry=0 this is unbounded, agent mode always hands the snapshot through a ConfigMap (pkg/snapshotter/agent.go:660), and the ConfigMap read path caps at 1 MiB (pkg/serializer/reader.go:719). A 100-node/150-label probe measured the topology YAML growing from 548,124 to 1,115,885 bytes — past the limit before any other measurement is included. Snapshots that fit before this change can now fail at the internal handoff; the payload needs to stay bounded (e.g. don't carry the same membership strings twice).
There was a problem hiding this comment.
I agree with items cross-referencing data for v1alpha2.
Slimming data is not safe: pre-PR binaries read it directly. Their node-set evaluator rejects truncated lists, while fingerprinting does not recover the (+N more) count. An older same-version consumer could therefore fail validation or undercount GPU nodes. ADR-011 requires the existing data encoding and semantics to remain unchanged.
For the cross-reference:
- Omit node-list only when the folded key uniquely identifies one item; collisions keep the inline list.
- Use an explicit reference to the data entry. An omitted node-list without that reference must fail decoding.
- Centralize hydration and validation in LabelReadings and TaintReadings, checking uniqueness, identity, count, and truncation.
- In pkg/diff, compare hydrated items when both snapshots have them; retain data normalization for mixed-vintage comparisons.
Agreed on removing data in the next snapshot apiVersion. New emissions can then use self-contained items, while the v1alpha2 compatibility decoder remains through its acceptance window. Worth documenting that transition in measurement-api.md.
| "regenerate the snapshot with a current aicr build", r.RawKey, r.Nodes), | ||
| map[string]any{ctxConstraint: GPUNodesLabelConstraintName, ctxReading: r.RawKey}) | ||
| } | ||
| entries = append(entries, labelNodeSet{ |
There was a problem hiding this comment.
decodeLabelEntriesFromItems appends every key-matching entry with no overlap check, while decodeLabelEntriesFromData requires accepted entries to partition their nodes (fail-closed, per its own comment). An externally produced snapshot listing one GPU node under both value=true and value=false lets evaluateEveryGPUNodeHasValue pass on the true entry alone. Since measurement-api.md is a documented producer contract, this ambiguous shape should return ErrCodeInvalidRequest like the legacy path — the overlap check ports over cheaply.
| // taintReadingsFromData decodes the legacy taint encoding, whose two shapes | ||
| // are told apart by field count: "<effect>|<value>|<nodes>" for a plain key, | ||
| // "<value>|<nodes>" for a disambiguated one where the effect is the key suffix. | ||
| func taintReadingsFromData(data map[string]measurement.Reading) []TaintReading { |
There was a problem hiding this comment.
In the two-field legacy fallback, taintReadingsFromData extracts Effect from the key.Effect suffix but leaves the suffix attached to Key, so legacy decoding returns Key "node.kubernetes.io/unreachable.NoExecute" where the items path returns "node.kubernetes.io/unreachable" — the two encodings disagree on the logical key for routine multi-effect taints.
The two-field encoding itself proves the final suffix was synthesized by encodeTaints, and the decoder already relies on that suffix to populate Effect — so it should remove the same suffix from Key, with RawKey preserving the original folded map key. Unlike the label case (where the verbatim-Key limitation is documented and genuinely ambiguous), no closed-set assumption is needed here. No in-repo consumer reads TaintReadings yet, but this is a newly exported contract.
| fmt.Sprintf("topology item %d: %q is %v but %q %s the truncation marker", | ||
| idx, itemDataTruncated, truncated, itemDataNodeList, | ||
| map[bool]string{true: "carries", false: "does not carry"}[marker])) | ||
| case truncated && count <= len(nodes): |
There was a problem hiding this comment.
The truncation consistency check requires count > len(visible) but never parses N from the "(+N more)" marker, so node-list "n1,n2 (+3 more)" with node-count 3 is accepted although the list itself claims 5 total. countGPUNodesFromItems then fingerprints from the inconsistent count. The marker format is fixed — parse N and require count == visible + N.
| // Nodes carry Key=Value. When Truncated, the list is incomplete and its | ||
| // last element holds the "(+N more)" marker. | ||
| Nodes []string | ||
| // NodeCount is the true total, including nodes omitted by truncation. |
There was a problem hiding this comment.
The struct doc says NodeCount is "the true total, including nodes omitted by truncation", but on the data fallback path (readings.go:306, :345) it is len(nodes) from the rendered list — the visible count only. No current consumer reads NodeCount from a legacy-data subtype, so this is latent, but the exported field should not have encoding-dependent semantics: prefer recovering the true total on the fallback path using the same parsed "(+N more)" marker, rather than only documenting that consumers must check Truncated first.
| {{- if eq (len $parts) 3 }} | ||
| | {{ $key }} | {{ index $parts 0 }} | {{ index $parts 1 }} | {{ index $parts 2 }} | | ||
| {{- else }} | ||
| | {{ $key }} | | {{ index $parts 0 }} | {{ index $parts 1 }} | |
There was a problem hiding this comment.
For a legacy data-only snapshot where one taint key carries multiple effects (the routine node.kubernetes.io/unreachable NoExecute/NoSchedule pair), the 2-part branch renders the synthesized "key.Effect" map key in the Taint Key column and leaves Effect blank. This branch is an improvement — the previous template hard-errored on 2-part readings — but the 2-part shape itself proves the trailing suffix is an encoded effect, so split the encoded effect suffix and render both columns correctly.
| t.Errorf("reading %d = {%q %q n=%d}, want {%q %q n=%d}", | ||
| i, got.Key, got.Value, got.NodeCount, w.Key, w.Value, w.NodeCount) | ||
| } | ||
| if len(got.Nodes) != len(w.Nodes) { |
There was a problem hiding this comment.
TestLabelReadingsRecoversEveryReading compares only len(got.Nodes) against len(w.Nodes); the exact node names in the want fixtures are never asserted (the taint test does compare contents). A decode bug attributing the right number of wrong nodes would pass.
| tt.mutate(&item) | ||
| st := &measurement.Subtype{Name: "label", Items: []measurement.ItemEntry{item}} | ||
| if _, err := LabelReadings(st); err == nil { | ||
| t.Error("LabelReadings() error = nil, want a decode error") |
There was a problem hiding this comment.
The malformed-item cases assert only err != nil, not that the error carries ErrCodeInvalidRequest via stderrors.Is. Callers are insulated today (pkg/constraints re-wraps decoder failures with its own ErrCodeInvalidRequest), but the exported decoder's error-code contract should be pinned by its own tests.
|
|
||
| out := render(t) | ||
| for _, want := range []string{ | ||
| "NVIDIA-H100-80GB-HBM3", |
There was a problem hiding this comment.
The fixture puts NVIDIA-B200 on gpu-b (line 62) but the rendered-output want list only checks NVIDIA-H100-80GB-HBM3 — the second value of the same label key is never asserted, which is the exact multi-value case the items encoding exists for.
Fixes #2003 Signed-off-by: Varun Ramesh <varamesh@nvidia.com>
Match node.kubernetes.io/instance-type exactly rather than by prefix. It is a complete label key, not a family, so a distinct label sitting under its prefix was counted as one of its values and reported a heterogeneous GPU pool on a uniform cluster. nvidia.com/gpu keeps child matching, which GFD's label family requires. Require effect on taint items. A missing field decoded to an empty Effect that looked valid and would satisfy a caller matching on one. The value is not checked against the known effects so a snapshot from a newer Kubernetes stays readable. Require node-count and truncated, and cross-check all three node fields against each other: truncated must match the "(+N more)" marker, and node-count must equal the names in node-list when complete or exceed them when truncated. Both count directions are rejected -- too low understates the cluster, too high lets a consumer read a partial list as complete. The previous code silently ignored a mistyped field and let a declared truncated:false override a visibly truncated list. Two test fixtures asserted shapes the collector cannot emit and are corrected: one rendered a truncated list without the marker, the other omitted node-count and truncated entirely. Signed-off-by: Varun Ramesh <varamesh@nvidia.com>
The "(+N more)" suffix states how many names were withheld, so an item's node-count is checkable exactly rather than by direction. The previous rule only required the count to exceed the names rendered, accepting "n1,n2 (+3 more)" with node-count 3 where the list itself states 5; pkg/fingerprint then reported that count as the cluster's GPU node total. Parse N and require node-count == names + N, which also subsumes the complete-list case as N == 0. Recover the same total on the data fallback. NodeCount is documented as the pre-truncation total, but the legacy path reported only the names it could see, so one exported field meant two different things depending on which encoding a snapshot carried. The marker carries the number on that path too. Split the encoded effect out of the folded key when rendering a legacy taint. Two fields exist only because encodeTaints moved the effect into the key, so the trailing segment is recoverable rather than ambiguous; the Taint Key column no longer shows "<key>.<effect>" with Effect blank. Test hardening: assert the decoder's error code rather than only that an error occurred, compare node names rather than their count, and assert both values of the multi-value label the item encoding exists for. Each previously passed whether or not the behaviour held. Signed-off-by: Varun Ramesh <varamesh@nvidia.com>
A node carries exactly one value of a given label key, so the readings for one key must partition their nodes. The item path appended every key-matching entry without checking, so a snapshot listing gpu-a under both key=true and key=false decoded cleanly, and evaluateEveryGPUNodeHasValue skipped the contradicting entry and returned PASS on a cluster that cannot exist. The folded path already rejects this, and items win whenever present, so a working guard had stopped running rather than merely being absent. The collector cannot emit the shape — it aggregates through a map — but this change publishes the item format for external producers, and measurement-api.md is a contract we now have to validate against. Compared on value rather than reappearance: a node repeated under one value is redundant, not contradictory, and the folded path accepts it. Rejecting it would open a fresh divergence between the encodings while closing one. The rule is labels-only; a node legitimately carries the same taint key with two effects, so a per-key rule would reject any NotReady node. Signed-off-by: Varun Ramesh <varamesh@nvidia.com>
Emitting items makes an aicr upgrade read as cluster drift: a baseline captured by an older build has no items, compareItems runs unconditionally, and every field of every item reports as added. The summary counts disagree too, because this branch sizes them off the items while older builds sized them off the folded map. Measured on an unchanged cluster, an upgrade produced 6 changes with no collision and 17 with one, so aicr diff --fail-on-drift fails the morning after an upgrade. Reduce both sides to the representation they share before comparing. When both carry items, drop the folded map: items are authoritative, and the map is not stable even within one build, since a key collision resolves by map iteration order. When exactly one carries items, drop items and restate the summary count over the folded map, which is the basis the older build used. When neither does, change nothing — there is no mismatch to reconcile, and restating would mask a corrupted count rather than report it. Rollback is the same problem mirrored and is handled by the same rule. Scoped to NodeTopology: elsewhere a one-sided item list is a real difference. Inputs are copied, since callers render a snapshot after comparing it. Signed-off-by: Varun Ramesh <varamesh@nvidia.com>
7be2ea3 to
685a787
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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 `@pkg/collector/topology/readings.go`:
- Around line 327-345: Update the legacy two-field decoding branch around the
`strings.LastIndex(key, ".")` logic to set the emitted `TaintReading.Key` to the
prefix before the final dot when extracting the synthesized effect, while
preserving the original key in `RawKey`. Add a Data-only regression test
covering one taint key with two effects and verifying the logical key, raw key,
and effects match the Items path.
In `@pkg/collector/topology/topology.go`:
- Around line 123-136: Update the topology measurement construction around the
TypeNodeTopology subtypes so node membership is not emitted unboundedly in both
Data and Items when MaxNodesPerEntry is zero. Apply a snapshot-safe cap to the
duplicated payload or remove the redundant membership copy, while preserving the
required compatibility path and existing count semantics.
- Around line 314-319: Update truncatedNodeListRemainder to return an error when
strconv.Atoi cannot parse the matched suffix, including integer overflow,
instead of returning (0, false). Propagate that error through itemNodes so
malformed matched suffixes fail closed, and add a regression test covering an
overflowing remainder such as “+9223372036854775808 more”.
In `@pkg/constraints/gpu_nodes.go`:
- Around line 213-220: The comment in the truncated-reading branch should state
that truncated readings are rejected because node-set constraints require a
complete node list, not that token validation is skipped. Update the comment
directly above the `if r.Truncated` check and leave the existing
`ErrCodeInvalidRequest` return behavior unchanged.
In `@pkg/diff/topology.go`:
- Around line 72-73: Update the mixed-vintage branch around baseHas != targetHas
in the topology diff logic so independently folded legacy Data maps are not
compared as authoritative values. Normalize legacy entries against structured
Items with collision-aware handling, or exclude ambiguous folded entries,
ensuring unchanged collections cannot report drift when collision winners
differ; add a regression test covering legacy and current snapshots with
different collision winners.
In `@pkg/snapshotter/snapshot.go`:
- Around line 325-330: Update the measurement loop in hasGPUNodesInTopology to
skip nil entries before accessing m.Type, matching the nil guard used by the
equivalent topology loop in gpu_driver_state.go. Preserve the existing filtering
and topology label-reading behavior for non-nil measurements.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: efe03923-0603-4193-94ef-329fa9eb5a8a
📒 Files selected for processing (22)
.agents/skills/aicr-analyzing-snapshots/SKILL.mddocs/contributor/collector.mddocs/integrator/data-flow.mddocs/integrator/measurement-api.mdexamples/templates/snapshot-template.md.tmplpkg/client/v1/gpu_driver_state.gopkg/client/v1/gpu_driver_state_test.gopkg/collector/topology/readings.gopkg/collector/topology/readings_test.gopkg/collector/topology/template_test.gopkg/collector/topology/topology.gopkg/collector/topology/topology_test.gopkg/constraints/gpu_nodes.gopkg/constraints/gpu_nodes_test.gopkg/diff/diff.gopkg/diff/topology.gopkg/diff/topology_test.gopkg/evidence/redact/redact_test.gopkg/fingerprint/from_measurements.gopkg/fingerprint/from_measurements_test.gopkg/snapshotter/snapshot.gopkg/snapshotter/snapshot_test.go
Items whose fold key is unambiguous (only one reading maps to it) carry node-list-ref pointing at the data entry rather than repeating the node names inline. This keeps items additive without doubling snapshot size — a 1000-node/40-label probe grows from 602 KB to 610 KB (×1.01) instead of 1.21 MB (×2.01, past the 1 MiB CM cap). Colliding readings keep their own inline node-list since the shared data entry describes only one of them unpredictably. Also in this commit: - pkg/diff: hydrate references before comparing items; exclude collision-ambiguous keys from mixed-vintage folded-map comparison - pkg/snapshotter: nil guard on hasGPUData measurement loop - HydrateItems: defensive length guard against readings/items mismatch - docs/measurement-api.md: fix label-count in worked example (1→3) - skill file: document node-list-ref alongside node-list Signed-off-by: Varun Ramesh <varamesh@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/integrator/measurement-api.md (1)
303-308: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCorrect the legacy
datadescription.The statement that the
taintandlabelsubtypes carry every reading twice is incorrect. The legacydatamap is lossy when folded keys collide. The example has threeitemsentries but only twodatakeys. State thatitemscontains every reading and thatdatais a compatibility representation that can drop collisions.Proposed wording
-The `taint` and `label` subtypes carry every reading twice — as `items` (lossless) -and as the legacy folded `data` map. +The `taint` and `label` subtypes expose readings as lossless `items`; they also +retain the legacy folded `data` map, which can drop colliding readings.🤖 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/integrator/measurement-api.md` around lines 303 - 308, Update the NodeTopology shape documentation to state that taint and label subtypes retain every reading in items, while the legacy data map is a compatibility representation that may lose readings when folded keys collide. Remove the claim that both fields contain every reading twice.
🤖 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 `@pkg/collector/topology/readings_test.go`:
- Around line 573-623: Extend TestReadingsRejectMalformedReference with a
two-item subtype case whose item keys fold onto the same data key while one item
retains itemDataNodeRef, and have it mutate the existing fixture accordingly.
Ensure the case exercises resolveNodeList’s folds[ref] != 1 guard and continues
asserting LabelReadings returns a rejected decode.
In `@pkg/collector/topology/readings.go`:
- Around line 550-568: Document the order contract in both
labelReadingsFromItems and taintReadingsFromItems, stating that each decoder
preserves the original item order so HydrateItems can safely pair nodes[i] with
st.Items[i]. Do not change the existing index-based hydration logic.
- Around line 445-447: Update the default branch of the legacy taint parsing
logic to reject one-field entries by returning an invalid-request error instead
of assigning parts[0] to value. Preserve the existing handling for the
documented two- and three-field encodings and ensure malformed input does not
produce a zero-node reading.
---
Outside diff comments:
In `@docs/integrator/measurement-api.md`:
- Around line 303-308: Update the NodeTopology shape documentation to state that
taint and label subtypes retain every reading in items, while the legacy data
map is a compatibility representation that may lose readings when folded keys
collide. Remove the claim that both fields contain every reading twice.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 5b79b718-cdb6-44ef-bdf8-79a8feaa7074
📒 Files selected for processing (11)
.agents/skills/aicr-analyzing-snapshots/SKILL.mddocs/integrator/measurement-api.mdexamples/templates/snapshot-template.md.tmplpkg/collector/topology/readings.gopkg/collector/topology/readings_test.gopkg/collector/topology/topology.gopkg/constraints/gpu_nodes.gopkg/diff/topology.gopkg/diff/topology_test.gopkg/snapshotter/snapshot.gotools/registry-inventory/registry-allowlist.yaml
- taintReadingsFromData: the 1-field default branch now returns ErrCodeInvalidRequest instead of decoding to Effect:"", which the items path already forbids. Unreachable from encodeTaints, but a snapshot is caller-supplied input. - TestTaintDataPathRejectsSingleField: covers that branch (was 0%). - TestReadingsRejectMalformedReference: new case exercising the folds[ref]!=1 guard via a 3-item collision subtype where zone=us-west and zone.us-west=true both produce rawKey "zone.us-west" (was 0%). - labelReadingsFromItems / taintReadingsFromItems: document that item order is load-bearing — the data path sorts, these must not, because HydrateItems pairs readings to items by index. Coverage: pkg/collector/topology 93.4% -> 94.0% Signed-off-by: Varun Ramesh <varamesh@nvidia.com>
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 `@pkg/collector/topology/readings.go`:
- Around line 449-451: Reject two-field taints with a suffix-less rawKey by
returning ErrCodeInvalidRequest from the decoding logic in
pkg/collector/topology/readings.go (lines 449-451), while preserving valid two-
and three-field parsing. Add a corresponding two-field suffix-less input and
assert decode rejection in pkg/collector/topology/readings_test.go (lines
453-474).
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 12711ebe-088b-4139-abe5-c6eeee33dd37
📒 Files selected for processing (2)
pkg/collector/topology/readings.gopkg/collector/topology/readings_test.go
| default: | ||
| return nil, errors.New(errors.ErrCodeInvalidRequest, | ||
| fmt.Sprintf("taint reading %q: expected 2 or 3 pipe-separated fields, got 1", rawKey)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject legacy two-field taints without an effect suffix.
The two-field format stores Effect in the final rawKey segment. A key without . currently returns a reading with Effect == "".
pkg/collector/topology/readings.go#L449-L451: returnErrCodeInvalidRequestwhen a two-fieldrawKeyhas no effect suffix.pkg/collector/topology/readings_test.go#L453-L474: add a two-field input with a suffix-less key and assert decode rejection.
📍 Affects 2 files
pkg/collector/topology/readings.go#L449-L451(this comment)pkg/collector/topology/readings_test.go#L453-L474
🤖 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 `@pkg/collector/topology/readings.go` around lines 449 - 451, Reject two-field
taints with a suffix-less rawKey by returning ErrCodeInvalidRequest from the
decoding logic in pkg/collector/topology/readings.go (lines 449-451), while
preserving valid two- and three-field parsing. Add a corresponding two-field
suffix-less input and assert decode rejection in
pkg/collector/topology/readings_test.go (lines 453-474).
Source: Coding guidelines
3 participants
Summary
The topology collector encodes a label's key and value into a single snapshot key, so two different readings can produce the same key and one is silently dropped. Adds an
itemsencoding that keeps each reading's fields separate; the existingdatamap is unchanged.Motivation / Context
encodeLabelsrenders a multi-valued label as<key>.<value>, which collides with a label literally named that.encodeTaintshas a second defect: it counts entries per key but disambiguates with effect, so two taints sharing both collapse into one entry.itemscarries key, value, effect and node membership as separate fields. Nothing is manufactured, so nothing can collide.Fixes: #2003
Related: N/A
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)cmd/aicrd,pkg/server)pkg/recipe)pkg/bundler,pkg/component/*)pkg/collector,pkg/snapshotter)pkg/validator)pkg/errors,pkg/k8s)docs/,examples/)pkg/constraints,pkg/fingerprint,pkg/client/v1,pkg/evidence/redactImplementation Notes
Additive, not a migration.
datais byte-identical to before — verified by capturing the same cluster with the released binary and this build and diffing both subtypes. Consumers that are not migrated cannot regress.No
apiVersionbump. Adding an optional field beside an existing one is additive-only under ADR-011 matching howda21e227introducedSubtype.Items.One decode path.
topology.LabelReadings/TaintReadingspreferitemsand fall back todata, so the branch exists once rather than in each consumer.pkg/constraints,pkg/fingerprint,pkg/snapshotter,pkg/client/v1and theshipped template all read through it. Every legacy path is untouched and still exercised by tests, so pre-existing snapshots evaluate exactly as they did.
Testing
make qualify # exit 0, all six stagesAlso verified against a live 3-node Kind cluster:
dataoutput byte-identical between the released binary and this build,itemspresent and correct, and--max-nodes-per-entrystill capping the rendered list.New coverage includes collision determinism (50 and 200 iterations, since the failure being guarded is a race on map ordering), legacy
data-only compatibility, and the first rendering test for the shipped snapshot template.Risk Assessment
Checklist
make testwith-race)make lint)git commit -S) —