Optimize the helm chart #208

wenchajun · 2022-02-21T15:44:00Z

Signed-off-by: chengdehao wenchajun@gmail.com

benjaminhuo · 2022-02-22T02:36:48Z

charts/fluent-operator/values.yaml

+
+#Set this to containerd or crio if you want to collect CRI format logs
+containerRuntime: docker
+Kubernetes: false


what is this Kubernetes used for?
this operator can only be used for k8s

benjaminhuo · 2022-02-22T02:38:09Z

charts/fluent-operator/values.yaml

-  fluentd:
+
+fluentd:
+  enable: false


we need resources for fluentd as well, @zhu733756 do you have any suggest values?

we need resources for fluentd as well, @zhu733756 do you have any suggest values?

I have some suggests below, totally lgtm.

zhu733756 · 2022-02-22T03:01:38Z

charts/fluent-operator/values.yaml

+#Set this to containerd or crio if you want to collect CRI format logs
+containerRuntime: docker
+Kubernetes: false
+
 operator:
  image: "kubesphere/fluent-operator"


Suggested to

initcontainer: image: xx tag: xx container: image: xxx tag: xxx

Or I think initcontainer can hard code in yaml.

zhu733756 · 2022-02-22T03:05:12Z

charts/fluent-operator/values.yaml

    forward:
-      enable: false
      host: "fluentd.kubesphere-logging-system.svc"


Does have anyway to keep host equals with fluentd.name，like host: "{fluentd.name}.kubesphere-logging-system.svc", Or this forward is determined by the fluentd section is enough?

zhu733756 · 2022-02-22T03:07:20Z

charts/fluent-operator/values.yaml

+  name: flentd
+  port: 24224
+  image: "kubesphere/fluentd"
+  tag: "v1.14.4"


repo and tag always under the same field named image.
like :

image: repository: "" tag: ""

zhu733756 · 2022-02-23T01:16:47Z

charts/fluent-operator/values.yaml

+    - kube-system
+    - kubesphere-monitoring-system
+    #Configure the output plugin parameter in FluentBit.
+    #You can set enable to true to output logs to the specified location.


The comment here should fix.

zhu733756 · 2022-02-23T01:19:10Z

charts/fluent-operator/values.yaml

  initcontainer:
    image: "docker"
    tag: "19.03"
+  container:
+    image: "kubesphere/fluent-operator"


image here suggested to the field repository. I think image means repository+ tag.

zhu733756 · 2022-02-23T01:44:36Z

README.md

