fix(collector): remove prometheus annotations on disablePrometheusAnnotations update

[praveen9354]

Fixes #5043

Description

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 #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

• Yes, PR includes docs updates
• Yes, issue opened
• No

[swiatekm]

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.

[github-actions]

E2E Test Results

 38 files  261 suites   2h 14m 48s ⏱️
110 tests 108 ✅ 2 💤 0 ❌
283 runs  277 ✅ 6 💤 0 ❌

Results for commit 564b45c.

♻️ This comment has been updated with latest results.

[iblancasa]

We'd need to track the state in some way - for example add another annotation indicating that it was the operator who added these.

Yep. It makes sense.

[praveen9354]

@swiatekm @iblancasa — agreed, the unconditional strip is too aggressive. The operator should only remove keys it planted.

Plan for v2:

1. When the operator stamps prometheus.io/{scrape,port,path} onto the pod template, also write a meta-annotation opentelemetry.io/operator-managed-annotations: prometheus.io/scrape,prometheus.io/port,prometheus.io/path recording which keys it owns on that pod template.
2. On reconcile, before the merge, parse the meta-annotation from existing.Annotations into a set, and strip only those keys when they are absent from desired. Update the meta-annotation to reflect the current desired set after the strip.
3. If a user later adds their own prometheus.io/scrape while disablePrometheusAnnotations: true, that key is not in the meta-set, so it survives the merge.

Migration shape:

• Pod templates created before this change have no meta-annotation. On the first reconcile after upgrade, the operator stamps the meta-annotation reflecting the current desired set (so existing operator-planted keys are correctly tracked going forward), but does not strip anything that turn — there is no recorded ownership yet.
• From the second reconcile onward, behavior matches the new contract.

Tests (mutatePodTemplate, table-driven):

• operator-added key + disable flip → key stripped, user keys preserved (the bug).
• operator-added key + enable flip → key kept, meta-annotation updated.
• user-added prometheus.io/scrape while disable=true and no meta → user key preserved (legacy migration).
• user-added prometheus.io/scrape while disable=true with explicit meta → user key preserved, operator-managed keys still stripped.
• no operator-managed keys on either side → user annotations untouched.

CI:

• .chloggen/<filename>.yaml entry added (the current changelog check is failing because this PR has no chloggen file).
• make lint clean.
• e2e instrumentation/targetallocator jobs re-run after the redesign.

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 (opentelemetry.io/operator-managed-annotations) is reserved for something else or you have a preferred namespace key, happy to adjust.

[praveen9354]

Friendly ping @swiatekm @iblancasa when you have a moment. To recap where we left off:

You both flagged that the current PR's unconditional strip of prometheus.io/{scrape,port,path} is too aggressive — if a user later adds their own copy of those annotations while disablePrometheusAnnotations: true, the operator would silently remove them on the next reconcile.

My proposed redesign (posted 2026-05-13):

