MEP0003: Mokka Node Agent - #670
Conversation
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
left a comment
There was a problem hiding this comment.
Nice proposal, Roma. The component decomposition is the right call, and the source
abstraction (FileSource and ControlPlaneSource behind one interface) is exactly the
boundary we agreed on: the agent executes, the control plane configures, and swapping the
source touches no component. The per-surface coverage matrix is the most useful thing in the
document and I want to keep it.
Most of my comments are about accuracy rather than direction. I checked the matrix against
the tree and several rows do not match what is actually shipped, in both directions. I also
have one blocking issue where the document argues against itself, and a set of Go lifecycle
details that are worth settling now because nine components will be written against these
interfaces.
Requesting changes, mainly for the nodeidentity contradiction and the MEP-0001 mapping.
Blocking
B1. The nodeidentity section argues against its own design
Anchor: #### Kubernetes-visible node identity, opening paragraph.
This logic is either not supposed to be a part of the Mokka Node Agent (Mokka Control
Plane is a better place for node labeling) or it's supposed to be done via GFD.
Agents should not work with the K8s API or be responsible for node labeling.
The table directly below then ships nodeidentity writing nvidia.com/gpu.present=true
"via K8s API", and the Delivery line says "K8s label via API (RBAC required)". The design
that survives is the one the paragraph says should not exist.
This matters beyond tidiness. Giving every node agent get and patch on Nodes puts N writers
against the K8s API in a tool whose value is that it does not perturb the control-plane
footprint people are measuring. That is the observer-effect argument that pushed labeling
out of the agent in the first place.
Pick one and delete the other:
- Control plane owns node identity. The agent drops
nodeidentityentirely and needs no RBAC. - GFD or NFD owns it. The agent writes only the NFD feature file, no K8s client, no RBAC.
- The agent keeps it. Then delete the paragraph and justify the exception explicitly,
including the RBAC surface and why N writers is acceptable.
Worth noting for whichever you pick: today setup.sh step 7 already does this with
get+patch on nodes, for gpu.present only, so option 3 is the status quo and options 1 and 2
are the actual change. The chart docs also say nvml-mock writes nvidia.com/gpu.present
directly rather than deriving it from GFD, so option 2 is a behavior change for that label,
not just a relocation.
B2. The MEP-0001 mapping does not match the merged CRDs
Anchor: the State struct, and the comment MEP-0001 §SGPUProfile.spec.software.
Four problems, all checkable against internal/controlplane/api/v1alpha1/ on main:
There is no SGPUProfile kind. The registered kinds are SGPURackProfile,
SGPUInventory, SGPURuntimePolicy, SGPURack. SGPURackProfile.spec.software is the
field you want, and SGPUSoftware{DriverVersion, NVMLVersion, CUDAVersion} does map cleanly
onto your SoftwareVersions, so this is just the kind name. Note there is a merged test
asserting the name you used does not exist:
// internal/controlplane/api/v1alpha1/groupversion_info_test.go:29
require.False(t, scheme.Recognizes(GroupVersion.WithKind("SGPUProfile")))MEP-0001 itself never uses the string SGPUProfile, so this looks like it was coined here.
DeviceState collides. State.Devices []DeviceState uses that identifier for a per-GPU
struct of "identity + hardware + runtime". In the merged API it is already a health enum:
// internal/controlplane/api/v1alpha1/shared_runtime_types.go:21
type DeviceState string // Healthy | Degraded | Failedinternal/agent will have to import v1alpha1 to consume control-plane state, so this is a
real collision, and a confusing one even if you alias around it. Rename yours, DeviceSpec
or GPUState.
Fabric identity is rack-scoped, and clique is pinned to zero. FabricUUID and CliqueID
live on SGPURackIdentity, not on a profile, so State.Fabric is sourced from SGPURack
rather than the profile. More importantly the merged CRD constrains the value:
// sgpurack_types.go
// CliqueID remains zero while each rack represents one fabric clique.
// +kubebuilder:validation:Minimum=0
// +kubebuilder:validation:Maximum=0
CliqueID int32 `json:"cliqueID"`The agent design reads as though clique ID is a varying input it applies per node. Today the
control plane can only ever emit 0. Either say the agent applies a constant for now, or note
that this bound has to be relaxed first and reference where.
Runtime state has no carrier. The merged API has a whole runtime surface delivered
through a separate CRD: SGPURuntimePolicy carrying RuntimeState with DeviceState,
RuntimeModes (persistence, compute, MIG, ECC, accounting) and RuntimeTelemetry
(utilization, power, temperature, clocks). MEP-0001 has dedicated sections on how that
policy fans out and applies. State has no field for it, and StateSource is a single
channel. If the agent has to reconcile a profile and a policy, that is two sources with
different fanout semantics, and the single-State, single-Watch shape may not express it.
Worth resolving here rather than after nine components exist.
B3. Cleanup runs on a cancelled context, and we already know better in this repo
Anchor: Agent.Run(ctx) step 5, "On ctx.Done(): cancel Run goroutines, then call
Cleanup(ctx, host)".
By step 5 that ctx is done. Plain os file work still completes, but anything going
through a context will not: client-go calls fail at RoundTrip, and exec.CommandContext
starts the child then SIGKILLs it at Wait. So on shutdown you get a partial reversal, files
removed but the node label left behind, which is the worst case since the node keeps
advertising a driver that is gone.
The sibling component already solved this, and the comment there explains why: without
WithoutCancel the drain never gets a chance to start.
// internal/controlplane/server.go:84
shutdownCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), s.cfg.ShutdownTimeout)Same shape here:
cleanupCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), cleanupTimeout)
defer cancel()Two details worth writing into the MEP: call stop() from signal.NotifyContext before
cleanup so a second SIGTERM is not swallowed, and keep cleanupTimeout strictly below the
DaemonSet's terminationGracePeriodSeconds or the kubelet kills you mid-reversal. go.mod
is on go 1.26.0, so WithoutCancel and errors.Join are both available.
B4. Nothing orders the components, and three of them publish externally
Anchor: ### The parallel-reconcile shape and the max(t_component) claim.
Level-triggered reconciliation genuinely covers agent-internal drift: if a pass leaves things
inconsistent, the next pass repairs it. It does not cover commitments a third party acts on
irreversibly, and three components make exactly those:
cdiwrites into a directory containerd watches. Win that race and a container is admitted
against a spec referencing chardevs and driver paths that do not exist yet.nodeidentitywrites a node label, which is a scheduling signal. Label before
driverfootprinthas materialized and GPU pods land on a node that is not yet capable.- The
/run/nvidia/driversymlink created before its target is a dangling symlink, and
consumers thatstatrather thanlstatget ENOENT.
This is not hypothetical, because the script being replaced already has the ordering and
set -e makes it load-bearing. In setup.sh the node label is step 7, after the driver
files (steps 2, 2b, 3, 4, 4c) and after both CDI specs (3b, 3c). The compatibility symlink is
step 8, after everything it points at. A flat nine-way fan-out drops both orderings, so this
is a behavior regression rather than a new risk.
You do not need a DAG engine. A two-phase split keeps almost all the speedup:
derivewave: everything that materializes artifacts, fully parallel.- barrier.
publishwave:cdi,nodeidentity, the symlink. Have eachLstatthe paths it
references before writing, and write atomically.
Then the honest cost is max(t_derive) + max(t_publish), which is worth stating instead of
max(t_component). Cleanup is the mirror image, and the MEP currently has it backwards:
retract the label and the CDI spec first, then tear down the artifacts they point at.
Should fix before merge
S1. The NVML coverage number is wrong, by the MEP's own instrument
Anchor: "89 real implementations in pkg/gpu/mocknvml/engine + auto-generated stubs.
Coverage measurable via generate-bridge --stats."
Running the tool the row cites, on main:
NVML Function Coverage:
Total functions: 413
Hand-written implementations: 142 (34.4%)
Generated stubs: 271 (65.6%)
142, not 89. The string 89 does not appear anywhere in pkg/gpu/mocknvml/. Since the row
names the command, quote its output, or drop the number and just name the command so it
cannot go stale.
S2. The InfiniBand row cites the wrong package and undersells the implementation
Anchor: the /sys/class/infiniband/... row, marked covered "via pkg/network/mockib/sysfs".
pkg/network/mockib/sysfs contains only scan.go, which reads a real host's IB tree. The
renderer is pkg/network/mockib/render/render.go.
The attribute list is also off. The row names eight attributes, one of which
(gid_tbl_len) is not rendered at all, and is not a sysfs file in the first place, it is an
ibv_port_attr struct field. Meanwhile the renderer emits eighteen:
board_id cap_mask fw_ver hca_type hw_rev lid lid_mask_count link_layer
node_desc node_guid node_type phys_state port_guid rate sm_lid sm_sl
state sys_image_guid
plus gids/0 derived correctly as fe80:: + port GUID, pkeys/0, and the gid_attrs and
counters directories. That is a notably more complete HCA than the row suggests.
This matters more than a citation nit. The matrix is a gap analysis, and people will scope
work from it. A row that understates coverage sends someone to implement node_type and
cap_mask that already exist, and a named attribute that does not exist sends someone to
implement a file the kernel never had.
S3. errgroup.WithContext in Reconcile defeats the per-surface attribution you promise
Anchor: the devicedriver.Reconcile example, and "returns an error tagged with the surface
name for /readyz attribution", plus "Ready() powers /readyz and attributes failures per
surface".
errgroup.WithContext gives you first-error-wins: errOnce keeps only one error, and the
derived context cancels the other six surfaces the moment any one fails. So /readyz can
report one surface, not per-surface status, and the siblings get torn off mid-write, which
for a multi-file tree that a dlopen'd shim reads is worse than either finishing or not
starting.
For wave 1, use a plain WaitGroup on the parent context, collect a per-surface status map,
and join with errors.Join. Keep errgroup.WithContext for wave 2, where first-error-wins
and sibling cancellation are the behavior you actually want, which is how
cmd/mokka-control-plane/main.go:76 uses it today.
S4. StateSource cannot express failure, sync, or staleness
Anchor: the StateSource interface.
Watch(ctx context.Context) (<-chan State, error)Silence on that channel means both "nothing changed" and "the control plane has been
returning 5xx for forty minutes", and /readyz stays green against arbitrarily stale desired
state. Channel-close semantics are unspecified too: a select that ignores the ok value
receives zero-valued State forever and reconciles the node down to nothing. And there is no
resync, so out-of-band host mutation produces no event, which makes the design
level-triggered with respect to the source but edge-triggered with respect to the host, and
the host is the thing you are reconciling.
Suggest adding an error channel, HasSynced() bool, LastSync() time.Time, a Generation
on State, and a resync period. Also state whether /healthz is liveness only. If a control
plane outage can fail /healthz, one outage restart-loops the whole fleet at once.
S5. State is copied but its slices are shared
Anchor: Watch(...) (<-chan State, error) versus Reconcile(ctx, host, state *State).
Sent by value, taken by pointer. The struct copy is shallow, so Devices and Software are
the same backing arrays across all nine components running concurrently. One component
sorting Devices in place to make its output deterministic is a data race, and that is a
natural thing for a renderer to want to do.
Simplest fix is to make State immutable by construction and pass it by value throughout, or
say explicitly that components must treat it as read-only and never sort or mutate in place.
S6. Ready() bool cannot carry what /readyz is specified to report
Anchor: Ready() bool in Component.
The document asks /readyz to be "aggregated + per-component" and to attribute failures per
surface. A bool cannot say which surface failed, when it last succeeded, or which generation
it observed. Consider:
type Health struct {
Ready bool
Reason string
Surfaces map[string]SurfaceStatus
LastAttempt, LastSuccess time.Time
Generation int64
}Also worth putting Optional() bool on the interface. Required versus optional currently
lives in agent config, but it changes Reconcile failure handling, so the component should
declare it.
S7. Empty Risks, Drawbacks and Alternatives
enhancements/README.md step 4 says to "expand Design Details, Risks, and Alternatives as
consensus forms", and step 5 merges after that. Both prior MEPs filled Drawbacks and
Alternatives, and both filled Risks and Mitigations. MEP-0003 leaves all three as template
comments.
User Stories and Notes are marked Optional in template.md, so leaving those empty is fine.
Risks and Mitigations is not marked optional.
For Alternatives, the obvious ones to record are: keep the CLIs and add a thin supervisor;
keep setup.sh and only add the state source; and one binary with subcommands instead of
components. For Risks, the two I would want written down are the ordering regression in B4
and the blast radius of one process replacing a dozen (one panic now takes out every
simulated surface on the node, where today a mock-ib crash leaves the GPU footprint
standing).
Smaller things
N1. Two references to a test plan this MEP does not have
The PCI driver-symlink row says "add explicit test to MEP-0003 test plan", and the
fabric-manager process row says "call out in MEP-0003". There is no test plan section and no
call-out. MEP-0002 has a ### Test plan section if you want the precedent. These read as
author TODOs that got shipped in the table.
N2. The table of contents is stale
It lists User Stories, Story 1 and Story 2, none of which exist in the body, so those three
anchors are dead. It omits Interfaces, Components, The parallel-reconcile shape, and
Simulated Surface, which are the entire design. There is no toc tooling in the Makefile or CI,
so this is hand-maintained and nothing will catch it.
N3. nvidia-uvm major 510 is not a fixed number
The driver row says majors 195 and 510 with "mknod with correct major/minor", and
setup.sh:61 calls both "standard NVIDIA major numbers". 195 is genuinely fixed for
nvidia. nvidia-uvm is not: uvm_chardev_create uses alloc_chrdev_region, so the kernel
allocates it dynamically and 510 is just what one machine happened to get.
Interesting contrast in the same script: IMEX handles this correctly already.
IMEX_MAJOR=${IMEX_CHANNEL_MAJOR:-235} is overridable, and the same variable feeds both
render-imex-procdevices --imex-major and the mknod, so the rendered /proc/devices and
the device nodes cannot disagree. nvidia-uvm has neither the override nor a rendered
/proc/devices entry.
Since this bash is moving into Go anyway, that is the moment to carry the IMEX pattern over:
one source of truth for the major, rendered /proc/devices authoritative, mknod derived
from it.
N4. Engine config write mechanism is described imprecisely
The row says "atomic write with unix.Flock; co-writer nvml-mock-ctl shares the lock".
Two independent mechanisms are doing two different jobs in pkg/gpu/mockctl/publish.go:
WriteAtomic gets atomicity from same-directory CreateTemp plus Rename, and
LockOverride takes flock on a sibling .lock file to serialize concurrent writers. Flock
is advisory and the reader is a dlopen'd shim in another container that takes no lock, so
it is not what makes the read safe.
Worth being precise because someone implementing driverfootprint from this row could
reasonably write flock-only and lose the atomicity. Ideally say that driverfootprint reuses
pkg/gpu/mockctl rather than reimplementing, and carry forward the two constraints that
package encodes: the chmod to 0644 (the temp file is 0600 and the shim may run non-root), and
the requirement that the config directory is the bind mount, since a single-file bind pins
the original inode and hides the rename. That second one is documented in setup.sh around
line 154 and is easy to lose in a rewrite.
One genuine gap while you are there: WriteAtomic does not fsync the temp file before rename
nor fsync the parent directory, so a crash can expose the rename without the contents.
N5. Stale line citation
"shape from cmd/nvml-mock-nri/main.go:107 serveHealth" points at the line above the
comment block. The comment starts at 108 and the function at 112. Line numbers drift; naming
the function alone is enough.
What looks good
- The component decomposition is the right unit. "What do we pretend exists, what contract
surfaces does it have, who consumes them" is a much better organizing principle than the
current CLI split, and it makes the gaps legible. - The
StateSourceabstraction is the important architectural win.FileSourceand
ControlPlaneSourcebehind one interface means the control plane integration touches zero
components, which is exactly the boundary we wanted. - The coverage matrix is the most valuable artifact here. My complaints are about specific
cell contents, not the idea. Please keep it and keep it current. - The gap rows I spot-checked are honest. Nothing in the tree handles
/proc/modulesor
lsmod, anddocs/architecture.mdalready names module state as part of the story, so
that row is correctly marked and correctly placed. - Retiring
cmd/fake-imexin favor of the real--nogpudaemon, and saying so in the table,
is the right call and good to have written down. 690 lines of bash driving 11 numbered phaseschecks out exactly, which is a fair
characterization of the problem.
One cross-cutting suggestion
Between this and #661 we keep encoding identity strings that real NVIDIA consumers read, and
getting them slightly wrong in ways nothing catches: a kind name that does not exist
(SGPUProfile), a sysfs attribute that is not a file (gid_tbl_len), a dynamically
allocated major hardcoded as though it were fixed (510), a stale implementation count (89).
Each is individually minor and each is caught only by a human who happens to know that
surface.
The repo already has the shape of the fix. groupversion_info_test.go asserts both that the
real kinds are recognized and that a plausible wrong one is not. Generalizing that into a
conformance test over emitted identity strings, the labels we write against the keys GFD, the
device plugin, NFD and the DRA driver actually read, and our rendered /proc/devices against
the majors we mknod, would retire the whole class instead of catching one instance per
review. Probably its own issue rather than something to bolt onto this MEP.
Verification log
Everything above was checked against upstream/main at 95e94598, PR head ea6e3e7.
| Claim | Result |
|---|---|
setup.sh is 690 lines, 11 numbered phases |
confirmed exactly, 11 top-level steps, 19 including letter substeps |
~a dozen CLIs under cmd/ |
11 directories, fair |
| 89 NVML implementations | wrong, generate-bridge --stats reports 142 of 413 |
generate-bridge --stats exists |
confirmed, cmd/generate-bridge/main.go:52 |
15 functions in pkg/gpu/mockcuda |
confirmed exactly, 15 //export in bridge/cuda.go |
serveHealth at main.go:107 |
off, comment at 108, func at 112 |
SGPUProfile kind |
does not exist, negative assertion at groupversion_info_test.go:29 |
SGPURackProfile.spec.software |
exists, SGPUSoftware{DriverVersion, NVMLVersion, CUDAVersion} |
DeviceState collision |
confirmed, shared_runtime_types.go:21 is a health enum |
CliqueID bounds |
confirmed pinned, Minimum=0 Maximum=0 |
| Go floor supports proposed fixes | go 1.26.0, all available |
WithoutCancel precedent |
internal/controlplane/server.go:84, with rationale comment |
| atomic write already implemented | pkg/gpu/mockctl/publish.go, temp+rename plus sibling .lock |
setup.sh ordering is load-bearing |
set -e, label step 7 after driver steps, symlink step 8 last |
| IB sysfs renderer location | pkg/network/mockib/render/render.go, not sysfs |
gid_tbl_len rendered |
no, zero hits in Go source |
| IB attributes rendered | 18, plus gids/0, pkeys/0, gid_attrs/, counters/ |
| IMEX major hardcoded | no, IMEX_CHANNEL_MAJOR override, single var feeds renderer and mknod |
nvidia-uvm major hardcoded |
yes, 510 with no override |
| NFD label form | repo has a documented position verified against NFD v0.19.0, chart uses local-source feature file, MEP row is accurate |
/proc/modules gap |
confirmed, nothing in tree handles it |
| prior MEPs fill Risks/Drawbacks/Alternatives | yes, both 0001 and 0002 |
| SPDX headers on MEPs | none of the three have them, not a finding |
| markdown lint in CI | not configured |
CODEOWNERS covers enhancements/ |
yes, via default * @ArangoGutierrez |
| PR state | open, not draft, MERGEABLE, approved by giuliocalzo on ea6e3e7, CI green |
|
Roma raised offline that my review reads as an unaddressable wall of text, and he is Format first: I should have posted these as inline threads on their anchors, one per On substance, an MEP is a direction document, not a changeset. These four are the ones
Plus one from B2 that I buried under a wrong kind name, which is my fault for leading Everything else I raised, the counts, the package citations, the line numbers, the kind Leaving the review state as is for now so the four above do not get lost, happy to clear |
Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
…vml mock codebase 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>
Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
…oma/meps/619-node-agent
|
@ArangoGutierrez thank you for the review!
I have updated the specification to be more precise about
Updated the proposal to expose state-fetch-time errors.
This is a major one. If there is an order between components or order of operations than one Reconcile() method won't be helpful. I've followed that prepare/publish two stage idea and introduces To be precise, this is an issue today as well despite having a sequential bash file. NFD file is updated before pci tree is rendered or simdriver is applied which doesn't seem like the right order of operations. |
ArangoGutierrez
left a comment
There was a problem hiding this comment.
The rework answers everything structural I raised, so I am clearing the block. You said you would start implementing once this is approved, and nothing left here is worth holding that up.
What you changed and I think is now right:
- Splitting
ReconcileintoStageandApplywith a barrier, and putting onlycdi,gpudriverandpcibusbehindApplier. Naming the reason (containerd cannot un-admit a container) is the part that will keep the invariant alive when someone adds simulator eight. - Teardown as
RevokethenDiscard, so the inverse order is written once in the agent. Using plainerrgroup.Groupthere for best-effort, andWithContextfor staging, is the right distinction. Update{State, Err, At}plusGeneration. Silence and failure are now different things, and the closed-channel and zero-Statesemantics are stated rather than implied.- Dropping
nodeidentityand delegating the node label out of the agent. The Tech Debt appendix explaining where each surface went is better than deleting the section outright. /healthzas liveness only, with the fleet-restart reasoning spelled out.
Most of my remaining comments come from Tuesday's sync rather than from the document, because that conversation surfaced two things this MEP is silent about and one constraint it is arguably designed against. The IB one is the only comment I would like resolved in the text before merge, and it is a paragraph, not a redesign. Everything else can be follow-up.
Two I would rather file as issues than block on: reboot survival (Aleksei saw the GPU Operator lose its NVML links after a node reboot and report no devices; /dev is devtmpfs on most distros so the chardevs do not survive, and the race against GPU Operator coming up is unspecified), and super pods (roughly 8 racks, and SGPURackIdentity.CliqueID is pinned Minimum=0 Maximum=0 in the merged CRDs, so multi-rack fabric domains are blocked at the API before they are blocked here).
Drawbacks and Alternatives are still template comments. Not blocking, but the honest Drawback is in my first inline comment: one process replacing a dozen means one panic takes out every simulated surface on the node.
|
|
||
| ### Non-Goals | ||
|
|
||
| - This proposal wants to keep the current simulation logic identical. Any missing coverage or improvements should be done outside the MEP. |
There was a problem hiding this comment.
The one I would like in the text before merge.
Aleksei had to turn the IB mock off at 2,304 nodes on Mistral. His words: CPU over the roof, contract issues, and turning the IB simulator off made it healthy again. He attributed it to agents re-broadcasting the whole topology repeatedly, and said the fix belongs in the new agent architecture.
This line freezes that subsystem by policy, while line 336 marks it fully covered and line 251 names it the dominant startup cost. The series this is for targets 5K nodes, so we would be institutionalising a component known to fall over at under half of that, behind a coverage table that reads green.
Not asking you to fix it here. Asking for a non-goal and a risk that say it out loud, so the next reader does not take the table at face value. If the Run contract should eventually bound what a simulator may broadcast and how often, this is the cheapest moment to say so.
There was a problem hiding this comment.
The series this is for targets 5K nodes, so we would be institutionalising a component known to fall over at under half of that, behind a coverage table that reads green.
@ArangoGutierrez em, IB is a part of the mokka logic and it remains to be so even if it has issues. The fact we have an issue with it doesn't mean that it has anything to do with this proposal that is orthogonal to that bug.
There should be still a way to disable IB simulator after the proposal is implemented so @avasilevskii 's workaround still works.
| - Mokka Node Agent should act as a supervisor that gives a shared lifecycle to all e.g. `Stage()`, `Apply()`, `Run()` (for long-lived operations like IB sim servers), `Revoke()`, `Discard()`. It will act as a reconciler. | ||
|
|
||
| Additionally, we want to run as much of the simulation logic in parallel as possible, leveraging their independence, | ||
| so they apply as fast as available CPU/IOPS allow. |
There was a problem hiding this comment.
Carlos's constraint from the sync: Mokka has to keep running on a GitHub Actions runner, which is 2 vCPU, and the DRA folks are the customer that protects. You and Aleksei both agreed the standalone agent plus YAML has to stay viable.
The stage wave is 7 simulators, and gpudriver.Stage fans out 7 more inside it, on unbounded errgroups. On 2 vCPU that is contention rather than speed. Worth stating the small-runner target as a non-functional requirement and bounding the fan-out with SetLimit(GOMAXPROCS). Cheap now, awkward once seven packages each have their own errgroup.
Related: the sync also agreed standalone with a YAML and no control plane is a supported mode, and that bare metal should not be an afterthought. Dropping the K8s API dependency got you most of the way there; stating it costs a sentence.
There was a problem hiding this comment.
Since golang 1.25, GOMAXPROCS is cgroup-aware: https://go.dev/doc/go1.25#container-aware-gomaxprocs
Nothing to do here, Golang runtime is smart enough to schedule goroutines without micromanagement from our side.
| 3. **Barrier, then apply wave (parallel)** — once every `Stage` has returned, call `Apply(ctx, host, state)` on every `Applier`. The barrier is what stops containerd admitting a container against a CDI spec whose chardevs do not exist yet; reconciling again cannot undo that admission. | ||
| 4. **Supervisor wave (parallel, launched once)** — each simulator's `Run(ctx)` is launched under a supervisor `errgroup` at startup. Runs continue across state changes; only a canceled `ctx` stops them. | ||
| 5. Expose `/healthz` + `/readyz` HTTP endpoints, aggregated + per-simulator (shape from `cmd/nvml-mock-nri/main.go:107` `serveHealth`). `/healthz` is liveness only and never depends on `StateSource` reachability: otherwise one Control Plane outage restarts — and per step 6, tears down — the whole fleet at once. `/readyz` means the simulators reconciled the last accepted `State`, and is red only until the first one arrives; later staleness is a metric over `Update.At` and `State.Generation`, not a probe. | ||
| 6. On `ctx.Done()`: cancel `Run` goroutines, `Revoke` on every `Applier` in parallel, then — after that wave completes — `Discard` on every simulator in parallel. Both teardown waves use `errgroup.Group` rather than `errgroup.WithContext`: staging wants fail-fast, teardown wants best-effort so one stuck simulator does not strand the rest of the host. |
There was a problem hiding this comment.
Carried over from the first pass, and slightly more acute now: ctx is already done at step 6, and teardown is now two sequential waves instead of one. Anything context-aware in Revoke or Discard fails immediately, so the node keeps advertising surfaces the agent believes it retracted.
internal/controlplane/server.go:84 already does the fix: context.WithTimeout(context.WithoutCancel(ctx), timeout). Worth one line here so the implementation does not inherit the bug, budgeted under terminationGracePeriodSeconds.
| type State struct { | ||
| Generation int64 // MEP-0001 allocation generation; reported back as observed | ||
| Node NodeMeta // hostRoot, nodeName, hostname | ||
| Software SoftwareVersions // driver / NVML / CUDA (MEP-0001 §SGPUProfile.spec.software) |
There was a problem hiding this comment.
There is no SGPUProfile kind. The merged types are SGPURackProfile, SGPUInventory, SGPURuntimePolicy and SGPURack, and groupversion_info_test.go:29 explicitly asserts SGPUProfile is not recognised.
You want SGPURackProfile.spec.software, which is SGPUSoftware{DriverVersion, NVMLVersion, CUDAVersion} and maps onto SoftwareVersions cleanly.
| Node NodeMeta // hostRoot, nodeName, hostname | ||
| Software SoftwareVersions // driver / NVML / CUDA (MEP-0001 §SGPUProfile.spec.software) | ||
| NodeShape NodeShape // GPU count, host CPU, PCIe/NUMA topology, GPU fabric, network | ||
| Devices []DeviceState // per-GPU: identity + hardware + runtime |
There was a problem hiding this comment.
DeviceState is already taken in the merged API: shared_runtime_types.go:21 defines it as a health enum (Healthy, Degraded, Failed). internal/agent will import that package, so this collides. DeviceSpec or GPUState avoids it.
| | `/sys/class/infiniband/mlx5_<N>/*` | Network Operator, `ibstat`, `ibv_devinfo`, DCGM fabric metrics, Topograph | ✓ `ibhca` via `pkg/network/mockib/render` — full HCA surface (18 file attributes + `gids/`, `pkeys/`, `counters/`, `gid_attrs/` subdirs); `gids/0` derived as `fe80::+port_guid` | | ||
| | `libibverbs` C ABI (`ibv_get_device_list`, `ibv_open_device`, `ibv_query_port`, ...) | RDMA-aware apps, MPI, DCGM | ✓ `ibhca` stages `libibmockverbs.so` for LD_PRELOAD | | ||
| | `libibumad` UMAD socket protocol | admin/diagnostic tools (`ibping`, `iblinkinfo`, `ibnetdiscover`, `sminfo`) | ✓ `libibmockumad.so` LD_PRELOAD → Unix socket to in-process `mock-ib` daemon via `pkg/network/mockib/daemon.Server`; `Run()` supervises the daemon | | ||
| | Cross-node fabric relay (TCP, `MOCK_IB=full`) | multi-node `iblinkinfo`, subnet discovery | ✓ `ibhca` fabric mode via `pkg/network/mockib/fabric` | |
There was a problem hiding this comment.
This is the specific row Aleksei disabled in production at 2,304 nodes. Suggest marking it partial, with a note that it is not currently viable at multi-thousand-node scale, rather than a clean tick. Same argument as my comment on the Non-Goals, but this is the cell someone will actually read when scoping a run.
Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
…rmination Signed-off-by: Roman Hlushko <rhlushko@nvidia.com>
What This PR Does
This proposal suggests a refactoring of the current state of the NVML mock CLIs in order to:
Why
This is a followup proposal stemmed from MEP0001 (Mokka Control Plane).
The proposal addresses the current challenges with the NVML mock organization and make it possible to connect node agent to the control plane in the future PRs.
Checklist
git commit -s)go test -v -race ./...)make lint-fix)