extracted elasticsearch-logging service name as environment variable #54215

mrahbar · 2017-10-19T09:02:25Z

What this PR does / why we need it:
Deploying the cluster-addon fluentd-elasticsearch with customized resource definitions can cause elasticsearch discovery to fail because the service name elasticsearch-logging is hard-coded in cluster/addons/fluentd-elasticsearch/es-image/elasticsearch_logging_discovery.go

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
-> none yet

Special notes for your reviewer:
The name of the environment variable is ELASTICSEARCH_SERVICE_NAME. When non is given the fallback service-name fallback is elasticsearch-logging

[fluentd-elasticsearch addon] Elasticsearch service name can be overridden via env variable ELASTICSEARCH_SERVICE_NAME

k8s-ci-robot · 2017-10-19T09:02:40Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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.

crassirostris · 2017-10-19T09:42:32Z

SGTM

Have you built a test image and tried it in a cluster?

mrahbar · 2017-10-19T09:51:53Z

@crassirostris I'm currently on it but I can't notice any drawbacks

crassirostris · 2017-10-19T09:57:36Z

Sure, I'm not talking about any drawbacks, I just want to verify that everything builds and runs as expected :)

/ok-to-test

mrahbar · 2017-10-19T10:14:08Z

I build it and it works as expected :)
I also signed the cla.

crassirostris · 2017-10-19T10:50:33Z

/retest

mrahbar · 2017-10-20T07:18:22Z

Any updates on this PR? The cause of the failing tests seems unrelated.

crassirostris · 2017-10-20T10:49:00Z

I'll take a look by the EOW

crassirostris · 2017-10-23T10:16:07Z

@mrahbar OK, seems fine. Could you please do the following?

update the image version in the Makefile
write the release note (include [fluentd-elasticsearch plugin] there)

mrahbar · 2017-10-23T10:26:21Z

I change the image version on Makefile from v5.6.2 to v5.6.3
Where do I supposed to write the release notes?

crassirostris · 2017-10-23T10:56:56Z

Sorry, forgot about it. This version we try to correlate with the version of the Elasticsearch, e.g. currently the version of Elasticsearch and of the image is 5.6.2. Could you please replace 5.6.3 with 5.6.2-1?

Where do I supposed to write the release notes?

In the PR description, in the following format:

```release-note
[fluentd-elasticsearch addon] ...
```

mrahbar · 2017-11-03T21:54:46Z

Any updates on this?
@crassirostris do you know why the summary says "commit missing GitHub user"? I added my commit email address to GitHub

crassirostris · 2017-11-09T00:07:40Z

@mrahbar Sorry, it slipped past me

Regarding commit missing GitHub user, two out of three your commits are marked as not associated with your GitHub account. Maybe you pushed them from a different computer, with a different email setup in git config?

Overall SGTM, last comments:

Make release note more crisp, something like Elasticsearch service name now can be configured via env variable
Squash commits into one
Use the new Elasticsearch version in the DaemonSet definition or make a follow-up PR to do this

mrahbar · 2017-11-09T11:54:15Z

I changed the release note as you proposed
I squashed the commits and hopefully added all my email adresses to my account for cla to work
Regarding: 'Use the new Elasticsearch version in the DaemonSet ' I would prefer creating a new PR because it is unrelated to this PR

crassirostris · 2017-11-09T12:07:43Z

@mrahbar Thanks!

I changed the release note as you proposed

My main point was to leave out the specific details. Release notes consist of hundreds of messages, technical details only makes them harder to read. If somebody wants the details, this person will open the PR or docs

I would prefer creating a new PR because it is unrelated to this PR

Sounds good. Please assign this PR to me

mrahbar · 2017-11-09T12:37:16Z

@crassirostris I created a new PR #55400 for version update of elasticsearch and kibana but I can't assign it to you.

mrahbar · 2017-11-09T12:55:15Z

@crassirostris I'm fine with this PR and the release notes. :)

crassirostris · 2017-11-09T13:22:39Z

@mrahbar

Regarding changing the image version. I would really like to avoid creating image releases that are not used, it's a manual process for now

Regarding the notes. I suggest to hear third opinion. @coffeepac could you please take a look?

coffeepac · 2017-11-09T17:31:42Z

terse is better for release notes. I suggest:

[fluentd-elasticsearch addon] Elasticsearch service name can be overridden via env variable ELASTICSEARCH_SERVICE_NAME

That okay @mrahbar ?

Also, thanks for the PR!

crassirostris · 2017-11-09T17:45:42Z

That also LGTM

mrahbar · 2017-11-09T18:18:42Z

Release note also LGTM. Changed it accordingly.

crassirostris · 2017-11-09T18:20:02Z

What about image version?

mrahbar · 2017-11-09T18:34:21Z

I guess I miss understood you regarding the image version. Resulting in the PR #55400 thus I closed it. I thought you wanted to use this product to also update to the newest es version. But I guess you just wanted the es image in the daemon-set to be v5.6.2-1.
I will push the changes shortly.

crassirostris · 2017-11-09T18:37:47Z

