Refactored the fluentd-es addon #50082

crassirostris · 2017-08-03T10:16:18Z

Refactor fluentd-elasticsearch addon:

Decrease the number of files by moving RBAC-related objects in the same files where they're used
Move the fluentd configuration out of the image
Don't use PVC to avoid leaking resources in e2e tests
Fluentd now ingest docker and kubelet logs that are written to journald
Disable X-Pack, because it's not free

Fixes #41462
Fixes #49816
Fixes #48973
Fixes #49450

@aknuds1 @coffeepac Could you please take a look?

Fluentd DaemonSet in the fluentd-elasticsearch addon is configured via ConfigMap and includes journald plugin
Elasticsearch StatefulSet in the fluentd-elasticsearch addon uses local storage instead of PVC by default

aknuds1 · 2017-08-03T12:56:34Z

@crassirostris Thanks, will have a look!

aknuds1 · 2017-08-03T13:37:09Z

cluster/addons/fluentd-elasticsearch/README.md

+
+The Elasticsearch StatefulSet will use the [EmptyDir][emptyDir] volume to
+store data. This volume will be erased with the pod evictions, it is used
+just for the testing purposes.


I would say 'just for testing purposes'.

aknuds1 · 2017-08-03T13:37:55Z

cluster/addons/fluentd-elasticsearch/README.md

-that by default will be 100 Gi per replica. Please adjust this to your needs (including
-possibly choosing a more suitable StorageClass).
+
+The Elasticsearch StatefulSet will use the [EmptyDir][emptyDir] volume to


It's called emptyDir rather than EmptyDir?

It's referred as EmptyDir in the API docs (e.g. in https://kubernetes.io/docs/api-reference/v1.7/#emptydirvolumesource-v1-core)

aknuds1 · 2017-08-03T13:38:55Z

cluster/addons/fluentd-elasticsearch/README.md

-[![Analytics](https://kubernetes-site.appspot.com/UA-36037335-10/GitHub/cluster/addons/fluentd-elasticsearch/README.md?pixel)]()
+[fluentd]: http://www.fluentd.org/
+[elasticsearch]: http://www.elasticsearch.org/
+[kibana]: https://www.elastic.co/products/elasticsearch


https://www.elastic.co/products/kibana

Yeah, it's funny, I actually noticed that, but it kinda fell in the crack :D

Ack

aknuds1 · 2017-08-03T13:39:11Z

cluster/addons/fluentd-elasticsearch/README.md


-[![Analytics](https://kubernetes-site.appspot.com/UA-36037335-10/GitHub/cluster/addons/fluentd-elasticsearch/README.md?pixel)]()
+[fluentd]: http://www.fluentd.org/
+[elasticsearch]: http://www.elasticsearch.org/


https://www.elastic.co/products/elasticsearch

aknuds1 · 2017-08-03T13:41:03Z

cluster/addons/fluentd-elasticsearch/es-image/config/elasticsearch.yml

@@ -12,3 +12,6 @@ path.data: /data
 network.host: 0.0.0.0

 discovery.zen.minimum_master_nodes: ${MINIMUM_MASTER_NODES}
+
+xpack.security.enabled: false


aknuds1 · 2017-08-03T13:45:14Z

cluster/addons/fluentd-elasticsearch/fluentd-es-ds.yaml

        resources:
          limits:
-            memory: 200Mi
+            memory: 500Mi


How did you determine the new value?

Empirically. Note that it's limit, I haven't changed the request. that's just to protect the fluentd from ruby having memory spikes

aknuds1 · 2017-08-03T13:45:35Z

cluster/addons/fluentd-elasticsearch/fluentd-es-ds.yaml

@@ -64,3 +102,10 @@ spec:
      - name: varlibdockercontainers
        hostPath:
          path: /var/lib/docker/containers
+      # It is needed to copy systemd library to decompress journals


aknuds1 · 2017-08-03T13:46:41Z

cluster/addons/fluentd-elasticsearch/fluentd-es-image/Gemfile

+gem 'fluent-plugin-kubernetes_metadata_filter', '~>0.27.0'
+gem 'fluent-plugin-elasticsearch', '~>1.9.5'
+gem 'fluent-plugin-systemd', '~>0.0.8'
+gem 'fluent-plugin-prometheus', '~>0.3.0'


aknuds1 · 2017-08-03T13:47:13Z

cluster/addons/fluentd-elasticsearch/fluentd-es-image/README.md


+[fluentd]: http://www.fluentd.org/
+[elasticsearch]: http://www.elasticsearch.org/


https://www.elastic.co/products/elasticsearch

aknuds1 · 2017-08-03T13:48:33Z

cluster/addons/fluentd-elasticsearch/kibana-deployment.yaml

+            value: /api/v1/proxy/namespaces/kube-system/services/kibana-logging
+          - name: XPACK_MONITORING_ENABLED
+            value: false
+          - name: XPACK_SECURITY_ENABLED


crassirostris · 2017-08-03T14:05:11Z

@aknuds1 Thanks! I'll address it shortly

crassirostris · 2017-08-03T16:19:41Z

/retest

crassirostris · 2017-08-03T16:35:07Z

/retest

crassirostris · 2017-08-03T17:53:13Z

/retest

coffeepac · 2017-08-04T00:12:42Z

cluster/addons/fluentd-elasticsearch/fluentd-es-ds.yaml

 spec:
  template:
    metadata:
      labels:
        k8s-app: fluentd-es
        kubernetes.io/cluster-service: "true"
-        version: v1.24
+        version: v0.2.0


why is this version going backwards?

I decided to move to the semver, but I agree that moving 1.24 -> 0.1.24 and then updating to 0.2.0 is less intuitive than 1.24 -> 1.24.0 -> 2.0.0

I'll change the version, thanks for noticing!

coffeepac · 2017-08-04T00:16:57Z

this fixes: #41462

crassirostris · 2017-08-04T09:22:15Z

Thanks for reminding!

aknuds1 · 2017-08-04T09:28:58Z

cluster/addons/fluentd-elasticsearch/README.md

+[X-Pack plugin][xPack]. See configuration parameter `xpack.security.enabled`
+in Elasticsearch and Kibana configurations. It can also be set via
+`XPACK_SECURITY_ENABLED` env variable. You can then use [ConfigMaps][configMap]
+and [Secrets][secret] to set up password in Kibana and fluentd.


To set up credentials (rather than 'password')?

aknuds1 · 2017-08-04T09:30:21Z

cluster/addons/fluentd-elasticsearch/README.md

-possibly choosing a more suitable StorageClass).
+
+The Elasticsearch StatefulSet will use the [EmptyDir][emptyDir] volume to
+store data. This volume will be erased with the pod evictions, it is used


"when pods terminate" maybe instead of "with the pod evictions"?

aknuds1 · 2017-08-04T09:31:32Z

cluster/addons/fluentd-elasticsearch/README.md

+store data. This volume will be erased with the pod evictions, it is used
+just for testing purposes.
+
+**Note:** please change the storage to the persistent volume claim if you


Maybe say "change the storage to persistent volume claim" instead of "change the storage to the persistent volume claim"?

aknuds1 · 2017-08-04T09:32:37Z

cluster/addons/fluentd-elasticsearch/README.md


-Learn more at: https://kubernetes.io/docs/tasks/debug-application-cluster/logging-elasticsearch-kibana
+**Note:** in order for Fluentd to work, every Kubernetes node must be labeled
+with `beta.kubernetes.io/fluentd-ds-ready=true`, as otherwise Fluentd DaemonSet


I would say "the Fluentd DaemonSet".

aknuds1 · 2017-08-04T09:38:32Z

@crassirostris Should you document perhaps how the Fluentd configuration must be changed in order to use credentials towards Elasticsearch?

crassirostris

Should you document perhaps how the Fluentd configuration must be changed in order to use credentials towards Elasticsearch

Ack, will address everything shortly

crassirostris · 2017-08-04T13:26:16Z

cluster/addons/fluentd-elasticsearch/README.md

+[X-Pack plugin][xPack]. See configuration parameter `xpack.security.enabled`
+in Elasticsearch and Kibana configurations. It can also be set via
+`XPACK_SECURITY_ENABLED` env variable. You can then use [ConfigMaps][configMap]
+and [Secrets][secret] to set up password in Kibana and fluentd.


crassirostris · 2017-08-04T13:26:28Z

cluster/addons/fluentd-elasticsearch/README.md

-possibly choosing a more suitable StorageClass).
+
+The Elasticsearch StatefulSet will use the [EmptyDir][emptyDir] volume to
+store data. This volume will be erased with the pod evictions, it is used


crassirostris · 2017-08-04T13:26:40Z

cluster/addons/fluentd-elasticsearch/README.md

+store data. This volume will be erased with the pod evictions, it is used
+just for testing purposes.
+
+**Note:** please change the storage to the persistent volume claim if you


crassirostris · 2017-08-04T13:27:01Z

cluster/addons/fluentd-elasticsearch/README.md


-Learn more at: https://kubernetes.io/docs/tasks/debug-application-cluster/logging-elasticsearch-kibana
+**Note:** in order for Fluentd to work, every Kubernetes node must be labeled
+with `beta.kubernetes.io/fluentd-ds-ready=true`, as otherwise Fluentd DaemonSet


crassirostris · 2017-08-04T13:40:16Z

@aknuds1 Done, PTAL

aknuds1 · 2017-08-04T14:04:25Z

@crassirostris On it

aknuds1 · 2017-08-04T14:10:11Z

cluster/addons/fluentd-elasticsearch/README.md


+Since Fluentd talks to Elastcisearch service inside the cluster, instance on


I think it's better to word it "Since Fluentd talks to the Elasticsearch service inside the cluster, instances on masters won't work, because masters have no kube-proxy. Add a taint on your masters to avoid Fluentd pod scheduling there".

aknuds1 · 2017-08-04T14:11:23Z

cluster/addons/fluentd-elasticsearch/README.md


+Since Fluentd talks to Elastcisearch service inside the cluster, instance on
+the master won't work, because there's no kube-proxy on master. Add taint on
+your master to avoid Fluentd pod scheduling there.


Is it really necessary to taint masters, since the label beta.kubernetes.io/fluentd-ds-ready=true is already used to control the nodes on which Fluentd is scheduled?

Makes sense actually. It's the problem of GCE setup in this repo, not a generic one. I'll rephrase

aknuds1 · 2017-08-04T14:13:36Z

cluster/addons/fluentd-elasticsearch/fluentd-es-ds.yaml

 apiVersion: extensions/v1beta1
 kind: DaemonSet
 metadata:
-  name: fluentd-es-v1.24
+  name: fluentd-es-v2.0.0


Is there any point in appending the version to the name? We stopped doing this for Elasticsearch itself after all.

I guess there is none, thanks for noticing

crassirostris

Thanks a lot for your thorough review!

crassirostris · 2017-08-04T14:35:24Z

cluster/addons/fluentd-elasticsearch/fluentd-es-ds.yaml

 apiVersion: extensions/v1beta1
 kind: DaemonSet
 metadata:
-  name: fluentd-es-v1.24
+  name: fluentd-es-v2.0.0


I guess there is none, thanks for noticing

crassirostris · 2017-08-04T14:36:22Z

cluster/addons/fluentd-elasticsearch/README.md


+Since Fluentd talks to Elastcisearch service inside the cluster, instance on


crassirostris · 2017-08-04T14:36:31Z

cluster/addons/fluentd-elasticsearch/README.md


+Since Fluentd talks to Elastcisearch service inside the cluster, instance on
+the master won't work, because there's no kube-proxy on master. Add taint on
+your master to avoid Fluentd pod scheduling there.


Makes sense actually. It's the problem of GCE setup in this repo, not a generic one. I'll rephrase

coffeepac · 2017-08-04T16:47:39Z

@aknuds1 this looks good to me, but I'll wait for your approval before I put a lgtm label on it

aknuds1 · 2017-08-04T17:52:34Z

cluster/addons/fluentd-elasticsearch/README.md

-possibly choosing a more suitable StorageClass).
+
+The Elasticsearch StatefulSet will use the [EmptyDir][emptyDir] volume to
+store data. This volume will be erased when the pod terminates, it is used


I think this sounds more correct, as there will be several pods: "when the pods terminate".

I wanted to say that an EmptyDir volume is erased if the pod it belongs to is restarted

@crassirostris Aha, when reading through again it makes sense in context.

aknuds1 · 2017-08-04T17:53:09Z

cluster/addons/fluentd-elasticsearch/README.md

+just for testing purposes.
+
+**Note:** please change the storage to persistent volume claim if you want
+to use this deployment in your setup!


It's not a deployment though, maybe call it 'StatefulSet'?

aknuds1 · 2017-08-04T17:53:41Z

cluster/addons/fluentd-elasticsearch/README.md

+Since Fluentd talks to the Elasticsearch service inside the cluster, instances
+on masters won't work, because masters have no kube-proxy. Don't mark masters
+with a label mentioned in the previous paragraph or add a taint on them to
+avoid Fluentd pods scheduling there.


aknuds1 · 2017-08-04T17:55:11Z

cluster/addons/fluentd-elasticsearch/README.md

+[configMap]: https://kubernetes.io/docs/tasks/configure-pod-container/configmap/
+[secret]: https://kubernetes.io/docs/concepts/configuration/secret/
+[statefulSet]: https://kubernetes.io/docs/concepts/workloads/controllers/statefulset
+[initContianer]: https://kubernetes.io/docs/concepts/workloads/pods/init-containers/


initContainer, not initContianer

Wow, thanks for catching this!

…n to ConfigMap

crassirostris · 2017-08-04T18:09:28Z

@aknuds1 Thanks! Rephrased Storage paragraph little bit

crassirostris · 2017-08-04T21:20:52Z

/retest

coffeepac · 2017-08-05T00:00:59Z

/lgtm

k8s-github-robot · 2017-08-05T01:34:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: coffeepac, crassirostris

Associated issue: 41462

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

Needs approval from an approver in each of these OWNERS Files:

~~cluster/addons/fluentd-elasticsearch/OWNERS~~ [coffeepac,crassirostris]
~~cluster/addons/fluentd-gcp/OWNERS~~ [crassirostris]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-08-05T12:14:02Z

Automatic merge from submit-queue (batch tested with PRs 48532, 50054, 50082)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 3, 2017

k8s-github-robot assigned coffeepac and piosz Aug 3, 2017

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Aug 3, 2017

crassirostris force-pushed the cleanup-fluentd-es branch 2 times, most recently from cd11fb7 to 1f32693 Compare August 3, 2017 12:50

crassirostris changed the title ~~WIP: Refactored the fluentd-es addon~~ Refactored the fluentd-es addon Aug 3, 2017

crassirostris added the sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. label Aug 3, 2017

crassirostris requested a review from coffeepac August 3, 2017 12:55

k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Aug 3, 2017

crassirostris force-pushed the cleanup-fluentd-es branch 2 times, most recently from 2568ce9 to b1f39fa Compare August 3, 2017 13:36

aknuds1 suggested changes Aug 3, 2017

View reviewed changes

crassirostris force-pushed the cleanup-fluentd-es branch 3 times, most recently from d327e29 to 68da746 Compare August 3, 2017 15:18

coffeepac reviewed Aug 4, 2017

View reviewed changes

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 4, 2017

crassirostris force-pushed the cleanup-fluentd-es branch 2 times, most recently from 5854152 to 8dd80fa Compare August 4, 2017 09:34

aknuds1 suggested changes Aug 4, 2017

View reviewed changes

crassirostris commented Aug 4, 2017

View reviewed changes

crassirostris force-pushed the cleanup-fluentd-es branch from 8dd80fa to 17e25e9 Compare August 4, 2017 13:40

aknuds1 suggested changes Aug 4, 2017

View reviewed changes

crassirostris commented Aug 4, 2017

View reviewed changes

crassirostris force-pushed the cleanup-fluentd-es branch from 17e25e9 to cf997af Compare August 4, 2017 14:41

coffeepac approved these changes Aug 4, 2017

View reviewed changes

aknuds1 suggested changes Aug 4, 2017

View reviewed changes

Refactored the fluentd-es addon files, moved the fluentd configuratio…

46f53da

…n to ConfigMap

crassirostris force-pushed the cleanup-fluentd-es branch from cf997af to 46f53da Compare August 4, 2017 18:05

aknuds1 approved these changes Aug 4, 2017

View reviewed changes

kubernetes deleted a comment from k8s-github-robot Aug 4, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 5, 2017

k8s-github-robot merged commit 70b4db2 into kubernetes:master Aug 5, 2017

piosz removed their assignment Aug 9, 2017


		[fluentd]: http://www.fluentd.org/
		[elasticsearch]: http://www.elasticsearch.org/


		Since Fluentd talks to Elastcisearch service inside the cluster, instance on

Refactored the fluentd-es addon #50082

Refactored the fluentd-es addon #50082

Conversation

crassirostris commented Aug 3, 2017 • edited Loading

aknuds1 commented Aug 3, 2017

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

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

crassirostris commented Aug 3, 2017

crassirostris commented Aug 3, 2017

crassirostris commented Aug 3, 2017

crassirostris commented Aug 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coffeepac commented Aug 4, 2017

crassirostris commented Aug 4, 2017

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

aknuds1 commented Aug 4, 2017

crassirostris left a 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

crassirostris commented Aug 4, 2017

aknuds1 commented Aug 4, 2017

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

crassirostris left a 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

coffeepac commented Aug 4, 2017

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

crassirostris commented Aug 4, 2017

crassirostris commented Aug 4, 2017

coffeepac commented Aug 5, 2017

k8s-github-robot commented Aug 5, 2017

k8s-github-robot commented Aug 5, 2017

crassirostris commented Aug 3, 2017 •

edited

Loading