@@ -202,6 +202,26 @@ If your container runtime is `cri-o`
 helm upgrade fluentbit-operator --create-namespace -n kubesphere-logging-system charts/fluent-operator/  --set Kubernetes=true,containerRuntime=crio
 ```


fluentbit-operator => fluent-operator

zhu733756 · 2022-02-23T01:46:47Z

charts/fluent-operator/values.yaml

    forward:
-      enable: false
-      host: "fluentd.kubesphere-logging-system.svc"
+      host: "kubesphere-logging-system.svc"


enable: false should keep.

zhu733756 · 2022-02-23T01:58:10Z

charts/fluent-operator/values.yaml

    forward:
-      enable: false
-      host: "fluentd.kubesphere-logging-system.svc"
+      host: "kubesphere-logging-system.svc"


I think host and port should be removed and only defined in fluentd part. We should keep host and port defined in one place. Besides, forward.enabled to enable or disable the function of fluentd is enough.

What do u guys think of? @benjaminhuo @wenchajun

forward logs to fluentd should be controlled by the fluentd switch, and should be in the fluentd part.

Yeah, I almost get it. Fluentd can enable by its switch for other requirements. And logs from Fluent Bit forwarding to fluentd would use two switches.

My understanding is that the forward profile is controlled by fluentd.enable and no dedicated forward profile is being set up. Is this right?

If suggested to remove Fluentbit forward part and needs some docs to implement this case. User's confusion about the forward plugin can be defined to other fluentds. In addition, one fluentd activated, there must be one service, the docs also should implement this case.

fluentbit forwarding to fluentd is only required when fluentd is enabled and there is requirements to receive logs from fluentbit. Fluentd can also receive logs from non-fluentd source such as HTTP or syslog

No need to add fluentbit forward CR in fluentbit configs because you don't know if the fluentd is enabled in fluentbit config.

benjaminhuo · 2022-02-23T06:08:11Z

README.md

@@ -112,19 +112,19 @@ Fluent Bit Operator supports `docker` as well as `containerd` and `CRI-O`. `cont
 If your container runtime is `docker`

 ```shell
-helm install fluentbit-operator  --create-namespace -n kubesphere-logging-system charts/fluent-operator/ --set containerRuntime=docker
+helm install fluent-operator  --create-namespace -n kubesphere-logging-system charts/fluent-operator/ --set containerRuntime=docker


The namespace should use an env variable to set before any helm install cmd, and reference this env in the following helm install cmd

helm does not initially support the environment variable approach for security reasons. Please see：helm/helm#944 . helm3 can use set to set environment variables. But I don't think there is much difference between this way and the previous one. Perhaps we could create a namespace.yaml file and change its namespace by setting the value in value.yaml?

Maybe we can use {{ .Release.Namespace }} to tell the CRs what's namespace the helm used?

Ok, maybe we needn't change the namespace as a helm parameter.
My concern is this is a community project now, the kubesphere namespace is not appropriate anymore.

We might need to use a namespace like fluent for readme, helm, YAML and samples
@wenchajun you can do this in another PR and @zhu733756 can help to review

OK，I will revise and test as soon as possible

Signed-off-by: chengdehao <wenchajun@gmail.com>

benjaminhuo requested changes Feb 22, 2022

View reviewed changes

zhu733756 reviewed Feb 22, 2022

View reviewed changes

zhu733756 mentioned this pull request Feb 22, 2022

Renaming fluentbit-operator to fluent-operator #189 #190

Closed

8 tasks

wenchajun force-pushed the fix branch from 9f12141 to bda459c Compare February 22, 2022 14:23

zhu733756 reviewed Feb 23, 2022

View reviewed changes

wenchajun force-pushed the fix branch 3 times, most recently from 1d258f3 to 353d70b Compare February 23, 2022 05:19

benjaminhuo reviewed Feb 23, 2022

View reviewed changes

wenchajun force-pushed the fix branch from 353d70b to 9d21128 Compare February 23, 2022 08:36

Optimize the helm chart

42e92cc

Signed-off-by: chengdehao <wenchajun@gmail.com>

wenchajun force-pushed the fix branch from 9d21128 to 42e92cc Compare February 23, 2022 09:18

benjaminhuo approved these changes Feb 23, 2022

View reviewed changes

benjaminhuo merged commit 0a7e3dd into fluent:master Feb 23, 2022

wenchajun deleted the fix branch March 3, 2022 07:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize the helm chart #208

Optimize the helm chart #208

wenchajun commented Feb 21, 2022

benjaminhuo Feb 22, 2022

benjaminhuo Feb 22, 2022

zhu733756 Feb 22, 2022

zhu733756 Feb 22, 2022 •

edited

zhu733756 Feb 22, 2022

zhu733756 Feb 22, 2022 •

edited

zhu733756 Feb 23, 2022

zhu733756 Feb 23, 2022 •

edited

zhu733756 Feb 23, 2022

zhu733756 Feb 23, 2022

zhu733756 Feb 23, 2022 •

edited

zhu733756 Feb 23, 2022

benjaminhuo Feb 23, 2022

zhu733756 Feb 23, 2022 •

edited

wenchajun Feb 23, 2022

zhu733756 Feb 23, 2022 •

edited

benjaminhuo Feb 23, 2022 •

edited

benjaminhuo Feb 23, 2022

wenchajun Feb 23, 2022

zhu733756 Feb 23, 2022 •

edited

benjaminhuo Feb 23, 2022 •

edited

wenchajun Feb 23, 2022

		@@ -202,6 +202,26 @@ If your container runtime is `cri-o`
		helm upgrade fluentbit-operator --create-namespace -n kubesphere-logging-system charts/fluent-operator/ --set Kubernetes=true,containerRuntime=crio
		```

Optimize the helm chart #208

Optimize the helm chart #208

Conversation

wenchajun commented Feb 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhu733756 Feb 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhu733756 Feb 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhu733756 Feb 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhu733756 Feb 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhu733756 Feb 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhu733756 Feb 23, 2022 • edited

Choose a reason for hiding this comment

benjaminhuo Feb 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhu733756 Feb 23, 2022 • edited

Choose a reason for hiding this comment

benjaminhuo Feb 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhu733756 Feb 22, 2022 •

edited

zhu733756 Feb 22, 2022 •

edited

zhu733756 Feb 23, 2022 •

edited

zhu733756 Feb 23, 2022 •

edited

zhu733756 Feb 23, 2022 •

edited

zhu733756 Feb 23, 2022 •

edited

benjaminhuo Feb 23, 2022 •

edited

zhu733756 Feb 23, 2022 •

edited

benjaminhuo Feb 23, 2022 •

edited