Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

@aknuds1
Copy link
Contributor

@aknuds1 aknuds1 commented Jul 10, 2017

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
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Jul 10, 2017

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
Copy link
Member

@coffeepac coffeepac commented Jul 11, 2017

@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 aknuds1 force-pushed the aknuds1:upgrade-fluentd-elasticsearch branch 2 times, most recently from 07eb090 to b8a9c0b Jul 11, 2017
accessModes: ["ReadWriteOnce"]
resources:
requests:
storage: 20Gi

This comment has been minimized.

@aknuds1

aknuds1 Jul 11, 2017
Author Contributor

Should this be configurable?

This comment has been minimized.

@coffeepac

coffeepac Jul 13, 2017
Member

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

This comment has been minimized.

@aknuds1

aknuds1 Jul 13, 2017
Author Contributor

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

chown -R elasticsearch:elasticsearch /data

exec gosu elasticsearch sh /elasticsearch/bin/elasticsearch
sysctl -w vm.max_map_count=262144

This comment has been minimized.

@aknuds1

aknuds1 Jul 11, 2017
Author Contributor

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

This comment has been minimized.

@coffeepac

coffeepac Jul 13, 2017
Member

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.

This comment has been minimized.

@aknuds1

aknuds1 Jul 13, 2017
Author Contributor

@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?

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

This comment has been minimized.

@aknuds1

aknuds1 Jul 11, 2017
Author Contributor

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

This comment has been minimized.

@coffeepac

coffeepac Jul 13, 2017
Member

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

This comment has been minimized.

@aknuds1

aknuds1 Jul 13, 2017
Author Contributor

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

@aknuds1
Copy link
Contributor Author

@aknuds1 aknuds1 commented Jul 11, 2017

@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.

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 && \

This comment has been minimized.

@aknuds1

aknuds1 Jul 11, 2017
Author Contributor

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

This comment has been minimized.

@coffeepac

coffeepac Jul 13, 2017
Member

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

This comment has been minimized.

@aknuds1

aknuds1 Jul 13, 2017
Author Contributor

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

This comment has been minimized.

@coffeepac

coffeepac Jul 13, 2017
Member

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

@aknuds1 aknuds1 force-pushed the aknuds1:upgrade-fluentd-elasticsearch branch from b8a9c0b to a6ee0f9 Jul 11, 2017
@crassirostris crassirostris removed their assignment Jul 11, 2017
# 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 comment has been minimized.

@coffeepac

coffeepac Jul 13, 2017
Member

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

@@ -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

This comment has been minimized.

@coffeepac

coffeepac Jul 13, 2017
Member

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

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

This comment has been minimized.

@coffeepac

coffeepac Jul 13, 2017
Member

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).

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

This comment has been minimized.

@coffeepac

coffeepac Jul 13, 2017
Member

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
Copy link
Member

@coffeepac coffeepac commented Jul 13, 2017

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.

@aknuds1 aknuds1 force-pushed the aknuds1:upgrade-fluentd-elasticsearch branch from a6ee0f9 to 8e499e4 Jul 13, 2017
@aknuds1 aknuds1 force-pushed the aknuds1:upgrade-fluentd-elasticsearch branch 2 times, most recently from 5cec436 to b5e71b2 Jul 13, 2017
@coffeepac
Copy link
Member

@coffeepac coffeepac commented Jul 13, 2017

/ok-to-test

@aknuds1 aknuds1 force-pushed the aknuds1:upgrade-fluentd-elasticsearch branch from 68d9dd2 to 126d5d8 Aug 2, 2017
@aknuds1 aknuds1 force-pushed the aknuds1:upgrade-fluentd-elasticsearch branch from 126d5d8 to 0ed0f02 Aug 2, 2017
@aknuds1
Copy link
Contributor Author

@aknuds1 aknuds1 commented Aug 2, 2017

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

@aknuds1 aknuds1 changed the title Upgrade fluentd-elasticsearch addon to Elasticearch 5.5 Upgrade fluentd-elasticsearch addon to Elasticsearch/Kibana 5.5 Aug 2, 2017
@crassirostris
Copy link

@crassirostris crassirostris commented Aug 2, 2017

/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
Copy link

@crassirostris crassirostris commented Aug 2, 2017

/approve no-issue

@k8s-github-robot
Copy link
Contributor

@k8s-github-robot k8s-github-robot commented Aug 2, 2017

[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:

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

@aknuds1
Copy link
Contributor Author

@aknuds1 aknuds1 commented Aug 2, 2017

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

@crassirostris
Copy link

@crassirostris crassirostris commented Aug 2, 2017

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

@fejta-bot
Copy link

@fejta-bot fejta-bot commented Aug 2, 2017

/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
Copy link
Contributor

@k8s-github-robot k8s-github-robot commented Aug 3, 2017

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

@k8s-github-robot k8s-github-robot merged commit ae0ca36 into kubernetes:master Aug 3, 2017
11 checks passed
11 checks passed
Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation aknuds1 authorized
Details
pull-kubernetes-bazel Job succeeded.
Details
pull-kubernetes-cross Jenkins job succeeded.
Details
pull-kubernetes-e2e-gce-etcd3 Jenkins job succeeded.
Details
pull-kubernetes-e2e-kops-aws Jenkins job succeeded.
Details
pull-kubernetes-federation-e2e-gce Jenkins job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Jenkins job succeeded.
Details
pull-kubernetes-node-e2e Jenkins job succeeded.
Details
pull-kubernetes-unit Jenkins job succeeded.
Details
pull-kubernetes-verify Jenkins job succeeded.
Details
@sebglon
Copy link

@sebglon sebglon commented Aug 3, 2017

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
Copy link

@crassirostris crassirostris commented Aug 3, 2017

@sebglon Was not pushed, sorry, Just fixed it

@aknuds1 aknuds1 deleted the aknuds1:upgrade-fluentd-elasticsearch branch Aug 3, 2017
@aknuds1
Copy link
Contributor Author

@aknuds1 aknuds1 commented Aug 3, 2017

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

@crassirostris
Copy link

@crassirostris crassirostris commented Aug 3, 2017

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

@aknuds1
Copy link
Contributor Author

@aknuds1 aknuds1 commented Aug 3, 2017

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

@crassirostris
Copy link

@crassirostris crassirostris commented Aug 3, 2017

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

@aknuds1
Copy link
Contributor Author

@aknuds1 aknuds1 commented Aug 3, 2017

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

@crassirostris
Copy link

@crassirostris crassirostris commented Aug 3, 2017

@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
Copy link
Contributor Author

@aknuds1 aknuds1 commented Aug 3, 2017

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

@crassirostris
Copy link

@crassirostris crassirostris commented Aug 3, 2017

@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
Copy link
Contributor Author

@aknuds1 aknuds1 commented Aug 3, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.