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

add public controller interface #763

Closed

Conversation

joejulian
Copy link

@joejulian joejulian commented Sep 5, 2023

#653 moved the helmrelease controller to internal. We built our own operator around this controller. This adds a stable public wrapper around the internal controllers.

Applies to #699, redpanda-data/redpanda#13088

@joejulian joejulian force-pushed the add_public_controller_interface branch from 71c17b3 to 8d2c303 Compare September 5, 2023 21:19
@stefanprodan stefanprodan added the area/api API related issues and pull requests label Sep 8, 2023
@stefanprodan stefanprodan requested a review from a team September 8, 2023 20:28
Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

Besides my comment, I would prefer the HelmReleaseReconcilerFactory to contain a comment about usage not being advised nor supported by the project itself, and it being available on a "best-effort basis".

More specifically because it can result in the consumer their controller fighting a helm-controller instance if e.g. sharding (or namespace selection) is not properly configured (or available).

In addition to issues around the life-cycle of Custom Resource Definitions, and most likely any other hidden surprise which can arise when a custom built controller and a helm-controller are deployed in the same cluster.

controller/helmrelease_controller.go Outdated Show resolved Hide resolved
@joejulian
Copy link
Author

More specifically because it can result in the consumer their controller fighting a helm-controller instance if e.g. sharding (or namespace selection) is not properly configured (or available).

In addition to issues around the life-cycle of Custom Resource Definitions, and most likely any other hidden surprise which can arise when a custom built controller and a helm-controller are deployed in the same cluster.

The contention can be handled by not reconciling resources that have an owner reference by default. When our controller creates the resources, it'll set the owner reference to the (in our case) Redpanda resource. If a helmrelease, for example, is created in any normal fashion, it won't have an owner reference. #769

Please comment on that issue if you think this would be an acceptable approach and I can (or I can get someone to) work on it.

@joejulian joejulian force-pushed the add_public_controller_interface branch from 8d2c303 to 1802175 Compare September 11, 2023 15:55
Signed-off-by: Joe Julian <me@joejulian.name>
@joejulian joejulian force-pushed the add_public_controller_interface branch from 1802175 to 92c3c06 Compare September 11, 2023 15:56
@stefanprodan
Copy link
Member

I don’t think we should allow others to reconcile helm.toolkit.fluxcd.io resources. Redpanda should use their own API group. By allowing this, we undermine the FluxCD users, on clusters with the redpanda controller they can’t use Flux, which feels very wrong to me.

@joejulian
Copy link
Author

We already have users that use both.

@joejulian joejulian closed this Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api API related issues and pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants