feat(helm): machine-a-tron mac address allocation#3700
Open
akorobkov-nvda wants to merge 1 commit into
Open
Conversation
akorobkov-nvda
requested review from
a team,
polarweasel and
shayan1995
as code owners
July 17, 2026 22:23
Contributor
Summary by CodeRabbit
WalkthroughThe Helm chart adds deterministic, configurable per-pod MAC pools and hardware MAC ranges. Values, templates, multi-pod examples, and deployment documentation now describe automatic allocation and explicit override paths. ChangesPer-pod MAC pool allocation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant HelmValues
participant ConfigMapTemplate
participant MatToml
HelmValues->>ConfigMapTemplate: Supply MAC defaults and pod overrides
ConfigMapTemplate->>ConfigMapTemplate: Sort active pods and calculate podIndex
ConfigMapTemplate->>MatToml: Render per-pod MAC pool sections
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 5
🤖 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 `@helm/charts/nico-machine-a-tron/README.md`:
- Around line 47-51: Restore the TOML code-fence boundaries in
helm/charts/nico-machine-a-tron/README.md: at lines 47-51, open a toml fence
before [site_explorer] and close it after override_target_port; at lines
235-236, open a toml fence before [networks.pod-0-oob] and close it after the
final gateway. Ensure subsequent Markdown sections render normally.
- Around line 367-375: Update the troubleshooting section in the README by
changing the “Check service selector” and “Check pod labels” headings to
lower-level headings that remain nested under the parent section. Wrap each
kubectl command in its own closed bash code fence, keeping the existing commands
and making the Markdown structure render correctly.
In `@helm/charts/nico-machine-a-tron/templates/configmap.yaml`:
- Line 36: Update the $hwMacBasePrefix fallback in the ConfigMap template from
the single-octet "06" value to the documented two-octet default "02:01",
matching values.yaml and producing valid hardware address prefixes when
basePrefix is absent.
- Around line 54-60: Validate the computed $podIndex after the podList lookup
and fail template rendering when it is greater than or equal to 256, before the
mac_address_pool and hw_mac_address_ranges values format it with %02x. Preserve
the existing index calculation for valid indices and use the chart’s established
template failure mechanism.
- Around line 29-35: Update the macAddressPool and hwMacAddressRanges generation
toggle assignments, including $macPoolEnabled and $hwMacRangesEnabled, so
explicit false values are preserved instead of being replaced by default true.
Use dig with an enabled key default or an explicit key-presence check, while
retaining true as the fallback when the key is absent.
🪄 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: 40c58f70-2d80-4a00-a75e-ab3bba7f42e5
📒 Files selected for processing (5)
docs/development/machine-a-tron-deployment.mdhelm-prereqs/values/machine-a-tron-multipod.yamlhelm/charts/nico-machine-a-tron/README.mdhelm/charts/nico-machine-a-tron/templates/configmap.yamlhelm/charts/nico-machine-a-tron/values.yaml
Comment on lines
+47
to
51
| ```text | ||
|
|
||
| --- | ||
|
|
||
| ## Mode 2: ClusterIP Mode (Scale Testing) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore code-fence boundaries around the TOML examples. Both fences begin after their intended snippets, causing following sections to render as literal text.
helm/charts/nico-machine-a-tron/README.md#L47-L51: open atomlfence before[site_explorer]and close it afteroverride_target_port.helm/charts/nico-machine-a-tron/README.md#L235-L236: open atomlfence before[networks.pod-0-oob]and close it after the final gateway.
As per path instructions, review Markdown for correctness and operator usability.
📍 Affects 1 file
helm/charts/nico-machine-a-tron/README.md#L47-L51(this comment)helm/charts/nico-machine-a-tron/README.md#L235-L236
🤖 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 47 - 51, Restore the
TOML code-fence boundaries in helm/charts/nico-machine-a-tron/README.md: at
lines 47-51, open a toml fence before [site_explorer] and close it after
override_target_port; at lines 235-236, open a toml fence before
[networks.pod-0-oob] and close it after the final gateway. Ensure subsequent
Markdown sections render normally.
Source: Path instructions
Comment on lines
+367
to
+375
| ## Check service selector | ||
|
|
||
| kubectl -n nico-mat get svc nico-machine-a-tron-bmc-10-100-0-2 -o jsonpath='{.spec.selector}' | ||
|
|
||
| # Check pod labels | ||
| ## Check pod labels | ||
|
|
||
| kubectl -n nico-mat get pods -l nvidia-infra-controller/pod-name=pod-0 --show-labels | ||
| ``` | ||
|
|
||
| ```text |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore troubleshooting heading and command-block structure.
These ## headings escape the parent troubleshooting section, and the fence opens after both commands. Use lower-level headings and closed bash fences around each command.
As per path instructions, review Markdown for correctness and operator usability.
🤖 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 367 - 375, Update the
troubleshooting section in the README by changing the “Check service selector”
and “Check pod labels” headings to lower-level headings that remain nested under
the parent section. Wrap each kubectl command in its own closed bash code fence,
keeping the existing commands and making the Markdown structure render
correctly.
Source: Path instructions
Comment on lines
+29
to
+35
| {{- $macPoolEnabled := $macPool.enabled | default true }} | ||
| {{- $macBasePrefix := $macPool.basePrefix | default "02:00" }} | ||
| {{- $macHostBits := $macPool.hostBits | default 16 }} | ||
|
|
||
| {{/* HW MAC address ranges config */}} | ||
| {{- $hwMacRanges := .Values.hwMacAddressRanges | default dict }} | ||
| {{- $hwMacRangesEnabled := $hwMacRanges.enabled | default true }} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
cat >/tmp/mac-pools-disabled.yaml <<'EOF'
macAddressPool:
enabled: false
hwMacAddressRanges:
enabled: false
EOF
helm template mat helm/charts/nico-machine-a-tron \
-f /tmp/mac-pools-disabled.yaml >/tmp/mac-pools-disabled.rendered.yaml
! rg -n '^\s*\[(mac_address_pool|hw_mac_address_ranges)\]' \
/tmp/mac-pools-disabled.rendered.yamlRepository: NVIDIA/infra-controller
Length of output: 204
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '\n== configmap template ==\n'
sed -n '1,120p' helm/charts/nico-machine-a-tron/templates/configmap.yaml
printf '\n== values references ==\n'
rg -n "macAddressPool|hwMacAddressRanges|basePrefix|hostBits|enabled" helm/charts/nico-machine-a-tron -g '!**/charts/**'Repository: NVIDIA/infra-controller
Length of output: 4595
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '\n== chart values and docs ==\n'
rg -n "macAddressPool|hwMacAddressRanges|enabled|basePrefix|hostBits|rangeHostBits" \
helm/charts/nico-machine-a-tron -g 'values*' -g 'README*' -g '*.md' -g '*.yaml'
printf '\n== surrounding values file ==\n'
for f in helm/charts/nico-machine-a-tron/values.yaml helm/charts/nico-machine-a-tron/values.yml; do
if [ -f "$f" ]; then
echo "--- $f"
sed -n '1,260p' "$f"
fi
doneRepository: NVIDIA/infra-controller
Length of output: 15683
Preserve explicit false for both generation toggles.
default true treats false as empty, so macAddressPool.enabled: false and hwMacAddressRanges.enabled: false still emit the generated sections. Use dig "enabled" true ... or an explicit key check so the documented disable path works.
🤖 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/configmap.yaml` around lines 29 -
35, Update the macAddressPool and hwMacAddressRanges generation toggle
assignments, including $macPoolEnabled and $hwMacRangesEnabled, so explicit
false values are preserved instead of being replaced by default true. Use dig
with an enabled key default or an explicit key-presence check, while retaining
true as the fallback when the key is absent.
Source: Path instructions
| {{/* HW MAC address ranges config */}} | ||
| {{- $hwMacRanges := .Values.hwMacAddressRanges | default dict }} | ||
| {{- $hwMacRangesEnabled := $hwMacRanges.enabled | default true }} | ||
| {{- $hwMacBasePrefix := $hwMacRanges.basePrefix | default "06" }} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the documented two-octet hardware prefix as the fallback.
When hwMacAddressRanges.basePrefix is absent, "06" renders a five-octet address such as 06:00:00:00:00. It must match the valid "02:01" default in values.yaml.
Proposed fix
-{{- $hwMacBasePrefix := $hwMacRanges.basePrefix | default "06" }}
+{{- $hwMacBasePrefix := $hwMacRanges.basePrefix | default "02:01" }}📝 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
| {{- $hwMacBasePrefix := $hwMacRanges.basePrefix | default "06" }} | |
| {{- $hwMacBasePrefix := $hwMacRanges.basePrefix | default "02:01" }} |
🤖 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/configmap.yaml` at line 36, Update
the $hwMacBasePrefix fallback in the ConfigMap template from the single-octet
"06" value to the documented two-octet default "02:01", matching values.yaml and
producing valid hardware address prefixes when basePrefix is absent.
Comment on lines
+54
to
+60
| {{/* Get pod index for MAC address pool */}} | ||
| {{- $podIndex := 0 }} | ||
| {{- range $i, $p := $podList }} | ||
| {{- if eq $p $podName }} | ||
| {{- $podIndex = $i }} | ||
| {{- end }} | ||
| {{- end }} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
sed -n '1,140p' helm/charts/nico-machine-a-tron/templates/configmap.yaml
printf '\n--- SEARCH MAC / POD INDEX ---\n'
rg -n "%02x|podIndex|MAC|mac" helm/charts/nico-machine-a-tron -SRepository: NVIDIA/infra-controller
Length of output: 31333
Guard the pod-index MAC base at 256
Both mac_address_pool and hw_mac_address_ranges format $podIndex with %02x; once $podIndex >= 256, the generated octet becomes 100, so fail template rendering before emitting an invalid MAC prefix.
🤖 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/configmap.yaml` around lines 54 -
60, Validate the computed $podIndex after the podList lookup and fail template
rendering when it is greater than or equal to 256, before the mac_address_pool
and hw_mac_address_ranges values format it with %02x. Preserve the existing
index calculation for valid indices and use the chart’s established template
failure mechanism.
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3700.docs.buildwithfern.com/infra-controller |
poroh
approved these changes
Jul 17, 2026
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
2 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.
Adds auto-generated per-pod MAC address pools to prevent collisions in multi-pod machine-a-tron deployments.
mac_address_pool- Standalone MACs (BMC, DPU interfaces)hw_mac_address_ranges- Contiguous NIC blocks (ConnectX, storage)Related issues
Closes #3698
Type of Change
Breaking Changes
Testing
Test Results