Upgrade fluentd-elasticsearch addon to Elasticsearch/Kibana 5.5 #48722

aknuds1 · 2017-07-10T18:18:10Z

This is a patch to upgrade the fluentd-elasticsearch addon to Elasticsearch/Kibana 5.5. Please provide feedback!

* Upgrade Elasticsearch/Kibana to 5.5.1 in fluentd-elasticsearch addon
* Switch to basing our image of Elasticsearch in fluentd-elasticsearch addon off the official one
* Switch to the official image of Kibana in fluentd-elasticsearch addon
* Use StatefulSet for Elasticsearch instead of ReplicationController, with persistent volume claims
* Require authenticating towards Elasticsearch, as Elasticsearch 5.5 by default requires basic authentication

k8s-ci-robot · 2017-07-10T18:18:17Z

Hi @aknuds1. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /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 by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

coffeepac · 2017-07-11T15:58:27Z

@Aknuds thanks for the PR! Do you want any feedback on this now? There are a few things that will prevent me from kicking off tests.

aknuds1 · 2017-07-11T11:34:55Z

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

+      accessModes: ["ReadWriteOnce"]
+      resources:
+        requests:
+          storage: 20Gi


Should this be configurable?

yes, 20Gi is far too low for a production use case.

@coffeepac How should we make it configurable in your opinion?

aknuds1 · 2017-07-11T18:40:04Z

cluster/addons/fluentd-elasticsearch/es-image/run.sh

-chown -R elasticsearch:elasticsearch /data
-
-exec gosu elasticsearch sh /elasticsearch/bin/elasticsearch
+sysctl -w vm.max_map_count=262144


This seems to cause a warning about the filesystem being read only. Is it even needed?

I'm not sure if its required for the es5 image, it may already be set. however, ES5 has a strictly enforced minimum number of memory maps that must be available or ES refuses to start.

@coffeepac The official ES5 image does not call sysctl, nor does it seem to set vm.mx_map_count in any other way. I guess it's unnecessary with 5.5?

I believe this is still enforced by ES 5.5: https://www.elastic.co/guide/en/elasticsearch/reference/5.x/_maximum_map_count_check.html

there is another workaround but I don't fully understand it:
https://www.elastic.co/guide/en/elasticsearch/reference/5.x/breaking-changes-5.5.html#breaking_55_packaging_changes

aknuds1 · 2017-07-11T18:41:06Z

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

        command:
          - '/bin/sh'
          - '-c'
          - '/usr/sbin/td-agent $FLUENTD_ARGS'
        env:
        - name: FLUENTD_AGRS
          value: -q
+        # TODO: Make into secret!!


What do we do about these Elasticsearch credentials? Should we make them configurable via a Makefile?

what are these used for? is this basic auth against the ES cluster?

@coffeepac That is correct, since authentication is enabled by default for ES 5.5.

aknuds1 · 2017-07-11T18:43:23Z

@coffeepac I would love any feedback at this point! I'm really especially interested in hearing about my approach when it comes to the Elasticsearch Dockerfile. I've justed based it off the original one, instead of writing it from scratch, as was done in the original.

I'm also interested in hearing about how installation of Elasticsearch plugins should be made configurable, instead of hardwiring repository-s3 like I've done.

Note also please that I include a binary of elasticsearch_logging_discovery instead of the corresponding source code, as I could not successfully build this neither on OS X nor Linux.

aknuds1 · 2017-07-11T18:43:59Z

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

+USER elasticsearch
+# Temporarily move config out of the way before installing plugin as installation will fail
+# otherwise
+RUN mv config config.bak && ./bin/elasticsearch-plugin install -b repository-s3 && \


I suspect that which plugins to install, repository-s3 in this case, should be configurable in some way.

yes and I'm not sure how it could be done.

@coffeepac I'm sure there could be a way. I might have seen it being done in another repository in fact.

if you have seen it I would love to get that in here. there are several things that should be parameterized.

coffeepac · 2017-07-13T03:58:42Z

cluster/addons/fluentd-elasticsearch/es-image/run.sh

-# distributed under the License is distributed on an "AS IS" BASIS,
-# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-# See the License for the specific language governing permissions and
-# limitations under the License.


this copyright information should remain and should be at the top of all files

coffeepac · 2017-07-13T04:05:12Z

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

@@ -22,7 +24,7 @@ spec:
    spec:
      serviceAccountName: elasticsearch-logging
      containers:
-      - image: gcr.io/google_containers/elasticsearch:v2.4.1-2
+      - image: gcr.io/google_containers/elasticsearch-k8s-s3:5.5.0-11


