fix(collector): remove prometheus annotations on disablePrometheusAnnotations update#5069
Conversation
…otations update When `spec.observability.metrics.disablePrometheusAnnotations` flipped from false to true on an existing OpenTelemetryCollector, the operator's pod- template mutate path preserved the prometheus.io/scrape, port, and path annotations on the rolled pods. The renderer in `manifestutils.PodAnnotations` correctly omitted those keys from the desired pod template, but `mutatePodTemplate` runs a mergeWithOverride against the existing pod template's annotations — that merge intentionally preserves any key present in existing but absent from desired (the long-standing "external annotations on the pod template should survive a reconcile" contract enforced by the Ingress mutate tests). So the prometheus.io/* keys planted at create-time stuck around forever. Fix: track a small fixed set of operator-managed pod-template annotation keys (currently the three prometheus.io annotations) and strip those keys from existing.Annotations before the merge runs, when they are absent from desired. Keys outside that set continue to be preserved. Adds three table-driven test cases against `mutatePodTemplate`: - disable removes prometheus annotations but keeps user keys (the bug) - enable keeps prometheus annotations when desired sets them (regression) - no operator-managed keys on either side leaves user annotations intact Fixes open-telemetry#5043 Signed-off-by: praveen9354 <praveen9354@gmail.com>
|
This is a little dicey, because these annotations don't belong to the operator, but rather to Prometheus. If someone flips the attribute on and then adds their own, we'd remove them with this change. @open-telemetry/operator-approvers what do you think? We'd need to track the state in some way - for example add another annotation indicating that it was the operator who added these. |
E2E Test Results 36 files 259 suites 3h 52m 59s ⏱️ Results for commit 1ca314b. |
Yep. It makes sense. |
|
@swiatekm @iblancasa — agreed, the unconditional strip is too aggressive. The operator should only remove keys it planted. Plan for v2:
Migration shape:
Tests (
CI:
Will rework on the same branch — likely a force-push since the diff shape changes, unless you would prefer me to stack a second commit so the v1 → v2 evolution is visible in the PR history. Let me know which you prefer. If the meta-annotation key name ( |
Fixes #5043
Description
When
spec.observability.metrics.disablePrometheusAnnotationsflipped fromfalse to true on an existing OpenTelemetryCollector, the operator's pod-
template mutate path preserved the prometheus.io/scrape, port, and path
annotations on the rolled pods.
The renderer in
manifestutils.PodAnnotationscorrectly omitted those keysfrom the desired pod template, but
mutatePodTemplateruns amergeWithOverride against the existing pod template's annotations — that
merge intentionally preserves any key present in existing but absent from
desired (the long-standing "external annotations on the pod template
should survive a reconcile" contract enforced by the Ingress mutate tests).
So the prometheus.io/* keys planted at create-time stuck around forever.
Fix: track a small fixed set of operator-managed pod-template annotation
keys (currently the three prometheus.io annotations) and strip those keys
from existing.Annotations before the merge runs, when they are absent from
desired. Keys outside that set continue to be preserved.
Adds three table-driven test cases against
mutatePodTemplate:Fixes #5043
Signed-off-by: praveen9354 praveen9354@gmail.com
Link to tracking Issue(s)
See description and commits above.
Testing
Local verification of the changed file's adjacent test suite.
Upstream CI runs the full matrix on push.
Documentation