1. When the operator stamps prometheus.io/* annotations, also write a meta-annotation opentelemetry.io/operator-managed-annotations listing which keys it owns on this pod template.
2. On reconcile, parse the meta-annotation from existing.Annotations and strip only those keys when they're absent from desired.
3. User-added prometheus.io/scrape survives because it isn't in the meta-list.
4. Legacy pod templates (no meta-annotation yet) get the meta stamped on first reconcile after upgrade without stripping anything — self-healing migration.

Pending your ack on:

• Whether the meta-annotation key name (opentelemetry.io/operator-managed-annotations) is acceptable or you'd prefer a different namespace key.
• Whether you'd prefer a force-push on the same branch (clean single-commit diff) or a stacked second commit so v1 → v2 history is visible in the PR.

I'll start the rework as soon as I have either of those — happy to take whatever direction works best.

[swiatekm]

This might be a pattern we adopt for other similar situations around reconciliation, so we'll have to discuss this during our SIG meeting on Thursday (see https://github.com/open-telemetry/community#sig-operator if you'd like to join). We'll get back to you with a direction afterwards. Sorry for the wait!

[praveen9354]

@swiatekm @iblancasa gentle ping — has the operator-tracked-annotations approach we discussed on 2026-05-13 (track the keys the operator stamped so we only remove those, rather than the unconditional strip) gotten any SIG-meeting time since 2026-05-18? Happy to send a v2 implementing the "only remove keys we planted" pattern whenever you have direction. Thanks!

[praveen9354]

Rebased onto current main to clear CI drift:

• changelog (chloggen): quoted the note: value so YAML parses the inline disablePrometheusAnnotations: true colon as text rather than as a mapping key
• gci linting: re-aligned the const block in the new test (TestMutatePodTemplateStripsOperatorManagedAnnotations) to gci's expected formatting
• Govulncheck (GO-2026-5026): cleared by upstream's recent golang.org/x/net bump (now at v0.55.0 on main), no dep change needed from my side

No production code changes — internal/manifests/mutate.go is the same fix you reviewed earlier. Still waiting on the SIG-meeting outcome for the operator-tracked-annotations approach we discussed on 2026-05-13; happy to revise to that pattern once direction is set.

[swiatekm]

@swiatekm @iblancasa gentle ping — has the operator-tracked-annotations approach we discussed on 2026-05-13 (track the keys the operator stamped so we only remove those, rather than the unconditional strip) gotten any SIG-meeting time since 2026-05-18? Happy to send a v2 implementing the "only remove keys we planted" pattern whenever you have direction. Thanks!

Hey, sorry for making you wait. We've discussed it during the SIG meeting on 04.06.2026 and decided that we're ok with the tracked annotation approach. Essentially, we'd add our own annotation (let's say it's going to be opentelemetry.io/operator/prometheus-annotations-added, but it's to be decided) when we add these prometheus annotations, and only delete them if that annotation is present.

[praveen9354]

Thanks for the update @swiatekm — and no apology needed, this kind of cross-cutting reconciliation question is exactly what SIG meetings are for.

Confirming the design I'll send in v2:

• On reconcile, when the operator adds the prometheus.io annotations (scrape, port, path) to the pod template, also add a marker annotation opentelemetry.io/operator/prometheus-annotations-added: "true" (final name TBD per your message — happy to align on what you'd like once I see whether true / a hash / a generation marker makes the most sense for reconcile idempotency).
• On update, when disablePrometheusAnnotations: true, the mutate path strips the prometheus.io annotations only if the marker is present. The marker itself is also removed in the same step so the operator's claim is cleaned up.
• Backward compatibility: existing collectors on operator-upgrade won't have the marker. The marker gets added on the first reconcile after upgrade, so the conditional strip path works on subsequent toggles. The first toggle after upgrade is the only one where the existing prometheus annotations stay (which I think matches what you'd want — the operator can't claim ownership of annotations it didn't add).
• Tests: the existing regression test will be reworked to exercise both paths (marker present → strip happens; marker absent → strip skipped). I'll also add a test covering the upgrade scenario.

I'll send v2 shortly. One small ask: any preference between opentelemetry.io/operator/prometheus-annotations-added (your suggestion), something shorter like operator.opentelemetry.io/prom-annotations, or pegging it to a more general "operator-managed-annotations" name in case future operator-managed annotation classes want to share the mechanism? I'll go with your verbatim suggestion unless you'd rather pick a different shape.

[swiatekm]

@praveen9354 your design looks good to me. Let's start with opentelemetry.io/operator/prometheus-annotations-added - we can argue about the exact name later, having reviewed the mechanism.

[praveen9354]

Force-pushed b534908a implementing the marker-gated design from the 2026-06-04 SIG meeting.

One naming change. The literal name you suggested — opentelemetry.io/operator/prometheus-annotations-added — doesn't validate as a Kubernetes annotation key (the API only allows one /, splitting DNS prefix from suffix). Reconciler envtest surfaced this as Invalid value: "opentelemetry.io/operator/...": a valid label key must consist of.... I renamed to collector.opentelemetry.io/prometheus-annotations-added, matching the existing <subsystem>.opentelemetry.io/<key> shape used elsewhere in the operator (e.g. clusterobservability.opentelemetry.io/finalizer, instrumentation.opentelemetry.io/container-names). Happy to bikeshed further if you'd prefer a different shape — the design is name-agnostic and the constant lives in one place.

Key shape:

• manifestutils.PrometheusAnnotationsAddedKey (the marker) is stamped by PodAnnotations only when the operator actually adds at least one default prometheus.io/* annotation. If the user pre-set all three on metadata.annotations or spec.podAnnotations, the operator added nothing on top and the marker is not stamped — it claims nothing.
• mutatePodTemplate strips prometheus.io/{scrape,port,path} + the marker only when the marker is present on the existing pod template AND the keys are absent from the desired pod template.

Upgrade compatibility (the piece worth flagging):

• Pre-upgrade collectors have no marker. On the first reconcile after upgrade with DisablePrometheusAnnotations=false, the marker is stamped and subsequent toggles work normally.
• If a user upgrades and immediately sets DisablePrometheusAnnotations=true, the operator never stamps the marker on the existing pod template (the stamp path is gated on !Disable), so the pre-upgrade prom annotations survive that single toggle. Documented in the test as the one-time cost — the alternative was a pre-marker one-shot strip that would re-introduce the v1 "operator clobbers user's prom annotations" risk.

Tests:

• TestMutatePodTemplateStripsOperatorStampedPrometheusAnnotations covers the 5 mutate-path scenarios (marker present + disable → strip; marker absent → preserve; enable path → no-op; upgrade-compat → marker gets stamped; upgrade + immediate disable → preserved as one-time cost).
• TestDefaultAnnotations, TestNonDefaultPodAnnotation, TestUserAnnotations updated with marker presence/absence assertions. The TestUserAnnotations case (user pre-set all three) explicitly asserts the marker is not stamped — the operator claims nothing.
• Collector deployment/daemonset/statefulset render tests and the internal/controllers/testdata/build_collector_*.yaml fixtures updated for the new annotation in the rendered output.

[codecov]

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
101	1	100	2

View the top 1 failed test(s) by shortest run time

::privilege-escalation-delta

Stack Traces | 0.364s run time

($error != null): Invalid value: true: Expected value: false

To view more test analytics, go to the Test Analytics Dashboard
_{📋 Got 3 mins? Take this short survey to help us improve Test Analytics.}

[swiatekm]

This looks good to me now, thanks for making the changes. I looked into what annotations we use for tracking similar changes for Services, and I think what we should use here is operator.opentelemetry.io/prometheus-annotations-added. @open-telemetry/operator-approvers let me know if you disagree.

[praveen9354]

Force-pushed 4bdcd203 with the rename to operator.opentelemetry.io/prometheus-annotations-added. Confirmed locally that the prefix matches the existing service-tracking annotations in this repo (operator.opentelemetry.io/collector-headless-service, collector-monitoring-service, collector-service-type).

Single-commit rebase on top of b534908a, no logic changes — touches the constant in manifestutils/annotations.go, the 3 collector render tests, the 5 internal/controllers/testdata/build_collector_*.yaml golden fixtures (key sorts after opentelemetry-operator-config/sha256 now), and the chloggen subtext.

[praveen9354]

@iblancasa — friendly nudge for a second approval slot when you have bandwidth. You agreed with the marker-annotation direction on 2026-05-12, and @swiatekm has now approved v3 (4bdcd203), which renames the marker to operator.opentelemetry.io/prometheus-annotations-added to match the prefix used by other Service-tracking annotations in the repo. PR is MERGEABLE; all 56 checks SUCCESS + 1 NEUTRAL (Scorecard). Happy to address anything else.

[praveen9354]

Gentle nudge on the second approval, @iblancasa @jaronoff97 — @swiatekm approved 2026-06-23 and it's been waiting since. Happy to rebase or address any additional feedback.