Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

[stable/elasticsearch] support elasticsearch 7 & graceful termination on master nodes #13290

Closed
wants to merge 11 commits into from
Closed

[stable/elasticsearch] support elasticsearch 7 & graceful termination on master nodes #13290

wants to merge 11 commits into from

Conversation

kimxogus
Copy link
Contributor

@kimxogus kimxogus commented Apr 25, 2019 •
edited
Loading

What this PR does / why we need it:

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

Special notes for your reviewer:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • DCO signed
  • Chart Version bumped
  • Variables are documented in the README.md

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kimxogus
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: desaintmartin

If they are not already assigned, you can assign the PR to them by writing /assign @desaintmartin in a comment when ready.

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@helm-bot helm-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 25, 2019
@k8s-ci-robot
Copy link
Contributor

Hi @kimxogus. Thanks for your PR.

I'm waiting for a helm 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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 25, 2019
@kimxogus kimxogus force-pushed the stable/elasticsearch/support-es7 branch from 5e21186 to d959d7b Compare April 25, 2019 16:31
@helm-bot helm-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Apr 25, 2019
@kimxogus
Copy link
Contributor Author

/assign @desaintmartin

@kimxogus
Copy link
Contributor Author

I didn't bump es version because es7 doesn't support rolling upgrade.

@helm-bot helm-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). labels Apr 26, 2019
desaintmartin
desaintmartin reviewed Apr 26, 2019
View reviewed changes
Copy link
Collaborator

@desaintmartin desaintmartin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
/ok-to-test

stable/elasticsearch/templates/configmap.yaml Outdated Show resolved Hide resolved
stable/elasticsearch/templates/configmap.yaml Outdated Show resolved Hide resolved
stable/elasticsearch/templates/configmap.yaml
{{- end }}
{{- end }}
{{- else if hasPrefix "7." .Values.appVersion }}
# see https://github.com/kubernetes/kubernetes/issues/3595
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this specific to ES7?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, I have no idea about this issue and es7, but es nodes fail with bootstrap.memory_lock: true option.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should check this and find the source before blindly merge it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that 6.7 doc and 7.0 doc are same and nothing has changed about this issue.

and reference in elastic's own chart elastic/helm-charts#7

stable/elasticsearch/templates/configmap.yaml Outdated Show resolved Hide resolved
stable/elasticsearch/templates/configmap.yaml Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 26, 2019
@desaintmartin
Copy link
Collaborator

You'll also need to sign your commits!

@kimxogus kimxogus force-pushed the stable/elasticsearch/support-es7 branch from 38aa70c to 3075df0 Compare April 27, 2019 04:33
@helm-bot helm-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Apr 27, 2019
@kimxogus
Copy link
Contributor Author

signed all commits

@helm-bot helm-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 29, 2019
@kimxogus
Copy link
Contributor Author

@desaintmartin Refactored configmap.yaml

@kimxogus kimxogus force-pushed the stable/elasticsearch/support-es7 branch from 8e553e0 to 960bb0b Compare April 30, 2019 01:04
@kimxogus
support elasticsearch 7
5f04115 
Signed-off-by: Taehyun Kim <kgyoo8232@gmail.com>
@kimxogus
fix script
3b1a2c2 
Signed-off-by: Taehyun Kim <kgyoo8232@gmail.com>
@kimxogus
fix http function following chart
c022594 
Signed-off-by: Taehyun Kim <kgyoo8232@gmail.com>
@kimxogus
remove log4j2.properties for es7
e442f63 
Signed-off-by: Taehyun Kim <kgyoo8232@gmail.com>
@kimxogus
minor updates
5064328 
Signed-off-by: Taehyun Kim <kgyoo8232@gmail.com>
@kimxogus
refactor configmap
2100d14 
Signed-off-by: Taehyun Kim <kgyoo8232@gmail.com>
@kimxogus
use sidecar for es graceful termination
b9398d4 
Signed-off-by: Taehyun Kim <kgyoo8232@gmail.com>
@kimxogus kimxogus force-pushed the stable/elasticsearch/support-es7 branch from 960bb0b to b9398d4 Compare May 2, 2019 03:45
@kimxogus kimxogus changed the title [stable/elasticsearch] support elasticsearch 7 [stable/elasticsearch] support elasticsearch 7 & graceful termination on master nodes May 2, 2019
@kimxogus
Copy link
Contributor Author

kimxogus commented May 2, 2019 •
edited
Loading

@desaintmartin rewrote master termination handling process. This can be applied to all es versions

@kimxogus
rename container name
e780727 
Signed-off-by: Taehyun Kim <kgyoo8232@gmail.com>
@kimxogus
enhance sidecar
a759a22 
Signed-off-by: Taehyun Kim <kgyoo8232@gmail.com>
@kimxogus
Copy link
Contributor Author

kimxogus commented May 3, 2019

/test pull-charts-e2e

@kimxogus kimxogus force-pushed the stable/elasticsearch/support-es7 branch from a6871d6 to 6ef8721 Compare May 6, 2019 08:44
@stale
Copy link

stale bot commented Jun 8, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

@stale stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 8, 2019
@stale
Copy link

stale bot commented Jun 22, 2019

This issue is being automatically closed due to inactivity.

@stale stale bot closed this Jun 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@desaintmartin desaintmartin desaintmartin left review comments

@icereval icereval Awaiting requested review from icereval

Assignees

@desaintmartin desaintmartin

Labels
Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. ok-to-test size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[stable/elasticsearch] Terminating current master pod causes cluster outage of more than 30 seconds
4 participants
@kimxogus @k8s-ci-robot @desaintmartin @helm-bot