feat(node-agent): Introduce Mokka Node Agent (p1) - #705
Conversation
… API Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
…context Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
… list & reflected agent readiness in health server Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
…nset in order to do smooth migration from nvml mock 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>
| mkdir -p "$DEV_ROOT" "$CONFIG_DIR" | ||
| mkdir -p "$HOST/run" | ||
|
|
||
| # 2. Copy mock NVML library + create symlinks |
There was a problem hiding this comment.
Purged gpudriver-related logic from nvml mock. It's not running through node agent.
Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
| } | ||
| } | ||
|
|
||
| func runStart(ctx context.Context, cmd *cli.Command) error { |
There was a problem hiding this comment.
The main entrypoint
| ) | ||
|
|
||
| // Simulator implements agent.Simulator and agent.Applier. | ||
| type Simulator struct { |
There was a problem hiding this comment.
This is how gpudriver simulator looks like in this design.
There was a problem hiding this comment.
The main staging logic for the gpudriver.
Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
ArangoGutierrez
left a comment
ArangoGutierrez
left a comment
There was a problem hiding this comment.
The supervisor decomposition reads well. Splitting Daemon out of Simulator and reaching it by type assertion keeps the interface honest for the simulators that never need a background loop, and putting the barrier between the Stage wave and the Apply wave is the right call given appliers depend on Stage artifacts. The internal/logging lift is clean and it kept the handler-type assertions the old controlplane test had, so nothing was lost moving it.
One thing to fix first: all 13 red e2e legs are the same line. setup.sh step 6 injected system.num_devices into both on-host config copies and nothing replaced it, so grep -F "num_devices: 8" exits 1 in every standalone, DRA and multi-node run. Worth clearing early, because 75 specs sit skipped behind that first failure and have not been exercised against this change yet. The nri, gpu-operator and nfd legs pass, and they are the ones that never call those assertions.
The rest is inline, plus a few notes at the end of this body for the ones that could not anchor to a diff line.
- Dropping the BUILT_SO block also dropped the exit 1 that used to stop setup.sh before this heredoc when the mock library was missing, so the CDI spec is now published naming driver/usr/lib64/libnvidia-ml.so.1 and driver/usr/bin/nvidia-smi whether or not node-agent has staged them, and the container still prints "Mock GPU environment ready". (deployments/nvml-mock/scripts/setup.sh:100) —
k8s-test-infra/deployments/nvml-mock/scripts/setup.sh
Lines 98 to 104 in 4c0b2c0
- The only elevated thing node-agent does is the unix.Mknod in stageCharDevs; everything else is a root-owned write into a hostPath, and CAP_MKNOD is already in the CRI default set. allowPrivilegeEscalation: false with capabilities drop ALL plus add MKNOD would scope it, though the allocation-watcher's bare drop ALL cannot be copied as-is since it strips MKNOD. (deployments/nvml-mock/helm/nvml-mock/templates/daemonset.yaml:233)
- The nodeAgent block landed between the allocationWatcher header comment and the allocationWatcher key, so the pod-resources and synthetic-bytes text now reads as documenting nodeAgent. (deployments/nvml-mock/helm/nvml-mock/values.yaml:130)
| if err := ctx.Err(); err != nil { | ||
| return err | ||
| } | ||
| if err := h.WriteFile(p, state.ConfigRaw, 0o644); err != nil { |
There was a problem hiding this comment.
writeEngineConfig writes ConfigRaw verbatim, so the system.num_devices line setup.sh used to splice into both on-host copies is gone with nothing replacing it. That is what the e2e legs are hitting: grep -F -- "num_devices: 8" /var/lib/nvml-mock/config/config.yaml exits 1 at scenario_standalone_test.go:72, and the same assertion fails against the driver-root copy on the DRA legs.
Also at internal/agent/gpudriver/stage.go:31.
There was a problem hiding this comment.
I have accounted for GPU_COUNT env var override, but the whole configuration management should be revisited as it grows in complexity (#717)
|
|
||
| // Remove removes path; not-exist is not an error. | ||
| func (h *Host) Remove(path string) error { | ||
| if err := os.Remove(path); !os.IsNotExist(err) { |
There was a problem hiding this comment.
The condition is inverted: os.IsNotExist(nil) is false, so a successful removal returns remove : %!w() and a genuinely missing path returns nil. Revoke is the only caller today, so every clean shutdown logs a failure that did not happen; err != nil && !os.IsNotExist(err) is the form Discard already uses.
| - name: host-fabric-state | ||
| mountPath: {{ .Values.fabricmanager.stateDir }} | ||
| {{- end }} | ||
| - name: node-agent |
There was a problem hiding this comment.
node-agent is a plain sibling container, so nothing orders it ahead of nvml-mock, whose setup.sh reaches nri_cdi_device_nodes before stageCharDevs has created nvidiactl, nvidia-uvm and nvidia-uvm-tools. The [ -e "$DEV_ROOT/$_extra" ] guard then drops those three from the "all" device at exit 0 with no diagnostic, and the spec is written once, which is the narrowing the comment above that function says must not happen.
There was a problem hiding this comment.
Added a temp workaround in setup.sh to wait for node agent to finish:
| // mknodChar creates a character device at path; EEXIST is treated as success (idempotent). | ||
| func mknodChar(path string, major, minor uint32) error { | ||
| //nolint:gosec // Mknod requires the cast; values are controlled constants | ||
| err := unix.Mknod(path, uint32(syscall.S_IFCHR)|0o666, int(unix.Mkdev(major, minor))) |
There was a problem hiding this comment.
mknod(2) subtracts the umask, so these nodes land at 0644 where the mknod -m 666 lines they replace produced 0666, and the NRI plugin copies the host mode verbatim into injected containers. An os.Chmod placed after the EEXIST filter rather than only on the successful-Mknod path also repairs nodes an earlier run left behind, and it would line up with the NVreg_DeviceFileMode: 438 that stage.go still advertises.
| // Discard removes the driver tree and engine config written by Stage. | ||
| func (s *Simulator) Discard(_ context.Context, h *host.Host) error { | ||
| driverRoot := filepath.Join(h.Root, "driver") | ||
| if err := os.RemoveAll(driverRoot); err != nil { |
There was a problem hiding this comment.
Discard removes the whole driver tree, but Stage only writes driver/dev, driver/usr/lib64, driver/usr/bin, driver/proc/driver/nvidia and driver/config/config.yaml. Teardown therefore also takes the IB tools and etc/libibverbs.d that setup.sh stages and the overrides.yaml the allocation-watcher writes, and entrypoint.sh runs setup.sh once and then sleeps, so none of it comes back. Scoping this to the paths Stage wrote, and skipping it when ready was never set, is what the MEP promises.
There was a problem hiding this comment.
Scoped gpudriver.Discard() to only files the simulator creates: dd5cc4b ✅
| continue | ||
| } | ||
| if err := a.reconcile(ctx, u.State); err != nil { | ||
| a.log.Error("reconcile failed", "generation", u.State.Generation, "err", err) |
There was a problem hiding this comment.
A failed reconcile is logged and the loop then waits for the next Update, but FileSource.poll advances lastHash before it compiles and emits, so identical config content is never re-sent and the failed Stage is never retried. The setup.sh path this replaces ran under set -e and got retried by a kubelet restart, so a transient mknod EIO or ENOSPC used to heal itself and now does not.
Also at deployments/nvml-mock/helm/nvml-mock/templates/daemonset.yaml:242.
There was a problem hiding this comment.
I'm happy to fail health check for now in order to get pod restarted:
This closely mimics the current setup.sh behaviour. We will be able to make more sophisticated by supporting retries if needed.
| HostRoot: "/host", | ||
| }, | ||
| Software: agent.SoftwareVersions{ | ||
| DriverVersion: cfg.System.DriverVersion, |
There was a problem hiding this comment.
DriverVersion comes straight from the profile config and the node-agent container gets no DRIVER_VERSION env, so .Values.driverVersion no longer reaches the staged .so name, the procfs NVRM line or the nvidia-smi fallback now that those blocks moved out of setup.sh. The nvml-mock.driverVersion helper still tells users to set it when a config has no system.driver_version, and following that advice now stages libnvidia-ml.so. with a blank version.
There was a problem hiding this comment.
Preserved the DriverVersion override via env vars: 7e95c7a
@ArangoGutierrez btw why do we need to override DriverVersion via env vars? What's the use case we are targeting here?
|
|
||
| // Run starts the agent and blocks until ctx is cancelled or a required component | ||
| // fails. On return it executes a best-effort Revoke → Discard teardown. | ||
| func (a *Agent) Run(ctx context.Context) error { |
There was a problem hiding this comment.
internal/agent, host, gpudriver, health and cmd/node-agent all report no test files, and host.go's package comment advertises a t.TempDir() seam that nothing exercises. A table test over Host.Remove would have caught the inverted check, and pinning the ordering invariant that no Apply runs until every Stage has returned is worth having before the follow-up porting PRs build on this.
There was a problem hiding this comment.
Yes, we should have tests. Added them in 4678ed4 ✅
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>
facc33d to
57805d5
Compare
Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
…und) Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
…ates 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>
…vars Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
…ma/688-node-agent-mvp
…its for node agent Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
2 participants
What This PR Does
In this PR I'm introducing Mokka Node Agent. The PR:
SimulatorThe changes are done in way that the remaining logic is kept to work via the nvml mock approach while ported simulators are already ran from node agent. As we progress in this porting, more and more logic will be turned into node agent simulators until setup.sh script is empty so we can remove it all together.
We will go over porting the rest of the logic gradually in followup PRs.
Why
MEP0001 and MEP0003 go over why.
Checklist
git commit -s)go test -v -race ./...)make lint-fix)