having s3 included in the default is a bit tough as mentioned above. Its not a useful plugin for GKE clusters.

coffeepac · 2017-07-13T04:06:29Z

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

+  - metadata:
+      name: elasticsearch-logging
+    spec:
+      storageClassName: gp2


this isn't portable, its AWS specific. allow this to be the cluster default (which I think is an empty string? the semantics of storage class default is very particular).

coffeepac · 2017-07-13T04:07:56Z

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

  namespace: kube-system
  labels:
    k8s-app: fluentd-es
    kubernetes.io/cluster-service: "true"
    addonmanager.kubernetes.io/mode: Reconcile
-    version: v1.22
+    version: v1.23


I think the current version is 24? probably shouldn't change it yet as there may be other PRs that bump the version but something to check when closer to merge.

coffeepac · 2017-07-13T04:18:00Z

I don't have any ideas for how to provide a list of plugins to be installed at install time. @crassirostris do you have any thoughts on that? its something I've been considering but I don't know if this is something that there is already a pattern for elsewhere in kubernetes.

coffeepac · 2017-07-13T20:34:12Z

/ok-to-test

crassirostris · 2017-08-02T17:34:14Z

cluster/addons/fluentd-elasticsearch/README.md

+[init container](https://kubernetes.io/docs/concepts/workloads/pods/init-containers/) executing
+before Elasticsearch containers themselves, in order to ensure that the kernel state variable
+`vm.max_map_count` is at least 262144, since this is a requirement of Elasticsearch.
+You may remove the init container if you know that your host filesystem meets this requirement.


It's not about filesystem, but OS

@crassirostris Ah, thank you! My bad.

aknuds1 · 2017-08-02T17:44:04Z

@crassirostris Both nits handled. Please review my release notes as I added a couple.

crassirostris · 2017-08-02T17:49:59Z

/lgtm

Thanks a lot for doing the work! One nit towards the release notes:

Require authenticating towards Elasticsearch, as is the default

I don't understand what it means :) Last part at least, "as is the default". Could you please rephrase it?

crassirostris · 2017-08-02T17:50:27Z

/approve no-issue

k8s-github-robot · 2017-08-02T17:50:31Z

[APPROVALNOTIFIER] This PR is APPROVED

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

Associated issue requirement bypassed by: crassirostris

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~~ [crassirostris]

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

aknuds1 · 2017-08-02T17:58:34Z

@crassirostris I revised my release notes, trying to make it more clear what I mean about requiring Elasticsearch authentication.

crassirostris · 2017-08-02T18:07:18Z

@aknuds1 SGTM, thanks again! Sorry in advance for those test flakes =__=

fejta-bot · 2017-08-02T21:25:28Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

k8s-github-robot · 2017-08-03T05:32:58Z

Automatic merge from submit-queue (batch tested with PRs 48365, 49902, 49808, 48722, 47045)

sebglon · 2017-08-03T07:42:35Z

Image are not pushed on not granted for public?
Failed to pull image "gcr.io/google_containers/fluentd-elasticsearch:1.24": rpc error: code = 2 desc = Error: Status 405 trying to pull repository google_containers/fluentd-elasticsearch: "v1 Registry API is disabled. If you are not explicitly using the v1 Registry API, it is possible your v2 image could not be found. Verify that your image is available, or retry withdockerd --disable-legacy-registry. See https://cloud.google.com/container-registry/docs/support/deprecation-notices"

crassirostris · 2017-08-03T07:44:58Z

@sebglon Was not pushed, sorry, Just fixed it

aknuds1 · 2017-08-03T07:54:04Z

Thanks very much @crassirostris @coffeepac for helping out and getting this merged!

crassirostris · 2017-08-03T09:32:03Z

I'm rolling back the PD, because it's leaking resources in the e2e tests

aknuds1 · 2017-08-03T09:35:56Z

@crassirostris What do you mean by PD? Is there any particular issue causing resource leaks?

crassirostris · 2017-08-03T09:38:44Z

@aknuds1 "Persistent disk", sorry, GCP terminology :) I mean PVC in the elasticsearch-logging

aknuds1 · 2017-08-03T09:41:13Z

@crassirostris What's the solution then? Not having persistentVolumeClaimTemplate in the Elasticsearch StatefulSet (because of test issues)?

crassirostris · 2017-08-03T09:44:32Z

@aknuds1 Yup, and document it better. Ideally, I'm thinking about splitting fluentd to a separate repo and having test yamls in this repo and an example with PVC in another

aknuds1 · 2017-08-03T09:45:20Z

@crassirostris So the idea is to let the user define persistentVolumeClaimTemplate instead? You know best in any case.

crassirostris · 2017-08-03T09:47:43Z

@aknuds1 The idea is to replace persistentVolumeClaimTemplate with emptyDir volume in this repo and add a note to the readme, stating that when adapting this example for your own need, you have to replace it with a PVC. Sounds good?

aknuds1 · 2017-08-03T09:48:47Z

@crassirostris That's what I thought you would do. Sounds like the best solution everything taken into consideration!

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 10, 2017

k8s-github-robot assigned coffeepac and crassirostris Jul 10, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Jul 10, 2017

aknuds1 force-pushed the upgrade-fluentd-elasticsearch branch 4 times, most recently from 13f38ca to 09d6dcb Compare July 10, 2017 18:23

aknuds1 force-pushed the upgrade-fluentd-elasticsearch branch 2 times, most recently from 07eb090 to b8a9c0b Compare July 11, 2017 18:39

aknuds1 commented Jul 11, 2017

View reviewed changes

aknuds1 force-pushed the upgrade-fluentd-elasticsearch branch from b8a9c0b to a6ee0f9 Compare July 11, 2017 21:13

crassirostris removed their assignment Jul 11, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 13, 2017

coffeepac reviewed Jul 13, 2017

View reviewed changes

aknuds1 force-pushed the upgrade-fluentd-elasticsearch branch from a6ee0f9 to 8e499e4 Compare July 13, 2017 07:21

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 13, 2017

aknuds1 force-pushed the upgrade-fluentd-elasticsearch branch 2 times, most recently from 5cec436 to b5e71b2 Compare July 13, 2017 07:34

crassirostris reviewed Aug 2, 2017

View reviewed changes

aknuds1 force-pushed the upgrade-fluentd-elasticsearch branch from 68d9dd2 to 126d5d8 Compare August 2, 2017 17:39

Upgrade Elasticsearch/Kibana to 5.5.1 and use official Kibana image

0ed0f02

aknuds1 force-pushed the upgrade-fluentd-elasticsearch branch from 126d5d8 to 0ed0f02 Compare August 2, 2017 17:41

aknuds1 changed the title ~~Upgrade fluentd-elasticsearch addon to Elasticearch 5.5~~ Upgrade fluentd-elasticsearch addon to Elasticsearch/Kibana 5.5 Aug 2, 2017

k8s-ci-robot assigned crassirostris Aug 2, 2017

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

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

k8s-github-robot merged commit ae0ca36 into kubernetes:master Aug 3, 2017

sebglon mentioned this pull request Aug 3, 2017

fluentd-elasticsearch deploy error #50056

Closed

aknuds1 deleted the upgrade-fluentd-elasticsearch branch August 3, 2017 07:53

Upgrade fluentd-elasticsearch addon to Elasticsearch/Kibana 5.5 #48722

Upgrade fluentd-elasticsearch addon to Elasticsearch/Kibana 5.5 #48722

Conversation

aknuds1 commented Jul 10, 2017 • edited Loading

k8s-ci-robot commented Jul 10, 2017

coffeepac commented Jul 11, 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

aknuds1 Jul 13, 2017 • 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

aknuds1 commented Jul 11, 2017 • 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

coffeepac commented Jul 13, 2017

coffeepac commented Jul 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aknuds1 commented Aug 2, 2017

crassirostris commented Aug 2, 2017

crassirostris commented Aug 2, 2017

k8s-github-robot commented Aug 2, 2017

aknuds1 commented Aug 2, 2017

crassirostris commented Aug 2, 2017

fejta-bot commented Aug 2, 2017

k8s-github-robot commented Aug 3, 2017

sebglon commented Aug 3, 2017 • edited Loading

crassirostris commented Aug 3, 2017

aknuds1 commented Aug 3, 2017

crassirostris commented Aug 3, 2017

aknuds1 commented Aug 3, 2017 • edited Loading

crassirostris commented Aug 3, 2017

aknuds1 commented Aug 3, 2017

crassirostris commented Aug 3, 2017

aknuds1 commented Aug 3, 2017

crassirostris commented Aug 3, 2017

aknuds1 commented Aug 3, 2017

aknuds1 commented Jul 10, 2017 •

edited

Loading

aknuds1 Jul 13, 2017 •

edited

Loading

aknuds1 commented Jul 11, 2017 •

edited

Loading

sebglon commented Aug 3, 2017 •

edited

Loading

aknuds1 commented Aug 3, 2017 •

edited

Loading