fix: use otel selector labels instead of regular labels for proper trace wiring#40
Conversation
adityachoudhari26
requested review from
jsbroks and
zacharyblasczyk
as code owners
📝 WalkthroughWalkthroughThe pull request updates the OTEL subchart to standardize Kubernetes resource selectors across DaemonSet, Deployment, and Service templates. Instead of combining ChangesOTEL Selector Label Standardization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
charts/ctrlplane/tests/otel_selector_test.yaml (1)
1-55: 💤 Low valueTest suite looks solid — one minor nit on
configmap.yamlinclusion.The structure is correct:
equalonspec.selector.matchLabels/spec.selectorcatches exact label drift, the pod-template test guards against accidentally removing version labels from pod metadata, and the deployment test correctly gates onotel.workload.kind: Deploymentto satisfy the template condition.The only nit:
configmap.yaml(line 6) is listed in suite-leveltemplatesbut is never the target of any test case. If it's only there to satisfy rendering dependencies, it's harmless, but if it's unused, it adds noise.♻️ Remove if no rendering dependency requires it
templates: - charts/otel/templates/daemonset.yaml - charts/otel/templates/deployment.yaml - charts/otel/templates/service.yaml - - charts/otel/templates/configmap.yaml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/ctrlplane/tests/otel_selector_test.yaml` around lines 1 - 55, The tests include "configmap.yaml" in the suite-level templates list but no test targets it; either remove "configmap.yaml" from the suite templates to avoid noise (if it was only included for rendering and isn’t required), or add a focused test case that targets the configmap template (e.g., an asserts block validating expected keys/values) so the inclusion is purposeful; locate the templates array in otel_selector_test.yaml and update it or add a new test entry referencing configmap.yaml accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/ctrlplane/charts/otel/templates/daemonset.yaml`:
- Around line 16-18: The DaemonSet's spec.selector.matchLabels has been changed
(the selector is rendered via the template include "otel.selectorLabels"), but
spec.selector is immutable after creation which will cause helm upgrade
failures; update release notes/NOTES.txt and CHANGELOG to clearly document this
breaking change and required pre-upgrade steps (e.g., run helm upgrade --force
or kubectl delete daemonset <name> --cascade=orphan) and include the exact error
message example; additionally, avoid changing the selector in future releases by
keeping the include "otel.selectorLabels" output stable or bumping chart major
version if selector must change.
In `@charts/ctrlplane/charts/otel/templates/deployment.yaml`:
- Around line 23-25: Deployment spec.selector.matchLabels is immutable and
changing the labels generated by the otel.selectorLabels template will break
upgrades; ensure the chart does not alter existing selector labels for resources
of kind Deployment (keep the output of include "otel.selectorLabels" unchanged
between releases) and, if a selector change is unavoidable, add explicit upgrade
notes documenting the required pre-upgrade mitigation (use --force or delete the
Deployment manually) so operators know to perform the manual migration before
upgrading.
---
Nitpick comments:
In `@charts/ctrlplane/tests/otel_selector_test.yaml`:
- Around line 1-55: The tests include "configmap.yaml" in the suite-level
templates list but no test targets it; either remove "configmap.yaml" from the
suite templates to avoid noise (if it was only included for rendering and isn’t
required), or add a focused test case that targets the configmap template (e.g.,
an asserts block validating expected keys/values) so the inclusion is
purposeful; locate the templates array in otel_selector_test.yaml and update it
or add a new test entry referencing configmap.yaml accordingly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: bb817edd-29f2-4e23-b585-bf3661fb49ab
⛔ Files ignored due to path filters (1)
charts/ctrlplane/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
charts/ctrlplane/Chart.yamlcharts/ctrlplane/charts/otel/Chart.yamlcharts/ctrlplane/charts/otel/templates/daemonset.yamlcharts/ctrlplane/charts/otel/templates/deployment.yamlcharts/ctrlplane/charts/otel/templates/service.yamlcharts/ctrlplane/tests/otel_selector_test.yaml
| selector: | ||
| matchLabels: | ||
| {{- include "ctrlplane.selectorLabels" $ | nindent 6 }} | ||
| {{- include "otel.labels" . | nindent 6 }} | ||
| {{- include "otel.selectorLabels" . | nindent 6 }} |
There was a problem hiding this comment.
Breaking upgrade: spec.selector.matchLabels is immutable on DaemonSet.
spec.selector is an immutable field on Kubernetes DaemonSets (and Deployments) after initial creation. A helm upgrade from the previous chart version will fail with:
The DaemonSet "<name>" is invalid: spec.selector: Invalid value: ... field is immutable
This affects any cluster currently running the prior chart version. Required pre-upgrade action (one of):
helm upgrade --force— triggers delete + re-create (causes brief downtime/pod disruption).- Manual deletion —
kubectl delete daemonset <name> --cascade=orphanbefore runninghelm upgrade.
This should be called out in NOTES.txt, a CHANGELOG, or upgrade documentation to prevent operators from being caught off-guard.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/ctrlplane/charts/otel/templates/daemonset.yaml` around lines 16 - 18,
The DaemonSet's spec.selector.matchLabels has been changed (the selector is
rendered via the template include "otel.selectorLabels"), but spec.selector is
immutable after creation which will cause helm upgrade failures; update release
notes/NOTES.txt and CHANGELOG to clearly document this breaking change and
required pre-upgrade steps (e.g., run helm upgrade --force or kubectl delete
daemonset <name> --cascade=orphan) and include the exact error message example;
additionally, avoid changing the selector in future releases by keeping the
include "otel.selectorLabels" output stable or bumping chart major version if
selector must change.
| selector: | ||
| matchLabels: | ||
| {{- include "ctrlplane.selectorLabels" $ | nindent 6 }} | ||
| {{- include "otel.labels" . | nindent 6 }} | ||
| {{- include "otel.selectorLabels" . | nindent 6 }} |
There was a problem hiding this comment.
Same immutable selector breaking change applies to Deployment.
spec.selector.matchLabels is immutable on Deployments post-creation. Clusters using workload.kind: Deployment will experience the same upgrade failure described for the DaemonSet. The same pre-upgrade mitigation applies (--force or manual deletion), and both cases should be covered in upgrade notes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/ctrlplane/charts/otel/templates/deployment.yaml` around lines 23 - 25,
Deployment spec.selector.matchLabels is immutable and changing the labels
generated by the otel.selectorLabels template will break upgrades; ensure the
chart does not alter existing selector labels for resources of kind Deployment
(keep the output of include "otel.selectorLabels" unchanged between releases)
and, if a selector change is unavoidable, add explicit upgrade notes documenting
the required pre-upgrade mitigation (use --force or delete the Deployment
manually) so operators know to perform the manual migration before upgrading.
2 participants
Summary by CodeRabbit
Version Updates
Improvements
Tests