Skip to content

feat(machine-a-tron): k8s controller for mock BMC services#3955

Open
akorobkov-nvda wants to merge 17 commits into
NVIDIA:mainfrom
akorobkov-nvda:feat/machine-a-tron-k8s-ctrl
Open

feat(machine-a-tron): k8s controller for mock BMC services#3955
akorobkov-nvda wants to merge 17 commits into
NVIDIA:mainfrom
akorobkov-nvda:feat/machine-a-tron-k8s-ctrl

Conversation

@akorobkov-nvda

@akorobkov-nvda akorobkov-nvda commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Implements a Kubernetes controller that dynamically creates Services for machine-a-tron mock BMC endpoints, enabling multi-pod deployments where each BMC needs stable network exposure.

Why this change?
When machine-a-tron runs in Kubernetes with multiple pods, mock BMCs need stable network exposure so NICo components can reach Redfish endpoints. This controller consumes machine-a-tron's /machines/status API and reconciles one Service per mock BMC.

Machine-a-tron itself stays Kubernetes-agnostic - all K8s logic lives in this separate controller.

What's included:
Controller (dev/k8s/machine-a-tron-controller/):

  • Discovers machine-a-tron pods via nvidia-infra-controller/mat-service=true label
  • Polls /machines/status from each discovered instance
  • Creates Services with BMC IP as ClusterIP for direct addressing
  • Pod-specific selectors ensure traffic routes to the correct machine-a-tron instance
  • Supports hosts, DPUs, switches and power shelves
  • Skips deletions on partial fetch failures to prevent spurious Service removal
  • Handles ClusterIP immutability via delete+recreate

Helm subchart (helm/charts/nico-machine-a-tron/charts/mat-k8s-controller/):

  • Enable with mat-k8s-controller.enabled=true
  • Configurable discovery/target selectors scoped to release
  • RBAC for Service management

CI/CD:

  • Added build-mat-k8s-controller job to main CI workflow
  • Multi-arch Docker image (amd64/arm64)

Related issues

Type of Change

  • Add - New feature or capability
  • Change - Changes in existing functionality
  • Fix - Bug fixes
  • Remove - Removed features or deprecated functionality
  • Internal - Internal changes (refactoring, tests, docs, etc.)

Testing

  • Unit tests added/updated
  • Integration tests added/updated
  • Manual testing performed
  • No testing required (docs, internal refactor, etc.)

Manual testing verified:

  • Controller discovers multiple machine-a-tron pods
  • Services created with correct ClusterIPs matching BMC IPs
  • Pod-specific selectors route traffic to correct instance
  • Services updated when machine status changes
  • Stale services deleted when machines removed

Additional Notes

ServiceCIDR requirement: BMC IPs must fall within Kubernetes ServiceCIDR range

@akorobkov-nvda
akorobkov-nvda requested review from a team and lauragra-y as code owners July 23, 2026 06:41
@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

This PR adds a Kubernetes controller that polls machine-a-tron status, discovers mock BMC Services, and reconciles host and DPU Services. It includes an HTTP client, container image, Helm deployment resources, parent-chart integration, tests, and CI image building.

Changes

Machine-a-tron Kubernetes controller

