-
Notifications
You must be signed in to change notification settings - Fork 16.8k
[incubator/fluentd] Generalize chart to work with different backends #5357
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: goruha Assign the PR to them by writing 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:
Approvers can indicate their approval by writing |
/assign @prydonius |
/assign @rendhalver |
apiVersion: apps/v1beta1 | ||
kind: Deployment | ||
apiVersion: extensions/v1beta1 | ||
kind: DaemonSet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you switching this to a DaemonSet?
There is an fluentd-elasticsearch chart that does exactly this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nitpick: deamonset.yaml
misspelled)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK awesome. Thank you.
volumeMounts: | ||
- name: varlog |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
It's two extra lines to exclude the mounts.
Not every implementation needs them and not all pods should have access to them.
buffer_chunk_limit: 2M | ||
buffer_queue_limit: 8 | ||
env: | ||
open: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
configMap
would be more clear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@osterman env
shows that we config container with environment variables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @osterman.
Config maps are clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@osterman @rendhalver can you provide the example of the structure that would you prefer?
I can not get what is the problem
Deamonsets are not really updateable as most of the fields are immutable. So using a damon set should be optional. |
@osterman @mattfarina @monotek @rendhalver |
@goruha |
I agree with @monotek here too. |
@@ -29,15 +34,26 @@ spec: | |||
- name: {{ .Chart.Name }} | |||
image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}" | |||
imagePullPolicy: {{ .Values.image.pullPolicy }} | |||
securityContext: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rendhalver can you provide example what structure do you prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
env: instead of env.open:
one less level to the data structure
@@ -49,23 +65,29 @@ spec: | |||
- name: http-input | |||
containerPort: 9880 | |||
protocol: TCP | |||
livenessProbe: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how that achieves the same ends.
@@ -49,13 +30,6 @@ configMaps: | |||
@type null | |||
</match> | |||
|
|||
# Used for health checking |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above
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. |
I think we try to mix different purpose charts.
What do you think about ? |
Imho if different fluentd images has to be used (output plugins) we should also use different charts. |
We have separate charts for the different usages. 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. |
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. |
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. |
This issue is being automatically closed due to inactivity. |
What
Why
in_tail