[incubator/fluentd] Generalize chart to work with different backends #5357

goruha · 2018-05-02T14:57:43Z

What

Made working with environment variables flexible
Made Fluentd be daemon set
Remove health check
Parametrise config mount dir
Mount host logs into the container

Why

Make chart be flexible enough to work with different fluentd containers (ex. fluentd-cloudwatch)
To store secrets private
DeamonSet allow collecting logs from all nodes
Not all fluentd containers support health checks
Different fluentd containers expect configs in a different paths
Allow collecting host logs with fluentd in_tail

k8s-ci-robot · 2018-05-02T14:57:54Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: goruha
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: prydonius

Assign the PR to them by writing /assign @prydonius in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

goruha · 2018-05-03T11:06:16Z

/assign @prydonius

prydonius · 2018-05-08T10:27:44Z

/assign @rendhalver

rendhalver · 2018-05-08T12:15:03Z

incubator/fluentd/templates/deamonset.yaml

-apiVersion: apps/v1beta1
-kind: Deployment
+apiVersion: extensions/v1beta1
+kind: DaemonSet


Why are you switching this to a DaemonSet?
There is an fluentd-elasticsearch chart that does exactly this.

that's true.
I'd like to make chart general enough for the different use of fluentd.
there are already fluentd-elasticsearch, fluentd-clouwatch.
I use fluentd chart with datadog.
It seems to be a mistake to create a chart for each type of backend.
What do you think about?

I like the idea but if we are going to replace all three charts with one it needs to be flexible and configurable enough to do what each of those chart do already without reducing the functionality the current Charts.
I wrote this one to replace logstash in my Elastic-Stack cluster so it didn't need to be a DaemonSet or need access to node logs.

Two things...

If you're going to use something from the workloads api (e.g., DaemonSet) use it from the apps API group. Extensions should not be used anymore.

I like the idea of a fluentd chart that can have handle being installed with various backends. Yet, having different ones for each backend is ok, too. But, we should pick one way and stay consistent with it. If there is one it should cleanly handle all the cases and needs for all of them.

If the DaemonSet can be still used as part of a Service then that works for me.
We use this chart to route logs from devices outside our cluster to an ES cluster in Kube so I would like to keep that functionality.

(nitpick: deamonset.yaml misspelled)

If the DaemonSet can be still used as part of a Service then that works for me

A Service works by selecting Pods according to a selector so it's compatible with DaemonSets which create pods. A Service doesn't care (or know) how the Pods were created.

OK awesome. Thank you.

rendhalver · 2018-05-08T12:17:18Z

incubator/fluentd/templates/deamonset.yaml

        volumeMounts:
+        - name: varlog


See above. You are basically turning this into the fluentd-elasticsearch chart.
If you are trying to merge these two please make these mounts optional.
There are uses for fluentd that don't involve container logs.

@rendhalver would it be problem if we will mount logs inside container that do not require them?
I think we need mount logs always and simplify chart install

Why?
It's two extra lines to exclude the mounts.
Not every implementation needs them and not all pods should have access to them.

osterman · 2018-05-08T18:28:28Z

incubator/fluentd/values.yaml

-  buffer_chunk_limit: 2M
-  buffer_queue_limit: 8
+env:
+  open:


configMap would be more clear

@osterman env shows that we config container with environment variables

I agree with @osterman.
Config maps are clearer.

@osterman @rendhalver can you provide the example of the structure that would you prefer?
I can not get what is the problem

monotek · 2018-05-10T12:35:37Z

Deamonsets are not really updateable as most of the fields are immutable. So using a damon set should be optional.

goruha · 2018-05-14T07:15:46Z

@monotek we will use https://kubernetes.io/docs/tasks/manage-daemon/update-daemon-set/

goruha · 2018-05-14T09:36:24Z

@osterman @mattfarina @monotek @rendhalver
I addressed your comments and provide readme with examples how to use this chart with different backends.
Hope you would find chart flexible enough and ready to release

monotek · 2018-05-14T10:00:42Z

@goruha
I still think daemonset or deployment should be optional.
Maybe one don't wants to use it for kubernetes / docker logs but for sending app logs to the fluentd forward plugin on port 24224.
In this case 1 pod created via a deployment would be enough.

rendhalver · 2018-05-14T12:11:22Z

I agree with @monotek here too.
We should have the option of running as a DaemonSet or a Deployment.

rendhalver · 2018-05-14T12:17:41Z

incubator/fluentd/templates/deamonset.yaml

@@ -29,15 +34,26 @@ spec:
      - name: {{ .Chart.Name }}
        image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}"
        imagePullPolicy: {{ .Values.image.pullPolicy }}
+        securityContext:


This should be dependent on whether the logs are mounted.
If the pod doersn't need access to the logs if shouldn't have root access.

rendhalver · 2018-05-14T12:19:23Z

incubator/fluentd/templates/deamonset.yaml

-            value: {{ .Values.output.buffer_chunk_limit | quote }}
-          - name: OUTPUT_BUFFER_QUEUE_LIMIT
-            value: {{ .Values.output.buffer_queue_limit | quote }}
+{{- range $name, $value := .Values.env.open }}


Why not just have one less level here and put the env vars in env.
Are you planning on putting anything else in this array?

@rendhalver can you provide example what structure do you prefer?

env: instead of env.open:
one less level to the data structure

rendhalver · 2018-05-14T12:20:52Z

incubator/fluentd/templates/deamonset.yaml

@@ -49,23 +65,29 @@ spec:
          - name: http-input
            containerPort: 9880
            protocol: TCP
-        livenessProbe:


Why remove this?
We have health checks for a reason.
Do we not want to know if the pods are functioning correctly before we route traffic to them?

Because liveness probe required config to provide the endpoint for the check.
Not all containers support such configuration.
Instead of that, I disabled supervisor mode, so pod will failed if something goes wrong.
https://github.com/kubernetes/charts/pull/5357/files#diff-6ec61ff69620bf74339bd25610e91021R20

I don't see how that achieves the same ends.

rendhalver · 2018-05-14T12:23:08Z

incubator/fluentd/values.yaml

@@ -49,13 +30,6 @@ configMaps:
      @type null
    </match>

-    # Used for health checking


monotek · 2018-05-15T06:51:52Z

Imho we should also provide 2 configmaps for optional deployment / daemonset.

If deployment is used I'm pretty sure most of the current config is unwanted.

goruha · 2018-07-05T09:51:48Z

I think we try to mix different purpose charts.
My suggestion is to have 2 charts

fluentd - for fluentd as independent software (so leave this chart as is and close PR)
fluentd_kubernetes - run fluentd to collect kubernetes logs and forward it to different backends (create a new chart with separate PR)

What do you think about ?
@monotek @osterman @mattfarina @rendhalver @prydonius

monotek · 2018-07-05T09:58:45Z

Imho if different fluentd images has to be used (output plugins) we should also use different charts.

rendhalver · 2018-07-05T10:01:50Z

We have separate charts for the different usages.
We currently have multiple charts for the logging that send to different backends.

Calling the logging chart fluentd_kubernetes isn't the best choice of name, fluentd-logging or fluentd-container-logging would be a better name and more explicitly explains what it does.

stale · 2018-08-18T22:46:23Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

stale · 2018-09-17T23:24:31Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

stale · 2018-10-02T17:34:48Z

This issue is being automatically closed due to inactivity.

Made working with envvars flexible

49eebdb

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 2, 2018

k8s-ci-robot assigned prydonius May 3, 2018

Redo deamonset

b3c8285

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 7, 2018

k8s-ci-robot assigned rendhalver May 8, 2018

rendhalver suggested changes May 8, 2018

View reviewed changes

goruha changed the title ~~Made working with envvars flexible~~ [incubator/fluentd] Generalize chart to work with different backends May 8, 2018

goruha mentioned this pull request May 8, 2018

Add Datadog Log Forwarding with Fluentd cloudposse/geodesic#134

Merged

osterman reviewed May 8, 2018

View reviewed changes

Address PR comments

f5789f3

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 14, 2018

goruha added 2 commits May 14, 2018 15:32

Address PR comments

0354ee0

Address PR comments

9f98474

rendhalver suggested changes May 14, 2018

View reviewed changes

goruha mentioned this pull request Jul 5, 2018

Use cloudposse fluentd chart cloudposse/geodesic#173

Merged

mattfarina unassigned rendhalver Jul 19, 2018

stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 18, 2018

mattfarina added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Aug 27, 2018

stale bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 27, 2018

stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 17, 2018

stale bot closed this Oct 2, 2018

[incubator/fluentd] Generalize chart to work with different backends #5357

[incubator/fluentd] Generalize chart to work with different backends #5357

Conversation

goruha commented May 2, 2018 • edited Loading

What

Why

k8s-ci-robot commented May 2, 2018

goruha commented May 3, 2018

prydonius commented May 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goruha May 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monotek commented May 10, 2018 • edited Loading

goruha commented May 14, 2018

goruha commented May 14, 2018

monotek commented May 14, 2018 • edited Loading

rendhalver commented May 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monotek commented May 15, 2018

goruha commented Jul 5, 2018

monotek commented Jul 5, 2018 • edited Loading

rendhalver commented Jul 5, 2018

stale bot commented Aug 18, 2018

stale bot commented Sep 17, 2018

stale bot commented Oct 2, 2018

goruha commented May 2, 2018 •

edited

Loading

goruha May 14, 2018 •

edited

Loading

monotek commented May 10, 2018 •

edited

Loading

monotek commented May 14, 2018 •

edited

Loading

rendhalver commented May 14, 2018 •

edited

Loading

monotek commented Jul 5, 2018 •

edited

Loading