feat(dev): DPF operator simulator for machine-a-tron (#3323)#3807
feat(dev): DPF operator simulator for machine-a-tron (#3323)#3807shayan1995 wants to merge 6 commits into
Conversation
Summary by CodeRabbit
WalkthroughAdds a standalone Go-based DPF simulator that creates and advances DPU resources through the NICo-observed lifecycle, with Kubernetes deployment tooling and documentation. It also persists the DPU agent boot state and makes discovery ChangesDPF simulator
Machine-state recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DPUDevice
participant DPUDeviceReconciler
participant DPUNode
participant DPU
DPUDevice->>DPUDeviceReconciler: trigger reconciliation
DPUDeviceReconciler->>DPUNode: resolve parent node
DPUDeviceReconciler->>DPU: create or read DPU
DPUDeviceReconciler->>DPUNode: coordinate hold or reboot annotations
DPUDeviceReconciler->>DPU: advance phase after gate
DPU-->>DPUDeviceReconciler: owned-resource event
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)dev/k8s/dpf-sim-controller/DockerfileTraceback (most recent call last): dev/k8s/dpf-sim-controller/config/manager/manager.yamlTraceback (most recent call last): dev/k8s/dpf-sim-controller/config/rbac/role.yamlTraceback (most recent call last): Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
dev/k8s/dpf-sim-controller/config/manager/manager.yaml (1)
20-24: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet an explicit container security context.
runAsNonRootis a good baseline, but privilege escalation remains unspecified and the container does not explicitly drop capabilities, require the runtime seccomp profile, or use a read-only root filesystem. Add these restrictions for defense in depth; Checkov already flags the missingallowPrivilegeEscalation: false.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dev/k8s/dpf-sim-controller/config/manager/manager.yaml` around lines 20 - 24, Update the manager container’s securityContext to explicitly set allowPrivilegeEscalation to false, drop all Linux capabilities, require the RuntimeDefault seccomp profile, and enable readOnlyRootFilesystem while preserving runAsNonRoot: true.Source: Linters/SAST tools
dev/k8s/dpf-sim-controller/Dockerfile (1)
2-11: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin both base images by digest.
golang:1.25andgcr.io/distroless/static:nonrootare mutable tags, so later rebuilds may silently use different toolchains or runtime layers. Pin verified digests while retaining the explicitlinux/amd64build path.Suggested change
-FROM golang:1.25 AS build +FROM golang:1.25@sha256:<verified-digest> AS build ... -FROM gcr.io/distroless/static:nonroot +FROM gcr.io/distroless/static:nonroot@sha256:<verified-digest>As per path instructions, Dockerfiles should provide reproducible builds and explicit architecture support.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dev/k8s/dpf-sim-controller/Dockerfile` around lines 2 - 11, Pin both the build image in the FROM golang stage and the runtime image in the FROM gcr.io/distroless stage to verified immutable digests, preserving the existing linux/amd64 architecture target and build flow.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dev/k8s/dpf-sim-controller/cmd/main.go`:
- Around line 46-50: Validate phaseDwell after flag.Parse and before
constructing the reconciler, rejecting any value less than zero with a clear
error and terminating startup. Preserve zero as valid and leave the existing
positive-duration behavior unchanged.
In `@dev/k8s/dpf-sim-controller/config/manager/manager.yaml`:
- Around line 23-27: Replace the mutable dpf-sim-controller image tag in
dev/k8s/dpf-sim-controller/config/manager/manager.yaml and update the
corresponding deployment logic in dev/k8s/dpf-sim-controller/Makefile so
redeployments use an immutable tag or digest, or explicitly refresh the workload
when IMG remains mutable. Ensure both sites consistently prevent reuse of stale
simulator image bytes.
In `@dev/k8s/dpf-sim-controller/config/rbac/role.yaml`:
- Around line 17-46: Replace the ClusterRole and ClusterRoleBinding with a
namespaced Role and RoleBinding for the configured dpfNamespace, using the
existing Makefile namespace substitution mechanism in metadata.namespace and the
binding subject namespace. Update the role’s DPU rule to remove the delete verb
while preserving the other required permissions and resource rules.
In `@dev/k8s/dpf-sim-controller/internal/controller/dpudevice_controller.go`:
- Around line 65-70: Update the error handling around resolveNodeID in Reconcile
so only its dedicated “no referencing node yet” sentinel follows the delayed
requeue path. Propagate all other errors, including API, RBAC, transport, and
serialization failures, instead of returning a successful result; preserve the
existing logging and requeue behavior for the sentinel case.
- Around line 232-236: Before creating the DPU in the reconciliation flow,
validate that device.Labels[carbide.LabelDPUMachineID] is non-empty. If it is
missing or empty, requeue and avoid persisting the DPU; preserve the existing
label propagation once the required machine ID is available.
- Around line 299-305: Update rebootHandshake around setNodeAnnotation and
setDPUAnnotation so a successful first patch cannot lose the reboot intent when
the second patch fails. Persist the request state durably before or alongside
these updates, or make subsequent reconciliation recoverable when either the
node annotation or DPU marker exists; ensure the next reconcile does not issue a
duplicate reboot after NICo clears AnnRebootRequired.
- Around line 200-205: Update the DPU reference matching in the node lookup loop
to use only exact supported values: retain ref.Name == device.Name and replace
the strings.HasSuffix comparison with ref.Name == deviceID. Remove any
now-unused strings dependency if applicable.
In `@dev/k8s/dpf-sim-controller/README.md`:
- Around line 13-23: Update the README’s three unlabeled fenced blocks—the
diagram, phase sequence, and layout tree—to use text fences, and remove the
blank line between adjacent blockquotes or replace it with a normal separator so
Markdown lint passes MD028.
---
Nitpick comments:
In `@dev/k8s/dpf-sim-controller/config/manager/manager.yaml`:
- Around line 20-24: Update the manager container’s securityContext to
explicitly set allowPrivilegeEscalation to false, drop all Linux capabilities,
require the RuntimeDefault seccomp profile, and enable readOnlyRootFilesystem
while preserving runAsNonRoot: true.
In `@dev/k8s/dpf-sim-controller/Dockerfile`:
- Around line 2-11: Pin both the build image in the FROM golang stage and the
runtime image in the FROM gcr.io/distroless stage to verified immutable digests,
preserving the existing linux/amd64 architecture target and build flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 117b6fec-484a-4fac-bffb-2a48b61207a3
⛔ Files ignored due to path filters (1)
dev/k8s/dpf-sim-controller/go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
crates/machine-a-tron/src/machine_state_machine.rsdev/k8s/dpf-sim-controller/.gitignoredev/k8s/dpf-sim-controller/Dockerfiledev/k8s/dpf-sim-controller/Makefiledev/k8s/dpf-sim-controller/PROJECTdev/k8s/dpf-sim-controller/README.mddev/k8s/dpf-sim-controller/cmd/main.godev/k8s/dpf-sim-controller/config/manager/manager.yamldev/k8s/dpf-sim-controller/config/rbac/role.yamldev/k8s/dpf-sim-controller/config/samples/dpudevice_dpunode.yamldev/k8s/dpf-sim-controller/go.moddev/k8s/dpf-sim-controller/internal/carbide/labels.godev/k8s/dpf-sim-controller/internal/carbide/labels_test.godev/k8s/dpf-sim-controller/internal/controller/dpudevice_controller.godev/k8s/dpf-sim-controller/internal/simulator/phases.godev/k8s/dpf-sim-controller/internal/simulator/phases_test.go
| // cadence. | ||
| switch simulator.Gate(dpu.Status.Phase) { | ||
| case simulator.GateHold: | ||
| held, err := r.nodeHasAnnotationTrue(ctx, nodeName, carbide.AnnHoldNodeEffect) |
There was a problem hiding this comment.
Should this check DPUNodeMaintenance instead of DPUNode? NICo calls release_maintenance_hold (
infra-controller/crates/machine-controller/src/handler/dpf.rs
Lines 369 to 372 in 3070ef0
infra-controller/crates/dpf/src/sdk.rs
Lines 1742 to 1760 in 3070ef0
There was a problem hiding this comment.
The simulator now creates the DPUNodeMaintenance CR named {node}-hold (with the wait-for-external-nodeeffect annotation) on Node Effect entry and parks until NICo's release patches it to "false" — matching release_maintenance_hold's actual patch target. Fixed in ad6e7b4.
| Spec: provisioningv1.DPUSpec{ | ||
| DPUNodeName: nodeName, | ||
| DPUDeviceName: device.Name, | ||
| SerialNumber: device.Spec.SerialNumber, | ||
| // DPUFlavor and BFB are required by the CRD but have no meaning for | ||
| // the simulator; use placeholder values so the CR is accepted. | ||
| DPUFlavor: "sim", | ||
| BFB: "sim", | ||
| // NoEffect: the simulator never touches real K8s node taints/drains. | ||
| // NodeEffect embeds Action; NoEffect lives on Action, not NodeEffect directly. | ||
| NodeEffect: provisioningv1.NodeEffect{ | ||
| Action: provisioningv1.Action{NoEffect: &noEffect}, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
The simulator’s created DPU spec does not populate BMCIP.
NICo’s DPF watcher explicitly skips the reboot callback (
infra-controller/crates/dpf/src/watcher.rs
Lines 369 to 380 in 3070ef0
There was a problem hiding this comment.
DPU.spec.bmcIP is now populated with the host BMC IP taken from the carbide.nvidia.com/host-bmc-ip label NICo puts on the DPUDevice (bare IP so the watcher's parse succeeds), and DPU creation waits until that label is populated. Fixed in ad6e7b4.
| if err := controllerutil.SetControllerReference(device, &dpu, r.Scheme); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
| if err := controllerutil.SetControllerReference(device, &dpu, r.Scheme); err != nil { | |
| return nil, err | |
| } | |
| controllerutil.SetControllerReference( | |
| device, | |
| &dpu, | |
| r.Scheme, | |
| controllerutil.WithBlockOwnerDeletion(false), | |
| ) |
SetControllerReference defaults blockOwnerDeletion to true, but this service account only has get/list/watch on DPUDevice. Kubernetes can therefore reject DPU creation because setting that owner reference requires additional permission on the owner.
https://kubernetes.io/docs/reference/kubernetes-api/definitions/owner-reference-v1-meta/
There was a problem hiding this comment.
Applied as suggested — WithBlockOwnerDeletion(false), with a comment explaining the RBAC interaction. Fixed in ad6e7b4.
| return false, client.IgnoreNotFound(err) | ||
| } | ||
| nodeWantsReboot := node.Annotations[carbide.AnnRebootRequired] == "true" | ||
| alreadyRequested := dpu.Annotations[carbide.AnnSimRebootRequested] == "true" |
There was a problem hiding this comment.
This marker is per DPU, but the reboot is shared by every DPU on the DPUNode. After NICo clears the annotation for the first reboot, another DPU without this marker will request another host reboot. Real DPF handles this in the DPUNode controller and completes all rebooting DPUs from one annotation cycle. Could this bookkeeping be node-level instead?
There was a problem hiding this comment.
Reboot bookkeeping is now node-level: the request annotation and a requested-at stamp go down in one DPUNode patch, completion is stamped on the node, and a DPU is satisfied iff a cycle completed after it entered Rebooting — one NICo cycle completes every DPU already rebooting on the node, and no duplicate host reboots are requested for the same walk. Fixed in ad6e7b4.
Add a development-only simulator that plays the DPF operator's half of the
NICo <-> DPF contract so machine-a-tron simulated fleets progress past dpuinit
without real BlueField hardware or a real DPF install. It watches the
DPUDevice/DPUNode CRs NICo creates, creates the matching DPU CR
(node-{dpfID}-device-{deviceID}, with the machine-id label copied from the
DPUDevice), and walks status.phase through the authentic doca-platform v26.4.0
sequence to Ready, honoring the two dances the machine controller depends on:
the node-effect hold, and the reboot round-trip (sets the reboot-required
annotation, waits for NICo to clear it, using a simulator-local marker so a
clear-by-deletion is told apart from never-requested).
CR types come straight from github.com/nvidia/doca-platform v26.4.0 (no
hand-written schema), pinned to match the CRDs NICo ships in crates/dpf/crds.
DPU.Spec is filled with the CRD-required fields (dpuNodeName, dpuDeviceName,
serialNumber, nodeEffect=noEffect, placeholder dpuFlavor/bfb) so the object is
accepted; status.phase is what NICo actually reads.
Ships least-privilege RBAC on the DPF CRs, a Deployment, a standalone
DPUDevice/DPUNode sample, and a one-command `make deploy IMG=... [DPF_NAMESPACE=]
[PULL_SECRET=]` that installs ONLY the CRDs the simulator drives — never the DPF
operator, so it is independent of the setup.sh DPF-install work and cannot fight
a real operator over DPU status. go build / go vet / go test pass.
Signed-off-by: Shayan Namaghi <snamaghi@nvidia.com>
At MAT cold start the simulated machines power on and PXE-boot the DPU
agent image before site-explorer has created their machine records, so
the agent's initial DiscoverMachine fails with InvalidArgument
("not discovered by site-explorer"). Treating that as terminal
(MachineNotFound -> FailedAndWaitForReboot) permanently loses the
installed OS: by the next boot the host has moved past DpuInitState::Init,
PXE answers EXIT, and the DPU boots OsImage::None forever. With DPF
enabled this deadlocks every host in dpuinit — NICo waits for a DPU agent
that MAT believes was never installed
("Waiting for DPU agent to apply network config").
On real hardware this window does not exist: NICo powers hosts on only
after machine creation. It is purely a MAT cold-start ordering artifact,
so treat InvalidArgument as retryable and re-run the queued discovery
action until site-explorer creates the machine.
Verified live on a simulation cluster: 3 hosts x 2 DPUs, all discovery
retries converge after ingestion, DPU agents boot, and all machines
reach the terminal ready state (with the dpf-sim-controller from NVIDIA#3323
playing the DPF operator role).
Signed-off-by: Shayan Namaghi <snamaghi@nvidia.com>
NICo's PXE serves a DPU EXIT in every DPUInit sub-state after Init — on real hardware the DPF/BFB install has written an OS to the DPU's disk by then, so EXIT boots it. The simulation never modeled that write: installed_os was only recorded after a successful initial discovery, which can fail at MAT cold start (the machine record may not exist yet — the same window the discovery-retry fix addresses, reachable via NotFound as well as InvalidArgument). Any mid-walk reboot — BIOS setup, the DPF reboot handshake, an external power-cycle — then stranded the DPU OS-less: agent dead, no network-health reports, dpuinit parked at waitingfornetworkconfig forever, and the host's DHCP-through-DPU relay timing out. Record the netbooted agent image as installed at PXE-boot time (the simulation's equivalent of the BFB disk write), so every later EXIT boots the agent from "disk" exactly like real hardware. Validated end-to-end on a simulation cluster: DPUs now survive reboots at every walk phase and the 9-machine fleet reaches terminal ready. Signed-off-by: Shayan Namaghi <snamaghi@nvidia.com>
Review follow-ups from NVIDIA#3807 (poroh + coderabbit), all verified against the NICo source of truth (crates/dpf/src/sdk.rs, watcher.rs, machine-controller/src/handler/dpf.rs): - Node-effect hold: the handshake lives on a DPUNodeMaintenance CR named {node}-hold that the OPERATOR creates — NICo's release_maintenance_hold only patches its wait-for-external-nodeeffect annotation to "false" (404-no-op if absent). The simulator now creates that CR on Node Effect entry and parks until the release lands, instead of reading an annotation off the DPUNode that NICo never writes. - Reboot bookkeeping is node-level, written atomically: NICo clears the dpunode-external-reboot-required annotation once per node, and real DPF completes every rebooting DPU from that one cycle. The request annotation and the simulator's requested-at marker go down in a single DPUNode patch (a partial write can no longer lose intent), completion is stamped on the node, and a DPU is satisfied iff a cycle completed after it entered Rebooting — so DPUs sharing a host no longer trigger duplicate reboots. - DPU.spec.bmcIP now carries the host BMC IP (from the carbide.nvidia.com/host-bmc-ip label): NICo's watcher silently skips the Rebooting callback when it is absent or unparseable. - DPU creation waits for NICo to populate the dpu-machine-id and host-bmc-ip labels instead of persisting an empty mapping; resolveNodeID matches DPURef names exactly (no suffix matching) and only the no-referencing-node case takes the polling requeue — real list/client failures propagate to controller-runtime. Signed-off-by: Shayan Namaghi <snamaghi@nvidia.com>
Remaining NVIDIA#3807 review follow-ups: - RBAC: namespaced Role/RoleBinding instead of ClusterRole (grants now match the namespace-scoped cache/reconciler), no delete verb (ownerRef GC covers cleanup), plus get/list/watch/create on dpunodemaintenances for the hold handshake. - Deployment: imagePullPolicy Always + a rollout restart in make deploy, so repushing a mutable tag (:dev) actually lands; securityContext hardening (seccomp RuntimeDefault, no privilege escalation, all capabilities dropped, read-only root fs). - Dockerfile: base images pinned by digest for reproducible rebuilds. - main.go: reject a negative --phase-dwell instead of hot-looping. Signed-off-by: Shayan Namaghi <snamaghi@nvidia.com>
Review follow-ups from NVIDIA#3807 (poroh + coderabbit), all verified against the NICo source of truth (crates/dpf/src/sdk.rs, watcher.rs, machine-controller/src/handler/dpf.rs): - Node-effect hold: the handshake lives on a DPUNodeMaintenance CR named {node}-hold that the OPERATOR creates — NICo's release_maintenance_hold only patches its wait-for-external-nodeeffect annotation to "false" (404-no-op if absent). The simulator now creates that CR on Node Effect entry and parks until the release lands, instead of reading an annotation off the DPUNode that NICo never writes. - Reboot bookkeeping is node-level, written atomically: NICo clears the dpunode-external-reboot-required annotation once per node, and real DPF completes every rebooting DPU from that one cycle. The request annotation and the simulator's requested-at marker go down in a single DPUNode patch (a partial write can no longer lose intent), completion is stamped on the node, and a DPU is satisfied iff a cycle completed after it entered Rebooting — so DPUs sharing a host no longer trigger duplicate reboots. - DPU.spec.bmcIP now carries the host BMC IP (from the carbide.nvidia.com/host-bmc-ip label): NICo's watcher silently skips the Rebooting callback when it is absent or unparseable. - DPU creation waits for NICo to populate the dpu-machine-id and host-bmc-ip labels instead of persisting an empty mapping; resolveNodeID matches DPURef names exactly (no suffix matching) and only the no-referencing-node case takes the polling requeue — real list/client failures propagate to controller-runtime. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Shayan Namaghi <snamaghi@nvidia.com>
Remaining NVIDIA#3807 review follow-ups: - RBAC: namespaced Role/RoleBinding instead of ClusterRole (grants now match the namespace-scoped cache/reconciler), no delete verb (ownerRef GC covers cleanup), plus get/list/watch/create on dpunodemaintenances for the hold handshake. - Deployment: imagePullPolicy Always + a rollout restart in make deploy, so repushing a mutable tag (:dev) actually lands; securityContext hardening (seccomp RuntimeDefault, no privilege escalation, all capabilities dropped, read-only root fs). - Dockerfile: base images pinned by digest for reproducible rebuilds. - main.go: reject a negative --phase-dwell instead of hot-looping. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Shayan Namaghi <snamaghi@nvidia.com>
3070ef0 to
3883f22
Compare
|
Rebased onto current main ( @poroh's items (all in CodeRabbit items: namespaced Role/RoleBinding without New commit Also documented in the README: simulation sites on cores including #3561 must set A full clean e2e rerun (teardown → setup.sh → simulator → MAT → 9/9 machines ready) from this exact stack is in progress; results will be posted here. 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
dev/k8s/dpf-sim-controller/Makefile (2)
24-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake manifest overrides fail closed.
The renderer depends on exact literals in
config/manager/manager.yaml. A formatting or default-value change can silently makeIMGorPHASE_DWELLineffective. Prefer structured patching, or verify that each requested substitution occurred before applying the manifest.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dev/k8s/dpf-sim-controller/Makefile` around lines 24 - 28, Update the Makefile’s RENDER_MGR manifest-rendering flow to fail closed when IMG or PHASE_DWELL substitutions do not match their expected manager.yaml literals. Prefer structured manifest patching, or make the existing substitution command verify that each requested replacement occurred and return failure otherwise; preserve namespace replacement behavior.
67-79: 🩺 Stability & Availability | 🔵 TrivialFail closed on the Kubernetes target.
These commands mutate whichever cluster is selected by the ambient
kubectlcontext, including cluster-scoped CRDs and RBAC. Add explicitKUBE_CONTEXTplumbing and validation, or require a simulation-cluster marker before applying resources.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dev/k8s/dpf-sim-controller/Makefile` around lines 67 - 79, Update the deploy target and its underlying install-crds/RBAC/deployment commands to require an explicit KUBE_CONTEXT, validate that it is provided and targets the intended simulation cluster, and pass it to every kubectl invocation, including namespace creation, resource application, patching, rollout restart, and rollout status. Fail before any mutation when the context is missing or invalid.Source: Path instructions
dev/k8s/dpf-sim-controller/internal/controller/dpudevice_controller.go (1)
429-438: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftNo test coverage for the reconciliation state machine.
Reconcile's hold/reboot/dwell gating and the reboot handshake's timestamp-based satisfaction logic are intricate enough that prior review rounds surfaced several subtle correctness bugs here (exact-match regressions, partial-write loss, per-DPU vs per-node bookkeeping). None of that logic has accompanying tests in this file set. A fake-client/envtest-backed table-driven suite coveringresolveNodeID,ensureDPU,rebootHandshake, andmaintenanceHoldActivewould catch regressions before they reach the simulator's e2e validation. As per coding guidelines, write tests in table-driven style for critical input/output logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dev/k8s/dpf-sim-controller/internal/controller/dpudevice_controller.go` around lines 429 - 438, Expand the DPUDevice controller test suite with table-driven fake-client or envtest coverage for resolveNodeID, ensureDPU, rebootHandshake, and maintenanceHoldActive. Exercise Reconcile’s hold, reboot, and dwell gating, including exact timestamp matches, partial-write preservation, and per-DPU versus per-node bookkeeping, while asserting each expected state transition and result.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dev/k8s/dpf-sim-controller/config/rbac/role.yaml`:
- Around line 26-38: Add an RBAC rule to the Role for the dpunodemaintenances
resource in the provisioning.dpu.nvidia.com API group, granting get, list,
watch, and create to match the controller declarations and maintenanceHoldActive
operations used by the GateHold path. Keep the existing rules unchanged.
In `@dev/k8s/dpf-sim-controller/Makefile`:
- Around line 81-84: Update the undeploy target to use the same DPF_NAMESPACE as
the preceding deploy, rather than silently rendering the default namespace.
Persist or discover the namespace used by deploy and reuse it during undeploy,
or require an explicit matching DPF_NAMESPACE before running the existing
RENDER_MGR and RENDER_RBAC deletion commands.
- Around line 53-60: Update the docker-build and image targets to build and
publish images for both linux/amd64 and linux/arm64, replacing the current
single-architecture docker build while preserving the existing IMG tag and push
workflow.
- Around line 64-70: Update the deploy target after install-crds and before
applying the manager manifests to wait until the DPUDevice, DPUNode, and DPU
CRDs reach the Established condition using kubectl wait. Keep the existing
namespace and RBAC steps unchanged, and ensure manager startup only proceeds
after all three CRDs are established.
---
Nitpick comments:
In `@dev/k8s/dpf-sim-controller/internal/controller/dpudevice_controller.go`:
- Around line 429-438: Expand the DPUDevice controller test suite with
table-driven fake-client or envtest coverage for resolveNodeID, ensureDPU,
rebootHandshake, and maintenanceHoldActive. Exercise Reconcile’s hold, reboot,
and dwell gating, including exact timestamp matches, partial-write preservation,
and per-DPU versus per-node bookkeeping, while asserting each expected state
transition and result.
In `@dev/k8s/dpf-sim-controller/Makefile`:
- Around line 24-28: Update the Makefile’s RENDER_MGR manifest-rendering flow to
fail closed when IMG or PHASE_DWELL substitutions do not match their expected
manager.yaml literals. Prefer structured manifest patching, or make the existing
substitution command verify that each requested replacement occurred and return
failure otherwise; preserve namespace replacement behavior.
- Around line 67-79: Update the deploy target and its underlying
install-crds/RBAC/deployment commands to require an explicit KUBE_CONTEXT,
validate that it is provided and targets the intended simulation cluster, and
pass it to every kubectl invocation, including namespace creation, resource
application, patching, rollout restart, and rollout status. Fail before any
mutation when the context is missing or invalid.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f3b1e5d5-bdc3-4fc6-9ec1-96f76221f677
⛔ Files ignored due to path filters (1)
dev/k8s/dpf-sim-controller/go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
crates/machine-a-tron/src/machine_state_machine.rsdev/k8s/dpf-sim-controller/.gitignoredev/k8s/dpf-sim-controller/Dockerfiledev/k8s/dpf-sim-controller/Makefiledev/k8s/dpf-sim-controller/PROJECTdev/k8s/dpf-sim-controller/README.mddev/k8s/dpf-sim-controller/cmd/main.godev/k8s/dpf-sim-controller/config/manager/manager.yamldev/k8s/dpf-sim-controller/config/rbac/role.yamldev/k8s/dpf-sim-controller/config/samples/dpudevice_dpunode.yamldev/k8s/dpf-sim-controller/go.moddev/k8s/dpf-sim-controller/internal/carbide/labels.godev/k8s/dpf-sim-controller/internal/carbide/labels_test.godev/k8s/dpf-sim-controller/internal/controller/dpudevice_controller.godev/k8s/dpf-sim-controller/internal/simulator/phases.godev/k8s/dpf-sim-controller/internal/simulator/phases_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- dev/k8s/dpf-sim-controller/.gitignore
- dev/k8s/dpf-sim-controller/internal/carbide/labels_test.go
- dev/k8s/dpf-sim-controller/go.mod
- dev/k8s/dpf-sim-controller/Dockerfile
- dev/k8s/dpf-sim-controller/PROJECT
- dev/k8s/dpf-sim-controller/config/samples/dpudevice_dpunode.yaml
- dev/k8s/dpf-sim-controller/config/manager/manager.yaml
- dev/k8s/dpf-sim-controller/cmd/main.go
- dev/k8s/dpf-sim-controller/internal/simulator/phases_test.go
- dev/k8s/dpf-sim-controller/internal/simulator/phases.go
- dev/k8s/dpf-sim-controller/README.md
| docker build --platform linux/amd64 -t $(IMG) . | ||
|
|
||
| .PHONY: docker-push | ||
| docker-push: ## Push the image at IMG | ||
| docker push $(IMG) | ||
|
|
||
| .PHONY: image | ||
| image: docker-build docker-push ## Build + push the image at IMG |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Build images for both required architectures.
docker-build hard-codes linux/amd64, so make image cannot produce a runnable image for arm64 simulation nodes. Use a multi-platform buildx build, or add an explicit arm64 build/push path before deployment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dev/k8s/dpf-sim-controller/Makefile` around lines 53 - 60, Update the
docker-build and image targets to build and publish images for both linux/amd64
and linux/arm64, replacing the current single-architecture docker build while
preserving the existing IMG tag and push workflow.
Source: Coding guidelines
There was a problem hiding this comment.
Parameterized in 83cabe4: PLATFORMS ?= linux/amd64 with override for arm64/mixed fleets (multi-arch by default would double the build for clusters that are amd64-only here; the README documents the Apple-Silicon cross-compile caveat).
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Makefile architecture/build targets ---'
sed -n '1,130p' dev/k8s/dpf-sim-controller/Makefile
echo
echo '--- README architecture and image-build guidance ---'
rg -n -i -C 4 '(arm64|apple silicon|platform|buildx|docker-build|make image|multi-arch|multiarch)' \
dev/k8s/dpf-sim-controller/README.mdLength of output: 9055
@shayan1995, this is an improvement for a single explicitly selected architecture, but it does not yet support the documented mixed-fleet override.
docker build --platform linux/amd64,linux/arm64 ... cannot produce a multi-platform image through the normal Docker driver; it needs docker buildx build and a registry export such as --push to publish the manifest list. The subsequent standalone docker push $(IMG) does not remedy that.
Accordingly, the finding remains valid for mixed amd64/arm64 deployments—and the repository guideline requires both architecture images before deployment. A production-grade fix is to have the multi-platform path use docker buildx build --platform $(PLATFORMS) --push -t $(IMG) . (with image avoiding a second push), while retaining an amd64-only default if that remains the intended local-development optimization.
✏️ Learnings added
Learnt from: shayan1995
Repo: NVIDIA/infra-controller PR: 3807
File: dev/k8s/dpf-sim-controller/Makefile:0-0
Timestamp: 2026-07-23T01:41:45.684Z
Learning: For the development-only DPF simulator in `dev/k8s/dpf-sim-controller`, the repository’s current simulation clusters are amd64 by default. The Makefile exposes `PLATFORMS` for architecture selection, and mixed amd64/arm64 deployment requires a buildx-based multi-platform image build that publishes a registry manifest.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
…rgonomics Second review round (coderabbit): - role.yaml now carries the dpunodemaintenances get/list/watch/create rule the kubebuilder markers already declared — without it the node-effect hold gate 403s and the DPU parks in Node Effect forever. - make deploy waits for the applied CRDs to reach Established before starting the manager, so a fresh cluster cannot hit cache/watch init failures. - make undeploy defaults to the namespace recorded by the last deploy (.deploy-namespace, gitignored) instead of silently rendering the default and leaving a simulator running in a custom namespace. - docker-build platforms are parameterized (PLATFORMS ?= linux/amd64) for arm64/mixed fleets without doubling the default build. Signed-off-by: Shayan Namaghi <snamaghi@nvidia.com>
3883f22 to
83cabe4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dev/k8s/dpf-sim-controller/Makefile (1)
82-86: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClear stale
imagePullSecretswhenPULL_SECRETis unset.After one deployment sets
PULL_SECRET, a latermake deploy PULL_SECRET=skips this block, leaving the old secret reference on the Deployment. That can make subsequent rollouts fail or retain an unintended credential dependency. Render the field declaratively or explicitly patch it to[]in the empty branch.Suggested fix
`@if` [ -n "$(PULL_SECRET)" ]; then \ echo "patching imagePullSecrets=$(PULL_SECRET)"; \ kubectl -n $(DPF_NAMESPACE) patch deployment dpf-sim-controller --type merge \ -p '{"spec":{"template":{"spec":{"imagePullSecrets":[{"name":"$(PULL_SECRET)"}]}}}}'; \ + else \ + kubectl -n $(DPF_NAMESPACE) patch deployment dpf-sim-controller --type merge \ + -p '{"spec":{"template":{"spec":{"imagePullSecrets":[]}}}}'; \ fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dev/k8s/dpf-sim-controller/Makefile` around lines 82 - 86, Update the deployment patch logic in the Makefile target so the unset PULL_SECRET branch explicitly clears imagePullSecrets to an empty list, while preserving the existing named-secret patch when PULL_SECRET is provided.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dev/k8s/dpf-sim-controller/Makefile`:
- Around line 76-81: Update the deploy target to validate NICO_IMAGE_REGISTRY,
NICO_CORE_IMAGE_TAG, and NICO_REST_IMAGE_TAG before running install-crds or
applying manifests, failing with a clear error when any value is unset or
invalid. Ensure the deployment workflow requires these variables to reference
the pushed amd64/arm64 images and preserves reproducible prerequisites.
---
Outside diff comments:
In `@dev/k8s/dpf-sim-controller/Makefile`:
- Around line 82-86: Update the deployment patch logic in the Makefile target so
the unset PULL_SECRET branch explicitly clears imagePullSecrets to an empty
list, while preserving the existing named-secret patch when PULL_SECRET is
provided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3eaffdd8-e3f9-4406-940b-f7012545e016
📒 Files selected for processing (11)
crates/machine-a-tron/src/machine_state_machine.rsdev/k8s/dpf-sim-controller/.gitignoredev/k8s/dpf-sim-controller/Dockerfiledev/k8s/dpf-sim-controller/Makefiledev/k8s/dpf-sim-controller/README.mddev/k8s/dpf-sim-controller/cmd/main.godev/k8s/dpf-sim-controller/config/manager/manager.yamldev/k8s/dpf-sim-controller/config/rbac/role.yamldev/k8s/dpf-sim-controller/internal/carbide/labels.godev/k8s/dpf-sim-controller/internal/controller/dpudevice_controller.godev/k8s/dpf-sim-controller/internal/simulator/phases.go
🚧 Files skipped from review as they are similar to previous changes (10)
- dev/k8s/dpf-sim-controller/.gitignore
- dev/k8s/dpf-sim-controller/config/manager/manager.yaml
- dev/k8s/dpf-sim-controller/Dockerfile
- dev/k8s/dpf-sim-controller/config/rbac/role.yaml
- dev/k8s/dpf-sim-controller/cmd/main.go
- dev/k8s/dpf-sim-controller/README.md
- dev/k8s/dpf-sim-controller/internal/simulator/phases.go
- dev/k8s/dpf-sim-controller/internal/carbide/labels.go
- crates/machine-a-tron/src/machine_state_machine.rs
- dev/k8s/dpf-sim-controller/internal/controller/dpudevice_controller.go
| .PHONY: deploy | ||
| deploy: install-crds ## One-command deploy: CRDs + RBAC + Deployment (override IMG=, DPF_NAMESPACE=, PULL_SECRET=) | ||
| @echo "$(DPF_NAMESPACE)" > .deploy-namespace | ||
| kubectl get namespace $(DPF_NAMESPACE) >/dev/null 2>&1 || kubectl create namespace $(DPF_NAMESPACE) | ||
| $(RENDER_RBAC) | kubectl apply -f - | ||
| $(RENDER_MGR) | kubectl apply -f - |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate the required image configuration before deployment.
deploy applies the simulator manifests without checking that NICO_IMAGE_REGISTRY, NICO_CORE_IMAGE_TAG, and NICO_REST_IMAGE_TAG are configured to the pushed amd64/arm64 images. Add a preflight check or make these values explicit inputs to the deployment workflow so deployment cannot proceed with an incomplete image contract.
As per coding guidelines, both architectures must be built/pushed and the required NICO image variables must reference them. As per path instructions, development tooling must provide reproducible deployment prerequisites.
🧰 Tools
🪛 checkmake (0.3.2)
[warning] 77-77: Target body for "deploy" exceeds allowed length of 5 lines (13).
(maxbodylength)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dev/k8s/dpf-sim-controller/Makefile` around lines 76 - 81, Update the deploy
target to validate NICO_IMAGE_REGISTRY, NICO_CORE_IMAGE_TAG, and
NICO_REST_IMAGE_TAG before running install-crds or applying manifests, failing
with a clear error when any value is unset or invalid. Ensure the deployment
workflow requires these variables to reference the pushed amd64/arm64 images and
preserves reproducible prerequisites.
Sources: Coding guidelines, Path instructions
|
Clean end-to-end rerun from the final review-fixed stack (this PR + #3808, rebased on Two results worth calling out:
One environment note for simulation clusters on current main (tracked in the validation runbook): the host network-config builder now requires |
2 participants
A machine-a-tron simulation cluster has no BlueField hardware and no DPF operator, so once NICo creates the
DPUDevice/DPUNodeCRs atdpuinit, nothing drivesDPU.status.phaseand every simulated host is stuck there forever. This PR adds the missing counterpart (issue #3323): a development-only controller-runtime controller, deployed alongside machine-a-tron and enabled per site, that plays the DPF operator's role — it watches theDPUDevice/DPUNodeCRs NICo creates, creates the matchingDPUCR with the CRD-required spec fields, and walksstatus.phasethrough the real provisioning sequence toReady, reproducing the exact transitions NICo waits on: the node-effect hold and the external-reboot round-trip (request annotation → NICo power-cycles the host → clear). CR types are imported from the pinned doca-platform module, not hand-written, so the contract can't silently drift from the installed CRDs.Two commits:
feat(dev): DPF operator simulator for machine-a-tron— the simulator underdev/k8s/dpf-sim-controller/(own Go module; reconciler, phase walker, NICo name/label/annotation constants mirrored fromcrates/dpf, RBAC + Deployment manifests, one-commandmake deploythat also installs the in-repo DPF CRDs). Dwell phases are gated on a phase-entry timestamp (not requeue cadence, whichOwns()re-enqueues far faster than), the reboot marker is cleared only after the phase advance is durable, and the manager cache is scoped to the DPF namespace.fix(machine-a-tron): retry DPU discovery until the machine is ingested— the one bug in existing code that end-to-end validation of the simulator exposed. At MAT cold start the simulated machines PXE the DPU-agent image before site-explorer has created their machine records; treating the resultingInvalidArgumentas terminal permanently loses the installed OS (later boots PXE EXIT into "no OS"), deadlocking every DPF-enabled host indpuinit. On real hardware this window doesn't exist — NICo powers hosts on only after machine creation — so the failure is retried until ingestion instead.Validated end-to-end on a simulation cluster (3 hosts × 2 DPUs,
MAT_MODE=scale): all 6 DPUs reachstatus.phase: Readywith NICo completing the reboot handshakes, DPU agents apply network config, and all 9 machines reach the terminalreadystate. That validation run also surfaced an existing machine-controller bug in the DPF reboot handshake, filed separately as #3805 (not addressed here).Related issues
Type of Change
Breaking Changes
Testing
Additional Notes
DPU.status.phase.make deployinstalls only the CRDs (fromcrates/dpf/crds), never the operator.go.modmust match the DPF CRD version installed on the cluster.TODO(#3323)markers are fidelity follow-ups (per-phase dwell durations;DPURef.Namesemantics), not blockers.🤖 Generated with Claude Code