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

feat/add shareProcessName capability #2472

Merged
merged 1 commit into from
Jan 18, 2024
Merged

feat/add shareProcessName capability #2472

merged 1 commit into from
Jan 18, 2024

Conversation

bmiguel-teixeira
Copy link
Contributor

Description:
Adding an additional field to the OpenTelemetryCollector CRD called shareProcessNamespace that essentially toggles the equivalent field in the PodSpec of the generated resources (DS, Deployment or STS).

No issue opened on my side.

The reason for this addition in to allow configuration reloads. The OtelCollectors allow config reload via SIGHUP, but there is currently no way to this via the additionalContainers field without this capability.

Testing:

Unit tests added.

Documentation:

@bmiguel-teixeira bmiguel-teixeira requested a review from a team December 21, 2023 22:24
@swiatekm
Copy link
Contributor

What do you need the SIGHUP reload for, if I may ask? I'm not opposed to adding this attribute, but reloading config via a signal seems like an antipattern in Kubernetes. We already restart collector Pods whenever configuration changes,

Copy link

linux-foundation-easycla bot commented Dec 24, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: bmiguel-teixeira / name: Bruno Teixeira (94dd491)

@bmiguel-teixeira
Copy link
Contributor Author

bmiguel-teixeira commented Dec 24, 2023

Hi @swiatekm-sumo

While the bulk of the configuration is indeed static and managed by the controller, a small section of it, the filter section,

  processors:
    filter/md: ${file:/var/conf/extraconf/dynamic.yaml}

is dynamic in our use case and usually varies from cluster to cluster.

P.S: pushed the missing manifests change.

Happy holidays.

@bmiguel-teixeira
Copy link
Contributor Author

Hi @swiatekm-sumo

Pushed missing changelog file. Any comments/feedback on this?

Ty

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.

Could you also set this in one of the E2E tests?

apis/v1alpha1/opentelemetrycollector_types.go Outdated Show resolved Hide resolved
@swiatekm swiatekm self-requested a review January 14, 2024 12:27
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.

@bmiguel-teixeira you've got some failures in the builder unit tests. Other than that, I think we're good to go.

@bmiguel-teixeira
Copy link
Contributor Author

hi @swiatekm-sumo

Fixed unit tests. Unit and e2e all good locally. ty

@swiatekm
Copy link
Contributor

@bmiguel-teixeira you still have a minor lint issue.

@bmiguel-teixeira
Copy link
Contributor Author

@swiatekm-sumo done. 🤞

@swiatekm swiatekm self-requested a review January 18, 2024 13:06
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, thanks for your patience!

@jaronoff97 we probably want to add this attribute to common fields as well.

@jaronoff97
Copy link
Contributor

yep, i'll open a follow up issue for that, no need to further block this.

@jaronoff97 jaronoff97 merged commit e2773be into open-telemetry:main Jan 18, 2024
27 checks passed
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants