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.

Sign up for GitHub

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

Jump to bottom

Add an argument for disabling unused controllers #621

Merged
merged 11 commits into from
Mar 25, 2023
Merged

Add an argument for disabling unused controllers #621

merged 11 commits into from
Mar 25, 2023

Conversation

Kristian-ZH
Copy link
Member

@Kristian-ZH Kristian-ZH commented Mar 17, 2023 •
edited
Loading

What this PR does / why we need it:

This PR adds a new flag called disable-component-controllers which disabled the given component's controllers
The possible values are: fluent-bit and fluentd
The default value of the argument will be an empty string which will not break the current work of the operator and will not break its consumers.

The benefit which this PR brings is that the operator's consumers will no any more have to install CRDs and expand the operator's RBAC with configurations for a component that they do not use in their setups.

Which issue(s) this PR fixes:

Fixes #612

Does this PR introduced a user-facing change?

A new argument called disable-component-controllers is added which can turn off the unused component's controllers

Add an argument for disabling unused controllers
69ed018 
Signed-off-by: Kristian-ZH <k.zhelyazkov@sap.com>
@Kristian-ZH Kristian-ZH force-pushed the disable-component-controllers branch from 8b56443 to 69ed018 Compare March 17, 2023 11:07
wenchajun
wenchajun previously approved these changes Mar 19, 2023
View reviewed changes
@benjaminhuo benjaminhuo requested a review from wanjunlei March 21, 2023 15:22
@wanjunlei
Copy link
Collaborator

It should not add the fluentbit CRDs to scheme when fluent-bit is disabled.

https://github.com/fluent/fluent-operator/blob/master/cmd/fluent-manager/main.go#L53

benjaminhuo
benjaminhuo requested changes Mar 22, 2023
View reviewed changes
@benjaminhuo
Copy link
Member

benjaminhuo commented Mar 22, 2023 •
edited
Loading

It should not add the fluentbit CRDs to scheme when fluent-bit is disabled.

https://github.com/fluent/fluent-operator/blob/master/cmd/fluent-manager/main.go#L53

@wanjunlei
The init runs before the main function in which the disable flag is parsed, so how to do this actually?
And the helm will still try to install the CRDs even if the controller is disabled

@wanjunlei
Copy link
Collaborator

wanjunlei commented Mar 22, 2023 •
edited
Loading

It should not add the fluentbit CRDs to scheme when fluent-bit is disabled.
https://github.com/fluent/fluent-operator/blob/master/cmd/fluent-manager/main.go#L53

@wanjunlei The init runs before the main function in which the disable flag is parsed, so how to do this actually? And the helm will still try to install the CRDs even if the controller is disabled

Just move these codes to the main.
The helm chart should be modified accordingly to support only installing the fluentbit or fluentd CRDS.

@nickytd nickytd mentioned this pull request Mar 23, 2023
Merged
@Kristian-ZH Kristian-ZH requested review from benjaminhuo and wenchajun and removed request for wanjunlei and benjaminhuo March 24, 2023 08:33
@Kristian-ZH
Copy link
Member Author

All the comments should be resolved, can you please take a second look?

@benjaminhuo
Copy link
Member

@Kristian-ZH Nice move! I like the idea of moving the crds to subchart!
Thanks!

@benjaminhuo benjaminhuo merged commit 66bac9c into fluent:master Mar 25, 2023
@benjaminhuo
Copy link
Member

@Kristian-ZH I'm going to invite you as the fluent-operator maintainer!

@Kristian-ZH Kristian-ZH deleted the disable-component-controllers branch March 27, 2023 06:41
@benjaminhuo benjaminhuo mentioned this pull request Mar 27, 2023
Merged
@benjaminhuo
Copy link
Member

@Kristian-ZH would you help to revise the install guide? https://github.com/fluent/fluent-operator#deploy-fluent-operator-with-helm

@Kristian-ZH Kristian-ZH mentioned this pull request Mar 27, 2023
Merged
jjsiv pushed a commit to jjsiv/fluent-operator that referenced this pull request Mar 27, 2023
@nickytd @jjsiv
Add an argument for disabling unused controllers (fluent#621)
8d0f06e 
* Add an argument for disabling unused controllers

Signed-off-by: Kristian-ZH <k.zhelyazkov@sap.com>

* Add an argument for disabling unused controllers

Signed-off-by: Kristian-ZH <k.zhelyazkov@sap.com>

* addresses PR feedback

* Moves CRDs deployments into sub-charts, allowing optionally to provision only fluentd or fluent-bit resources.

* adjust manifets controller gen target in the project Makefile

* add generated deepcopy of fluentd api

* rebase main.go to latest in master

* address PR feedback

---------

Signed-off-by: Kristian-ZH <k.zhelyazkov@sap.com>
Signed-off-by: Niki Dokovski <nickytd@gmail.com>
Co-authored-by: Niki Dokovski <nickytd@gmail.com>
@Waidmann Waidmann mentioned this pull request Mar 30, 2023
Closed
@wenchajun wenchajun mentioned this pull request Sep 20, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjaminhuo benjaminhuo benjaminhuo approved these changes

@wenchajun wenchajun Awaiting requested review from wenchajun

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make it possible to turn off fluent-bit/fluentd controllers when it is not used
5 participants
@Kristian-ZH @wanjunlei @benjaminhuo @wenchajun @nickytd