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

Add helm variables to choose controllers of karmada and kubernetes ma… #2542

Closed
wants to merge 1 commit into from
Closed

Add helm variables to choose controllers of karmada and kubernetes ma… #2542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions charts/karmada/templates/karmada-controller-manager.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ spec:
- /bin/karmada-controller-manager
- --kubeconfig=/etc/kubeconfig
- --bind-address=0.0.0.0
{{- if .Values.controllerManager.config.controllers }}
- --controllers={{ .Values.controllerManager.config.controllers }}
{{- end }}
- --cluster-status-update-frequency=10s
- --secure-port=10357
- --leader-elect-resource-namespace={{ include "karmada.namespace" . }}
Expand Down
2 changes: 1 addition & 1 deletion charts/karmada/templates/kube-controller-manager.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ spec:
- --cluster-name=karmada
- --cluster-signing-cert-file=/etc/karmada/pki/server-ca.crt
- --cluster-signing-key-file=/etc/karmada/pki/server-ca.key
- --controllers=namespace,garbagecollector,serviceaccount-token,ttl-after-finished,bootstrapsigner,csrapproving,csrcleaner,csrsigning
- --controllers={{ .Values.kubeControllerManager.config.controllers }}
- --kubeconfig=/etc/kubeconfig
- --leader-elect=true
- --node-cidr-mask-size=24
Expand Down
6 changes: 6 additions & 0 deletions charts/karmada/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,9 @@ controllerManager:
## - myRegistryKeySecretName
##
pullSecrets: []
## @param controllerManager.config.controllers the controllers launched by the manager.
config:
Copy link
Member

Choose a reason for hiding this comment

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

I prefer to use controllers directly. config looks a bit redundant.

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 can use controllers directly instead of config.controllers, but I preferred the latter because it will be more organized if some additional parameters need to be added such as cluster-status-update-frequency or v...
All these values will be clearly under controllerManager.config rather than directly at the root of controllerManager, which I find more readable.

What do you think about it ?

Copy link
Member

Choose a reason for hiding this comment

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

I got what you mean. I prefer to put some related configuration items under the same group. It's ok to use controllers directly when there are currently no other options.
BTW, I don't think config is a good group name. All of them in controllerManager belong to the category of config.

## controller manager config
controllerManager:

Copy link
Member

Choose a reason for hiding this comment

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

I can use controllers directly instead of config.controllers, but I preferred the latter because it will be more organized if some additional parameters need to be added such as cluster-status-update-frequency or v...

I agree that we may need to customize other parameters later, they are organized together would be great!

I don't think config is a good group name. All of them in controllerManager belong to the category of config.

How about args?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes args seems like a better name to me.
Should we use it as

  • an object with predefined properties (like I did in the PR)
  • an array where as many elements can be added freely. We could fill this variable like this during a release
controllerManager:
  args:
    - --controllers=-namespace,*
    - --v=3

The latter makes it possible to better cover all the possibilities without modification.

Copy link
Member

Choose a reason for hiding this comment

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

I think the former is better. The latter may confuse users about which configuration items can be configured.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The latter option allows to configure all args of karmada-controller-manager, that allows the user to configure anything he wants, not just what we imagine : there can always be unimagined scenarios, like for example mine. And both current and future. And the chart will be compatible without change not only with current arguments but also with future ones.
@RainbowMango What do you think about this two solutions ?

Copy link
Member

Choose a reason for hiding this comment

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

The latter option allows to configure all args of karmada-controller-manager, that allows the user to configure anything he wants, not just what we imagine

The idea is great, but my concern is the user experience. The users might need to specify all args at here, even they just want to customize the --controllers.

Copy link
Member

Choose a reason for hiding this comment

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

Kindly ping @fredgate .

controllers: ""
## @param controllerManager.resources resource quota of the karmada-controller-manager
resources: {}
# If you do want to specify resources, uncomment the following
Expand Down Expand Up @@ -458,6 +461,9 @@ kubeControllerManager:
## - myRegistryKeySecretName
##
pullSecrets: []
## @param kubeControllerManager.config.controllers the controllers launched by the kubernetes manager.
config:
controllers: namespace,garbagecollector,serviceaccount-token,ttl-after-finished,bootstrapsigner,csrapproving,csrcleaner,csrsigning
## @param kubeControllerManager.resources resource quota of the kube-controller-manager
resources:
# If you do want to specify resources, uncomment the following
Expand Down