Add fluentd-cloudwatch log forwarder #211

icereval · 2016-11-06T22:58:07Z

Adds a Fluentd Cloudwatch chart adapted from fluentd-elasticsearch.

k8s-ci-robot · 2016-11-06T22:58:08Z

Can a kubernetes member verify that this patch is reasonable to test? If so, please reply with "@k8s-bot ok to test" on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands will still work. Regular contributors should join the org to skip this step.

If you have questions or suggestions related to this bot's behavior, please file an issue against the kubernetes/test-infra repository.

prydonius · 2016-11-10T19:22:36Z

incubator/fluentd-cloudwatch/templates/daemonset.yaml

+        release: "{{ .Release.Name }}"
+        kubernetes.io/cluster-service: "true"
+      annotations:
+        {{- include "toYaml" .Values.annotations | indent 8 }}


Helm now comes with a built-in toYaml function, so you can change this to:

annotations: {{- toYaml .Values.annotations | indent 8 }}

Note though, that it needs to be unindented.

My approach for this is to use a simple key/value iterator:

annotations: {{- range $key, $value := .Values.annotations }} {{ $key }}: {{ $value }} {{- end }}

Annotations are only ever k:v anyway, so there's no need to feed them through the toYaml filter.

prydonius · 2016-11-10T19:28:07Z

incubator/fluentd-cloudwatch/values.yaml

@@ -0,0 +1,26 @@
+image: 18fgsa/fluentd-cloudwatch
+imageTag: 0.1.0
+namespace: kube-system


Is the kube-system default a hard requirement? Generally we prefer the namespace be managed by helm using the --namespace flag.

prydonius · 2016-11-10T19:30:19Z

@k8s-bot ok to test

mgoodness · 2016-11-10T22:22:11Z

incubator/fluentd-cloudwatch/README.md

+
+| Parameter                  | Description                                | Default                                                    |
+| -----------------------    | ----------------------------------         | ---------------------------------------------------------- |
+| `image`                    | Image                                      | `18fgsa/fluentd-cloudwatch:{VERSION}`                      |


Don't need the :{VERSION} here since it's declared in imageTag

mgoodness · 2016-11-10T22:24:09Z

incubator/fluentd-cloudwatch/templates/daemonset.yaml

+        release: "{{ .Release.Name }}"
+        kubernetes.io/cluster-service: "true"
+      annotations:
+        {{- include "toYaml" .Values.annotations | indent 8 }}


My approach for this is to use a simple key/value iterator:

annotations: {{- range $key, $value := .Values.annotations }} {{ $key }}: {{ $value }} {{- end }}

Annotations are only ever k:v anyway, so there's no need to feed them through the toYaml filter.

mgoodness · 2016-11-10T22:45:30Z

incubator/fluentd-cloudwatch/templates/daemonset.yaml

+    chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
+    heritage: "{{ .Release.Service }}"
+    release: "{{ .Release.Name }}"
+    kubernetes.io/cluster-service: "true"


I don't think the cluster-service label is needed/useful. Per the add-on docs, it only has an effect on static manifests.

mgoodness · 2016-11-10T22:47:00Z

incubator/fluentd-cloudwatch/templates/daemonset.yaml

+        - name: varlibdockercontainers
+          mountPath: /var/lib/docker/containers
+          readOnly: true
+      terminationGracePeriodSeconds: 30


Would you consider adding hostNetwork: {{ default false .Values.hostNetwork }} here? We rely on it to create journald log streams using the hostname.

mgoodness · 2016-11-10T23:18:43Z

incubator/fluentd-cloudwatch/templates/configmap.yaml

+      log_group_name "#{ENV['LOG_GROUP_NAME']}"
+      auto_create_stream true
+      use_tag_as_stream true
+    </match>


I recommend/request moving the fluentd config to a values.yaml key, then templating it into a ConfigMap. That would make this more of a generic fluentd chart (though with CloudWatch/ES defaults), allowing others to provide their own, custom image & settings.

icereval · 2016-11-12T01:50:50Z

changes made from review, thanks!

found a bug when testing w/ annotations and reported:

Install Command: Unable to Escape Periods in Keys helm#1540

viglesiasce · 2016-11-22T18:36:54Z

Is it possible to make this the fluentd chart instead of the fluentd-cloudwatch chart? Can we move the config file (or at least its relevant parts) into the values file (similar to the prometheus chart)?

icereval · 2016-11-27T21:20:14Z

I'll move the current td-agent.conf into the values.yaml and update the chart.

As for a generic chart, it seems possible though it might complicate the deployments and overall maintenance of the chart more than one would like.

Some thoughts on a possible implementation:

Environment variables as a generic variable to support platforms like AWS.
Platform & OS specific configurations in the values.yaml for td-agent.conf configurations.
Image can be part of the platform/os values.yaml configuration or a generic image w/ all ruby gems installed.

fluentd:
  env:
    AWS_REGION: us-east-1
    LOG_GROUP_NAME: kubernetes
  platform: googleCloud
  configurations:
    # source: https://github.com/kubernetes/kubernetes/blob/b5b47620f5f13ec2d45dd8879206b99f9a7ecc2f/cluster/addons/fluentd-gcp/fluentd-gcp-image/google-fluentd.conf
    googleCloud:
      image: kubernetes/fluentd-gcp:latest
      tdAgent: |
        # This configuration file for Fluentd / td-agent is used
        # to watch changes to Docker log files that live in the
        ...
    awsCloudWatch:
      image: 18fgsa/fluentd-cloudwatch:0.1.0
      tdAgent: |
        ...
    googleCloudJournal: |
      ...
    elasticsearch: |
      ...

Google Cloud

$ helm install --name my-release \
  --set fluentd.platform=googleCloud \
    incubator/fluentd

Google Cloud Journal

$ helm install --name my-release \
  --set fluentd.platform=googleCloudJournal \
    incubator/fluentd

AWS CloudWatch

$ helm install --name my-release \
  --set fluentd.platform=awsCloudWatch, \
    fluentd.env.AWS_REGION=us-east-1, \
    fluentd.env.LOG_GROUP_NAME=kubernetes \
    incubator/fluentd

ElasticSearch

$ helm install --name my-release \
  --set fluentd.platform=elasticseach, \
    fluentd.env.ELASTIC_SEARCH_HOST=elasticsearch-logging, \
    fluentd.env.ELASTIC_SEARCH_PORT=9200 \
    incubator/fluentd

Thoughts?

k8s-ci-robot · 2016-11-27T21:38:24Z

Jenkins Charts e2e failed for commit 83dd8cd. Full PR test history.

The magic incantation to run this job again is @k8s-bot e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

viglesiasce · 2016-11-30T05:52:27Z

I really like the idea of allowing configurability of the platform to help guide users to the right set of working plugins and configs. How about something like:

fluentd:
  env:
    AWS_REGION: us-east-1
    LOG_GROUP_NAME: kubernetes
  platform: googleCloud

Then having the logic for which image to use in an if statement of the DaemonSet and a similar block in the ConfigMap.

viglesiasce · 2016-12-01T02:05:53Z

@k8s-bot ok to test

viglesiasce · 2017-01-13T05:50:28Z

This change is

aaglenn · 2017-01-23T23:52:35Z

Apologies if this is out of line. I came across this PR while looking for a fluentd Chart. My use case is strictly as a syslog ingest to influxdb store. I would like to voice my support for a generic, configurable fluentd Chart.

viglesiasce · 2017-01-31T19:19:53Z

@aaglenn not out of line at all! Thanks for providing your feedback.

icereval · 2017-02-08T03:03:48Z

Time is spread to thin and I'm not finding an opportunity to investigate the complexities of making this more agnostic, gonna close for now.

prydonius · 2017-02-08T03:09:25Z

FWIW I'd be okay merging this as-is and perhaps someone else can work on making it more agnostic, given it's in incubator.

mgoodness · 2017-02-08T03:30:05Z

@icereval Do you mind if I step in and work on it? Mostly there, IMHO. I have an idea about making it generic while providing some example (yet usable) out-of-the-box configs.

icereval · 2017-02-08T09:15:19Z

@mgoodness please do, not a problem at all

so0k · 2017-03-14T09:50:28Z

@mgoodness, as mentioned in fluent/fluentd-kubernetes-daemonset#3 - I'm making the base fluentd configurable so that multiple tags exist based on the plugins available within. I had created a PR for a fluentd-logentries Chart previously, but will look at how to use this work to base on.

old PR: #779

so0k · 2017-03-15T05:15:52Z

PR for unified fluentd repository @icereval - is it ok if I add your cloudwatch fluentd conf in there?

fluent/fluentd-kubernetes-daemonset#4

Edit: Done, added cloudwatch image build to fluentd-kubernetes image

so0k · 2017-03-15T07:31:36Z

@viglesiasce - I personally prefer config files to be in a separate folder and use the helm AsSecrets / AsConfig functions with a Files.Glob and a target directory as follows:

config file (in this case it has secrets)
helm template consuming all config files

The advantage this gives is that you do not need to worry about the yaml format (and spacing) which you do have to worry about if you work with config files that are embedded in yaml. I find it more user friendly as well (just drop your config that you're used to in there, and run this command, done)

Your thoughts?

so0k · 2017-04-05T17:29:31Z

I believe this is relevant to this PR; https://github.com/krallistic/kubernetes-fluentd

The container, and the chart is not called 'fluentd-kubernetes-daemonset' The configuration is now inside the container (from PR comment: helm#211 (comment)), so I removed the configMap. The AWS keys are added as secrets from files, using Files.Glob (from PR comment helm#211 (comment))

The configuration is now inside the container (from PR comment: helm#211 (comment)), I then removed the configMap holding the config. The AWS keys are added as secrets from files, using Files.Glob (from PR comment helm#211 (comment))

* fluentd cloudwatch incubator * rename deployment -> daemonset * annotations as value * reorder values * use helm namespace * address code review * move td-agent.conf into values.yaml * Updated fluentd-cloudwatch code from PR 211 The configuration is now inside the container (from PR comment: #211 (comment)), I then removed the configMap holding the config. The AWS keys are added as secrets from files, using Files.Glob (from PR comment #211 (comment)) * Add configMap for fluentd config Copied the config from what's in /fluentd/etc in image: fluent/fluentd-kubernetes-daemonset:v0.12-cloudwatch * Update to use namespaces * Add appVersion * Add fluentd icon * Using github usernames instead of real names * Update label for configmap

* fluentd cloudwatch incubator * rename deployment -> daemonset * annotations as value * reorder values * use helm namespace * address code review * move td-agent.conf into values.yaml * Updated fluentd-cloudwatch code from PR 211 The configuration is now inside the container (from PR comment: helm#211 (comment)), I then removed the configMap holding the config. The AWS keys are added as secrets from files, using Files.Glob (from PR comment helm#211 (comment)) * Add configMap for fluentd config Copied the config from what's in /fluentd/etc in image: fluent/fluentd-kubernetes-daemonset:v0.12-cloudwatch * Update to use namespaces * Add appVersion * Add fluentd icon * Using github usernames instead of real names * Update label for configmap

googlebot added the cla: yes label Nov 6, 2016

prydonius reviewed Nov 10, 2016

View reviewed changes

mgoodness suggested changes Nov 10, 2016

View reviewed changes

icereval added 6 commits November 11, 2016 18:01

fluentd cloudwatch incubator

b7aad98

rename deployment -> daemonset

f6563fe

annotations as value

d3cbb6d

reorder values

e130cd7

use helm namespace

0455357

address code review

3a83304

prydonius added the awaiting review label Nov 21, 2016

viglesiasce added changes needed and removed awaiting review labels Nov 22, 2016

move td-agent.conf into values.yaml

83dd8cd

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 27, 2016

viglesiasce modified the milestone: Milestone 1 Feb 3, 2017

icereval closed this Feb 8, 2017

mboersma mentioned this pull request Feb 15, 2017

Use upstream fluentd chart deis/monitor#179

Closed

so0k mentioned this pull request Mar 15, 2017

Unify and Automate maintenance of multiple logging targets fluent/fluentd-kubernetes-daemonset#4

Merged

5 tasks

kenden mentioned this pull request Apr 21, 2017

Add Fluentd > cloudwatch log forwarder #952

Merged

kenden mentioned this pull request May 8, 2017

Helm Package fluent/fluentd-kubernetes-daemonset#8

Closed

Cryptophobia mentioned this pull request Mar 20, 2018

Use upstream fluentd chart teamhephy/monitor#6

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fluentd-cloudwatch log forwarder #211

Add fluentd-cloudwatch log forwarder #211

icereval commented Nov 6, 2016

k8s-ci-robot commented Nov 6, 2016

prydonius Nov 10, 2016

mgoodness Nov 10, 2016

prydonius Nov 10, 2016

prydonius commented Nov 10, 2016

mgoodness Nov 10, 2016

mgoodness Nov 10, 2016

mgoodness Nov 10, 2016

mgoodness Nov 10, 2016

mgoodness Nov 10, 2016

icereval commented Nov 12, 2016

viglesiasce commented Nov 22, 2016

icereval commented Nov 27, 2016

k8s-ci-robot commented Nov 27, 2016

viglesiasce commented Nov 30, 2016

viglesiasce commented Dec 1, 2016

viglesiasce commented Jan 13, 2017

aaglenn commented Jan 23, 2017

viglesiasce commented Jan 31, 2017

icereval commented Feb 8, 2017

prydonius commented Feb 8, 2017

mgoodness commented Feb 8, 2017

icereval commented Feb 8, 2017

so0k commented Mar 14, 2017

so0k commented Mar 15, 2017 •

edited

Loading

so0k commented Mar 15, 2017 •

edited

Loading

so0k commented Apr 5, 2017

Add fluentd-cloudwatch log forwarder #211

Add fluentd-cloudwatch log forwarder #211

Conversation

icereval commented Nov 6, 2016

k8s-ci-robot commented Nov 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prydonius commented Nov 10, 2016

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

icereval commented Nov 12, 2016

viglesiasce commented Nov 22, 2016

icereval commented Nov 27, 2016

k8s-ci-robot commented Nov 27, 2016

viglesiasce commented Nov 30, 2016

viglesiasce commented Dec 1, 2016

viglesiasce commented Jan 13, 2017

aaglenn commented Jan 23, 2017

viglesiasce commented Jan 31, 2017

icereval commented Feb 8, 2017

prydonius commented Feb 8, 2017

mgoodness commented Feb 8, 2017

icereval commented Feb 8, 2017

so0k commented Mar 14, 2017

so0k commented Mar 15, 2017 • edited Loading

so0k commented Mar 15, 2017 • edited Loading

so0k commented Apr 5, 2017

so0k commented Mar 15, 2017 •

edited

Loading

so0k commented Mar 15, 2017 •

edited

Loading