Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

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

Closed
wants to merge 5 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
49eebdb
Made working with envvars flexible
goruha May 2, 2018
b3c8285
Redo deamonset
goruha May 7, 2018
f5789f3
Address PR comments
goruha May 14, 2018
0354ee0
Address PR comments
goruha May 14, 2018
9f98474
Address PR comments
goruha May 14, 2018
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
2 changes: 1 addition & 1 deletion incubator/fluentd/Chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ apiVersion: v1
description: A Fluentd Elasticsearch Helm chart for Kubernetes.
icon: https://github.com/raw/fluent/fluentd-docs/master/public/logo/Fluentd_square.png
name: fluentd
version: 0.1.4
version: 0.2.0
appVersion: 0.12
home: https://www.fluentd.org/
sources:
Expand Down
67 changes: 43 additions & 24 deletions incubator/fluentd/templates/deployment.yaml → incubator/fluentd/templates/deamonset.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
apiVersion: apps/v1beta1
kind: Deployment
apiVersion: extensions/v1beta1
kind: DaemonSet
Copy link
Collaborator

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.

Copy link
Contributor Author

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?

Copy link
Collaborator

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Two things...

  1. 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.
  2. 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.

Copy link
Collaborator

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.

Copy link

Choose a reason for hiding this comment

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

(nitpick: deamonset.yaml misspelled)

Copy link

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK awesome. Thank you.

metadata:
name: {{ template "fluentd.fullname" . }}
labels:
Expand All @@ -18,6 +18,8 @@ spec:
labels:
app: {{ template "fluentd.name" . }}
release: {{ .Release.Name }}
annotations:
checksum/config: {{ include (print $.Template.BasePath "/configmap.yaml") . | sha256sum }}
spec:
{{- if .Values.image.pullSecrets }}
imagePullSecrets:
Expand All @@ -29,15 +31,26 @@ spec:
- name: {{ .Chart.Name }}
image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}"
imagePullPolicy: {{ .Values.image.pullPolicy }}
securityContext:
Copy link
Collaborator

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.

runAsNonRoot: false
runAsUser: 0
env:
- name: OUTPUT_HOST
value: {{ .Values.output.host | quote }}
- name: OUTPUT_PORT
value: {{ .Values.output.port | quote }}
- name: OUTPUT_BUFFER_CHUNK_LIMIT
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 }}
Copy link
Collaborator

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?

Copy link
Contributor Author

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?

Copy link
Collaborator

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

{{- if not (empty $value) }}
- name: {{ $name | quote }}
value: {{ $value | quote }}
{{- end }}
{{- end }}
{{- $secret_name := include "fluentd.fullname" . }}
{{- range $name, $value := .Values.env.secret }}
{{- if not ( empty $value) }}
- name: {{ $name | quote }}
valueFrom:
secretKeyRef:
name: {{ $secret_name }}
key: {{ $name | quote }}
{{- end }}
{{- end }}
resources:
{{ toYaml .Values.resources | indent 12 }}
ports:
Expand All @@ -49,23 +62,29 @@ spec:
- name: http-input
containerPort: 9880
protocol: TCP
livenessProbe:
Copy link
Collaborator

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?

Copy link
Contributor Author

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

Copy link
Collaborator

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.

httpGet:
# Use percent encoding for query param.
# The value is {"log": "health check"}.
# the endpoint itself results in a new fluentd
# tag 'fluentd.pod-healthcheck'
path: /fluentd.pod.healthcheck?json=%7B%22log%22%3A+%22health+check%22%7D
port: 9880
initialDelaySeconds: 5
timeoutSeconds: 1
volumeMounts:
- name: varlog
Copy link
Collaborator

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.

Copy link
Contributor Author

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

Copy link
Collaborator

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.

mountPath: /var/log
readOnly: true
- name: varlogpos
mountPath: /mnt/pos
- name: varlibdockercontainers
mountPath: /var/lib/docker/
readOnly: true
- name: config-volume-{{ template "fluentd.fullname" . }}
mountPath: /etc/fluent/config.d
mountPath: {{ .Values.configDir }}
volumes:
- name: config-volume-{{ template "fluentd.fullname" . }}
configMap:
name: {{ template "fluentd.fullname" . }}
- name: varlog
hostPath:
path: /var/log
- name: varlogpos
emptyDir: {}
- name: varlibdockercontainers
hostPath:
path: /var/lib/docker/
- name: config-volume-{{ template "fluentd.fullname" . }}
configMap:
name: {{ template "fluentd.fullname" . }}
{{- with .Values.nodeSelector }}
nodeSelector:
{{ toYaml . | indent 8 }}
Expand Down
16 changes: 16 additions & 0 deletions incubator/fluentd/templates/secret.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
apiVersion: v1
kind: Secret
metadata:
name: {{ include "fluentd.fullname" . }}
labels:
app: {{ template "fluentd.name" . }}
chart: {{ template "fluentd.chart" . }}
release: {{ .Release.Name }}
heritage: {{ .Release.Service }}
type: Opaque
data:
{{- range $name, $value := .Values.env.secret }}
{{- if not (empty $value) }}
{{ $name }}: {{ $value | b64enc }}
{{- end }}
{{- end }}
15 changes: 10 additions & 5 deletions incubator/fluentd/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,15 @@ image:
# - secret1
# - secret2

output:
host: elasticsearch-client.default.svc.cluster.local
port: 9200
buffer_chunk_limit: 2M
buffer_queue_limit: 8
env:
open:
Copy link

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

Copy link
Contributor Author

@goruha goruha May 14, 2018 •
edited
Loading

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

Copy link
Collaborator

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.

Copy link
Contributor Author

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

OUTPUT_HOST: elasticsearch-client.default.svc.cluster.local
OUTPUT_PORT: 9200
OUTPUT_BUFFER_CHUNK_LIMIT: 2M
OUTPUT_BUFFER_QUEUE_LIMIT: 8
FLUENT_UID: 0
FLUENTD_OPT: ""
secret:

service:
type: ClusterIP
Expand All @@ -40,6 +44,7 @@ ingress:
# hosts:
# - http-input.local

configDir: /etc/fluent/config.d
configMaps:
general.conf: |
# Prevent fluentd from handling records containing its own logs. Otherwise
Expand Down