Skip to content

feat(discovery): expose read-only cluster inventory - #281

Open
efegokdemir wants to merge 8 commits into
NVIDIA:mainfrom
efegokdemir:dev/62-read-only-discovery
Open

efegokdemir wants to merge 8 commits into
NVIDIA:mainfrom
efegokdemir:dev/62-read-only-discovery

Conversation

@efegokdemir

Copy link
Copy Markdown

Summary

  • add networkoperatorplugin.DiscoverReadOnly for clusters with an existing NIC Configuration Daemon
  • return ErrNotInstalled when no daemon pod is present
  • keep the path read-only: it lists daemon pods, nodes, and NicDevice resources without bootstrap, label patches, or cleanup
  • document the read-only contract and required read permissions

Testing

  • go test ./pkg/networkoperatorplugin/... -count=1
  • go test ./... -race -count=1
  • go vet ./...
  • go build ./...
  • make test
  • make lint
  • python3 scripts/check-docs.py --binary build/l8k
  • git diff --check

Fixes #62

Add a read-only library entry point that consumes an existing NIC Configuration Daemon and NicDevice resources without mutating cluster state. Return a sentinel when the daemon is absent and document the read-only RBAC contract.

Fixes NVIDIA#62

Signed-off-by: Efe Gökdemir <efe@rexcode.co.uk>
@greptile-apps

greptile-apps Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Medium risk] Adds read-only discovery API for cluster inventory.

The PR appears safe to merge based on the changes since the previous review.

Summary

The PR adds a read-only library entry point for inventory from an existing NIC Configuration Daemon.

  • Callers can restrict daemon lookup to a configured namespace.
  • Discovery reads pods, nodes, and NicDevice resources without changing cluster resources.
  • Documentation and tests cover the new entry point and namespace selection.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[DiscoverReadOnly] --> B{Namespace configured?}
  B -->|Yes| C[Inspect configured namespace]
  B -->|No| D[Inspect standard operator and Launch Kit namespaces]
  C --> E[Wait for daemon pods in selected namespace]
  D --> F[Wait with operator-namespace fallback]
  E --> G[List nodes and NicDevices]
  F --> G
  G --> H[Build read-only cluster inventory]
Loading

Reviews (6) · Last reviewed commit: "fix: preserve read-only daemon readiness..."

Comment thread pkg/networkoperatorplugin/discovery/readonly.go Outdated
Comment thread pkg/networkoperatorplugin/discovery/readonly.go Outdated
Comment thread pkg/networkoperatorplugin/discovery/readonly.go
Comment thread pkg/networkoperatorplugin/discovery/readonly.go
Comment thread pkg/networkoperatorplugin/discovery/readonly.go Outdated
Comment thread docs/user/discovery.md Outdated
Signed-off-by: Efe Gökdemir <efe@rexcode.co.uk>
@efegokdemir

Copy link
Copy Markdown
Author

Updated the read-only discovery path to address the review findings: it now locates the existing daemon across namespaces, scopes inventory to Ready daemon nodes, derives a selector for a single group, returns NicDevice list errors directly, filters stale devices, and documents the namespace/RBAC scope.

Validation: go test ./pkg/networkoperatorplugin/... -count=1, go vet ./pkg/networkoperatorplugin/..., and git diff --check passed.

Comment thread docs/user/discovery.md Outdated
Comment thread pkg/networkoperatorplugin/discovery/readonly.go
Comment thread pkg/networkoperatorplugin/discovery/readonly.go
Comment thread pkg/networkoperatorplugin/discovery/readonly.go Outdated
Comment thread pkg/networkoperatorplugin/discovery/readonly.go Outdated
Signed-off-by: Efe Gökdemir <efe@rexcode.co.uk>
@efegokdemir

Copy link
Copy Markdown
Author

