Skip to content

Conversation

@adityachoudhari26
Copy link
Contributor

@adityachoudhari26 adityachoudhari26 commented Nov 18, 2025

Summary by CodeRabbit

  • Chores
    • Updated OpenTelemetry workload to run as a DaemonSet instead of a Deployment, removed replica settings, and aligned configuration keys accordingly.
    • Bumped chart version to 0.8.0 to reflect the rollout of these configuration changes.

@coderabbitai
Copy link

coderabbitai bot commented Nov 18, 2025

Walkthrough

Converted the OpenTelemetry Helm chart to use a Kubernetes DaemonSet instead of a Deployment; updated Helm value references from .Values.deployment.* to .Values.daemonset.*; removed the replicas field; renamed the top-level values key from deployment to daemonset.

Changes

Cohort / File(s) Summary
Workload template and values
charts/ctrlplane/charts/otel/templates/daemonset.yaml, charts/ctrlplane/charts/otel/values.yaml
Replaced resource kind Deployment → DaemonSet; removed replicas; updated Helm references from .Values.deployment.*.Values.daemonset.*; renamed top-level values key deploymentdaemonset.
Chart metadata
charts/ctrlplane/Chart.yaml
Bumped chart version from 0.7.1 to 0.8.0.

Sequence Diagram(s)

sequenceDiagram
    participant Dev as Developer (Helm)
    participant Helm as Helm chart
    participant K8s as Kubernetes API
    rect rgba(115,179,255,0.08)
      Dev->>Helm: render templates with .Values.daemonset.*
      Helm->>K8s: apply DaemonSet manifest
      K8s->>Nodes: schedule one pod per node (DaemonSet)
    end
    note right of K8s: replicas field removed — scheduling handled by DaemonSet controller
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Check all occurrences of .Values.deployment.* across the chart and docs.
  • Validate the DaemonSet template for required fields (selector, pod template) and indentation/templating correctness.
  • Ensure no external tooling or consumers expect the deployment top-level key.
  • Confirm chart version bump aligns with release policy.

Poem

🐰 I swapped one pod for every road and trail,
No replica count, each node tells the tale.
DaemonSet hops to every tree and lair,
Telemetry scampers and breathes the air,
A rabbit's cheer for wide deployment flair! 🥕✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: converting the OTEL workload from a Deployment to a DaemonSet across the Helm chart templates and values.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch otel-daemonset

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between a3ba26e and ce1628f.

📒 Files selected for processing (1)
  • charts/ctrlplane/Chart.yaml (1 hunks)
🔇 Additional comments (1)
charts/ctrlplane/Chart.yaml (1)

5-5: Verify version bump strategy for the breaking change.

The chart version was bumped from 0.7.1 to 0.8.0 (a MINOR version increment). However, the underlying change—converting the OTEL workload from Deployment to DaemonSet—is a breaking change that fundamentally alters how the OTEL collector is deployed (one instance per node vs. arbitrary replicas). This typically warrants a MAJOR version bump or clear migration documentation.

Consider whether a MAJOR version bump (e.g., 0.9.0 or 1.0.0) is more appropriate, or ensure migration/upgrade guidance is provided to users.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
charts/ctrlplane/charts/otel/templates/daemonset.yaml (1)

7-12: Standardize whitespace in template conditionals.

Line 12 has inconsistent spacing after {{- compared to line 7-8. Helm templates typically omit extra spaces here for consistency.

Apply this diff to align formatting:

-    {{-   toYaml .Values.daemonset.annotations | nindent 4 }}
+    {{- toYaml .Values.daemonset.annotations | nindent 4 }}
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 4d0f795 and a3ba26e.

📒 Files selected for processing (2)
  • charts/ctrlplane/charts/otel/templates/daemonset.yaml (1 hunks)
  • charts/ctrlplane/charts/otel/values.yaml (1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
charts/ctrlplane/charts/otel/templates/daemonset.yaml

[error] 6-6: syntax error: expected the node content, but found '-'

(syntax)


[warning] 12-12: too many spaces after hyphen

(hyphens)

🔇 Additional comments (4)
charts/ctrlplane/charts/otel/templates/daemonset.yaml (3)

1-19: Verify YAMLlint syntax error doesn't cause deployment failures.

Static analysis flagged an error on line 6: "syntax error: expected the node content, but found '-'". This appears to be a false positive from YAMLlint not recognizing Helm template syntax, but please verify the template renders correctly when deployed.

Can you confirm the Helm chart renders without errors using helm template or helm lint on this file?


32-35: Verify tolerations configuration for DaemonSet scheduling.

The template conditionally applies tolerations from .tolerations (which is empty in values.yaml: tolerations: []). For a DaemonSet to run on all nodes, you may need default tolerations for nodes with taints. Confirm that either:

  • Default taints don't exist in the target cluster, or
  • Tolerations will be provided at deploy-time via --set tolerations[0]=...

1-82: Migration to DaemonSet looks correct.

The conversion from Deployment to DaemonSet is properly implemented:

  • kind correctly set to DaemonSet (line 2)
  • replicas field appropriately removed
  • All .Values.deployment.* references migrated to .Values.daemonset.* (lines 7-8, 11-12)
  • Pod spec structure remains compatible
charts/ctrlplane/charts/otel/values.yaml (1)

53-55: DaemonSet values configuration is correct.

The top-level key rename from deployment to daemonset properly aligns with the template updates. The structure preserves labels and annotations for customization, which is appropriate.

@adityachoudhari26 adityachoudhari26 merged commit 94aa010 into main Nov 18, 2025
2 checks passed
@adityachoudhari26 adityachoudhari26 deleted the otel-daemonset branch November 18, 2025 22:08
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