Layer / File(s) Summary
Machine status client and data model
dev/k8s/machine-a-tron-controller/pkg/matclient/*, dev/k8s/machine-a-tron-controller/go.mod
Defines machine status payloads and retrieves /machines/status with URL validation, bounded response handling, optional TLS verification, and tests.
Service discovery and reconciliation
dev/k8s/machine-a-tron-controller/pkg/controller/*
Discovers labeled Services, builds host/DPU Services with BMC metadata and ports, computes changes, applies Kubernetes operations, and validates behavior with unit tests.
Controller entrypoint and image
dev/k8s/machine-a-tron-controller/cmd/*, Dockerfile, Makefile, README.md
Adds configuration parsing, signal-aware periodic reconciliation, logging, development targets, documentation, and a static non-root distroless image.
Controller Helm chart
helm/charts/nico-machine-a-tron/charts/mat-k8s-controller/*
Adds configurable Deployment, ServiceAccount, RBAC, naming helpers, selectors, resource settings, and security context configuration.
Parent chart integration and CI packaging
helm/charts/nico-machine-a-tron/{Chart.yaml,values.yaml,templates/*}, .github/workflows/ci.yaml, crates/machine-a-tron/README.md
Adds chart integration, prevents conflicting BMC Service generation, labels discoverable Services, tightens pod filtering, updates documentation, and builds the controller image for amd64 and arm64 in CI.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Controller
  participant MATServices
  participant MachineATron
  participant KubernetesAPI
  Controller->>MATServices: Discover labeled Services
  MATServices-->>Controller: Service URLs and pod names
  Controller->>MachineATron: GET /machines/status
  MachineATron-->>Controller: Machine and BMC status
  Controller->>KubernetesAPI: List managed Services
  Controller->>KubernetesAPI: Create, update, recreate, or delete Services
Loading

Suggested labels: infra

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 46.15% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change: adding a Kubernetes controller for machine-a-tron mock BMC services.
Description check ✅ Passed The description is directly aligned with the controller, Helm, and CI changes in the pull request.
Linked Issues check ✅ Passed The changes satisfy #3380 by polling /machines/status, reconciling Services, exposing Redfish/IPMI, labeling identities, and deleting stale Services.
Out of Scope Changes check ✅ Passed No clear out-of-scope code changes stand out; the supporting workflow, Helm, tests, and docs all relate to the controller feature.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@akorobkov-nvda akorobkov-nvda changed the title Feat/machine a tron k8s ctrl feat(machine-a-tron): k8s controller for mock BMC services Jul 23, 2026
@akorobkov-nvda
akorobkov-nvda requested a review from poroh July 23, 2026 06:47

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (4)
dev/k8s/machine-a-tron-controller/pkg/controller/discovery.go (1)

57-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider making the cluster DNS domain configurable.

svc.cluster.local is hardcoded; clusters with a custom --cluster-domain would break discovery URLs. As per path instructions for dev/**, development tooling should use "safe defaults" while remaining consistent with deployment manifests — exposing this as a constructor parameter (defaulting to cluster.local) would make the tool portable.

🤖 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/machine-a-tron-controller/pkg/controller/discovery.go` at line 57,
Make the cluster DNS domain configurable in the discovery component by adding a
constructor parameter with a safe default of "cluster.local", then use that
value instead of the hardcoded suffix in the URL built by the discovery logic.
Update the relevant constructor call sites while preserving existing behavior
when no custom domain is provided.

Source: Path instructions

dev/k8s/machine-a-tron-controller/pkg/controller/controller.go (2)

156-160: 🩺 Stability & Availability | 🔵 Trivial

Reserve the BMC IP range from dynamic Service allocation.

Setting ClusterIP directly from the BMC's static IP works only if that address is guaranteed free. Unless the BMC IP subrange is excluded from Kubernetes' dynamic Service IP allocator (e.g. via --service-cluster-ip-range carve-out or a reserved sub-range), an unrelated auto-assigned Service could already hold that IP, causing Create/Update here to fail with an allocation conflict.

🤖 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/machine-a-tron-controller/pkg/controller/controller.go` around lines
156 - 160, Reserve the BMC IP range used by the ClusterIP assignment in the
Kubernetes Service CIDR configuration, using a dedicated non-overlapping
sub-range excluded from dynamic Service allocation. Update the
deployment/configuration referenced by the controller’s Service creation path so
addresses assigned from machine.BMC.IP cannot be auto-allocated to unrelated
Services.

361-367: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Reuse matclient.Client instances across reconcile passes.

A new matclient.Client is constructed per instance on every Reconcile() call. When WithInsecureSkipVerify is part of clientOpts (likely, given self-signed BMC certs), each call clones a brand-new *http.Transport, discarding the previous pass's connection pool and forcing a fresh TCP+TLS handshake every cycle instead of reusing keep-alives.

♻️ Proposed fix: cache clients per instance URL
 type Reconciler struct {
 	discovery      *MatPodDiscovery
 	serviceBuilder *ServiceBuilder
 	k8sClient      K8sServiceClient
 	clientOpts     []matclient.Option
 	logger         zerolog.Logger
+	clients        map[string]*matclient.Client
 }
@@
 	for _, instance := range instances {
-		client, err := matclient.NewClient(instance.URL, r.clientOpts...)
-		if err != nil {
-			result.Errors = append(result.Errors, fmt.Errorf("creating client for %s: %w", instance.URL, err))
-			fetchFailed = true
-			continue
+		client, ok := r.clients[instance.URL]
+		if !ok {
+			var err error
+			client, err = matclient.NewClient(instance.URL, r.clientOpts...)
+			if err != nil {
+				result.Errors = append(result.Errors, fmt.Errorf("creating client for %s: %w", instance.URL, err))
+				fetchFailed = true
+				continue
+			}
+			if r.clients == nil {
+				r.clients = make(map[string]*matclient.Client)
+			}
+			r.clients[instance.URL] = client
 		}
🤖 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/machine-a-tron-controller/pkg/controller/controller.go` around lines
361 - 367, Cache matclient.Client instances by instance.URL across Reconcile
passes instead of constructing them on every iteration in Reconcile. Add or
reuse controller-level client storage, return the cached client when available,
and only call matclient.NewClient for unseen URLs; preserve the existing error
aggregation and fetchFailed behavior for client-creation failures.
dev/k8s/machine-a-tron-controller/pkg/controller/controller_test.go (1)

280-389: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a recreate-on-ClusterIP-change test case.

The TestComputeServiceDiff table covers create/update/delete but omits the Recreate path (immutable ClusterIP change), which is an explicitly called-out behavior for this controller.

✅ Proposed additional test case
+		{
+			name: "recreate on ClusterIP change",
+			desired: []*corev1.Service{
+				func() *corev1.Service {
+					svc := makeTestService("svc-1", "mat-id-1")
+					svc.Spec.ClusterIP = "10.0.0.5"
+					return svc
+				}(),
+			},
+			existing: []*corev1.Service{
+				func() *corev1.Service {
+					svc := makeTestService("svc-1", "mat-id-1")
+					svc.Spec.ClusterIP = "10.0.0.6"
+					return svc
+				}(),
+			},
+			wantRecreateCount: 1,
+		},

(Requires adding a wantRecreateCount field to the test struct and asserting diff.Recreate.)

🤖 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/machine-a-tron-controller/pkg/controller/controller_test.go` around
lines 280 - 389, Add a wantRecreateCount field to the TestComputeServiceDiff
table, add a case with matching service identity but different ClusterIP values
expecting one recreate, and assert diff.Recreate alongside Create, Update, and
Delete counts. Keep the existing cases’ expected recreate count at zero.
🤖 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 @.github/workflows/ci.yaml:
- Line 1863: Update the core-ci-pass job’s needs list to include
build-mat-k8s-controller, matching its inclusion in build-summary and
notify-build-status. Preserve the existing dependency entries and ensure a
failed controller build prevents core-ci-pass from passing.
- Around line 819-843: Add build-mat-k8s-controller to the core-ci-pass
aggregator’s required job dependencies so its failure makes the merge-gating
check fail. Preserve the existing job configuration and ensure the dependency
references this exact job identifier.

In `@dev/k8s/machine-a-tron-controller/cmd/mat-k8s-controller/main.go`:
- Line 123: Validate the sync interval before the time.NewTicker call in the
controller startup flow, rejecting zero or negative values from either
SYNC_INTERVAL or --sync-interval. Fail fast with a clear error message, and only
create the ticker after the interval passes validation.

In `@dev/k8s/machine-a-tron-controller/Makefile`:
- Around line 4-10: Add an `all` target to the Makefile and include it in the
`.PHONY` declaration, wiring it to the appropriate default build workflow so
`make all` succeeds and satisfies checkmake.

In `@dev/k8s/machine-a-tron-controller/pkg/matclient/client.go`:
- Around line 109-123: Update applyInsecureTLS to set the cloned transport’s
TLSClientConfig.MinVersion to the required minimum TLS protocol, while
preserving InsecureSkipVerify for dev/test certificates and the existing
transport cloning behavior.

In `@dev/k8s/machine-a-tron-controller/README.md`:
- Around line 68-72: Update the README command example for make run to use the
$HOME-based kubeconfig path instead of ~/.kube/config, ensuring Make and the
shell pass the expanded absolute path to client-go.

---

Nitpick comments:
In `@dev/k8s/machine-a-tron-controller/pkg/controller/controller_test.go`:
- Around line 280-389: Add a wantRecreateCount field to the
TestComputeServiceDiff table, add a case with matching service identity but
different ClusterIP values expecting one recreate, and assert diff.Recreate
alongside Create, Update, and Delete counts. Keep the existing cases’ expected
recreate count at zero.

In `@dev/k8s/machine-a-tron-controller/pkg/controller/controller.go`:
- Around line 156-160: Reserve the BMC IP range used by the ClusterIP assignment
in the Kubernetes Service CIDR configuration, using a dedicated non-overlapping
sub-range excluded from dynamic Service allocation. Update the
deployment/configuration referenced by the controller’s Service creation path so
addresses assigned from machine.BMC.IP cannot be auto-allocated to unrelated
Services.
- Around line 361-367: Cache matclient.Client instances by instance.URL across
Reconcile passes instead of constructing them on every iteration in Reconcile.
Add or reuse controller-level client storage, return the cached client when
available, and only call matclient.NewClient for unseen URLs; preserve the
existing error aggregation and fetchFailed behavior for client-creation
failures.

In `@dev/k8s/machine-a-tron-controller/pkg/controller/discovery.go`:
- Line 57: Make the cluster DNS domain configurable in the discovery component
by adding a constructor parameter with a safe default of "cluster.local", then
use that value instead of the hardcoded suffix in the URL built by the discovery
logic. Update the relevant constructor call sites while preserving existing
behavior when no custom domain 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: 3dd9240a-418e-49b6-916d-35ce5e8b9a28

📥 Commits

Reviewing files that changed from the base of the PR and between 2de970b and af3987e.

⛔ Files ignored due to path filters (1)
  • dev/k8s/machine-a-tron-controller/go.sum is excluded by !**/*.sum
📒 Files selected for processing (26)
  • .github/workflows/ci.yaml
  • crates/machine-a-tron/README.md
  • dev/k8s/machine-a-tron-controller/Dockerfile
  • dev/k8s/machine-a-tron-controller/Makefile
  • dev/k8s/machine-a-tron-controller/README.md
  • dev/k8s/machine-a-tron-controller/cmd/mat-k8s-controller/main.go
  • dev/k8s/machine-a-tron-controller/go.mod
  • dev/k8s/machine-a-tron-controller/pkg/controller/controller.go
  • dev/k8s/machine-a-tron-controller/pkg/controller/controller_test.go
  • dev/k8s/machine-a-tron-controller/pkg/controller/discovery.go
  • dev/k8s/machine-a-tron-controller/pkg/controller/k8s_client.go
  • dev/k8s/machine-a-tron-controller/pkg/matclient/client.go
  • dev/k8s/machine-a-tron-controller/pkg/matclient/client_test.go
  • dev/k8s/machine-a-tron-controller/pkg/matclient/types.go
  • helm/charts/nico-machine-a-tron/Chart.yaml
  • helm/charts/nico-machine-a-tron/README.md
  • helm/charts/nico-machine-a-tron/charts/mat-k8s-controller/Chart.yaml
  • helm/charts/nico-machine-a-tron/charts/mat-k8s-controller/templates/_helpers.tpl
  • helm/charts/nico-machine-a-tron/charts/mat-k8s-controller/templates/deployment.yaml
  • helm/charts/nico-machine-a-tron/charts/mat-k8s-controller/templates/rbac.yaml
  • helm/charts/nico-machine-a-tron/charts/mat-k8s-controller/templates/serviceaccount.yaml
  • helm/charts/nico-machine-a-tron/charts/mat-k8s-controller/values.yaml
  • helm/charts/nico-machine-a-tron/templates/bmc-services.yaml
  • helm/charts/nico-machine-a-tron/templates/deployment.yaml
  • helm/charts/nico-machine-a-tron/templates/service.yaml
  • helm/charts/nico-machine-a-tron/values.yaml

