Skip to content

NRI Plugin Refactoring - #751

Open
roma-glushko wants to merge 16 commits into
mainfrom
nri-refactor
Open

NRI Plugin Refactoring#751
roma-glushko wants to merge 16 commits into
mainfrom
nri-refactor

Conversation

@roma-glushko

@roma-glushko roma-glushko commented Aug 28, 2026

Copy link
Copy Markdown
Member

What This PR Does

Refactoring NRI plugin to:

  • move it's codebase to interna/nri package
  • use urfave/cli as CLI framework
  • use a unified, common logging setup
  • separate containerd machinery from the actual injection logic
  • break down injection logic into a clear multi-step pipeline that is easy to unit test

Since we have three components that behave like servers (Node Agent, Control Plane, NRI), I have promoted the health server logic into its own package reused in all three.

Why

It's stemmed from MEP0003. I'm keeping NRI in line with the recent restructuring and revisiting it's codebase to make sure it's clearly organized.

Checklist

  • Commits are signed off (git commit -s)
  • Tests pass (go test -v -race ./...)
  • Linter passes (make lint-fix)
  • New code has SPDX license headers
  • Documentation updated (if applicable)
  • CHANGELOG.md updated (if user-facing change)

Stack created with GitHub Stacks CLIGive Feedback 💬

Comment thread cmd/nri-plugin/main.go
// Grouped in the order the plugin uses them: how the process behaves,
// how it attaches to the runtime, then one group per injection step.
Flags: slices.Concat(
processFlags(),

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too many flags, had to group them like this 😃

@roma-glushko
roma-glushko marked this pull request as ready for review August 28, 2026 11:33
@greptile-apps

greptile-apps Bot commented Aug 28, 2026

Copy link
Copy Markdown

Greptile Summary

The refactor separates NRI runtime integration from injection logic and consolidates health-probe handling.

  • Moves the NRI implementation into internal/nri and introduces a staged injection pipeline.
  • Replaces the NRI command-line parsing with urfave/cli.
  • Reuses the shared health server across the node agent, control plane, and NRI plugin.
  • Updates the image and Helm chart for the renamed nri-plugin binary and logging options.

Reviews (11): Last reviewed commit: "Updated changelog" | Re-trigger Greptile

@roma-glushko
roma-glushko force-pushed the nri-refactor branch 2 times, most recently from 8a51f10 to 845df21 Compare August 31, 2026 14:14
Base automatically changed from node-agent-nvlink to main September 1, 2026 12:20
Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
…rm a plugin pipeline

Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
…rm a plugin pipeline

Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
ArangoGutierrez
ArangoGutierrez previously approved these changes Sep 1, 2026

@ArangoGutierrez ArangoGutierrez left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. Splitting adjust.go into one file per injection step makes the pipeline much easier to follow, and the step order, the three-arm GPU switch and every fail-open branch come through the move unchanged.

The behaviour moved with the code; what is left to tidy sits around the refactor rather than inside it.

  • The shipped binary changes name (nvml-mock-nri to nri-plugin), all fourteen env-var bindings move to the MOKKA_NRI_ prefix, the --plugin-name default becomes mokka-nri-plugin, and pkg/nri/nvmlmock stops being importable, with nothing recorded under [Unreleased]. Anyone configuring the plugin through the environment falls back to defaults after this with no signal. (CHANGELOG.md:8)
    and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
    ## [Unreleased]
    ### Fixed
  • The DefaultConfig comment says it returns the identity the chart deploys with, but the chart passes --plugin-name=nvml-mock from values.yaml while the default here is mokka-nri-plugin. Rewording it keeps the next reader from trusting the default to match a real deployment. (internal/nri/config.go:27)


nri:
enabled: false
logging:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nodeAgent.logging and controlPlane.logging both declare level and format enums in values.schema.json, but nri.logging does not, so --set nri.logging.level=verbose templates cleanly and then the plugin exits 1 on ParseLevel. Copying that schema block under properties.nri keeps a typo failing at template time instead of on every node.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should help a5d8a51

Comment thread internal/agent/agent.go
for _, sim := range a.simulators {
m[sim.Name()] = sim.Ready()
ready := sim.Ready()
if !ready {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleting these two lines leaves the agent and health suites green, so /readyz would answer 200 for a node whose simulator is not ready. TestReadyz_OneNotReady covered exactly this rule before the move; a case in agent_test.go driving Readiness with one simulator down would put it back.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Covered the test gap in 419e547

giuliocalzo
giuliocalzo previously approved these changes Sep 1, 2026
Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
# Conflicts:
#	deployments/nvml-mock/helm/nvml-mock/tests/nri_daemonset_test.yaml
Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
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.

3 participants