Skip to content
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

Merged
merged 9 commits into from
Jul 18, 2024
Merged

Conversation

frzifus
Copy link
Member

@frzifus frzifus commented Jul 10, 2024

Description:

Link to tracking Issue(s):

Testing:

Documentation:

@frzifus frzifus added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jul 10, 2024
@frzifus frzifus marked this pull request as ready for review July 10, 2024 09:09
@frzifus frzifus requested a review from a team as a code owner July 10, 2024 09:09
@pavolloffay
Copy link
Member

e2e test failed


### Components

* [OpenTelemetry Collector - v0.104.0](https://github.com/open-telemetry/opentelemetry-collector/releases/tag/v0.104.0)
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Contributor

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.

CHANGELOG.md Outdated Show resolved Hide resolved
@frzifus frzifus force-pushed the prepare-0.104.0 branch 2 times, most recently from d7f4215 to a4b6da4 Compare July 11, 2024 13:08
Copy link
Contributor

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@frzifus
Copy link
Member Author

frzifus commented Jul 11, 2024

Copy link
Member

@pavolloffay pavolloffay left a 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.

if g.Object == nil {
g.Object = make(map[string]interface{})
}
g.Object["endpoint"] = "0.0.0.0:4317"
Copy link
Member

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

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@frzifus
Copy link
Member Author

frzifus commented Jul 11, 2024

it would be great to resolve https://issues.redhat.com/browse/TRACING-4458 before the merge.

y, I try to reproduce it already.

@frzifus frzifus force-pushed the prepare-0.104.0 branch 2 times, most recently from 1158f14 to fdf0f4c Compare July 12, 2024 15:15
// 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) {
Copy link
Contributor

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

@frzifus frzifus force-pushed the prepare-0.104.0 branch 6 times, most recently from 01f97ef to d0d4f39 Compare July 13, 2024 12:44
@frzifus
Copy link
Member Author

frzifus commented Jul 13, 2024

I am working on #3124

@frzifus
Copy link
Member Author

frzifus commented Jul 15, 2024

Seems we still have another issue:
https://issues.redhat.com/browse/TRACING-4469

@pavolloffay
Copy link
Member

I booked it here #3133

Copy link
Member

@pavolloffay pavolloffay left a 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:
Copy link
Member

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pavolloffay
Copy link
Member

Is that something we want to do? I thought its important to not break existing setups and change it to 0.0.0.0 or $POD_IP afterwards.

I would prefer if this is done as part of this release, but it depends on the complexity of the PR

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
TAUnifiyEnvVarExpansion(otelcol)
TAUnifyEnvVarExpansion(otelcol)

@@ -79,6 +79,14 @@ func (c CollectorWebhook) Default(_ context.Context, obj runtime.Object) error {
otelcol.Spec.TargetAllocator.Replicas = &one
}

if otelcol.Spec.TargetAllocator.Enabled {
Copy link
Contributor

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

Copy link
Member Author

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?


// DefaultOTLPAddress binds configured otlp receivers to 0.0.0.0 if nothing else
// is explicitly defined.
func DefaultOTLPAddress(otelcol *OpenTelemetryCollector) error {
Copy link
Contributor

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

Copy link
Member Author

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?

Copy link
Contributor

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

Copy link
Member Author

@frzifus frzifus Jul 17, 2024

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.

cc @jaronoff97 @pavolloffay

pkg/collector/upgrade/v0_104_0.go Outdated Show resolved Hide resolved
Comment on lines +93 to +100
{
Version: *semver.MustParse("0.104.0"),
upgradeV1beta1: upgrade0_104_0_TA,
},
{
Version: *semver.MustParse("0.104.0"),
upgradeV1beta1: upgrade0_104_0,
},
Copy link
Contributor

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?

Copy link
Member Author

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 <[email protected]>
Signed-off-by: Benedikt Bongartz <[email protected]>
Signed-off-by: Benedikt Bongartz <[email protected]>
Signed-off-by: Benedikt Bongartz <[email protected]>

fix: config structure

Signed-off-by: Benedikt Bongartz <[email protected]>
Signed-off-by: Benedikt Bongartz <[email protected]>
@frzifus frzifus force-pushed the prepare-0.104.0 branch 8 times, most recently from 53f50d2 to 4c270c4 Compare July 17, 2024 18:58
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@pavolloffay pavolloffay merged commit a9251b3 into open-telemetry:main Jul 18, 2024
33 checks passed
@frzifus frzifus deleted the prepare-0.104.0 branch July 18, 2024 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Release the operator v0.104.0
5 participants