No-no, sorry for misunderstanding. I suggest to leave the version in this PR unchanged, because #55400 will change it anyway, why change twice?

mrahbar · 2017-11-09T19:28:45Z

@crassirostris So then this PR has come to an end. Do you agree. The version update of es will happen in #55400. Any last thinks to do here?

crassirostris · 2017-11-09T19:54:25Z

@mrahbar No :) I suggest to change the code in this PR and then change the version in another PR. Does it sound reasonable?

mrahbar · 2017-11-09T20:44:07Z

@crassirostris can this PR be merge now?

crassirostris · 2017-11-09T23:33:27Z

@mrahbar It still includes the Makefile change. Revert this one line (v5.6.2 -> v5.6.2-1) in this PR and I'll lgtm both of them

mrahbar · 2017-11-10T08:41:28Z

@crassirostris I reverted to line in Makefile. I'm currently on my way and can't squash the commits. Is this ok?

crassirostris · 2017-11-10T12:19:29Z

@mrahbar Yes, perfect, thanks! LGTM, I'll apply the label right after you squash commits

…ELASTICSEARCH_SERVICE_NAME with fallback on default

mrahbar · 2017-11-10T13:36:53Z

@crassirostris Squashed them 👍

crassirostris · 2017-11-10T13:39:59Z

/lgtm

coffeepac · 2017-11-10T17:47:14Z

/approve no-issue

k8s-github-robot · 2017-11-10T17:48:06Z

[APPROVALNOTIFIER] This PR is APPROVED

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

Associated issue requirement bypassed by: coffeepac

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]

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

crassirostris · 2017-11-10T17:49:14Z

@coffeepac Thanks!

k8s-github-robot · 2017-11-10T22:51:31Z

Automatic merge from submit-queue (batch tested with PRs 54987, 55221, 54099, 55144, 54215). If you want to cherry-pick this change to another branch, please follow the instructions here.

@crassirostris

Automatic merge from submit-queue (batch tested with PRs 55998, 55400). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Update of elasticsearch kibana version **What this PR does / why we need it**: Updated elasticsearch and kibana version to version 5.6.4 This was motivated by @crassirostris in #54215 (comment) **Release note**: ```release-note [fluentd-elasticsearch addon] Elasticsearch and Kibana are updated to version 5.6.4 ```

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Oct 19, 2017

k8s-github-robot assigned crassirostris and piosz Oct 19, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 19, 2017

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 23, 2017

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 23, 2017

mrahbar force-pushed the elasticsearch_logging_discovery branch from 83d75d5 to 5f7a8d3 Compare November 9, 2017 11:50

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Nov 9, 2017

mrahbar closed this Nov 9, 2017

mrahbar reopened this Nov 9, 2017

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 10, 2017

extracted elasticsearch-logging service name as environment variable …

4ecd54f

…ELASTICSEARCH_SERVICE_NAME with fallback on default

mrahbar force-pushed the elasticsearch_logging_discovery branch from 3c84667 to 4ecd54f Compare November 10, 2017 13:14

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

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

k8s-github-robot merged commit dad41f8 into kubernetes:master Nov 10, 2017

extracted elasticsearch-logging service name as environment variable #54215

extracted elasticsearch-logging service name as environment variable #54215

Conversation

mrahbar commented Oct 19, 2017 • edited

k8s-ci-robot commented Oct 19, 2017

crassirostris commented Oct 19, 2017

mrahbar commented Oct 19, 2017

crassirostris commented Oct 19, 2017

mrahbar commented Oct 19, 2017

crassirostris commented Oct 19, 2017

mrahbar commented Oct 20, 2017

crassirostris commented Oct 20, 2017

crassirostris commented Oct 23, 2017

mrahbar commented Oct 23, 2017

crassirostris commented Oct 23, 2017

mrahbar commented Nov 3, 2017

crassirostris commented Nov 9, 2017

mrahbar commented Nov 9, 2017

crassirostris commented Nov 9, 2017

mrahbar commented Nov 9, 2017

mrahbar commented Nov 9, 2017

crassirostris commented Nov 9, 2017

coffeepac commented Nov 9, 2017

crassirostris commented Nov 9, 2017

mrahbar commented Nov 9, 2017

crassirostris commented Nov 9, 2017

mrahbar commented Nov 9, 2017

crassirostris commented Nov 9, 2017 • edited

mrahbar commented Nov 9, 2017 • edited

crassirostris commented Nov 9, 2017

mrahbar commented Nov 9, 2017

crassirostris commented Nov 9, 2017 • edited

mrahbar commented Nov 10, 2017

crassirostris commented Nov 10, 2017

mrahbar commented Nov 10, 2017

crassirostris commented Nov 10, 2017

coffeepac commented Nov 10, 2017

k8s-github-robot commented Nov 10, 2017

crassirostris commented Nov 10, 2017

k8s-github-robot commented Nov 10, 2017

mrahbar commented Oct 19, 2017 •

edited

crassirostris commented Nov 9, 2017 •

edited

mrahbar commented Nov 9, 2017 •

edited

crassirostris commented Nov 9, 2017 •

edited