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

Enable controller and webhook for TargetAllocator CR #3426

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

swiatekm
Copy link
Contributor

@swiatekm swiatekm commented Nov 6, 2024

Description:

Enable the TargetAllocator CRD. This is really just a bunch of bookkeeping manifest changes, plus some new E2E tests. Note that whether the Collector subresource uses the new CR is still controlled by an alpha feature gate.

I'll clean up and consolidate the tests after this is merged.

Link to tracking Issue(s):

Testing:

  • Added new e2e tests

Documentation:

  • Add documentation

@swiatekm swiatekm added discuss-at-sig This issue or PR should be discussed at the next SIG meeting and removed discuss-at-sig This issue or PR should be discussed at the next SIG meeting labels Nov 7, 2024
@swiatekm swiatekm marked this pull request as ready for review November 13, 2024 15:42
@swiatekm swiatekm requested a review from a team as a code owner November 13, 2024 15:42
Copy link
Contributor

@jaronoff97 jaronoff97 left a comment

Choose a reason for hiding this comment

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

minor docs comments, otherwise this looks good!

controllers/targetallocator_controller.go Outdated Show resolved Hide resolved
controllers/targetallocator_controller.go Outdated Show resolved Hide resolved
controllers/targetallocator_controller.go Outdated Show resolved Hide resolved
swiatekm and others added 4 commits December 19, 2024 12:47
# Conflicts:
#	bundle/community/manifests/opentelemetry-operator.clusterserviceversion.yaml
#	bundle/openshift/manifests/opentelemetry-operator.clusterserviceversion.yaml

# Conflicts:
#	bundle/community/manifests/opentelemetry-operator.clusterserviceversion.yaml
#	bundle/openshift/manifests/opentelemetry-operator.clusterserviceversion.yaml
Copy link
Contributor

@iblancasa iblancasa left a comment

Choose a reason for hiding this comment

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

Great job!

@@ -122,6 +123,51 @@ In essence, Prometheus Receiver configs are overridden with a `http_sd_config` d
Allocator, these are then loadbalanced/sharded to the Collectors. The [Prometheus Receiver](https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/receiver/prometheusreceiver/README.md) configs that are overridden
are what will be distributed with the same name.

## TargetAllocator CRD
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: how about documenting the operator.collector.targetallocatorcr feature flag and explain what is the effect regarding this CRD?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure that's helpful at this point. That flag controls how the collector CR is reconciled, it doesn't have anything to do with the TargetAllocator CR per se. I introduced it specifically so that this PR doesn't change any existing code paths.

@iblancasa
Copy link
Contributor

@IshwarKanse maybe you can provide some ideas about the testing for this?

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.

Introduce TargetAllocator CRD
3 participants