Comment thread .github/workflows/ci.yaml
Comment thread .github/workflows/ci.yaml
Comment thread dev/k8s/machine-a-tron-controller/cmd/mat-k8s-controller/main.go
Comment thread dev/k8s/machine-a-tron-controller/Makefile Outdated
Comment thread dev/k8s/machine-a-tron-controller/pkg/matclient/client.go
Comment thread dev/k8s/machine-a-tron-controller/README.md

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
.github/workflows/ci.yaml (1)

819-844: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Trigger the controller build when controller files change.

This job is gated by source_files_changed, but the prepare path filter at Lines [168]-[173] does not include dev/k8s/machine-a-tron-controller/**. Consequently, changes to the controller’s Go sources or Dockerfile can skip this job and leave the deployed image stale.

Add the controller path to the source filter or introduce a dedicated change flag.

As per path instructions, GitHub Actions trigger correctness and CI coverage gaps must be reviewed.

Proposed fix
             source_files:
               - 'crates/**'
+              - 'dev/k8s/machine-a-tron-controller/**'
               - 'Cargo.toml'
🤖 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 @.github/workflows/ci.yaml around lines 819 - 844, Update the prepare path
filter used to produce source_files_changed so it includes
dev/k8s/machine-a-tron-controller/**. Preserve the existing
build-mat-k8s-controller condition and ensure changes to the controller sources
or Dockerfile set source_files_changed to true, triggering the controller image
build.

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.

Outside diff comments:
In @.github/workflows/ci.yaml:
- Around line 819-844: Update the prepare path filter used to produce
source_files_changed so it includes dev/k8s/machine-a-tron-controller/**.
Preserve the existing build-mat-k8s-controller condition and ensure changes to
the controller sources or Dockerfile set source_files_changed to true,
triggering the controller image build.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: bc6891e1-6c2f-4160-b296-af703f5b0bba

📥 Commits

Reviewing files that changed from the base of the PR and between af3987e and 6718862.

📒 Files selected for processing (5)
  • .github/workflows/ci.yaml
  • dev/k8s/machine-a-tron-controller/Makefile
  • dev/k8s/machine-a-tron-controller/README.md
  • dev/k8s/machine-a-tron-controller/cmd/mat-k8s-controller/main.go
  • dev/k8s/machine-a-tron-controller/pkg/matclient/client.go
🚧 Files skipped from review as they are similar to previous changes (3)
  • dev/k8s/machine-a-tron-controller/README.md
  • dev/k8s/machine-a-tron-controller/cmd/mat-k8s-controller/main.go
  • dev/k8s/machine-a-tron-controller/pkg/matclient/client.go

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
.github/workflows/ci.yaml (1)

174-174: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Use a controller-specific change gate.

source_files_changed also matches broad paths such as crates/**, Cargo.toml, and Cargo.lock. As a result, unrelated Rust changes trigger this two-architecture controller image build even though its context is only dev/k8s/machine-a-tron-controller. Add a dedicated controller-path output and gate this job on that output, while retaining source_files_changed for build-machine-a-tron.

As per path instructions, this workflow should minimize unnecessary CI work and preserve accurate coverage.

Also applies to: 820-827

🤖 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 @.github/workflows/ci.yaml at line 174, Update the path-filter outputs and
the controller image job’s condition in the CI workflow to use a dedicated
output matching only dev/k8s/machine-a-tron-controller/**. Keep
source_files_changed as the gate for build-machine-a-tron, and ensure the
controller-specific output is used consistently for the two-architecture
controller build.

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.

Nitpick comments:
In @.github/workflows/ci.yaml:
- Line 174: Update the path-filter outputs and the controller image job’s
condition in the CI workflow to use a dedicated output matching only
dev/k8s/machine-a-tron-controller/**. Keep source_files_changed as the gate for
build-machine-a-tron, and ensure the controller-specific output is used
consistently for the two-architecture controller build.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 47106ce5-ef3e-4a3a-a298-73a5951d6bfc

📥 Commits

Reviewing files that changed from the base of the PR and between 6718862 and 9bf6d81.

📒 Files selected for processing (1)
  • .github/workflows/ci.yaml

@akorobkov-nvda
akorobkov-nvda force-pushed the feat/machine-a-tron-k8s-ctrl branch from 9bf6d81 to 5068f1b Compare July 23, 2026 17:33
@github-actions

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
dev/k8s/machine-a-tron-controller/Dockerfile (1)

5-13: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Cross-compile instead of emulating the arm64 builder.

The CI job builds linux/amd64 and linux/arm64 on an amd64 runner. Without an explicit BUILDPLATFORM builder and TARGETOS/TARGETARCH arguments, the arm64 build may run the Go toolchain under emulation, slowing CI and adding an unnecessary QEMU dependency.

As per path instructions, Dockerfiles should explicitly support architecture-aware builds.

Proposed fix
 # Build stage
-FROM golang:1.26.4-alpine@sha256:3ad57304ad93bbec8548a0437ad9e06a455660655d9af011d58b993f6f615648 AS builder
+FROM --platform=$BUILDPLATFORM golang:1.26.4-alpine@sha256:3ad57304ad93bbec8548a0437ad9e06a455660655d9af011d58b993f6f615648 AS builder
+ARG TARGETOS
+ARG TARGETARCH

 WORKDIR /app
 ...
-RUN CGO_ENABLED=0 GOOS=linux go build -ldflags="-s -w" -o mat-k8s-controller ./cmd/mat-k8s-controller
+RUN CGO_ENABLED=0 GOOS=$TARGETOS GOARCH=$TARGETARCH go build -ldflags="-s -w" -o mat-k8s-controller ./cmd/mat-k8s-controller
🤖 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/machine-a-tron-controller/Dockerfile` around lines 5 - 13, Update the
Dockerfile’s builder stage to use the native build platform and declare the
target platform arguments, then pass TARGETOS and TARGETARCH to the Go build
command so linux/arm64 is cross-compiled rather than emulated. Preserve the
existing dependency download, static build settings, output name, and pinned
base image.

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/machine-a-tron-controller/pkg/controller/controller.go`:
- Around line 72-78: Update BuildServiceName to use the complete matID when
constructing the Service name; remove the shortID truncation logic and preserve
the existing “mat-bmc-<machineType>-<matID>” format.

In `@helm/charts/nico-machine-a-tron/README.md`:
- Line 101: Update the dpuPerHostCount example comment to use the literal ->
arrow instead of the escaped -&gt; entity, preserving the surrounding YAML
example unchanged.

---

Nitpick comments:
In `@dev/k8s/machine-a-tron-controller/Dockerfile`:
- Around line 5-13: Update the Dockerfile’s builder stage to use the native
build platform and declare the target platform arguments, then pass TARGETOS and
TARGETARCH to the Go build command so linux/arm64 is cross-compiled rather than
emulated. Preserve the existing dependency download, static build settings,
output name, and pinned base image.
🪄 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: 380282ec-0288-4e49-b256-d65c5d549e18

📥 Commits

Reviewing files that changed from the base of the PR and between 9bf6d81 and 5068f1b.

⛔ Files ignored due to path filters (1)
  • dev/k8s/machine-a-tron-controller/go.sum is excluded by !**/*.sum