Follow-up update on the latest review: read-only discovery now rejects incomplete inventories instead of silently omitting ready daemon nodes, prefers a non-bootstrap operator namespace when both exist, verifies derived selectors do not match outside nodes, and documents the required cluster-wide pod/NicDevice list permissions.

Validation: gofmt, git diff --check, go test ./pkg/networkoperatorplugin/discovery ./pkg/networkoperatorplugin -count=1.

Comment thread pkg/networkoperatorplugin/discovery/readonly.go Outdated
Signed-off-by: Efe Gökdemir <efe@rexcode.co.uk>
@efegokdemir

Copy link
Copy Markdown
Author

Updated at bcc3a22: daemon lookup now uses namespace-scoped reads with Network Operator preference, preserves ready-node/NicDevice completeness filtering, and validates selectors against all node labels. Documentation and regression coverage were updated. Local validation passed: go test ./... -count=1; go test ./... -race -count=1; go vet ./...; go build ./...; make lint; python3 scripts/check-docs.py --binary build/l8k; git diff --check.

Comment thread pkg/networkoperatorplugin/discovery/readonly.go Outdated
Comment thread pkg/networkoperatorplugin/discovery/readonly.go Outdated
Comment thread docs/user/discovery.md Outdated
Signed-off-by: Efe Gökdemir <efe@rexcode.co.uk>
@efegokdemir

Copy link
Copy Markdown
Author

Verified and fixed the selector finding in 62e4407. Read-only discovery now evaluates every shared-label candidate in preference order, skips candidates that match outside nodes, and selects the first safe candidate. It only errors when no candidate is safe; the single-node hostname fallback remains available.

Added regressions for a non-unique first label followed by a unique label and for the no-safe-candidate case. Validation: gofmt, go test ./pkg/networkoperatorplugin/discovery ./pkg/networkoperatorplugin -count=1, go vet ./pkg/networkoperatorplugin/discovery ./pkg/networkoperatorplugin, and git diff --check.

Signed-off-by: Efe Gökdemir <efe@rexcode.co.uk>
@efegokdemir

Copy link
Copy Markdown
Author

Addressed the remaining namespace/readiness findings in dfb3b11. Read-only discovery now accepts the configured Network Operator namespace through the backward-compatible WithReadOnlyNetworkOperatorNamespace option, probes only that namespace when supplied, and otherwise checks the standard operator/bootstrap namespaces for usable Ready daemon pods. An unready preferred installation no longer hides a Ready fallback.

Added regressions for custom namespaces and unready-preferred fallback. Validation: gofmt, focused discovery tests, go test ./pkg/networkoperatorplugin/discovery ./pkg/networkoperatorplugin -count=1, go vet ./pkg/networkoperatorplugin/discovery ./pkg/networkoperatorplugin, and git diff --check.

Signed-off-by: Efe Gökdemir <efe@rexcode.co.uk>
Comment thread pkg/networkoperatorplugin/discovery/readonly.go
Comment thread pkg/networkoperatorplugin/discovery/readonly.go
Comment thread pkg/networkoperatorplugin/discovery/readonly.go Outdated
Signed-off-by: Efe Gökdemir <efe@rexcode.co.uk>
@efegokdemir

Copy link
Copy Markdown
Author

Addressed the three latest findings in 94f67c6.

  • Configured namespaces are now scoped for both lookup and readiness polling, so fallback namespaces cannot bypass an explicit choice.
  • Namespace selection prefers a Ready daemon but retains a namespace with only starting pods so the existing readiness wait can observe startup progress.
  • Pod-list errors are preserved when no usable namespace can be selected.

Validation: gofmt, GOMAXPROCS=2 go test ./pkg/networkoperatorplugin/discovery -count=1 -timeout=90s, GOMAXPROCS=2 go vet ./pkg/networkoperatorplugin/discovery ./pkg/networkoperatorplugin, and git diff --check.

This branch has not been deployed

No deployments
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.

Expose read-only DiscoverReadOnly() API for library consumers

1 participant