-
Notifications
You must be signed in to change notification settings - Fork 438
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prepare release 0.104.0 #3119
Prepare release 0.104.0 #3119
Conversation
e2e test failed |
|
||
### Components | ||
|
||
* [OpenTelemetry Collector - v0.104.0](https://github.com/open-telemetry/opentelemetry-collector/releases/tag/v0.104.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/open-telemetry/opentelemetry-collector/releases/tag/v0.104.0
otlpreceiver: Switch to localhost as the default for all endpoints. (open-telemetry/opentelemetry-collector#8510)
We should at least change docs and think about handling this via upgrade or when we generate config map for the collector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you recommend? In case its not specified we switch it explicitly to 0.0.0.0
or set the $POD_IP
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say 0.0.0.0
, as this was the default up until now.
d7f4215
to
a4b6da4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Some e2e tests still fail, I will check this in a bit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be great to resolve https://issues.redhat.com/browse/TRACING-4458 before the merge.
pkg/collector/upgrade/v0_104_0.go
Outdated
if g.Object == nil { | ||
g.Object = make(map[string]interface{}) | ||
} | ||
g.Object["endpoint"] = "0.0.0.0:4317" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is handling the upgrade, but can you please change examples and readmes to ensure newly crated CRs define endpoint correctly?
https://github.com/open-telemetry/opentelemetry-operator
https://github.com/open-telemetry/opentelemetry-operator/tree/main/config/samples
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also think about implementing some validation/defaulting for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also think about implementing some validation/defaulting for it.
Is it a behaviour we want to keep in the long run? If so, I would recommend to set the pod IP instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
y, I try to reproduce it already. |
1158f14
to
fdf0f4c
Compare
apis/v1beta1/collector_webhook.go
Outdated
// TAUnifiyEnvVarExpansion enables confmap.unifyEnvVarExpansion featuregate on collector instances associated to TA.. | ||
// NOTE: We need this for now until 0.105.0 is out with this fix: | ||
// https://github.com/open-telemetry/opentelemetry-collector/commit/637b1f42fcb7cbb7ef8a50dcf41d0a089623a8b7 | ||
func TAUnifiyEnvVarExpansion(otelcol *OpenTelemetryCollector) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we can also be clever about this and only add it if in the config we find a $$1
instance, but this is fine otherwise...
01f97ef
to
d0d4f39
Compare
I am working on #3124 |
Seems we still have another issue: |
I booked it here #3133 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/open-telemetry/opentelemetry-operator/tree/main/config/samples should be updated as well to include the host if it is mandatory.
@@ -449,3 +453,26 @@ func SetupCollectorWebhook(mgr ctrl.Manager, cfg config.Config, reviewer *rbac.R | |||
WithDefaulter(cvw). | |||
Complete() | |||
} | |||
|
|||
// TAUnifiyEnvVarExpansion enables confmap.unifyEnvVarExpansion featuregate on collector instances associated to TA.. | |||
// NOTE: We need this for now until 0.105.0 is out with this fix: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please book a ticket to remove this and add a link to it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer if this is done as part of this release, but it depends on the complexity of the PR |
apis/v1beta1/collector_webhook.go
Outdated
@@ -79,6 +79,14 @@ func (c CollectorWebhook) Default(_ context.Context, obj runtime.Object) error { | |||
otelcol.Spec.TargetAllocator.Replicas = &one | |||
} | |||
|
|||
if otelcol.Spec.TargetAllocator.Enabled { | |||
TAUnifiyEnvVarExpansion(otelcol) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TAUnifiyEnvVarExpansion(otelcol) | |
TAUnifyEnvVarExpansion(otelcol) |
apis/v1beta1/collector_webhook.go
Outdated
@@ -79,6 +79,14 @@ func (c CollectorWebhook) Default(_ context.Context, obj runtime.Object) error { | |||
otelcol.Spec.TargetAllocator.Replicas = &one | |||
} | |||
|
|||
if otelcol.Spec.TargetAllocator.Enabled { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we want to run this for anything with prometheus, not just TA
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So instead we would check if any prometheus receiver has been configured?
apis/v1beta1/collector_webhook.go
Outdated
|
||
// DefaultOTLPAddress binds configured otlp receivers to 0.0.0.0 if nothing else | ||
// is explicitly defined. | ||
func DefaultOTLPAddress(otelcol *OpenTelemetryCollector) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we should just disable -component.UseLocalHostAsDefaultHost instead of this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But in the long run we have to change this anyway, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but i think it's a bit cleaner to just use the flag given that's the approach for the other breaking change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FTR: we discussed on slack to enable the fg for now and do the PR to bind listeners to 0.0.0.0 or $POD_IP in an extra PR.
{ | ||
Version: *semver.MustParse("0.104.0"), | ||
upgradeV1beta1: upgrade0_104_0_TA, | ||
}, | ||
{ | ||
Version: *semver.MustParse("0.104.0"), | ||
upgradeV1beta1: upgrade0_104_0, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it matter if there are two entries for the same version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats not an issue. :)
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de> fix: config structure Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
53f50d2
to
4c270c4
Compare
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
Description:
Link to tracking Issue(s):
Testing:
Documentation: