Feat/machine a tron e2e scripts#3258
Closed
shayan1995 wants to merge 14 commits into
Closed
Conversation
…osts Everything needed to take a running NICo site from nothing to a simulated fleet with machines created and managed, validated live on dev6 at 1 -> 100 -> 1000 -> 4500 hosts (x2 DPUs = 13,500 BMC endpoints): - helm-prereqs/setup-machine-a-tron.sh: single idempotent script — namespace, pull secret, CA/Vault secret refresh, full BMC/UEFI credential chain, nico-core site config (bmc_proxy, simulated networks, site_explorer throughput knobs, lo-ip pool), DHCP dual-pool sizing with auto-fit, DB safety checks, cert reissue, throttled helm deploy, and a verification loop that shepherds ingestion (latch clearing + endpoint unparking). - helm-prereqs/cleanup-machine-a-tron.sh: full inverse for reproducible from-scratch runs (per-MAC Vault cred purge batched server-side). - MAT_MODE=scale + values/machine-a-tron-scale.yaml: PROXY-DIRECT transport — site_explorer.bmc_proxy points at the mock's single ClusterIP service and the Redfish client injects "Forwarded: host=<BMC IP>" (RFC 7239), which the mock's shared registry routes on. One service carries the whole fleet; the nginx/MetalLB per-BMC-Service mode remains for real-hardware coexistence. - Chart fixes for the nginx/MetalLB mode: upstream must target the -bmc-mock Service; Forwarded must carry $host (kube-proxy DNATs the LB IP to the pod IP, so $server_addr breaks registry routing); required anti-affinity between proxy replicas (externalTrafficPolicy Local blackholes nodes without a proxy pod) with maxUnavailable=1/maxSurge=0 (surge deadlocks when replicas == nodes). - fix(api): grant Machineatron the AddExpectedMachine permission — its register_expected_machines call was 403'd; the principal already holds the sibling grants (DiscoverDhcp, CreateNetworkSegment, GetExpectedSwitch). - Dockerfile (cross-compiled x86_64 build), deployment guide, and a scale-testing writeup (docs/development/machine-a-tron-scale-testing.md) with the complete 19-issue log, measured ingestion rates, and open questions for review. Results: stage 2 = 3000/3000 machines END TO END OK in one unattended run (~25 min); stage 3 = 13,500/13,500 endpoints explored, 10k+ machines created and progressing through machine-controller lifecycle states on dev-sized postgres, with ingestion running autonomously through extended client connectivity outages.
Contributor
Summary by CodeRabbit
WalkthroughThis PR introduces a new "machine-a-tron" bare-metal simulator: a cross-compiled Docker image, a complete Helm chart (deployment, services, RBAC, MetalLB/nginx proxy, certificates, tests, values), standalone setup/cleanup provisioning scripts, override and scale values profiles, devspace config updates, deployment/scale-testing documentation, and an RBAC permission expansion granting ChangesMachine-a-Tron simulator delivery
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. 🔧 Trivy (0.69.3)Trivy execution failed: 2026-07-08T17:41:17Z FATAL Fatal error run error: fs scan error: scan error: scan failed: failed analysis: post analysis error: post analysis error: helm scan error: fs filter error: fs filter error: walk error range error: stat smartylint.json: no such file or directory: range error: stat smartylint.json: no such file or directory Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (7)
helm/charts/nico-machine-a-tron/templates/bmc-proxy-deployment.yaml (2)
54-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the nginx image to a fixed digest/version.
The default
nginx:alpineis a floating tag that can change underneath you between deployments, hurting reproducibility of scale-test runs.📌 Proposed fix
- image: {{ .Values.nginxBmcProxy.image | default "nginx:alpine" }} + image: {{ .Values.nginxBmcProxy.image | default "nginx:1.27-alpine" }}🤖 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 `@helm/charts/nico-machine-a-tron/templates/bmc-proxy-deployment.yaml` at line 54, The nginx image reference in the BMC proxy deployment is using a floating default tag, so update the image value handling in the bmc-proxy Deployment template to use a fixed, pinned digest or version instead of the fallback in the image field. Keep the change localized to the nginxBmcProxy image reference so deployments remain reproducible across runs.
52-84: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winNo
securityContexton the nginx proxy container.The container runs without
runAsNonRoot/readOnlyRootFilesystemconstraints. Given this proxy terminates TLS and is exposed via LoadBalancer/MetalLB in scale mode, hardening the pod/container security context is worthwhile even for simulation-only tooling.🤖 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 `@helm/charts/nico-machine-a-tron/templates/bmc-proxy-deployment.yaml` around lines 52 - 84, The nginx proxy container in the bmc-proxy deployment is missing a securityContext, so harden the nginx container by adding pod/container security constraints on the nginx container definition. Update the nginx container block in bmc-proxy-deployment.yaml to set runAsNonRoot and readOnlyRootFilesystem (and any other appropriate non-privileged settings) so the proxy runs with minimal privileges while still supporting the TLS volume mounts and probe setup.crates/machine-a-tron/Dockerfile (1)
46-62: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRuntime container has no non-root
USER.The final image runs the
machine-a-tronbinary as root by default. Adding a dedicated non-root user and adjusting ownership of/opt/machine-a-tronand/tmp/machine-a-tron-datawould tighten the container's runtime security posture.🔒 Proposed fix
RUN mkdir -p /opt/machine-a-tron/bin /opt/machine-a-tron/templates /tmp/machine-a-tron-data +RUN useradd -r -u 1000 -g root mat && \ + chown -R mat:root /opt/machine-a-tron /tmp/machine-a-tron-data COPY --from=builder /artifacts/machine-a-tron /opt/machine-a-tron/bin/machine-a-tron COPY crates/machine-a-tron/templates /opt/machine-a-tron/templates COPY crates/bmc-mock/dell_poweredge_r750.tar.gz /opt/machine-a-tron/dell_poweredge_r750.tar.gz COPY crates/bmc-mock/nvidia_dpu.tar.gz /opt/machine-a-tron/nvidia_dpu.tar.gz +USER mat🤖 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 `@crates/machine-a-tron/Dockerfile` around lines 46 - 62, The final runtime image for machine-a-tron is missing a non-root USER, so the container defaults to running as root. Add a dedicated runtime user in the Dockerfile, chown /opt/machine-a-tron and /tmp/machine-a-tron-data to that user, and switch to it before the final entrypoint so the existing machine-a-tron binary runs without root privileges.helm/charts/nico-machine-a-tron/tests/certificate_test.yaml (1)
13-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSecond test case doesn't assert on the overridden values.
The "should use custom certificate settings" case sets
certificate.serviceNameandcertificate.extraDnsNamesbut only assertsisKind: Certificate— it never verifies those overrides actually reach the rendered manifest (e.g.spec.dnsNames). As written, this test would still pass even if the override wiring incertificate.yamlwere broken.✅ Proposed fix
- it: should use custom certificate settings set: certificate: serviceName: custom-service extraDnsNames: - extra.example.com asserts: - isKind: of: Certificate + - contains: + path: spec.dnsNames + content: extra.example.com🤖 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 `@helm/charts/nico-machine-a-tron/tests/certificate_test.yaml` around lines 13 - 21, The “should use custom certificate settings” test only checks the resource kind and never validates the overridden certificate values, so it won’t catch broken wiring. Update the test in certificate_test.yaml to assert the rendered Certificate manifest reflects the set values from certificate.serviceName and certificate.extraDnsNames, using the relevant Certificate spec fields such as spec.dnsNames and any service name–derived field exposed by the template. Keep the existing isKind assertion, but add value assertions tied to the certificate.yaml rendering so the override behavior is actually verified.helm/charts/nico-machine-a-tron/templates/deployment.yaml (1)
23-107: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider adding a baseline
securityContext.No pod- or container-level
securityContextis set (e.g.,runAsNonRoot,allowPrivilegeEscalation: false,readOnlyRootFilesystem, capability drops). Given this workload runs alongside site infrastructure, a minimal hardened default would improve the security posture without much effort.As per path instructions for
helm/**: "Review Helm charts for template correctness, values compatibility, Kubernetes API compatibility, security context, RBAC, probes, and upgrade behavior."🤖 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 `@helm/charts/nico-machine-a-tron/templates/deployment.yaml` around lines 23 - 107, Add a baseline securityContext to the Deployment template so the workload runs with hardened defaults. Update the nico-machine-a-tron container/pod spec in deployment.yaml to include sensible settings like runAsNonRoot, allowPrivilegeEscalation: false, readOnlyRootFilesystem, and dropped capabilities, using existing Helm values patterns if needed. Keep the change localized around the containers section and ensure the new securityContext is compatible with the current volume mounts and startup behavior.Source: Path instructions
helm/charts/nico-machine-a-tron/templates/service.yaml (1)
12-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
service.ssh.targetPortvalue is unused/dead configuration.The Service always sets
targetPort: ssh(the named container port), whilevalues.yamlalso definesservice.ssh.targetPort: 2222. That values field is never referenced by this template, which can mislead operators into thinking they can retarget the SSH port by changing it.🧹 Suggested cleanup
- ssh: - port: 22 - targetPort: 2222 + ssh: + port: 22🤖 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 `@helm/charts/nico-machine-a-tron/templates/service.yaml` around lines 12 - 22, The SSH service is hardcoded to use the named container port via the Service template, so `service.ssh.targetPort` is dead configuration. Update the `service.yaml` template to either reference `.Values.service.ssh.targetPort` in the SSH port entry or remove that unused value from `values.yaml` and any related docs so the `service.ssh` settings are consistent.helm/charts/nico-machine-a-tron/README.md (1)
242-244: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd language hints to fenced code blocks.
Static analysis flags these blocks (error/output examples) as missing a language identifier.
✏️ Proposed fix
-``` +```text SCALE LIMIT EXCEEDED: 16500 BMCs requested, maximum is 16384 (/18 subnet).Also applies to: 345-347, 421-423
🤖 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 `@helm/charts/nico-machine-a-tron/README.md` around lines 242 - 244, The fenced example blocks in the README are missing a language identifier, which triggers static analysis warnings. Update each affected fenced block in the documentation to use the appropriate text language hint by editing the Markdown blocks in the README examples, including the ones around the SCALE LIMIT EXCEEDED output and the other referenced output snippets, so they are clearly marked as plain text.Source: Linters/SAST tools
🤖 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 `@crates/machine-a-tron/Dockerfile`:
- Around line 41-44: The Dockerfile is building and copying the debug binary for
machine-a-tron, which is inappropriate for the scale-test workload. Update the
build step in the machine-a-tron Dockerfile to produce a release artifact (using
the existing cargo build invocation for carbide-machine-a-tron / machine-a-tron)
and change the copy step to take the binary from the release output directory
instead of debug so the shipped image runs the optimized executable.
In `@helm-prereqs/cleanup-machine-a-tron.sh`:
- Around line 160-167: The namespace deletion success message is printed
unconditionally inside the confirm block, so it can report success even when
kubectl delete namespace fails. Update the delete flow in
cleanup-machine-a-tron.sh so the ok message only runs after a successful kubectl
delete namespace call, and otherwise falls through to warn; use the existing
confirm, kubectl delete namespace, ok, and warn flow to make the success path
conditional on the delete command succeeding.
- Around line 244-283: The cleanup logic in the Phase 5 Vault credential removal
block hardcodes the host BMC vendor path to `dell`, which can miss credentials
seeded by `setup-machine-a-tron.sh` when `HOST_BMC_VENDOR` is overridden. Update
the `MAT_VAULT_PATHS` construction in `cleanup-machine-a-tron.sh` to use the
same vendor value that setup uses (the `HOST_BMC_VENDOR` variable or its
resolved default) so the factory-default `bmc-metadata-items` path always
matches the seeded credential path.
In `@helm-prereqs/setup-machine-a-tron.sh`:
- Around line 415-425: The config-pruning logic in setup-machine-a-tron.sh is
too broad because the drop filtering in the TOML rewrite matches substrings
anywhere in each raw line, so unrelated values or comments can be removed from
the nico-api-site-config-files ConfigMap. Update the filtering in the branches
that build drop/knobs to match only the actual TOML key at the start of the line
(for example, by anchoring on the key name before the equals sign) instead of
using a raw substring search, and apply the same fix in the duplicated
override-mode path.
- Around line 326-372: The Vault purge gate currently trusts psql_count()’s
fallback of "0", so a Postgres query failure can look like an empty site and
trigger deletion of live per-MAC BMC creds. Update the empty-graph check in
setup-machine-a-tron.sh to use psql_q() for both count queries, and fail closed
by aborting the purge path if either query returns an error instead of treating
it as zero.
- Around line 159-163: The scale-mode reservation value is too low in the
simulated network setup, so the gateway address is not being reserved along with
the network address. Update the `SCALE_RESERVE` setting in
`setup-machine-a-tron.sh` to at least 2, or align it with the live
`reserve_first` value used by the scale values file, so the allocation logic
matches the intended reserved addresses.
In `@helm-prereqs/values/machine-a-tron-scale.yaml`:
- Around line 1-24: The top-of-file header in machine-a-tron-scale.yaml
conflicts with the active configuration later in the same file: the opening doc
block describes a MetalLB/nginx BMC mode with nginxBmcProxy.enabled true and
site_explorer.bmc_proxy unset, but the actual machineATron/nginxBmcProxy
settings are for proxy-direct with nginxBmcProxy.enabled false. Update the
header comment to match the real default architecture, and move the
MetalLB/nginx wording into a secondary note about the alternate mode so the
narrative aligns with the configuration symbols nginxBmcProxy.enabled and
machineATron.useSingleBmcMock.
In `@helm/charts/nico-machine-a-tron/templates/_helpers.tpl`:
- Around line 47-84: The issue is in nico-machine-a-tron.certificateSpec:
extraDnsNames and extraUris are only added when cert.dnsNames/cert.uris are
unset, so explicit lists drop required SANs like the BMC mock entry. Update
certificateSpec so the base dnsNames/uris come from either the explicit cert
values or the generated defaults, but always append .cert.extraDnsNames and
.cert.extraUris afterward in both branches. Keep the existing
service/namespace/Spiffe fallback logic intact while making the “extra” lists
additive.
In `@helm/charts/nico-machine-a-tron/templates/metallb-pool.yaml`:
- Around line 15-17: The MetallB pool template is dropping an explicitly set
false value because the guard around nginxBmcProxy.ipPool.autoAssign only
renders the field when it is truthy. Update the template condition in
metallb-pool.yaml so autoAssign is emitted whenever the value is defined,
including false, and keep the existing field name/value wiring intact in the
nginxBmcProxy.ipPool.autoAssign block.
In `@helm/charts/nico-machine-a-tron/templates/metallb-services.yaml`:
- Around line 32-75: The IP generation in metallb-services.yaml only uses the
lower bound from nginxBmcProxy.ipRange and can render loadBalancerIPs past the
configured pool when totalBMCs exceeds the range. Update the template logic
around the ipRange parsing and the range loop to also parse the upper bound,
compute each generated IP in relation to that limit, and fail early before
emitting any service entries that would overflow the MetalLB range.
---
Nitpick comments:
In `@crates/machine-a-tron/Dockerfile`:
- Around line 46-62: The final runtime image for machine-a-tron is missing a
non-root USER, so the container defaults to running as root. Add a dedicated
runtime user in the Dockerfile, chown /opt/machine-a-tron and
/tmp/machine-a-tron-data to that user, and switch to it before the final
entrypoint so the existing machine-a-tron binary runs without root privileges.
In `@helm/charts/nico-machine-a-tron/README.md`:
- Around line 242-244: The fenced example blocks in the README are missing a
language identifier, which triggers static analysis warnings. Update each
affected fenced block in the documentation to use the appropriate text language
hint by editing the Markdown blocks in the README examples, including the ones
around the SCALE LIMIT EXCEEDED output and the other referenced output snippets,
so they are clearly marked as plain text.
In `@helm/charts/nico-machine-a-tron/templates/bmc-proxy-deployment.yaml`:
- Line 54: The nginx image reference in the BMC proxy deployment is using a
floating default tag, so update the image value handling in the bmc-proxy
Deployment template to use a fixed, pinned digest or version instead of the
fallback in the image field. Keep the change localized to the nginxBmcProxy
image reference so deployments remain reproducible across runs.
- Around line 52-84: The nginx proxy container in the bmc-proxy deployment is
missing a securityContext, so harden the nginx container by adding pod/container
security constraints on the nginx container definition. Update the nginx
container block in bmc-proxy-deployment.yaml to set runAsNonRoot and
readOnlyRootFilesystem (and any other appropriate non-privileged settings) so
the proxy runs with minimal privileges while still supporting the TLS volume
mounts and probe setup.
In `@helm/charts/nico-machine-a-tron/templates/deployment.yaml`:
- Around line 23-107: Add a baseline securityContext to the Deployment template
so the workload runs with hardened defaults. Update the nico-machine-a-tron
container/pod spec in deployment.yaml to include sensible settings like
runAsNonRoot, allowPrivilegeEscalation: false, readOnlyRootFilesystem, and
dropped capabilities, using existing Helm values patterns if needed. Keep the
change localized around the containers section and ensure the new
securityContext is compatible with the current volume mounts and startup
behavior.
In `@helm/charts/nico-machine-a-tron/templates/service.yaml`:
- Around line 12-22: The SSH service is hardcoded to use the named container
port via the Service template, so `service.ssh.targetPort` is dead
configuration. Update the `service.yaml` template to either reference
`.Values.service.ssh.targetPort` in the SSH port entry or remove that unused
value from `values.yaml` and any related docs so the `service.ssh` settings are
consistent.
In `@helm/charts/nico-machine-a-tron/tests/certificate_test.yaml`:
- Around line 13-21: The “should use custom certificate settings” test only
checks the resource kind and never validates the overridden certificate values,
so it won’t catch broken wiring. Update the test in certificate_test.yaml to
assert the rendered Certificate manifest reflects the set values from
certificate.serviceName and certificate.extraDnsNames, using the relevant
Certificate spec fields such as spec.dnsNames and any service name–derived field
exposed by the template. Keep the existing isKind assertion, but add value
assertions tied to the certificate.yaml rendering so the override behavior is
actually verified.
🪄 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: af8230fb-7576-4649-b3a6-94e250391803
📒 Files selected for processing (33)
crates/api-core/src/auth/internal_rbac_rules.rscrates/machine-a-tron/Dockerfilecrates/machine-a-tron/README.mddev/deployment/devspace/machine-a-tron.yamldocs/development/machine-a-tron-deployment.mddocs/development/machine-a-tron-scale-testing.mdhelm-prereqs/cleanup-machine-a-tron.shhelm-prereqs/setup-machine-a-tron.shhelm-prereqs/values/machine-a-tron-scale.yamlhelm-prereqs/values/machine-a-tron.yamlhelm/Chart.yamlhelm/charts/nico-machine-a-tron/Chart.yamlhelm/charts/nico-machine-a-tron/README.mdhelm/charts/nico-machine-a-tron/templates/_helpers.tplhelm/charts/nico-machine-a-tron/templates/bmc-proxy-configmap.yamlhelm/charts/nico-machine-a-tron/templates/bmc-proxy-deployment.yamlhelm/charts/nico-machine-a-tron/templates/certificate.yamlhelm/charts/nico-machine-a-tron/templates/configmap.yamlhelm/charts/nico-machine-a-tron/templates/deployment.yamlhelm/charts/nico-machine-a-tron/templates/external-service.yamlhelm/charts/nico-machine-a-tron/templates/metallb-pool.yamlhelm/charts/nico-machine-a-tron/templates/metallb-services.yamlhelm/charts/nico-machine-a-tron/templates/metrics-service.yamlhelm/charts/nico-machine-a-tron/templates/pvc.yamlhelm/charts/nico-machine-a-tron/templates/rbac.yamlhelm/charts/nico-machine-a-tron/templates/service-account.yamlhelm/charts/nico-machine-a-tron/templates/service-monitor.yamlhelm/charts/nico-machine-a-tron/templates/service.yamlhelm/charts/nico-machine-a-tron/tests/certificate_test.yamlhelm/charts/nico-machine-a-tron/tests/configmap_test.yamlhelm/charts/nico-machine-a-tron/values.yamlhelm/templates/NOTES.txthelm/values.yaml
💤 Files with no reviewable changes (1)
- dev/deployment/devspace/machine-a-tron.yaml
Comment on lines
+41
to
+44
| cargo build -p carbide-machine-a-tron --bin machine-a-tron --locked \ | ||
| --target x86_64-unknown-linux-gnu && \ | ||
| mkdir -p /artifacts && \ | ||
| cp /cargo-target/x86_64-unknown-linux-gnu/debug/machine-a-tron /artifacts/machine-a-tron |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Debug build shipped for a scale-test workload.
cargo build (no --release) produces an unoptimized binary, and the copy at Line 44 pulls from the debug/ output directory. For a tool explicitly built to drive 4500 hosts / 13,500 BMC endpoints, an unoptimized binary will materially skew CPU usage and throughput, undermining the scale-test results this image is meant to produce.
🚀 Proposed fix
cargo build -p carbide-machine-a-tron --bin machine-a-tron --locked \
- --target x86_64-unknown-linux-gnu && \
+ --target x86_64-unknown-linux-gnu --release && \
mkdir -p /artifacts && \
- cp /cargo-target/x86_64-unknown-linux-gnu/debug/machine-a-tron /artifacts/machine-a-tron
+ cp /cargo-target/x86_64-unknown-linux-gnu/release/machine-a-tron /artifacts/machine-a-tronAlso applies to: 58-58
🤖 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 `@crates/machine-a-tron/Dockerfile` around lines 41 - 44, The Dockerfile is
building and copying the debug binary for machine-a-tron, which is inappropriate
for the scale-test workload. Update the build step in the machine-a-tron
Dockerfile to produce a release artifact (using the existing cargo build
invocation for carbide-machine-a-tron / machine-a-tron) and change the copy step
to take the binary from the release output directory instead of debug so the
shipped image runs the optimized executable.
Comment on lines
+160
to
+167
| if $DELETE_NAMESPACE; then | ||
| if kubectl get ns "$MAT_NAMESPACE" >/dev/null 2>&1; then | ||
| confirm "Delete the entire ${MAT_NAMESPACE} namespace (removes pull secret + copied secrets)?" \ | ||
| && { kubectl delete namespace "$MAT_NAMESPACE" --wait=false >/dev/null; ok "namespace ${MAT_NAMESPACE} deletion requested"; } \ | ||
| || warn "skipped namespace deletion" | ||
| else | ||
| ok "namespace ${MAT_NAMESPACE} already absent" | ||
| fi |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Success message printed even if kubectl delete namespace fails.
confirm ... && { kubectl delete namespace ...; ok "..."; } || warn "skipped namespace deletion" — because the two statements inside the { } group are separated by ; (not &&), ok "namespace ... deletion requested" runs unconditionally once the group is entered, regardless of whether the kubectl delete call actually succeeded. An operator relying on this output for confirmation could be misled if the delete silently failed (e.g. a stuck finalizer, RBAC issue).
🔧 Proposed fix
- confirm "Delete the entire ${MAT_NAMESPACE} namespace (removes pull secret + copied secrets)?" \
- && { kubectl delete namespace "$MAT_NAMESPACE" --wait=false >/dev/null; ok "namespace ${MAT_NAMESPACE} deletion requested"; } \
- || warn "skipped namespace deletion"
+ if confirm "Delete the entire ${MAT_NAMESPACE} namespace (removes pull secret + copied secrets)?"; then
+ kubectl delete namespace "$MAT_NAMESPACE" --wait=false >/dev/null \
+ && ok "namespace ${MAT_NAMESPACE} deletion requested" \
+ || warn "failed to request namespace ${MAT_NAMESPACE} deletion"
+ else
+ warn "skipped namespace deletion"
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if $DELETE_NAMESPACE; then | |
| if kubectl get ns "$MAT_NAMESPACE" >/dev/null 2>&1; then | |
| confirm "Delete the entire ${MAT_NAMESPACE} namespace (removes pull secret + copied secrets)?" \ | |
| && { kubectl delete namespace "$MAT_NAMESPACE" --wait=false >/dev/null; ok "namespace ${MAT_NAMESPACE} deletion requested"; } \ | |
| || warn "skipped namespace deletion" | |
| else | |
| ok "namespace ${MAT_NAMESPACE} already absent" | |
| fi | |
| if $DELETE_NAMESPACE; then | |
| if kubectl get ns "$MAT_NAMESPACE" >/dev/null 2>&1; then | |
| if confirm "Delete the entire ${MAT_NAMESPACE} namespace (removes pull secret + copied secrets)?"; then | |
| kubectl delete namespace "$MAT_NAMESPACE" --wait=false >/dev/null \ | |
| && ok "namespace ${MAT_NAMESPACE} deletion requested" \ | |
| || warn "failed to request namespace ${MAT_NAMESPACE} deletion" | |
| else | |
| warn "skipped namespace deletion" | |
| fi | |
| else | |
| ok "namespace ${MAT_NAMESPACE} already absent" | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 163-163: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
🤖 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 `@helm-prereqs/cleanup-machine-a-tron.sh` around lines 160 - 167, The namespace
deletion success message is printed unconditionally inside the confirm block, so
it can report success even when kubectl delete namespace fails. Update the
delete flow in cleanup-machine-a-tron.sh so the ok message only runs after a
successful kubectl delete namespace call, and otherwise falls through to warn;
use the existing confirm, kubectl delete namespace, ok, and warn flow to make
the success path conditional on the delete command succeeding.
Comment on lines
+244
to
+283
| # ============================================================================= | ||
| # Phase 5 — remove site BMC root Vault credential | ||
| # ============================================================================= | ||
| phase "Phase 5 — remove machine-a-tron Vault credentials" | ||
| # site root + the host factory cred that setup-machine-a-tron.sh seeds. | ||
| # (The DPU factory + UEFI creds belong to nico-prereqs kvSeeds — left alone.) | ||
| # NB: vendor segment is lowercase ("dell") — BMCVendor's Display impl | ||
| # lowercases; must match what setup-machine-a-tron.sh seeds. | ||
| MAT_VAULT_PATHS="machines/bmc/site/root machines/all_hosts/factory_default/bmc-metadata-items/dell" | ||
| if $KEEP_VAULT_CRED; then | ||
| warn "--keep-vault-cred set; leaving credentials in place" | ||
| elif confirm "Delete Vault credentials seeded/rotated for machine-a-tron (site root, host factory, per-MAC)?"; then | ||
| for p in $MAT_VAULT_PATHS; do | ||
| if vault_cmd "vault kv get secrets/$p" >/dev/null 2>&1; then | ||
| vault_cmd "vault kv metadata delete secrets/$p" >/dev/null 2>&1 \ | ||
| || vault_cmd "vault kv delete secrets/$p" >/dev/null 2>&1 || true | ||
| ok "removed $p" | ||
| else | ||
| ok "already absent: $p" | ||
| fi | ||
| done | ||
| # Per-MAC rotated creds (machines/bmc/<mac>/root) — written by site-explorer | ||
| # when it rotates each BMC's password to the site root. MUST be purged: a | ||
| # fresh mock boots at the factory password, but a surviving per-MAC entry | ||
| # tells site-explorer the BMC was already rotated, so it presents the old | ||
| # rotated password, gets 401 Unauthorized, and permanently latches | ||
| # AvoidLockout (NICO-SITEEXPLORER-144) on the endpoint. | ||
| # Batch server-side in ONE kubectl exec: at scale there are thousands of | ||
| # per-MAC entries and one exec per deletion takes HOURS (one round-trip + | ||
| # exec setup each); a shell loop on the vault pod does them all in seconds. | ||
| _n="$(vault_cmd 'count=0 | ||
| for m in $(vault kv list -format=yaml secrets/machines/bmc 2>/dev/null | sed "s/^- //" | grep -v "^site/"); do | ||
| vault kv metadata delete "secrets/machines/bmc/${m%/}/root" >/dev/null 2>&1 && count=$((count+1)) | ||
| done | ||
| echo $count' || echo 0)" | ||
| ok "removed ${_n:-0} per-MAC rotated creds (batched server-side)" | ||
| else | ||
| warn "kept credentials" | ||
| fi | ||
|
|
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Vault vendor path is hardcoded, diverging from setup-machine-a-tron.sh's configurable HOST_BMC_VENDOR.
setup-machine-a-tron.sh allows overriding the host BMC vendor via HOST_BMC_VENDOR (default dell), and seeds machines/all_hosts/factory_default/bmc-metadata-items/${HOST_BMC_VENDOR}. This cleanup script hardcodes .../dell regardless of what vendor was actually used at setup time. If an operator ran setup with a non-default vendor, cleanup silently leaves that factory-default credential behind, and a subsequent redeploy could hit the "poisoned per-MAC credential" failure mode this same pair of scripts otherwise guards against.
🔧 Proposed fix
+HOST_BMC_VENDOR="${HOST_BMC_VENDOR:-dell}"
MAT_VAULT_PATHS="machines/bmc/site/root machines/all_hosts/factory_default/bmc-metadata-items/dell"
+MAT_VAULT_PATHS="machines/bmc/site/root machines/all_hosts/factory_default/bmc-metadata-items/${HOST_BMC_VENDOR}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # ============================================================================= | |
| # Phase 5 — remove site BMC root Vault credential | |
| # ============================================================================= | |
| phase "Phase 5 — remove machine-a-tron Vault credentials" | |
| # site root + the host factory cred that setup-machine-a-tron.sh seeds. | |
| # (The DPU factory + UEFI creds belong to nico-prereqs kvSeeds — left alone.) | |
| # NB: vendor segment is lowercase ("dell") — BMCVendor's Display impl | |
| # lowercases; must match what setup-machine-a-tron.sh seeds. | |
| MAT_VAULT_PATHS="machines/bmc/site/root machines/all_hosts/factory_default/bmc-metadata-items/dell" | |
| if $KEEP_VAULT_CRED; then | |
| warn "--keep-vault-cred set; leaving credentials in place" | |
| elif confirm "Delete Vault credentials seeded/rotated for machine-a-tron (site root, host factory, per-MAC)?"; then | |
| for p in $MAT_VAULT_PATHS; do | |
| if vault_cmd "vault kv get secrets/$p" >/dev/null 2>&1; then | |
| vault_cmd "vault kv metadata delete secrets/$p" >/dev/null 2>&1 \ | |
| || vault_cmd "vault kv delete secrets/$p" >/dev/null 2>&1 || true | |
| ok "removed $p" | |
| else | |
| ok "already absent: $p" | |
| fi | |
| done | |
| # Per-MAC rotated creds (machines/bmc/<mac>/root) — written by site-explorer | |
| # when it rotates each BMC's password to the site root. MUST be purged: a | |
| # fresh mock boots at the factory password, but a surviving per-MAC entry | |
| # tells site-explorer the BMC was already rotated, so it presents the old | |
| # rotated password, gets 401 Unauthorized, and permanently latches | |
| # AvoidLockout (NICO-SITEEXPLORER-144) on the endpoint. | |
| # Batch server-side in ONE kubectl exec: at scale there are thousands of | |
| # per-MAC entries and one exec per deletion takes HOURS (one round-trip + | |
| # exec setup each); a shell loop on the vault pod does them all in seconds. | |
| _n="$(vault_cmd 'count=0 | |
| for m in $(vault kv list -format=yaml secrets/machines/bmc 2>/dev/null | sed "s/^- //" | grep -v "^site/"); do | |
| vault kv metadata delete "secrets/machines/bmc/${m%/}/root" >/dev/null 2>&1 && count=$((count+1)) | |
| done | |
| echo $count' || echo 0)" | |
| ok "removed ${_n:-0} per-MAC rotated creds (batched server-side)" | |
| else | |
| warn "kept credentials" | |
| fi | |
| # ============================================================================= | |
| # Phase 5 — remove site BMC root Vault credential | |
| # ============================================================================= | |
| phase "Phase 5 — remove machine-a-tron Vault credentials" | |
| # site root + the host factory cred that setup-machine-a-tron.sh seeds. | |
| # (The DPU factory + UEFI creds belong to nico-prereqs kvSeeds — left alone.) | |
| # NB: vendor segment is lowercase ("dell") — BMCVendor's Display impl | |
| # lowercases; must match what setup-machine-a-tron.sh seeds. | |
| HOST_BMC_VENDOR="${HOST_BMC_VENDOR:-dell}" | |
| MAT_VAULT_PATHS="machines/bmc/site/root machines/all_hosts/factory_default/bmc-metadata-items/dell" | |
| MAT_VAULT_PATHS="machines/bmc/site/root machines/all_hosts/factory_default/bmc-metadata-items/${HOST_BMC_VENDOR}" | |
| if $KEEP_VAULT_CRED; then | |
| warn "--keep-vault-cred set; leaving credentials in place" | |
| elif confirm "Delete Vault credentials seeded/rotated for machine-a-tron (site root, host factory, per-MAC)?"; then | |
| for p in $MAT_VAULT_PATHS; do | |
| if vault_cmd "vault kv get secrets/$p" >/dev/null 2>&1; then | |
| vault_cmd "vault kv metadata delete secrets/$p" >/dev/null 2>&1 \ | |
| || vault_cmd "vault kv delete secrets/$p" >/dev/null 2>&1 || true | |
| ok "removed $p" | |
| else | |
| ok "already absent: $p" | |
| fi | |
| done | |
| # Per-MAC rotated creds (machines/bmc/<mac>/root) — written by site-explorer | |
| # when it rotates each BMC's password to the site root. MUST be purged: a | |
| # fresh mock boots at the factory password, but a surviving per-MAC entry | |
| # tells site-explorer the BMC was already rotated, so it presents the old | |
| # rotated password, gets 401 Unauthorized, and permanently latches | |
| # AvoidLockout (NICO-SITEEXPLORER-144) on the endpoint. | |
| # Batch server-side in ONE kubectl exec: at scale there are thousands of | |
| # per-MAC entries and one exec per deletion takes HOURS (one round-trip + | |
| # exec setup each); a shell loop on the vault pod does them all in seconds. | |
| _n="$(vault_cmd 'count=0 | |
| for m in $(vault kv list -format=yaml secrets/machines/bmc 2>/dev/null | sed "s/^- //" | grep -v "^site/"); do | |
| vault kv metadata delete "secrets/machines/bmc/${m%/}/root" >/dev/null 2>&1 && count=$((count+1)) | |
| done | |
| echo $count' || echo 0)" | |
| ok "removed ${_n:-0} per-MAC rotated creds (batched server-side)" | |
| else | |
| warn "kept credentials" | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 274-278: Expressions don't expand in single quotes, use double quotes for that.
(SC2016)
🤖 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 `@helm-prereqs/cleanup-machine-a-tron.sh` around lines 244 - 283, The cleanup
logic in the Phase 5 Vault credential removal block hardcodes the host BMC
vendor path to `dell`, which can miss credentials seeded by
`setup-machine-a-tron.sh` when `HOST_BMC_VENDOR` is overridden. Update the
`MAT_VAULT_PATHS` construction in `cleanup-machine-a-tron.sh` to use the same
vendor value that setup uses (the `HOST_BMC_VENDOR` variable or its resolved
default) so the factory-default `bmc-metadata-items` path always matches the
seeded credential path.
Comment on lines
+159
to
+163
| # Simulated networks for scale mode — must match the scale values file | ||
| # (relay addresses) and the MetalLB ipRange (inside the OOB prefix). | ||
| SCALE_OOB_PREFIX="10.100.0.0/17"; SCALE_OOB_GW="10.100.0.1" | ||
| SCALE_ADMIN_PREFIX="10.102.0.0/18"; SCALE_ADMIN_GW="10.102.0.1" | ||
| SCALE_RESERVE=1 |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect how reserve_first / SCALE_RESERVE is consumed for IP allocation.
rg -n -C5 'reserve_first' --type=rust crates/Repository: NVIDIA/infra-controller
Length of output: 42515
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the referenced script sections.
sed -n '150,170p' helm-prereqs/setup-machine-a-tron.sh
printf '\n---\n'
sed -n '615,635p' helm-prereqs/setup-machine-a-tron.sh
printf '\n=== reserve_first usage / allocator semantics ===\n'
rg -n -C4 'num_reserved|reserve_first|gateway.*reserve|skip before allocating|allocat' crates/api-model crates/api-core crates/rpc crates/api-db helm-prereqs -g '!**/target/**'
printf '\n=== relevant prefix helpers ===\n'
sed -n '380,430p' crates/api-model/src/network_segment/mod.rs
printf '\n---\n'
sed -n '1,80p' crates/rpc/src/model/network_prefix.rsRepository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read only the exact referenced areas from the script.
sed -n '150,170p' helm-prereqs/setup-machine-a-tron.sh
printf '\n---\n'
sed -n '615,635p' helm-prereqs/setup-machine-a-tron.sh
printf '\n=== nearby allocator logic ===\n'
rg -n -C6 'reserve_first|gateway|network address|operator-reserved|free_ip_count' helm-prereqs/setup-machine-a-tron.sh crates/api-core crates/api-model crates/rpc crates/api-db -g '!**/target/**' | head -n 200Repository: NVIDIA/infra-controller
Length of output: 20398
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the allocation logic that consumes num_reserved / reserve_first.
rg -n -C8 'num_reserved|reserve_first|gateway' crates/api-core crates/api-db crates/api-model crates/rpc crates/site-explorer crates/machine-a-tron helm-prereqs -g '!**/target/**' | head -n 250
printf '\n=== likely allocator implementations ===\n'
rg -n -C10 'next.*ip|allocate.*ip|candidate.*ip|skip.*gateway|broadcast|cidr.*iterate|hosts\(' crates/api-core crates/api-db -g '!**/target/**' | head -n 250Repository: NVIDIA/infra-controller
Length of output: 47314
Bump SCALE_RESERVE to reserve the gateway too. setup-machine-a-tron.sh treats reserve_first as covering the network address, gateway, and other leading reservations; with gateway = .1, SCALE_RESERVE=1 only skips the network address and leaves the gateway in the allocatable pool. Use at least 2 here, or mirror the live reserve_first value instead.
🤖 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 `@helm-prereqs/setup-machine-a-tron.sh` around lines 159 - 163, The scale-mode
reservation value is too low in the simulated network setup, so the gateway
address is not being reserved along with the network address. Update the
`SCALE_RESERVE` setting in `setup-machine-a-tron.sh` to at least 2, or align it
with the live `reserve_first` value used by the scale values file, so the
allocation logic matches the intended reserved addresses.
Comment on lines
+326
to
+372
| _kv_put() { # $1=path $2=username $3=password | ||
| vault_cmd "echo '{\"UsernamePassword\":{\"username\":\"$2\",\"password\":\"$3\"}}' | vault kv put secrets/$1 -" >/dev/null \ | ||
| || die "failed to write $1 to Vault" | ||
| } | ||
| _kv_password() { # $1=path → prints current password (empty if absent) | ||
| # `|| true` is load-bearing: when the path is absent, vault kv get fails and | ||
| # under `set -e` a failing $(...) assignment would kill the whole script. | ||
| vault_cmd "vault kv get -format=json secrets/$1" 2>/dev/null | jq -r '.data.data.UsernamePassword.password // empty' 2>/dev/null || true | ||
| } | ||
| # Factory-default creds — the INITIAL login site-explorer uses before rotating. | ||
| # Host and DPU factories DIFFER (see constants above); the wrong one yields 401 | ||
| # Unauthorized on every host BMC and a permanent AvoidLockout latch. | ||
| _kv_put "machines/all_hosts/factory_default/bmc-metadata-items/${HOST_BMC_VENDOR}" root "$FACTORY_HOST_BMC_PASSWORD" | ||
| ok "factory host cred: .../${HOST_BMC_VENDOR} = root/${FACTORY_HOST_BMC_PASSWORD}" | ||
| _kv_put "machines/all_dpus/factory_default/bmc-metadata-items/root" root "$FACTORY_DPU_BMC_PASSWORD" | ||
| ok "factory DPU cred: .../root = root/${FACTORY_DPU_BMC_PASSWORD}" | ||
| # Site-wide root — the rotation target; must differ from both factory passwords. | ||
| _cur="$(_kv_password machines/bmc/site/root)" | ||
| if [[ -z "$_cur" || "$_cur" == "$FACTORY_HOST_BMC_PASSWORD" || "$_cur" == "$FACTORY_DPU_BMC_PASSWORD" ]]; then | ||
| _kv_put "machines/bmc/site/root" "$BMC_USERNAME" "$BMC_PASSWORD" | ||
| ok "machines/bmc/site/root seeded (${BMC_USERNAME}/**** — distinct from factory)" | ||
| else | ||
| ok "machines/bmc/site/root already present with a non-factory password" | ||
| fi | ||
| # Self-heal stale per-MAC rotated creds (machines/bmc/<mac>/root). site-explorer | ||
| # writes one per BMC after rotating its password to the site root; entries | ||
| # surviving from a previous deployment poison a fresh one — the fresh mock is at | ||
| # the factory password, but the per-MAC entry makes site-explorer present the | ||
| # old rotated password: 401 → permanent AvoidLockout. Only safe to purge when | ||
| # the machine graph is empty (live machines' per-MAC creds are real). | ||
| # "Fresh deployment" = no machines AND no interfaces. machines==0 alone is | ||
| # NOT enough: mid-ingestion (interfaces DHCP'd, endpoints explored, machines | ||
| # not yet created) the per-MAC creds are LIVE — purging them forces every | ||
| # endpoint back through the credential fallback. | ||
| if [[ "$(psql_count "SELECT count(*) FROM machines;")" == "0" \ | ||
| && "$(psql_count "SELECT count(*) FROM machine_interfaces;")" == "0" ]]; then | ||
| # Batched server-side in ONE kubectl exec — thousands of entries at scale; | ||
| # one exec per deletion takes hours. | ||
| _n="$(vault_cmd 'count=0 | ||
| for m in $(vault kv list -format=yaml secrets/machines/bmc 2>/dev/null | sed "s/^- //" | grep -v "^site/"); do | ||
| vault kv metadata delete "secrets/machines/bmc/${m%/}/root" >/dev/null 2>&1 && count=$((count+1)) | ||
| done | ||
| echo $count' || echo 0)" | ||
| if [[ "${_n:-0}" != "0" ]]; then | ||
| warn "purged ${_n} stale per-MAC BMC creds from a previous deployment (machine graph was empty)" | ||
| fi | ||
| fi |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant helpers and the purge block.
file="helm-prereqs/setup-machine-a-tron.sh"
# Show the surrounding lines with numbers.
sed -n '210,390p' "$file" | cat -n
# Also inspect the later restore logic referenced by the comment.
sed -n '640,680p' "$file" | cat -nRepository: NVIDIA/infra-controller
Length of output: 15232
Fail closed before the Vault purge gate helm-prereqs/setup-machine-a-tron.sh:151-160 uses psql_count(), whose documented fallback is "0" on query failure. That makes a transient Postgres outage indistinguishable from an empty site and can delete live per-MAC BMC creds. Use psql_q() here and abort if either count query fails.
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 364-368: Expressions don't expand in single quotes, use double quotes for that.
(SC2016)
🤖 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 `@helm-prereqs/setup-machine-a-tron.sh` around lines 326 - 372, The Vault purge
gate currently trusts psql_count()’s fallback of "0", so a Postgres query
failure can look like an empty site and trigger deletion of live per-MAC BMC
creds. Update the empty-graph check in setup-machine-a-tron.sh to use psql_q()
for both count queries, and fail closed by aborting the purge path if either
query returns an error instead of treating it as zero.
Comment on lines
+415
to
+425
| drop = ("bmc_proxy", "override_target_host", "override_target_ip", "override_target_port", | ||
| "concurrent_explorations", "explorations_per_run", "machines_created_per_run") | ||
| knobs = [ | ||
| # PROXY-DIRECT: the Redfish client injects "Forwarded: host=<BMC IP>" | ||
| # whenever bmc_proxy is set; the mock's registry routes on it. One | ||
| # ClusterIP service serves the whole simulated fleet. | ||
| f' bmc_proxy = "{env["BMC_PROXY"]}"', | ||
| f' concurrent_explorations = {env["KNOB_CONC"]}', | ||
| f' explorations_per_run = {env["KNOB_EPR"]}', | ||
| f' machines_created_per_run = {env["KNOB_MCPR"]}', | ||
| ] |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Overly broad substring matching risks stripping unrelated config lines.
The drop tuple is matched with any(t in ln for t in drop) against the entire raw TOML line, not just the key position. Any line whose value or trailing comment happens to contain one of these substrings (e.g. a site_description = "... bmc_proxy test ..." or a commented-out example) would be silently deleted from the live nico-api-site-config-files ConfigMap the next time this script runs — a subtle, hard-to-diagnose config corruption on a ConfigMap that then triggers a nico-api restart. The same pattern is duplicated in the override-mode branch (Line 559-567).
♻️ Proposed fix — anchor the match to the key position
+import re
...
- if any(t in ln for t in drop):
+ if any(re.match(rf'^\s*{re.escape(t)}\s*=', ln) for t in drop):
continueAlso applies to: 449-465, 553-568
🤖 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 `@helm-prereqs/setup-machine-a-tron.sh` around lines 415 - 425, The
config-pruning logic in setup-machine-a-tron.sh is too broad because the drop
filtering in the TOML rewrite matches substrings anywhere in each raw line, so
unrelated values or comments can be removed from the nico-api-site-config-files
ConfigMap. Update the filtering in the branches that build drop/knobs to match
only the actual TOML key at the start of the line (for example, by anchoring on
the key name before the equals sign) instead of using a raw substring search,
and apply the same fix in the duplicated override-mode path.
Comment on lines
+1
to
+24
| # ============================================================================= | ||
| # machine-a-tron-scale.yaml — LARGE-SCALE site values (MetalLB/nginx BMC mode) | ||
| # | ||
| # For scale testing NICo ingestion with hundreds to thousands of simulated | ||
| # hosts. Used by: MAT_MODE=scale setup-machine-a-tron.sh | ||
| # See docs/development/machine-a-tron-deployment.md and the chart README's | ||
| # "METALLB MODE" section. | ||
| # | ||
| # How this differs from Override Mode (values/machine-a-tron.yaml): | ||
| # - nginxBmcProxy.enabled: true — every simulated BMC gets its own | ||
| # LoadBalancer IP (one Service per BMC, chart cap 16384). nginx routes to | ||
| # the right mock via "Forwarded: host=<service IP>". | ||
| # - machineATron.useSingleBmcMock: false — the mock registers one router per | ||
| # BMC keyed by its DHCP'd IP instead of one combined server. | ||
| # - site_explorer.bmc_proxy MUST BE UNSET in nico-core — site-explorer dials | ||
| # each BMC's dedicated IP directly (setup script removes it in scale mode). | ||
| # - Requires the simulated-oob / simulated-admin networks in the nico-core | ||
| # site config (added by the setup script's scale mode): | ||
| # [networks.simulated-oob] prefix 10.100.0.0/17 gateway 10.100.0.1 | ||
| # [networks.simulated-admin] prefix 10.102.0.0/18 gateway 10.102.0.1 | ||
| # The MetalLB ipRange below MUST sit inside the simulated-oob prefix so | ||
| # NICo-allocated BMC IPs match the MetalLB Service IPs (allocation is | ||
| # sequential on both sides). | ||
| # ============================================================================= |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Header doc block contradicts the actual configuration further down.
Lines 1-24 describe this file as the "MetalLB/nginx BMC mode" profile, stating nginxBmcProxy.enabled: true is what differentiates this from Override Mode, and that site_explorer.bmc_proxy "MUST BE UNSET." However, the actual settings at Lines 91-102 set nginxBmcProxy.enabled: false and document a "PROXY-DIRECT" architecture where nginx/MetalLB is explicitly called out as inferior for simulation-only use. This mismatch between the header narrative and the effective configuration will mislead anyone reading the top of the file to configure/troubleshoot the wrong mode.
📝 Proposed fix
Update the header block (Lines 1-24) to describe the proxy-direct architecture as the default, and demote the MetalLB/nginx description to a note about the (currently unused) alternate mode, so both sections agree on which mode is actually configured.
Also applies to: 91-102
🤖 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 `@helm-prereqs/values/machine-a-tron-scale.yaml` around lines 1 - 24, The
top-of-file header in machine-a-tron-scale.yaml conflicts with the active
configuration later in the same file: the opening doc block describes a
MetalLB/nginx BMC mode with nginxBmcProxy.enabled true and
site_explorer.bmc_proxy unset, but the actual machineATron/nginxBmcProxy
settings are for proxy-direct with nginxBmcProxy.enabled false. Update the
header comment to match the real default architecture, and move the
MetalLB/nginx wording into a secondary note about the alternate mode so the
narrative aligns with the configuration symbols nginxBmcProxy.enabled and
machineATron.useSingleBmcMock.
Comment on lines
+47
to
+84
| {{- define "nico-machine-a-tron.certificateSpec" -}} | ||
| duration: {{ .global.certificate.duration }} | ||
| renewBefore: {{ .global.certificate.renewBefore }} | ||
| commonName: {{ printf "%s.%s.svc.cluster.local" (.cert.serviceName | default .svcName) (.cert.identityNamespace | default .namespace) }} | ||
| dnsNames: | ||
| {{- if .cert.dnsNames }} | ||
| {{- range .cert.dnsNames }} | ||
| - {{ . }} | ||
| {{- end }} | ||
| {{- else }} | ||
| - {{ printf "%s.%s.svc.cluster.local" (.cert.serviceName | default .svcName) (.cert.identityNamespace | default .namespace) }} | ||
| {{- if ne (toString .cert.includeShortDnsName) "false" }} | ||
| - {{ printf "%s.%s" (.cert.serviceName | default .svcName) (.cert.identityNamespace | default .namespace) }} | ||
| {{- end }} | ||
| {{- range .cert.extraDnsNames | default list }} | ||
| - {{ . }} | ||
| {{- end }} | ||
| {{- end }} | ||
| uris: | ||
| {{- if .cert.uris }} | ||
| {{- range .cert.uris }} | ||
| - {{ . }} | ||
| {{- end }} | ||
| {{- else }} | ||
| - {{ printf "spiffe://%s/%s/sa/%s" .global.spiffe.trustDomain (.cert.identityNamespace | default .namespace) (.cert.spiffeServiceName | default .cert.serviceName | default .svcName) }} | ||
| {{- range .cert.extraUris | default list }} | ||
| - {{ . }} | ||
| {{- end }} | ||
| {{- end }} | ||
| privateKey: | ||
| algorithm: {{ .global.certificate.privateKey.algorithm }} | ||
| size: {{ .global.certificate.privateKey.size }} | ||
| issuerRef: | ||
| kind: {{ .global.certificate.issuerRef.kind }} | ||
| name: {{ .global.certificate.issuerRef.name }} | ||
| group: {{ .global.certificate.issuerRef.group }} | ||
| secretName: {{ .name }} | ||
| {{- end }} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
extraDnsNames/extraUris are silently dropped when dnsNames/uris is explicitly set.
certificateSpec only appends .cert.extraDnsNames (and .cert.extraUris) inside the else branch — i.e., only when .cert.dnsNames (.cert.uris) is unset. certificate.yaml relies on extraDnsNames to always inject the BMC mock DNS SAN via merge, but if a caller sets certificate.dnsNames explicitly, that essential SAN (and any other extraDnsNames/extraUris) is silently omitted, breaking TLS validation for the BMC mock endpoint.
Make the "extra" lists additive regardless of the explicit list branch:
🔧 Proposed fix
dnsNames:
{{- if .cert.dnsNames }}
{{- range .cert.dnsNames }}
- {{ . }}
{{- end }}
{{- else }}
- {{ printf "%s.%s.svc.cluster.local" (.cert.serviceName | default .svcName) (.cert.identityNamespace | default .namespace) }}
{{- if ne (toString .cert.includeShortDnsName) "false" }}
- {{ printf "%s.%s" (.cert.serviceName | default .svcName) (.cert.identityNamespace | default .namespace) }}
{{- end }}
-{{- range .cert.extraDnsNames | default list }}
- - {{ . }}
-{{- end }}
{{- end }}
+{{- range .cert.extraDnsNames | default list }}
+ - {{ . }}
+{{- end }}
uris:
{{- if .cert.uris }}
{{- range .cert.uris }}
- {{ . }}
{{- end }}
{{- else }}
- {{ printf "spiffe://%s/%s/sa/%s" .global.spiffe.trustDomain (.cert.identityNamespace | default .namespace) (.cert.spiffeServiceName | default .cert.serviceName | default .svcName) }}
-{{- range .cert.extraUris | default list }}
- - {{ . }}
-{{- end }}
{{- end }}
+{{- range .cert.extraUris | default list }}
+ - {{ . }}
+{{- end }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {{- define "nico-machine-a-tron.certificateSpec" -}} | |
| duration: {{ .global.certificate.duration }} | |
| renewBefore: {{ .global.certificate.renewBefore }} | |
| commonName: {{ printf "%s.%s.svc.cluster.local" (.cert.serviceName | default .svcName) (.cert.identityNamespace | default .namespace) }} | |
| dnsNames: | |
| {{- if .cert.dnsNames }} | |
| {{- range .cert.dnsNames }} | |
| - {{ . }} | |
| {{- end }} | |
| {{- else }} | |
| - {{ printf "%s.%s.svc.cluster.local" (.cert.serviceName | default .svcName) (.cert.identityNamespace | default .namespace) }} | |
| {{- if ne (toString .cert.includeShortDnsName) "false" }} | |
| - {{ printf "%s.%s" (.cert.serviceName | default .svcName) (.cert.identityNamespace | default .namespace) }} | |
| {{- end }} | |
| {{- range .cert.extraDnsNames | default list }} | |
| - {{ . }} | |
| {{- end }} | |
| {{- end }} | |
| uris: | |
| {{- if .cert.uris }} | |
| {{- range .cert.uris }} | |
| - {{ . }} | |
| {{- end }} | |
| {{- else }} | |
| - {{ printf "spiffe://%s/%s/sa/%s" .global.spiffe.trustDomain (.cert.identityNamespace | default .namespace) (.cert.spiffeServiceName | default .cert.serviceName | default .svcName) }} | |
| {{- range .cert.extraUris | default list }} | |
| - {{ . }} | |
| {{- end }} | |
| {{- end }} | |
| privateKey: | |
| algorithm: {{ .global.certificate.privateKey.algorithm }} | |
| size: {{ .global.certificate.privateKey.size }} | |
| issuerRef: | |
| kind: {{ .global.certificate.issuerRef.kind }} | |
| name: {{ .global.certificate.issuerRef.name }} | |
| group: {{ .global.certificate.issuerRef.group }} | |
| secretName: {{ .name }} | |
| {{- end }} | |
| {{- define "nico-machine-a-tron.certificateSpec" -}} | |
| duration: {{ .global.certificate.duration }} | |
| renewBefore: {{ .global.certificate.renewBefore }} | |
| commonName: {{ printf "%s.%s.svc.cluster.local" (.cert.serviceName | default .svcName) (.cert.identityNamespace | default .namespace) }} | |
| dnsNames: | |
| {{- if .cert.dnsNames }} | |
| {{- range .cert.dnsNames }} | |
| - {{ . }} | |
| {{- end }} | |
| {{- else }} | |
| - {{ printf "%s.%s.svc.cluster.local" (.cert.serviceName | default .svcName) (.cert.identityNamespace | default .namespace) }} | |
| {{- if ne (toString .cert.includeShortDnsName) "false" }} | |
| - {{ printf "%s.%s" (.cert.serviceName | default .svcName) (.cert.identityNamespace | default .namespace) }} | |
| {{- end }} | |
| {{- end }} | |
| {{- range .cert.extraDnsNames | default list }} | |
| - {{ . }} | |
| {{- end }} | |
| uris: | |
| {{- if .cert.uris }} | |
| {{- range .cert.uris }} | |
| - {{ . }} | |
| {{- end }} | |
| {{- else }} | |
| - {{ printf "spiffe://%s/%s/sa/%s" .global.spiffe.trustDomain (.cert.identityNamespace | default .namespace) (.cert.spiffeServiceName | default .cert.serviceName | default .svcName) }} | |
| {{- end }} | |
| {{- range .cert.extraUris | default list }} | |
| - {{ . }} | |
| {{- end }} | |
| privateKey: | |
| algorithm: {{ .global.certificate.privateKey.algorithm }} | |
| size: {{ .global.certificate.privateKey.size }} | |
| issuerRef: | |
| kind: {{ .global.certificate.issuerRef.kind }} | |
| name: {{ .global.certificate.issuerRef.name }} | |
| group: {{ .global.certificate.issuerRef.group }} | |
| secretName: {{ .name }} | |
| {{- end }} |
🤖 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 `@helm/charts/nico-machine-a-tron/templates/_helpers.tpl` around lines 47 - 84,
The issue is in nico-machine-a-tron.certificateSpec: extraDnsNames and extraUris
are only added when cert.dnsNames/cert.uris are unset, so explicit lists drop
required SANs like the BMC mock entry. Update certificateSpec so the base
dnsNames/uris come from either the explicit cert values or the generated
defaults, but always append .cert.extraDnsNames and .cert.extraUris afterward in
both branches. Keep the existing service/namespace/Spiffe fallback logic intact
while making the “extra” lists additive.
Comment on lines
+15
to
+17
| {{- if .Values.nginxBmcProxy.ipPool.autoAssign }} | ||
| autoAssign: {{ .Values.nginxBmcProxy.ipPool.autoAssign }} | ||
| {{- end }} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
autoAssign: false is silently ignored.
The truthy guard {{- if .Values.nginxBmcProxy.ipPool.autoAssign }} omits the field whenever the value is false, causing MetalLB to fall back to its own default (true). A user who explicitly disables auto-assignment gets the opposite behavior.
🐛 Proposed fix
- {{- if .Values.nginxBmcProxy.ipPool.autoAssign }}
- autoAssign: {{ .Values.nginxBmcProxy.ipPool.autoAssign }}
- {{- end }}
+ {{- if hasKey .Values.nginxBmcProxy.ipPool "autoAssign" }}
+ autoAssign: {{ .Values.nginxBmcProxy.ipPool.autoAssign }}
+ {{- end }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {{- if .Values.nginxBmcProxy.ipPool.autoAssign }} | |
| autoAssign: {{ .Values.nginxBmcProxy.ipPool.autoAssign }} | |
| {{- end }} | |
| {{- if hasKey .Values.nginxBmcProxy.ipPool "autoAssign" }} | |
| autoAssign: {{ .Values.nginxBmcProxy.ipPool.autoAssign }} | |
| {{- end }} |
🤖 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 `@helm/charts/nico-machine-a-tron/templates/metallb-pool.yaml` around lines 15
- 17, The MetallB pool template is dropping an explicitly set false value
because the guard around nginxBmcProxy.ipPool.autoAssign only renders the field
when it is truthy. Update the template condition in metallb-pool.yaml so
autoAssign is emitted whenever the value is defined, including false, and keep
the existing field name/value wiring intact in the
nginxBmcProxy.ipPool.autoAssign block.
Comment on lines
+32
to
+75
| {{/* | ||
| Parse IP range: "10.100.0.2-10.100.7.254" -> start IP parts | ||
| We generate services from startIP to startIP + totalBMCs - 1 | ||
| TODO: Need a more elegant solution | ||
| */}} | ||
| {{- $ipRange := .Values.nginxBmcProxy.ipRange }} | ||
| {{- $startIP := (split "-" $ipRange)._0 }} | ||
| {{- $ipParts := split "." $startIP }} | ||
| {{- $octet1 := $ipParts._0 | int }} | ||
| {{- $octet2 := $ipParts._1 | int }} | ||
| {{- $octet3 := $ipParts._2 | int }} | ||
| {{- $octet4 := $ipParts._3 | int }} | ||
|
|
||
| {{/* Generate services for each BMC */}} | ||
| {{- range $i := until (int $totalBMCs) }} | ||
| {{/* | ||
| Calculate IP: baseIP + offset | ||
| Handle octet overflow: .4 → .3 → .2 → .1 | ||
| */}} | ||
| {{- $offset := $i }} | ||
| {{- $o4 := add $octet4 $offset }} | ||
| {{- $o3 := $octet3 }} | ||
| {{- $o2 := $octet2 }} | ||
| {{- $o1 := $octet1 }} | ||
|
|
||
| {{/* Handle octet 4 overflow */}} | ||
| {{- if gt $o4 255 }} | ||
| {{- $o3 = add $o3 (div $o4 256) }} | ||
| {{- $o4 = mod $o4 256 }} | ||
| {{- end }} | ||
|
|
||
| {{/* Handle octet 3 overflow */}} | ||
| {{- if gt $o3 255 }} | ||
| {{- $o2 = add $o2 (div $o3 256) }} | ||
| {{- $o3 = mod $o3 256 }} | ||
| {{- end }} | ||
|
|
||
| {{/* Handle octet 2 overflow */}} | ||
| {{- if gt $o2 255 }} | ||
| {{- $o1 = add $o1 (div $o2 256) }} | ||
| {{- $o2 = mod $o2 256 }} | ||
| {{- end }} | ||
|
|
||
| {{- $currentIP := printf "%d.%d.%d.%d" (int $o1) (int $o2) (int $o3) (int $o4) }} |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target template and nearby chart values/documentation.
git ls-files | rg 'helm/charts/nico-machine-a-tron/(templates/metallb-services\.yaml|values(\..*)?\.ya?ml|README\.md|Chart\.ya?ml|templates/.*)'
echo '--- FILE: helm/charts/nico-machine-a-tron/templates/metallb-services.yaml ---'
cat -n helm/charts/nico-machine-a-tron/templates/metallb-services.yaml | sed -n '1,220p'
echo '--- SEARCH: nginxBmcProxy.ipRange / hostCount / dpuPerHostCount ---'
rg -n 'nginxBmcProxy\.ipRange|hostCount|dpuPerHostCount|ipRange|totalBMCs|metallb-services' helm/charts/nico-machine-a-tron -g '!**/target/**'Repository: NVIDIA/infra-controller
Length of output: 9674
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- FILE: helm/charts/nico-machine-a-tron/templates/metallb-pool.yaml ---'
cat -n helm/charts/nico-machine-a-tron/templates/metallb-pool.yaml | sed -n '1,220p'
echo '--- README: ipRange / scale-limit / examples ---'
sed -n '170,210p' helm/charts/nico-machine-a-tron/README.md
echo '---'
sed -n '240,330p' helm/charts/nico-machine-a-tron/README.md
echo '---'
sed -n '340,440p' helm/charts/nico-machine-a-tron/README.md
echo '--- values.yaml: nginxBmcProxy block ---'
sed -n '286,316p' helm/charts/nico-machine-a-tron/values.yaml
echo '--- programmatic probe: carry behavior ---'
python3 - <<'PY'
def next_ip(start, offset):
o1, o2, o3, o4 = map(int, start.split('.'))
o4 += offset
if o4 > 255:
o3 += o4 // 256
o4 %= 256
if o3 > 255:
o2 += o3 // 256
o3 %= 256
if o2 > 255:
o1 += o2 // 256
o2 %= 256
return o1, o2, o3, o4
cases = [
("10.100.0.2", 16383),
("10.255.255.250", 16383),
("255.255.255.250", 1),
("254.255.255.250", 1024),
]
for start, offset in cases:
ip = next_ip(start, offset)
print(start, offset, "=>", ".".join(map(str, ip)), "valid=", all(0 <= x <= 255 for x in ip))
PYRepository: NVIDIA/infra-controller
Length of output: 10248
Validate the generated IPs against the configured range. The template only reads the lower bound of nginxBmcProxy.ipRange; if hostCount/dpuPerHostCount produce more BMCs than the pool can hold, it will render loadBalancerIPs beyond the MetalLB range and leave services unassigned. Parse the upper bound and fail before emitting the overflow entries.
🤖 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 `@helm/charts/nico-machine-a-tron/templates/metallb-services.yaml` around lines
32 - 75, The IP generation in metallb-services.yaml only uses the lower bound
from nginxBmcProxy.ipRange and can render loadBalancerIPs past the configured
pool when totalBMCs exceeds the range. Update the template logic around the
ipRange parsing and the range loop to also parse the upper bound, compute each
generated IP in relation to that limit, and fail early before emitting any
service entries that would overflow the MetalLB range.
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
Contributor
|
One of the docs indicates this is still draft work, though the PR itself isn't marked draft... If you'd like a docs review, ping me on Slack! |
Contributor
Author
|
I've merged these changes into @akorobkov-nvda branch instead. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Deploy and scale-test machine-a-tron end to end on a NICo site. Adds a single idempotent setup script (and its cleanup inverse) that takes a running site from nothing to a simulated fleet with machines created, plus a
MAT_MODE=scaleproxy-direct profile validated live at 1 → 100 → 1000 → 4500 hosts (×2 DPUs = 13,500 BMC endpoints) on dev6. Includes chart fixes for the nginx/MetalLB mode, a one-line RBAC fix (Machineatron→AddExpectedMachine), and a scale-testing writeup with the complete 19-issue log for review.Builds on Alexander's machine-a-tron helm chart work (picked up from his draft).
Related issues
N/A — supports the NICo 4500-node scale initiative. See
docs/development/machine-a-tron-scale-testing.mdfor open questions raised by this work.Type of Change
Breaking Changes
No breaking changes: the scripts and values are new opt-in tooling; the chart fixes correct broken behavior in the nginx/MetalLB mode (nonexistent upstream Service, Forwarded-header routing, proxy placement); the RBAC change only adds a grant.
Testing
Validated live on dev6 (3-node cluster, dev-sized postgres):
Additional Notes
docs/development/machine-a-tron-scale-testing.md— it has the architecture decision (proxy-direct vs nginx/MetalLB), the full issue log with root causes, measured ingestion rates, and six open questions where early feedback is wanted (RBAC grant, seed-once reconcile semantics for networks/pools, AvoidLockout ergonomics for simulated fleets, mock credential fidelity across resets, lo-ip pool sizing, exploration-cycle economics).