📒 Files selected for processing (26)
  • .github/workflows/ci.yaml
  • crates/machine-a-tron/README.md
  • dev/k8s/machine-a-tron-controller/Dockerfile
  • dev/k8s/machine-a-tron-controller/Makefile
  • dev/k8s/machine-a-tron-controller/README.md
  • dev/k8s/machine-a-tron-controller/cmd/mat-k8s-controller/main.go
  • dev/k8s/machine-a-tron-controller/go.mod
  • dev/k8s/machine-a-tron-controller/pkg/controller/controller.go
  • dev/k8s/machine-a-tron-controller/pkg/controller/controller_test.go
  • dev/k8s/machine-a-tron-controller/pkg/controller/discovery.go
  • dev/k8s/machine-a-tron-controller/pkg/controller/k8s_client.go
  • dev/k8s/machine-a-tron-controller/pkg/matclient/client.go
  • dev/k8s/machine-a-tron-controller/pkg/matclient/client_test.go
  • dev/k8s/machine-a-tron-controller/pkg/matclient/types.go
  • helm/charts/nico-machine-a-tron/Chart.yaml
  • helm/charts/nico-machine-a-tron/README.md
  • helm/charts/nico-machine-a-tron/charts/mat-k8s-controller/Chart.yaml
  • helm/charts/nico-machine-a-tron/charts/mat-k8s-controller/templates/_helpers.tpl
  • helm/charts/nico-machine-a-tron/charts/mat-k8s-controller/templates/deployment.yaml
  • helm/charts/nico-machine-a-tron/charts/mat-k8s-controller/templates/rbac.yaml
  • helm/charts/nico-machine-a-tron/charts/mat-k8s-controller/templates/serviceaccount.yaml
  • helm/charts/nico-machine-a-tron/charts/mat-k8s-controller/values.yaml
  • helm/charts/nico-machine-a-tron/templates/bmc-services.yaml
  • helm/charts/nico-machine-a-tron/templates/deployment.yaml
  • helm/charts/nico-machine-a-tron/templates/service.yaml
  • helm/charts/nico-machine-a-tron/values.yaml
