Skip to content

Normalize generated group identities - #175

Merged
almaslennikov merged 1 commit into
mainfrom
feat/drop-nvidia-group-identifiers
Aug 17, 2026
Merged

Normalize generated group identities#175
almaslennikov merged 1 commit into
mainfrom
feat/drop-nvidia-group-identifiers

Conversation

@almaslennikov

@almaslennikov almaslennikov commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • use one normalized value for each source-group identifier and its nvidia.kubernetes-launch-kit.machine node label
  • remove complete NVIDIA segments and cap generated identities at 30 characters
  • shorten common machine segments: ThinkSystem to ts and PowerEdge to pe
  • balance long machine/GPU prefixes and append a 6-character deterministic hash
  • preserve the existing nvidia.kubernetes-launch-kit.gpu label value and 63-character limit
  • update tests, documentation, and bundled skills with the new identity contract

Behavior

  • DGX-B200 + NVIDIA-H200 becomes dgx-b200-h200 for both the identifier and machine label
  • ThinkSystem-SR675-V3 + NVIDIA-RTX-PRO-6000-Blackwell-Server-Edition becomes ts-sr675-v3-rtx-pro-600-46c7a9
  • the GPU label remains NVIDIA-RTX-PRO-6000-Blackwell-Server-Edition
  • NVIDIAN-H200 remains nvidian-h200; only complete vendor segments are removed

Testing

  • CGO_ENABLED=0 go test ./...
  • CGO_ENABLED=0 make build
  • CGO_ENABLED=0 go vet ./...
  • git diff --check

Local limitations

  • golangci-lint was not installed locally
  • MkDocs was not installed locally

@greptile-apps

greptile-apps Bot commented Aug 17, 2026

Copy link
Copy Markdown

Greptile Summary

The PR makes generated source-group identifiers and machine-label values share one normalized, 30-byte identity while preserving the existing GPU-label representation.

  • Removes complete NVIDIA segments and applies common machine-name aliases.
  • Uses balanced machine/GPU prefixes with a deterministic six-character hash for long identities.
  • Updates discovery, rendering, tests, documentation, and bundled skills to the new identity contract.

Confidence Score: 4/5

The PR should not merge until generated identities are disambiguated when distinct source groups normalize to the same value.

Complete NVIDIA segments are still treated as non-identifying, and discovery assigns the resulting value directly to identifiers and machine labels without collision handling, leaving distinct groups vulnerable to conflated filtering and generated outputs; the previously reported truncation-order defect is fixed.

Files Needing Attention: pkg/config/config.go and pkg/networkoperatorplugin/discovery/discover.go

Important Files Changed

Filename Overview
pkg/config/config.go Introduces the shared normalized identity algorithm; truncation ordering is fixed, but complete NVIDIA-segment removal still permits identifier collisions.
pkg/networkoperatorplugin/discovery/discover.go Assigns the generated identity to source-group identifiers and machine labels without detecting duplicate normalized identities.
pkg/networkoperatorplugin/render_plan.go Updates legacy selector fallback generation to use the normalized shared identity.
pkg/config/config_test.go Covers normalization, aliases, balanced truncation, deterministic hashing, and the corrected truncation-before-segment-removal case.
pkg/networkoperatorplugin/templates_test.go Aligns merge and rendering expectations with normalized source and GPU-bucket identifiers.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Discovered machineType and gpuType] --> B[Normalize components]
  B --> C[Remove NVIDIA segments and apply aliases]
  C --> D[Bound identity to 30 bytes]
  D --> E[ClusterConfig identifier]
  D --> F[Machine node label]
  A --> G[Preserved GPU label normalization]
  G --> H[GPU node label]
Loading

Reviews (3): Last reviewed commit: "fix: normalize generated group identitie..." | Re-trigger Greptile

Comment thread pkg/config/config.go Outdated
Comment thread pkg/config/config.go Outdated
@almaslennikov
almaslennikov force-pushed the feat/drop-nvidia-group-identifiers branch from 0dfd44c to 051df24 Compare August 17, 2026 08:49
@almaslennikov almaslennikov changed the title Omit NVIDIA from generated group identifiers Normalize generated group identities Aug 17, 2026
@almaslennikov

Copy link
Copy Markdown
Collaborator Author

Addressed the review findings and the follow-up identity requirements in amended commit 051df24:

  1. Normalization collisions: a complete NVIDIA segment is intentionally non-identifying, so inputs that differ only by that segment produce the same group identity. TestGeneratedGroupIdentityDropsNVIDIASegment now records that contract explicitly; NVIDIAN and other non-exact segments remain distinct.
  2. Truncation before removal: source identities are now normalized from the separate machineType and gpuType values before any shortening. Long identities use balanced component prefixes, so truncation cannot split and preserve part of an NVIDIA segment. The ThinkSystem/RTX PRO regression covers this path.
  3. Shared machine identity: the source-group identifier and nvidia.kubernetes-launch-kit.machine label now use exactly the same vendor-free value, capped at 30 characters with a 6-hex deterministic hash. The separate .gpu label retains its previous value and limit.

Verified with CGO_ENABLED=0 go test ./..., CGO_ENABLED=0 make build, CGO_ENABLED=0 go vet ./..., focused package tests, and git diff --check. Local golangci-lint is unavailable; the prior PR lint job passed and the amended CI run will cover it again.

Use one vendor-free 30-character identity for source groups and their machine labels. Balance long machine and GPU prefixes, retain a compact deterministic hash, and preserve the existing GPU label value.

Signed-off-by: Alexander Maslennikov <amaslennikov@nvidia.com>
@almaslennikov
almaslennikov force-pushed the feat/drop-nvidia-group-identifiers branch from 051df24 to dff988f Compare August 17, 2026 08:53
@almaslennikov

Copy link
Copy Markdown
Collaborator Author

Follow-up requirement added in amended commit dff988f: the same normalization pass now maps exact ThinkSystem segments to ts and exact PowerEdge segments to pe.

  • The source-group identifier and .machine label still share one value.
  • Stored machineType, gpuType, preset names, and the .gpu label are unchanged.
  • ThinkSystem-SR675-V3 + NVIDIA-RTX-PRO-6000-Blackwell-Server-Edition now produces ts-sr675-v3-rtx-pro-600-46c7a9.
  • Exact-segment tests ensure names such as NVIDIAN are not rewritten.

Re-verified with CGO_ENABLED=0 go test ./..., CGO_ENABLED=0 make build, CGO_ENABLED=0 go vet ./..., focused package tests, and git diff --check.

@almaslennikov
almaslennikov merged commit 089ceea into main Aug 17, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant