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

Generate only TargetAllocator CR from Collector CR #3402

Merged
merged 1 commit into from
Oct 30, 2024

Conversation

swiatekm
Copy link
Contributor

@swiatekm swiatekm commented Oct 28, 2024

Description:

Currently when the target allocator is enabled on the collector CR, the collector reconciler generates the target allocator manifests directly. This PR introduces a feature flag that causes it to generate a Target Allocator CR instead, and lets the Target Allocator reconciler take care of the rest.

All the changes are gated behind the flag, including actually enabling the TA controller and webhook. Users who don't set the flag are completely unaffected.

I'm not adding a changelog entry for this, because it's only really meant for testing. I'll add the entry alongside the documentation when we actually enable the controller and webhook unconditionally.

Link to tracking Issue(s):

Testing:
Added:

  • new unit tests
  • new builder tests for cases we're already checking
  • controller tests using envtest
  • run e2e tests with the flag enabled

@swiatekm swiatekm added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Oct 28, 2024
@swiatekm swiatekm force-pushed the feat/ta-crd-feature-flag branch from e342ad5 to e92527f Compare October 28, 2024 18:03
@swiatekm swiatekm marked this pull request as ready for review October 28, 2024 18:19
@swiatekm swiatekm requested a review from a team as a code owner October 28, 2024 18:19
@swiatekm swiatekm force-pushed the feat/ta-crd-feature-flag branch from e92527f to c1e6ecd Compare October 28, 2024 18:19
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.

nothing blocking, just a few minor questions/suggestions

controllers/targetallocator_controller.go Show resolved Hide resolved
Comment on lines +184 to +187
// watch collectors which have embedded Target Allocator enabled
// we need to do this separately from collector reconciliation, as changes to Config will not lead to changes
// in the TargetAllocator CR
ctrlBuilder.Watches(
Copy link
Contributor

Choose a reason for hiding this comment

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

didn't realize the controller has this capability, very cool :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is only implemented in software here, so we get every update, and then the ones where the condition isn't true are discarded. But that's not really a problem in this case.

internal/manifests/mutate.go Show resolved Hide resolved
controllers/common.go Show resolved Hide resolved
@swiatekm swiatekm force-pushed the feat/ta-crd-feature-flag branch from c1e6ecd to 2fc677d Compare October 28, 2024 18:56
@swiatekm swiatekm requested a review from pavolloffay October 28, 2024 18:59
This is hidden behind a feature flag. Nothing changes by default.
@swiatekm swiatekm force-pushed the feat/ta-crd-feature-flag branch from 2fc677d to d2d4ed4 Compare October 30, 2024 17:23
@swiatekm swiatekm merged commit 49ca805 into open-telemetry:main Oct 30, 2024
36 checks passed
@swiatekm swiatekm deleted the feat/ta-crd-feature-flag branch October 30, 2024 17:56
araiu added a commit to araiu/opentelemetry-helm-charts that referenced this pull request Dec 5, 2024
* `secrets`:  Add TLS support to auto-instrumentation  [#3338](open-telemetry/opentelemetry-operator#3338)
* `targetallocators`: Generate only TargetAllocator CR from Collector CR [#3402](open-telemetry/opentelemetry-operator#3402)
araiu added a commit to araiu/opentelemetry-helm-charts that referenced this pull request Dec 5, 2024
* `secrets`:  Add TLS support to auto-instrumentation  [#3338](open-telemetry/opentelemetry-operator#3338)
* `targetallocators`: Generate only TargetAllocator CR from Collector CR [#3402](open-telemetry/opentelemetry-operator#3402)
araiu added a commit to araiu/opentelemetry-helm-charts that referenced this pull request Dec 5, 2024
* `secrets`:  Add TLS support to auto-instrumentation  [#3338](open-telemetry/opentelemetry-operator#3338)
* `targetallocators`: Generate only TargetAllocator CR from Collector CR [#3402](open-telemetry/opentelemetry-operator#3402)
araiu added a commit to araiu/opentelemetry-helm-charts that referenced this pull request Dec 6, 2024
* `secrets`:  Add TLS support to auto-instrumentation  [#3338](open-telemetry/opentelemetry-operator#3338)
* `targetallocators`: Generate only TargetAllocator CR from Collector CR [#3402](open-telemetry/opentelemetry-operator#3402)
TylerHelmuth pushed a commit to open-telemetry/opentelemetry-helm-charts that referenced this pull request Dec 6, 2024
* Release operator 0.114.1

Signed-off-by: Alex Raiu <raiu.andrei@protonmail.com>

* Update operator clusterrole

* `secrets`:  Add TLS support to auto-instrumentation  [#3338](open-telemetry/opentelemetry-operator#3338)
* `targetallocators`: Generate only TargetAllocator CR from Collector CR [#3402](open-telemetry/opentelemetry-operator#3402)

* Add `targetAllocatorFallbackStrategy` feature flag

Feature flag available since `v0.114.0` (Add allocation_fallback_strategy option as fallback strategy for per-node strategy [#3482](open-telemetry/opentelemetry-operator#3482))

* Update operator-test with manager label

Adding `control-plane: controller-manager` label to match [operator-restart e2e test](open-telemetry/opentelemetry-operator#3486)

---------

Signed-off-by: Alex Raiu <raiu.andrei@protonmail.com>
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.

2 participants