Skip to content

fix(nvca): inject BYOO env vars into Helm utils collector#346

Merged
shobham-nv merged 1 commit into
mainfrom
shobham/fix-helm-byoo-log-chunking-utils-pod
Jul 22, 2026
Merged

fix(nvca): inject BYOO env vars into Helm utils collector#346
shobham-nv merged 1 commit into
mainfrom
shobham/fix-helm-byoo-log-chunking-utils-pod

Conversation

@shobham-nv

@shobham-nv shobham-nv commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

TL;DR

Inject BYOO OTel collector environment variables directly into the collector sidecar on Helm-generated utils pods.

The webhook special-cases utils pods and skips the normal metadata-based environment injection. Consequently, log-chunking settings were present in MiniService metadata but absent from the collector, causing oversized logs to be exported without chunking.

Additional Details (optional for docs, build, test, refactor, ci, chore, style, and revert PRs)

  • Preserve metadata-based collector configuration for workload pods.
  • Apply BYOOOTelCollectorEnvVars() directly to the generated Helm utils pod using AddBYOOEnvVarsToPodSpec.
  • The helper targets only the byoo-otel-collector sidecar.
  • Using BYOOOTelCollectorEnvVars() keeps this compatible with additional collector settings introduced by feat(byoo): gate collector config env vars #194.
  • Add regression coverage confirming the collector receives:
    • BYOO_LOG_CHUNK_MAX_BODY_BYTES=262144
    • BYOO_LOG_CHUNK_DRY_RUN=true
    • BYOO_LOG_EXPORTER_BATCH_MAX_SIZE_BYTES=1000000
  • End-to-end staging validation is not included in this PR.

Related: #194

For the Reviewer

Please review:

  • src/compute-plane-services/nvca/internal/miniservice/reconcile.go
    • Confirm direct injection into the generated utils pod is the appropriate scope.
    • Confirm the metadata path should remain for workload pods.
  • src/compute-plane-services/nvca/internal/miniservice/reconcile_test.go
    • Confirm the regression assertions cover the expected collector settings.

For QA (optional for docs, build, test, refactor, ci, chore, style, and revert PRs)

Local verification:

GOWORK=off go test ./internal/miniservice/ -run 'TestReconcile_Function$' -count=1 -ldflags '-X github.com/NVIDIA/k8s-dra-driver-gpu/internal/info.version=v25.8.0'

Result: passed.

QA is recommended in staging:

  1. Deploy a Helm-based function with BYOO logs enabled.
  2. Inspect the byoo-otel-collector container on the generated utils pod.
  3. Verify the expected BYOO_LOG_* environment variables are present.
  4. Emit an oversized log record.
  5. Verify it is exported as chunked records and no HTTP 413 is reported.

Issues

NO-REF

Checklist

  • I am familiar with the Contributing Guidelines.
  • I have signed off my commits for Developer Certificate of Origin (DCO) compliance.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

Summary by CodeRabbit

  • Bug Fixes
    • Improved BYOO telemetry setup by applying collector environment settings directly to the utility pod.
    • Ensured BYOO log chunking and OpenTelemetry collector settings are consistently applied and reflected in deployment metadata.
  • Tests
    • Added coverage validating collector sidecar environment variables and updated log chunking expectations.

@shobham-nv
shobham-nv requested a review from a team as a code owner July 22, 2026 07:38
@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

BYOO collector environment variables are now injected directly into the utils pod while remaining in metadata input. Reconciliation tests verify the collector sidecar environment and update expected log chunking values.

Changes

BYOO collector environment handling

Layer / File(s) Summary
Inject collector environment variables into the utils pod
src/compute-plane-services/nvca/internal/miniservice/reconcile.go, src/compute-plane-services/nvca/internal/miniservice/reconcile_test.go
doInstall injects BYOO collector variables into utilsPod.Spec; reconciliation tests verify sidecar environment values and updated metadata expectations.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: kristinapathak

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title follows Conventional Commits and accurately describes the main change to inject BYOO env vars into the Helm utils collector.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch shobham/fix-helm-byoo-log-chunking-utils-pod

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.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"


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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/compute-plane-services/nvca/internal/miniservice/reconcile_test.go (1)

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

Assert the full collector environment contract in the sidecar.

BYOOOTelCollectorEnvVars() also includes metric-subset and workload-metrics variables, but this new pod assertion checks only the three log-related keys. The later metadata assertions do not prove those additional values were injected into the sidecar; add equivalent sidecar assertions or compare against the complete expected env set.

Proposed test extension
 	assert.Equal(t, "262144", byooCollectorEnv[nvcaconfig.BYOOLogChunkMaxBodyBytesEnv])
 	assert.Equal(t, "true", byooCollectorEnv[nvcaconfig.BYOOLogChunkDryRunEnv])
 	assert.Equal(t, "1000000", byooCollectorEnv[nvcaconfig.BYOOLogExporterBatchMaxSizeBytesEnv])
+	assert.Equal(t, "true", byooCollectorEnv[nvcaconfig.BYOOMetricSubsetEnabledEnv])
+	assert.Contains(t, byooCollectorEnv[nvcaconfig.BYOOMetricSubsetFilterConfigEnv], "metric.name")
+	assert.Equal(t, "metric_subset_enabled,custom_label",
+		byooCollectorEnv[nvcaconfig.BYOOWorkloadMetricsDropLabelsEnv])
🤖 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 `@src/compute-plane-services/nvca/internal/miniservice/reconcile_test.go`
around lines 662 - 676, The sidecar test only validates three log-related
environment variables and misses the metric-subset and workload-metrics values
from BYOOOTelCollectorEnvVars(). Extend the assertions for byooCollectorEnv in
the existing utils pod test to cover every expected collector environment
variable, or compare the collected environment against the complete expected set
returned by BYOOOTelCollectorEnvVars(), while preserving the current log
assertions.
🤖 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 `@src/compute-plane-services/nvca/internal/miniservice/reconcile_test.go`:
- Around line 662-676: The sidecar test only validates three log-related
environment variables and misses the metric-subset and workload-metrics values
from BYOOOTelCollectorEnvVars(). Extend the assertions for byooCollectorEnv in
the existing utils pod test to cover every expected collector environment
variable, or compare the collected environment against the complete expected set
returned by BYOOOTelCollectorEnvVars(), while preserving the current log
assertions.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: cb106089-36b3-4e71-ba4e-e2fb0d3552d5

📥 Commits

Reviewing files that changed from the base of the PR and between 96448e1 and e63b3b4.

📒 Files selected for processing (2)
  • src/compute-plane-services/nvca/internal/miniservice/reconcile.go
  • src/compute-plane-services/nvca/internal/miniservice/reconcile_test.go

@apartha-nv apartha-nv 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.

lgtm

@shobham-nv
shobham-nv added this pull request to the merge queue Jul 22, 2026
Merged via the queue into main with commit bd8fc4f Jul 22, 2026
13 checks passed
@shobham-nv
shobham-nv deleted the shobham/fix-helm-byoo-log-chunking-utils-pod branch July 22, 2026 08:45
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.

2 participants