open-telemetry
Remove unnecessary instances of app.kubernetes.io/managed-by
Description:
This PR resolves a bug where we applying the app.kubernetes.io/managed-by to resources unnecessarily. We also relied on this label to be present which caused a nasty bug with instrumentation. Because we expect this to be on all CRDs and we use that for querying for upgrades, when we go to query for instrumentations, any that were applied by helm (which sets this label itself) would be omitted, causing them to fail to be upgraded. This change is technically breaking, but is also a bug fix because we are no longer failing the user expectation for auto instrumentation fixes.
Link to tracking Issue(s): <Issue number if applicable>
- Resolves: #3014
Testing: Unit, manual
Documentation: <Describe the documentation added.>
@swiatekm-sumo i think if we were going the other way (beginning to filter) that would be problematic, but this should encapsulate all collector CRs now, not just the ones created without helm. I will test this prior to merging.
Hey folks, are there plans to merge this? Someone asked me about this one today, they would benefit from this fix.
@jpkrohling i still need to do some testing on this. I want to be sure we're not going to break existing installations more than we need to. I'll see if I can prioritize that for this week, but I'm juggling a few responsibilities currently.
I want to be sure we're not going to break existing installations
@jaronoff97 Release https://github.com/open-telemetry/opentelemetry-operator/releases/tag/v0.107.0 has already broken existing installations, since upgrade 105 is not being applied due to this bug. If the feature flag that was removed in the collector is present in the custom resource, then the collector pod will begin crashing due to invalid configuration. A workaround is to edit the CR to remove the offending flag, though this is tedious if you manage many collectors.
@kennytrytek-wf thanks for the report. I will make this my priority and this will block our next release. Sincere apologies for the tediousness there.
At this point I think we should either split the upgrade fix into its own PR or add another changelog entry. What we're doing here is way beyond just removing the labels.
Thank you for the quick turnaround on getting this merged and released!