🚧 Files skipped from review as they are similar to previous changes (17)
  • helm/charts/nico-machine-a-tron/templates/service.yaml
  • helm/charts/nico-machine-a-tron/charts/mat-k8s-controller/Chart.yaml
  • helm/charts/nico-machine-a-tron/Chart.yaml
  • helm/charts/nico-machine-a-tron/charts/mat-k8s-controller/values.yaml
  • dev/k8s/machine-a-tron-controller/README.md
  • dev/k8s/machine-a-tron-controller/Makefile
  • dev/k8s/machine-a-tron-controller/pkg/matclient/client_test.go
  • dev/k8s/machine-a-tron-controller/go.mod
  • crates/machine-a-tron/README.md
  • dev/k8s/machine-a-tron-controller/pkg/matclient/types.go
  • helm/charts/nico-machine-a-tron/charts/mat-k8s-controller/templates/_helpers.tpl
  • helm/charts/nico-machine-a-tron/values.yaml
  • helm/charts/nico-machine-a-tron/templates/deployment.yaml
  • dev/k8s/machine-a-tron-controller/cmd/mat-k8s-controller/main.go
  • dev/k8s/machine-a-tron-controller/pkg/controller/k8s_client.go
  • dev/k8s/machine-a-tron-controller/pkg/controller/discovery.go
  • dev/k8s/machine-a-tron-controller/pkg/controller/controller_test.go

Comment thread dev/k8s/machine-a-tron-controller/pkg/controller/controller.go
Comment thread helm/charts/nico-machine-a-tron/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(machine-a-tron): add Kubernetes controller for mock BMC Services

2 participants